Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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
- Use a query parameter to pass the blacklist ID.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 0001-1719470-by-sun-Fixed-Delete-link-for-blacklist-terms.patch | 5.86 KB | sun |
| #5 | mollom.blacklist-entry-delete-5.patch | 5.62 KB | killua99 |
| #1 | mollom.blacklist-entry-delete.1.patch | 5.12 KB | sun |
Comments
Comment #1
sunComment #2
sunCommitted 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.
Comment #3
sunOK, 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.
Comment #4
sunThe 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.
Comment #5
killua99 CreditAttribution: killua99 commentedPath to be reviewed. I test it, seem good. Pay attention on the links querys etc.
Comment #7
sunComment #8
sunThanks 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.
Comment #9.0
(not verified) CreditAttribution: commentedUpdated issue summary.