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.

Files: 
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
FAILED: [[SimpleTest]]: [MySQL] 3 pass(es), 5 fail(s), and 6 exception(es).
[ View ]

Comments

pwolanin’s picture

Status:Active» Needs review
StatusFileSize
new1.34 KB
FAILED: [[SimpleTest]]: [MySQL] 3 pass(es), 5 fail(s), and 6 exception(es).
[ View ]

something like this?

greggles’s picture

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

Status:Needs work» Fixed
greggles’s picture

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

Status:Needs work» Needs review
StatusFileSize
new1.16 KB
Dave Reid’s picture

StatusFileSize
new1.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

Priority:Normal» Critical

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

Dave Reid’s picture

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

StatusFileSize
new1.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

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

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.