Bug 179018

Summary: rwt should allow service worker to load localhost HTTPS resources with any certificate
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, buildbot, cdumez, commit-queue, ggaren, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=178985
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Patch
none
Tess that can be unskipped for me locally with certificate fix
none
Patch
none
Patch
none
Patch for landing
none
Patch none

Description youenn fablet 2017-10-30 09:55:54 PDT
rwt should allow service worker to load localhost HTTPS resources with any certificate
Comment 1 youenn fablet 2017-10-30 09:59:42 PDT
Created attachment 325358 [details]
Patch
Comment 2 Build Bot 2017-10-30 11:23:40 PDT
Comment on attachment 325358 [details]
Patch

Attachment 325358 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5041249

New failing tests:
http/tests/workers/service/service-worker-fetch.https.html
Comment 3 Build Bot 2017-10-30 11:23:41 PDT
Created attachment 325361 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 4 Chris Dumez 2017-10-30 13:31:55 PDT
Comment on attachment 325358 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessPool.cpp:1394
> +#if ENABLE(SERVICE_WORKERS)

ah ah ah You have an extra S at the end of WORKER so this code does nothing :D
Comment 5 Chris Dumez 2017-10-30 13:32:29 PDT
Comment on attachment 325358 [details]
Patch

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

>> Source/WebKit/UIProcess/WebProcessPool.cpp:1394
>> +#if ENABLE(SERVICE_WORKERS)
> 
> ah ah ah You have an extra S at the end of WORKER so this code does nothing :D

I made the same mistake twice already!
Comment 6 Alex Christensen 2017-10-30 14:54:04 PDT
Comment on attachment 325358 [details]
Patch

Please don't add this or anything like it to WKContext/WKProcessPool.  Just respond to the challenge.
We're also trying to remove the step of canAuthenticateAgainstProtectionSpace.  This would be a strange step in the wrong direction.
Comment 7 Chris Dumez 2017-10-30 14:56:50 PDT
(In reply to Alex Christensen from comment #6)
> Comment on attachment 325358 [details]
> Patch
> 
> Please don't add this or anything like it to WKContext/WKProcessPool.  Just
> respond to the challenge.
> We're also trying to remove the step of
> canAuthenticateAgainstProtectionSpace.  This would be a strange step in the
> wrong direction.

Challenge is per page, no? A ServiceWorker is not associated with a page.
Comment 8 Alex Christensen 2017-10-30 15:29:18 PDT
A delegate should probably be per-WebsiteDataStore
Comment 9 youenn fablet 2017-10-30 16:44:35 PDT
Created attachment 325388 [details]
Patch
Comment 10 Build Bot 2017-10-30 18:06:17 PDT
Comment on attachment 325388 [details]
Patch

Attachment 325388 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5044523

New failing tests:
http/tests/workers/service/service-worker-fetch.https.html
Comment 11 Build Bot 2017-10-30 18:06:19 PDT
Created attachment 325395 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 12 Chris Dumez 2017-10-31 09:25:18 PDT
Comment on attachment 325388 [details]
Patch

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

> Source/WebKit/UIProcess/ServiceWorkerProcessProxy.cpp:61
> +            challenge->useCredential(nullptr);

Here is what WKTR is doing in this case:
    if (WKProtectionSpaceGetAuthenticationScheme(protectionSpace) == kWKProtectionSpaceAuthenticationSchemeServerTrustEvaluationRequested) {
        // Any non-empty credential signals to accept the server trust. Since the cross-platform API
        // doesn't expose a way to create a credential from server trust, we use a password credential.

        WKRetainPtr<WKCredentialRef> credential = adoptWK(WKCredentialCreate(toWK("accept server trust").get(), toWK("").get(), kWKCredentialPersistenceNone));
        WKAuthenticationDecisionListenerUseCredential(decisionListener, credential.get());
        return;
    }

I believe the parameter should not be nullptr. You need to construct credentials as WKTR does.
Comment 13 youenn fablet 2017-10-31 09:30:11 PDT
Created attachment 325451 [details]
Patch
Comment 14 Chris Dumez 2017-10-31 09:38:54 PDT
Comment on attachment 325451 [details]
Patch

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

> Source/WebKit/UIProcess/ServiceWorkerProcessProxy.cpp:69
> +    ASSERT_NOT_REACHED();

I don't think it is a valid assertion. What prevents regular HTTP authentication to happen in a service worker?
Also, I think we should deal with the challenge either way (either cancel or probably better, do the default behavior).
Comment 15 Chris Dumez 2017-10-31 10:03:16 PDT
Created attachment 325455 [details]
Tess that can be unskipped for me locally with certificate fix
Comment 16 youenn fablet 2017-10-31 10:10:35 PDT
Comment on attachment 325451 [details]
Patch

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

>> Source/WebKit/UIProcess/ServiceWorkerProcessProxy.cpp:69
>> +    ASSERT_NOT_REACHED();
> 
> I don't think it is a valid assertion. What prevents regular HTTP authentication to happen in a service worker?
> Also, I think we should deal with the challenge either way (either cancel or probably better, do the default behavior).

I will replace it with notImplemented so that it is clear that things are missing here and that we should really delegate this to the app.
Comment 17 youenn fablet 2017-10-31 10:49:42 PDT
Created attachment 325459 [details]
Patch
Comment 18 youenn fablet 2017-10-31 12:53:31 PDT
Created attachment 325474 [details]
Patch
Comment 19 Chris Dumez 2017-10-31 13:40:54 PDT
Comment on attachment 325474 [details]
Patch

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

r=me

> LayoutTests/TestExpectations:140
>  webkit.org/b/179035 imported/w3c/web-platform-tests/service-workers/cache-storage [ Skip ]

Will likely need rebaselining now that this was fixed.
Comment 20 youenn fablet 2017-10-31 13:42:03 PDT
> > LayoutTests/TestExpectations:140
> >  webkit.org/b/179035 imported/w3c/web-platform-tests/service-workers/cache-storage [ Skip ]
> 
> Will likely need rebaselining now that this was fixed.

Let's follow up once it is landed.
Comment 21 WebKit Commit Bot 2017-10-31 13:42:48 PDT
Comment on attachment 325474 [details]
Patch

Rejecting attachment 325474 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 325474, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
eb-platform-tests/streams/readable-streams/readable-stream-reader.serviceworker.https-expected.txt
patching file LayoutTests/imported/w3c/web-platform-tests/streams/readable-streams/tee.serviceworker.https-expected.txt
patching file LayoutTests/imported/w3c/web-platform-tests/streams/readable-streams/templated.serviceworker.https-expected.txt

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Chris Dumez']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/5054351
Comment 22 Chris Dumez 2017-10-31 13:42:48 PDT
(In reply to youenn fablet from comment #20)
> > > LayoutTests/TestExpectations:140
> > >  webkit.org/b/179035 imported/w3c/web-platform-tests/service-workers/cache-storage [ Skip ]
> > 
> > Will likely need rebaselining now that this was fixed.
> 
> Let's follow up once it is landed.

My point is that it will likely conflict and not land. But let's see if you're lucky.
Comment 23 Chris Dumez 2017-10-31 13:43:02 PDT
(In reply to Chris Dumez from comment #22)
> (In reply to youenn fablet from comment #20)
> > > > LayoutTests/TestExpectations:140
> > > >  webkit.org/b/179035 imported/w3c/web-platform-tests/service-workers/cache-storage [ Skip ]
> > > 
> > > Will likely need rebaselining now that this was fixed.
> > 
> > Let's follow up once it is landed.
> 
> My point is that it will likely conflict and not land. But let's see if
> you're lucky.

Well, surprise :D
Comment 24 youenn fablet 2017-10-31 13:50:35 PDT
Created attachment 325485 [details]
Patch for landing
Comment 25 Chris Dumez 2017-10-31 13:58:28 PDT
Comment on attachment 325485 [details]
Patch for landing

You did not unskip the tests under imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/ :(
Comment 26 Chris Dumez 2017-10-31 14:23:07 PDT
Created attachment 325495 [details]
Patch
Comment 27 WebKit Commit Bot 2017-10-31 15:11:52 PDT
Comment on attachment 325495 [details]
Patch

Clearing flags on attachment: 325495

Committed r224250: <https://trac.webkit.org/changeset/224250>
Comment 28 WebKit Commit Bot 2017-10-31 15:11:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Radar WebKit Bug Importer 2017-11-15 12:36:20 PST
<rdar://problem/35567860>