Bug 179018 – rwt should allow service worker to load localhost HTTPS resources with any certificate

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

rwt should allow service worker to load localhost HTTPS resources with any certificate

[youenn fablet]

Created attachment 325358 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Chris Dumez]

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

[Chris Dumez]

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!

[Alex Christensen]

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.

[Chris Dumez]

(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.

[Alex Christensen]

A delegate should probably be per-WebsiteDataStore

[youenn fablet]

Created attachment 325388 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Chris Dumez]

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.

[youenn fablet]

Created attachment 325451 [details]
Patch

[Chris Dumez]

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).

[Chris Dumez]

Created attachment 325455 [details]
Tess that can be unskipped for me locally with certificate fix

[youenn fablet]

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.

[youenn fablet]

Created attachment 325459 [details]
Patch

[youenn fablet]

Created attachment 325474 [details]
Patch

[Chris Dumez]

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.

[youenn fablet]

> > 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.

[WebKit Commit Bot]

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

[Chris Dumez]

(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.

[Chris Dumez]

(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

[youenn fablet]

Created attachment 325485 [details]
Patch for landing

[Chris Dumez]

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/ :(

[Chris Dumez]

Created attachment 325495 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 325495 [details]
Patch

Clearing flags on attachment: 325495

Committed r224250: <https://trac.webkit.org/changeset/224250>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35567860>