trusted-types icon indicating copy to clipboard operation
trusted-types copied to clipboard

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

Open lukewarlow opened this issue 1 year ago • 13 comments
trafficstars

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 avatar Feb 29 '24 12:02 lukewarlow

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.

lukewarlow avatar Feb 29 '24 12:02 lukewarlow

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

koto avatar Feb 29 '24 14:02 koto

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 avatar Feb 29 '24 14:02 lukewarlow

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.

mbrodesser-Igalia avatar Mar 04 '24 09:03 mbrodesser-Igalia

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?

lukewarlow avatar Mar 04 '24 12:03 lukewarlow

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

mbrodesser-Igalia avatar Mar 04 '24 16:03 mbrodesser-Igalia

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

caridy avatar Mar 04 '24 17:03 caridy

@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 avatar Mar 04 '24 19:03 lukewarlow

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 avatar Mar 04 '24 19:03 lukewarlow

https://github.com/w3c/trusted-types/pull/465 is the relevant change to this spec and the tc39 proposal change is linked in it (already approved by nico)

lukewarlow avatar Mar 04 '24 19:03 lukewarlow

@koto @otherdaniel based on the discussions above would you be okay with https://github.com/tc39/proposal-dynamic-code-brand-checks/pull/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?

lukewarlow avatar Mar 06 '24 19:03 lukewarlow

I think it makes sense.

koto avatar Mar 07 '24 13:03 koto

Yes, looks good!

otherdaniel avatar Mar 07 '24 13:03 otherdaniel

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

lukewarlow avatar Mar 14 '24 12:03 lukewarlow

This has been done inside of the WebKit implementation and the PRs upstreaming it have this baked in.

If we wanted to revisit this it would require tc39 changes aswell as CSP changes.

lukewarlow avatar Jun 12 '24 13:06 lukewarlow

Based on discussions at the web engines hackfest, @nicolo-ribaudo it might be useful if you could find the historical push back against being able to change the value. I know this time around there might not have been an explicit discussion about it when presenting but my understanding is that this was based on historical push back when the proposal was previously presented?

If you can't find the historical reasoning no worries it just might be useful to have them collected somewhere.

lukewarlow avatar Jun 12 '24 13:06 lukewarlow