Bug 93777

Created attachment 157907 [details] First pass.

This isn't a patch for landing, but I think it's worth putting up for review. Two issues I see: 1. After a lot of back and forth with GDB, 'MainResourceLoader::willSendRequest' is the only place I see that gets called both on form submission, and on a redirect after form submission. That's where this patch places the CSP checks. I'm not sure that's good enough, as I don't think it gives the ability to check a chain of redirects (and I'm also not sure that checking `httpBody` is enough to determine if we're in a form submission?). 2. It doesn't currently implement the response the spec mandates: an empty 400 response. I'm at a bit of a loss as to how I can generate such a thing. WDYT, Adam? CCing Jochen as well, as he knows strange things about loaders. Thanks!

Comment on attachment 157907 [details] First pass. I don't think the MainResourceLoader is the right place to do the check: - submissions to javascript URls never hit the MainResourceLoader - what about submissions using GET? - what about XHRs using POST? Can you add tests for all three cases? I would either tell the MainResourceLoader that it's processing a form submission, or have the main frame loader call back to the frame loader in willSendRequest so it can do the check ResourceLoader has a cancel method that takes a ResourceError. You can use this to create a custom error code.

GET submissions were a good suggestion, thanks Jochen. As it turns out, cancelling them is buggy: covered in issue 93850. I'll upload a patch in a moment that addresses your concerns (with the exception of XHR, as you noted in chat that they don't go through MainResourceLoader). It includes a fix for the above-noted issue, which I'll take out once it's landed on trunk. I've added a call to ::cancel with a ResourceError, as you suggested. It might or might not work. It's certainly the case that neither a 'load' nor 'error' event is generated on the form, which I believe Adam wanted in order to drop the `setTimeout` call to end the test. I'll dig into that a bit more, but ideas would be welcome. :) Thanks!

Created attachment 158010 [details] Patch

Comment on attachment 158010 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158010&action=review > Source/WebCore/loader/FrameLoader.h:224 > + // Content Security Policy related functions. I'd skip this comment. It doesn't really add anything. > Source/WebCore/page/ContentSecurityPolicy.cpp:716 > + if (type == "formAction") formAction -> form-action ?

This approach looks reasonable to me. Let's get Bug 93850 resolved and then we can finish this one up.

Comment on attachment 158010 [details] Patch Attachment 158010 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13487366 New failing tests: http/tests/security/contentSecurityPolicy/worker-connect-src-blocked.html http/tests/security/contentSecurityPolicy/1.1/form-action-src-blocked.html http/tests/security/contentSecurityPolicy/1.1/form-action-src-redirect-blocked.html http/tests/security/contentSecurityPolicy/connect-src-eventsource-blocked.html http/tests/security/contentSecurityPolicy/source-list-parsing-malformed-meta.html http/tests/security/contentSecurityPolicy/1.1/form-action-src-javascript-blocked.html http/tests/security/contentSecurityPolicy/connect-src-websocket-blocked.html http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-blocked.html http/tests/security/contentSecurityPolicy/1.1/form-action-src-get-blocked.html

Created attachment 158053 [details] Archive of layout-test-results from gce-cr-linux-05 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 158087 [details] Rebase, fix tests.

(In reply to comment #7) > This approach looks reasonable to me. Let's get Bug 93850 resolved and then we can finish this one up. Rebased on top of that fix, and I'm happy to fix any issues you see in the patch. :)

Created attachment 158337 [details] Rebase on top of plugin-types.

Comment on attachment 158337 [details] Rebase on top of plugin-types. Overall looks good. Let's wait what Adam thinks View in context: https://bugs.webkit.org/attachment.cgi?id=158337&action=review > Source/WebCore/loader/FrameLoader.cpp:308 > + // hit it. i'd argue that this is obvious from the code below :) > Source/WebCore/loader/FrameLoader.cpp:919 > +bool FrameLoader::checkIfFormActionAllowed(const KURL& url) const maybe we should call it "...ByCSP" as the calling side will generate an error event that claims it stems from CSPs? > Source/WebCore/loader/MainResourceLoader.cpp:208 > + cancel(ResourceError(newRequest.url().host(), 400, newRequest.url().string(), "CSP")); I think the error domain is not a hostname. I think the empty string would be the correct domain here > Source/WebCore/page/ContentSecurityPolicy.cpp:895 > + checkSource(m_formAction.get(), url); nit. why not put it all in one line? > LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-allowed.html:20 > + <form action='/navigation/resources/form-target.pl' id='theform' method='post'> nit. 4 spaces. not that it would matter

Comment on attachment 158337 [details] Rebase on top of plugin-types. View in context: https://bugs.webkit.org/attachment.cgi?id=158337&action=review >> Source/WebCore/loader/FrameLoader.cpp:919 >> +bool FrameLoader::checkIfFormActionAllowed(const KURL& url) const > > maybe we should call it "...ByCSP" as the calling side will generate an error event that claims it stems from CSPs? Sure. Easy enough. >> Source/WebCore/loader/MainResourceLoader.cpp:208 >> + cancel(ResourceError(newRequest.url().host(), 400, newRequest.url().string(), "CSP")); > > I think the error domain is not a hostname. I think the empty string would be the correct domain here The ResourceError/ResourceErrorBase classes weren't exactly overflowing with documentation. I was guessing at most of these values. ;) Can you shed some light on what they're actually used for? >> Source/WebCore/page/ContentSecurityPolicy.cpp:895 >> + checkSource(m_formAction.get(), url); > > nit. why not put it all in one line? Because that's unreadable. :) >> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-allowed.html:20 >> + <form action='/navigation/resources/form-target.pl' id='theform' method='post'> > > nit. 4 spaces. not that it would matter I'm not sure how I ended up with two spaces here and four above. Thanks, I'll fix it.

Comment on attachment 158337 [details] Rebase on top of plugin-types. View in context: https://bugs.webkit.org/attachment.cgi?id=158337&action=review LGTM. I'll let jochen do the final review once you've posted a patch that addresses his comments. >>> Source/WebCore/page/ContentSecurityPolicy.cpp:895 >>> + checkSource(m_formAction.get(), url); >> >> nit. why not put it all in one line? > > Because that's unreadable. :) I think it's fine to have this code span multiple lines.

Created attachment 158375 [details] Jochen's feedback.

Comment on attachment 158375 [details] Jochen's feedback. View in context: https://bugs.webkit.org/attachment.cgi?id=158375&action=review > Source/WebCore/loader/MainResourceLoader.cpp:208 > + cancel(ResourceError("", 400, newRequest.url().string(), "CSP")); I read through the source a bit, and I think it's wrong to create a ResourceError here. Usually, when we want a specific kind of error, we'd call the FrameLoaderClient to give us such an error. I also read the spec, and it doesn't say that we should create a 400, it says we should react as if we'd received a 400. I believe just calling cancel() would suffice in that case.

Created attachment 158385 [details] No ResourceError.

(In reply to comment #17) > (From update of attachment 158375 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158375&action=review > > > Source/WebCore/loader/MainResourceLoader.cpp:208 > > + cancel(ResourceError("", 400, newRequest.url().string(), "CSP")); > > I read through the source a bit, and I think it's wrong to create a ResourceError here. Usually, when we want a specific kind of error, we'd call the FrameLoaderClient to give us such an error. Alright. I'll ask you about this tomorrow, since the loader code is still a bit of a black box for me. Or something more complicated than a box. :)

Comment on attachment 158385 [details] No ResourceError. Rejecting attachment 158385 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ebkit-commit-queue/Source/WebKit/chromium/third_party/ots --revision 94 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 42>At revision 94. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/13518104

Comment on attachment 158385 [details] No ResourceError. View in context: https://bugs.webkit.org/attachment.cgi?id=158385&action=review > LayoutTests/http/tests/navigation/resources/redirection-response.php:7 > +if ($_GET['host']) { You should use "if (isset($_GET['host']))" here. Per WebKit style, you could also skip the curly brackets.

Created attachment 158764 [details] *sigh*

Created attachment 158765 [details] Beverloo + PHP = ♥

(In reply to comment #23) > Created an attachment (id=158765) [details] > Beverloo + PHP = ♥ Thanks, Peter! :)

Comment on attachment 158765 [details] Beverloo + PHP = ♥ Clearing flags on attachment: 158765 Committed r125772: <http://trac.webkit.org/changeset/125772>

All reviewed patches have been landed. Closing bug.