1121857 - document.baseURI does not get updated to document.location after base tag is removed from DOM for site with a CSP

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[lalitkishore09]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150108202552

Steps to reproduce:

Steps :
1. Load a page say 'http://www.my-page.com'
2. log document.baseURI. Result -  'http://www.my-page.com'
3. Add a base tag :  
var baseTag = document.head.appendChild(document.createElement('base'));
baseTag.href = 'http://www.base-tag.com'
4. Remove above baseTag added in #3.




Actual results:

After step 4 :
log document.baseURI. Result -  'http://www.base-tag.com'


Expected results:

After step 4 :
log document.baseURI. Result -  'http://www.my-page.com'

This was not happening in FFv34. Also not in Chrome or IE.

Comment 1

[lalitkishore09]

(In reply to lalitkishore09 from comment #0)
> User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101
> Firefox/35.0
> Build ID: 20150108202552
> 
> Steps to reproduce:
> 
> Steps :
> 1. Load a page say 'https://www.my-page.com' [Some https website]
> 2. log document.baseURI. Result -  'http://www.my-page.com'
> 3. Add a base tag :  
> var baseTag = document.head.appendChild(document.createElement('base'));
> baseTag.href = 'http://www.base-tag.com'
> 4. Remove above baseTag added in #3.
> 
> 
> 
> 
> Actual results:
> 
> After step 4 :
> log document.baseURI. Result -  'http://www.base-tag.com'
> 
> 
> Expected results:
> 
> After step 4 :
> log document.baseURI. Result -  'http://www.my-page.com'
> 
> This was not happening in FFv34. Also not in Chrome or IE.

Comment 2

[Loic]

(In reply to lalitkishore09 from comment #0)

> 4. Remove above baseTag added in #3.

With delete baseTag;?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> 1. Load a page say 'http://www.my-page.com'

Clearly not.  And the problem doesn't appear on various other web pages (e.g. on this bug page) either.  It really would have helped to provide a link to the actual page you're seeing this on.

In any case, given the lack of data I'm going to guess this is a regression from bug 1045897, but a link to the actual page you see the problem on would be useful to confirm that.

When the <base> is removed, nsDocument::SetBaseURI is called with null, to indicate "reset back to the document URI".  But the CSP code added in that bug unconditionally (well, as long as there is a CSP) does:

3626     rv = csp->Permits(aURI, nsIContentSecurityPolicy::BASE_URI_DIRECTIVE,
3627                       true, &permitsBaseURI);
3628     NS_ENSURE_SUCCESS(rv, rv);

and CSP will return an error rv when aURI is null.  All that CSP stuff needs to be conditioned on aURI.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> With delete baseTag;?

No, with baseTag.remove()

Comment 5

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1121857_csp_base_uri_regression.patch

Thanks Boris, you are totally right!

Sid, do we need additional tests?
test/csp/test_base-uri.html still passes.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking for all upcoming branches, also for 35 as a potential ride along if there was a low risk fix available.  Tyler - just ni? on you for a heads up that in the wild this might look like reports of site breakage along the lines of no page loading or incorrect page loading.

Comment 7

[lalitkishore09]

For those who are not able to reproduce this :
load https://www.facebook.com [A valid https site]
console.log(document.baseURI);
var baseTag = document.head.appendChild(document.createElement('base'));
baseTag.href = 'http://www.base-tag.com';

console.log(document.baseURI);
document.head.remove(baseTag);

// Even after removing the baseTag. baseURI is still 'http://www.base-tag.com'
console.log(document.baseURI);
Expected : document.baseURI : https://www.facebook.com

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8549751 [details] [diff] [review]
bug_1121857_csp_base_uri_regression.patch

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

r=me, but don't land without test coverage.  Our tests did not catch this behavior, and should: removing the base element from a document doesn't get rejected by CSP.  We can probably do this with some code like in comment 7.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Christoph, could you land this patch asap and nominate for uplift for aurora, beta & release branches? We are going to take it in a potential 35.0.1. Thanks

Comment 10

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Sylvestre Ledru [:sylvestre] from comment #9)
> Christoph, could you land this patch asap and nominate for uplift for
> aurora, beta & release branches? We are going to take it in a potential
> 35.0.1. Thanks

We still need tests for it, but I can definitely land this week.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Christoph, sorry for the pressure but this is one of the last patch we expect for 35.0.1.
I am requesting the checkin-needed. Could you fill the uplift requests for aurora, beta & release? thanks

Comment 12

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Sylvestre Ledru [:sylvestre] from comment #11)
> Christoph, sorry for the pressure but this is one of the last patch we
> expect for 35.0.1.
> I am requesting the checkin-needed. Could you fill the uplift requests for
> aurora, beta & release? thanks

Working on the testcases at the moment. Clearing the needinfo for now. Will land the patch including the test as soon as we have the test reviewed.

Comment 13

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1121857_csp_base_uri_regression_tests.patch

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

Lets not rush the tests.  

Chris: Mark this bug leave-open, land the patch that's been reviewed and uplift it once it's settled.  We'll finish the tests on a normal/low-pressure schedule and land them afterwards since they're only here to catch regression of this bug anyway and don't need to be uplifted.

Comment 15

[Christoph Kerschbaumer [:ckerschb}]

As Sid suggested, I am marking this one as a leave-open. Just landing the patch as of now, so we can uplift the patch. The testcase we can land later.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3e15ba0fd92

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/86d5480e21ee along with bug 1122445 for cpp unit test failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=5725391&repo=mozilla-inbound

Comment 17

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Wes Kocher (:KWierso) from comment #16)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/86d5480e21ee along
> with bug 1122445 for cpp unit test failures:
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=5725391&repo=mozilla-
> inbound

Sorry about that, the problem is within bug 1122445 - got the test updated there!

Comment 18

[Christoph Kerschbaumer [:ckerschb}]

Alrighty, here we go:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4f06ceb8921

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b4f06ceb8921

Comment 20

[Christoph Kerschbaumer [:ckerschb}]

Comment on attachment 8549751 [details] [diff] [review]
bug_1121857_csp_base_uri_regression.patch

Approval Request Comment
[Feature/regressing bug #]:
Implement base-uri for CSP - Bug 1045897.

[User impact if declined]:
Users that define a CSP (not necessarily using the base-uri directive) get false positives if dom elements get removed that try to revert the baseURI to the original baseURI of the page.

[Describe test coverage new/current, TreeHerder]:
We have a testcase within that bug, it's not landed on mc central yet.

[Risks and why]:
Risk of uplift is very low since it only affects pages deploying CSP.

[String/UUID change made/needed]:
no

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1bbeed92ac70
https://hg.mozilla.org/releases/mozilla-beta/rev/a9b183f77f8d
https://hg.mozilla.org/releases/mozilla-release/rev/104d5a02e6ac

Comment 22

[Kohei Yoshino [:kohei]]

https://developer.mozilla.org/en-US/Firefox/Releases/35/Site_Compatibility#DOM

Comment 23

[Petruta Horea [:phorea]]

Reproduced the issue on Firefox 35.0 RC.

Verified as fixed on Firefox 35.0.1, Firefox 36 beta 3, latest Developer Edition 37.0a2 and latest Nightly 38.0a1 (2015-01-23) under Win 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5.

Comment 24

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8552619 [details] [diff] [review]
bug_1121857_csp_base_uri_regression_tests.patch

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

::: dom/base/test/csp/test_null_baseuri.html
@@ +41,5 @@
> +  try {
> +    document.getElementById("testframe").removeEventListener('load', checkBaseURI, false);
> +    var testframe = document.getElementById("testframe");
> +    var baseURI = testframe.contentWindow.document.baseURI;
> +    ok(baseURI.startsWith(EXPECTED_BASEURI), "baseURI should be " + EXPECTED_BASEURI);

The document "load" event may get fired before the base URI messing-around happens (because the inline script hasn't executed).  Can you instead use postmessage to call back after the script runs?

Comment 25

[Sid Stamm [:geekboy or :sstamm]]

Flagging Chris for need-info since this bug has been closed (despite the leave-open keyword).

Comment 26

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1121857_csp_base_uri_regression_tests.patch

Hey Sid, sorry this took a while. Anyway, using postMessage for the tests makes it actually more robust and we can do 3 checks of the base-uri: a) before, b) after modifying the base by appending a child, and c) after removing that child again.

Comment 27

[Christoph Kerschbaumer [:ckerschb}]

Pushing the missing test; once landed we can mark this bug as fixed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/8f54679a7951

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f54679a7951

Comment 29

[Christoph Kerschbaumer [:ckerschb}]

Test landed on mc; marking this bug as resolved.

Comment 30

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.