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

RESOLVED FIXED in Firefox 48

Status

()

Core
Networking
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: bdahl, Assigned: kmag)

Tracking

Trunk
mozilla48
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [necko-active])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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

Comment 1

5 years ago
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.
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1220305
(Assignee)

Updated

2 years ago
Blocks: 1214433

Comment 3

2 years ago
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)

Updated

2 years ago
Assignee: nobody → kmaglione+bmo
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1242080
(Assignee)

Comment 5

2 years ago
Created attachment 8716171 [details]
MozReview Request: Bug 719905: Fix resolution of resource: URLs containing : and / characters. r=valentin

Review commit: https://reviewboard.mozilla.org/r/33745/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33745/
Attachment #8716171 - Flags: review?(wmccloskey)
(Assignee)

Comment 6

2 years ago
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.
(Assignee)

Comment 7

2 years ago
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/
(Assignee)

Comment 8

2 years ago
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)
(Assignee)

Comment 10

2 years ago
(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.
(Assignee)

Updated

2 years ago
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)
(Assignee)

Updated

2 years ago
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]

Comment 13

a year ago
Any news about this patch?
(Assignee)

Comment 14

a year ago
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.
(Assignee)

Comment 15

a year ago
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
(Assignee)

Updated

a year ago
Duplicate of this bug: 1256124
(Assignee)

Updated

a year ago
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)
(Assignee)

Comment 18

a year ago
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+
(Assignee)

Comment 20

a year ago
https://hg.mozilla.org/integration/fx-team/rev/6741a3b28c55793fc1048afede74a305cc778165
Bug 719905: Fix resolution of resource: URLs containing : and / characters. r=valentin

Comment 21

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6741a3b28c55
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Updated

a year ago
Duplicate of this bug: 1240376
You need to log in before you can comment on or make changes to this bug.