Bug 177629 - Add more infrastructure to apply custom header fields to same-origin requests
Summary: Add more infrastructure to apply custom header fields to same-origin requests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks: 178356
  Show dependency treegraph
 
Reported: 2017-09-28 16:23 PDT by Alex Christensen
Modified: 2017-10-16 14:10 PDT (History)
6 users (show)

See Also:


Attachments
Patch (30.64 KB, patch)
2017-09-28 16:44 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (31.58 KB, patch)
2017-10-02 11:37 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (31.51 KB, patch)
2017-10-02 11:47 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (31.56 KB, patch)
2017-10-06 14:45 PDT, Alex Christensen
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2017-09-28 16:23:51 PDT
Apply custom headers to same-origin requests
Comment 1 Alex Christensen 2017-09-28 16:44:12 PDT
Created attachment 322144 [details]
Patch
Comment 2 Daniel Bates 2017-09-28 18:43:07 PDT
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”.
Comment 3 Alex Christensen 2017-09-28 21:48:25 PDT
(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?
Comment 4 Daniel Bates 2017-09-28 22:35:53 PDT
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());

‘:’, ‘ ‘ => “: “
Comment 5 Alex Christensen 2017-09-29 13:40:11 PDT
(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.
Comment 6 Daniel Bates 2017-09-29 17:04:34 PDT
(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.
Comment 7 Alex Christensen 2017-10-02 11:37:37 PDT
Created attachment 322401 [details]
Patch
Comment 8 Alex Christensen 2017-10-02 11:47:05 PDT
Created attachment 322404 [details]
Patch
Comment 9 Alex Christensen 2017-10-02 12:12:56 PDT
rdar://problem/31074470
Comment 10 Daniel Bates 2017-10-03 17:06:20 PDT
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()) : { };
Comment 11 Alex Christensen 2017-10-06 14:45:18 PDT
Created attachment 323046 [details]
Patch
Comment 12 Ryosuke Niwa 2017-10-06 15:04:59 PDT
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.
Comment 13 Alex Christensen 2017-10-06 15:08:00 PDT
Committed without the ResourceRequestBase.cpp change.
http://trac.webkit.org/r223001
Comment 14 Radar WebKit Bug Importer 2017-10-06 16:12:06 PDT
<rdar://problem/34866836>
Comment 15 Ryosuke Niwa 2017-10-06 16:13:02 PDT
Let's use a separate Bugzilla bug to land the remaining pieces to make things clear.
Comment 16 Daniel Bates 2017-10-06 18:21:10 PDT
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.
Comment 17 Daniel Bates 2017-10-06 18:27:11 PDT
(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.