Posted by sun on August 8, 2012 at 5:16pm
4 followers
| Project: | Mollom |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
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 ishttp://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.
Comments
#1
#2
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.
#3
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.
#4
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.
#5
Path to be reviewed. I test it, seem good. Pay attention on the links querys etc.
#6
The last submitted patch, mollom.blacklist-entry-delete-5.patch, failed testing.
#7
#8
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.
#9
Automatically closed -- issue fixed for 2 weeks with no activity.