Bug 213556

Summary: JavaScript cannot be injected into iframes
Product: WebKit Reporter: Ali Juma <ajuma>
Component: WebKit APIAssignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, cdumez, darin, eugenebut, ggaren, kc, rohitrao, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Ali Juma 2020-06-24 07:10:46 PDT
WKWebView now allows injecting JavaScript into isolated worlds using evaluateJavaScript, but doesn't expose a way to inject WKUserScripts into isolated worlds.

WKUserScripts have important functionality that is missing from evaluateJavaScript. In particular, WKUserScripts can be injected into *all frames* rather than just the main frame, and can be automatically injected when the frame is loaded.

We'd love to move most of our JS injection into isolated worlds, but we need to be able to inject into non-main frames.

One approach to fixing this would be to add a public WKUserScript constructor that takes a WKContentWorld argument (there's already such a constructor in WKUserScriptPrivate.h).
Comment 1 Ali Juma 2020-06-24 07:16:53 PDT
I've also filed FB7772359 for this.
Comment 2 Alexey Proskuryakov 2020-06-24 09:41:15 PDT
rdar://problem/64700590
Comment 3 Alex Christensen 2020-06-24 10:13:26 PDT
I'm wondering if these SPIs would meet your needs?

- (void)_evaluateJavaScript:(NSString *)javaScriptString inFrame:(WKFrameInfo *)frame inContentWorld:(WKContentWorld *)contentWorld completionHandler:(void (^)(id, NSError * error))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
...
- (void)_callAsyncJavaScript:(NSString *)functionBody arguments:(NSDictionary<NSString *, id> *)arguments inFrame:(WKFrameInfo *)frame inContentWorld:(WKContentWorld *)contentWorld completionHandler:(void (^)(id, NSError *error))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
...
- (void)_frames:(void (^)(_WKFrameTreeNode *))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
Comment 4 Ali Juma 2020-06-24 14:51:39 PDT
(In reply to Alex Christensen from comment #3)
> I'm wondering if these SPIs would meet your needs?
> 
> - (void)_evaluateJavaScript:(NSString *)javaScriptString
> inFrame:(WKFrameInfo *)frame inContentWorld:(WKContentWorld *)contentWorld
> completionHandler:(void (^)(id, NSError * error))completionHandler
> WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> ...
> - (void)_callAsyncJavaScript:(NSString *)functionBody
> arguments:(NSDictionary<NSString *, id> *)arguments inFrame:(WKFrameInfo
> *)frame inContentWorld:(WKContentWorld *)contentWorld
> completionHandler:(void (^)(id, NSError *error))completionHandler
> WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> ...
> - (void)_frames:(void (^)(_WKFrameTreeNode *))completionHandler
> WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

My main concerns with this approach are how we would find out about new frames appearing in _frames, and how early we would be able to inject script into a new frame. We don't currently get much by way of navigation callbacks for subframes in public API -- decidePolicyForNavigationAction has a targetFrame, but it's presumably too early to inject at that point, and decidePolicyForNavigationResponse tells us whether the response is for a subframe, but doesn't include a WKFrameInfo. Periodically polling _frames doesn't seem like a great solution.

As a concrete example, we inject logic for form autofill. This code adds event listeners on |document| for events that could be related to a form being edited (like 'focus', 'blur', 'change'). It's important that this gets reliably injected before the user interacts with the page, so using a WKUserScript with WKUserScriptInjectionTimeAtDocumentStart works well.
Comment 5 Brady Eidson 2020-06-29 15:39:16 PDT
Retitling to a more general "JavaScript cannot be injected into iframes"
Comment 6 Brady Eidson 2020-06-29 16:02:55 PDT
Created attachment 403128 [details]
Patch
Comment 7 Alex Christensen 2020-06-29 16:14:49 PDT
Comment on attachment 403128 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKUserScript.mm:44
> +    API::Object::constructInWrapper<API::UserScript>(self, WebCore::UserScript { source, API::UserScript::generateUniqueURL(), { }, { }, API::toWebCoreUserScriptInjectionTime(injectionTime), forMainFrameOnly ? WebCore::UserContentInjectedFrames::InjectInTopFrameOnly : WebCore::UserContentInjectedFrames::InjectInAllFrames, WebCore::WaitForNotificationBeforeInjecting::No }, contentWorld ? *contentWorld->_contentWorld : API::ContentWorld::pageContentWorld());

contentWorld is non-null, so this check and fallback should be unused.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:876
> +    if (details.type == WebCore::ExceptionDetails::Type::InvalidTargetFrame)

This should probably be a switch statement.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:882
> +    [userInfo setObject:static_cast<NSString *>(details.message) forKey:_WKJavaScriptExceptionMessageErrorKey];

I think this static_cast is syntactically unnecessary because of WTF::String::operator NSString *
Comment 8 Darin Adler 2020-06-29 16:18:38 PDT
Comment on attachment 403128 [details]
Patch

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

Should wait for EWS until I set review+, but looks good

> Source/WebCore/bindings/js/ExceptionDetails.h:46
>      // This bizarre explicit initialization of String is because older compilers (like on High Sierra)
>      // don't properly handle partial initialization lists unless every struct member has an explicit default value.
>      // Once we stop building on those platforms we can remove this.

I don’t think we need this any more.

> Source/WebKit/Shared/WebCoreArgumentCoders.h:32
> +#include <WebCore/ExceptionDetails.h>

This seems quite unfortunate. Why do we need to include this here now, instead of in the places that invoke the encode/decode for ExceptionDetails?

> Source/WebKit/UIProcess/API/Cocoa/WKError.mm:78
> +        return WEB_UI_STRING("JavaScript execution was targetted at a frame that can no longer be targetted", "WKErrorJavaScriptMissingFrameTarget description");

Looked up the spelling to be sure: it's "targeted" with only the two "t"s.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:824
>      [self _evaluateJavaScript:javaScriptString asAsyncFunction:NO withSourceURL:nil withArguments:nil forceUserGesture:YES inFrame:nil inWorld:contentWorld completionHandler:completionHandler];

Why aren’t we removing this? It’s no longer mentioned in the header.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:873
> +    RetainPtr<NSMutableDictionary> userInfo = adoptNS([[NSMutableDictionary alloc] init]);

I suggest using auto here.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:887
> +    [userInfo setObject:localizedDescriptionForErrorCode(errorCode) forKey:NSLocalizedDescriptionKey];
> +    [userInfo setObject:static_cast<NSString *>(details.message) forKey:_WKJavaScriptExceptionMessageErrorKey];
> +    [userInfo setObject:@(details.lineNumber) forKey:_WKJavaScriptExceptionLineNumberErrorKey];
> +    [userInfo setObject:@(details.columnNumber) forKey:_WKJavaScriptExceptionColumnNumberErrorKey];
> +
> +    if (!details.sourceURL.isEmpty())
> +        [userInfo setObject:[NSURL _web_URLWithWTFString:details.sourceURL] forKey:_WKJavaScriptExceptionSourceURLErrorKey];

Things like this are so much nicer when they use dictionary literals; easier to read and more efficient. But the sourceURL part of this would be hard to do that way; it would have to be two literals. Or a mutation of a copy of the literal.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:959
> +            auto error = nsErrorFromExceptionDetails(*details);
> +            rawHandler(nil, error.get());

I think this would read better as a one-liner without the local variable.
Comment 9 Geoffrey Garen 2020-06-29 16:20:42 PDT
Comment on attachment 403128 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/API/Cocoa/WKError.mm:77
> +    case WKErrorJavaScriptMissingFrameTarget:

Let's call this WKErrorJavaScriptInvalidFrameTarget since "invalid" is the word you used in headerdoc.

> Source/WebKit/UIProcess/API/Cocoa/WKError.mm:78
> +        return WEB_UI_STRING("JavaScript execution was targetted at a frame that can no longer be targetted", "WKErrorJavaScriptMissingFrameTarget description");

targetted => targeted

But also, maybe we can be clearer: JavaScript execution targeted an invalid frame.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:237
> + Passing `nil` for the frame argument is equivalent to passing in a value representing the main frame of the WKWebView.

=> Pass nil to target the main frame.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:287
> + Passing `nil` for the frame argument is equivalent to passing in a value representing the main frame of the WKWebView.

=> Pass nil to target the main frame.
Comment 10 Brady Eidson 2020-06-29 17:49:03 PDT
Oh boy, do I have news for you all about the use of the misspelled "targetted" in WebKit... :)

(Will still fix it here)
Comment 11 Brady Eidson 2020-06-29 17:53:09 PDT
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:237
> > + Passing `nil` for the frame argument is equivalent to passing in a value representing the main frame of the WKWebView.
> 
> => Pass nil to target the main frame.
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:287
> > + Passing `nil` for the frame argument is equivalent to passing in a value representing the main frame of the WKWebView.
> 
> => Pass nil to target the main frame.

Not quite accurate, since you can also target the main frame with a valid WKFrameInfo targeting the main frame.

But I changed it to "Passing nil is equivalent to targeting the main frame."
Comment 12 Brady Eidson 2020-06-29 18:12:49 PDT
Created attachment 403149 [details]
Patch
Comment 13 Brady Eidson 2020-06-29 18:57:32 PDT
(In reply to Darin Adler from comment #8)
> Comment on attachment 403128 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403128&action=review
> 
> Should wait for EWS until I set review+, but looks good
> 
> > Source/WebCore/bindings/js/ExceptionDetails.h:46
> >      // This bizarre explicit initialization of String is because older compilers (like on High Sierra)
> >      // don't properly handle partial initialization lists unless every struct member has an explicit default value.
> >      // Once we stop building on those platforms we can remove this.
> 
> I don’t think we need this any more.

The build bot errors with this patch are because I removed this 😭
Comment 14 Brady Eidson 2020-06-29 18:59:17 PDT
Created attachment 403158 [details]
Patch
Comment 15 Brady Eidson 2020-06-29 18:59:36 PDT
(In reply to Brady Eidson from comment #13)
> (In reply to Darin Adler from comment #8)
> > Comment on attachment 403128 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=403128&action=review
> > 
> > Should wait for EWS until I set review+, but looks good
> > 
> > > Source/WebCore/bindings/js/ExceptionDetails.h:46
> > >      // This bizarre explicit initialization of String is because older compilers (like on High Sierra)
> > >      // don't properly handle partial initialization lists unless every struct member has an explicit default value.
> > >      // Once we stop building on those platforms we can remove this.
> > 
> > I don’t think we need this any more.
> 
> The build bot errors with this patch are because I removed this 😭

E.g. https://ews-build.webkit.org/#/builders/22/builds/21940/steps/7/logs/errors
Comment 16 Brady Eidson 2020-06-29 22:38:50 PDT
*** Bug 204557 has been marked as a duplicate of this bug. ***
Comment 17 EWS 2020-06-29 22:43:23 PDT
Committed r263727: <https://trac.webkit.org/changeset/263727>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403158 [details].
Comment 18 Darin Adler 2020-06-30 10:57:14 PDT
(In reply to Brady Eidson from comment #15)
> (In reply to Brady Eidson from comment #13)
> > (In reply to Darin Adler from comment #8)
> > > > Source/WebCore/bindings/js/ExceptionDetails.h:46
> > > >      // This bizarre explicit initialization of String is because older compilers (like on High Sierra)
> > > >      // don't properly handle partial initialization lists unless every struct member has an explicit default value.
> > > >      // Once we stop building on those platforms we can remove this.
> > > 
> > > I don’t think we need this any more.
> > 
> > The build bot errors with this patch are because I removed this 😭
> 
> E.g.
> https://ews-build.webkit.org/#/builders/22/builds/21940/steps/7/logs/errors

Seems like the comment is inaccurate because whatever the problem is, it's not about "older compilers".