Closed Bug 1415612 Opened 3 years ago Closed 3 years ago

Allow all plain text types when navigating top-level data URIs

Categories

(Core :: DOM: Security, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

(Regressed 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 2 obsolete files)

As explained within Bug 1401895 (comment 3) we should allow all plain/text types when navigating top-level data URIs.
Assignee: nobody → ckerschb
Blocks: 1401895
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [domsecurity-active]
Attachment #8926625 - Flags: review?(bzbarsky)
Comment on attachment 8926625 [details] [diff] [review]
bug_1415612_allow_all_plain_text_data.patch

>+  if (StringBeginsWith(filePath, NS_LITERAL_CSTRING("application/pdf"))) {

So... This will actually have a false positive for things like "application/pdfoops" and whatnot.  I guess that's OK because we don't render any types like that internally, but worth documenting.

>+  // Please note that data URIs are defined as:

That definition is moderately bunk.  Anne is trying to spec the actual syntax of data: URIs, but there's some work to be done there.

What we should do here is use nsDataHandler::ParseURI, because it handles all the weirdness correctly.  For example, it will correctly produce "text/plain" as the type for "data:,text", which the code here does not do.

Alternately, since we have the type information in nsDataHandler::NewURI we could stash it on the URI somewhere...
Attachment #8926625 - Flags: review?(bzbarsky) → review-
Thanks for that info Boris. I guess in that case it makes sense to just reuse the queried contentType from ParseURI() for all other whitelist-comparisons.
Attachment #8926625 - Attachment is obsolete: true
Attachment #8926907 - Flags: review?(bzbarsky)
Comment on attachment 8926907 [details] [diff] [review]
bug_1415612_allow_all_plain_text_data.patch

> Please note
>+  // that IsPlainTextType() returns some false positives, which is OK,
>+  // because we do not render those in the browser anyway.

We render everything IsPlainTextType() returns true for in the browser.  What is this comment talking about, exactly?
Attachment #8926907 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> We render everything IsPlainTextType() returns true for in the browser. 
> What is this comment talking about, exactly?

What I was referring to was that StringBeginsWith(filePath, NS_LITERAL_CSTRING("application/pdf") would have allowed "application/pdffoo" which was obviously not within IsPlainTextType(). I blame the jet lag for writing that nonsense.

Anyway, now that we are querying the contentType using ParseURI we can actually perform an exact string matching instead of using StringBeginsWith(filePath, NS_LITERAL_CSTRING("application/pdf").

Sorry for the confusion and thanks for pointing it out.
Attachment #8926907 - Attachment is obsolete: true
Attachment #8927367 - Flags: review?(bzbarsky)
Comment on attachment 8927367 [details] [diff] [review]
bug_1415612_allow_all_plain_text_data.patch

>+  // Whitelist all plain text types as well as data PDFs.

s/data/data:/

r=me.  Thank you!
Attachment #8927367 - Flags: review?(bzbarsky) → review+
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4017a9d65a94
Allow all plain text types when navigating top-level data URIs. r=bz
https://hg.mozilla.org/mozilla-central/rev/4017a9d65a94
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1418642
No longer depends on: 1418642
Depends on: 1418642
Regressions: 1555609
You need to log in before you can comment on or make changes to this bug.