Problem

  • The internal identifier for a blacklist term (returned by Mollom) may contain slashes (i.e., when the blacklisted term contains slashes).
  • The identifier is properly escaped, except for the potentially contained slashes, since Drupal's URL path and query parameter encoding functions intentionally revert escaped slashes back to unescaped slashes (beautification of URLs).
  • Given a blacklist term "http://foo", the delete link URL being output is
    http://example.com/admin/config/content/mollom/blacklist/delete/6fcc7ea...-http%3A//foo?destination=admin/config/content/mollom/blacklist

Details

  • We cannot avoid the unescaping, unless we'd double-escape slashes.
  • The actual cause however is that we're placing the blacklist term ID into the path, instead of a query parameter.
  • We're using a path parameter, since that is what Drupal is normally using. In this case, Drupal core's built-in beautification of path parameters produces a problem though (the same problem is known for autocomplete callbacks in D7 already).

Proposed solution

  1. Use a query parameter to pass the blacklist ID.
Files: 
CommentFileSizeAuthor
#7 0001-1719470-by-sun-Fixed-Delete-link-for-blacklist-terms.patch5.86 KBsun
PASSED: [[SimpleTest]]: [MySQL] 4,292 pass(es).
[ View ]
#5 mollom.blacklist-entry-delete-5.patch5.62 KBkillua99
FAILED: [[SimpleTest]]: [MySQL] 3,845 pass(es), 14 fail(s), and 2 exception(s).
[ View ]
#1 mollom.blacklist-entry-delete.1.patch5.12 KBsun
PASSED: [[SimpleTest]]: [MySQL] 4,883 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new5.12 KB
PASSED: [[SimpleTest]]: [MySQL] 4,883 pass(es).
[ View ]

Version:7.x-2.x-dev» 6.x-2.x-dev
Status:Needs review» Patch (to be ported)

Committed and pushed to 7.x-2.x (including the mollom.admin.css file, which I forgot to git-add in #1)

Moving to 6.x-2.x for backport.

Version:6.x-2.x-dev» 7.x-2.x-dev
Priority:Normal» Major
Status:Patch (to be ported)» Active
Issue tags:+Release blocker

OK, this wasn't a good idea after all. Turns out that there is a big difference in blacklist term IDs between the REST production API and the testing API.

That explains why this bug was never visible in tests, and why this patch also did not need any changes to tests.

So before backporting this, let's wait for Mollom backend engineers to figure out how to achieve parity between both APIs, and what this means for the module.

Version:7.x-2.x-dev» 6.x-2.x-dev
Priority:Major» Normal
Status:Active» Patch (to be ported)
Issue tags:-Release blocker

The actual bug was fixed on the server-side Mollom backend/API, which is using a UUID for each blacklist entry now.

Therefore, the committed change is basically unnecessary. However, I reviewed the changes once more, and I think it makes sense to pass the (UU)ID as a query string parameter instead.

So I think we can simply move forward and backport this change to 6.x-2.x.

Status:Patch (to be ported)» Needs review
StatusFileSize
new5.62 KB
FAILED: [[SimpleTest]]: [MySQL] 3,845 pass(es), 14 fail(s), and 2 exception(s).
[ View ]

Path to be reviewed. I test it, seem good. Pay attention on the links querys etc.

Status:Needs review» Needs work

The last submitted patch, mollom.blacklist-entry-delete-5.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.86 KB
PASSED: [[SimpleTest]]: [MySQL] 4,292 pass(es).
[ View ]

Status:Needs review» Fixed

Thanks for reporting, reviewing, and testing! Committed and pushed to 6.x-2.x.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

Status:Fixed» Closed (fixed)

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

Issue summary:View changes

Updated issue summary.