Safari does not present CertificateInfo for service-worker served documents
<rdar://problem/58674661>
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.
<rdar://problem/65410875>
Created attachment 404796 [details] Patch
EWS says the new API tests are failing.
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.
Created attachment 404857 [details] Patch
Created attachment 404859 [details] Patch
Created attachment 404889 [details] Patch
We don't check in failing tests though
Committed r264687: <https://trac.webkit.org/changeset/264687> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404889 [details].
Reopening to attach actual fix. That was just a test showing current behavior.
Created attachment 404898 [details] Patch
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.
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.
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.
(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.
Created attachment 404930 [details] Patch
(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==.
Created attachment 404942 [details] Patch
Created attachment 404953 [details] Patch
https://trac.webkit.org/changeset/264724/webkit