Bug 115321 – Implement file path restrictions in WebKit Objective C API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Alexey Proskuryakov]

Currently, allowedDirectory argument is just ignored.

<rdar://problem/13574729>

[Alexey Proskuryakov]

Created attachment 199959 [details]
proposed patch

[Darin Adler]

Comment on attachment 199959 [details]
proposed patch

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

> Source/WebCore/platform/KURL.cpp:340
> +KURL KURL::fromFileSystemPath(const String& path)
> +{
> +    return KURL(ParsedURLString, "file://" + path);
> +}

This doesn’t seem right unless the path has special characters such as "#" already URL-encoded. Given the function name, I don’t see how that is clear.

[Darin Adler]

Comment on attachment 199959 [details]
proposed patch

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

> Source/WebCore/platform/KURL.h:53
> +enum FileSystemPathTag { FileSystemPath };

Looks like this is unused; presumably used in an earlier version of the patch.

[Darin Adler]

Comment on attachment 199959 [details]
proposed patch

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

>> Source/WebCore/platform/KURL.cpp:340
>> +}
> 
> This doesn’t seem right unless the path has special characters such as "#" already URL-encoded. Given the function name, I don’t see how that is clear.

I think a correct version needs to call encodeWithURLEscapeSequences. And I think we need a fast path in encodeWithURLEscapeSequences for when nothing needs encoding. And I think that KURL::setPath has an incorrect FIXME in it.

[Alexey Proskuryakov]

I'd like to hear what Sam or Anders think about the proposed C API before updating the patch. Perhaps it should take a WKURLRef, not a WKStringRef?

[Sam Weinig]

(In reply to comment #5)
> I'd like to hear what Sam or Anders think about the proposed C API before updating the patch. Perhaps it should take a WKURLRef, not a WKStringRef?

A URL makes more sense to me.

[Alexey Proskuryakov]

Created attachment 199984 [details]
proposed patch

> A URL makes more sense to me.

OK

[Darin Adler]

Comment on attachment 199984 [details]
proposed patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:751
> +        resourceDirectoryURL = KURL(ParsedURLString, "file:///");
> +
> +    String resourceDirectoryPath = resourceDirectoryURL.fileSystemPath();

Seems a little strange to construct a URL with "file:///" in it just to extract the "/" out of it afterward. Could define resourceDirectoryPath earlier and set it to "/" when resourceDirectoryURLString in a more direct way.

I think it’s also good to use ASCIILiteral in cases like this.

> Source/WebKit2/UIProcess/WebPageProxy.h:291
> +    void loadFile(const String& fileURL, const String& resourceDirectoryURL);

Strange for this to take URL strings instead of URLs.

> Source/WebKit2/UIProcess/API/C/WKPage.cpp:97
> +    toImpl(pageRef)->loadFile(toWTFString(fileURL), toWTFString(resourceDirectoryURL));

Why not convert WKURLRef to KURL instead of going through String?

> Source/WebKit2/UIProcess/API/C/WKPage.h:365
> +WK_EXPORT void WKPageLoadFile(WKPageRef page, WKURLRef fileURL, WKURLRef resourceDirectoryURL);

I agree that these argument types make sense given Cocoa’s use of CFURL and NSURL for file paths.

> Source/WebKit2/UIProcess/API/mac/WKBrowsingContextController.mm:128
>      WKRetainPtr<WKURLRef> wkURL = adoptWK(WKURLCreateWithCFURL((CFURLRef)URL));
> -    WKPageLoadURL(self._pageRef, wkURL.get());
> +    WKRetainPtr<WKURLRef> wkAllowedDirectory = adoptWK(WKURLCreateWithCFURL((CFURLRef)allowedDirectory));
> +    
> +    WKPageLoadFile(self._pageRef, wkURL.get(), wkAllowedDirectory.get());

It seems a little unwieldy to have these local variables. WIth a helper function to convert NSURL to WKRetainPtr<WKURLRef> we could make this a lot simpler.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/r149422>.

> Strange for this to take URL strings instead of URLs.

I was following the pattern of loadURL(const String&) here.