644013 - improve url validation

Status: RESOLVED FIXED

(Cloud Services :: Share: Firefox Client, defect)

Description

[Shane Caraveo (:mixedpuppy)]

We grab a bunch of urls out of content for sharing, and often they are malformed.  There is a quick hack to do a simple validation, but I'm sure there is something in mozilla to do real url validation.  see _validURL in panel.js of the addon.

Comment 1

[Shane Caraveo (:mixedpuppy)]

url for testing:

http://www.universalsubtitles.org/en/videos/NNxEqfuQSaPN/info/Mozilla%20Firefox:%20A%20Different%20Kind%20of%20Browser/

from bug 642905

Comment 2

[Shane Caraveo (:mixedpuppy)]

I'm beginning to think the best solution is a regular expression since I cannot find any api in firefox that provides validation of good urls.  Using nsIURI/nsIURL does not catch some invalid urls (below).  Here's the regex I came up with:

^\w+?://\w+(?:\.\w+)*(?::\d+)?(?:/.*)?$

It works with the following set (correctly identifying valid urls).

http://invalid.comhttp://invalid.com
http:///invalid/path
http://invalid.com:foo/somepath
http://www.invalid.orghttp://s3.www.invalid.org/images/small_logo.png
http://valid/
http://valid.com
http://valid.com/somepath
http://valid.com:80/somepath
https://valid.com:80/somepath#foobar?test=1
http://s3.www.valid.org/images/small_logo.png


Anyone have a better idea?

Comment 3

[Philippe M. Chiasson (:gozer)]

From http://search.cpan.org/~gaas/URI-1.58/URI.pm#PARSING_URIs_WITH_REGEXP

As an alternative to this module, the following (official) regular expression can be used to decode a URI:

  my($scheme, $authority, $path, $query, $fragment) =
  $uri =~ m|(?:([^:/?#]+):)?(?://([^/?#]*))?([^?#]*)(?:\?([^#]*))?(?:#(.*))?|;

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: url validate patch

Comment 5

[Philipp von Weitershausen [:philikon]]

Re [needs test]: This is a small enough function that the tests can be xpcshell-tests.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: fix with test

in order to avoid setup of the panel I had to split the specific code I wanted to test into a function and export it, is there a better way?

Comment 7

[Philipp von Weitershausen [:philikon]]

Comment on attachment 523612 [details] [diff] [review]
fix with test

>+Cu.import("resource://gre/modules/NetUtil.jsm");

Unused import ;)

>+// First test urls that should fail validation
>+gTests.push(function test_url_validation_fail() {
>+    for (var i=0; i < bad_urls.length; i++) {
>+      do_check_eq(validateURL(bad_urls[i]), null);
>+    }
>+    run_next_test();
>+});
>+
>+// Test some good urls now
>+gTests.push(function test_url_validation_success() {
>+    for (var i=0; i < good_urls.length; i++) {
>+      do_check_false(validateURL(good_urls[i]) === null);

There's do_check_neq(), although we do know what the return value of validateURL() is when the URL is valid (it's the URL), so why not test for that?

>+    }
>+    run_next_test();
>+});
>+
>+
>+function run_test() {
>+  run_next_test();
>+}

That's a little more complex than it has to be. gTests + run_text_test() is only necessary when the tests are async. Here you can just do:

function run_test() {
  // First test urls that should fail validation.
  for (var i=0; i < bad_urls.length; i++) {
    do_check_eq(validateURL(bad_urls[i]), null);
  }

  // Test some good urls now.
  for (var i=0; i < good_urls.length; i++) {
    do_check_eq(validateURL(good_urls[i]), good_urls[i]);
  }
}

r=me with that.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: fixed patch

Comment 9

[Shane Caraveo (:mixedpuppy)]

landing with previous r+ from philikon, all changes made, tests included

Comment 10

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/users/pweitershausen_mozilla.com/fx-share/rev/53bec1ffcc35
https://hg.mozilla.org/users/pweitershausen_mozilla.com/fx-share/rev/87224f3a639e