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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P1)

Description

[J.C. Jones [:jcj] (he/him)]

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

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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

[Anne (:annevk)]

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 3

[J.C. Jones [:jcj] (he/him)]

Attachment: Bug 1342258 - Refactor SetDomain to IsRegistrableDomainSuffixOfOrEqualTo

This commit refactors the SetDomain method in a Document to call a new function
IsRegistrableDomainSuffixOfOrEqualTo(), defined in HTML [1]. This commit tries
not to rename anything except input variables, so as to remain as clear as
possible. It likely should have various variables renamed, but given the
author's unfamiliarity with this module, review seems a good time to do that.
It's also duplicating comments a little bit; let me know which one(s) you'd like
to keep!

To avoid the use of URI types, this changes the use of the tldService from
GetBaseDomain() to GetBaseDomainFromHost().

Note: Commentary on the HTML change is available in the PR [2], and the
rationale for this behavior in Web Auentication, where this algorithm will be
used, is also recorded [3].

[1] https://html.spec.whatwg.org/multipage/browsers.html#is-a-registrable-domain-suffix-of-or-is-equal-to
[2] https://github.com/whatwg/html/pull/2365
[3] https://github.com/w3ctag/spec-reviews/issues/97#issuecomment-175766580

Review commit: https://reviewboard.mozilla.org/r/116836/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116836/

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I'm not super familiar with the HTML spec, so letting someone else with more experience take a look.

Comment 5

[Anne (:annevk)]

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.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 7

[J.C. Jones [:jcj] (he/him)]

(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!

Comment 8

[J.C. Jones [:jcj] (he/him)]

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 9

[J.C. Jones [:jcj] (he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116836/diff/1-2/

Comment 10

[J.C. Jones [:jcj] (he/him)]

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!

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 12

[J.C. Jones [:jcj] (he/him)]

Thanks for the detailed reviews, :smaug! Marking checkin-needed.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ffa2f50d49ce
Refactor SetDomain to IsRegistrableDomainSuffixOfOrEqualTo r=smaug

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 15

[J.C. Jones [:jcj] (he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116836/diff/2-3/

Comment 16

[J.C. Jones [:jcj] (he/him)]

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.

Comment 17

[J.C. Jones [:jcj] (he/him)]

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

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 19

[J.C. Jones [:jcj] (he/him)]

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 20

[J.C. Jones [:jcj] (he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116836/diff/3-4/

Comment 21

[J.C. Jones [:jcj] (he/him)]

Let's try this again, good sheriffs!

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1147dd18f1d9
Refactor SetDomain to IsRegistrableDomainSuffixOfOrEqualTo r=smaug

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1147dd18f1d9