March 2005 - Development - lists.drupal.org

[drupal-devel] Question on taxonomy_form() function
by Scott Courtney 31 Mar '05

31 Mar '05

Good morning! I've read the API docs on this, and even looked at the function's source code. I understand how it works, but apparently not how to use it properly. :-) My new image_import module (a feature that I'm developing in cooperation with walkah's excellent work, and yes, I've emailed with him to discuss it first) has code that looks like this: $html = ''; $vocabs =& taxonomy_get_vocabularies(IMAGE_IMPORT_IMAGE_NODETYPE); // The above function orders by weight, so we don't have to sort $group = ''; foreach ($vocabs as $vocab) { $group .= taxonomy_form($vocab->vid, $_POST['edit']['taxonomy'],NULL); } $html .= form_group(t('Categories'),$group,t(' ... long help text here ...')); The constant IMAGE_IMPORT_IMAGE_NODETYPE is just the string 'image'. After submitting a form, the $_POST['edit']['taxonomy'] subarray looks like this: array(3) { [0]=> string(1) "1" [1]=> array(1) { [0]=> string(1) "3" } [2]=> array(1) { [0]=> string(1) "7" } } The three selected term IDs are "1", "3", and "7". My problem is that the second vocabulary (vid==2, tid=={3,7}) allows multiple selection, whereas the first vocabulary (vid==1, tid==1) does not. The way the underlying form_select() called by taxonomy_form() seems to work is that it uses a subarray for the field only if multiple selection is allowed. This puts the resulting tids at different levels in the array tree, and causes my $_POST['edit']['taxonomy'] to fail as a way to pass the existing value(s) to taxonomy_form() for a given vid. My first thought was to try using a $name parameter for taxonomy_form(), something akin to taxonomy_1 for vid 1, taxonomy_2 for vid 2, etc. That seems awfully clumsy. I also tried passing $name as an array-like string, e.g., $name=="[taxonomy][1]" for vid 1, etc. That failed syntactically. There isn't a good way to match up those sub-subarrays with the vid to which they point, so simple conditional logic inside my foreach{} loop is not going to be feasible if the site administrator has associated more than one multi-term- allowed vocabulary with images. There's probably a "right way" to do this, but I'll be damned if I see it. The only method I can figure out looks like a hideous kludge. Can someone suggest the correct approach to this, or point me to a module that does it for some example code? The only place I could think of that does this is in node.module, but the problem there is that is has a $node object and passes that to an entirely different function instead of taxonomy_form. My module needs to pick vocabulary terms in advance of the existence of a node object, then apply them to multiple nodes as those nodes are created, so I don't have a $node to pass as node.module and other node-type modules do. My module doesn't define any new node types, but just adds a new way to create nodes of an existing type. I'm probably going to feel stupid when I see the answer to this, but I'll risk it. Any suggestions? Is it worth considering a new core function like taxonomy_forms() that would accept a node type as its parameter and return the HTML for all of the vocabs associated with that node, in such a way that the resulting $_POST is easily parsed by vid to support previews? Thanks! Scott -- -----------------------+------------------------------------------------------ Scott Courtney | "I don't mind Microsoft making money. I mind them scott(a)4th.com | having a bad operating system." -- Linus Torvalds http://4th.com/ | ("The Rebel Code," NY Times, 21 February 1999) | PGP Public Key at http://4th.com/keys/scott.pubkey

2 8

RE: [drupal-devel] "My Projects" on drupal.org -- broken?
by Boerland Bert 31 Mar '05

31 Mar '05

-----Original Message----- From: drupal-devel-bounces(a)drupal.org on behalf of Carl McDade > "you own no projects" gets my vote because there is the likelyhood of > the person clicking on the link being a developer wanting to contribute. I would say, display a link "My own projects" when a user has projects and display the link "All projects" when a user has no "own" projects. In case usability experts disagree with having useless links, it may be best not to display the link at all for useris without projects.

1 0

[drupal-devel] "My Projects" on drupal.org -- broken?
by Scott Courtney 31 Mar '05

31 Mar '05

I am logged in as my username on drupal.org, but when I go to "My Projects" (http://drupal.org/project/user) what I see is a list of *all* projects. Is this feature b0rk3n, or am I doing something wrong or misunderstanding what it's for? Scott -- -----------------------+------------------------------------------------------ Scott Courtney | "I don't mind Microsoft making money. I mind them scott(a)4th.com | having a bad operating system." -- Linus Torvalds http://4th.com/ | ("The Rebel Code," NY Times, 21 February 1999) | PGP Public Key at http://4th.com/keys/scott.pubkey

4 8

[drupal-devel] [feature] Request New Password Security
by Jose A Reyero 31 Mar '05

31 Mar '05

Issue status update for http://drupal.org/node/18719 Project: Drupal Version: cvs Component: user.module Category: feature requests Priority: critical Assigned to: Anonymous Reported by: neofactor Updated by: Jose A Reyero Status: patch Attachment: http://drupal.org/files/issues/user_one_time_login_2.patch (12.77 KB) Ok, I've started with chx's reviewed patch, and done most of Dries suggestions Main changes are: ----------------------- - Rewording, variable names, path, etc... as pointed out by Dries. Now it's user/reset. And yes, it was not clean-URL safe; it is now. - Allowed also for admin account, I dont like it though, but seem to be the only one :( - Used also for user registration, but I also kept old style login/password, think it can be some use and anyway, it's up to the site admin to remove it from the welcome e-mail. However it is only for 'no admin approval', see below about this.... - Added back in the config option for time-out, though reworked completely for usability. Default is '1 day' which I think should be reasonable. However, there's no time out for firts time users. About this, read below, please... ----------------------- Using this for first time login has some problems when admin aproval is required, as the hashed pass depends on 'changed' timestamp, and it changes when the admin approves an account. Actually, there's no way to know -or I didn't find it- when it is the first login for an approved account. For no admin approval, I just used 'changed' == 'created' as a criterium. I think we should be separating this changed date and last login date creating a new field in the user table, but that maybe for a future patch. That would also increase security, as the Administrator's info would be more accurate, and 'last login date' could be shown when user logs in, which is also security+ Related with this I've experienced problems, as administrator, when editing user accounts, then last login info for the users gets lost -is reset-... About the config option for time out, though I agree that enforcing security can be good in general, this is one of this cases where more security means less usability. And that decission should be left to Site administrators -each site has its own security requirements-. So I think the target here should be a 'reasonable' default, and then some flexibility for site admins. IMHO this latest patch is quite well balanced. So... everybody happy? Jose A Reyero Previous comments: ------------------------------------------------------------------------ March 11, 2005 - 03:52 : neofactor Problem: Any user can force another user's password to change... simply by selecting "request new password" and putting in their username. The user gets an email with the new.. but this feels like a violation to the user... and a pain. Solution? If someone requests a new password... Don't blindly change it... send an email that says...."Is this a real request authorized by you? Click here to confirm otherwise disregard this message" Please consider this critical for user by-in to Drupal as a secure system. I appreciate your consideration. http://drupal.org/node/18689 ------------------------------------------------------------------------ March 11, 2005 - 05:05 : neofactor I added some code to prevent the admin account from being reset... Please add as a patch. if ($account->uid == 1 { unset($account); form_set_error('name', t('Sorry. The username %name is not allowed to be changed.', array('%name' => ''. $edit['name'] .''))); } // Just above this code on line 911: if ($account) { $from = variable_get('site_mail', ini_get('sendmail_from')); $pass = user_password(); ------------------------------------------------------------------------ March 11, 2005 - 05:08 : killes(a)www.drop.org Please only set to "patch" if you are actually submitting one. The solution you proposed is rather a workaround and will not be acceptable for core. ------------------------------------------------------------------------ March 11, 2005 - 09:21 : Deno I was in charge of a web site with >10000 paying customers for some time, and the "customers care" was constantly troubled with pasword change requests, until we switched to a system that worked in the following way: 1) "I forgot the password" function accepts either users email, or users login name, generates a random phrase, and adds an entry to "pasword_change_request" table. This entry consists of: - User ID (corresponding to email or login) - random phrase - datestamp User ID is unique key, and the entries in this table older than MAX_TIME are regularly purged by a cron job. 2) System sends an email with a link to "change forgotten password" page with this random phrase as argument to the user. That would be something like: https://drupal.org/user/cfp/d438rwiuodw894732ehdw 3) When user follows the link, "cfp" page checks if the random phrase exists in the table, allows the user to immediately change the password, and sends another email with "your password was changed on RIGHT_NOW ... " note to the user in question. - Password change automatically triggers deleting of the corresponding entry in pasword_change_request table. - Password change automatically logs-in the user as well. This way, users weren't bothered by accidental password changes, and they also understood the system better than the one with automatically generated passwords. For additional security, we also built some brakes that assured that the number of attempts to change the password, such as: - "An email with instructions was sent to you X minutes ago. In case you don't receive it within N hours, please contact the site administrator" - "According to our logs, you have visited the CFP page more than NMAX times within last 24 hours. For security reasons, you will be denied access to this page for the next 24 hours. Please contact the site administrator." hope this helps... ------------------------------------------------------------------------ March 30, 2005 - 21:01 : Jose A Reyero Attachment: http://drupal.org/files/issues/user_one_time_login.patch (6.52 KB) This patch for -cvs- addresses this issue. It generates an *only one use* URL to login into user account, instead of a new password. Then the user clicks on the URL, logs in, and may change his old password if desired. It is a only one use URL, and has some additional security measures, like a configurable time-out -can be confgured in admin/user/settings- and also, if somebody uses it or password is changed in the meanwhile, previously generated login URLs of this type won't work. About how it works, it creates a new url with: - user id, - timestamp - a hash of current timestamp, changed date for account, and the old password hashed With this patch, this wont work for admin accounts, which I think is better, you never know... It has a number of features like: - User can be ignoring these emails if somebody else is requesting access, a new password request wont change current password. However, the user will be warned when seeing the emails - It reveals no sensible information about the user account in the URL itself - the generated hash is not predictable at all if you dont know the old password, as it depends on user's old password - there's a configurable timeout - The URL's are only one use, as they depend on two timestamps, one of which will be changing in case this login is used -or in case user logs in for other means - Any 'new password' request is independent of others, so no one can be interfering with one user asking for his password back. Also doesn't require any new db field nor keeping track of requested passwords. ------------------------------------------------------------------------ March 30, 2005 - 21:41 : chx Very nice patch. Please consider for 4.6. ------------------------------------------------------------------------ March 30, 2005 - 23:16 : drumm Couldn't those nested if statements be made into one with a bit cleaner indentation? It looks like the page which the user arrives at after getting the new login information is their user page, not a page to set the password. The "Time out for password recovery e-mail" configuration is very unfriendly. It should be a drop down select of the most used human readable durations. Or we could just decide on one timeout and leave that hard coded in; who actually needs to use that setting? The url is quite long, what could be done to make it shorter? ------------------------------------------------------------------------ March 31, 2005 - 00:50 : Jose A Reyero /Couldn't those nested if statements be made into one with a bit cleaner indentation?/ Maybe, but I had to split that if conditions in two parts because the $account var didn't get the value before the rest of the conditions -latest two- where evaluated. Someone can help with PHP here? /It looks like the page which the user arrives at after getting the new login information is their user page, not a page to set the password. The "Time out for password recovery e-mail" configuration is very unfriendly. It should be a drop down select of the most used human readable durations. Or we could just decide on one timeout and leave that hard coded in; who actually needs to use that setting?/ Agreed, will be polishing this a little bit, just wating for some more suggestions... For the duration, I'll change to 'hours', ok? Just don't like to restrict options using unneccessary dropdowns, it's only intended for administrators anyway. Maybe I add also a '0' for unlimited. Hardcoding it, would need anyway to be mentioned in the module help, so it's about the same overhead, both for coding and for the admin interface, and harcoding in two places in code is not good anyway. And I like having options, am I the only one? / The url is quite long, what could be done to make it shorter?/ Well, the url has to be long, it has neccessary params and an MD5 hash, which yes, could be cut someway, but that adding inneccessary code and also reducing security -though half an MD5 hash may be secure enough- but I see no point in that. ------------------------------------------------------------------------ March 31, 2005 - 02:18 : FactoryJoe(a)civicspacelabs.org "Or we could just decide on one timeout and leave that hard coded in; who actually needs to use that setting? " +1 for making this *not* configurable! C'mon, Drupal should have standards about security -- and if best practices say that, say, 8 hours, is a good amount of time for that URL to be active, then do that. Cut out the confusion and extra brain juice it takes to decide... "Gee, is 4 hours better than 5?" Additionally, there's a simple way to make this workflow better for the user. If you visit an outdated change password URL, you should be told, "Sorry, but that link you followed to change your password has expired. If you would like to reset your password, please fill out this form: [form] If you did not request to change your password, please notify [administrator's name w/ PM link]". This way, if you missed your change password deadline, you can request your password again right from the timeout page. If you didn't request the change, it will be obvious that someone else was trying to change it, and then you can contact the administrator and find out who it was that was messin' with your account. ------------------------------------------------------------------------ March 31, 2005 - 06:55 : chx Attachment: http://drupal.org/files/issues/user_one_time_login_0.patch (4.72 KB) I cleaned up the patch a bit. I hope code style is OK. So may -- there are no checks against the arguments? There is no need. user_load uses %d for 'uid', so that's dealt with. Comparison by < and > means an implicit cast to integer, so $timestamp is also dealt with automatically. I do not think timeout needs to be configured. Please comment on not letting uid 1 using this one. I am not sure whether this is necessary. ------------------------------------------------------------------------ March 31, 2005 - 07:12 : chx Attachment: http://drupal.org/files/issues/user_one_time_login_1.patch (6.21 KB) Implemented Chris' idea, so if you use a one time URl which is expired, you are sent to user/password with a message to ask for another. I changed a few messages and deleted the "not for admin" feature. Do we really want one gazillion support requests "I locked myself out of my site, user/password tells me not for admin, what now"? ------------------------------------------------------------------------ March 31, 2005 - 08:24 : chx One more point on uid 1. Yes, uid 1 can reset his pwd over the database. But then we would need to lock out administer users privileged users. Then administer nodes, 'cos of XSS. Then administer comments... then administer filters... OK, this is pointless. If someone can eavesdrop on your emails, everything is lost, I think. This is not something Drupal shall address. Maybe we could employ a security question -- secret answer pair. ------------------------------------------------------------------------ March 31, 2005 - 11:52 : Bèr Kessels Eventhough I am a less-settings advocate, I think this must remain a setting :). Why? Sites on Drupal differ a lot, and have a very differnet userbase. I run very low level sites, where it sometimes takes days before someone will read his/her mail. But I also know sites (drupal.org) where days would certainly be too long. Sorry Chris et al, I +1 on configurable duration. (hell, we really need some about:config alike screen in drupal) ------------------------------------------------------------------------ March 31, 2005 - 12:46 : Dries IMO, that setting has nothing to do with a site being "low level". If you request a new password, you probably want to login ASAP. I vote against a configuration option. Either way, this patch needs work: 1. We don't glue words together (eg. $hashedpass, $currenttime, $loginurl). Similarly, I suggest to change 'user/newpassword' to 'user/reset' or 'user/request-password'. 2. The patch exposes technical details to the user; terminology like "one time login URL" is likely to confuse many users. 3. The login URL is not clean URL safe, I think. 4. Please rename the variable $pass1 in user_pass_rehash(). 5. The code comments are often inconsistent. Some end with a punctuation, some don't. Some span two lines, others don't. On a related note, can't we reuse this system for the initial password? That is, instead of mailing a password, we mail an URL and force the user to enter a password.

1 0

[drupal-devel] improving the book module?
by Anisa 31 Mar '05

31 Mar '05

Is there anyone interested in improving the book module and can help out codewise? There are some ideas being bounced around on the docs list, such as offline editing, editing multiple pages at once (moving, deleting, etc), reducing the number of steps it takes to edit etc. But without the code to back it up, people are really just fussing at each other. ^.^; Anisa.

2 2

[drupal-devel] [feature] Request New Password Security
by Dries 31 Mar '05

31 Mar '05

Issue status update for http://drupal.org/node/18719 Project: Drupal Version: cvs Component: user.module Category: feature requests Priority: critical Assigned to: Anonymous Reported by: neofactor Updated by: Dries Status: patch IMO, that setting has nothing to do with a site being "low level". If you request a new password, you probably want to login ASAP. I vote against a configuration option. Either way, this patch needs work: 1. We don't glue words together (eg. $hashedpass, $currenttime, $loginurl). Similarly, I suggest to change 'user/newpassword' to 'user/reset' or 'user/request-password'. 2. The patch exposes technical details to the user; terminology like "one time login URL" is likely to confuse many users. 3. The login URL is not clean URL safe, I think. 4. Please rename the variable $pass1 in user_pass_rehash(). 5. The code comments are often inconsistent. Some end with a punctuation, some don't. Some span two lines, others don't. On a related note, can't we reuse this system for the initial password? That is, instead of mailing a password, we mail an URL and force the user to enter a password. Dries Previous comments: ------------------------------------------------------------------------ March 11, 2005 - 04:52 : neofactor Problem: Any user can force another user's password to change... simply by selecting "request new password" and putting in their username. The user gets an email with the new.. but this feels like a violation to the user... and a pain. Solution? If someone requests a new password... Don't blindly change it... send an email that says...."Is this a real request authorized by you? Click here to confirm otherwise disregard this message" Please consider this critical for user by-in to Drupal as a secure system. I appreciate your consideration. http://drupal.org/node/18689 ------------------------------------------------------------------------ March 11, 2005 - 06:05 : neofactor I added some code to prevent the admin account from being reset... Please add as a patch. if ($account->uid == 1 { unset($account); form_set_error('name', t('Sorry. The username %name is not allowed to be changed.', array('%name' => ''. $edit['name'] .''))); } // Just above this code on line 911: if ($account) { $from = variable_get('site_mail', ini_get('sendmail_from')); $pass = user_password(); ------------------------------------------------------------------------ March 11, 2005 - 06:08 : killes(a)www.drop.org Please only set to "patch" if you are actually submitting one. The solution you proposed is rather a workaround and will not be acceptable for core. ------------------------------------------------------------------------ March 11, 2005 - 10:21 : Deno I was in charge of a web site with >10000 paying customers for some time, and the "customers care" was constantly troubled with pasword change requests, until we switched to a system that worked in the following way: 1) "I forgot the password" function accepts either users email, or users login name, generates a random phrase, and adds an entry to "pasword_change_request" table. This entry consists of: - User ID (corresponding to email or login) - random phrase - datestamp User ID is unique key, and the entries in this table older than MAX_TIME are regularly purged by a cron job. 2) System sends an email with a link to "change forgotten password" page with this random phrase as argument to the user. That would be something like: https://drupal.org/user/cfp/d438rwiuodw894732ehdw 3) When user follows the link, "cfp" page checks if the random phrase exists in the table, allows the user to immediately change the password, and sends another email with "your password was changed on RIGHT_NOW ... " note to the user in question. - Password change automatically triggers deleting of the corresponding entry in pasword_change_request table. - Password change automatically logs-in the user as well. This way, users weren't bothered by accidental password changes, and they also understood the system better than the one with automatically generated passwords. For additional security, we also built some brakes that assured that the number of attempts to change the password, such as: - "An email with instructions was sent to you X minutes ago. In case you don't receive it within N hours, please contact the site administrator" - "According to our logs, you have visited the CFP page more than NMAX times within last 24 hours. For security reasons, you will be denied access to this page for the next 24 hours. Please contact the site administrator." hope this helps... ------------------------------------------------------------------------ March 30, 2005 - 22:01 : Jose A Reyero Attachment: http://drupal.org/files/issues/user_one_time_login.patch (6.52 KB) This patch for -cvs- addresses this issue. It generates an *only one use* URL to login into user account, instead of a new password. Then the user clicks on the URL, logs in, and may change his old password if desired. It is a only one use URL, and has some additional security measures, like a configurable time-out -can be confgured in admin/user/settings- and also, if somebody uses it or password is changed in the meanwhile, previously generated login URLs of this type won't work. About how it works, it creates a new url with: - user id, - timestamp - a hash of current timestamp, changed date for account, and the old password hashed With this patch, this wont work for admin accounts, which I think is better, you never know... It has a number of features like: - User can be ignoring these emails if somebody else is requesting access, a new password request wont change current password. However, the user will be warned when seeing the emails - It reveals no sensible information about the user account in the URL itself - the generated hash is not predictable at all if you dont know the old password, as it depends on user's old password - there's a configurable timeout - The URL's are only one use, as they depend on two timestamps, one of which will be changing in case this login is used -or in case user logs in for other means - Any 'new password' request is independent of others, so no one can be interfering with one user asking for his password back. Also doesn't require any new db field nor keeping track of requested passwords. ------------------------------------------------------------------------ March 30, 2005 - 22:41 : chx Very nice patch. Please consider for 4.6. ------------------------------------------------------------------------ March 31, 2005 - 00:16 : drumm Couldn't those nested if statements be made into one with a bit cleaner indentation? It looks like the page which the user arrives at after getting the new login information is their user page, not a page to set the password. The "Time out for password recovery e-mail" configuration is very unfriendly. It should be a drop down select of the most used human readable durations. Or we could just decide on one timeout and leave that hard coded in; who actually needs to use that setting? The url is quite long, what could be done to make it shorter? ------------------------------------------------------------------------ March 31, 2005 - 01:50 : Jose A Reyero /Couldn't those nested if statements be made into one with a bit cleaner indentation?/ Maybe, but I had to split that if conditions in two parts because the $account var didn't get the value before the rest of the conditions -latest two- where evaluated. Someone can help with PHP here? /It looks like the page which the user arrives at after getting the new login information is their user page, not a page to set the password. The "Time out for password recovery e-mail" configuration is very unfriendly. It should be a drop down select of the most used human readable durations. Or we could just decide on one timeout and leave that hard coded in; who actually needs to use that setting?/ Agreed, will be polishing this a little bit, just wating for some more suggestions... For the duration, I'll change to 'hours', ok? Just don't like to restrict options using unneccessary dropdowns, it's only intended for administrators anyway. Maybe I add also a '0' for unlimited. Hardcoding it, would need anyway to be mentioned in the module help, so it's about the same overhead, both for coding and for the admin interface, and harcoding in two places in code is not good anyway. And I like having options, am I the only one? / The url is quite long, what could be done to make it shorter?/ Well, the url has to be long, it has neccessary params and an MD5 hash, which yes, could be cut someway, but that adding inneccessary code and also reducing security -though half an MD5 hash may be secure enough- but I see no point in that. ------------------------------------------------------------------------ March 31, 2005 - 03:18 : FactoryJoe(a)civicspacelabs.org "Or we could just decide on one timeout and leave that hard coded in; who actually needs to use that setting? " +1 for making this *not* configurable! C'mon, Drupal should have standards about security -- and if best practices say that, say, 8 hours, is a good amount of time for that URL to be active, then do that. Cut out the confusion and extra brain juice it takes to decide... "Gee, is 4 hours better than 5?" Additionally, there's a simple way to make this workflow better for the user. If you visit an outdated change password URL, you should be told, "Sorry, but that link you followed to change your password has expired. If you would like to reset your password, please fill out this form: [form] If you did not request to change your password, please notify [administrator's name w/ PM link]". This way, if you missed your change password deadline, you can request your password again right from the timeout page. If you didn't request the change, it will be obvious that someone else was trying to change it, and then you can contact the administrator and find out who it was that was messin' with your account. ------------------------------------------------------------------------ March 31, 2005 - 07:55 : chx Attachment: http://drupal.org/files/issues/user_one_time_login_0.patch (4.72 KB) I cleaned up the patch a bit. I hope code style is OK. So may -- there are no checks against the arguments? There is no need. user_load uses %d for 'uid', so that's dealt with. Comparison by < and > means an implicit cast to integer, so $timestamp is also dealt with automatically. I do not think timeout needs to be configured. Please comment on not letting uid 1 using this one. I am not sure whether this is necessary. ------------------------------------------------------------------------ March 31, 2005 - 08:12 : chx Attachment: http://drupal.org/files/issues/user_one_time_login_1.patch (6.21 KB) Implemented Chris' idea, so if you use a one time URl which is expired, you are sent to user/password with a message to ask for another. I changed a few messages and deleted the "not for admin" feature. Do we really want one gazillion support requests "I locked myself out of my site, user/password tells me not for admin, what now"? ------------------------------------------------------------------------ March 31, 2005 - 09:24 : chx One more point on uid 1. Yes, uid 1 can reset his pwd over the database. But then we would need to lock out administer users privileged users. Then administer nodes, 'cos of XSS. Then administer comments... then administer filters... OK, this is pointless. If someone can eavesdrop on your emails, everything is lost, I think. This is not something Drupal shall address. Maybe we could employ a security question -- secret answer pair. ------------------------------------------------------------------------ March 31, 2005 - 12:52 : Bèr Kessels Eventhough I am a less-settings advocate, I think this must remain a setting :). Why? Sites on Drupal differ a lot, and have a very differnet userbase. I run very low level sites, where it sometimes takes days before someone will read his/her mail. But I also know sites (drupal.org) where days would certainly be too long. Sorry Chris et al, I +1 on configurable duration. (hell, we really need some about:config alike screen in drupal)

1 0

[drupal-devel] [feature] Request New Password Security
by Bèr Kessels 31 Mar '05

31 Mar '05

Issue status update for http://drupal.org/node/18719 Project: Drupal Version: cvs Component: user.module Category: feature requests Priority: critical Assigned to: Anonymous Reported by: neofactor Updated by: Bèr Kessels Status: patch Eventhough I am a less-settings advocate, I think this must remain a setting :). Why? Sites on Drupal differ a lot, and have a very differnet userbase. I run very low level sites, where it sometimes takes days before someone will read his/her mail. But I also know sites (drupal.org) where days would certainly be too long. Sorry Chris et al, I +1 on configurable duration. (hell, we really need some about:config alike screen in drupal) Bèr Kessels Previous comments: ------------------------------------------------------------------------ March 11, 2005 - 03:52 : neofactor Problem: Any user can force another user's password to change... simply by selecting "request new password" and putting in their username. The user gets an email with the new.. but this feels like a violation to the user... and a pain. Solution? If someone requests a new password... Don't blindly change it... send an email that says...."Is this a real request authorized by you? Click here to confirm otherwise disregard this message" Please consider this critical for user by-in to Drupal as a secure system. I appreciate your consideration. http://drupal.org/node/18689 ------------------------------------------------------------------------ March 11, 2005 - 05:05 : neofactor I added some code to prevent the admin account from being reset... Please add as a patch. if ($account->uid == 1 { unset($account); form_set_error('name', t('Sorry. The username %name is not allowed to be changed.', array('%name' => ''. $edit['name'] .''))); } // Just above this code on line 911: if ($account) { $from = variable_get('site_mail', ini_get('sendmail_from')); $pass = user_password(); ------------------------------------------------------------------------ March 11, 2005 - 05:08 : killes(a)www.drop.org Please only set to "patch" if you are actually submitting one. The solution you proposed is rather a workaround and will not be acceptable for core. ------------------------------------------------------------------------ March 11, 2005 - 09:21 : Deno I was in charge of a web site with >10000 paying customers for some time, and the "customers care" was constantly troubled with pasword change requests, until we switched to a system that worked in the following way: 1) "I forgot the password" function accepts either users email, or users login name, generates a random phrase, and adds an entry to "pasword_change_request" table. This entry consists of: - User ID (corresponding to email or login) - random phrase - datestamp User ID is unique key, and the entries in this table older than MAX_TIME are regularly purged by a cron job. 2) System sends an email with a link to "change forgotten password" page with this random phrase as argument to the user. That would be something like: https://drupal.org/user/cfp/d438rwiuodw894732ehdw 3) When user follows the link, "cfp" page checks if the random phrase exists in the table, allows the user to immediately change the password, and sends another email with "your password was changed on RIGHT_NOW ... " note to the user in question. - Password change automatically triggers deleting of the corresponding entry in pasword_change_request table. - Password change automatically logs-in the user as well. This way, users weren't bothered by accidental password changes, and they also understood the system better than the one with automatically generated passwords. For additional security, we also built some brakes that assured that the number of attempts to change the password, such as: - "An email with instructions was sent to you X minutes ago. In case you don't receive it within N hours, please contact the site administrator" - "According to our logs, you have visited the CFP page more than NMAX times within last 24 hours. For security reasons, you will be denied access to this page for the next 24 hours. Please contact the site administrator." hope this helps... ------------------------------------------------------------------------ March 30, 2005 - 21:01 : Jose A Reyero Attachment: http://drupal.org/files/issues/user_one_time_login.patch (6.52 KB) This patch for -cvs- addresses this issue. It generates an *only one use* URL to login into user account, instead of a new password. Then the user clicks on the URL, logs in, and may change his old password if desired. It is a only one use URL, and has some additional security measures, like a configurable time-out -can be confgured in admin/user/settings- and also, if somebody uses it or password is changed in the meanwhile, previously generated login URLs of this type won't work. About how it works, it creates a new url with: - user id, - timestamp - a hash of current timestamp, changed date for account, and the old password hashed With this patch, this wont work for admin accounts, which I think is better, you never know... It has a number of features like: - User can be ignoring these emails if somebody else is requesting access, a new password request wont change current password. However, the user will be warned when seeing the emails - It reveals no sensible information about the user account in the URL itself - the generated hash is not predictable at all if you dont know the old password, as it depends on user's old password - there's a configurable timeout - The URL's are only one use, as they depend on two timestamps, one of which will be changing in case this login is used -or in case user logs in for other means - Any 'new password' request is independent of others, so no one can be interfering with one user asking for his password back. Also doesn't require any new db field nor keeping track of requested passwords. ------------------------------------------------------------------------ March 30, 2005 - 21:41 : chx Very nice patch. Please consider for 4.6. ------------------------------------------------------------------------ March 30, 2005 - 23:16 : drumm Couldn't those nested if statements be made into one with a bit cleaner indentation? It looks like the page which the user arrives at after getting the new login information is their user page, not a page to set the password. The "Time out for password recovery e-mail" configuration is very unfriendly. It should be a drop down select of the most used human readable durations. Or we could just decide on one timeout and leave that hard coded in; who actually needs to use that setting? The url is quite long, what could be done to make it shorter? ------------------------------------------------------------------------ March 31, 2005 - 00:50 : Jose A Reyero /Couldn't those nested if statements be made into one with a bit cleaner indentation?/ Maybe, but I had to split that if conditions in two parts because the $account var didn't get the value before the rest of the conditions -latest two- where evaluated. Someone can help with PHP here? /It looks like the page which the user arrives at after getting the new login information is their user page, not a page to set the password. The "Time out for password recovery e-mail" configuration is very unfriendly. It should be a drop down select of the most used human readable durations. Or we could just decide on one timeout and leave that hard coded in; who actually needs to use that setting?/ Agreed, will be polishing this a little bit, just wating for some more suggestions... For the duration, I'll change to 'hours', ok? Just don't like to restrict options using unneccessary dropdowns, it's only intended for administrators anyway. Maybe I add also a '0' for unlimited. Hardcoding it, would need anyway to be mentioned in the module help, so it's about the same overhead, both for coding and for the admin interface, and harcoding in two places in code is not good anyway. And I like having options, am I the only one? / The url is quite long, what could be done to make it shorter?/ Well, the url has to be long, it has neccessary params and an MD5 hash, which yes, could be cut someway, but that adding inneccessary code and also reducing security -though half an MD5 hash may be secure enough- but I see no point in that. ------------------------------------------------------------------------ March 31, 2005 - 02:18 : FactoryJoe(a)civicspacelabs.org "Or we could just decide on one timeout and leave that hard coded in; who actually needs to use that setting? " +1 for making this *not* configurable! C'mon, Drupal should have standards about security -- and if best practices say that, say, 8 hours, is a good amount of time for that URL to be active, then do that. Cut out the confusion and extra brain juice it takes to decide... "Gee, is 4 hours better than 5?" Additionally, there's a simple way to make this workflow better for the user. If you visit an outdated change password URL, you should be told, "Sorry, but that link you followed to change your password has expired. If you would like to reset your password, please fill out this form: [form] If you did not request to change your password, please notify [administrator's name w/ PM link]". This way, if you missed your change password deadline, you can request your password again right from the timeout page. If you didn't request the change, it will be obvious that someone else was trying to change it, and then you can contact the administrator and find out who it was that was messin' with your account. ------------------------------------------------------------------------ March 31, 2005 - 06:55 : chx Attachment: http://drupal.org/files/issues/user_one_time_login_0.patch (4.72 KB) I cleaned up the patch a bit. I hope code style is OK. So may -- there are no checks against the arguments? There is no need. user_load uses %d for 'uid', so that's dealt with. Comparison by < and > means an implicit cast to integer, so $timestamp is also dealt with automatically. I do not think timeout needs to be configured. Please comment on not letting uid 1 using this one. I am not sure whether this is necessary. ------------------------------------------------------------------------ March 31, 2005 - 07:12 : chx Attachment: http://drupal.org/files/issues/user_one_time_login_1.patch (6.21 KB) Implemented Chris' idea, so if you use a one time URl which is expired, you are sent to user/password with a message to ask for another. I changed a few messages and deleted the "not for admin" feature. Do we really want one gazillion support requests "I locked myself out of my site, user/password tells me not for admin, what now"? ------------------------------------------------------------------------ March 31, 2005 - 08:24 : chx One more point on uid 1. Yes, uid 1 can reset his pwd over the database. But then we would need to lock out administer users privileged users. Then administer nodes, 'cos of XSS. Then administer comments... then administer filters... OK, this is pointless. If someone can eavesdrop on your emails, everything is lost, I think. This is not something Drupal shall address. Maybe we could employ a security question -- secret answer pair.

1 0

[drupal-devel] [task] Drupal Install System
by adrian 31 Mar '05

31 Mar '05

Issue status update for http://drupal.org/node/5496 Project: Drupal Version: cvs Component: base system Category: tasks Priority: normal Assigned to: adrian Reported by: adrian Updated by: adrian Status: patch Yeah. I've been planning that from the start (before it had a fancy name), I'm currently planning to have it happen around phase four or five. adrian Previous comments: ------------------------------------------------------------------------ January 29, 2004 - 11:10 : adrian I am in the process of developing a new Install API for the drupal core, which will allow a completely web based install / set up process, as well as allow contributed modules to integrate more directly into this system. A more detailed description of the system is available in this thread. This task has been created to place each of the successive patches in the patch queue. ------------------------------------------------------------------------ January 29, 2004 - 11:20 : adrian Attachment: http://drupal.org/files/issues/install_api_first_level.patch (6.42 KB) Kjartan correctly identified one of the problems in creating a unified install system. There's a lot of code that gets run by simply including the core files (bootstrap.inc, database.inc, session.inc and common.inc). A lot of this code relies upon the database connection going off without a hitch, like for instance the current system variables, themes and session code. To work around this i created a new drupal_init() function, which needs to be called when you want the system to initialize. drupal_init has a 'db_init' argument, which allows you to bypass the loading of all the database related code with a simple flag. install.php needs to run drupal core, while ignoring any database connections .. until atleast the point where the configuration settings for the database are entered, and confirmed to work. There is a new function created called 'database_connected()' , which returns a boolean true only if the database both connected and the table was selected successfully. This patch does not have a suitable error message for index.php upon not being able to make a database connection ,but that message should very likely be discussed and finalized. In the next level of the patch I will introduce the major changes, or rather additions. I haven't modified any existing files beyond the ones in this patch (at this time). ------------------------------------------------------------------------ January 29, 2004 - 14:24 : Kjartan This patch has some serious flaws though. <?php include_once "includes/bootstrap.inc"; -drupal_page_header(); include_once "includes/common.inc"; ?> By removing the drupal_page_header() you break the whole point of bootstrap as the common.inc stuff is included when its not necessary. It needs some more thought. Maybe you should take the time to make a workflow of the current Drupal init process before modifying it. Also the naming of constants, I would prefer calling them DATABASE_ATTEMPT as it makes it easier to tell them appart. ------------------------------------------------------------------------ February 9, 2004 - 22:18 : adrian I have reworked the patch into a very minimal set of requirements. To prevent the .inc files from running any code when you don't want them to , you 'define' a constant called 'DRUPAL_NO_INIT' before including bootstrap.inc. This relatively small patch is required for the install stuff to work. ------------------------------------------------------------------------ February 9, 2004 - 22:22 : adrian Attachment: http://drupal.org/files/issues/drupal_no_init_flag.patch (3.88 KB) i'll even attach it this time. ------------------------------------------------------------------------ February 9, 2004 - 22:48 : adrian Attachment: http://drupal.org/files/issues/database_init_return.patch (1.96 KB) The following patch is a modification to the includes/database*.inc set of files, to make db_connect return a boolean value depending on wether the connection was successfully made. I have changed database.inc to die() upon connection error with the message "Database Connection Unsuccessful". Cleaning up that message and placing a help page might be a good idea from an user experience point of view. This patch is required for the install system, in that it needs to test wether it has made a successfull database connection before it allows you to actually install the schema. ------------------------------------------------------------------------ February 10, 2004 - 03:15 : adrian Attachment: http://drupal.org/files/issues/install_system_alpha.tar.bz2 (11.59 KB) This is an alpha version of the new install API code. It is completely capable of installing the drupal core system without requiring any interaction with the database side. There are currently 5 configuration screens , but the API is sound for us to add / remove / change these screens as we need. In it's current state, it isn't capable of updating the base system, however the backend stubs exist, and have been tested. In the push to get the install process running smoothly i simply disabled the update interface. The previous 2 patches on this node need to be applied, but have been combined into install_system_alpha.patch which is included in this distribution. The rest of the files are new , and should not interfere with the drupal core at all. They are includes/install.inc ... the file that hosts the install api. install.php ... the front end script that initializes the install api (a total of 5 lines) install/system.install ... the install module for the core. It contains the bulk of code from update.php, and all the configuration information needed to successfully install the core. In the future , the install api will be able to easily install any contrib module through the modulename.install file that will hopefully become well supported. Note: I need to update the postgres compatibility in this .install file, and as such only mysql is fully tested now. I have posted some screenshots at my site ------------------------------------------------------------------------ February 10, 2004 - 20:22 : adrian Unconed just pointed out there is a conflict with windows installations in that there can not be an 'INSTALL' file and an 'install' directory (which contains the system.install currently) He suggested that we rename the README and INSTALL files to README.txt and INSTALL.txt. I personally move that we store the .install files within the modules directory , as in the future installing a contributed module should be as simple as dropping the module directory (containing .install , .module and whichever external files) into modules/ , and then installing the module within drupal. ------------------------------------------------------------------------ February 12, 2004 - 07:10 : moshe weitzman I played around with the patch, and successfully installed a new Drupal instance on the fitst try. Nice work, Adrian! I reviewed the code too, and found it quite Drupalish and satsfactory. Here are some notes from my install: - On the configuration file page, I selected from the radio button options a file which was not conf.php and thus did not exist. I strugged with the 'file is not writeable' error for a while until I realized that you expected me to create the file and then the installer would edit it ... in general, this page is a pretty complex introduction into drupal. I suggest simply assuming 'conf.php' here. I haven't thought about this much though. - Similar to above, I figured Drupal would create a database for me. Not so, I had to create it and then Drupal takes over. Some simple help text adddresses this point. - Use form_password() or similar when requesting a database password - The default $base_url was missing a slall for me. It said 'http://medrinstall' instead of 'http://me/drinstall' - After completing the DB install, there is no page telling you what to do next. A link to the home page is all thats needed IMO. -The site_name chosen during the install was not saved in the 'variable' table. - When you return to install.php some time later, you have no indication what a working site already exists. Not sure thats the right thing to do. Not a big deal though. P.S. Adrians suggestion to store the .install files in module specific subdirs under modules makes sense to me. Thus system.install would move to modules/system/system.install ------------------------------------------------------------------------ February 13, 2004 - 06:04 : skip I had to set up a pMachine blog yesterday for a client. I have to say... it was the easiest install process I've ever gone through (CMS-wise). Very user friendly. Lots of feedback to the user. If you haven't seen it before, you should check it out for ideas. ------------------------------------------------------------------------ February 18, 2004 - 07:26 : adrian Attachment: http://drupal.org/files/issues/install_phase2.tar.bz2 (12.96 KB) So I have spent far too much time over the last few weeks battling with this install system. My current results are in this here tarball. This time I am even less sure of it fully working in all cases, than last time. With this release I added extensive error checking to the install process, finished MOST of what is needed for the update process , but most importantly... it is a lot more secure than previously, and is specifically built to allow people to host their multiple drupal installs from just a single root. I have handled this by adding a global variable to the config file (allow_override_config) , which is checked by the install script before a new configuration file is made that would override the currently active one. There are three settings to this : 'no' , 'any' and 'auth'. I feel auth is generally the best option, as it requires anyone trying to override a working configuration file to enter the username and password for the site he is trying to override. Added checking also exists for the minimum requirements of running a drupal system (the tests probably need to be fleshed out), aswell as refusing to install a database dump into a db_url/db_prefix which seems to have a valid site installed. When a working configuration is detected on startup , the default action is to require the administrator user of the site to log in. This is for security reasons. The install script has lots of code that stops it from ever overwriting any config file that has already been created, but as I said .. it allows for the creation of a config file that will be parsed before the current valid install, but only with the express permission of that config. Config files have been moved to './conf/' , and the default config has been renamed to 'conf/default.php'. I have split install.inc into install.inc and install.module, as there was a lot of interface code specifically for the update/install sequence (and it is best available for reuse .. so i didnt wanna put it in install.php). This has an unrelated quirk for xtemplate .. I left it in the patch because for some reason I was unable to get a working default theme because xtemplate refused to populate the 'path' template variable. I also have an extensive 'gallery' of the new changes if you just want to see what it looks like. Directions for use : untar in a drupal cvs tree , and 'patch -p0 < install_alpha_2.patch' go to http://mysite/install.php ... previous configs in includes/ will be ignored for extra fun , create a hostname entry to sitea.siteb.sitec.sited.mysite , and watch the install script pick up the valid configs. ------------------------------------------------------------------------ February 18, 2004 - 17:14 : TDobes Adrian, thanks for all your work on the install system... I'll check it out once I get home this evening. Just FYI, here's a link to the XTemplate bug [1] you mentioned. (It's a simple two-line patch.) [1] http://drupal.org/node/view/5858 ------------------------------------------------------------------------ June 20, 2004 - 22:29 : adrian taking out of patch queue. Still on my todo list, but I have to get some other things done first. ------------------------------------------------------------------------ July 27, 2004 - 17:25 : JonBob The install system should (will?) handle version numbers. Some related issues to examine: None of these are really duplicates of one another, so while I'd like to close them I can't. Consider this a reminder to do so if and when the install system addresses the problems. New 'version' system field and Drupal version [2] Module dependencies [3] Show drupal version to admins [4] Meta tag with drupal version [5] Version number [6] [2] http://drupal.org/node/view/5811 [3] http://drupal.org/node/view/8402 [4] http://drupal.org/node/view/2986 [5] http://drupal.org/node/view/2828 [6] http://drupal.org/node/view/8762 ------------------------------------------------------------------------ March 13, 2005 - 18:03 : publian It made it based on install_phase2. It makes it with drupal CVS as of 2005.03.14. The improvement point is as follows. 1) The installation query command execution read database/database.mysql or database/database.pgsql. 2) Two head bytes of $_SERVER 'HTTP_ACCEPT_LANGUAGE' were seen, and the installation language was distinguished automatically. This sees the install.lang file in the languege folder. (funcution lt() like funcution t()) Making) 3)　In the installation process, the mother tongue can have been selected. (The import is done from the file in the translations folder. ) 4) Additionally, a detailed place was corrected. The bug moves though thinks many still. -- Drupal Japan ------------------------------------------------------------------------ March 13, 2005 - 18:10 : publian Attachment: http://drupal.org/files/issues/install_phase3.tar.gz (134.72 KB) It is as for appending. ------------------------------------------------------------------------ March 26, 2005 - 10:11 : judah I was just about to start creating an install system for modules when someone mentioned you were already working on one. Here is my design specs, http://drupal.org/node/19427 I could not contact you via any other means so please advise what the current status is on this. Thanks ------------------------------------------------------------------------ March 31, 2005 - 01:13 : adrian Attachment: http://drupal.org/files/issues/install_api_phase_1.tar.bz2 (21.49 KB) Attached is the first iteration of my new install system. To install : Extract tarball into a clean checkout, and apply the install_api_phase_1.patch file. This patch implements the following : Core install api. /(includes/install.inc)/ Modifications to drupal_load that disables a module that has an out of data schema. /(includes/bootstrap.inc)/ Does not allow modules to be enabled unless they have been installed, or updated and the allow for them to be installed and updated directly from admin/modules. /(modules/system.module)/ This patch does NOT : Install or update Drupal core, however it is possible to do so with the api (and I have done so before), significant changes to Drupal need to happen before it is practical to handle core with it. Description : This is the first phase of a four or five phase project, so it's not meant to solve the whole problem. I am writing a post for the Drupal forums at present detailing the progress with the install system, and directions I want to take it. To enable a module to be installable, you add an implementation of hook_info() to it , i.e. : <?php function buddylist_info($key) { $info['schema'] = 1; return $info[$key]; } ?> This tells the system that a schema version of 1 is required to operate this module. You then create a file called modulename.install (preferably in the same directory in the module, although it doesn't really matter. You then need to specify which schema version it implements by : <?php function buddylist_schema($key) { $info['version'] = 1; return $info[$key]; } ?> Although, in the current implementation this function is optional when you only have a single schema version (the default is 1). Then you implement your hook_install(). <?php function buddylist_install() { $query = " CREATE TABLE {buddylist} ( uid int(10) UNSIGNED NOT NULL, buddy int(10) UNSIGNED NOT NULL, timestamp int(11) NOT NULL, PRIMARY KEY (uid, buddy) )"; db_query($query); db_query("ALTER TABLE {buddylist} ADD received tinyint(1) NOT NULL;"); } ?> This function would be called for any database type that has not specified it's own function. To add Postgres support, the function would be called buddylist_install_pgsql();. If the schema of your module ever changes, to allow for it to be updated, you proceed as follows : increment the 'schema' value in your hook_info() function. increment the 'version' value in your hook_schema function, within your modulename.install file. modify your schema as required, by changing it in the hook_install() function. provide an update script for users of the previous version. The following is an example of an update script. <?php function buddylist_update_1() { db_query("ALTER TABLE .... ..... "); } ?> These updates can also be database specific (ie: buddylist_update_1_pgsql() etc). When a module needs to be updated, it will execute each of the functions in order (exactly like one would in updates.inc currently.) I have included copies of the Trackback and Buddylist modules for testing. ------------------------------------------------------------------------ March 31, 2005 - 03:26 : FactoryJoe(a)civicspacelabs.org No idea if this is of any use, but I thought it would be cool if there were feeds for each module: http://www.speirs.org/appcasting/ Drumm calls it RSS for RSS' sake. Perhaps, but it's worth considering.

1 0

[drupal-devel] [bug] Clean up plain-text checking
by Steven 31 Mar '05

31 Mar '05

Issue status update for http://drupal.org/node/18817 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: normal Assigned to: Steven Reported by: Steven Updated by: Steven Status: patch Attachment: http://drupal.org/files/issues/check.patch (91.83 KB) Back story Bart Jansens brought up some possible XSS issues recently which prompted me to reevaluate the text checking in Drupal. The big issue is "plain text": what a user typically enters in single-line boxes. When this text is put into HTML, special characters like < > and & need to be escaped. When it is put into an HTML attribute, " needs to be escaped too. Note that it is not about filtering (what check_output() does), but about handling plain-text (commonly associated with single-line editboxes). The current 'standard' in Drupal is to do escaping when outputting the titles in HTML, and keep the plain-text in the database. We should keep this as it is in line with "keep exactly what the user typed and process it later". Furthermore it is hard to undo this escaping, which is needed when communicating with other services or when exporting data. This brings up the problem: where should text be escaped? There are in fact many places where this escaping is not done yet, which means there are problems if the user enters e.g. < or & in a title. Usually these bugs are in admin areas, so not critical for security, but they should still be fixed in the spirit of XHTML validation and usability. So this means that before the call to check_plain(), the data is plain-text and no HTML can be used in it. Once the text has been run through check_plain(), it is HTML, and tags can be added. Logically, it is easy: data should be escaped as soon as it becomes HTML, but exactly deciding where data becomes HTML is tricky. Any parameter which is passed as plain-text and only checked later in the code can not contain HTML, so I've had to decide where to add missing escapings based on context and common sense. This issue also affects URLs, but there the XSS issues are more important. Plus all of this should not be confused with urlencode() which is simply used to put arbitrary data into an URL without it disrupting normal URL semantics (directories, anchors, query arguments, etc). Concrete changes drupal_specialchars() and check_form() were almost the same, except that one would escape " while the other wouldn't. This difference is negligable and it does no harm to escape too much. In the interest of cleaning up text handling, I merged these into check_plain(). check_url() had a toss at extra XSS protection in it, but it was in fact broken and wholly incorrect. I replaced it with what was probably the original intention. Many URL handling functions in fact worked with HTML-escaped URLs (using & instead of & for example). I got rid of this, and did the escaping in a more logical place (e.g. drupal_goto no longer has to reconvert the &). Node/comment titles used to be run through strip_tags(), which means you couldn't use HTML but were still forced to escape angle brackets and amps. I changed them to be consistent with other plain-text fields in Drupal. Profile checkbox items were handled inconsistently as well and were fixed. The l() function now escapes the title by default, in the interest of keeping the code cleaner. But the old behaviour is still available (and needed in some places) with an extra parameter. Text handling in Drupal after this patch Any piece of user submitted text should be run through one of the check_ functions before being put into HTML. Use check_plain() for plain-text, check_output() for rich text. Dynamic data in URLs should be urlencode()d, regardless of where that URL ends up. URLs in HTML attributes should be check_url()'d. If you are unsure if you're doing the right thing, the easiest test is to try out putting HTML tags in your module's fields where they are not intended. They should be displayed on-screen and not interpreted. Steven

5 12

[drupal-devel] [feature] Request New Password Security
by chx 31 Mar '05

31 Mar '05

Issue status update for http://drupal.org/node/18719 Project: Drupal Version: cvs Component: user.module Category: feature requests Priority: critical Assigned to: Anonymous Reported by: neofactor Updated by: chx Status: patch One more point on uid 1. Yes, uid 1 can reset his pwd over the database. But then we would need to lock out administer users privileged users. Then administer nodes, 'cos of XSS. Then administer comments... then administer filters... OK, this is pointless. If someone can eavesdrop on your emails, everything is lost, I think. This is not something Drupal shall address. Maybe we could employ a security question -- secret answer pair. chx Previous comments: ------------------------------------------------------------------------ March 11, 2005 - 03:52 : neofactor Problem: Any user can force another user's password to change... simply by selecting "request new password" and putting in their username. The user gets an email with the new.. but this feels like a violation to the user... and a pain. Solution? If someone requests a new password... Don't blindly change it... send an email that says...."Is this a real request authorized by you? Click here to confirm otherwise disregard this message" Please consider this critical for user by-in to Drupal as a secure system. I appreciate your consideration. http://drupal.org/node/18689 ------------------------------------------------------------------------ March 11, 2005 - 05:05 : neofactor I added some code to prevent the admin account from being reset... Please add as a patch. if ($account->uid == 1 { unset($account); form_set_error('name', t('Sorry. The username %name is not allowed to be changed.', array('%name' => ''. $edit['name'] .''))); } // Just above this code on line 911: if ($account) { $from = variable_get('site_mail', ini_get('sendmail_from')); $pass = user_password(); ------------------------------------------------------------------------ March 11, 2005 - 05:08 : killes(a)www.drop.org Please only set to "patch" if you are actually submitting one. The solution you proposed is rather a workaround and will not be acceptable for core. ------------------------------------------------------------------------ March 11, 2005 - 09:21 : Deno I was in charge of a web site with >10000 paying customers for some time, and the "customers care" was constantly troubled with pasword change requests, until we switched to a system that worked in the following way: 1) "I forgot the password" function accepts either users email, or users login name, generates a random phrase, and adds an entry to "pasword_change_request" table. This entry consists of: - User ID (corresponding to email or login) - random phrase - datestamp User ID is unique key, and the entries in this table older than MAX_TIME are regularly purged by a cron job. 2) System sends an email with a link to "change forgotten password" page with this random phrase as argument to the user. That would be something like: https://drupal.org/user/cfp/d438rwiuodw894732ehdw 3) When user follows the link, "cfp" page checks if the random phrase exists in the table, allows the user to immediately change the password, and sends another email with "your password was changed on RIGHT_NOW ... " note to the user in question. - Password change automatically triggers deleting of the corresponding entry in pasword_change_request table. - Password change automatically logs-in the user as well. This way, users weren't bothered by accidental password changes, and they also understood the system better than the one with automatically generated passwords. For additional security, we also built some brakes that assured that the number of attempts to change the password, such as: - "An email with instructions was sent to you X minutes ago. In case you don't receive it within N hours, please contact the site administrator" - "According to our logs, you have visited the CFP page more than NMAX times within last 24 hours. For security reasons, you will be denied access to this page for the next 24 hours. Please contact the site administrator." hope this helps... ------------------------------------------------------------------------ March 30, 2005 - 21:01 : Jose A Reyero Attachment: http://drupal.org/files/issues/user_one_time_login.patch (6.52 KB) This patch for -cvs- addresses this issue. It generates an *only one use* URL to login into user account, instead of a new password. Then the user clicks on the URL, logs in, and may change his old password if desired. It is a only one use URL, and has some additional security measures, like a configurable time-out -can be confgured in admin/user/settings- and also, if somebody uses it or password is changed in the meanwhile, previously generated login URLs of this type won't work. About how it works, it creates a new url with: - user id, - timestamp - a hash of current timestamp, changed date for account, and the old password hashed With this patch, this wont work for admin accounts, which I think is better, you never know... It has a number of features like: - User can be ignoring these emails if somebody else is requesting access, a new password request wont change current password. However, the user will be warned when seeing the emails - It reveals no sensible information about the user account in the URL itself - the generated hash is not predictable at all if you dont know the old password, as it depends on user's old password - there's a configurable timeout - The URL's are only one use, as they depend on two timestamps, one of which will be changing in case this login is used -or in case user logs in for other means - Any 'new password' request is independent of others, so no one can be interfering with one user asking for his password back. Also doesn't require any new db field nor keeping track of requested passwords. ------------------------------------------------------------------------ March 30, 2005 - 21:41 : chx Very nice patch. Please consider for 4.6. ------------------------------------------------------------------------ March 30, 2005 - 23:16 : drumm Couldn't those nested if statements be made into one with a bit cleaner indentation? It looks like the page which the user arrives at after getting the new login information is their user page, not a page to set the password. The "Time out for password recovery e-mail" configuration is very unfriendly. It should be a drop down select of the most used human readable durations. Or we could just decide on one timeout and leave that hard coded in; who actually needs to use that setting? The url is quite long, what could be done to make it shorter? ------------------------------------------------------------------------ March 31, 2005 - 00:50 : Jose A Reyero /Couldn't those nested if statements be made into one with a bit cleaner indentation?/ Maybe, but I had to split that if conditions in two parts because the $account var didn't get the value before the rest of the conditions -latest two- where evaluated. Someone can help with PHP here? /It looks like the page which the user arrives at after getting the new login information is their user page, not a page to set the password. The "Time out for password recovery e-mail" configuration is very unfriendly. It should be a drop down select of the most used human readable durations. Or we could just decide on one timeout and leave that hard coded in; who actually needs to use that setting?/ Agreed, will be polishing this a little bit, just wating for some more suggestions... For the duration, I'll change to 'hours', ok? Just don't like to restrict options using unneccessary dropdowns, it's only intended for administrators anyway. Maybe I add also a '0' for unlimited. Hardcoding it, would need anyway to be mentioned in the module help, so it's about the same overhead, both for coding and for the admin interface, and harcoding in two places in code is not good anyway. And I like having options, am I the only one? / The url is quite long, what could be done to make it shorter?/ Well, the url has to be long, it has neccessary params and an MD5 hash, which yes, could be cut someway, but that adding inneccessary code and also reducing security -though half an MD5 hash may be secure enough- but I see no point in that. ------------------------------------------------------------------------ March 31, 2005 - 02:18 : FactoryJoe(a)civicspacelabs.org "Or we could just decide on one timeout and leave that hard coded in; who actually needs to use that setting? " +1 for making this *not* configurable! C'mon, Drupal should have standards about security -- and if best practices say that, say, 8 hours, is a good amount of time for that URL to be active, then do that. Cut out the confusion and extra brain juice it takes to decide... "Gee, is 4 hours better than 5?" Additionally, there's a simple way to make this workflow better for the user. If you visit an outdated change password URL, you should be told, "Sorry, but that link you followed to change your password has expired. If you would like to reset your password, please fill out this form: [form] If you did not request to change your password, please notify [administrator's name w/ PM link]". This way, if you missed your change password deadline, you can request your password again right from the timeout page. If you didn't request the change, it will be obvious that someone else was trying to change it, and then you can contact the administrator and find out who it was that was messin' with your account. ------------------------------------------------------------------------ March 31, 2005 - 06:55 : chx Attachment: http://drupal.org/files/issues/user_one_time_login_0.patch (4.72 KB) I cleaned up the patch a bit. I hope code style is OK. So may -- there are no checks against the arguments? There is no need. user_load uses %d for 'uid', so that's dealt with. Comparison by < and > means an implicit cast to integer, so $timestamp is also dealt with automatically. I do not think timeout needs to be configured. Please comment on not letting uid 1 using this one. I am not sure whether this is necessary. ------------------------------------------------------------------------ March 31, 2005 - 07:12 : chx Attachment: http://drupal.org/files/issues/user_one_time_login_1.patch (6.21 KB) Implemented Chris' idea, so if you use a one time URl which is expired, you are sent to user/password with a message to ask for another. I changed a few messages and deleted the "not for admin" feature. Do we really want one gazillion support requests "I locked myself out of my site, user/password tells me not for admin, what now"?

1 0