September 2005 - Development - lists.drupal.org

[drupal-devel] [bug] missing vid field in the forum table
by crunchywelch 19 Sep '05

19 Sep '05

Issue status update for http://drupal.org/node/30344 Post a follow up: http://drupal.org/project/comments/add/30344 Project: Drupal Version: cvs Component: database system Category: bug reports Priority: critical Assigned to: Anonymous Reported by: crunchywelch Updated by: crunchywelch Status: patch (code needs review) Attachment: http://drupal.org/files/issues/database.mysql_6.patch (1.07 KB) The database.* files do not contain the vid field in the forum table, causing forum module to break when submitting forum topics. Also, database.pgsql contains an old field called 'shadow' in the forum table. These patches fix this problem. crunchywelch

3 6

[drupal-devel] [bug] PostgreSQL Socket Connection
by iamtheari 19 Sep '05

19 Sep '05

Issue status update for http://drupal.org/node/26836 Post a follow up: http://drupal.org/project/comments/add/26836 Project: Drupal Version: 4.6.2 Component: postgresql database Category: bug reports Priority: normal Assigned to: Anonymous Reported by: iamtheari Updated by: iamtheari Status: patch Attachment: http://drupal.org/files/issues/drupal-pgsql-socket.patch (917 bytes) Drupal does not allow for PostgreSQL connections via unix socket. I have written a patch that allows it to do so. The effect of this patch is that a hostname of 'localhost' in a pgsql:// $db_url will use Unix sockets unless a port it set in the URL. A hostname of '127.0.0.1' will use TCP/IP as before. Examples: Unix socket: pgsql://user:pass@localhost/db TCP/IP: pgsql://user:pass@localhost:4444/db TCP/IP: pgsql://user:pass@127.0.0.1/db TCP/IP: pgsql://user:pass@127.0.0.1:4444/db The patch is attached to this bug report. From a top-level Drupal directory, type 'patch -p0 < /path/to/drupal-pgsql-socket.patch' to apply it (omit the single-quotes). iamtheari

5 13

[drupal-devel] [feature] Themeable profile list patch for profile.module
by Uwe Hermann 19 Sep '05

19 Sep '05

Issue status update for http://drupal.org/node/20479 Post a follow up: http://drupal.org/project/comments/add/20479 Project: Drupal Version: cvs Component: profile.module Category: feature requests Priority: normal Assigned to: vfilby Reported by: vfilby Updated by: Uwe Hermann -Status: patch (code needs review) +Status: patch (code needs work) Patch doesn't apply any more. Uwe Hermann Previous comments: ------------------------------------------------------------------------ Thu, 14 Apr 2005 03:16:04 +0000 : vfilby Attachment: http://drupal.org/files/issues/profile-themeable-user-list.patch (2.08 KB) Here is a patch for the profile module that provides a way to theme user lists. I simply took the last part of the giant if in profile_browse, moved it to a new function called theme_profile_list and mde profile_browse call the new function. ------------------------------------------------------------------------ Thu, 30 Jun 2005 04:52:17 +0000 : moshe weitzman looka useful ... please provide unified diff format. as per http://drupal.org/handbook ------------------------------------------------------------------------ Thu, 07 Jul 2005 10:39:32 +0000 : robertDouglass It doesn't really look suitable for theming, though, due to all the database logic. To make this really a theme function, you'd have to prepare the data into some variables that you can get by calling function A, and make a theme function B that outputs the variables. Things that can't be in theme functions include SQL and user access logic. ------------------------------------------------------------------------ Thu, 04 Aug 2005 03:58:09 +0000 : ejort Attachment: http://drupal.org/files/issues/profile.module.theme_profile_list.patch (2.17 KB) Hi, This is a patch providing the same thing, but without taking the database code into the theme_profile_list function. I've had this running on one of my sites [1] for a while and found it necessary to output a table listing member contact details (note: there aren't many profile fields accessible by anonymous users in that example, but you get the idea). Unfortunately I never got around to separating it from my other profile.module modifications until now. Let me know what you think. Cheers, Eric [1] http://www.renegades.com.au/?q=members/contact-lists/all-men

1 0

[drupal-devel] [bug] blocked users still have profile link (which doesn't work)
by Uwe Hermann 19 Sep '05

19 Sep '05

Issue status update for http://drupal.org/node/30134 Post a follow up: http://drupal.org/project/comments/add/30134 Project: Drupal Version: cvs Component: user system Category: bug reports Priority: normal Assigned to: Anonymous Reported by: moshe weitzman Updated by: Uwe Hermann -Status: patch (code needs review) +Status: patch (code needs work) Patch applies, but needs more work IMHO. The "Submitted by" link is still there if the user is blocked. The same is true for the content list in "admin/node", the link to disabled users is still there. There are probably more occurences... Uwe Hermann Previous comments: ------------------------------------------------------------------------ Wed, 31 Aug 2005 20:47:18 +0000 : moshe weitzman a blocked user has a regular profile link issued by theme('username'). the problem is that if you follow the link, you get 'page not found' error. ------------------------------------------------------------------------ Wed, 31 Aug 2005 20:53:37 +0000 : killes(a)www.drop.org In which way would a fix be desirable: a) not display a link b) make profile display work? ------------------------------------------------------------------------ Wed, 31 Aug 2005 20:58:27 +0000 : moshe weitzman a) - one of the main reasons for blocking a user is that they post inappropriate content in their profile ------------------------------------------------------------------------ Wed, 31 Aug 2005 21:06:10 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/username.patch (628 bytes) Makes sense, care to try this patch?

1 0

[drupal-devel] [feature] Localize date formats
by Uwe Hermann 19 Sep '05

19 Sep '05

Issue status update for http://drupal.org/node/11623 Post a follow up: http://drupal.org/project/comments/add/11623 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: pancho Reported by: Goba Updated by: Uwe Hermann -Status: patch (code needs review) +Status: patch (code needs work) Doesn't apply anymore. Also, please provide _one_ patch (not one for each file you change). Uwe Hermann Previous comments: ------------------------------------------------------------------------ Fri, 15 Oct 2004 18:52:24 +0000 : Goba Attachment: http://drupal.org/files/issues/Drupal-translate-date-formats.patch (1.99 KB) Currently if I set a date format for the site, then it will be used with all interface translations. Different languages however have different needs when presenting dates. The Hungarian date format does not allow the day name to be the first and it calls for a dot between the hour and minute (yes, a dot not a colon). This means that Drupal ships with no suitable date format for Hungarians. Also none of the Hungarian date formats will be suitable on an English interface, while none of the English date formats are acceptable on a Hungarian interface on a bilingual site. The only option is to make date formats translateable. Since these are dynamic t() calls, extractor.php needs to be extended to include the date formats shipped with drupal in the pot output, so translators will get them (much like we do for some numbers and day names). I will take care of extending extractor.php, once this patch gets applied. ------------------------------------------------------------------------ Fri, 15 Oct 2004 19:00:27 +0000 : Goba Attachment: http://drupal.org/files/issues/Drupal-translate-date-formats_0.patch (1.99 KB) Well, the t() needs to be called on the date format set by the admin, so I needed to fix three t() calls. Updated patch attached. ------------------------------------------------------------------------ Fri, 15 Oct 2004 19:01:56 +0000 : killes(a)www.drop.org *g* I recall that there was a time when you didn't like this approach. Anyway, I fully endorse it and would like to see it in 4.5. There is also no suitable data format for German in core. And yes, that's a bug not a feature request. ------------------------------------------------------------------------ Fri, 15 Oct 2004 19:05:33 +0000 : Goba Yes, I didn't like it, because these are dynamic t()s. But since the selected format always needs to be presented in the actual language, it needs to be dynamic by nature. We have some of these dynamic t()s already, handled specially in extractor.php. ------------------------------------------------------------------------ Fri, 15 Oct 2004 23:07:29 +0000 : Steven I like this idea a lot, but I don't like the simplistic "wrap it all in t" feature. There are instances where format_date output is intended for script output. Changing those date formats would mean a bug in the code or UI (for example the way a node date is specified and parsed). Only those strings that are intended for user consumption should be translated. That's why I think we need a more specialized date handling component. It would deal with stuff like variations ('show hour/hide hour'), long/short variations (we need to review the usage of small/medium/long), as well as localizing them. We would need to revise the formats shipped with Drupal, but this gets tricky. For example would we only ship US-style AM/PM date formats with core? Should we only have M/D/Y ordering, or also D/M/Y ordering? If we move the time format settings to locale (where they do below, in principle) we might get a drop in usability: moving from a simple selection box, to requiring the whole localisation framework 'just to switch from 12 hours to 24 hours'. In some parts of the world, there is only one format for dates (e.g. chinese/japanese 2004年10月16日 1時00分). For them, the only variations are long vs short (whether to show the year, the time, etc), in other parts of the world all bets are off and people use what they like. This would mean that if we provide a default set of formats, and let them be translated through PO files, some languages might require more items than provided and some languages would have to use duplicates to fill all available options. On top of that, there is no guarantee that format #3 in French matches format #3 in German, so you'd need to be able to specify this per language. Ideally, we would let a language define any number of date formats, and let the user choose one of those per language. We could also provide a custom option where you enter your own date format, for the realy picky ones. The question remains: what do we do for sites without locale enabled? We can't go to requiring locale just to switch the date on a non-localized site. In that case, perhaps we could do this: - Have locale.module deal with date formats when it is enabled. Have a per-language format setting, selected from a language-specific, po-defined date format list. - Have system.module implement a 'localization' admin menu item if locale.module is /not/ enabled. This page carries the old selector, which acts like a simplified version of locale's format selector, with the list we current have in Drupal (i.e. variations of 24/12hours, d/m/y, y/m/d, m/d/y, ...). To prevent confusion, we add a link to system.module's version of the localization page, which instructs users to enable locale.module for full localization abilities). I know it sounds a bit weird (we certainly don't do this anywhere else), but it would be consistent in terms of UI (the date format is "localization" no matter how you look at it) and keep both non-localized site admins and localized site admins happy, without sacrificing too much ease of set up. ------------------------------------------------------------------------ Sat, 16 Oct 2004 09:34:09 +0000 : Goba Steven, you say: "/There are instances where format_date output is intended for script output. Changing those date formats would mean a bug in the code or UI/ " But the current approach is to translate the month and day names in every case, so format_date **already** assumes that the date it outputs is for human consumption and not for machines. So changing the order of the fields in user date formats should not hurt. Sure it might not be a useability dream, but it does not hurt. By the way, adjusting date formats based on user language is only a small piece of localisation not covered by Drupal core yet. I need to use one mission statement and site name in all languages, and even my primary and secondary links need to have the same text in all site languages, since they are not translateable. If we are about to find a generic solution for locale covered settings, we need to take these into account too. ------------------------------------------------------------------------ Fri, 29 Oct 2004 00:10:44 +0000 : Steven ""so format_date *already* assumes that the date it outputs is for human consumption and not for machines." " What I mean is changing the placement of the date components (day, month, year). For numerical dates which are parsed by a program, this will introduce bugs. I'd much rather have a cleaner date format selection method integrated with locale. ------------------------------------------------------------------------ Fri, 29 Oct 2004 13:19:01 +0000 : Goba Steven, the custom date formats are not translated, only those selected by the admin. And the admin can select date types with spelled out month/day names at any level, so they are not machine parsable anyway in the current Drupal version. **This patch does not introduce bugs.** I admit it does not look good, but there is no system for locale dependant admin settings yet. ------------------------------------------------------------------------ Fri, 29 Oct 2004 15:05:28 +0000 : Steven I'm saying we do need such a system. With your patch, the format that the admin selects in language A will not necessarily match the one in language B. We should go for locale-defined formats with a locale-neutral list to fallback on. ------------------------------------------------------------------------ Sun, 13 Mar 2005 14:55:22 +0000 : pancho Attachment: http://drupal.org/files/issues/dateformat.patch.zip (14.91 KB) ************************ DATEFORMAT-PATCH PACKAGE ************************ by pancho(a)suenderhauf.net 2005-03-13 03:07 This 'DateFormat' patch package only works with the latest CVS version of Drupal 4.60 RC. This 'DateFormat' patch package applies the following changes: 1. It introduces a more flexible system of standard date and time formats which helps both users and developers while staying backwards-compatible. We often see special date formats be used in modules, which is only second best, as these can neither be adapted by the user nor localized. More flexibility allows the developers to use the built-in format functions wherever possible. Changes in detail: - The standard date formats used to be combined date&time-formats. This is now split into separate date formats and time formats, to allow dates and times to be rendered in a more flexible way. - Still it's backward compatible: the combined formats - while deprecated - still exist. - A "Tiny" format is added to the "Small", "Medium" and "Large" formats. 2. It enables Drupal to show e.g. German and Hungarian date formats (with a colon as separator), which was _not_ possible before. 3. It prepares both the Drupal system and module developers for the upcoming locale_dateformat module. This module will fully localize date and time formats according to the localization of the current user. The locale_dateformat module will be available within the next weeks. I'm always happy about suggestions and criticism, if constructive. You can either write a comment on www.cvs.net or an email to pancho(a)suenderhauf.net. Bye, Pancho ------------- INSTALLATION: ------------- The package contains two patches for the latest CVS versions of Drupal 4.60 RC, both of which should be applied: 1. common.inc (patch for v.1.429) 2. system.module (patch for v.1.197) There are no changes in database structure. ------------------------------------------------------------------------ Sun, 13 Mar 2005 14:59:33 +0000 : pancho P.S. What the hell did I write about www.cvs.net? No, what I meant was of course www.drupal.org (here), where you can write a comment! ------------------------------------------------------------------------ Sun, 13 Mar 2005 15:08:35 +0000 : pancho I feel better if I refile the thread as a feature request. The only thing you could describe as a bug was that German & Hungarian date formats were not supported. The rest are definitely feature improvements. ------------------------------------------------------------------------ Sun, 13 Mar 2005 16:38:02 +0000 : Steven - Localizing date formats seems like something that should be solved properly in core once and tied into locale.module. We need a solution which does not pose problems with mixing languages, and which does not require us to keep adding more and more date formats to the list (e.g. what about asian date formats? They don't have spaces.) Your "if (!module_exist('locale_dateformat'))" solution is unacceptable for core. - What's the point of the 'tiny' date format if it's not used anywhere? Where would it be used? - Please don't post patches as zips, this is annoying. Just make one global patch file instead of separate ones. Multiple files can be posted with multiple follow ups. - You need to see how this affects the date usage around Drupal, e.g. the profile date field formatting, which depends on the date format being in a specific format. - Code style: check spaces around quotes. Also, avoid trailing spaces on lines, this is untidy. - This issue is not "critical" IMO. It is a new feature request, and as such should probably wait until after 4.6, as 4.6 is in feature freeze. - The apostrophes around the separator item seem a bit unnecessary. ------------------------------------------------------------------------ Mon, 14 Mar 2005 17:08:34 +0000 : pancho */First of all: This patch works! Even it may be not yet ready to be adopted in the core, it is good for individual use./* To Steven: Okay, Steven, I was asking for criticism... :) With some of the points you make I can agree: - This feature should be completely and properly rewritten and not just fixed to have a good solution for a longer period and not only for now. - The "if (!module_exist('locale_dateformat'))" solution is not good code style at least for core, as well as the spaces. I didn't take enough care about that, as I wanted to present a solution now, before the release of 4.6. - Sorry about the zipped patches. I didn't expect it to bother anyone. But on the other hand: - It's not about adding more and more date formats. You will agree that the German web is a big enough market which should not be left aside anymore. Therefore the feature request /is critical/ from my perspective at least. And the solution I presented doesn't blow up the date format lists, in the contrary it makes them easier to use. - How can you complain about the tiny version not to be used. If it doesn't exist it /can't be used/. - The apostrophees do make sense, because the separator can also be a single space. Nobody would identify that in the menu without apostrophees. So you were critizising some points which I will fix in a new version of the patch as soon as possible. But you didn't say anything about the split of date and time formats. To me it makes sense, as there have already been more people asking for "just date" or "just time". While I already started the development of a "locale_dateformat.module", I will put this on hold, until we figured out a good solution altogether. Anyway, I'm ready to help developing this feature within "locale.module" if I'm being asked. Otherwise developing a separate module would at least offer the so needed functionality /within this century/ ;-) Pancho ------------------------------------------------------------------------ Fri, 18 Mar 2005 07:50:14 +0000 : Steven Firstly: "Even it may be not yet ready to be adopted in the core, it is good for individual use. " The patch queue is really part of the Drupal development process. It does not help us much to work on patches that are not intended for core. "It's not about adding more and more date formats. You will agree that the German web is a big enough market which should not be left aside anymore. Therefore the feature request is critical from my perspective at least. And the solution I presented doesn't blow up the date format lists, in the contrary it makes them easier to use. " I'm just saying, that solutions like the German date format (which has a special notation for the day numbers) or Asian formats (which have no spaces and reversed order) should really be handled through locale IMO, where date formats are picked per language, and can be added manually or through .po files. "How can you complain about the tiny version not to be used. If it doesn't exist it can't be used. " I imagine you introduced this format for a reason, no? Adding new features without knowing exactly where they should be used is silly. If you add a new feature in a patch, you should put in the work to see that this feature is actually used. I didn't really talk about splitting date/time formats for now, as it is mostly a usability thing. I'd much rather wait for a solution integrated with locale before I comment on it. ------------------------------------------------------------------------ Wed, 27 Jul 2005 16:32:19 +0000 : Bèr Kessels +1 for the idea. +1 for the implementation. The idea is simple: just pull the format string trough the locales. No new interfaces (+1), no new configuration options (+1) and no additional cruft. po files will then take care of the real format. However, I see one issue: we should remove the dopdowns in /admin/settings. The patch still applies.

1 0

[drupal-devel] [feature] Comment approval queue a pain to use
by Uwe Hermann 19 Sep '05

19 Sep '05

Issue status update for http://drupal.org/node/28595 Post a follow up: http://drupal.org/project/comments/add/28595 Project: Drupal Version: cvs Component: comment.module Category: feature requests Priority: normal Assigned to: mrowe Reported by: robertDouglass Updated by: Uwe Hermann Status: patch (code needs review) Attachment: http://drupal.org/files/issues/comment_bulk_approval_2.patch (2.45 KB) Rerolled patch and fixed a few issues: Using ' instead of " now where possible, made "Save changes" translatable, use %d more often, indent code using 2 spaces (not 4). The functionality works as expected and is quite an improvement over the current stuff. +1 from me. Uwe Hermann Previous comments: ------------------------------------------------------------------------ Wed, 10 Aug 2005 08:29:14 +0000 : robertDouglass When using the comment approval queue the workflow for approving comments is awful. You have to click administer->comments->approval queue to get a list of comments awaiting approval. Then you have to click edit on one of the comments, click the collabsible field-set for the publishing controls, click to published and save the comment. Then repeat the process for the next comment. I'd say that this workflow is a significant deterrent to anyone using this functionality. To make it better, there would be checkboxes next to each comment in the table on the approval queue page, and a set of operators analogous to the content overview page. Better yet would be if each comment in the table was an AJAX link to the comment's body and info which would expand right there in the table itself, since it is usually impossible just from the comment title to know whether to publish or not. If one could click the various comments open, right there in the table, then mass publish or delete, we'd have a real approval queue. Perhaps the spam module could tie into the actions that can be taken on comments so a mass "mark as spam" could be done too. ------------------------------------------------------------------------ Wed, 14 Sep 2005 13:32:47 +0000 : mrowe Attachment: http://drupal.org/files/issues/comment_bulk_approval.patch (2.24 KB) Here's a patch (against HEAD) that gives you the cheap-and-dirty approach (checkboxes that allow bulk approval/unapproval of comments). I'll leave the gold-plated AJAX solution to someone with more time. :) ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:17:21 +0000 : Dries - Don't use tabs. - SQL queries might be vularnable to SQL injection attacks. Use db_query()'s %s and %d directives. ------------------------------------------------------------------------ Thu, 15 Sep 2005 02:24:17 +0000 : mrowe Attachment: http://drupal.org/files/issues/comment_bulk_approval_0.patch (2.38 KB) Ok, I think I got rid of all the tabs this time... I've had to use db_escape_string, rather than the sprintf params to db_query, but it should have the same effect. ------------------------------------------------------------------------ Thu, 15 Sep 2005 03:28:50 +0000 : moshe weitzman minor nits: we use an extra line in our else clauses. this should be two lines: } else { also, you can almost always avoid using db_escape_string() directly. you will need to do a str_repeat('%d, ', count($unpublish)) in the SQL(for unpublish query) and then pass a an array as the last argument to db_query. This is the seldom used 'pass all values in an array' feature of db_query: http://drupaldocs.org/api/head/function/db_query ------------------------------------------------------------------------ Thu, 15 Sep 2005 11:10:20 +0000 : mrowe Attachment: http://drupal.org/files/issues/comment_bulk_approval_1.patch (2.43 KB) Oh, so you're saying I should have read the style guide... ;) I've fixed the brace position, and I've redone the db_query as you suggested. Thanks! ------------------------------------------------------------------------ Sun, 18 Sep 2005 11:36:02 +0000 : Dries Code needs more small style fixes. Haven't tried the patch, nor did I try to look at the big picture. However, I agree that the comment approval workflow sucks. How do other systems deal with this? What does their UI and interaction design look like?

1 0

[drupal-devel] [bug] comment block shows comments from unpublished nodes
by Uwe Hermann 19 Sep '05

19 Sep '05

Issue status update for http://drupal.org/node/29866 Post a follow up: http://drupal.org/project/comments/add/29866 Project: Drupal -Version: 4.6.2 +Version: cvs Component: comment.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: Florian Hufsky Updated by: Uwe Hermann -Status: patch (code needs review) +Status: patch (ready to be committed) Attachment: http://drupal.org/files/issues/comment_17.patch (1.1 KB) Patch rerolled, boldly setting to "ready to be committed". Uwe Hermann Previous comments: ------------------------------------------------------------------------ Sun, 28 Aug 2005 16:58:10 +0000 : Florian Hufsky The comment block shows links comments which were once published, but later set to "don't publish". How to reproduce the bug: 1. create a node 2. post a comment 3. edit the node: set uncheck published Bugfix: change the sql query for comments to show in comment_block: modules/comment.module Line 184 to SELECT c.nid, c.* FROM {comments} c INNER JOIN {node} n USING (nid) WHERE n.status = 1 AND c.status = 0 ORDER BY c.timestamp DESC ------------------------------------------------------------------------ Sun, 28 Aug 2005 17:45:07 +0000 : Uwe Hermann Attachment: http://drupal.org/files/issues/comment_14.patch (1.07 KB) Confirmed in HEAD. Patch attached. Should probably be backported to 4-6, too.

1 0

[drupal-devel] [bug] comment preview "Required" is easily bypassed
by Jeremy 19 Sep '05

19 Sep '05

Issue status update for http://drupal.org/node/28420 Post a follow up: http://drupal.org/project/comments/add/28420 Project: Drupal Version: cvs Component: comment.module Category: bug reports Priority: normal Assigned to: Jeremy(a)kerneltrap.org Reported by: Jeremy(a)kerneltrap.org Updated by: Jeremy(a)kerneltrap.org Status: patch (code needs review) Attachment: http://drupal.org/files/issues/common.inc_12.patch (905 bytes) I have been unable to get my is_hash_duplicate() function working properly. I taught it the difference between "preview" and "submit", but it still gets confused if you try and go back and edit content, or if you try and post a second comment to the same place. I'm out of time for now. Instead, here is a simple patch to use the remote IP address in the token hash, making it a little more difficult for spammers to re-use a hash. Something else that could be used is the session ID -- but are there legitimate reason someone's session ID may change from page to page? ie, what if cookies aren't enabled, and the session ID isn't embedded in the URL? Jeremy(a)kerneltrap.org Previous comments: ------------------------------------------------------------------------ Mon, 08 Aug 2005 01:55:34 +0000 : Jeremy(a)kerneltrap.org Setting "Preview comment" to "Required" does not strictly require that the comment be previewed first. This is being abused by spammers to quickly and efficiently post spam comments. I discovered this after I added a new feature to my new spam module [1] to auto-blacklist spammer IP addresses, allowing me to block comment spammers when they preview a comment and thus preventing them from ever inserting their spam into my database. I configured my comment module to "require" comment previews, and yet found that the comments were slipping past my filter. I finally realized what the spammer is doing is setting $_POST['op'] to 'Post comment', effectively bypassing the preview phase. I'm currently looking for a clean solution to this. At the moment the only idea I have is to generate a token at the preview phase, and validate the token at the post phase. Unfortunately the token would have to be stored in the databse between the preview and the post, which adds overhead. Alternatively, I've considered using a time-based hash which would constantly update depending on the time of day. This could easily be validated without storing anything in the database. If too long has gone between the preview and the post, an additional preview step would be required... The down side here is that the time-based hash would be publically available, and thus the spammer could easily duplicate it in their script. A private key could solve for that, but increases the complexity as it adds a configuration step. I have the feeling I'm missing a simpler, cleaner solution. Suggestions? [1] http://kerneltrap.org/jeremy/drupal/spam/ ------------------------------------------------------------------------ Mon, 08 Aug 2005 02:26:21 +0000 : moshe weitzman even if you get this fixed, won't these bots just add a preview step? this 'preview required' feature is designed to maintain high quality submissions by forcing users to proof read. it isn;t designed for security. i think you want to hook into comment_validate(). just add a hook here - there is already a hook_comment() waiting for you to add an operation. ------------------------------------------------------------------------ Mon, 08 Aug 2005 08:49:30 +0000 : Eaton I posted a patch a few days ago (http://drupal.org/node/28255) that adds validation and form construction hooks for comments. It's similar to the one that the captcha module uses, though it adds comment form_pre and form_post hooks instead of a single comment form hook. ------------------------------------------------------------------------ Mon, 08 Aug 2005 13:30:34 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_11.patch (2.5 KB) > even if you get this fixed, won't these bots just add a preview step? Eventually, yes, but it drastically changes their ability to fling spam at a site. As is, they simply have a script that shoots data out at high speed without having to wait for messages to return from the server. It is the server that is doing all the work, thus making it simple for a spammer to DoS a site. If "preview required" really meant "preview required", they would be forced to first automate clicking "preview", and then wait for a response before clicking "submit". This requires more resources on their side, and allows us to add delays after clicking "preview" (if we detect that they are a spammer) further using their resources. > this 'preview required' feature is designed to maintain high quality > submissions by forcing users to proof read. it isn;t designed for security. Regardless of the intention, I was misled to believe that configuring my site to require previews would require that all comments were first previewed. As a site administrator, I would prefer to know that "required" really means "required". > i think you want to hook into comment_validate(). just add a hook here - > there is already a hook_comment() waiting for you to add an operation. Yes and no. Ultimately yes that will work and will allow my spam module to prevent the spam from ever being posted. But it still leaves the greatest burden on the web server, instead of on the spammer. The spammer can still use a very simple script that only pushes data, and thus can generate spam at an unbelievable rapid rate. Here is an example patch to enforce "preview required". It's one idea, I'm sure there are better ones. ------------------------------------------------------------------------ Mon, 08 Aug 2005 14:01:38 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_12.patch (1.4 KB) Here's a second version of the patch that doesn't require any manual configuration. ------------------------------------------------------------------------ Mon, 08 Aug 2005 16:27:27 +0000 : Jose A Reyero I like this idea, and the patch looks good Still, I think it misses something, like some timestamp related hash, because once you get the hash code you can post multiple comments with that. Another problem I can think of is, what happens when a cron run happens between the preview and the post?? I'm afraid comments would get lost For this second problem, I think a key generated only once after module activation could do. About the first one....mmm... I'll sit down for a while and think..... ------------------------------------------------------------------------ Tue, 09 Aug 2005 12:27:20 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_13.patch (1.33 KB) > I think it misses something, like some timestamp related hash, because > once you get the hash code you can post multiple comments with that. Using a timestamp will mean that the comment form "expires". That is, if you wait too long to preview your comment, it will generate an error when you try to post. Yes, technically a spammer could post one real comment, then based on what was in the session from that they could post the same identical comment over and over, so long as it was attached to the same node. But this is not what they do, they try and spread their spam throughout your webpage. Furthermore, the spam module is perfectly capable of detecting and preventing this. > Another problem I can think of is, what happens when a cron run happens > between the preview and the post?? I'm afraid comments would get lost The key is only generated once, that's what the first test is about. In any case, in the unlikely event that the key were to change between preview and post they would simply have to post a second time. My earlier patch wasn't quite right, I was testing the token in the wrong place. This patch fixes that. BTW: This is beneficial for maintaining high quality submissions too, as prior to this change someone could: 1) enter a comment 2) press preview 3) completely change their comment (introducing a mistake) 4) press submit and the comment (mistake and all) would go into the database unpreviewed After this change: 1) enter a comment 2) press preview 3) completely change their comment (introducing a mistake) 4) press submit and they get an error because they didn't preview their changes - forcing them to preview once after any change ------------------------------------------------------------------------ Wed, 10 Aug 2005 03:50:33 +0000 : Jeremy(a)kerneltrap.org FWIW: I've been getting slammed by spam attacks this whole week. Installing this patch has made a huge difference. Well over 100 spam attempts per minute (sometimes two and three times that) and I hardly notice the spammer, whereas before it was choking my database. (Granted, the spammer has not yet upgraded his script to first preview, then submit. But even if he did it wouldn't help him as testing has verified that the new spam module would prevent the comments from ever getting to the database.) Additionally, user and anonymous (nonspam) comments continue to show up at a normal rate. ------------------------------------------------------------------------ Tue, 16 Aug 2005 14:08:04 +0000 : Jeremy(a)kerneltrap.org I would love to see _any_ discussion on this. Drupal is currently too easy to spam, with little effort on the spammer's side, and lots of resources wasted on the Drupal side. A patch like this will greatly increase the spammer's burden, and make it possible to effectively block even the most aggressive spammer attacks. ------------------------------------------------------------------------ Wed, 17 Aug 2005 16:24:04 +0000 : Jose A Reyero Well, this patch is definitely better than what we have, and would save some spam for sure. But maybe keeping track, at the session level, of generated hashes for a user, and then removing them when the comment is sent, could do the work. This way we can forget about previewing comments or not, and also the "permission" to post the comment would expire when the session expires. Any randomly generated value could do for this, no need for complex hashes, but having nid and pid in the hash would add some extra security. ------------------------------------------------------------------------ Wed, 17 Aug 2005 19:58:02 +0000 : breyten Jeremy, a big +1 on the idea, but why not generate the private key when it is actually needed (Ie, when displaying the comment form), instead of wasting a _cron() hook on it? ------------------------------------------------------------------------ Thu, 18 Aug 2005 03:20:02 +0000 : Jeremy(a)kerneltrap.org > Well, this patch is definitely better than what we have, and would save some spam for sure. It is continuing to work very well on my site, which seems to be under nearly perpetual spam attacks from multiple sources. > But maybe keeping track, at the session level, of generated hashes for a user, and then > removing them when the comment is sent, could do the work. The catch is: the key has to be something unique to the server, not guessable or learnable from the outside Simply storing the hash data in the session alone is not enough, as then the spammer could create any random data and store it in the session. That said, the hash could be generated off something other than the text of the comment as it is now, so that a preview is not required. I'll look at doing something like that and submit another patch. > This way we can forget about previewing comments or not, and also the "permission" to > post the comment would expire when the session expires. Any randomly generated value > could do for this, no need for complex hashes, but having nid and pid in the hash would > add some extra security. nid and pid alone are worthless, as they are easy to learn. The pid can always be 0 (spam is rarely attached to a pre-existing comment). The nid is obtained in the path of where the spam is being posted. The solution is a "private-key", which is what my patch adds. Then sure, hash the private key plus the nid and the pid, and you've got enough protection to prevent most spammers. To make it even more secure, automatic rey-keying could be easily accomplished. ------------------------------------------------------------------------ Thu, 18 Aug 2005 04:09:10 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_15.patch (1.18 KB) The attached patch: 1) gets rid of the _cron() hook 2) no longer requires that comments be previewed Prior to this patch, comment spammers were able to send data to a Drupal server acting as though they'd filled out a comment form and pressed submit. As they didn't actually use the form, they could submit spam comments at an obscene rate. With this patch, comment spammers will have to actually load the form, enter text, and press submit. Yes, that can still be automated, but it takes much more work and slows them down, as they have to wait for the entry form to load each time. Unfortunately a spammer could manually submit one comment, then re-use that same session info over and over to attach repeated spam comments to the same node. Such an attempt would be detected and blocked by the spam module if enabled, but again such a session re-use attack could be done without loading the form each time. Fortunately there is much less gain for a spammer to submit 100 spam comments on the same page, versus submitting 100 spam comments each on a different page as they do now. Ideas to improve upon this concept include: - re-key every day or week, changing the private key regularly to be sure it couldn't ever be permanently cracked - add a key table, and generate a unique key for every comment form. essentially, upon comment form creation generate a random key which is stored both in a database table and in the session. when a comment is submited, look for the key from the session in the database table, if a match is found delete it from the database table and post the comment. this would prevent session re-use, but adds overhead. i don't know if it's worth it, perhaps as an external module if the hooks were available. ------------------------------------------------------------------------ Fri, 19 Aug 2005 18:55:11 +0000 : drumm <?php form_set_error('error', t('Validation error, please be sure cookies are enabled on your browser.')); ?> form_set_error [2]()'s fist argument should be the name of a form field, not 'error.' Using (..., 'error') would be better in this case. And the actual message needs work. Since this is a hidden field I don't think it has anything to do with cookies. [2] http://drupaldocs.org/api/head/function/form_set_error ------------------------------------------------------------------------ Fri, 19 Aug 2005 18:56:41 +0000 : drumm The unclosed link in my last update was supposed to say drupal_set_message(..., 'error') ------------------------------------------------------------------------ Sat, 20 Aug 2005 16:00:15 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_16.patch (1.11 KB) drupal_set_message(..., 'error') isn't sufficient to prevent the comment from being posted. I have instead updated the patch to set the error on the hidden 'token' form field. I have updated the message to read: "Unable to validate your comment, please try again. If this error persists, please contact the site administrator." If you don't like the error message, better suggestions are welcome. ------------------------------------------------------------------------ Fri, 09 Sep 2005 03:16:06 +0000 : Jeremy(a)kerneltrap.org Any feedback on this patch? I have been running it on my website for a couple of weeks, and it has completely stopped the most persistent auto-spam scripts that had been posting poker type comments constantly to my site. ------------------------------------------------------------------------ Sat, 10 Sep 2005 18:12:15 +0000 : Zed Pobre This patch is against HEAD? It doesn't want to apply to my 2.6.3 comment.module. ------------------------------------------------------------------------ Sun, 11 Sep 2005 18:09:08 +0000 : Abalieno It's is for cvs but I'm trying to manually apply it to 4.6.3. Will comment later to tell how it went. ------------------------------------------------------------------------ Mon, 12 Sep 2005 19:49:17 +0000 : Abalieno Well, it worked. No spam at all in more than a day. I don't know if other users are having problem but this patch broke the tool the spammer was using. :) ------------------------------------------------------------------------ Wed, 14 Sep 2005 02:47:40 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/form_validation.patch (2.72 KB) Here's a completely rewritten version of the patch, based on an email discussion with Dries. This provides a more generic interface that can be used to validate other form submissions, not just comments. The patch introduces two new functions, form_token() and form_validate(). The first function uses a private key and a public key to set a token in a hidden field. The second function validates the token. The patch also updates the comment module, demonstrating how these new functions are used. More information as to how the patch works can be found in the comments that are within the code. Based on my own experiences on kerneltrap.org, this patch blocks 99% of the current comment spammers. ------------------------------------------------------------------------ Wed, 14 Sep 2005 03:16:21 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/form_validate-2.patch (1.1 KB) This optional patch is intended to be applied after my form_validation.patch attached to #20 above. It makes the drupal_private_key more secure by regenerating it every 24 hours. Without this patch, it would be possible for a spammer to use brute-force to learn a site's private key by reading the code and observing the token generated for a given form. With this patch, brute force is still possible, but with this patch it would have to be done every 24 hours. It is possible for a form to be generated prior to a rekey, and then to be validated after a rekey. In this event, the key will fail validation and the user will see a message telling them something like "Validation error, please try again. If this error persists, please contact the site administrator." When they try pressing "submit" again it will work fine. I kept this patch separate as it adds complexity that may not be desired in core. Personally I think it is important and should be merged along with the first patch. I added it to the system module as it seemed the most logical place, and is a "required" module that can not be disabled. ------------------------------------------------------------------------ Wed, 14 Sep 2005 03:58:35 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/contact.module_8.patch (1.44 KB) This third patch updates the contact.module to use the new form token validation functions. Note that this is not yet a perfect solution. Someone wanting to spam the contact form could manually fill it out once to obtain a valid token. They could then use that token to spam the contact form repeatedly, so long as they don't change the fields that were used to generate the token. The solution is to keep a history of recently validated tokens. Each time a token is validated, make sure it was not already recently validated -- if it was, don't allow it a second time. I would be happy to provide patches for this if it is deemed necessary, and these first patches are merged. It would require a new database table, and a db_query to validate the token. Ideas for alternative solutions are welcome. ------------------------------------------------------------------------ Sun, 18 Sep 2005 11:51:00 +0000 : Dries For the contact module, maybe it is better to use the recipient's e-mail address to calculate the token. Like that, each personal contact form would have a unique token, making it slightly more difficult to reuse tokens. Another solution might be IP- or session-based tokens. Do spammers post from a single IP, or do they come from different IPs (e.g. using hacked computers). If you use the poster's IP to calculate the token, tokens become more dynamic. ------------------------------------------------------------------------ Sun, 18 Sep 2005 11:53:50 +0000 : Dries I committed the current implementation of this patch, though I'd like us to think some more about how we can make reusing tokens more difficult without introducing complex logic. For sake of simplicity, I didn't commit the cron-based private key regenerator. ------------------------------------------------------------------------ Sun, 18 Sep 2005 14:12:21 +0000 : Jeremy(a)kerneltrap.org I have written a function called hash_is_duplicate() that uses a serialized array stored in the variable table to validate that the token is not duplicated for the n last tokens. (n is a constant, defined as 256 in my patch). It is relatively simple, and avoids the need to introduce a new database table. However, left to do is to teach it the difference between a preview and a submit (ie, multiple previews are okay, but only one submit is okay), I'll try and post a finished patch soon. An alternative idea is to simply remember the last token used (rather than remembering n tokens). At this time, spammers simply submit the same thing over and over as fast as they can. But they're not stupid, and they'd quickly learn to switch back and forth between two different spams. > For the contact module, maybe it is better to use the recipient's e-mail > address to calculate the token. Like that, each personal contact form > would have a unique token, making it slightly more difficult to reuse tokens. That is what it currently does, on one of the forms. On the other, the information is dynamic so not a good token. The only way we can use dynamic data as tokens is to first force previews. > Another solution might be IP- or session-based tokens. Do spammers post > from a single IP, or do they come from different IPs (e.g. using hacked > computers). If you use the poster's IP to calculate the token, tokens become > more dynamic. Very good idea, and a simple change. From what I've observed, spammers use a wide range of IPs. > For sake of simplicity, I didn't commit the cron-based private key regenerator. Please reconsider. There are dozens of md5 brute force crackers freely available. They would have to be customized to easily crack our tokens, but as all data but the private key is freely available the customization is simple. Thus, a smart spammer could brute force crack a private key with relative ease and a little patience. An alternative approach would be to double the length of the private key, which would at least make this harder. Thoughts?

1 0

[drupal-devel] [bug] Move node revision information for files to a separate table
by Junyor 18 Sep '05

18 Sep '05

Issue status update for http://drupal.org/node/31354 Post a follow up: http://drupal.org/project/comments/add/31354 Project: Drupal Version: cvs Component: file system Category: bug reports Priority: normal Assigned to: Anonymous Reported by: Souvent22 Updated by: Junyor Status: patch (code needs review) Attachment: http://drupal.org/files/issues/drupal-head.file-revisions3.junyor.patch (8.73 KB) Updates after code review from Cvbge (thanks!): - fixed typo in database.pgsql - using a select statement in update_148() - fixed logic in upload_delete() Junyor Previous comments: ------------------------------------------------------------------------ Fri, 16 Sep 2005 00:55:42 +0000 : Souvent22 I think that the files table has too much redundat data in it since we now have the new revisions system. I think another table to link revisions to files would be more appropriate. ------------------------------------------------------------------------ Fri, 16 Sep 2005 14:17:55 +0000 : Junyor I agree. NID and VID should be removed, making a new table containing FID, NID, and VID with all three as keys. ------------------------------------------------------------------------ Sun, 18 Sep 2005 09:56:50 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-head.file-revisions.junyor.patch (8.27 KB) Here's a patch with the proposed changes. This is completely untested. I don't know SQL very well, so I'm sure I've done something wrong. I'll hopefully be able to do testing later today. ------------------------------------------------------------------------ Sun, 18 Sep 2005 17:04:38 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-head.file-revisions2.junyor.patch (8.55 KB) Updated patch: - cleaned up errors in database.* - cleaned up errors in update.inc I tested adding files, listing and unlisting files, deleting files, deleting revisions, deleting nodes, adding files only to certain revisions, etc. Deleting revisions does not delete the files, but as far as I can tell, that's because of http://drupal.org/node/31323.

1 0

[drupal-devel] [feature] Give file uploads titles
by timcn 18 Sep '05

18 Sep '05

Issue status update for http://drupal.org/node/30775 Post a follow up: http://drupal.org/project/comments/add/30775 Project: Drupal Version: cvs Component: upload.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: timcn Updated by: timcn Status: patch (code needs review) Attachment: http://drupal.org/files/issues/upload.zip (1.33 KB) With this patch, you can rename your file uploads. Normally, only the file name shows up. This adds an additional input field to supply a title for the file that is displayed instead. timcn

9 29