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

RESOLVED FIXED in Firefox 35

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: lalitkishore09, Assigned: ckerschb)

Tracking

(4 keywords)

35 Branch
mozilla38
x86_64
Windows 7
dev-doc-complete, leave-open, regression, site-compat
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox35+ verified, firefox36+ verified, firefox37+ verified, firefox38+ verified, relnote-firefox 35+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
(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.
Summary: document.baseURI does not get updated to document.location after base tag is removed from DOM → document.baseURI does not get updated to document.location after base tag is removed from DOM for https website.

Updated

3 years ago
Component: General → DOM: Core & HTML

Comment 2

3 years ago
(In reply to lalitkishore09 from comment #0)

> 4. Remove above baseTag added in #3.

With delete baseTag;?
> 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.
Assignee: nobody → mozilla
Blocks: 1045897
status-firefox35: --- → affected
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → affected
tracking-firefox35: --- → ?
tracking-firefox36: --- → ?
tracking-firefox37: --- → ?
tracking-firefox38: --- → ?
Keywords: regression
Summary: document.baseURI does not get updated to document.location after base tag is removed from DOM for https website. → document.baseURI does not get updated to document.location after base tag is removed from DOM for site with a CSP
> With delete baseTag;?

No, with baseTag.remove()
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 5

3 years ago
Created attachment 8549751 [details] [diff] [review]
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.
Attachment #8549751 - Flags: review?(sstamm)
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.
tracking-firefox35: ? → +
tracking-firefox36: ? → +
tracking-firefox37: ? → +
tracking-firefox38: ? → +
Flags: needinfo?(tdowner)
(Reporter)

Comment 7

3 years ago
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 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.
Attachment #8549751 - Flags: review?(sstamm) → review+
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
Flags: needinfo?(mozilla)
(Assignee)

Comment 10

3 years ago
(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.
Flags: needinfo?(mozilla)
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
Flags: needinfo?(mozilla)
Keywords: checkin-needed
(Assignee)

Comment 12

3 years ago
(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.
Flags: needinfo?(mozilla)
Keywords: checkin-needed
(Assignee)

Comment 13

3 years ago
Created attachment 8552619 [details] [diff] [review]
bug_1121857_csp_base_uri_regression_tests.patch
Attachment #8552619 - Flags: review?(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.
(Assignee)

Comment 15

3 years ago
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
Keywords: leave-open
Target Milestone: --- → mozilla38
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
Flags: needinfo?(mozilla)
(Assignee)

Comment 17

3 years ago
(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!
Flags: needinfo?(mozilla)
(Assignee)

Comment 18

3 years ago
Alrighty, here we go:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4f06ceb8921
https://hg.mozilla.org/mozilla-central/rev/b4f06ceb8921
(Assignee)

Comment 20

3 years ago
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
Attachment #8549751 - Flags: approval-mozilla-release?
Attachment #8549751 - Flags: approval-mozilla-beta?
Attachment #8549751 - Flags: approval-mozilla-aurora?
Attachment #8549751 - Flags: approval-mozilla-release?
Attachment #8549751 - Flags: approval-mozilla-release+
Attachment #8549751 - Flags: approval-mozilla-beta?
Attachment #8549751 - Flags: approval-mozilla-beta+
Attachment #8549751 - Flags: approval-mozilla-aurora?
Attachment #8549751 - Flags: approval-mozilla-aurora+

Updated

3 years ago
Keywords: dev-doc-needed, site-compat
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
status-firefox35: affected → fixed
status-firefox36: affected → fixed
status-firefox37: affected → fixed
status-firefox38: affected → fixed
Flags: in-testsuite?
https://developer.mozilla.org/en-US/Firefox/Releases/35/Site_Compatibility#DOM
Keywords: dev-doc-needed → dev-doc-complete
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.
status-firefox35: fixed → verified
status-firefox36: fixed → verified
status-firefox37: fixed → verified
status-firefox38: fixed → verified
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
relnote-firefox: --- → 35+
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?
Attachment #8552619 - Flags: review?(sstamm)
Flagging Chris for need-info since this bug has been closed (despite the leave-open keyword).
Flags: needinfo?(mozilla)
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 26

3 years ago
Created attachment 8561665 [details] [diff] [review]
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.
Attachment #8552619 - Attachment is obsolete: true
Flags: needinfo?(mozilla)
Attachment #8561665 - Flags: review?(sstamm)
Attachment #8561665 - Flags: review?(sstamm) → review+
(Assignee)

Comment 27

3 years ago
Pushing the missing test; once landed we can mark this bug as fixed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/8f54679a7951
https://hg.mozilla.org/mozilla-central/rev/8f54679a7951
(Assignee)

Comment 29

3 years ago
Test landed on mc; marking this bug as resolved.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Flags: needinfo?(tdowner)
You need to log in before you can comment on or make changes to this bug.