Implement wildcard behavior for Cross-Origin-Expose-Headers [1] while also checking for credentials mode. [1] https://fetch.spec.whatwg.org/#ref-for-concept-response-cors-exposed-header-name-list%E2%91%A2
Created attachment 393014 [details] Patch
Created attachment 393018 [details] Patch
Thoughts about the patch: - not too fond of PerformExposeAllHeadersCheck name. - was thinking of moving ResourceResponseBase::filter to FetchIdioms but then lazyInit can not be called. - was thinking of merging sanitizing and filter code paths but again it depends on lazyInit usage. - not sure what lazyInit would do (it differs per platform AFAIK)? The comment says it removes some headers fields but which?
Comment on attachment 393018 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393018&action=review > Source/WebCore/platform/network/ResourceResponseBase.cpp:451 > + if (type == SanitizationType::CrossOriginSafeWithoutCredentials && corsSafeHeaderSet.contains("*")) I do not think we need the type here. If the header name is '*' and credentials are true, the load will fail and we probably do not need to do any sanitisation. Hopefully, we can simplify the patch.
Created attachment 393169 [details] Patch
Comment on attachment 393018 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393018&action=review >> Source/WebCore/platform/network/ResourceResponseBase.cpp:451 >> + if (type == SanitizationType::CrossOriginSafeWithoutCredentials && corsSafeHeaderSet.contains("*")) > > I do not think we need the type here. > If the header name is '*' and credentials are true, the load will fail and we probably do not need to do any sanitisation. > Hopefully, we can simplify the patch. I made a logic error in one of the patches, but now that I fixed that I was a able to verify that indeed the sanitization part can be removed. Sadly that is what I felt was the clean part of the code! If you have suggestions to clean up the filter related code, let me know, I put some thoughts about it in one of the comments.
Comment on attachment 393169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393169&action=review > Source/WebCore/Modules/fetch/FetchResponse.cpp:337 > + m_response.m_filteredResponse = ResourceResponseBase::filter(resourceResponse, m_response.m_performExposeAllHeadersCheck); What about XHR here? Can we add a test? Probably DocumentThreadableLoader should do the filtering. > Source/WebCore/Modules/fetch/FetchResponse.h:168 > + ResourceResponse::PerformExposeAllHeadersCheck m_performExposeAllHeadersCheck { ResourceResponse::PerformExposeAllHeadersCheck::No }; This should probably be stored in BodyLoader instead. Ideally, we would pass FetchOptions::Credentials to match the spec but FetchOptions is not (yet) in WebCore/platform. > Source/WebCore/platform/network/ResourceResponseBase.cpp:184 > + return filteredResponse; Another strategy would be to remove Access-Control-Expose-Header header when we are doing AccessControlAllowOrigin check when its value is '*' and credentials mode is include. Not sure whether that would be simpler though. Your approach is probably better. > Source/WebCore/platform/network/ResourceResponseBase.h:197 > + static ResourceResponse filter(const ResourceResponse&, PerformExposeAllHeadersCheck = PerformExposeAllHeadersCheck::No); Let's not have a default value to be sure of what we are doing. Or change it to Yes as a default.
Created attachment 393232 [details] Patch
Created attachment 393235 [details] Patch
Created attachment 393272 [details] Patch
Created attachment 393345 [details] Patch
Comment on attachment 393169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393169&action=review >> Source/WebCore/Modules/fetch/FetchResponse.cpp:337 >> + m_response.m_filteredResponse = ResourceResponseBase::filter(resourceResponse, m_response.m_performExposeAllHeadersCheck); > > What about XHR here? Can we add a test? > Probably DocumentThreadableLoader should do the filtering. Sure, I added xhr/cors-expose-star.sub.any.html. Indeed DocumentThreadableLoader needed to do filtering. >> Source/WebCore/Modules/fetch/FetchResponse.h:168 >> + ResourceResponse::PerformExposeAllHeadersCheck m_performExposeAllHeadersCheck { ResourceResponse::PerformExposeAllHeadersCheck::No }; > > This should probably be stored in BodyLoader instead. > Ideally, we would pass FetchOptions::Credentials to match the spec but FetchOptions is not (yet) in WebCore/platform. I stored it in BodyLoader now. >> Source/WebCore/platform/network/ResourceResponseBase.cpp:184 >> + return filteredResponse; > > Another strategy would be to remove Access-Control-Expose-Header header when we are doing AccessControlAllowOrigin check when its value is '*' and credentials mode is include. > Not sure whether that would be simpler though. Your approach is probably better. I hope so :) >> Source/WebCore/platform/network/ResourceResponseBase.h:197 >> + static ResourceResponse filter(const ResourceResponse&, PerformExposeAllHeadersCheck = PerformExposeAllHeadersCheck::No); > > Let's not have a default value to be sure of what we are doing. > Or change it to Yes as a default. I removed the default value.
Comment on attachment 393345 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393345&action=review > Source/WebCore/Modules/fetch/FetchResponse.cpp:59 > + fetchResponse->m_filteredResponse = ResourceResponseBase::filter(fetchResponse->m_internalResponse, ResourceResponse::PerformExposeAllHeadersCheck::Yes); We might want some tests for this one as well. This might be testable using DOMCache or response clone. > Source/WebCore/Modules/fetch/FetchResponse.cpp:250 > if (!response->m_bodyLoader->start(context, request)) Let's have body loader computes this in start, this will remove performExposeAllHeadersCheck(). I would have body loader store the credentials mode and compute whether doing the check based on that in didReceiveResponse. > Source/WebCore/loader/DocumentThreadableLoader.cpp:396 > + m_client->didReceiveResponse(identifier, ResourceResponseBase::filter(response, m_options.credentials == FetchOptions::Credentials::Include ? ResourceResponse::PerformExposeAllHeadersCheck::No : ResourceResponse::PerformExposeAllHeadersCheck::Yes)); s/ResourceResponseBase/ResourceResponse/ > Source/WebCore/loader/DocumentThreadableLoader.cpp:469 > + m_client->didReceiveResponse(identifier, ResourceResponseBase::filter(response, m_options.credentials == FetchOptions::Credentials::Include ? ResourceResponse::PerformExposeAllHeadersCheck::No : ResourceResponse::PerformExposeAllHeadersCheck::Yes)); s/ResourceResponseBase/ResourceResponse/
Created attachment 393354 [details] Patch
Created attachment 393370 [details] Patch
Comment on attachment 393345 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393345&action=review >> Source/WebCore/Modules/fetch/FetchResponse.cpp:59 >> + fetchResponse->m_filteredResponse = ResourceResponseBase::filter(fetchResponse->m_internalResponse, ResourceResponse::PerformExposeAllHeadersCheck::Yes); > > We might want some tests for this one as well. > This might be testable using DOMCache or response clone. I am not even sure what the spec says here. But I'll try to work on it, response clone seems like a good candidate to use. >> Source/WebCore/Modules/fetch/FetchResponse.cpp:250 >> if (!response->m_bodyLoader->start(context, request)) > > Let's have body loader computes this in start, this will remove performExposeAllHeadersCheck(). > I would have body loader store the credentials mode and compute whether doing the check based on that in didReceiveResponse. Done, that is cleaner indeed. >> Source/WebCore/loader/DocumentThreadableLoader.cpp:396 >> + m_client->didReceiveResponse(identifier, ResourceResponseBase::filter(response, m_options.credentials == FetchOptions::Credentials::Include ? ResourceResponse::PerformExposeAllHeadersCheck::No : ResourceResponse::PerformExposeAllHeadersCheck::Yes)); > > s/ResourceResponseBase/ResourceResponse/ Done. Note that I did not come up with it :) >> Source/WebCore/loader/DocumentThreadableLoader.cpp:469 >> + m_client->didReceiveResponse(identifier, ResourceResponseBase::filter(response, m_options.credentials == FetchOptions::Credentials::Include ? ResourceResponse::PerformExposeAllHeadersCheck::No : ResourceResponse::PerformExposeAllHeadersCheck::Yes)); > > s/ResourceResponseBase/ResourceResponse/ Done.
Comment on attachment 393370 [details] Patch Clearing flags on attachment: 393370 Committed r258330: <https://trac.webkit.org/changeset/258330>
All reviewed patches have been landed. Closing bug.
<rdar://problem/60371727>
For the record, the test was committed to WPT (https://github.com/web-platform-tests/wpt/pull/22219).