Closed Bug 622088 Opened 9 years ago Closed 9 years ago

XHR referer header is not modified with history.pushState

Categories

(Core :: DOM: Navigation, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -
status2.0 --- wanted

People

(Reporter: jakub, Assigned: justin.lebar+bug)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 7 obsolete files)

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b8) Gecko/20100101 Firefox/4.0b8
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b8) Gecko/20100101 Firefox/4.0b8

When the script changes the location through history.pushState then further requests send the Referer header with the original location and not with the modified one.

Google Chrome works correct.

Reproducible: Always

Steps to Reproduce:
history.pushState('', '', location.href + '?test');
var req = new XMLHttpRequest();
req.open('GET', 'history.php?referer=1');
req.onreadystatechange = function () {
	if (req.readyState == 4) {
		alert(req.responseText);
	}
};
req.send();

Actual Results:  
'http://www.vrana.cz/test/history.php'

Expected Results:  
'http://www.vrana.cz/test/history.php?test'
Jason, do you want to fix, or should I?
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Component: History: Global → Document Navigation
Ever confirmed: true
QA Contact: history.global → docshell
Presumably it's just XMLHttpRequests?  I could have sworn we have a test for regular requests.
Blocks: 500328
I'll try and have a look at this soon.
Assignee: nobody → justin.lebar+bug
Ah.  Indeed, XHR gets its referrer value from the principal, which is unaffected by pushstate.

We really need a better setup for getting the referrer off the JS stack.  :(
Summary: Referer header is not modified with history.pushState → XHR referer header is not modified with history.pushState
I really don't think this is serious enough to block. bz do you disagree?
blocking2.0: ? → -
It's quite important for me (if my vote counts).
It's a sorta-serious bug in one of our new gecko 2.0 features that will force web developers into contorted workarounds.  I agree this should probably not block ship, but I think we should try to fix for gecko 2.0 if we can.
Attached patch Patch v1 (obsolete) — Splinter Review
Pushing to try now.
Attachment #501191 - Flags: review?(bzbarsky)
1)  The principal during Send() may not match mPrincipal (e.g. if the XHR is created in one window but send() is called from another one).  Which one does the spec say to actually use for the referrer?  Maybe we need to save the referrer URI when we save mPrincipal.

2)  GetContextDocumentCurrentURI doesn't do what the documentation says or what the name implies.  It does the right thing, so the documentation and name need to change.
(In reply to comment #10)
> 2)  GetContextDocumentCurrentURI doesn't do what the documentation says or what
> the name implies.  It does the right thing, so the documentation and name need
> to change.

Indeed.  Is it correct that, in the case that we don't use the principal's URI because the document's original URI doesn't match the principal's URI, the function does what the name implies and the documentation says?

Also, was it appropriate to add GetContextFromStack to nsJSUtils?  The comment at the top of the file [1] made it seem like maybe not.

[1] http://hg.mozilla.org/mozilla-central/file/f539d05eec83/dom/base/nsJSUtils.cpp#l41
> Is it correct that, in the case that we don't use the principal's URI
> because the document's original URI doesn't match the principal's URI, the
> function does what the name implies and the documentation says?

No.  The document involved is the document associated with whatever script is running, which might be wholly unrelated to the document that "owns" (the ownership is in the other direction, sorta) the given JSContext.

As for nsJSUtils, it's fine with me, but double-check with jst...
(In reply to comment #10)
> 1)  The principal during Send() may not match mPrincipal (e.g. if the XHR is
> created in one window but send() is called from another one).  Which one does
> the spec say to actually use for the referrer?

Do you know where this might be?  I don't see referer/referrer mentioned in [1].

[1] http://www.w3.org/TR/XMLHttpRequest/#dom-xmlhttprequest-send
(In reply to comment #12)
> As for nsJSUtils, it's fine with me, but double-check with jst...

cc'ing jst.
> I don't see referer/referrer mentioned in [1].

Sounds like some spec work is needed here.
(In reply to comment #15)
> > I don't see referer/referrer mentioned in [1].
> 
> Sounds like some spec work is needed here.

Anne, do you have thoughts on this?

FWIW, it seems to me that the referrer should be the window which calls send(), just as the referrer on a normal navigation is the window which initiates the navigation, not the window being navigated.  But I plead ignorance.
Well, should it be the window that calls send(), the window that calls open(), the window that makes the |new XMLHttpRequest()| call, or the window that the call is made on?  Those 4 can all be different windows.
And fwiw, for a scripted form submit the referrer is the form's document, not the script that called submit().  Assuming that's at all analogous to our situation here.
Maybe we shouldn't spin waiting for the Final Word on the spec here.  We can fix the big problem (pushState doesn't affect referrer) now and then figure out exactly which window should be the referrer later.

bz, which do you think should be the referrer?
I think we should preserve the old behavior for now and get the referrer at the same time as we set mPrincipal.
XMLHttpRequest references HTML5 for "fetch". HTML5 says that you should look at the "entry script's document" in this case. So it is defined.

We could change it however to make it the XMLHttpRequest Document instead. This might be more logical as a lot of XMLHttpRequest logic is tied to the XMLHttpRequest Document. This would require changes to both HTML5 (to let fetch accept a document as argument) and XMLHttpRequest (to make use of that).

I do not feel strongly either way.
OK, so that sounds like the equivalent of getting the "dynamic script context" (as opposed to "static script context", which is what the attached patch does) at the point when send() is called, then.

I'm not sure that makes much sense, necessarily, but in the edge cases nothing we do can make sense....
Is getting the dynamic script context just a matter of replacing 

static already_AddRefed<nsIDocument>
GetFrameDocument(JSContext *cx, JSStackFrame *fp)
{
  [...]
  nsCOMPtr<nsIDOMWindow> window =
    do_QueryInterface(nsJSUtils::GetStaticScriptGlobal(cx, scope));

with GetDynamicScriptGlobal?  Also, if I were to write a test to check that this gets the right referrer, what would I do?
Attachment #501191 - Flags: review?(bzbarsky)
Attachment #501191 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #504217 - Flags: review?(bzbarsky)
Patch v2 uses the dynamic script context for the XHR referrer.

The function names / comments might be off.  Also, I should explicitly test that we're getting the right referrer in the edge case.  But I'm not clear on what that edge case is.
Edge case for which?  A case where dynamic vs script context matters?  Or something else?
> A case where dynamic vs script context matters?

If s/script/static, yes.
Dynamic vs static matters in this situation.  You have two windows A and B with different URLS (say a page and its subframe).  You click a button in window A; the onclick handler is a function in window A which calls a function in window B which does an XHR.  In that situation, the dynamic script context is the one for window A, but the static one is the one for window B.
Either my test or my code is wrong here.  I suspect it's the code, but to check: Here's code which is running inside the main test window.  In this case, the dynamic script context for the inner window should be the outer window?

The referrer I'm getting here is the iframe's location.

var iframeFinishedLoading = false;
function callXHR() {
  if (innerFinishedLoading) {
    var inner = document.getElementById('iframe').contentWindow;
    var referrer = inner.doXHR();
    is (referrer, document.location); // FAILS
    SimpleTest.finish();
  }
  else {
    setTimeout(callXHR, 0); // ugh.
  }
}

callXHR();
> In this case, the dynamic script context for the inner window should be the
> outer window?

Yes.  If it's not, that's really odd.  Can you post a full testcase or mail it to me or something?
Well, it's probably a bug in my code!  I'll post the patch in a moment.
Attachment #504217 - Attachment is obsolete: true
Attachment #504217 - Flags: review?(bzbarsky)
Attached patch Patch v2.1 (added test) (obsolete) — Splinter Review
Attachment #506194 - Flags: feedback?(bzbarsky)
The file in question is test_bug622088_2.html, but presuming that's right, the relevant code is in the nsContentUtils.cpp hunk.

I need to double-check that XHR really is causing the dynamic context branch to run, but I'm pretty sure it is.
Yup, the two XHRs both go through the dynamic script global branch in nsContentUtils.cpp.
Ah.  The problem isn't the GetDynamicScriptGlobal() call -- that works.  The issue is that the principal we get from GetCxSubjectPrincipalAndFrame corresponds to the static context.  So since the dynamic context's original URL doesn't match the principal's URL, we take the principal's.
So the question is: How do I get the right principal in the dynamic case?
In the dynamic case, can't you just get the principal from the document?
Attachment #506194 - Attachment is obsolete: true
Attachment #506194 - Flags: feedback?(bzbarsky)
Attached patch Patch v3 (obsolete) — Splinter Review
Oh.  That was simple.  Only changes relative to v2.1 are in nsContentUtils.cpp.
Attachment #506254 - Flags: review?(bzbarsky)
>+   * Get the current URI associated with the given context, either going
>+   * through its static global or its dynamic global.

How about documenting GetStaticContextURI as "get the current uri associated with the code currently running on cx" and GetDynamicContextURI as "get the current uri associated with the window corresponding to cx"?  That would be clearer, imo.... (and feel free to rename the functions as you do this if you think of better names!).

> +  // First, we get the document corresponding to fp.

That's not quite true anymore.

Looks good to me otherwise, but need the failures sorted out, right?  The ctypes thing looks unlikely to be you... can you reproduce the others?
The ctypes certainly does appear not to be from this.  And the other one (timeouts in test_bug277724.html, test_bug172261.html) disappeared when I pushed without bug 615501, so presumably that bug (or its test) is doing something wrong.

I was pretty sure I was building off a green changeset!  But I'll update and push again.
Comment on attachment 506254 [details] [diff] [review]
Patch v3

r=me with the comments fixed.
Attachment #506254 - Flags: review?(bzbarsky) → review+
I think the errors on try are just a simple null pointer dereference.  I'll fix and push again.

> // XXX Do I need to enter a compartment here?
> window = do_QueryInterface(nsJSUtils::GetDynamicScriptGlobal(cx));
>
> nsCOMPtr<nsIScriptObjectPrincipal> scriptObj = do_QueryInterface(window);
> NS_ENSURE_TRUE(scriptObj, NS_ERROR_FAILURE);
> principal = scriptObj->GetPrincipal();

Do I need to enter a compartment before I call GetDynamicScriptGlobal?  I seem to recall that I don't.
Attachment #506254 - Attachment is obsolete: true
Attached patch Patch v4 (obsolete) — Splinter Review
Added null check, updated comment.
I have no idea for compartments.  Blake?
Attached patch is green on try.  I'll push once I hear about compartments.
jst says we don't need a compartment.  Let's do this.
Attachment #507310 - Flags: approval2.0?
Requesting a2.0.  This patch looks a little scarier than it actually is, since the non-XHR referrer code it's touching was just changed as a result of bug 593174.
This test is messing up mochitest, I think because it calls pushState on the main window.  I of all people should know better...
Attachment #507310 - Flags: approval2.0?
Attachment #507310 - Attachment is obsolete: true
Attached patch Patch v5 (obsolete) — Splinter Review
Updated test.
In v5, I removed test_bug622088_1.html, renamed test_bug6220088_2.html to test_bug622088.html, and added the following chunk to the test:

> +  // Now change the location of the inner frame.  This should be reflected in
> +  // the XHR's referrer.
> +  inner.history.pushState('', '', Math.random());
> +  referrer = inner.doXHR();
> +  is (referrer, inner.document.location, 'Expected inner frame location after pushstate');

The point is to avoid calling pushState on the main window and instead call it only on an inner window.

bz, can you review this test change?  It's all green on try now.
Attachment #508444 - Flags: review?(bzbarsky)
Hmm.  How was the _1 test doing pushState on the main window?
It did, outside a function,

> history.pushState('', '', Math.random());

So by "main window", I meant the top-level window for the test, not the top-level window which runs this test in an iframe.

When I ran the test stand-alone (outside Mochitest's iframe), the pushState worked fine, but going back caused Mochitest to reload and run the test again (onload fired and everything).  I'm not sure why this happened, but I presume this has to do with something strange Mochitest is doing, because I couldn't get it to happen on a regular page.
Hmm.  I don't think Mochitest is doing anything particularly weird.

But with your modification the test no longer test what it should be testing, does it?
We need to test that:

a) the referrer is updated as a result of push/replaceState, and
b) we get the dynamic script context.

Test 2 initially tested (b), and the hunk I quoted in comment 52 tests (a).

No?
Oh, because we actually got called from the onload handler in the inner.  OK, that makes sense.
Comment on attachment 508444 [details] [diff] [review]
Patch v5

r=me; thanks for explaining!
Attachment #508444 - Flags: review?(bzbarsky) → review+
Comment on attachment 508444 [details] [diff] [review]
Patch v5

Thanks for the review!  Requesting approval again; see comment 49 for rationale.
Attachment #508444 - Flags: approval2.0?
Comment on attachment 508444 [details] [diff] [review]
Patch v5

Actually, I think this is wrong. We shouldn't use the context of the caller, but rather use mScriptContext.

Since we resolve the URL passed to xhr.open against the window whose constructor was used to create the XHR, it makes sense to me to use that as the referrer too. In fact, the spec should specify this.
Attachment #508444 - Flags: review-
Please fight that out with Anne, then.  That's not what he said the spec says!
A quick, decisive fight would be preferable!
Attachment #508444 - Flags: approval2.0?
Jonas, should it be

  GetDynamicContextURI(mContext)

or

  GetStaticContextURI(mContext)

?
Oh, mScriptContext, not mContext.  So is it as simple as

> nsCOMPtr<nsIDocument> doc =
>   nsContentUtils::GetDocumentFromScriptContext(mScriptContext);
>
> nsCOMPtr<nsIURI> referrerURI;
> if (doc) {
>   referrerURI = doc->GetDocumentURI();
> }

?

this is how we get the URI to resolve the request against...
Attachment #508444 - Attachment is obsolete: true
Oh, I bet I need to do the same check I did in the navigation referrer code: Does the document's original URI match the principal's URI?  If so, use the document's current URI; otherwise, use the principal's URI.
Attachment #510144 - Flags: review?(jonas)
Attachment #510144 - Attachment is obsolete: true
Attached patch Patch v7Splinter Review
This mostly works, but has a small problem.  The patch includes a failing test which does the following:

Document X at location x contains an iframe with a data: URI.  X then calls a function inside the iframe which returns an XMLHttpRequest.  Then X calls replaceState on itself, changing its URI to y.

When we finally send the XHR, it will observe that its document's original URI doesn't match its principal's URI, since the principal is X's original URI.  So it then uses x as the referrer, when in fact the referrer should be y.
The problem above is basically the same as bug 631949.  Seeing as we already have this behavior for regular loads, maybe we can tolerate it for XHRs and revisit later.  Don't let perfect be the enemy of the good and all that.

Jonas, what do you think?
Attachment #510174 - Flags: review?(jonas)
Comment on attachment 510174 [details] [diff] [review]
Patch v7

Yeah, I think that's fine for now.

However would be great to have this refactored into a function on nsIDocument so as to make it reusable at all needed locations.
Attachment #510174 - Flags: review?(jonas) → review+
Comment on attachment 510174 [details] [diff] [review]
Patch v7

Requesting approval.  This newest patch is a nice, local change.
Attachment #510174 - Flags: approval2.0?
Attachment #510174 - Flags: approval2.0? → approval2.0+
By the way, don't try to check this patch in exactly as it is -- I need to remove the one failing test.

I should be able to push on Wednesday.
http://hg.mozilla.org/mozilla-central/rev/b3db908c1b87
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Thank you for your hard work.
And than you for reporting it!
So where is the email to public-webapps@w3.org? *sighs*
We should probably also document the behavior, whatever it is.  At this point, I've lost track of what got implemented here.
Keywords: dev-doc-needed
It would be helpful if someone can help figure out details of what the docs need to say, since if bz is confused, there's little hope for me.
(In reply to comment #75)
> So where is the email to public-webapps@w3.org? *sighs*

I'm sorry about this.  Jonas or I will write to the list soon.

(In reply to comment #77)
> It would be helpful if someone can help figure out details of what the docs
> need to say, since if bz is confused, there's little hope for me.

I think it should be sufficient to say that the referrer is the current URL of the document which created the XHR object.
Is it?  The code is using mScriptContext, which is not the document "that created the XHR object" (whatever that means), but the window that was the |this| when the constructor was invoked, no?
Anyone have further clarification here? Sounds like there's still some uncertainty about what exactly this fix accomplishes.
bz might correct me again, but AIUI:

The referrer is the current URL of the document associated with mScriptContext.  Per comment 79, that document belongs to the window that was |this| when |new XmlHttpRequest()| was called.

This may change in the future, depending on what Jonas and Anne work out for the XHR spec.  But it should remain the case that the referrer reflects changes to the URL caused by push/replaceState.
Depends on: 686032
You need to log in before you can comment on or make changes to this bug.