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

Can we drop the default policy value changing from Eval, new Function() (and other usages of the dynamic code brand checks proposal)? #461

Open
lukewarlow opened this issue Feb 29, 2024 · 14 comments
Milestone

Comments

@lukewarlow
Copy link
Member

Following on from discussions recently with @caridy it's possible we could avoid the default policy fallback for eval (and Function() etc).

This simplifies the TT spec slightly (not much though), it also simplifies implementation, but the key thing is it removes a potentially contentious point from the Ecmascript changes.

This would be a breaking change from Chromiums behaviour but I'm doubtful it has much usage? Especially based on discussions in #458

cc @koto @otherdaniel

@lukewarlow
Copy link
Member Author

The exact specifics of what this could look like are up for discussion, I can see us calling the default policy and just make sure the return value is the same as what went in? I can see us not calling default policy at all and just blocking all raw strings and requiring trusted types.

The main bit that could be good to remove is the abiity to return a different value from the default policy and having that execute.

@koto
Copy link
Member

koto commented Feb 29, 2024

@otherdaniel can we measure when the default policy for TrustedScripts changes the value?

If not used, I think we could write is such that a value that is different from the input (for this particular case only) would be like if false was returned, and in Chrome have a deprecation mechanism for the current behaviour - that doesn't change the function signature and allows most assumed usages to stay as-is.

@lukewarlow
Copy link
Member Author

Yeah so a use counter for when the policy is invoked for eval or function and the return value is a string and that string is different from the string you enter.

@lukewarlow lukewarlow changed the title Can we drop the default policy fallback from Eval and company? Can we drop the default policy value changing from Eval and company? Feb 29, 2024
@mbrodesser-Igalia
Copy link
Collaborator

Following on from discussions recently with @caridy it's possible we could avoid the default policy fallback for eval (and Function() etc).

Is the intention to remove calling the default policy for eval and some other injection sinks, but not all other injection sinks? From a web-dev's perspective that seems inconsistent.

@lukewarlow
Copy link
Member Author

Is the intention to remove calling the default policy for eval and some other injection sinks, but not all other injection sinks? From a web-dev's perspective that seems inconsistent.

Not to remove but to remove its ability to change the value. And yes specifically only for eval etc.

While I agree it would be inconsistent, TT is inconsistent with eval in general already, objects currently aren't evaluated in a meaningful way. So I don't think it's super unreasonable?

@mbrodesser-Igalia
Copy link
Collaborator

Is the intention to remove calling the default policy for eval and some other injection sinks, but not all other injection sinks? From a web-dev's perspective that seems inconsistent.

Not to remove but to remove its ability to change the value. And yes specifically only for eval etc.

While I agree it would be inconsistent, TT is inconsistent with eval in general already, objects currently aren't evaluated in a meaningful way. So I don't think it's super unreasonable?

Yes, if there already is unavoidable inconsistency for eval, it seems reasonable.

Btw. it would be clearer if this issue explicitly listed eval's "company".

@caridy
Copy link

caridy commented Mar 4, 2024

I'm very supportive of this additional restriction for eval and Function. cc @nicolo-ribaudo

@lukewarlow lukewarlow changed the title Can we drop the default policy value changing from Eval and company? Can we drop the default policy value changing from Eval, new Function() (and other usages of the dynamic code brand checks proposal? Mar 4, 2024
@lukewarlow lukewarlow changed the title Can we drop the default policy value changing from Eval, new Function() (and other usages of the dynamic code brand checks proposal? Can we drop the default policy value changing from Eval, new Function() (and other usages of the dynamic code brand checks proposal)? Mar 4, 2024
@lukewarlow
Copy link
Member Author

@otherdaniel when you're able to add those use counters could you leave a comment here just to keep the context all in one place.

@lukewarlow
Copy link
Member Author

I think our best course of action is to assume we can make this change, and update the tc39 spec and this one accordingly.

The tc39 change is more likely to get merged without this change based on all discussions so far. Worst case if we need this ability for a compat reason we can always reapproach tc39 for the necessary discussions or just bypass them for the specific change necessary and handle it in TT land.

@lukewarlow
Copy link
Member Author

#465 is the relevant change to this spec and the tc39 proposal change is linked in it (already approved by nico)

@lukewarlow
Copy link
Member Author

@koto @otherdaniel based on the discussions above would you be okay with tc39/proposal-dynamic-code-brand-checks#12 being merged and us progressing the tc39 proposal on the assumption the use counters come back okay?

If down the line the use counters reveal that it would be a compat risk we can revisit what to do (go back to tc39 with more changes, monkeypatch from this spec, etc). But I think it makes sense to try and progress the tc39 proposal sooner rather than later?

@koto
Copy link
Member

koto commented Mar 7, 2024

I think it makes sense.

@otherdaniel
Copy link
Member

Yes, looks good!

@lukewarlow
Copy link
Member Author

@otherdaniel were you able to add the use counter for this yet?

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

5 participants