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.
CommentFileSizeAuthor
#7 0001-1719470-by-sun-Fixed-Delete-link-for-blacklist-terms.patch5.86 KBsun
#5 mollom.blacklist-entry-delete-5.patch5.62 KBkillua99
#1 mollom.blacklist-entry-delete.1.patch5.12 KBsun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Active » Needs review
StatusFileSize
new mollom.blacklist-entry-delete.1.patch5.12 KB
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
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.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
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.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
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.

killua99’s picture

killua99 commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new mollom.blacklist-entry-delete-5.patch5.62 KB

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.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new 0001-1719470-by-sun-Fixed-Delete-link-for-blacklist-terms.patch5.86 KB
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
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.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated issue summary.

  • Commit 4526654 on 7.x-2.x, 8.x-2.x, fbajs, actions by sun: 
    - #1719470 by sun: Fixed Delete link for blacklist terms does not escape...

  • Commit 4526654 on 7.x-2.x, 8.x-2.x, fbajs, actions by sun: 
    - #1719470 by sun: Fixed Delete link for blacklist terms does not escape...