Closed Bug 719905 Opened 12 years ago Closed 8 years ago

resource:// Urls Don't Validate With ':' (colons)

Categories

(Core :: Networking, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: bdahl, Assigned: kmag)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file)

The following url gives the the error message "The URL is not valid and cannot be loaded."

resource://pdf.js/web/viewer.html?file=http%3A

Note the %3A which is an escaped colon, unescaped colons also don't work.

Chrome url's seem to work fine with colons:

chrome://pdf.js/content/web/viewer.html?file=http%3A

In IRC John-Galt suggested that the following code should strip the query string before the unescaping.

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/res/nsResProtocolHandler.cpp#432
Also any URL that contains a colon does not render as a link in Firefox. These links do render correctly in other browsers though.
For example, www.myurl.com:test will be displayed as plain text instead of as a link in Firefox. I have reproduced this in FF3.6 and FF10 in Windows XP and Windows 7 environments.
Blocks: webext
I am encountering this as well. I am trying to pass information in encoded URL arguments to a file inside my extension. If any of the arguments contain an encoded colon (such as a URL) I get the "The address isn't valid" error.
Assignee: nobody → kmaglione+bmo
There are a couple of points to note about this patch:

* The test that I changed was not working at all. It was throwing an error because Services was not defined, and since the only check was that it threw an error, it always passed.

* The test also wasn't actually testing the code in question, anymore, since resource:/// URLs are treated specially, and don't trigger those checks.

I initially added a new set of checks to ResolveURI to make sure the resolved URL was actually a child of the root URL, but I couldn't find any circumstance where they were actually necessary.
Comment on attachment 8716171 [details]
MozReview Request: Bug 719905: Fix resolution of resource: URLs containing : and / characters. r=valentin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33745/diff/1-2/
Updated, because apparently we have code that relies on an empty path resolving to a file rather than a directory.
Comment on attachment 8716171 [details]
MozReview Request: Bug 719905: Fix resolution of resource: URLs containing : and / characters. r=valentin

https://reviewboard.mozilla.org/r/33745/#review30823

This looks good, but I'm not a peer here and I don't think I should review. Perhaps bz would be a better choice.

::: netwerk/protocol/res/SubstitutingProtocolHandler.cpp:369
(Diff revision 2)
> +    path.InsertLiteral(".", 0);

I worry about corner cases here. Is there any reason you can't do what we do for chrome URLs?
https://dxr.mozilla.org/mozilla-central/source/chrome/nsChromeRegistry.cpp#308
Attachment #8716171 - Flags: review?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #9)
> ::: netwerk/protocol/res/SubstitutingProtocolHandler.cpp:369
> (Diff revision 2)
> > +    path.InsertLiteral(".", 0);
>
> I worry about corner cases here.

It has fewer corner cases than the previous code. There aren't any that I can
think of. A URL that starts with "./" will always be treated as relative to
the current file.

> Is there any reason you can't do what we do for chrome URLs?

It has the same issues as the former code here. Well, not quite as bad, since
it doesn't worry about the query string, but its checks are still pretty
overzealous. It doesn't allow any occurrence of ':' or ".." anywhere in the
path component.

The network code already takes care of the '..' issues for us. Adding a
leading '.' takes care of any other relative URL issues I can think of.
Attachment #8716171 - Flags: review?(bzbarsky)
Comment on attachment 8716171 [details]
MozReview Request: Bug 719905: Fix resolution of resource: URLs containing : and / characters. r=valentin

Please ask a necko peer to review necko code.
Attachment #8716171 - Flags: review?(bzbarsky)
Attachment #8716171 - Flags: review?(mcmanus)
Attachment #8716171 - Flags: review?(mcmanus) → review?(valentin.gosu)
Comment on attachment 8716171 [details]
MozReview Request: Bug 719905: Fix resolution of resource: URLs containing : and / characters. r=valentin

https://reviewboard.mozilla.org/r/33745/#review31223

::: netwerk/test/unit/test_bug337744.js:44
(Diff revision 2)
> +  if (/%2c|\\/i.test(channel.name)) {

I can't tell what this is for. There are not test cases that use it.

The patch looks good, apart from this minor issue.
Attachment #8716171 - Flags: review?(valentin.gosu) → review+
Whiteboard: [necko-active]
Any news about this patch?
Sorry, I ran into an issue on Windows after I addressed the issue in comment 12. I thought I'd updated the patch already, but the try run took so long to run that I guess I forgot.

I had to reintroduce the ban on \\ characters in the file path to prevent sandbox escapes on Windows, when the target URL isn't in a JAR file.
Comment on attachment 8716171 [details]
MozReview Request: Bug 719905: Fix resolution of resource: URLs containing : and / characters. r=valentin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33745/diff/2-3/
Attachment #8716171 - Attachment description: MozReview Request: Bug 719905: Fix resolution of resource: URLs containing : and / characters. r?billm → MozReview Request: Bug 719905: Fix resolution of resource: URLs containing : and / characters. r=valentin
Attachment #8716171 - Flags: review+ → review?(valentin.gosu)
Comment on attachment 8716171 [details]
MozReview Request: Bug 719905: Fix resolution of resource: URLs containing : and / characters. r=valentin

https://reviewboard.mozilla.org/r/33745/#review44295

::: netwerk/protocol/res/SubstitutingProtocolHandler.cpp:362
(Diff revisions 2 - 3)
>    rv = GetSubstitution(host, getter_AddRefs(baseURI));
>    if (NS_FAILED(rv)) return rv;
>  
> +  // Unescape the path so we can perform some checks on it.
> +  nsCOMPtr<nsIURL> url = do_QueryInterface(uri);
> +  NS_ENSURE_TRUE(url, NS_ERROR_MALFORMED_URI);

NS_ENSURE_ macros hide return statements. Please use:
if (!url) {
  return NS_ERROR_MALFORMED_URI;
}

::: netwerk/protocol/res/SubstitutingProtocolHandler.cpp:369
(Diff revisions 2 - 3)
> +  nsAutoCString unescapedPath;
> +  rv = url->GetFilePath(unescapedPath);
> +  if (NS_FAILED(rv)) return rv;
> +
> +  NS_UnescapeURL(unescapedPath);
> +  if (unescapedPath.FindChar('\\') != -1)

Add braces {} please.

::: netwerk/protocol/res/SubstitutingProtocolHandler.cpp:383
(Diff revision 3)
> -  nsCOMPtr<nsIURI> baseURI;
> -  rv = GetSubstitution(host, getter_AddRefs(baseURI));
> -  if (NS_FAILED(rv)) return rv;
> +  } else {
> +    // Make sure we always resolve the path as file-relative to our target URI.
> +    path.InsertLiteral(".", 0);
>  
> -  rv = baseURI->Resolve(nsDependentCString(p, path.Length()-1), result);
> +    rv = baseURI->Resolve(path, result);
> +    NS_ENSURE_SUCCESS(rv, rv);

Since both branches check the return value, can we move it outside the block, and use if NS_FAILED(rv) ... instead of NS_ENSURE ?
Attachment #8716171 - Flags: review?(valentin.gosu)
Comment on attachment 8716171 [details]
MozReview Request: Bug 719905: Fix resolution of resource: URLs containing : and / characters. r=valentin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33745/diff/3-4/
Attachment #8716171 - Flags: review?(valentin.gosu)
Comment on attachment 8716171 [details]
MozReview Request: Bug 719905: Fix resolution of resource: URLs containing : and / characters. r=valentin

https://reviewboard.mozilla.org/r/33745/#review44309

Thanks!
Attachment #8716171 - Flags: review?(valentin.gosu) → review+
https://hg.mozilla.org/integration/fx-team/rev/6741a3b28c55793fc1048afede74a305cc778165
Bug 719905: Fix resolution of resource: URLs containing : and / characters. r=valentin
https://hg.mozilla.org/mozilla-central/rev/6741a3b28c55
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Blocks: 1405174
See Also: → 1415574
You need to log in before you can comment on or make changes to this bug.