Bug 192375 – HTTPS Upgrade: Figure out if/how to tell clients that the HTTPS upgrade happened

• Component: Page Loading
• Status: RESOLVED FIXED

[Vivek Seth]

Use simulated redirect to tell clients that HTTPS Upgrade happened.

[Vivek Seth]

<rdar://problem/45851159>

[Vivek Seth]

Created attachment 356527 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/46464668>

[Chris Dumez]

Comment on attachment 356527 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkLoad.cpp:118
> +    if (!redirectCompletionHandler)

Why is this needed?

> Source/WebKit/NetworkProcess/NetworkLoad.h:69
> +    void willPerformHTTPRedirection(WebCore::ResourceResponse&&, WebCore::ResourceRequest&&, RedirectCompletionHandler&&) final;

Why is this needed?

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:58
> +    : didUpgradeCallback()

Not needed

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:232
> +    ResourceRequest oldRequest = request;

We normally call this "originalRequest"

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:235
> +    if (didUpgradeCallback && request.requester() == ResourceRequest::Requester::Main) {

Seems like the "request.requester() == ResourceRequest::Requester::Main" check could be inside applyHTTPSUpgradeIfNeeded().

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:258
> +    , oldRequest, request, didUpgradeRequest

Why are you capturing the request here? Also this is wrong since request is WTFMove()'d out on the same line.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:272
> +#if ENABLE(HTTPS_UPGRADE)

I do not think this part needs to be HTTPS_UPGRADE specific. I think we should simulate a redirect for all upgrades so that the Safari URL bar is up-to-date.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:273
> +        if (didUpgradeCallback && didUpgradeRequest) {

didUpgradeRequest can be replaced by checking if originalRequest's URL and result->value()'s request are different.

We may want to rename didUpgradeCallback() to didUpdateRequestURL()

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:274
> +            didUpgradeCallback(oldRequest, request, [this, request = WTFMove(result.value().request), handler = WTFMove(handler)]() mutable {

Why is this using request and not result->request?

> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:88
> +    WTF::Function<void(WebCore::ResourceRequest, WebCore::ResourceRequest, WTF::Function<void(void)>)> didUpgradeCallback;

Why is this public? This should be a private data member.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:124
> +        m_networkLoadChecker->didUpgradeCallback = [this](ResourceRequest originalRequest, ResourceRequest currentRequest, WTF::Function<void(void)> callback) {

Needs to be a proper setter. Setting a member like this is bad practice. Also parameters should probably not be passed by value:
[this](const ResourceRequest& originalRequest, const ResourceRequest& currentRequest, WTF::Function<void(void)>&& callback) {

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:125
> +            ResourceResponse redirectResponse { };

{ } is not needed.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:128
> +            redirectResponse.setHTTPVersion("HTTP/1.1");

"HTTP/1.1"_s

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:129
> +            redirectResponse.setHTTPHeaderField(String("Location"), currentRequest.url().string());

HTTPHeaderName::Location

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:130
> +            redirectResponse.setHTTPHeaderField(String("Cache-Control"), String("no-store"));

HTTPHeaderName::CacheControl
String("no-store") -> "no-store"_s

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:135
> +            callback();

Seems wrong to call the callback before continueWillSendRequest() has been called. 

Network process sends WillSendRequest IPC to the WebProcess with the proposed request
WebProcess may change the request
WebProcess sends ContinueWillSendRequest IPC back to the network process with the final URL.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:364
> +    m_networkLoadChecker->didUpgradeCallback = nullptr;

Why is this needed?

[Vivek Seth]

Created attachment 356651 [details]
Patch

[youenn fablet]

Comment on attachment 356651 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:194
>  bool NetworkLoadChecker::applyHTTPSUpgradeIfNeeded(ResourceRequest& request)

Existing issue but applyHTTPSUpgradeIfNeeded is called before checking for CSP while https://fetch.spec.whatwg.org/#main-fetch is doing it after.
I would tend to fix this if we want to do that for iframes.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:197
> +        return false;

Should it be done for top level page only or iframes as well? Requester::Main applies to iframes as well.
If we only want for top level pages, I would tend to have a simpler mechanism and do that directly in NetworkResourceLoader.
That would remove the need to have this double callback approach.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:229
> +    // Only upgade if there is a way to notify parent.

s/upgade/upgrade

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:230
> +    if (m_didUpdateRequestURL) {

I would do this if check in applyHTTPSUpgradeIfNeeded.

The name m_didUpdateRequestURL seems like a boolean while it is a callback.
It might be good to change the name to m_didUpdateRequestURLCallback

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:250
> +    processContentExtensionRulesForLoad(WTFMove(request), [this, handler = WTFMove(handler), originalRequest](auto result) mutable {

originalRequest = WTFMove(originalRequest)

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:265
> +                this->continueCheckingRequest(WTFMove(request), WTFMove(handler));

Are we sure 'this' will be valid once this callback is called?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:123
> +        m_networkLoadChecker->setDidUpdateRequestURL([this](ResourceRequest& originalRequest, ResourceRequest& currentRequest, WTF::Function<void(void)>&& callback) {

We move below originalRequest and currentRequest while they are not r values.
This is dangerous, we should fix this.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.h:216
> +    WTF::Function<void(void)> m_didSendSimulatedRedirectCallback;

s/void(void)/void()/

[Vivek Seth]

Comment on attachment 356651 [details]
Patch

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

Fixed some of the easy comments. I will look at the harder ones now.

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:229
>> +    // Only upgade if there is a way to notify parent.
> 
> s/upgade/upgrade

fixed this

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:230
>> +    if (m_didUpdateRequestURL) {
> 
> I would do this if check in applyHTTPSUpgradeIfNeeded.
> 
> The name m_didUpdateRequestURL seems like a boolean while it is a callback.
> It might be good to change the name to m_didUpdateRequestURLCallback

I agree, fixed this.

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:250
>> +    processContentExtensionRulesForLoad(WTFMove(request), [this, handler = WTFMove(handler), originalRequest](auto result) mutable {
> 
> originalRequest = WTFMove(originalRequest)

OK

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.h:216
>> +    WTF::Function<void(void)> m_didSendSimulatedRedirectCallback;
> 
> s/void(void)/void()/

Fixed this.

[Vivek Seth]

Created attachment 356764 [details]
Patch

[EWS Watchlist]

Attachment 356764 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/NetworkLoadChecker.h:65:  The parameter name "request" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Comment on attachment 356764 [details]
Patch

We are getting very close!
I am wondering whether we could have a way to write a test for it.

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

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:205
> +        m_networkLoadChecker->check(ResourceRequest { originalRequest() }, this, requestLoadType, [this] (auto&& result) {

requestLoadType should probably be set as m_networkLoadChecker creation time since it will stay valid for its whole lifetime.
That way, no need to change check and checkRedirection.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:220
> +                willSendRedirectedRequest(WTFMove(originalRequestCopy), WTFMove(currentRequest), WTFMove(redirectResponse));

To remove originalRequestCopy, you can write it:
willSendRedirectedRequest(ResourceRequest(originalRequestCopy), WTFMove(currentRequest), WTFMove(redirectResponse));

[Chris Dumez]

(In reply to youenn fablet from comment #10)
> Comment on attachment 356764 [details]
> Patch
> 
> We are getting very close!
> I am wondering whether we could have a way to write a test for it.

Yes, we've discussed this during one of our meetings and I believe we can write some tests. They will however require some test infrastructure and we are planning to add this later. Note that the code is not even enabled at build time at the moment, AFAIK.

> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356764&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:205
> > +        m_networkLoadChecker->check(ResourceRequest { originalRequest() }, this, requestLoadType, [this] (auto&& result) {
> 
> requestLoadType should probably be set as m_networkLoadChecker creation time
> since it will stay valid for its whole lifetime.
> That way, no need to change check and checkRedirection.
> 
> > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:220
> > +                willSendRedirectedRequest(WTFMove(originalRequestCopy), WTFMove(currentRequest), WTFMove(redirectResponse));
> 
> To remove originalRequestCopy, you can write it:
> willSendRedirectedRequest(ResourceRequest(originalRequestCopy),
> WTFMove(currentRequest), WTFMove(redirectResponse));

[Vivek Seth]

Created attachment 356838 [details]
Patch

[Alex Christensen]

Comment on attachment 356838 [details]
Patch

Why is this so different than the _schemeUpgraded code in NetworkSessionCocoa.mm?

[Chris Dumez]

Comment on attachment 356838 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:67
> +    , m_requestLoadType(LoadType::Other)

Not needed with inline initialization I suggest below.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:258
> +        if (result.value().request.url() != originalRequest.url()) {

Didn't we want to simulate redirects only for main-frame loads? Seems like we are now bypassing the checks in NetworkLoadChecker::continueCheckingRequest() for non-main frame loads (since those do not trigger a simulated redirect and a re-check).

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:267
> +    if (request.url() != originalRequest.url()) {

Can we try to avoid code duplication with above?

> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:88
> +    void setRequestLoadType(LoadType requestLoadType) { m_requestLoadType = requestLoadType; }

Should it be a constructor  parameter instead? Maybe with a default initialization value.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:145
> +    LoadType m_requestLoadType;

m_requestLoadType { LoadType::Other };

> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:148
> +    bool applyHTTPSUpgradeIfNeeded(WebCore::ResourceRequest&);

cannot this be "const"?

[Chris Dumez]

(In reply to Alex Christensen from comment #13)
> Comment on attachment 356838 [details]
> Patch
> 
> Why is this so different than the _schemeUpgraded code in
> NetworkSessionCocoa.mm?

Could you clarify what's different? We are similarly doing an upgrade and simulating a redirect. However, this happen before we have a NetworkLoad.

[Vivek Seth]

Comment on attachment 356838 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:67
>> +    , m_requestLoadType(LoadType::Other)
> 
> Not needed with inline initialization I suggest below.

OK

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:258
>> +        if (result.value().request.url() != originalRequest.url()) {
> 
> Didn't we want to simulate redirects only for main-frame loads? Seems like we are now bypassing the checks in NetworkLoadChecker::continueCheckingRequest() for non-main frame loads (since those do not trigger a simulated redirect and a re-check).

Fixed this

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:267
>> +    if (request.url() != originalRequest.url()) {
> 
> Can we try to avoid code duplication with above?

Fixed it

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:88
>> +    void setRequestLoadType(LoadType requestLoadType) { m_requestLoadType = requestLoadType; }
> 
> Should it be a constructor  parameter instead? Maybe with a default initialization value.

OK

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:145
>> +    LoadType m_requestLoadType;
> 
> m_requestLoadType { LoadType::Other };

OK

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:148
>> +    bool applyHTTPSUpgradeIfNeeded(WebCore::ResourceRequest&);
> 
> cannot this be "const"?

Yes it can. Fixed it.

[Vivek Seth]

Created attachment 356844 [details]
Patch

[Chris Dumez]

Comment on attachment 356844 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:226
> +    auto continueCheckingRequestOrAllowSimulatedRedirect = [this](ResourceRequest&& originalRequest, ResourceRequest&& currentRequest, ValidationHandler&& handler) {

continueCheckingRequestOrAllowSimulatedRedirect -> continueCheckingRequestOrDoSimulatedRedirect ?

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:228
> +        if ((m_requestLoadType != LoadType::MainFrame) && (currentRequest.url() != originalRequest.url())) {

Isn't this supposed to be m_requestLoadType == LoadType::MainFrame ?

Also, we prefer mot to avoid unnecessary parentheses.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:88
> +    void setRequestLoadType(LoadType requestLoadType) { m_requestLoadType = requestLoadType; }

As I mentioned before, I think it may look better without a setter and passing it as a parameter with a default value.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:148
> +    bool applyHTTPSUpgradeIfNeeded(WebCore::ResourceRequest&) const;

Methods should come *before* data members for clarity.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:212
> +            if (isMainFrameLoad() && (originalRequest().url() != currentRequest.url())) {

It is kind of sad that this check is duplicated from NetworkLoadChecker. I think it would look better it the check was done only in NetworkLoadChecker, and if we'd pass an extra "ResourceResponse redirect" to this lambda. We'd then have a if (!redirect.isNull()) check here instead.

[youenn fablet]

Comment on attachment 356844 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:226
> +    auto continueCheckingRequestOrAllowSimulatedRedirect = [this](ResourceRequest&& originalRequest, ResourceRequest&& currentRequest, ValidationHandler&& handler) {

How about making the lambda a method of NetworkLoadChecker.
This remove the need to capture this (as who knows if 'this' is valid).

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:238
> +        RELEASE_LOG_IF_ALLOWED("checkRequest - Upgrade URL from HTTP to HTTPS");

I would tend to make applyHTTPSUpgradeIfNeeded return void.
And put the RELEASE_LOG_IF_ALLOWED in applyHTTPSUpgradeIfNeeded.
Depending on applyHTTPSUpgradeIfNeeded implementation, the ASSERT might be useful or not.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:126
> +            m_networkLoadChecker->setRequestLoadType(NetworkLoadChecker::LoadType::MainFrame);

I think we should set request load type in NetworkLoadChecker constructor since it stays the same for the lifetime of m_networkLoadChecker .
This will remove the need for setRequestLoadType.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:216
> +                m_isWaitingContinueWillSendRequestForCachedRedirect = true;

I think this is ok to keep the name m_isWaitingContinueWillSendRequestForCachedRedirect even though it is not exactly correct anymore.

[Vivek Seth]

Comment on attachment 356844 [details]
Patch

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

>>> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:226
>>> +    auto continueCheckingRequestOrAllowSimulatedRedirect = [this](ResourceRequest&& originalRequest, ResourceRequest&& currentRequest, ValidationHandler&& handler) {
>> 
>> continueCheckingRequestOrAllowSimulatedRedirect -> continueCheckingRequestOrDoSimulatedRedirect ?
> 
> How about making the lambda a method of NetworkLoadChecker.
> This remove the need to capture this (as who knows if 'this' is valid).

OK

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:228
>> +        if ((m_requestLoadType != LoadType::MainFrame) && (currentRequest.url() != originalRequest.url())) {
> 
> Isn't this supposed to be m_requestLoadType == LoadType::MainFrame ?
> 
> Also, we prefer mot to avoid unnecessary parentheses.

OK

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:238
>> +        RELEASE_LOG_IF_ALLOWED("checkRequest - Upgrade URL from HTTP to HTTPS");
> 
> I would tend to make applyHTTPSUpgradeIfNeeded return void.
> And put the RELEASE_LOG_IF_ALLOWED in applyHTTPSUpgradeIfNeeded.
> Depending on applyHTTPSUpgradeIfNeeded implementation, the ASSERT might be useful or not.

Yeah I suppose the assert doesn't add much value. I'll make the change you suggest.

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:88
>> +    void setRequestLoadType(LoadType requestLoadType) { m_requestLoadType = requestLoadType; }
> 
> As I mentioned before, I think it may look better without a setter and passing it as a parameter with a default value.

Not sure I understand. Lets talk about it on Monday.

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:148
>> +    bool applyHTTPSUpgradeIfNeeded(WebCore::ResourceRequest&) const;
> 
> Methods should come *before* data members for clarity.

ok

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:126
>> +            m_networkLoadChecker->setRequestLoadType(NetworkLoadChecker::LoadType::MainFrame);
> 
> I think we should set request load type in NetworkLoadChecker constructor since it stays the same for the lifetime of m_networkLoadChecker .
> This will remove the need for setRequestLoadType.

Lets talk about this on Monday.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:212
>> +            if (isMainFrameLoad() && (originalRequest().url() != currentRequest.url())) {
> 
> It is kind of sad that this check is duplicated from NetworkLoadChecker. I think it would look better it the check was done only in NetworkLoadChecker, and if we'd pass an extra "ResourceResponse redirect" to this lambda. We'd then have a if (!redirect.isNull()) check here instead.

What makes this tricky is that ValidationHandler is called in 20 separate places in NetworkLoadChecker and we probably don't won't to modify all of the call sites to add an extra param to the lambda for a special case. I'll experiment with a few ideas, but I might need to speak to you about this on Monday.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:216
>> +                m_isWaitingContinueWillSendRequestForCachedRedirect = true;
> 
> I think this is ok to keep the name m_isWaitingContinueWillSendRequestForCachedRedirect even though it is not exactly correct anymore.

OK sounds good.

[Vivek Seth]

Created attachment 356935 [details]
Patch

[Vivek Seth]

Created attachment 356937 [details]
Patch

[Vivek Seth]

Comment on attachment 356937 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:56
> +    using ValidationHandler = CompletionHandler<void(RequestOrError&&, std::optional<WebCore::ResourceResponse>&&)>;

I don't like how adding a param here requires all callers of `ValidationHandler` to pass in std::nullopt if they don't want to do a simulated redirect. Since only 1 caller needs to do a simulated redirect, this pattern feels weird. I'm not sure of a better solution right now, but please let me know if you have any suggestions  for improving this. 

Also, I'm not sure my code handles the case where http://A redirects to http://B and domain B can be upgraded to https://B. I might need to make a change to `NetworkResourceLoader::willSendRedirectedRequest` for this to work. If this is indeed a bug, maybe we can fix it in a follow on change after this lands.

[youenn fablet]

The idea is to have NetworkLoadChecker::check take a completion handler that takes a new parameter.
ValidationHandler would become a private declaration for NetworkLoadChecker internal use.
That will require to change ping load but that is ok (hopefully at some point, we will kill it anyway).

As I suggested when we discussed last week, I like std::optional<ResourceResponse>&&.
ResourceResponse&& is sort of ok as well.

As of whether you handle correctly or not the redirection case, I think it is sort of ok right now: the new resource request URL should be upgraded and that is what probably  count. Of course testing should ensure that.

It is true that the redirect response will not match the new resource request URL.
Maybe we should update the redirect response location header to match the new resource request URL, for extra safety.
But this is an existing issue with the current code. I would tackle this as a follow-up.

[Chris Dumez]

(In reply to youenn fablet from comment #24)
> The idea is to have NetworkLoadChecker::check take a completion handler that
> takes a new parameter.
> ValidationHandler would become a private declaration for NetworkLoadChecker
> internal use.
> That will require to change ping load but that is ok (hopefully at some
> point, we will kill it anyway).
> 
> As I suggested when we discussed last week, I like
> std::optional<ResourceResponse>&&.
> ResourceResponse&& is sort of ok as well.
> 
> As of whether you handle correctly or not the redirection case, I think it
> is sort of ok right now: the new resource request URL should be upgraded and
> that is what probably  count. Of course testing should ensure that.
> 
> It is true that the redirect response will not match the new resource
> request URL.
> Maybe we should update the redirect response location header to match the
> new resource request URL, for extra safety.
> But this is an existing issue with the current code. I would tackle this as
> a follow-up.

I suggested ResourceResponse&& since we can already check if it is null. Similarly, we would not use std::optional for a String.

[Vivek Seth]

Created attachment 356978 [details]
Patch

[youenn fablet]

Comment on attachment 356978 [details]
Patch

It further improves.
Some suggestions below.

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

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:144
> +        if (WTF::holds_alternative<ResourceError>(result)) {

Let's use switchOn here and in the other cases.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:154
> +        if (WTF::holds_alternative<RedirectionTriplet>(result)) {

I would put this check as second, we usually do the rare cases first.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:157
> +            handler(RedirectionTriplet { WTFMove(request), WTFMove(triplet.redirectRequest), WTFMove(redirectResponse) });

You should be able to write it as:
handler(WTFMove(triplet))

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:210
> +            if (WTF::holds_alternative<NetworkLoadChecker::RedirectionTriplet>(result)) {

Ditto for the ordering.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:212
> +                auto triplet = WTF::get<NetworkLoadChecker::RedirectionTriplet>(result);

auto& would be better if possible, or WTFMove(WTF::get...).

> Source/WebKit/NetworkProcess/PingLoad.cpp:70
> +        if (WTF::holds_alternative<NetworkLoadChecker::RedirectionTriplet>(result)) {

I would put it as second in the list.

> Source/WebKit/NetworkProcess/PingLoad.cpp:71
> +            NetworkLoadChecker::RedirectionTriplet triplet = WTF::get<NetworkLoadChecker::RedirectionTriplet>(result);

auto& here as well.

> Source/WebKit/NetworkProcess/PingLoad.cpp:73
>              return;

Las return is not needed.

[Alex Christensen]

(In reply to Chris Dumez from comment #15)
> (In reply to Alex Christensen from comment #13)
> > Comment on attachment 356838 [details]
> > Patch
> > 
> > Why is this so different than the _schemeUpgraded code in
> > NetworkSessionCocoa.mm?
> 
> Could you clarify what's different? We are similarly doing an upgrade and
> simulating a redirect. However, this happen before we have a NetworkLoad.

Does it need to happen before we have a NetworkLoad?  Why not make a NetworkLoad, simulate a redirect, then start the actual load?  That seems like the best design to me

[Alex Christensen]

Comment on attachment 356978 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:209
> +    if (m_requestLoadType != LoadType::MainFrame)
> +        return;

Why don't we want to upgrade sub resource requests?

> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:64
> +    void check(WebCore::ResourceRequest&&, WebCore::ContentSecurityPolicyClient*, ValidationHandler&&);

This is a bad name.

[Chris Dumez]

(In reply to Alex Christensen from comment #29)
> Comment on attachment 356978 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356978&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:209
> > +    if (m_requestLoadType != LoadType::MainFrame)
> > +        return;
> 
> Why don't we want to upgrade sub resource requests?

This has been discussed, we have no guarantee that this would work. Only main frame URLs were tested.

> 
> > Source/WebKit/NetworkProcess/NetworkLoadChecker.h:64
> > +    void check(WebCore::ResourceRequest&&, WebCore::ContentSecurityPolicyClient*, ValidationHandler&&);
> 
> This is a bad name.

[Chris Dumez]

(In reply to Alex Christensen from comment #28)
> (In reply to Chris Dumez from comment #15)
> > (In reply to Alex Christensen from comment #13)
> > > Comment on attachment 356838 [details]
> > > Patch
> > > 
> > > Why is this so different than the _schemeUpgraded code in
> > > NetworkSessionCocoa.mm?
> > 
> > Could you clarify what's different? We are similarly doing an upgrade and
> > simulating a redirect. However, this happen before we have a NetworkLoad.
> 
> Does it need to happen before we have a NetworkLoad?  Why not make a
> NetworkLoad, simulate a redirect, then start the actual load?  That seems
> like the best design to me

We want to update as early as possible so that we can hit the disk cache.

[Vivek Seth]

Comment on attachment 356978 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:157
>> +            handler(RedirectionTriplet { WTFMove(request), WTFMove(triplet.redirectRequest), WTFMove(redirectResponse) });
> 
> You should be able to write it as:
> handler(WTFMove(triplet))

The reason I didn't do that is because `triplet.redirectResponse` != `redirectResponse`. As we discussed this morning we may need to modify `redirectResponse` if a simulated redirect needs to happen.

>>> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:209
>>> +        return;
>> 
>> Why don't we want to upgrade sub resource requests?
> 
> This has been discussed, we have no guarantee that this would work. Only main frame URLs were tested.

We only upgrade if we know that all sub-resource requests will also be HTTPS

[youenn fablet]

Note also that content blockers can potentially modify the URL.
For top level frames, we probably want to do this redirection.
This patch will fix that issue as well.

In terms of spec, we are trying to get closer to fetch.
The "upgrade" step is specified close to CSP checks for instance (https://fetch.spec.whatwg.org/#main-fetch) which this patch follows.

We cannot yet reunify NetworkLoadChecker and NetworkLoad because NetworkLoadChecker is used by NetworkResourceLoader and PingLoad, PingLoad using NetworkDataTask directly.

I would like to kill the current form of PingLoad and replace it with a 'keep alive' fetch.
At that point, we might be able to refactor NetworkLoadChecker/NetworkLoad relationship.

[Vivek Seth]

Comment on attachment 356978 [details]
Patch

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

>>> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:64
>>> +    void check(WebCore::ResourceRequest&&, WebCore::ContentSecurityPolicyClient*, ValidationHandler&&);
>> 
>> This is a bad name.
> 
> 

the check() method is not new. Its only showing up as changed because I re-ordered some lines.

[Vivek Seth]

Created attachment 357008 [details]
Patch

[Vivek Seth]

Created attachment 357009 [details]
Patch

[Chris Dumez]

Comment on attachment 357009 [details]
Patch

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

r=me with a couple of nits. We should also definitely fix GTK build.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:46
> +    enum class LoadType { MainFrame, Other };

enum class LoadType : bool { MainFrame, Other };

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:190
> +                    willSendRedirectedRequest(WTFMove(triplet.request), WTFMove(triplet.redirectRequest), WTFMove(triplet.redirectResponse));

You may need this-> here to to fix GTK build.

> Source/WebKit/NetworkProcess/PingLoad.cpp:65
> +                this->loadRequest(WTFMove(triplet.redirectRequest));

This think this should be an ASSERT_NOT_REACHED(). We should never be simulating redirects for Ping Loads.

[youenn fablet]

Comment on attachment 357009 [details]
Patch

LGTM with Chris comments

[Vivek Seth]

Created attachment 357066 [details]
Patch

[Alex Christensen]

Comment on attachment 357066 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:261
> +static ResourceResponse simulatedRedirectResponse(const URL& fromURL, const URL& toURL)

Is there a reason we are not removing synthesizeRedirectResponseIfNecessary in favor of this?

[Chris Dumez]

(In reply to Alex Christensen from comment #40)
> Comment on attachment 357066 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357066&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:261
> > +static ResourceResponse simulatedRedirectResponse(const URL& fromURL, const URL& toURL)
> 
> Is there a reason we are not removing synthesizeRedirectResponseIfNecessary
> in favor of this?

synthesizeRedirectResponseIfNecessary has this:
    if ([[[newRequest URL] scheme] isEqualToString:[[currentRequest URL] scheme]] && !schemeWasUpgradedDueToDynamicHSTS(newRequest))
        return nil;

So We'd have to move the check out to the caller but otherwise why not. Less duplication.

[Vivek Seth]

Created attachment 357078 [details]
Patch

[youenn fablet]

Comment on attachment 357078 [details]
Patch

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

> Source/WebCore/platform/network/ResourceResponseBase.cpp:128
> +    redirectResponse.setHTTPVersion("HTTP/1.1"_s);

Do we really need that one?

> Source/WebCore/platform/network/mac/WebCoreURLResponse.mm:353
>      return [[[NSHTTPURLResponse alloc] initWithURL:[currentRequest URL] statusCode:302 HTTPVersion:(NSString *)kCFHTTPVersion1_1 headerFields:synthesizedResponseHeaderFields] autorelease];

These two lines could be removed.

> Source/WebCore/platform/network/mac/WebCoreURLResponse.mm:356
> +    return synthesizedResponse.nsURLResponse();

Could be a one liner, or use auto instead of ResourceResponse.

[Vivek Seth]

Comment on attachment 357078 [details]
Patch

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

>> Source/WebCore/platform/network/ResourceResponseBase.cpp:128
>> +    redirectResponse.setHTTPVersion("HTTP/1.1"_s);
> 
> Do we really need that one?

I did a 1:1 match of other synthetic redirects that we were doing.

>> Source/WebCore/platform/network/mac/WebCoreURLResponse.mm:353
>>      return [[[NSHTTPURLResponse alloc] initWithURL:[currentRequest URL] statusCode:302 HTTPVersion:(NSString *)kCFHTTPVersion1_1 headerFields:synthesizedResponseHeaderFields] autorelease];
> 
> These two lines could be removed.

Good catch! I thought I had removed them.

>> Source/WebCore/platform/network/mac/WebCoreURLResponse.mm:356
>> +    return synthesizedResponse.nsURLResponse();
> 
> Could be a one liner, or use auto instead of ResourceResponse.

OK

[Vivek Seth]

Created attachment 357091 [details]
Patch

[EWS Watchlist]

Comment on attachment 357091 [details]
Patch

Attachment 357091 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10359723

New failing tests:
http/tests/security/contentSecurityPolicy/user-style-sheet-font-crasher.php

[EWS Watchlist]

Created attachment 357094 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Chris Dumez]

This patch is causing a crash:
System Integrity Protection: enabled

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       EXC_I386_GPFLT
Exception Note:        EXC_CORPSE_NOTIFY

Termination Signal:    Segmentation fault: 11
Termination Reason:    Namespace SIGNAL, Code 0xb
Terminating Process:   exc handler [0]

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.CFNetwork           	0x00007fff7b4d088b CFURLResponseGetURL + 25
1   com.apple.WebCore             	0x0000000108ae42e1 WebCore::ResourceResponse::platformLazyInit(WebCore::ResourceResponseBase::InitLevel) + 177 (ResourceResponseCocoa.mm:184)
2   com.apple.WebCore             	0x0000000109923dc3 WebCore::ResourceResponseBase::httpStatusCode() const + 19 (ResourceResponseBase.cpp:298)
3   com.apple.WebKit              	0x0000000107257049 WebKit::NetworkDataTaskCocoa::willPerformHTTPRedirection(WebCore::ResourceResponse&&, WebCore::ResourceRequest&&, WTF::CompletionHandler<void (WebCore::ResourceRequest&&)>&&) + 55 (NetworkDataTaskCocoa.mm:301)
4   com.apple.WebKit              	0x000000010726ff59 -[WKNetworkSessionDelegate URLSession:task:_schemeUpgraded:completionHandler:] + 239 (memory:2733)
5   com.apple.Foundation          	0x00007fff7ddbff19 __NSBLOCKOPERATION_IS_CALLING_OUT_TO_A_BLOCK__ + 7
6   com.apple.Foundation          	0x00007fff7ddbfbfc -[NSBlockOperation main] + 101
7   com.apple.Foundation          	0x00007fff7ddbe324 -[__NSOperationInternal _start:] + 672
8   com.apple.Foundation          	0x00007fff7ddba1db __NSOQSchedule_f + 201
9   libdispatch.dylib             	0x00007fff91f008fc _dispatch_client_callout + 8
10  libdispatch.dylib             	0x00007fff91f0daac _dispatch_main_queue_callback_4CF + 925
11  com.apple.CoreFoundation      	0x00007fff7c3acd69 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 9
12  com.apple.CoreFoundation      	0x00007fff7c36e04d __CFRunLoopRun + 2221
13  com.apple.CoreFoundation      	0x00007fff7c36d544 CFRunLoopRunSpecific + 420
14  com.apple.Foundation          	0x00007fff7dd9e252 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 277
15  com.apple.Foundation          	0x00007fff7dd9e12a -[NSRunLoop(NSRunLoop) run] + 76
16  libxpc.dylib                  	0x00007fff9218f89b _xpc_objc_main + 731
17  libxpc.dylib                  	0x00007fff9218e2e4 xpc_main + 494
18  com.apple.WebKit.Networking   	0x00000001071df68d WebKit::XPCServiceMain(int, char const**) + 490 (XPCServiceMain.mm:123)
19  com.apple.WebKit.Networking   	0x00000001071df81b main + 9 (XPCServiceMain.mm:46)
20  libdyld.dylib                 	0x00007fff91f36235 start + 1

[Chris Dumez]

Comment on attachment 357091 [details]
Patch

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

> Source/WebCore/platform/network/mac/WebCoreURLResponse.mm:350
> +    return ResourceResponse::syntheticRedirectResponse(URL([currentRequest URL]), URL([newRequest URL])).nsURLResponse();

You have a lifetime problem here, the NSURLResponse you are returning is owned by the ResourceResponse returned by syntheticRedirectResponse(), which is a temporary. This is likely the cause of the crash.

[Chris Dumez]

Comment on attachment 357091 [details]
Patch

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

>> Source/WebCore/platform/network/mac/WebCoreURLResponse.mm:350
>> +    return ResourceResponse::syntheticRedirectResponse(URL([currentRequest URL]), URL([newRequest URL])).nsURLResponse();
> 
> You have a lifetime problem here, the NSURLResponse you are returning is owned by the ResourceResponse returned by syntheticRedirectResponse(), which is a temporary. This is likely the cause of the crash.

I looks to me that synthesizeRedirectResponseIfNecessary() could be updated to return a ResponseResponse type. Looks like there is one call site and it needs a ResponseResponse type.

[Vivek Seth]

Created attachment 357163 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 357163 [details]
Patch

Clearing flags on attachment: 357163

Committed r239132: <https://trac.webkit.org/changeset/239132>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.