decodeURIComponent can throw an exception while trying to get original URL

RESOLVED FIXED in Firefox 38

Status

()

P5
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

39 Branch
mozilla40
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed, firefox40 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Splitting this off from bug 1147337. This can also handle moving the _getOriginalURL logic into a shared place (and creating a unit test for it).
(Assignee)

Comment 1

4 years ago
Created attachment 8589384 [details] [diff] [review]
Factor out logic to get original URL from reader URL into shared place, and handle malformed URI excpetions

Asking mcomella to take a look at the mobile changes here.

This should address the issues we talked about in bug 1147337.
Attachment #8589384 - Flags: review?(nperriault)
Attachment #8589384 - Flags: review?(michael.l.comella)
Comment on attachment 8589384 [details] [diff] [review]
Factor out logic to get original URL from reader URL into shared place, and handle malformed URI excpetions

Review of attachment 8589384 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r=me with nits possibly addressed.

::: browser/modules/ReaderParent.jsm
@@ +188,1 @@
>    },

Nit: If parseReaderUrl is now a pure proxy to ReaderMode.getOriginalUrl, can't we just call the latter directly?

::: mobile/android/chrome/content/browser.js
@@ +4564,5 @@
> +    if (originalUrl) {
> +      return originalUrl;
> +    }
> +
> +    return url;

Nit: `return originalUrl || url;` should works here, or the more verbose return originalUrl ? originalUrl : url;

::: toolkit/components/reader/AboutReader.jsm
@@ +790,2 @@
>      }
> +    return url;

Nit: Same as above.
Attachment #8589384 - Flags: review?(nperriault) → review+
(Assignee)

Comment 3

4 years ago
(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #2)
> Comment on attachment 8589384 [details] [diff] [review]
> Factor out logic to get original URL from reader URL into shared place, and
> handle malformed URI excpetions
> 
> Review of attachment 8589384 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, r=me with nits possibly addressed.
> 
> ::: browser/modules/ReaderParent.jsm
> @@ +188,1 @@
> >    },
> 
> Nit: If parseReaderUrl is now a pure proxy to ReaderMode.getOriginalUrl,
> can't we just call the latter directly?

I considered that, but ReaderParent.parseReaderUrl is called elsewhere in the tree. But I could look into updating those call sites as well. Just a matter of deciding how invasive to make this patch :)
(Assignee)

Comment 4

4 years ago
Created attachment 8589794 [details] [diff] [review]
(v2) Factor out logic to get original URL from reader URL into shared place, and handle malformed URI excpetions

On further consideration, I think it makes sense to just get rid of this `parseReaderUrl` method.

This touches more desktop front-end stuff, so I'm asking jaws to review that part.
Attachment #8589384 - Attachment is obsolete: true
Attachment #8589384 - Flags: review?(michael.l.comella)
Attachment #8589794 - Flags: review?(michael.l.comella)
Attachment #8589794 - Flags: review?(jaws)
Sounds like just a refactoring, so P5?
Priority: -- → P5
(Assignee)

Comment 6

4 years ago
(In reply to Justin Dolske [:Dolske] from comment #5)
> Sounds like just a refactoring, so P5?

Not just a refactoring... decodeURIComponent is throwing exceptions which break reader view for some poorly formed URLs.

We run into this with this test case from bug 1147337:
http://moztw.org/space/?;$%^^
Flags: needinfo?(dolske)
Comment on attachment 8589794 [details] [diff] [review]
(v2) Factor out logic to get original URL from reader URL into shared place, and handle malformed URI excpetions

Review of attachment 8589794 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. :D

::: mobile/android/chrome/content/Reader.js
@@ -4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> -XPCOMUtils.defineLazyModuleGetter(this, "ReaderMode", "resource://gre/modules/ReaderMode.jsm");

ReaderMode still seems to be used here - why are you removing it? Is it becauses the imports from browser.js are shared here?
Attachment #8589794 - Flags: review?(michael.l.comella) → review+
(Assignee)

Comment 8

4 years ago
(In reply to Michael Comella (:mcomella) from comment #7)

> ::: mobile/android/chrome/content/Reader.js
> @@ -4,5 @@
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  "use strict";
> >  
> > -XPCOMUtils.defineLazyModuleGetter(this, "ReaderMode", "resource://gre/modules/ReaderMode.jsm");
> 
> ReaderMode still seems to be used here - why are you removing it? Is it
> becauses the imports from browser.js are shared here?

Correct. Reader.js is loaded as a subscript in browser.js, so any imports here would be redundant.
Attachment #8589794 - Flags: review?(jaws) → review?(gijskruitbosch+bugs)

Comment 9

4 years ago
Comment on attachment 8589794 [details] [diff] [review]
(v2) Factor out logic to get original URL from reader URL into shared place, and handle malformed URI excpetions

Review of attachment 8589794 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-readinglist.js
@@ +251,5 @@
>      let uri;
>      if (this.enabled && state == "valid") {
>        uri = gBrowser.currentURI;
>        if (uri.schemeIs("about"))
> +        uri = ReaderMode.getOriginalUrl(uri.spec);

I vaguely wonder whether it's worth making getOriginalUrl deal with being passed an nsIURI, but I guess maybe not?

::: browser/base/content/test/general/browser_readerMode.js
@@ +100,5 @@
> +  is(ReaderMode.getOriginalUrl(url), null, "Did not find original URL from non-reader URL");
> +
> +  let badUrl = "http://foo.com/?;$%^^";
> +  is(ReaderMode.getOriginalUrl("about:reader?url=" + encodeURIComponent(badUrl)), badUrl, "Found original URL from encoded malformed URL");
> +  is(ReaderMode.getOriginalUrl("about:reader?url=" + badUrl), badUrl, "Found original URL from non-encoded malformed URL");

Please also add assertions here for the case that throws an exception.
Attachment #8589794 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 10

4 years ago
(In reply to :Gijs Kruitbosch from comment #9)

> ::: browser/base/content/test/general/browser_readerMode.js
> @@ +100,5 @@
> > +  is(ReaderMode.getOriginalUrl(url), null, "Did not find original URL from non-reader URL");
> > +
> > +  let badUrl = "http://foo.com/?;$%^^";
> > +  is(ReaderMode.getOriginalUrl("about:reader?url=" + encodeURIComponent(badUrl)), badUrl, "Found original URL from encoded malformed URL");
> > +  is(ReaderMode.getOriginalUrl("about:reader?url=" + badUrl), badUrl, "Found original URL from non-encoded malformed URL");
> 
> Please also add assertions here for the case that throws an exception.

I'm a bit confused by this comment. This function doesn't throw anymore, and these checks are the cases that used to throw (but now go through the catch statement in getOriginalUrl.

Comment 12

4 years ago
(In reply to :Margaret Leibovic from comment #10)
> (In reply to :Gijs Kruitbosch from comment #9)
> 
> > ::: browser/base/content/test/general/browser_readerMode.js
> > @@ +100,5 @@
> > > +  is(ReaderMode.getOriginalUrl(url), null, "Did not find original URL from non-reader URL");
> > > +
> > > +  let badUrl = "http://foo.com/?;$%^^";
> > > +  is(ReaderMode.getOriginalUrl("about:reader?url=" + encodeURIComponent(badUrl)), badUrl, "Found original URL from encoded malformed URL");
> > > +  is(ReaderMode.getOriginalUrl("about:reader?url=" + badUrl), badUrl, "Found original URL from non-encoded malformed URL");
> > 
> > Please also add assertions here for the case that throws an exception.
> 
> I'm a bit confused by this comment. This function doesn't throw anymore, and
> these checks are the cases that used to throw (but now go through the catch
> statement in getOriginalUrl.

Ah, then I must have misunderstood. Carry on!
https://hg.mozilla.org/mozilla-central/rev/a490eda23996
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40

Comment 14

4 years ago
Comment on attachment 8589794 [details] [diff] [review]
(v2) Factor out logic to get original URL from reader URL into shared place, and handle malformed URI excpetions

Approval Request Comment
[Feature/regressing bug #]: this breaks reader mode for weird escape sequences, e.g. bug 1147337 comment 27
[User impact if declined]: breaking reader mode
[Describe test coverage new/current, TreeHerder]: has tests!
[Risks and why]: pretty low, making us more fault-tolerant
[String/UUID change made/needed]: nope
Attachment #8589794 - Flags: approval-mozilla-beta?
Attachment #8589794 - Flags: approval-mozilla-aurora?
status-firefox38: --- → affected
status-firefox39: --- → affected
Comment on attachment 8589794 [details] [diff] [review]
(v2) Factor out logic to get original URL from reader URL into shared place, and handle malformed URI excpetions

Should be in 38 beta 5. We will still have RL enabled in this beta.
Attachment #8589794 - Flags: approval-mozilla-beta?
Attachment #8589794 - Flags: approval-mozilla-beta+
Attachment #8589794 - Flags: approval-mozilla-aurora?
Attachment #8589794 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.