Closed Bug 918768 Opened 11 years ago Closed 5 years ago

[XHR2] responseXML.cookie is undefined, cookie property expected on Document

Categories

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

defect

Tracking

()

RESOLVED DUPLICATE of bug 144795

People

(Reporter: hsteen, Assigned: wisniewskit)

References

(Blocks 1 open bug, )

Details

Attachments

(2 files, 3 obsolete files)

Test case:
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/responsexml-document-properties.htm

Spec clarification here:
http://lists.w3.org/Archives/Public/public-webapps/2013JulSep/0634.html

So bug 56622 may no longer be invalid ;)

Naturally, this doesn't have much to do with XHR. I'm not sure what the right component is..
smaug, I'm guessing that this this basically involves just moving the cookie-related methods from HTMLDocument to Document, as one might expect? (essentially making this part of bug 897815)

Note that Chrome and WebKit both pass these tests (and Edge passes the first one of the two), so it's definitely something we need to deal with for webcompat.
Flags: needinfo?(bugs)
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
Here's a patch to pass the tests. Mostly it's just moving the GetCookie/SetCookie (and related) code into Document, but it also adds Document::IsCookieAverse() in order to match that spec requirement: https://html.spec.whatwg.org/multipage/dom.html#dom-document-cookie

A try run is fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b53fd51038b3
Assignee: nobody → wisniewskit
Status: NEW → ASSIGNED
Flags: needinfo?(bugs)
Attachment #8797248 - Flags: review?(bugs)
Do other browsers put .cookie to Document?
Note, the whole HTMLDocument vs. Document is still unclear in the spec.
Ah, you mentioned the other browsers.
Comment on attachment 8797248 [details] [diff] [review]
918768-move_cookie_getting_and_setting_code_into_Document.diff

>+nsDocument::IsCookieAverse()
>+{
>+  // See https://html.spec.whatwg.org/multipage/dom.html#dom-document-cookie
>+
>+  // We're cookie-averse if we have no browsing context
>+  if (!GetShell()) {
That doesn't mean no browsing context.
You'd need to check whether there is docshell/container.
But, in practice, this could return true if the document is a data document.
LoadedAsData() && IsLoadedAsInteractiveData() (latter is for XBL)




>+  // We're cookie-averse if we don't have a
>+  // network scheme ("ftp", "http", or "https")
>+  nsCOMPtr<nsIPrincipal> principal = GetPrincipal();
>+  nsCOMPtr<nsIURI> uri;
>+  principal->GetURI(getter_AddRefs(uri));
You're checking principal here, not document's URL's scheme.
Principal can be inherited from opener or so, even if document is a data: url or about:blank or so.
Sounds like we're missing some wpt tests. Want to write some testing the cases when http: has a about:blank iframe or so and the iframe
tries to use document.cookie?


>+nsDocument::SetCookie(const nsAString& aCookie, ErrorResult& rv)
>+{
>+  if (mDisableCookieAccess) {
>+    return;
>+  }
>+
>+  // If the document's sandboxed origin flag is set, access to write cookies
>+  // is prohibited.
>+  if (mSandboxFlags & SANDBOXED_ORIGIN) {
>+    rv.Throw(NS_ERROR_DOM_SECURITY_ERR);
>+    return;
>+  }
>+
>+  if (IsCookieAverse()) {
>+    return;
>+  }
This 'if' should be before the sandbox check per spec, right? The whole point with averse is that it does nothing.

>   /**
>+   * Disables getting and setting cookies
>+   */
>+  virtual void DisableCookieAccess() = 0;
>+  NS_IMETHOD GetCookie(nsAString& aCookie) = 0;
>+  NS_IMETHOD SetCookie(const nsAString& aCookie) = 0;
Don't put NS_IMETHOD stuff here. virtual nsresult is good enough, same in .cpp


>     if (isCrossSite) {
>-      nsCOMPtr<nsIHTMLDocument> htmlDoc = do_QueryInterface(mResponseXML);
>-      if (htmlDoc) {
>-        htmlDoc->DisableCookieAccess();
>+      nsCOMPtr<nsIDocument> doc = do_QueryInterface(mResponseXML);
>+      if (doc) {
>+        doc->DisableCookieAccess();
Why this is needed now that you add averse-thingie?
Attachment #8797248 - Flags: review?(bugs) → review-
>Want to write some tests?

Sure, when I get the chance. I take it that they ought to be part of this patch before we land it?


>IsLoadedAsData() && IsLoadedAsInteractiveData() (latter is for XBL)

XHR documents are IsLoadedAsData() but not IsLoadedAsInteractiveData(), so I'll have to either keep the DisableCookieAccess() stuff, or this check isn't quite right (should it actually be an "or" there instead of an "and")?


>This 'if' should be before the sandbox check per spec, right?

Yeah, I'll fix that in the next version of the patch. The problem I was running into that made me re-order the checks was caused by me using the principal instead of mDocumentURI, so it's a non-issue now.


>Don't put NS_IMETHOD stuff here.

Sure, I'll drop the NS_IMETHOD variants.
Flags: needinfo?(bugs)
(In reply to Thomas Wisniewski from comment #6)
> >IsLoadedAsData() && IsLoadedAsInteractiveData() (latter is for XBL)
> 
> XHR documents are IsLoadedAsData() but not IsLoadedAsInteractiveData(), so
> I'll have to either keep the DisableCookieAccess() stuff, or this check
> isn't quite right (should it actually be an "or" there instead of an "and")?
er, my mistake. ||, not &&
Flags: needinfo?(bugs)
Alright, here's a version of the patch addressing your review comments, aside from adding more tests. I'll try to find some time to add tests soon, but this is a busy week for me so it may need to wait.

Try seems fine with the patch as-is: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea3c5d658119b3f9026e66c447d44489649e2fcc
Attachment #8797248 - Attachment is obsolete: true
Attached patch tests-cookie-aversion.diff (obsolete) — Splinter Review
Here's a patch to add web platform tests for checking whether documents are correctly cookie-averse.

Basically, it checks that documents from about, data, and other such URLs *are* cookie-averse, and also checks that ones loaded via http/https are *not* cookie-averse (at least, all the cases I can check in the WPTs). This should cover the spec (which says that docs must have browsing context and be http/https/ftp, or they're averse).

Does that seem sufficient to you? If so, I'll request a review from a WPT peer.
Attachment #8800103 - Flags: review?(bugs)
Comment on attachment 8800103 [details] [diff] [review]
tests-cookie-aversion.diff

>+    test(function() {
>+      var blob = new Blob(["<html>blob:</html>"], {type: "text/html"}),
>+          blobURL = URL.createObjectURL(blob);
>+      document.getElementById("blob_iframe").src = blobURL;
>+      test_document_is_cookie_averse(self[4].document);
So here you aren't testing what you're trying to test, as far as I see.
Loading is asynchronous, so self[4].document points still to the previous document.

>+      URL.revokeObjectURL(blobURL);
>+    }, "blob: documents are cookie averse");
>+
>+    test(function() {
>+      var requestFS = window.requestFileSystem ||
>+                      window.webkitRequestFileSystem;
>+      if (typeof requestFS === "undefined") {
>+        document.getElementById("filesystem_iframe").src = "javascript:'filesystem: (not supported)'";
>+        this.set_status(this.NOTRUN, "File System API not supported, cannot run test.");
>+        return;
>+      }
>+
>+      var fail = function(e) {
>+        assert_unreached("problem making filesystem URL: " + e.toString());
>+      };
>+
>+      var me = this;
>+      requestFS(window.TEMPORARY, 1024, function(fs) {
>+        fs.root.getFile('test.html', {create: true}, function(entry) {
>+          entry.createWriter(function(writer) {
>+            writer.onwriteend = function() {
>+              document.getElementById("filesystem_iframe").src = entry.toURL();
>+              test_document_is_cookie_averse(self[5].document);
>+            };
>+            writer.onerror = fail;
>+            var blob = new Blob(['<html>filesystem:</html>'], {type: 'text/html'});
>+            writer.write(blob);
>+          }, fail);
>+        }, fail);
>+      }, fail);
>+    }, "filesystem: documents are cookie averse");
What is this Chrome proprietary API stuff doing here. Please remove this test.
(Or make the test pass only in case requestFileSystem and webkitRequestFileSystem aren't in the window object )
Attachment #8800103 - Flags: review?(bugs) → review-
>Loading is asynchronous, so self[4].document points still to the previous document.

Right.. I'll modify the test appropriately.


>What is this Chrome proprietary API stuff doing here. Please remove this test.

Sure, I don't mind removing it. The spec just mentions the "filesystem" protocol specifically, so I figured it would be best to test it even if the FileSystem API did not gain traction.
Attached patch tests-cookie-aversion.diff (obsolete) — Splinter Review
Alright, I've removed the FileSystem API test and adjusted the blob code to use an onload handler. Is this more agreeable?
Attachment #8800103 - Attachment is obsolete: true
Attachment #8800315 - Flags: review?(bugs)
Actually, here's a new version with a better async-based blob test. Again, the FileSystem API test is done.
Attachment #8800315 - Attachment is obsolete: true
Attachment #8800315 - Flags: review?(bugs)
Attachment #8800355 - Flags: review?(bugs)
Attachment #8800355 - Flags: review?(bugs) → review+
Comment on attachment 8800355 [details] [diff] [review]
tests-cookie-aversion.diff

Anne, mind giving this new web platform test a review? Basically it's just adding tests for document cookie-aversion (at least what I think I can test in the WPTs).
Attachment #8800355 - Flags: review?(annevk)
<meta name=help> -> <link rel=help>

I think about:blank simply inherits the parent's cookie policy (maybe the specification is wrong). What are the test results you get? Maybe the specification is wrong too.
Here are the results of running these tests on existing browsers:
- Firefox passes none of the tests (unless my other patch in this ticket is applied).
- Chrome and WebKit pass only the blob one.
- Edge passes all but the "documents that fail to load" one, and the data URL one (which gives a "permission denied" error).
(Just in case you're waiting for a ni?, Anne, here's one)

I'm not sure what the best course of action would be based on the results I found in comment 16, given how inconsistent the results seem to me.
Flags: needinfo?(annevk)
I don't think we should land the test for now. It might be worth mentioning in https://github.com/whatwg/html/issues/332 though which is the standards issue around this.

If you do want to test this to some extent it's probably best to stay away from testing documents that are associated with a global object and a browsing context. So document.implementation.createDocument and XMLHttpRequest work, but <iframe> does not.

Now, if you're interested in improving the specification, that would be okay too, but I don't really have a concrete plan myself yet for what to do here so you'd be somewhat on your own.

So, sorry. 
Flags: needinfo?(annevk)
Sure, Anne... but are you ok with me landing the other patch that actually changes Firefox to match the spec, or were you suggesting to not bother with it for now? And if you think it's fine to land the other patch, should I perhaps just skip the cookie-aversion bits to better-match Chrome/Webkit for now?


Also, smaug, would you be ok with us just using mochitests for the time-being instead of web-platform tests, until this issue is ironed out, or would you recommend just passing on this ticket for now?
Flags: needinfo?(bugs)
Flags: needinfo?(annevk)
Hmm, based on the spec issue it sounds like we should wait a bit here.
Looks like nsDocument::IsCookieAverse() will need to be changed once the spec if fixed. And landing the current patch might break some pages.
Flags: needinfo?(bugs)
Sure. Then I'll cancel the ni for Anne, as it seems redundant now.
Flags: needinfo?(annevk)
I mean, I still think we could/should fix this for XMLHttpRequest, new Document(), and such. We should just not change anything for <iframe>-like scenarios.
Attachment #8800355 - Flags: review?(annevk)
(In reply to Anne (:annevk) from comment #22)
> I mean, I still think we could/should fix this for XMLHttpRequest, new
> Document(), and such. We should just not change anything for <iframe>-like
> scenarios.

I'm not against that, but I'm not sure how I would go about making such discriminations on how a document is loaded. Plus I'm not sure what to do in the iframe-like case... just throw as though there is no cookie property, to match what's currently being done?

And if we do go that route, should I add any web platform tests for cookie-aversion for the non-iframe-like cases (and if so, which cases should I test exactly? Docs loaded via XHR, fetch, XSL, and JS, I'm guessing?)
Flags: needinfo?(bugs)
Flags: needinfo?(annevk)
Hmm, currently we don't throw for a number of cases, right? <iframe src=about:blank></iframe> etc. Anyway, my thinking was that whenever a document is inside a browsing context it's probably not cookie-averse (and we should for now not adjust what we do), but

  XMLHttpRequest
  document.implementation.createDocument
  new Document

would be.
Flags: needinfo?(annevk)
Hmm. So basically, changing the IsCookieAverse method in my patch to this, for now:

>  if (have browsing context)
>     throw "undefined property 'cookie'" // to match current behavior
>  else
>     return true // cookie-averse

(And dropping all of the network-scheme checks until the spec has settled.)

That's what you had in mind, yes? If so, I'll give it a shot when I'm less ill to see if it works as desired.
Flags: needinfo?(annevk)
Maybe? It seems weird to throw since document.cookie usually works and should remain working. But I might be missing some subtlety.
Flags: needinfo?(annevk)
Yeah, I may be wrong about whether (or what) to throw in that case, I'd have to double-check what's being done right now (it may simply be to return false, now that I think about it).
I think we want to fix https://github.com/whatwg/dom/issues/221 before doing anything here.
Blocks: xhr
No longer blocks: xhr2pass
Priority: -- → P3

This was recently fixed in bug 144795, so I'll close this as a dupe.

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: needinfo?(bugs)
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: