Bug 209521

Summary: App-bound domain checks should provide more debugging details at script evaluation sites
Product: WebKit Reporter: Kate Cheney <katherine_cheney>
Component: WebKit Misc.Assignee: Kate Cheney <katherine_cheney>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, bfulgham, cdumez, ews-watchlist, mkwst, simon.fraser, thorton
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Kate Cheney 2020-03-24 16:38:48 PDT 
This would be helpful for debugging.
Comment 1 Kate Cheney 2020-03-24 16:39:27 PDT 
<rdar://problem/60837954>
Comment 2 Kate Cheney 2020-03-24 16:59:55 PDT 
Created attachment 394443 [details]
Patch
Comment 3 Chris Dumez 2020-03-24 17:04:07 PDT 
Comment on attachment 394443 [details]
Patch

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

> Source/WebCore/bindings/js/ScriptController.cpp:580
> +        m_frame.document()->addConsoleMessage(MessageSource::Security, MessageLevel::Warning, "Ignoring user script injection for non-app bound domain.");

Why do release logging here?
How do you know document is non null?

> Source/WebCore/page/Frame.cpp:630
>          return;

Why do release logging here?
Comment 4 Kate Cheney 2020-03-24 17:08:06 PDT 
Comment on attachment 394443 [details]
Patch

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

>> Source/WebCore/bindings/js/ScriptController.cpp:580
>> +        m_frame.document()->addConsoleMessage(MessageSource::Security, MessageLevel::Warning, "Ignoring user script injection for non-app bound domain.");
> 
> Why do release logging here?
> How do you know document is non null?

I didn't see any other examples of release logging so I wasn't sure if I could here. I'll add a check for m_frame.document().

>> Source/WebCore/page/Frame.cpp:630
>>          return;
> 
> Why do release logging here?

Same as above, I didn't see any other examples of release logging so I wasn't sure if I could here
Comment 5 Kate Cheney 2020-03-24 17:08:49 PDT 
(In reply to katherine_cheney from comment #4)
> Comment on attachment 394443 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394443&action=review
> 
> >> Source/WebCore/bindings/js/ScriptController.cpp:580
> >> +        m_frame.document()->addConsoleMessage(MessageSource::Security, MessageLevel::Warning, "Ignoring user script injection for non-app bound domain.");
> > 
> > Why do release logging here?
> > How do you know document is non null?
> 
> I didn't see any other examples of release logging so I wasn't sure if I
> could here. I'll add a check for m_frame.document().
> 

(no other examples in this file)

> >> Source/WebCore/page/Frame.cpp:630
> >>          return;
> > 
> > Why do release logging here?
> 
> Same as above, I didn't see any other examples of release logging so I
> wasn't sure if I could here
Comment 6 Chris Dumez 2020-03-24 17:38:07 PDT 
(In reply to katherine_cheney from comment #4)
> Comment on attachment 394443 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394443&action=review
> 
> >> Source/WebCore/bindings/js/ScriptController.cpp:580
> >> +        m_frame.document()->addConsoleMessage(MessageSource::Security, MessageLevel::Warning, "Ignoring user script injection for non-app bound domain.");
> > 
> > Why do release logging here?
> > How do you know document is non null?
> 
> I didn't see any other examples of release logging so I wasn't sure if I
> could here. I'll add a check for m_frame.document().
> 
> >> Source/WebCore/page/Frame.cpp:630
> >>          return;
> > 
> > Why do release logging here?
> 
> Same as above, I didn't see any other examples of release logging so I
> wasn't sure if I could here

I believe you can do release logging anywhere you'd like.
Comment 7 Kate Cheney 2020-03-25 08:54:32 PDT 
Created attachment 394502 [details]
Patch
Comment 8 EWS 2020-03-25 09:30:01 PDT 
Committed r258986: <https://trac.webkit.org/changeset/258986>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394502 [details].
Comment 9 Simon Fraser (smfr) 2020-03-25 09:55:38 PDT Comment hidden (obsolete)