Bug 234543 – Archived subresource loads fail if m_allowedNetworkHosts doesn't include the remote URL

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Matt Woodrow]

rdar://83970256

If WebKit is configured with a set of allowed network hosts, then subresource loads will be cancelled if the URL isn't within the list. This happens before the check to see if the subresource was already within the archived content, and wouldn't have actually needed a network load.

[Matt Woodrow]

Created attachment 447679 [details]
Patch

[Matt Woodrow]

Created attachment 447746 [details]
Patch

[Alexey Proskuryakov]

Not sure if the proposed change is OK given the initial intent of this SPI (disallow any 3rd party content). It's still 3rd party content even when it's loaded via an archive, and having it can still have security implications!

CC Alex to help untangle this.

[Alex Christensen]

Comment on attachment 447746 [details]
Patch

This is problematic for many reasons:
1. It doesn't contain a unit test to verify we won't break it in the future.
2. It does not contain a good description of what is going on or under what circumstances this changes behavior.
3. Being an "archive load" is not an inherent property of a resource request, which ought to be basically just a URL and header fields
4.  It substantially changes the behavior of when scheduleArchiveLoad is called, which almost certainly breaks web archive loading somehow.
5. There is no link to a radar or something motivating this change.

I believe the intent of this is to make allowsLoadFromURL return true if we know it will only load from a web archive and not from the network when _allowedNetworkHosts is being used.  Such a change should clearly not affect anything else.

[Matt Woodrow]

(In reply to Alex Christensen from comment #4)
> Comment on attachment 447746 [details]
> Patch
> 
> This is problematic for many reasons:
> 1. It doesn't contain a unit test to verify we won't break it in the future.

Agreed, can you please point me to related tests so that I have an idea where to start?
I saw WKURLSchemeHandler-1.mm, but I'm not sure where to start with loading sub resources from an archive.

> 2. It does not contain a good description of what is going on or under what
> circumstances this changes behavior.

Apologies, looks like --update-changelogs changed the description text. Is what's in the first comment of the bug sufficient?

> 3. Being an "archive load" is not an inherent property of a resource
> request, which ought to be basically just a URL and header fields

Would a change in naming to 'will load from archive' be satisfactory here?

> 4.  It substantially changes the behavior of when scheduleArchiveLoad is
> called, which almost certainly breaks web archive loading somehow.

Indeed it does. I was trying to avoid checking 'will load from archive' during the network host check, in advance of actually scheduling the archive load, since that feels overly fragile. It felt preferable to just schedule, and condition the other behaviour on the result.

I tried to audit possible behaviour changes from calling it early, and couldn't find anything, plus it passes test. It's possible that this is better suited to someone who knows this code.


> 5. There is no link to a radar or something motivating this change.

Comment 0 links to rdar://83970256, apologies if that's not the correct form of linkage.

[Alex Christensen]

(In reply to Matt Woodrow from comment #5)
> (In reply to Alex Christensen from comment #4)
> > Comment on attachment 447746 [details]
> > Patch
> > 
> > This is problematic for many reasons:
> > 1. It doesn't contain a unit test to verify we won't break it in the future.
> 
> Agreed, can you please point me to related tests so that I have an idea
> where to start?
> I saw WKURLSchemeHandler-1.mm, but I'm not sure where to start with loading
> sub resources from an archive.
Yes, that's where to find existing unit tests that use _allowedNetworkHosts.  Search for TEST(LoadWebArchive, HTTPSUpgrade) to see how to load a web archive.  You probably won't need to create a web archive dynamically for this, but you could.  Once you tell WKWebView to load a web archive, it will automatically load sub resources from the web archive.

> > 2. It does not contain a good description of what is going on or under what
> > circumstances this changes behavior.
> 
> Apologies, looks like --update-changelogs changed the description text. Is
> what's in the first comment of the bug sufficient?
It could use more clear language.  "Using WKWebViewConfiguration._allowedNetworkHosts prevents loads from web archives even when the network will not be used" makes it quite clear what is going on.

> Would a change in naming to 'will load from archive' be satisfactory here?
No, I don't think it should be a member of ResourceRequest.  I think it should be another parameter to Page::allowsLoadFromURL

> > 5. There is no link to a radar or something motivating this change.
> 
> Comment 0 links to rdar://83970256, apologies if that's not the correct form
> of linkage.
Sorry, I didn't see that.  That's a good link.  Put it in the Change Log.

[Alex Christensen]

I would fix this bug by moving the m_allowedNetworkHosts part of the allowsLoadFromURL check from ResourceLoadNotifier::dispatchWillSendRequest to WebLoaderStrategy::scheduleLoadFromNetworkProcess or somewhere like that.

[Matt Woodrow]

Created attachment 447837 [details]
Patch

[Alex Christensen]

Comment on attachment 447837 [details]
Patch

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

nicer

> Tools/TestWebKitAPI/Tests/mac/LoadWebArchive.mm:259
> +    NSData *data = [NSPropertyListSerialization dataFromPropertyList:archive format:NSPropertyListBinaryFormat_v1_0 errorDescription:nil];

This is all duplicate code with the test before it.  Could we instead share it and make a static function that returns an NSData?

[Matt Woodrow]

Created attachment 447852 [details]
Patch

[Alex Christensen]

Created attachment 447894 [details]
Patch

[Alex Christensen]

Created attachment 447896 [details]
Patch

[Alex Christensen]

Comment on attachment 447896 [details]
Patch

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

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:311
> +            RunLoop::main().dispatch([resourceLoader = Ref { resourceLoader }] {
> +                resourceLoader->didFail(resourceLoader->blockedError());

This changed to call didFail on another run loop iteration to make this behave more like a real load that was blocked and make the other existing tests pass.

> Source/WebKit/WebProcess/Network/WebResourceLoader.cpp:116
> +            if (!page->allowsLoadFromURL(proposedRequest.url(), mainFrameMainResource))

I added this to prevent an allowed host from redirecting to a forbidden host.  ResourceLoadNotifier::dispatchWillSendRequest was called when redirecting and when preparing to send the initial request, so after moving it we need to do this in two places.

[EWS]

Committed r287414 (245549@main): <https://commits.webkit.org/245549@main>

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