Bug 234934 – Implement CSP strict-dynamic for module scripts

• Component: New Bugs
• Status: RESOLVED FIXED

[Kate Cheney]

Implement CSP strict-dynamic for module scripts

[Kate Cheney]

Created attachment 448534 [details]
Patch

[Kate Cheney]

rdar://83728374

[Brent Fulgham]

Comment on attachment 448534 [details]
Patch

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

r=me

> Source/WebCore/dom/ScriptElement.cpp:379
> +    if (!contentSecurityPolicy.allowNonParserInsertedScripts(m_element.document().url(), m_startLineNumber, m_element.nonce(), sourceCode.source(), m_parserInserted))

Is there a specific CSP spec step we should reference for this decision? I know most of the CSP code doesn't add such comments, but I find them helpful when reviewing other areas of WebKit.

> LayoutTests/http/tests/security/contentSecurityPolicy/strict-dynamic-module-script.html:6
> +        <script src="/js-test-resources/testharnessreport.js" nonce='dummy'></script>

Nit: Weird spacing here

[Kate Cheney]

Comment on attachment 448534 [details]
Patch

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

>> Source/WebCore/dom/ScriptElement.cpp:379
>> +    if (!contentSecurityPolicy.allowNonParserInsertedScripts(m_element.document().url(), m_startLineNumber, m_element.nonce(), sourceCode.source(), m_parserInserted))
> 
> Is there a specific CSP spec step we should reference for this decision? I know most of the CSP code doesn't add such comments, but I find them helpful when reviewing other areas of WebKit.

Do you mean the m_startLineNumber change or why we check for allowNonParserInsertedScripts here? 

If it is the latter, generally yes but not specifically to module scripts. Under https://www.w3.org/TR/CSP3/#strict-dynamic-usage the spec explains "Script requests which are triggered by non-"parser-inserted" script elements are allowed." requestModuleScript is a place we were formerly not checking a requested script's parser-inserted status.

[Brent Fulgham]

Comment on attachment 448534 [details]
Patch

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

>>> Source/WebCore/dom/ScriptElement.cpp:379
>>> +    if (!contentSecurityPolicy.allowNonParserInsertedScripts(m_element.document().url(), m_startLineNumber, m_element.nonce(), sourceCode.source(), m_parserInserted))
>> 
>> Is there a specific CSP spec step we should reference for this decision? I know most of the CSP code doesn't add such comments, but I find them helpful when reviewing other areas of WebKit.
> 
> Do you mean the m_startLineNumber change or why we check for allowNonParserInsertedScripts here? 
> 
> If it is the latter, generally yes but not specifically to module scripts. Under https://www.w3.org/TR/CSP3/#strict-dynamic-usage the spec explains "Script requests which are triggered by non-"parser-inserted" script elements are allowed." requestModuleScript is a place we were formerly not checking a requested script's parser-inserted status.

I meant the latter, so I think you've explained it.

[Kate Cheney]

Created attachment 448604 [details]
Patch for landing

[EWS]

Committed r287756 (245819@main): <https://commits.webkit.org/245819@main>

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