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

EnsureCSPDoesNotBlockStringCompilation: Explain why we need to check TrustedScript's data (and add tests) #697

Open
fred-wang opened this issue Dec 4, 2024 · 0 comments

Comments

@fred-wang
Copy link
Contributor

fred-wang commented Dec 4, 2024

cc @koto @lukewarlow

This was initially open in the wrong repo (sorry about that): web-platform-tests/wpt#49371

Anyway, the summary of the conversation is:

  1. These is to cover the case of TrustedScript with a forged toString() i.e. Object.assign(policy.createScript("safe"), { toString: () => "unsafe" }) which would lead to mismatch between the stringified arguments and the associated trusted data of the arguments.

  2. Tests have been added to cover them, see [Gecko Bug 1934373] Add more tests for EnsureCSPDoesNotBlockStringCompilation. web-platform-tests/wpt#49449

  3. These checks are actually not needed for eval. More precisely, PerformEval algorithm guarantees that parameterArgs/Strings are empty and that bodyArg is either a string or an Object. Moreover, the HTML implementation of HostGetCodeForEval guarantees that an Object argument can only be a TrustedScript object with the associated data matching bodyString. So the check really reduces to isTrusted = true if bodyArg is an Object and false otherwise (if it is a string), which can be an interesting perf improvements (at least WebKit and Firefox's patches don't do the complete checks)

So what remains to do for this issue is a non-normative note explaining the rationale for this check (point 1) so readers understand where it is coming from.

About point 3, I don't know, maybe this can be a note with a suggestion in a note or changing the algo to describe simplified steps for eval, but maybe it's a separate issue anyway.

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

No branches or pull requests

1 participant