Bug 35920 – Test for referer information being stripped when the header is removed in willSendRequest

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Test for referer information being stripped when the header is removed in willSendRequest

Created attachment 50302 [details]
Patch

[WebKit Review Bot]

Attachment 50302 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/LayoutTestController.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 50303 [details]
https://bugs.webkit.org/show_bug.cgi?id=35920

Comment on attachment 50303 [details]
https://bugs.webkit.org/show_bug.cgi?id=35920

Please look esp. at the DumpRenderTree/win part - I failed to setup a build environment for windows, so I couldn't test :(

[WebKit Review Bot]

Attachment 50303 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/LayoutTestController.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 50305 [details]
patch

[Adam Barth]

Comment on attachment 50305 [details]
patch

I don't read objective-C, but this looks good to me.  I'm going to leave this here for a bit so someone who actually understands objective-C can look at it.

[Jeremy Orlow]

Comment on attachment 50303 [details]
https://bugs.webkit.org/show_bug.cgi?id=35920

> diff --git a/LayoutTests/http/tests/security/no-referer.html b/LayoutTests/http/tests/security/no-referer.html
> new file mode 100644
> index 0000000..31e1625
> --- /dev/null
> +++ b/LayoutTests/http/tests/security/no-referer.html
> @@ -0,0 +1,9 @@
> +<script>
> +if (window.layoutTestController) {
> +    layoutTestController.waitUntilDone();
> +    layoutTestController.dumpAsText();
> +    layoutTestController.setWillSendRequestClearHeader("Referer");
> +}
> +</script>
> +<div id=log></div>
> +<iframe src="http://127.0.0.1:8000/security/resources/no-referer-frame.php"></iframe>
> diff --git a/LayoutTests/http/tests/security/resources/no-referer-frame.php b/LayoutTests/http/tests/security/resources/no-referer-frame.php

Does this have to be done with an iframe?  Is it the normal way to do it?

> new file mode 100644
> index 0000000..f00b9bb
> --- /dev/null
> +++ b/LayoutTests/http/tests/security/resources/no-referer-frame.php
> @@ -0,0 +1,39 @@
> +<script>
> +function log(message)
> +{
> +    parent.document.getElementById("log").innerHTML += message + "<br>";
> +}
> +
> +if (document.referrer.toString() != "") {
> +  log("JavaScript: FAIL");
> +} else {
> +  log("JavaScript: PASS");
> +}

No {}'s



I don't know enough about DRT to r+ this, but it looks pretty solid to me.

[Jeremy Orlow]

(In reply to comment #8)
> (From update of attachment 50303 [details])
> > diff --git a/LayoutTests/http/tests/security/no-referer.html b/LayoutTests/http/tests/security/no-referer.html
> > new file mode 100644
> > index 0000000..31e1625
> > --- /dev/null
> > +++ b/LayoutTests/http/tests/security/no-referer.html
> > @@ -0,0 +1,9 @@
> > +<script>
> > +if (window.layoutTestController) {
> > +    layoutTestController.waitUntilDone();
> > +    layoutTestController.dumpAsText();
> > +    layoutTestController.setWillSendRequestClearHeader("Referer");
> > +}
> > +</script>
> > +<div id=log></div>
> > +<iframe src="http://127.0.0.1:8000/security/resources/no-referer-frame.php"></iframe>
> > diff --git a/LayoutTests/http/tests/security/resources/no-referer-frame.php b/LayoutTests/http/tests/security/resources/no-referer-frame.php
> 
> Does this have to be done with an iframe?  Is it the normal way to do it?
> 
> > new file mode 100644
> > index 0000000..f00b9bb
> > --- /dev/null
> > +++ b/LayoutTests/http/tests/security/resources/no-referer-frame.php
> > @@ -0,0 +1,39 @@
> > +<script>
> > +function log(message)
> > +{
> > +    parent.document.getElementById("log").innerHTML += message + "<br>";
> > +}
> > +
> > +if (document.referrer.toString() != "") {
> > +  log("JavaScript: FAIL");
> > +} else {
> > +  log("JavaScript: PASS");
> > +}
> 
> No {}'s
> 
> 
> 
> I don't know enough about DRT to r+ this, but it looks pretty solid to me.

(Or obj C)  So we still need an obj C reviewer to take a look.

[Adam Barth]

> Does this have to be done with an iframe?  Is it the normal way to do it?

Our loading code is sufficiently complicated that it makes sense to test as many of our load paths as we can.

In this case, he wants to check the document.referrer property, which needs a document (and hence a frame).

(In reply to comment #8)
> (From update of attachment 50303 [details])
> > diff --git a/LayoutTests/http/tests/security/no-referer.html b/LayoutTests/http/tests/security/no-referer.html
> > new file mode 100644
> > index 0000000..31e1625
> > --- /dev/null
> > +++ b/LayoutTests/http/tests/security/no-referer.html
> > @@ -0,0 +1,9 @@
> > +<script>
> > +if (window.layoutTestController) {
> > +    layoutTestController.waitUntilDone();
> > +    layoutTestController.dumpAsText();
> > +    layoutTestController.setWillSendRequestClearHeader("Referer");
> > +}
> > +</script>
> > +<div id=log></div>
> > +<iframe src="http://127.0.0.1:8000/security/resources/no-referer-frame.php"></iframe>
> > diff --git a/LayoutTests/http/tests/security/resources/no-referer-frame.php b/LayoutTests/http/tests/security/resources/no-referer-frame.php
> 
> Does this have to be done with an iframe?  Is it the normal way to do it?

It's mostly copied from http/tests/security/credentials-in-referer.html

> 
> > new file mode 100644
> > index 0000000..f00b9bb
> > --- /dev/null
> > +++ b/LayoutTests/http/tests/security/resources/no-referer-frame.php
> > @@ -0,0 +1,39 @@
> > +<script>
> > +function log(message)
> > +{
> > +    parent.document.getElementById("log").innerHTML += message + "<br>";
> > +}
> > +
> > +if (document.referrer.toString() != "") {
> > +  log("JavaScript: FAIL");
> > +} else {
> > +  log("JavaScript: PASS");
> > +}
> 
> No {}'s
> 
> 
> 
> I don't know enough about DRT to r+ this, but it looks pretty solid to me.

Created attachment 50308 [details]
patch

[Darin Adler]

Comment on attachment 50308 [details]
patch

Normally we use the misspelling "referer" only in the actual header name on the wire and use the normal English word "referrer" everywhere else.

>  #include <JavaScriptCore/JSObjectRef.h>
>  #include <JavaScriptCore/JSRetainPtr.h>
> -#include <wtf/RefCounted.h>
> +
> +#include <set>
>  #include <string>
>  #include <vector>
>  
> +#include <wtf/RefCounted.h>

Normally our includes are in a single paragraph sorted as if by the sort tool, not separate logical paragraphs.

> +    const std::set<std::string>& clearHeaders = gLayoutTestController->willSendRequestClearHeaders();

Normally we do "using namespace std" in our implementation files rather than using the "std::" prefix that is required in headers. Unless there's a name conflict of some sort.

No downside in adding a test, unless it has intermittent failures, so we should land this. r=me

Created attachment 50313 [details]
patch

[WebKit Review Bot]

Attachment 50313 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/win/ResourceLoadDelegate.cpp:41:  Use 'using namespace std;' instead of 'using std::set;'.  [build/using_std] [4]
WebKitTools/DumpRenderTree/win/ResourceLoadDelegate.cpp:42:  Use 'using namespace std;' instead of 'using std::string;'.  [build/using_std] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

I also added a missing ; in ResourceLoaderDelegate.mm:

[nsHeader release]; <-

(In reply to comment #15)
> Attachment 50313 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebKitTools/DumpRenderTree/win/ResourceLoadDelegate.cpp:41:  Use 'using
> namespace std;' instead of 'using std::set;'.  [build/using_std] [4]
> WebKitTools/DumpRenderTree/win/ResourceLoadDelegate.cpp:42:  Use 'using
> namespace std;' instead of 'using std::string;'.  [build/using_std] [4]
> Total errors found: 2 in 10 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

the original file already has use std::wstring; 

I assume there's a reason it doesn't use "using namespace std;"?

[Adam Barth]

Comment on attachment 50313 [details]
patch

Please address style nits.  :)

[Adam Barth]

> the original file already has use std::wstring; 
> 
> I assume there's a reason it doesn't use "using namespace std;"?

I suspect the original file is wrong.  We haven't had the style bot for that long.

[Alexey Proskuryakov]

Shouldn't the test be added to Skipped list for platforms that don't implement the new call?

Created attachment 50500 [details]
patch

Created attachment 50501 [details]
patch

Created attachment 50503 [details]
patch

Hopefully I got the changelog right now..

updates:
- fixed the nits
- actually managed to compile the code on windows and verified it's working there.

[Jeremy Orlow]

Comment on attachment 50503 [details]
patch

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index 70369a7..bd4c368 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,17 @@
> [phone]  Jochen Eisinger  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test for referrer information being stripped when the header is removed in willSendRequest
> +        https://bugs.webkit.org/show_bug.cgi?id=35920
> +
> +        * http/tests/security/no-referrer-expected.txt: Added.
> +        * http/tests/security/no-referrer.html: Added.
> +        * http/tests/security/resources/no-referrer-frame.php: Added.
> +        * http/tests/security/resources/no-referrer.php: Added.
> +	* platform/gtk/Skipped:
> +	* platform/qt/Skipped:

Use spaces, not tabs.

r=me (+ Darin on ObjC bits)

Created attachment 50507 [details]
patch

[WebKit Commit Bot]

Comment on attachment 50507 [details]
patch

Clearing flags on attachment: 50507

Committed r55893: <http://trac.webkit.org/changeset/55893>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.