Bug 209664 – Append Upgrade-Insecure-Requests header in CachedResourceLoader

• Component: New Bugs
• Status: RESOLVED FIXED

[Rob Buis]

Append Upgrade-Insecure-Requests header in CachedResourceLoader, following
the fetch spec [1, step 3].

[1] https://fetch.spec.whatwg.org/#concept-main-fetch

[Rob Buis]

Created attachment 394730 [details]
Patch

[Rob Buis]

Note that addHTTPUpgradeInsecureRequestsIfNeeded could also be a free function or go into FetchIdioms.h, I just feel it does not seem to be part of FrameLoader, for one thing it is static so it does not seem to have a strong connection.

[Alex Christensen]

Is there any observable behavior change with this change?  What motivated this change?

[Rob Buis]

(In reply to Alex Christensen from comment #3)
> Is there any observable behavior change with this change?  What motivated
> this change?

There should be no observable behavior change. The motivation is to stick closer to the Fetch specification and setup requests in a more central place, as well as using more Fetch data structures, for instance here checking Fetch mode rather than to insert calls in the code path for main resources.

[youenn fablet]

Comment on attachment 394730 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:819
> +    if (request.options().mode == FetchOptions::Mode::Navigate)

Spec says to check the destination is Document, might be best to do that since this is not 100% equivalent.
LinkLoader for instance uses FetchOptions::Mode::Navigate.

> Source/WebCore/platform/network/ResourceRequestBase.cpp:770
> +        // FIXME: Identify HSTS cases and avoid adding the header. <https://bugs.webkit.org/show_bug.cgi?id=157885>

If we have to check HSTS, I am not sure ResourceRequestBase is the right place to add this routine.
Maybe we can inline it in CachedResourceLoader for now.

[Rob Buis]

Created attachment 395057 [details]
Patch

[Darin Adler]

Comment on attachment 395057 [details]
Patch

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

I would have said review+ because all other changes seem obviously right, but there’s one I don’t understand

> Source/WebCore/loader/FrameLoader.cpp:1522
> -    addExtraFieldsToMainResourceRequest(r);
> +    addExtraFieldsToRequest(r, m_loadType, true);

Change log doesn’t explain this change. If addExtraFieldsToMainResourceRequest still exists, why is this one call site not using it?

[Rob Buis]

Comment on attachment 395057 [details]
Patch

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

>> Source/WebCore/loader/FrameLoader.cpp:1522
>> +    addExtraFieldsToRequest(r, m_loadType, true);
> 
> Change log doesn’t explain this change. If addExtraFieldsToMainResourceRequest still exists, why is this one call site not using it?

I would like to remove addExtraFieldsToMainResourceRequest and addExtraFieldsToSubresourceRequest if possible and this is a step in that direction. However I can remove this part of the patch if you feel it is not appropriate here.

[Darin Adler]

Comment on attachment 395057 [details]
Patch

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

>>> Source/WebCore/loader/FrameLoader.cpp:1522
>>> +    addExtraFieldsToRequest(r, m_loadType, true);
>> 
>> Change log doesn’t explain this change. If addExtraFieldsToMainResourceRequest still exists, why is this one call site not using it?
> 
> I would like to remove addExtraFieldsToMainResourceRequest and addExtraFieldsToSubresourceRequest if possible and this is a step in that direction. However I can remove this part of the patch if you feel it is not appropriate here.

I don’t mean to make any statement about whether it’s appropriate. I just don’t understand why it’s correct, nor does the change log say why.

I trust you that it’s a good change, but I’d like you to explain why.

[youenn fablet]

Comment on attachment 395057 [details]
Patch

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

>>>> Source/WebCore/loader/FrameLoader.cpp:1522
>>>> +    addExtraFieldsToRequest(r, m_loadType, true);
>>> 
>>> Change log doesn’t explain this change. If addExtraFieldsToMainResourceRequest still exists, why is this one call site not using it?
>> 
>> I would like to remove addExtraFieldsToMainResourceRequest and addExtraFieldsToSubresourceRequest if possible and this is a step in that direction. However I can remove this part of the patch if you feel it is not appropriate here.
> 
> I don’t mean to make any statement about whether it’s appropriate. I just don’t understand why it’s correct, nor does the change log say why.
> 
> I trust you that it’s a good change, but I’d like you to explain why.

Maybe it is better to keep it for now like this and have another patch that would remove addExtraFieldsToMainResourceRequest.

[Rob Buis]

Comment on attachment 395057 [details]
Patch

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

>>>>> Source/WebCore/loader/FrameLoader.cpp:1522
>>>>> +    addExtraFieldsToRequest(r, m_loadType, true);
>>>> 
>>>> Change log doesn’t explain this change. If addExtraFieldsToMainResourceRequest still exists, why is this one call site not using it?
>>> 
>>> I would like to remove addExtraFieldsToMainResourceRequest and addExtraFieldsToSubresourceRequest if possible and this is a step in that direction. However I can remove this part of the patch if you feel it is not appropriate here.
>> 
>> I don’t mean to make any statement about whether it’s appropriate. I just don’t understand why it’s correct, nor does the change log say why.
>> 
>> I trust you that it’s a good change, but I’d like you to explain why.
> 
> Maybe it is better to keep it for now like this and have another patch that would remove addExtraFieldsToMainResourceRequest.

Right, I can see now that it is not super obvious.
Basically addExtraFieldsToMainResourceRequest is only special because it does an extra thing besides calling addExtraFieldsToRequest correctly for main resources, it calls addHTTPUpgradeInsecureRequestsIfNeeded. With this change the addHTTPUpgradeInsecureRequestsIfNeeded is only being done in CachedResourceLoader, so there is not anything special anymore about addExtraFieldsToMainResourceRequest when being called from FrameLoader so I inlined it. The situation is different for the other call site, DocumentLoader, since it has no access to m_loadType, so I left it untouched for now.

[Darin Adler]

Comment on attachment 395057 [details]
Patch

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

>>>>>> Source/WebCore/loader/FrameLoader.cpp:1522
>>>>>> +    addExtraFieldsToRequest(r, m_loadType, true);
>>>>> 
>>>>> Change log doesn’t explain this change. If addExtraFieldsToMainResourceRequest still exists, why is this one call site not using it?
>>>> 
>>>> I would like to remove addExtraFieldsToMainResourceRequest and addExtraFieldsToSubresourceRequest if possible and this is a step in that direction. However I can remove this part of the patch if you feel it is not appropriate here.
>>> 
>>> I don’t mean to make any statement about whether it’s appropriate. I just don’t understand why it’s correct, nor does the change log say why.
>>> 
>>> I trust you that it’s a good change, but I’d like you to explain why.
>> 
>> Maybe it is better to keep it for now like this and have another patch that would remove addExtraFieldsToMainResourceRequest.
> 
> Right, I can see now that it is not super obvious.
> Basically addExtraFieldsToMainResourceRequest is only special because it does an extra thing besides calling addExtraFieldsToRequest correctly for main resources, it calls addHTTPUpgradeInsecureRequestsIfNeeded. With this change the addHTTPUpgradeInsecureRequestsIfNeeded is only being done in CachedResourceLoader, so there is not anything special anymore about addExtraFieldsToMainResourceRequest when being called from FrameLoader so I inlined it. The situation is different for the other call site, DocumentLoader, since it has no access to m_loadType, so I left it untouched for now.

So some mention of this in the change log would have helped me understand reviewing, and maybe helped others understand later, too.

Which is why I asked.

[Rob Buis]

Comment on attachment 395057 [details]
Patch

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

>>>>>>> Source/WebCore/loader/FrameLoader.cpp:1522
>>>>>>> +    addExtraFieldsToRequest(r, m_loadType, true);
>>>>>> 
>>>>>> Change log doesn’t explain this change. If addExtraFieldsToMainResourceRequest still exists, why is this one call site not using it?
>>>>> 
>>>>> I would like to remove addExtraFieldsToMainResourceRequest and addExtraFieldsToSubresourceRequest if possible and this is a step in that direction. However I can remove this part of the patch if you feel it is not appropriate here.
>>>> 
>>>> I don’t mean to make any statement about whether it’s appropriate. I just don’t understand why it’s correct, nor does the change log say why.
>>>> 
>>>> I trust you that it’s a good change, but I’d like you to explain why.
>>> 
>>> Maybe it is better to keep it for now like this and have another patch that would remove addExtraFieldsToMainResourceRequest.
>> 
>> Right, I can see now that it is not super obvious.
>> Basically addExtraFieldsToMainResourceRequest is only special because it does an extra thing besides calling addExtraFieldsToRequest correctly for main resources, it calls addHTTPUpgradeInsecureRequestsIfNeeded. With this change the addHTTPUpgradeInsecureRequestsIfNeeded is only being done in CachedResourceLoader, so there is not anything special anymore about addExtraFieldsToMainResourceRequest when being called from FrameLoader so I inlined it. The situation is different for the other call site, DocumentLoader, since it has no access to m_loadType, so I left it untouched for now.
> 
> So some mention of this in the change log would have helped me understand reviewing, and maybe helped others understand later, too.
> 
> Which is why I asked.

Thanks for the feedback, I do tend to be terse and hand-wavy, so I will try to improve here. I do like the detailed explanations given in Changelogs per file/method so maybe I'll try using that more.

[Rob Buis]

Created attachment 395067 [details]
Patch

[EWS]

Committed r259308: <https://trac.webkit.org/changeset/259308>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395067 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61121370>