Bug 209521 – App-bound domain checks should provide more debugging details at script evaluation sites

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

This would be helpful for debugging.

[Kate Cheney]

<rdar://problem/60837954>

[Kate Cheney]

Created attachment 394443 [details]
Patch

[Chris Dumez]

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?

[Kate Cheney]

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

[Kate Cheney]

(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

[Chris Dumez]

(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.

[Kate Cheney]

Created attachment 394502 [details]
Patch

[EWS]

Committed r258986: <https://trac.webkit.org/changeset/258986>

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

[Simon Fraser (smfr)]

Comment on attachment 394502 [details]
Patch

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

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

I don't think "user script injection" makes sense here, authors won't know what that means when they see it in the console. This needs to say something like "Scripts injected by extensions" or "Scripts injected by the application". Ideally it would explicitly identify the extension or app by name.

Also, no-one will know what "app-bound domains" are.