Refactor nsHTMLDocument::SetDomain's core logic to standalone algorithm

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
P1
enhancement
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: jcj, Assigned: jcj)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [webauthn], URL)

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

8 months ago
The W3C Web Authentication spec now makes reference [1] to the new HTML "is a registrable domain suffix of or is equal to" algorithm [2]. This bug is to refactor nsHTMLDocument::SetDomain to utilize a method like nsHTMLDocument::IsRegistrableDomainSuffixOfOrIsEqualTo() and expose that resulting method for WebAuthn's implementation (to be handled in Bug 1329764).

[1] https://w3c.github.io/webauthn/#makeCredential
[2] https://html.spec.whatwg.org/multipage/browsers.html#is-a-registrable-domain-suffix-of-or-is-equal-to
Why does any new spec rely on the horrible .domain feature?
Has it been considered to not rely on domain stuff in WebAuth?

Comment 2

8 months ago
https://github.com/w3ctag/spec-reviews/issues/97#issuecomment-175766580 is the rationale I got when I asked. There's also some discussion in bug 1329764 pointing out how it's not really like document.domain, but just wanting to use its Public Suffix logic (which is still bad, but alas).
Comment hidden (mozreview-request)
Attachment #8843094 - Flags: review?(kyle) → review?(bugs)
I'm not super familiar with the HTML spec, so letting someone else with more experience take a look.

Comment 5

8 months ago
J.C., it's probably worth filing a follow-up bug to see if our cookies implementation can use the same hook. Those are all the web-facing users of this comparison as far as I know.
(Assignee)

Updated

8 months ago
Blocks: 1344226

Comment 6

8 months ago
mozreview-review
Comment on attachment 8843094 [details]
Bug 1342258 - Refactor SetDomain to IsRegistrableDomainSuffixOfOrEqualTo

https://reviewboard.mozilla.org/r/116836/#review118832

This needs some refactoring, so that we don't end up having so much duplicated code.
I guess there could be some internal method which does all the logic IsRegistrableDomainSuffixOfOrEqualTo does, but also returns the new uri.
And then IsRegistrableDomainSuffixOfOrEqualTo would just return true if the returned uri isn't null or something. And SetDomain would use new uri
to call NodePrincipal()->SetDomain(newURI); etc.

::: dom/html/nsHTMLDocument.cpp:936
(Diff revision 1)
> +  if (NS_FAILED(rv)) {
> +    return false;
> +  }
> +
> +  // We use SetHostAndPort because we want to reset the port number if needed.
> +  rv = newURI->SetHostAndPort(NS_ConvertUTF16toUTF8(aDomain));

So this is replicating lots of the ::SetDomain method. It should all be only here.

::: dom/html/nsHTMLDocument.cpp:962
(Diff revision 1)
> +    if (!tldService) {
> +      return false;
> +    }
> +
> +    nsAutoCString currentBaseDomain;
> +    ok = NS_SUCCEEDED(tldService->GetBaseDomainFromHost(domain, 0, currentBaseDomain));

Please explain the change from GetBaseDomain() to GetBaseDomainFromHost().

There is the major difference that GetBaseDomain calls NS_GetInnermostURI(aURI);
Why do we not want that?

::: dom/html/nsHTMLDocument.cpp:994
(Diff revision 1)
>      rv.Throw(NS_ERROR_DOM_BAD_DOCUMENT_DOMAIN);
>      return;
>    }
>  
>    // Create new URI
>    nsCOMPtr<nsIURI> uri = GetDomainURI();

So here we have current uri

::: dom/html/nsHTMLDocument.cpp:1001
(Diff revision 1)
>    if (!uri) {
>      rv.Throw(NS_ERROR_FAILURE);
>      return;
>    }
>  
>    nsCOMPtr<nsIURI> newURI;

and here new uri, and both uris are recreated in the new method
Attachment #8843094 - Flags: review?(bugs) → review-
(Assignee)

Comment 7

8 months ago
(In reply to Olli Pettay [:smaug] from comment #6)
> This needs some refactoring, so that we don't end up having so much
> duplicated code.
> I guess there could be some internal method which does all the logic
> IsRegistrableDomainSuffixOfOrEqualTo does, but also returns the new uri.
> And then IsRegistrableDomainSuffixOfOrEqualTo would just return true if the
> returned uri isn't null or something. And SetDomain would use new uri
> to call NodePrincipal()->SetDomain(newURI); etc.

OK, this makes sense. That's really the kind of feedback I needed, but I figured I'd give a try here that we could comment from. I'll rework it to look like this, hit the points you made below, and get an updated patch to you.

Thanks for the help, Olli!
(Assignee)

Comment 8

8 months ago
mozreview-review-reply
Comment on attachment 8843094 [details]
Bug 1342258 - Refactor SetDomain to IsRegistrableDomainSuffixOfOrEqualTo

https://reviewboard.mozilla.org/r/116836/#review118832

I've refactored `IsRegistrableDomainSuffixOfOrEqualTo` and `SetDomain` to call a new protected method, `RegistrableDomainSuffixOfInternal`. That method takes a URI and a host (string), returning the provided host as a URI if it's permissible, `nullptr` otherwise. 

Both `RegistrableDomainSuffixOfInternal` and `IsRegistrableDomainSuffixOfOrEqualTo` then need to (separately) create `nsIURI` objects from a string, so I pulled that logic into another protected method `CreateInheritingURIForHost`, taking a string and returning an `nsIURI` cloned from `GetDomainURI`.

The diff is less clear this time, I'm afraid. I could pull this into two patches, one that adds the methods and one that modifies `SetDomain` if that'd be clearer.

> So this is replicating lots of the ::SetDomain method. It should all be only here.

Since this needs to happen in the SetDomain case and in the IsRegistrableDomainSuffixOfOrEqualTo case separately, I pulled the logic for creating a URI that inherits from the DomainURI / NodePrincipal into its own protected method, CreateInheritingURIForHost.

> Please explain the change from GetBaseDomain() to GetBaseDomainFromHost().
> 
> There is the major difference that GetBaseDomain calls NS_GetInnermostURI(aURI);
> Why do we not want that?

We do want that, so I've made the refactored RegistrableDomainSuffixOfInternal method take an nsIURI that we can still call GetBaseDomain on. SetDomain still sues GetDomainURI to populate that nsIURI argument, so it's back to as it was.

> and here new uri, and both uris are recreated in the new method

Fair. Caught in the refactoring.
Comment hidden (mozreview-request)
(Assignee)

Comment 10

7 months ago
Comment on attachment 8843094 [details]
Bug 1342258 - Refactor SetDomain to IsRegistrableDomainSuffixOfOrEqualTo

Olli, I think this is ready for re-review when you have a chance. Thanks!
Attachment #8843094 - Flags: review?(bugs)

Comment 11

7 months ago
mozreview-review
Comment on attachment 8843094 [details]
Bug 1342258 - Refactor SetDomain to IsRegistrableDomainSuffixOfOrEqualTo

https://reviewboard.mozilla.org/r/116836/#review123546

Interestingly mozreview was really bad for this patch, so I downloaded the raw patch and reviewed that.
Attachment #8843094 - Flags: review?(bugs) → review+
(Assignee)

Comment 12

7 months ago
Thanks for the detailed reviews, :smaug! Marking checkin-needed.
Keywords: checkin-needed

Comment 13

7 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ffa2f50d49ce
Refactor SetDomain to IsRegistrableDomainSuffixOfOrEqualTo r=smaug
Keywords: checkin-needed
Backed out for failing mochitest dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html:

https://hg.mozilla.org/integration/autoland/rev/2cafc12d8708e75e06e817c6df19a74a798e70dd

Push for which failing tests ran: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=34b87d7ffe4ebd0b60632bcd6adab13decd368a0&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=85124274&repo=autoland

[task 2017-03-20T19:50:00.150919Z] 19:50:00     INFO - TEST-START | dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html
[task 2017-03-20T19:50:00.531720Z] 19:50:00     INFO - TEST-INFO | started process screentopng
[task 2017-03-20T19:50:01.958454Z] 19:50:01     INFO - TEST-INFO | screentopng: exit 0
[task 2017-03-20T19:50:01.963348Z] 19:50:01     INFO - Buffered messages logged at 19:50:00
[task 2017-03-20T19:50:01.966919Z] 19:50:01     INFO - TEST-PASS | dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html | wrong sender 
[task 2017-03-20T19:50:01.970743Z] 19:50:01     INFO - TEST-PASS | dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html | unexpected data: idn-whitelist-response error-thrown-setting-to-idn(N,S,_,E,R,R,O,R,_,D,O,M,_,B,A,D,_,D,O,C,U,M,E,N,T,_,D,O,M,A,I,N,:, ,I,l,l,e,g,a,l, ,d,o,c,u,m,e,n,t,.,d,o,m,a,i,n, ,v,a,l,u,e) 
[task 2017-03-20T19:50:01.976833Z] 19:50:01     INFO - TEST-PASS | dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html | wrong source 
[task 2017-03-20T19:50:01.979359Z] 19:50:01     INFO - Buffered messages finished
[task 2017-03-20T19:50:01.984800Z] 19:50:01     INFO - TEST-UNEXPECTED-FAIL | dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html | wrong response for IDN - got "idn-whitelist-response error-thrown-setting-to-idn(N,S,_,E,R,R,O,R,_,D,O,M,_,B,A,D,_,D,O,C,U,M,E,N,T,_,D,O,M,A,I,N,:, ,I,l,l,e,g,a,l, ,d,o,c,u,m,e,n,t,.,d,o,m,a,i,n, ,v,a,l,u,e)", expected "idn-whitelist-response"
[task 2017-03-20T19:50:01.987334Z] 19:50:01     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:310:5
[task 2017-03-20T19:50:01.992759Z] 19:50:01     INFO -     receiveMessage@dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html:122:7
[task 2017-03-20T19:50:01.998183Z] 19:50:01     INFO -     EventListener.handleEvent*@dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html:206:1
[task 2017-03-20T19:50:02.001618Z] 19:50:02     INFO - TEST-PASS | dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html | IDN whitelist message not received 
[task 2017-03-20T19:50:02.004400Z] 19:50:02     INFO - TEST-PASS | dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html | wrong sender 
[task 2017-03-20T19:50:02.009436Z] 19:50:02     INFO - TEST-PASS | dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html | unexpected data: punycode-whitelist-response 
[task 2017-03-20T19:50:02.014596Z] 19:50:02     INFO - TEST-PASS | dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html | wrong source 
[task 2017-03-20T19:50:02.020942Z] 19:50:02     INFO - TEST-PASS | dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html | wrong response for punycode 
[task 2017-03-20T19:50:02.023684Z] 19:50:02     INFO - TEST-PASS | dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html | punycode whitelist message not received 
[task 2017-03-20T19:50:02.028395Z] 19:50:02     INFO - TEST-FAIL | dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html | wrong sender - got "http://sub1.xn--exaple-kqf.test", expected "http://sub1.exaмple.test"
[task 2017-03-20T19:50:02.031191Z] 19:50:02     INFO - TEST-FAIL | dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html | wrong sender - didn't expect "http://sub1.xn--exaple-kqf.test", but got it
[task 2017-03-20T19:50:02.037016Z] 19:50:02     INFO - TEST-PASS | dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html | unexpected data: idn-nowhitelist-response error-thrown-setting-to-idn(N,S,_,E,R,R,O,R,_,D,O,M,_,B,A,D,_,D,O,C,U,M,E,N,T,_,D,O,M,A,I,N,:, ,I,l,l,e,g,a,l, ,d,o,c,u,m,e,n,t,.,d,o,m,a,i,n, ,v,a,l,u,e) 
[task 2017-03-20T19:50:02.042328Z] 19:50:02     INFO - TEST-PASS | dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html | wrong source 
[task 2017-03-20T19:50:02.045342Z] 19:50:02     INFO - Not taking screenshot here: see the one that was previously logged
[task 2017-03-20T19:50:02.049720Z] 19:50:02     INFO - TEST-UNEXPECTED-FAIL | dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html | wrong response for IDN - got "idn-nowhitelist-response error-thrown-setting-to-idn(N,S,_,E,R,R,O,R,_,D,O,M,_,B,A,D,_,D,O,C,U,M,E,N,T,_,D,O,M,A,I,N,:, ,I,l,l,e,g,a,l, ,d,o,c,u,m,e,n,t,.,d,o,m,a,i,n, ,v,a,l,u,e)", expected "idn-nowhitelist-response"
[task 2017-03-20T19:50:02.052667Z] 19:50:02     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:310:5
[task 2017-03-20T19:50:02.058024Z] 19:50:02     INFO -     receiveMessage@dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html:134:7
[task 2017-03-20T19:50:02.060684Z] 19:50:02     INFO -     EventListener.handleEvent*@dom/tests/mochitest/dom-level0/test_setting_document.domain_idn.html:206:1
Flags: needinfo?(jjones)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

7 months ago
I apologize for the back-out; I had thought this got a full mochitest run before marking it for check-in. This patch seems to fix the issues, and a full mochitest try run is in progress.

Smaug: Sorry for the churn. I owe you one.
Flags: needinfo?(jjones)
(Assignee)

Comment 17

7 months ago
Comment on attachment 8843094 [details]
Bug 1342258 - Refactor SetDomain to IsRegistrableDomainSuffixOfOrEqualTo

Sincere apologies about this backout, smaug. This diff fixes the IDN handling by ensuring the Host provided is parsed. The try run of mochitests [1] looks good, so this should be right this time.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=34b7301f502a6aff875be1b00ed9638d8d74c2db
Attachment #8843094 - Flags: review+ → review?(bugs)

Comment 18

7 months ago
mozreview-review
Comment on attachment 8843094 [details]
Bug 1342258 - Refactor SetDomain to IsRegistrableDomainSuffixOfOrEqualTo

https://reviewboard.mozilla.org/r/116836/#review125524

::: dom/html/nsHTMLDocument.cpp:959
(Diff revisions 2 - 3)
> +  }
> +
>    // Check new domain - must be a superdomain of the current host
>    // For example, a page from foo.bar.com may set domain to bar.com,
>    // but not to ar.com, baz.com, or fi.foo.bar.com.
> -  nsAutoCString domain = NS_ConvertUTF16toUTF8(aNewDomain);
> +  nsAutoCString current, domain;

nit, variable definitions to their own lines
Attachment #8843094 - Flags: review?(bugs) → review+
(Assignee)

Comment 19

7 months ago
mozreview-review-reply
Comment on attachment 8843094 [details]
Bug 1342258 - Refactor SetDomain to IsRegistrableDomainSuffixOfOrEqualTo

https://reviewboard.mozilla.org/r/116836/#review125524

> nit, variable definitions to their own lines

Thanks! I had copied this from the current-in-tree code [1], but changed it for the landing rev.

[1] http://searchfox.org/mozilla-central/source/dom/html/nsHTMLDocument.cpp#962
Comment hidden (mozreview-request)
(Assignee)

Comment 21

7 months ago
Let's try this again, good sheriffs!
Keywords: checkin-needed

Comment 22

7 months ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1147dd18f1d9
Refactor SetDomain to IsRegistrableDomainSuffixOfOrEqualTo r=smaug
Keywords: checkin-needed

Comment 23

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1147dd18f1d9
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.