Summary: | JavaScript cannot be injected into iframes | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ali Juma <ajuma> | ||||||||
Component: | WebKit API | Assignee: | 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
Ali Juma
2020-06-24 07:10:46 PDT
I've also filed FB7772359 for this. 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)); (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. Retitling to a more general "JavaScript cannot be injected into iframes" Created attachment 403128 [details]
Patch
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 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 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. Oh boy, do I have news for you all about the use of the misspelled "targetted" in WebKit... :) (Will still fix it here)
> > 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."
Created attachment 403149 [details]
Patch
(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 😭 Created attachment 403158 [details]
Patch
(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 *** Bug 204557 has been marked as a duplicate of this bug. *** Committed r263727: <https://trac.webkit.org/changeset/263727> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403158 [details]. (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". |