Created attachment 281251 [details] Patch

This patch is a first step towards full cors checks in loaders code. It is not fully complete as: - It does not check the final response for cross-origin checks. CSS style sheets fetched with crossOrigin attribute should currently protect themselves at HTMLLinkElement level or equivalent. - If a resource to be fetched with a mode is already in the cache but with a different mode, the isClean() computation may not be valid. CachedResourceLoader should be able to handle that properly. I plan to fix those issues, ideally as a follow-up patch with additional tests.

Comment on attachment 281251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281251&action=review I'm not sure I'm sold on the idea of CORS protecting css stylesheets. What does it prevent? Is it specified anywhere? I found https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS but that's not a spec. > Source/WebCore/ChangeLog:11 > + or to Opaque if fetch mode is no-cors. CORS isn't consistently capitalized > Source/WebCore/css/CSSStyleSheet.h:54 > + static Ref<CSSStyleSheet> create(Ref<StyleSheetContents>&&, Node* ownerNode, bool); This definitely needs a name indicating what the bool is for, or use an enum.

I found this spec, but I don't know if there are any competing ones: <http://www.w3.org/TR/cssom-1/>.

The latest spec for the stylesheet link element is https://html.spec.whatwg.org/multipage/semantics.html#link-type-stylesheet It also points to https://drafts.csswg.org/cssom/#create-a-css-style-sheet

So we do have competing specs?

Comment on attachment 281251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281251&action=review > LayoutTests/http/tests/security/cannot-read-cssrules-redirect.html:28 > log("cssRules: " + sheet1.cssRules); I'm not sure there is a consensus among browsers about what to do with CORS css stylesheets. This line throws an exception in Firefox but not chrome. Current Safari (without this patch) seems to have the same output on this test as Chrome. Firefox also complains about the lack of meta charset in this test.

(In reply to comment #6) > So we do have competing specs? https://drafts.csswg.org/cssom is essentially the working draft of https://www.w3.org/TR/cssom-1/ and https://html.spec.whatwg.org/multipage/semantics.html is essentially http://w3c.github.io/html/. > > LayoutTests/http/tests/security/cannot-read-cssrules-redirect.html:28 > > log("cssRules: " + sheet1.cssRules); > > I'm not sure there is a consensus among browsers about what to do with CORS > css stylesheets. Good point, have yo tried with dev or regular browsers? I'll check tomorrow with dev chrome/firefox if you haven't done it. > This line throws an exception in Firefox but not chrome. Is it the case that sheet1 is null? If so, it might be the former behavior (https://www.w3.org/TR/html5/document-metadata.html#styling). AFAII, it is now changed to having a sheet object with a throwing cssRules accessor if origin-clean flag is not set. > Current Safari (without this patch) seems to have the same output on this test as Chrome. Current WebKit behavior is to have a sheet object that may return cssRules if same-origin or null if cross-origin. This patch is changing the case of declared-as-cross-origin-and-passing-cors-check resources. It is not changing the case of cross-origin-but-not-passing-cors-check (which should go from null to exception throwing).

Created attachment 281358 [details] Beefing up tests and addressing other comments

(In reply to comment #9) > Created attachment 281358 [details] > Beefing up tests and addressing other comments This patch adds some tests. I run them against chrome dev and firefox nightly. For cross-origin stylesheet link tests (security/cannot-read-cssrules.html), firefox and chrome behave differently but not far one from the other. In case of a link element without crossorigin attribute, chrome is returning null to cssRules accessor. Firefox is throwing an exception. WebKit does the same as Chrome. With a link element with a crossorigin attribute and cors check is failing, chrome is returning null to cssRules accessor. Firefox is returning an empty CSSRuleList object. WebKit exposes the CSSRuleList with the patch but not without the patch (expected regression to be fixed in a follow-up path). With a link element with a crossorigin attribute and cors check is successful, both expose the object. WebKit exposes the CSSRuleList with the patch but not without the patch. In case of redirections, behaviors are similar between Chrome and Firefox. In case of a link element with a crossorigin attribute, cross-origin redirection to a same-origin resource but cors check is failing, both Chrome and firefox expose empty cssRuleList. WebKit with the patch will return null and will expose the cssRuleList without the patch. What is surprising is that the same case but without the crossorigin attribute will make chrome and firefox exposing cssRules. This is surprising as the resource should be considered cross-origin.

It might be tempting to change the behavior the case of elements without crossorigin attributes to something more consistent. But given the interop analysis and the potential compatibility risk (?), we might consider to only change the behavior for link elements with crossorigin attributes. The other cases would stay in the status quo. I can update the patch accordingly.

Created attachment 281836 [details] Patch

(In reply to comment #12) > Created attachment 281836 [details] > Patch Patch now only changes behavior for link elements with cross origin attributes. As can be seen from the test results, cors checks are done for redirects but not yet for the final response.

(In reply to comment #13) > (In reply to comment #12) > > Created attachment 281836 [details] > > Patch > > Patch now only changes behavior for link elements with cross origin > attributes. > As can be seen from the test results, cors checks are done for redirects but > not yet for the final response. WIP patch of bug 158565 adds the final response check and updates the expectation of the css test accordingly.

Created attachment 281908 [details] Refactoring of redirectReceived

Created attachment 285101 [details] Rebasing

Comment on attachment 285101 [details] Rebasing View in context: https://bugs.webkit.org/attachment.cgi?id=285101&action=review > Source/WebCore/css/CSSStyleSheet.h:56 > + enum class OriginCleanFlag { > + True, > + False, > + Undefined > + }; I’m not clear on why the third state is called "undefined". I think the third state may mean something more like "based on the owning node"? It seems ugly that we have so many different versions of "tri state value"; the most natural is Optional<bool>, <wtf/TriState.h> where the third state is "mixed" rather than undefined. Since the point here is to make things readable at the call site, it seems using an enum class is a good idea. This kind of enum should be defined on one line; it’s not easier to read vertical like this. Just takes up a lot of extra space. > Source/WebCore/css/CSSStyleSheet.h:59 > + static Ref<CSSStyleSheet> create(Ref<StyleSheetContents>&&, Node* ownerNode, OriginCleanFlag = OriginCleanFlag::Undefined); Since we have to touch every call site, can ownerNode be changed to a Node& instead of Node*? > Source/WebCore/dom/ProcessingInstruction.cpp:196 > auto cssSheet = CSSStyleSheet::create(StyleSheetContents::create(href, parserContext), this); > - cssSheet.get().setDisabled(m_alternate); > - cssSheet.get().setTitle(m_title); > - cssSheet.get().setMediaQueries(MediaQuerySet::create(m_media)); > + cssSheet->setDisabled(m_alternate); > + cssSheet->setTitle(m_title); > + cssSheet->setMediaQueries(MediaQuerySet::create(m_media)); Why are we touching this at all? I don’t think the change here is an improvement. Although the "->" syntax is fewer characters, in the past we have talked about possibly preferring the ".get()." syntax. Talk to Andreas Kling if you want to know more. But more importantly, this patch does not require any change here. Although I can see you wanted to make this consistent with other call sites. > Source/WebCore/html/HTMLLinkElement.cpp:337 > + // FIXME: originClean should be turned to false except set to true if fetch mode is CORS. There is an extra space here before "set". > Source/WebCore/html/HTMLLinkElement.cpp:338 > + CSSStyleSheet::OriginCleanFlag originClean = CSSStyleSheet::OriginCleanFlag::Undefined; I suggest using auto here. > Source/WebCore/html/HTMLLinkElement.cpp:340 > + originClean = cachedStyleSheet.isClean() ? CSSStyleSheet::OriginCleanFlag::True : CSSStyleSheet::OriginCleanFlag::False; If we were using Optional<bool> this line would not need a ? : expression in it! > Source/WebCore/loader/SubresourceLoader.cpp:419 > + if (m_frame && m_frame->document()) { > + String errorMessage = "Cross-origin request to " + response.url().string() + " denied by Cross-Origin Resource Sharing policy: " + errorDescription; > + m_frame->document()->addConsoleMessage(MessageSource::Security, MessageLevel::Error, errorMessage); > + } > + cancel(ResourceError(errorDomainWebKitInternal, 0, response.url(), errorDescription, ResourceError::Type::AccessControl)); > + return false; I have the same complaint here that I had in another patch today; I am not so happy with the "checkXXX" function having the side effect of doing a cancel. I think we should consider finding some cleaner separation of responsibilities here to make the code more readable. This function should supply the decision that an error occurred and provide both the console message and the error, but it would be nice for the actual side effects to be more visible to the caller.

Created attachment 287949 [details] Patch for landing

Comment on attachment 287949 [details] Patch for landing Rejecting attachment 287949 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: a/EWS/WebKit/Source/WebCore/bindings/js/ReadableStreamDefaultController.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/ReadableStreamDefaultController.o ** BUILD FAILED ** The following build commands failed: CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/ProcessingInstruction.o dom/ProcessingInstruction.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler (1 failure) Full output: http://webkit-queues.webkit.org/results/2012070

Created attachment 287953 [details] Patch for landing

Comment on attachment 287953 [details] Patch for landing Clearing flags on attachment: 287953 Committed r205455: <http://trac.webkit.org/changeset/205455>

All reviewed patches have been landed. Closing bug.

> > Source/WebCore/css/CSSStyleSheet.h:56 > > + enum class OriginCleanFlag { > > + True, > > + False, > > + Undefined > > + }; > > I’m not clear on why the third state is called "undefined". I think the > third state may mean something more like "based on the owning node"? I would call it Legacy. > It seems ugly that we have so many different versions of "tri state value"; > the most natural is Optional<bool>, <wtf/TriState.h> where the third state > is "mixed" rather than undefined. Since the point here is to make things > readable at the call site, it seems using an enum class is a good idea. That was the idea, but it is only really useful at one calling place. I moved to Optional<bool> which should migrate at some point to bool. > This kind of enum should be defined on one line; it’s not easier to read > vertical like this. Just takes up a lot of extra space. OK > > Source/WebCore/css/CSSStyleSheet.h:59 > > + static Ref<CSSStyleSheet> create(Ref<StyleSheetContents>&&, Node* ownerNode, OriginCleanFlag = OriginCleanFlag::Undefined); > > Since we have to touch every call site, can ownerNode be changed to a Node& > instead of Node*? Done > > Source/WebCore/dom/ProcessingInstruction.cpp:196 > > auto cssSheet = CSSStyleSheet::create(StyleSheetContents::create(href, parserContext), this); > > - cssSheet.get().setDisabled(m_alternate); > > - cssSheet.get().setTitle(m_title); > > - cssSheet.get().setMediaQueries(MediaQuerySet::create(m_media)); > > + cssSheet->setDisabled(m_alternate); > > + cssSheet->setTitle(m_title); > > + cssSheet->setMediaQueries(MediaQuerySet::create(m_media)); > > Why are we touching this at all? I don’t think the change here is an > improvement. Although the "->" syntax is fewer characters, in the past we > have talked about possibly preferring the ".get()." syntax. Talk to Andreas > Kling if you want to know more. > > But more importantly, this patch does not require any change here. Although > I can see you wanted to make this consistent with other call sites. That was a left-over of another patch. I removed this from the landed patch. > > Source/WebCore/html/HTMLLinkElement.cpp:337 > > + // FIXME: originClean should be turned to false except set to true if fetch mode is CORS. > > There is an extra space here before "set". OK > > Source/WebCore/html/HTMLLinkElement.cpp:338 > > + CSSStyleSheet::OriginCleanFlag originClean = CSSStyleSheet::OriginCleanFlag::Undefined; > > I suggest using auto here. > > > Source/WebCore/html/HTMLLinkElement.cpp:340 > > + originClean = cachedStyleSheet.isClean() ? CSSStyleSheet::OriginCleanFlag::True : CSSStyleSheet::OriginCleanFlag::False; > > If we were using Optional<bool> this line would not need a ? : expression in > it! Done > > Source/WebCore/loader/SubresourceLoader.cpp:419 > > + if (m_frame && m_frame->document()) { > > + String errorMessage = "Cross-origin request to " + response.url().string() + " denied by Cross-Origin Resource Sharing policy: " + errorDescription; > > + m_frame->document()->addConsoleMessage(MessageSource::Security, MessageLevel::Error, errorMessage); > > + } > > + cancel(ResourceError(errorDomainWebKitInternal, 0, response.url(), errorDescription, ResourceError::Type::AccessControl)); > > + return false; > > I have the same complaint here that I had in another patch today; I am not > so happy with the "checkXXX" function having the side effect of doing a > cancel. I think we should consider finding some cleaner separation of > responsibilities here to make the code more readable. This function should > supply the decision that an error occurred and provide both the console > message and the error, but it would be nice for the actual side effects to > be more visible to the caller. Yes, this part was removed from this patch as a previous one landed the needed functionality.