[GTK] Empty referer header after WebKit session state restoration trips Bad Behavior Wordpress plugin

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Michael Catanzaro]

Reproducible in Epiphany on many Wordpress blogs:

 * Visit affected blog, e.g. https://blogs.gnome.org/mcatanzaro/
 * Close browser (triggering session state save)
 * Reopen browser (triggering session restoration)

It causes Wordpress to send an HTTP 400 error message, instead of properly reloading the page. Turns out we're tripping the Bad Behavior Wordpress plugin because we send an empty Referer header; Bad Behavior blocks the page load if the header is present but empty. I'm not sure why we are sending this empty header, but we should probably be saving the Referer in the session state. Alternatively we could just not send the Referer header, but I bet that would break other sites.

This is probably not reproducible in Safari because Cocoa ports do not reload pages after session restoration.

[Chris Dumez]

Indeed, I don't see any issue in Safari with this URL.

[Michael Catanzaro]

(In reply to comment #0)
> I'm not sure why we are sending
> this empty header, but we should probably be saving the Referer in the
> session state. Alternatively we could just not send the Referer header, but
> I bet that would break other sites.

It is actually saved in the session state, just not sent.

[Michael Catanzaro]

Similar issue on vox.com. When http://www.vox.com/ is restored from session state, we get an almost-blank page that just says "Forbidden". It may or may not be the same issue.

[Michael Catanzaro]

(In reply to comment #2) 
> It is actually saved in the session state, just not sent.

It actually is sent if there was a referrer originally; the problem occurs only on pages that didn't originally have a referrer before being saved to session state. In that case, we need to not add an empty referer header.

[Michael Catanzaro]

I can fix this by modifying the GTK+ session state decoding.

[Michael Catanzaro]

Created attachment 288494 [details]
Patch

[Michael Catanzaro]

(Probably ought to write a test for it.)

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

Created attachment 288510 [details]
Patch

[Michael Catanzaro]

So the first patch here was to WebKitWebViewSessionState.cpp, but I think I prefer the patch to ResourceRequestBase.cpp, which is less fragile.

[Carlos Garcia Campos]

Comment on attachment 288510 [details]
Patch

If we do this, we should rename the bug as it's not GTK+ specific. You should also check all the callers, because there are several already checking isEmpty(), and others checking isNull(). CachedResource clears the header in case of the referrer is empty.

[Michael Catanzaro]

Created attachment 288575 [details]
Patch

[Michael Catanzaro]

Here's a more comprehensive patch. I'm not sure about it, though. I think the callers checking with isNull() really wanted to check isEmpty(); in that case, this patch is correct. But if any  were checking with the intention of not changing a preexisting referrer, which seems unlikely but is possible, then this patch would be wrong. It's probably fine if it passes layout tests.

[Build Bot]

Comment on attachment 288575 [details]
Patch

Attachment 288575 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2060194

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-referrer-worker.html

[Build Bot]

Created attachment 288586 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288575 [details]
Patch

Attachment 288575 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2060200

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-referrer-worker.html

[Build Bot]

Created attachment 288588 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Michael Catanzaro]

(In reply to comment #13)
> I think the callers checking with isNull() really wanted to check isEmpty()

Evidently not so for the CORS code! It's safer to not change any of the isNull() checks, and only remove the isEmpty() checks.

[Michael Catanzaro]

Created attachment 288664 [details]
Patch

[Build Bot]

Comment on attachment 288664 [details]
Patch

Attachment 288664 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2063443

New failing tests:
imported/w3c/web-platform-tests/fetch/api/basic/referrer.html
imported/w3c/web-platform-tests/fetch/api/basic/request-referrer.html

[Build Bot]

Created attachment 288668 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288664 [details]
Patch

Attachment 288664 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2063453

New failing tests:
imported/w3c/web-platform-tests/fetch/api/basic/referrer.html
imported/w3c/web-platform-tests/fetch/api/basic/request-referrer.html

[Build Bot]

Created attachment 288669 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288664 [details]
Patch

Attachment 288664 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2063449

New failing tests:
imported/w3c/web-platform-tests/fetch/api/basic/referrer.html
imported/w3c/web-platform-tests/fetch/api/basic/request-referrer.html

[Build Bot]

Created attachment 288670 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288664 [details]
Patch

Attachment 288664 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2063463

New failing tests:
imported/w3c/web-platform-tests/fetch/api/basic/referrer.html
imported/w3c/web-platform-tests/fetch/api/basic/request-referrer.html

[Build Bot]

Created attachment 288672 [details]
Archive of layout-test-results from ews124 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[Michael Catanzaro]

(In reply to comment #11)
> Comment on attachment 288510 [details]
> Patch
> 
> If we do this, we should rename the bug as it's not GTK+ specific. You
> should also check all the callers, because there are several already
> checking isEmpty(), and others checking isNull(). CachedResource clears the
> header in case of the referrer is empty.

Turns out callers really expect different things here and I am lousy at predicting them. Let's make the minimal correct change, which is to set the referrer to null when an empty string is passed.

[Michael Catanzaro]

Created attachment 288691 [details]
Patch

[Alex Christensen]

Comment on attachment 288691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288691&action=review

> Source/WebCore/ChangeLog:11
> +        No new tests because it's hard to think of how to test it.

There ought to be a way to test this.  We have php scripts that echo all headers.

[Alexey Proskuryakov]

Comment on attachment 288691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288691&action=review

> Source/WebCore/ChangeLog:11
> +        No new tests because it's hard to think of how to test it.

There should be ways to trigger code that's so low in the stack, even if that's not for state restoration.

> Source/WebCore/platform/network/ResourceRequestBase.cpp:293
> +    // Many websites are intolerant of empty referrers, so never send one.

Do browsers actually never send one, not even when requested so in XMLHttpRequest? It's not obvious if this is the right level to make the fix.

[youenn fablet]

Comment on attachment 288691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288691&action=review

>> Source/WebCore/platform/network/ResourceRequestBase.cpp:293
>> +    // Many websites are intolerant of empty referrers, so never send one.
> 
> Do browsers actually never send one, not even when requested so in XMLHttpRequest? It's not obvious if this is the right level to make the fix.

Ideally, we would like to have something like ASSERT(!httpReferrer.isEmpty()).
There are cases where no referrer header is sent (no-referrer policy). A fetch call may lead to such request without referrer.

[Michael Catanzaro]

Comment on attachment 288494 [details]
Patch

Let's just go back to my original patch to WebKitWebViewSessionState. AP is right, modifying ResourceRequestBase is a bad idea; there's no good reason to prevent XHR from setting whatever value it wants for the header.

(In reply to comment #32)
> Ideally, we would like to have something like
> ASSERT(!httpReferrer.isEmpty()).
> There are cases where no referrer header is sent (no-referrer policy). A
> fetch call may lead to such request without referrer.

The assert would have to be (httpReferrer.isNull() || !httpReferrer.isEmpty()), because we of course want to allow sending requests with no referrer header. But anyway, not going to do that....

[Carlos Garcia Campos]

Comment on attachment 288494 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288494&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewSessionState.cpp:299
> +    // frameState.referrer must not be an empty string since we never want to
> +    // send an empty Referer header. Bug #159606.
> +    if (strlen(referrer))
> +        frameState.referrer = String::fromUTF8(referrer);

I think this is actually a workaround, and probably good enough for the stable branch. In my opinion the bug is not the in the decoder, but in the encoder, because we are not correctly handling optional strings (null strings). So, the optional fields that are strings should be encoded as "ms", instead of "s". We should review all other strings and change the encode/decode format, but we can do that in a separate bug, because that requires to add a new format version.

[WebKit Commit Bot]

Comment on attachment 288494 [details]
Patch

Clearing flags on attachment: 288494

Committed r206086: <http://trac.webkit.org/changeset/206086>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.