If an URL contains '[' or ']' characters, _filter_url function don't work properly.

ex:
- http://mysite.fr/node/ognodeadd?type=asset&gids=1074 is recognized as an URL
- http://mysite.fr/node/ognodeadd?type=asset&gids[]=1074 is not recognized as an URL

See attached patch for correction.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David Stosik’s picture

Hello,
I work with sdelbosc, I just want to add that you have to clean filter_cache table.
(and also, sdelbosc's post is a perfect example of what does not work).

Regards,
David

drumm’s picture

Version: 5.1 » 6.x-dev
Status: Needs review » Needs work

I think this patch has been reversed. The '][' characters should be added, not removed. These should be added at the end of the [...] of valid characters since ranges always go first. And the ']' probably needs to be escaped.

David Stosik’s picture

Version: 6.x-dev » 5.2
Assigned: Unassigned » David Stosik
Status: Needs work » Needs review
FileSize
1.14 KB

Hello,
Why did you put this bug report in 6.x-dev? I think this has to be corrected in a future 5.x release.
I modified the patch provided by sdelbosc to reflect the real modification. The two square brackets don't need to be escaped, as there is no special character inside a [] class, they are considered as simple characters (and it is a PERL rule that is verified when using preg_replace ;) ).
Please review this patch and tell me if it is correct.

Thanks,
David

drumm’s picture

Version: 5.2 » 5.x-dev

It is generally best to fix issues on the development branch, currently 6.x for bug fixes, and then backport as needed. If a patch needs work I generally push it to the best version to do work on. Since there is now a reviewable patch for 5.x, it can stay here.

David Stosik’s picture

Thanks! :)

drumm’s picture

Version: 5.x-dev » 7.x-dev

Sorry, I must have thought this was a backport or something. It should be resolved for the development branch first, and then backported as needed. This patch does apply fine against HEAD, so a new one is not needed now. The code looks okay. This might get more attention on HEAD since there are more people working on the development version.

catch’s picture

Status: Needs review » Needs work

This needs a test with it - haven't looked in detail, but there's a lot of failing tests included in #276597: TestingParty08: filter.module so it may just be a case of using one of those and combining it with this patch.

kleinmp’s picture

Status: Needs work » Needs review
FileSize
1.11 KB

Here's the patch again, but it was patched against drupal 7.

catch’s picture

Priority: Minor » Normal
Status: Needs review » Needs work

This still needs to be combined with the 'filter URL doesn't work with square brackets' test from http://drupal.org/files/issues/filter-failing-tests_php.txt

kleinmp’s picture

Status: Needs work » Needs review
FileSize
1.93 KB

Thanks, catch. I added the 'filter URL doesn't work with square brackets' test into the patch.

kleinmp’s picture

Status: Needs review » Needs work
FileSize
1.89 KB

Just tested this patch using Simpletest and it failed. I fixed a minor spacing issue, so I'm reposting it.

kleinmp’s picture

Status: Needs work » Needs review
FileSize
2.09 KB

This patch passed the test for me.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

kleinmp - try re-rolling from the root of your Drupal directory.

kleinmp’s picture

FileSize
1.96 KB

Ok, here it is re-rolled from the root folder.

kleinmp’s picture

Status: Needs work » Needs review

Forgot to change status.

catch’s picture

Issue tags: +Needs backport to D6

When the test bot is happy I'll RTBC this.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

   function testHtmlFilter() {
-
+    $filtered = _filter_url('http://www.example.com/index.php?a[]=1', 'f');
+    $this->assertEqual($filtered, '<a href="http://www.example.com/index.php?a[]=1" title="http://www.example.com/index.php?a[]=1">http://www.example.com/index.php?a[]=1</a>', t('Converting URLs -- addresses with square brackets.'));
+       }
   }

Failing test seems to be obvious. (Added closing curly-brace does match any opening brace)

Can we add some more tests? (than just the case this issue is about?)

lilou’s picture

Status: Needs work » Needs review
FileSize
2.28 KB

Reroll (delete curly-brace).

Status: Needs review » Needs work

The last submitted patch failed testing.

kscheirer’s picture

Status: Needs work » Needs review
FileSize
2.48 KB

Looks good, all tests pass. Rerolled against HEAD, changed variable name to $f and moved test case to testUrlFilter() since that's where the other tests for _filter_url() are.

Status: Needs review » Needs work

The last submitted patch failed testing.

kscheirer’s picture

Assigned: David Stosik » kscheirer
Status: Needs work » Needs review
FileSize
2.45 KB

rerolled against HEAD

Re-test of issue-190466_2.patch from comment #24 was requested by Arancaytar.

cburschka’s picture

Status: Needs review » Needs work
+++ modules/filter/filter.test	10 Sep 2009 20:37:41 -0000
@@ -542,6 +542,9 @@
+    $f = _filter_url('http://www.example.com/index.php?a[]=1', $filter);
+    $this->assertEqual($f, '<a href="http://www.example.com/index.php?a[]=1" title="http://www.example.com/index.php?a[]=1">http://www.example.com/index.php?a[]=1</a>', t('Converting URLs -- addresses with square brackets.'));
+

Let's have an inline comment explaining the case - "test for correct treatment of square brackets".

(Maybe some other special characters that should be accepted could also be placed into this test URL...)

This review is powered by Dreditor.

sun’s picture

Title: Filter url don't work with square brackets » URL filter fails on square brackets
kscheirer’s picture

Status: Needs work » Needs review

The test case already states 'Converting URLs -- addresses with square brackets.' I don't think we need to duplicate that as a comment.

kscheirer’s picture

#24: issue-190466_2.patch queued for re-testing.

kscheirer’s picture

FileSize
2.38 KB

Bah, we don't add the title to the link anymore since #784790: URL filter displays URL as link title, so rolled a new patch. With comment. Coming up on 3 years, I think this one is ready to go in!

meeli’s picture

Assigned: kscheirer » Unassigned
Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs reroll

I looked at the latest dev for 8 and it looks like there are tests for absolute URLs with square brackets. This means that you can put square brackets in all your URLs and Drupal will no longer remove them!

The test is in /core/modules/filter/lib/Drupal/filter/Tests/FilterUnitTest.php.

And this issue is now only for D7.

Because this is a bug report we need tests because patch in #30 has no tests.

Also, the patch in #30 needs a reroll.

error: filter/filter.module: No such file or directory
error: filter/filter.test: No such file or directory

Review bonus #2094585: [policy, no patch] Core review bonus for #1898464: toolbar.module - Convert theme_ functions to Twig

Sivaji_Ganesh_Jojodae’s picture

Issue summary: View changes

I think this issue is not valid for D7 anymore.

We have different code to handle absolute URLs in filter.module

  // Match absolute URLs.
  $url_pattern = "(?:$auth)?(?:$domain|$ip)/?(?:$trail)?";
  $pattern = "`((?:$protocols)(?:$url_pattern))($punctuation)`";
  $tasks['_filter_url_parse_full_links'] = $pattern;

In filter.test, there is a test case for checking url with [], snip below,


      // URI parts and special characters.
      '
http://trailingslash.com/ or www.trailingslash.com/
http://host.com/some/path?query=foo&bar[baz]=beer#fragment or www.host.com/some/path?query=foo&bar[baz]=beer#fragment
http://twitter.com/#!/example/status/22376963142324226
ftp://user:pass@ftp.example.com/~home/dir1
sftp://user@nonstandardport:222/dir
ssh://192.168.0.100/srv/git/drupal.git
' => array(
        '<a href="http://trailingslash.com/">http://trailingslash.com/</a>' => TRUE,
        '<a href="http://www.trailingslash.com/">www.trailingslash.com/</a>' => TRUE,
        '<a href="http://host.com/some/path?query=foo&amp;bar[baz]=beer#fragment">http://host.com/some/path?query=foo&amp;bar[baz]=beer#fragment</a>' => TRUE,
        '<a href="http://www.host.com/some/path?query=foo&amp;bar[baz]=beer#fragment">www.host.com/some/path?query=foo&amp;bar[baz]=beer#fragment</a>' => TRUE,
        '<a href="http://twitter.com/#!/example/status/22376963142324226">http://twitter.com/#!/example/status/22376963142324226</a>' => TRUE,
        '<a href="ftp://user:pass@ftp.example.com/~home/dir1">ftp://user:pass@ftp.example.com/~home/dir1</a>' => TRUE,
        '<a href="sftp://user@nonstandardport:222/dir">sftp://user@nonstandardport:222/dir</a>' => TRUE,
        '<a href="ssh://192.168.0.100/srv/git/drupal.git">ssh://192.168.0.100/srv/git/drupal.git</a>' => TRUE,
      ),