Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 [amp-script] skip sha384 check for cross-origin scripts in sandboxed mode #36618

Merged
merged 3 commits into from
Nov 2, 2021

Conversation

zshnr
Copy link
Contributor

@zshnr zshnr commented Oct 27, 2021

Summary

Skips the CSP hash check for a cross-origin source passed to amp-script since this is one of the features of sandboxed mode introduced in PR #33643.

Fixes #36614

'alert(1)'
)

service.checkSha384.withArgs('alert(1)').resolves()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function should never be called, so we don't need to stub it

Suggested change
service.checkSha384.withArgs('alert(1)').resolves()

element.setAttribute('src', 'https://bar.example/bar.js')
element.setAttribute('sandboxed', '')

script.buildCallback()
Copy link
Member

@samouri samouri Oct 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buildCallback returns a promise, so you'll need to await it

Suggested change
script.buildCallback()
await script.buildCallback()

@zshnr zshnr force-pushed the fix-csp-check-sandboxed-mode branch from 4799504 to 98ea7cc Compare November 2, 2021 13:45
@zshnr zshnr changed the title [amp-script] skips sha384 check for cross-origin scripts in sandboxed mode 🐛 [amp-script] skip sha384 check for cross-origin scripts in sandboxed mode Nov 2, 2021
@zshnr zshnr marked this pull request as ready for review November 2, 2021 17:48
@zshnr zshnr requested a review from samouri November 2, 2021 17:48
@samouri samouri merged commit bdc9729 into ampproject:main Nov 2, 2021
@zshnr zshnr deleted the fix-csp-check-sandboxed-mode branch November 2, 2021 19:35
@samouri
Copy link
Member

samouri commented Nov 2, 2021

Congratulations @zshnr on your first AMP PR 🎉

@zshnr
Copy link
Contributor Author

zshnr commented Nov 2, 2021

Thanks for all your help @samouri !! 😄

rileyajones pushed a commit to rileyajones/amphtml that referenced this pull request Nov 4, 2021
…d mode (ampproject#36618)

* skips sha384 check for remote scripts in sandboxed mode

* update unit tests for amp-script

* lint fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Amp-script in sandboxed mode still asks for CSP hash if src url is cross origin
3 participants