Bug 177629 – Add more infrastructure to apply custom header fields to same-origin requests

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Apply custom headers to same-origin requests

[Alex Christensen]

Created attachment 322144 [details]
Patch

[Daniel Bates]

Comment on attachment 322144 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322144&action=review

> Source/WebCore/loader/HTTPHeaderField.h:38
> +    bool isValid() { return !m_name.isNull() && !m_value.isNull(); }

Can you give an example where create() would return an invalid HTTPHeaderField?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:754
> +    // Add custom headers to same-origin requests only.

This comment saids what the code does. Either the comment should explain why or we should remove this comment.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:756
> +        if (type == CachedResource::Type::MainResource || (document() && document()->topOrigin().equal(SecurityOrigin::create(request.resourceRequest().url()).ptr()))) {

How did you come to the decision to look at the top origin as opposed to the document’s origin?

> Source/WebKit/UIProcess/API/C/WKWebsitePolicies.cpp:76
> +        if (field && field->name().startsWithIgnoringASCIICase("X-")) // Let's just pretend RFC6648 never happened.

This comment is not useful. Please explain why we should “pretend”.

[Alex Christensen]

(In reply to Daniel Bates from comment #2)
> Comment on attachment 322144 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322144&action=review
> 
> > Source/WebCore/loader/HTTPHeaderField.h:38
> > +    bool isValid() { return !m_name.isNull() && !m_value.isNull(); }
> 
> Can you give an example where create() would return an invalid
> HTTPHeaderField?

There are many such examples in HTTPHeaderField.cpp in TestWebKitAPI.

> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:754
> > +    // Add custom headers to same-origin requests only.
> 
> This comment saids what the code does. Either the comment should explain why
> or we should remove this comment.

I'll remove the comment.

> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:756
> > +        if (type == CachedResource::Type::MainResource || (document() && document()->topOrigin().equal(SecurityOrigin::create(request.resourceRequest().url()).ptr()))) {
> 
> How did you come to the decision to look at the top origin as opposed to the
> document’s origin?

The custom headers are passed to the main document's DocumentLoader through the WebsitePolicies during decidePolicyForNavigationAction's response.  They are to be applied only to that origin, which is the top document's security origin.

> > Source/WebKit/UIProcess/API/C/WKWebsitePolicies.cpp:76
> > +        if (field && field->name().startsWithIgnoringASCIICase("X-")) // Let's just pretend RFC6648 never happened.
> 
> This comment is not useful. Please explain why we should “pretend”.

RFC6648 says we shouldn't use the X- prefix for custom headers any more because some of them have become de-facto standards because of widespread use, but we need a way to make this only custom headers, so we check the X- anyways.
Do you think I should explain this more fully in the comment or remove it?

[Daniel Bates]

Comment on attachment 322144 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322144&action=review

>>> Source/WebCore/loader/HTTPHeaderField.h:38
>>> +    bool isValid() { return !m_name.isNull() && !m_value.isNull(); }
>> 
>> Can you give an example where create() would return an invalid HTTPHeaderField?
> 
> There are many such examples in HTTPHeaderField.cpp in TestWebKitAPI.

IsValid() is never used in this patch. Where is an example of create() returning a non-nullopt value whose isValid() is false?

>>> Source/WebKit/UIProcess/API/C/WKWebsitePolicies.cpp:76
>>> +        if (field && field->name().startsWithIgnoringASCIICase("X-")) // Let's just pretend RFC6648 never happened.
>> 
>> This comment is not useful. Please explain why we should “pretend”.
> 
> RFC6648 says we shouldn't use the X- prefix for custom headers any more because some of them have become de-facto standards because of widespread use, but we need a way to make this only custom headers, so we check the X- anyways.
> Do you think I should explain this more fully in the comment or remove it?

I am tired and going to bed. Will reply back to you tomorrow.

> Tools/TestWebKitAPI/Tests/WebCore/HTTPHeaderField.cpp:39
> +    return makeString(field->name(), ':', ' ', field->value());

‘:’, ‘ ‘ => “: “

[Alex Christensen]

(In reply to Daniel Bates from comment #4)
> IsValid() is never used in this patch. Where is an example of create()
> returning a non-nullopt value whose isValid() is false?
Oh, shoot.  You're right.  I had this before I had it return a std::nullopt, but I decided not to require every client to call isValid to help them realize parsing can fail.  I'll remove isValid.

[Daniel Bates]

(In reply to Alex Christensen from comment #3)
> > > Source/WebKit/UIProcess/API/C/WKWebsitePolicies.cpp:76
> > > +        if (field && field->name().startsWithIgnoringASCIICase("X-")) // Let's just pretend RFC6648 never happened.
> > 
> > This comment is not useful. Please explain why we should “pretend”.
> 
> RFC6648 says we shouldn't use the X- prefix for custom headers any more
> because some of them have become de-facto standards because of widespread
> use, but we need a way to make this only custom headers, so we check the X-
> anyways.
> Do you think I should explain this more fully in the comment or remove it?

I do not think a comment is necessary. The code reflects our policy at the time it was written: a custom header is defined to a be a header whose name begins with "X-". If you feel that this definition should never change in the future and/or you feel that a person may question why we chose this definition given the existence of RFC6648 at the time the code was written and assume (wrongly) that author ignorance is the cause for the code being written the way it is then adding a comment that explained that the code was written intentionally against the recommendation of RFC6648 and why would be informative. For example, "Although RFC6648 recommends custom headers not be prefixed with X-, we require this prefix because of A, B, C.....". Alternatively you could document this reasoning in the ChangeLog and omit the comment.

[Alex Christensen]

Created attachment 322401 [details]
Patch

[Alex Christensen]

Created attachment 322404 [details]
Patch

[Alex Christensen]

rdar://problem/31074470

[Daniel Bates]

Comment on attachment 322404 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322404&action=review

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=177629

Please add the radar URL below this line before landing.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:760
> +    if (m_documentLoader && !m_documentLoader->customHeaderFields().isEmpty() && m_documentLoader->frame() && m_documentLoader->frame()->isMainFrame()) {
> +        bool loadingSameOriginSubresource = document() && document()->securityOrigin().equal(SecurityOrigin::create(request.resourceRequest().url()).ptr());
> +        if (type == CachedResource::Type::MainResource || loadingSameOriginSubresource) {
> +            for (auto& field : m_documentLoader->customHeaderFields())
> +                request.resourceRequest().addHTTPHeaderField(field.name(), field.value());
> +        }
> +    }

Is it intentional that we do not apply the custom header fields to subframes and child windows?

> Tools/TestWebKitAPI/Tests/WebCore/HTTPHeaderField.cpp:39
> +    if (!field)
> +        return { };
> +    return makeString(field->name(), ": ", field->value());

This is OK as-is. I would write this in one line using the ternary operator:

return field ? makeString(field->name(), ": ", field->value()) : { };

[Alex Christensen]

Created attachment 323046 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 323046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323046&action=review

> Source/WebCore/platform/network/ResourceRequestBase.cpp:152
> -    if (url().protocolIsInHTTPFamily())
> -        m_platformRequestUpdated = false;
> +    m_platformRequestUpdated = false;

We should probably do this work when we make the test working.

[Alex Christensen]

Committed without the ResourceRequestBase.cpp change.
http://trac.webkit.org/r223001

[Radar WebKit Bug Importer]

<rdar://problem/34866836>

[Ryosuke Niwa]

Let's use a separate Bugzilla bug to land the remaining pieces to make things clear.

[Daniel Bates]

Comment on attachment 323046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323046&action=review

I am disappointed to see that this patch was reviewed and landed. This patch reverts the purpose of the change made in bug #175524 and returns us (the WebKit OpenSource community) to having a CachedResourceRequest that exposes its underlying resource request. This is error prone. On another note, the ChangeLog is out-of-date, does not reflect the actual changes in this patch, and does not provide any explanatory text to help clarify the purpose of this feature or what part this patch represents in the journey to add this feature.

> Source/WebCore/ChangeLog:27
> +        (WebCore::CachedResourceLoader::requestResource):
> +        
> +        If the DocumentLoader has custom header fields from the WebsitePolicies, apply them to any same-origin requests.
> +        

Where is this code?

> Source/WebCore/loader/cache/CachedResourceRequest.h:55
> +    ResourceRequest& resourceRequest() { return m_resourceRequest; }

This reverts the change made in r220632 (bug #175524) and makes this class error prone.

[Daniel Bates]

(In reply to Daniel Bates from comment #16)
> This patch reverts the purpose of the change made in bug #175524 and returns us (the
> WebKit OpenSource community) to having a CachedResourceRequest that exposes
> its underlying resource request. This is error prone.

Disregard this remark.

> > Source/WebCore/loader/cache/CachedResourceRequest.h:55
> > +    ResourceRequest& resourceRequest() { return m_resourceRequest; }
> 
> This reverts the change made in r220632 (bug #175524) and makes this class
> error prone.

Disregard this remark.