Bug 208800 – Implement wildcard behavior for Cross-Origin-Expose-Headers

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

[Rob Buis]

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

[Rob Buis]

Created attachment 393014 [details]
Patch

[Rob Buis]

Created attachment 393018 [details]
Patch

[Rob Buis]

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?

[youenn fablet]

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.

[Rob Buis]

Created attachment 393169 [details]
Patch

[Rob Buis]

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.

[youenn fablet]

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.

[Rob Buis]

Created attachment 393232 [details]
Patch

[Rob Buis]

Created attachment 393235 [details]
Patch

[Rob Buis]

Created attachment 393272 [details]
Patch

[Rob Buis]

Created attachment 393345 [details]
Patch

[Rob Buis]

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.

[youenn fablet]

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/

[Rob Buis]

Created attachment 393354 [details]
Patch

[Rob Buis]

Created attachment 393370 [details]
Patch

[Rob Buis]

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.

[WebKit Commit Bot]

Comment on attachment 393370 [details]
Patch

Clearing flags on attachment: 393370

Committed r258330: <https://trac.webkit.org/changeset/258330>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60371727>

[Rob Buis]

For the record, the test was committed to WPT (https://github.com/web-platform-tests/wpt/pull/22219).