Safari does not present CertificateInfo for service-worker served documents

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

Safari does not present CertificateInfo for service-worker served documents

[Radar WebKit Bug Importer]

<rdar://problem/58674661>

[youenn fablet]

This happens in a few cases:
- service worker intercepts the document fetch, call fetch() itself and sends back the response
- service worker intercepts the document fetch and returns a DOMCache response/synthesized response.

[youenn fablet]

<rdar://problem/65410875>

[Alex Christensen]

Created attachment 404796 [details]
Patch

[Geoffrey Garen]

EWS says the new API tests are failing.

[youenn fablet]

Comment on attachment 404796 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:2163
> +    "    event.respondWith(new Response(new Blob(['<script>alert(\"hello from service worker\")</script>'], {type: 'text/html'})));"

We should probably exercise three code paths here.
The first one with synthetic responses as above.

The second one is when using DOMCache by putting in DOMCache an existing resource and the feeding it into respondWith.
You can find an example in https://mdn.github.io/sw-test/sw.js

The third one by doing something like: event.respondWith(fetch(event.request.url)).
Hopefully this one should work.

[Alex Christensen]

Created attachment 404857 [details]
Patch

[Alex Christensen]

Created attachment 404859 [details]
Patch

[Alex Christensen]

Created attachment 404889 [details]
Patch

[Brady Eidson]

We don't check in failing tests though

[EWS]

Committed r264687: <https://trac.webkit.org/changeset/264687>

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

[Alex Christensen]

Reopening to attach actual fix.  That was just a test showing current behavior.

[Alex Christensen]

Created attachment 404898 [details]
Patch

[Darin Adler]

Comment on attachment 404898 [details]
Patch

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

> Source/WebCore/platform/network/cf/CertificateInfoCFNet.cpp:53
> +    CertificateInfo copy;
> +#if HAVE(SEC_TRUST_SERIALIZATION)
> +    copy.m_trust = m_trust;
> +#endif
> +    copy.m_certificateChain = m_certificateChain;
> +    return copy;

Since there is nothing to isolate, can just write this:

    return *this;

Could even put it inline in the header if you like.

[youenn fablet]

Comment on attachment 404898 [details]
Patch

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

> Source/WebCore/workers/WorkerScriptLoader.cpp:130
> +    options.certificateInfoPolicy = certificateInfoPolicy;

Another way to pass this information down to network process is to piggy back on the destination option, which should be Destination::ServiceWorker, not Destination::Worker.
For ServiceWorker destination, NetworkResourceLoader could decide to pass the certificateInfo (or WebLoaderStrategy could set needsCertificateInfo to true).

We could think of removing certificateInfoPolicy/needsCertificateInfo in the future.

> Source/WebCore/workers/service/ServiceWorkerGlobalScope.h:71
> +    const CertificateInfo& certificateInfo() { return m_contextData.certificateInfo; }

const

> Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp:106
> +    resourceResponse.setCertificateInfo(WTFMove(certificateInfo));

We should probably only do that if the response has no certificate info. And restrict that to navigation loads only.
We do not want to pretend to use the service worker certificate info for third party resources.

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:408
> +        // FIXME: Consider serializing the certificate info to disk so WKWebView.serverTrust will be non-null when offline.

Could you also add a FIXME for the following case:
- service worker is installed and has a certificate info
- service worker is soft updated, script does not change but certificate info does (say the old certificate was soon to be obsolete).
We should in that case update the service worker registration info and potentially the service worker certificate info.
A quick fix would be to consider that a change in certificate info is like a change in the script. We would then trigger install/activate and update the on disk registration.
See SWServerJobQueue::scriptFetchFinished.

[Alex Christensen]

Thanks for the reviews.  I found that we do need to persist the data to fix the radar, so I incremented schemaVersion and added it.
I added a FIXME for the certificate change in soft update, and used destination to mean certificate info policy.

[youenn fablet]

(In reply to Alex Christensen from comment #16)
> Thanks for the reviews.  I found that we do need to persist the data to fix
> the radar, so I incremented schemaVersion and added it.
> I added a FIXME for the certificate change in soft update, and used
> destination to mean certificate info policy.

Great!
Is there an way to easily compare CertificateInfo equality?
If so, we should probably try to update CertificateInfo as if the script was modified as a follow-up patch.

[Alex Christensen]

Created attachment 404930 [details]
Patch

[Alex Christensen]

(In reply to youenn fablet from comment #17)
> Is there an way to easily compare CertificateInfo equality?
There is a function called certificatesMatch.  We should probably refactor that into something like CertificateInfo::operator==.

[Alex Christensen]

Created attachment 404942 [details]
Patch

[Alex Christensen]

Created attachment 404953 [details]
Patch

[Alex Christensen]

https://trac.webkit.org/changeset/264724/webkit