The install script creates a hash that is supposed to be secret, but it's not quite super secret enough. Ideally it should use something like http://us.php.net/uniqid as part of the hash to make it harder (near impossible) to calculate.

CommentFileSizeAuthor
#10 790586-fix-salts_1.patch1.18 KBgreggles
#7 790586-fix-salts.patch1.16 KBdave reid
#6 790586-fix-salts.patch1.16 KBdave reid
#1 790586-better-install-1.patch1.34 KBpwolanin

Comments

pwolanin’s picture

pwolanin commented
Status: Active » Needs review
StatusFileSize
new 790586-better-install-1.patch1.34 KB

something like this?

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

Looks great to me - thanks Peter!

Status: Needs review » Needs work

The last submitted patch, 790586-better-install-1.patch, failed testing.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs work » Fixed

Finally committed - thanks http://drupalcode.org/project/comment_notify.git/commit/7423926

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Fixed » Needs work

apparently "uniqid(mt_rand, TRUE)" is not really the right thing to do.

#1261532: Error on new update install

@pwolanin, what was your intent here?

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Needs review
StatusFileSize
new 790586-fix-salts.patch1.16 KB
dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
StatusFileSize
new 790586-fix-salts.patch1.16 KB

Better patch that uses COALESCE for non-pgsql since that's more standard that IFNULL and uppercases SQL functions for readability.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Priority: Normal » Critical

Bumping to critical as this prevents the module from being installed properly.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Also, it should be possible to do this if we nest CONCAT() statements using two arguments per call - then it's 100% DBTNG compatible and we don't have to special-case for PostgreSQL.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
StatusFileSize
new 790586-fix-salts_1.patch1.18 KB

Great. I feel like nesting concats is ugly enough that it offsets portability.

#7 didn't work perfectly b/c it leaves us with the same salt for notifications on the same node.

Attached version adds cid to provide even more uniqueness.

kingfisher64’s picture

kingfisher64 commented

I get the following error message when the module is enabled on drupal 7.8. I followed the link from http://drupal.org/node/1261532.

Notice: Use of undefined constant mt_rand - assumed 'mt_rand' in comment_notify_install()

Can someone tell me if the patch above is the fix to problem? If so could someone tell me how to apply this patch (i've never done a patch install before).

Thank you in advance for any help received

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs review » Fixed

@kingfisher64, yes, this is to fix that problem.

Thanks, Dave. This is now committed: http://drupalcode.org/project/comment_notify.git/commit/581f0ff

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.