Test for referer information being stripped when the header is removed in willSendRequest
Created attachment 50302 [details] Patch
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 :(
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
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.
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.
(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.
> 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
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
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;"?
Comment on attachment 50313 [details] patch Please address style nits. :)
> 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.
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.
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 @@ > +2010-03-09 Jochen Eisinger <jochen@chromium.org> > + > + 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
Comment on attachment 50507 [details] patch Clearing flags on attachment: 50507 Committed r55893: <http://trac.webkit.org/changeset/55893>
All reviewed patches have been landed. Closing bug.