Add `{{StringContext}}` extended attribute #1392

lukewarlow · 2024-03-11T18:15:30Z

Add a {{StringContext}} extended attribute that can be defined on DOMString and USVString.

This is to hook up the Trusted Types validation during the ES->IDL type
conversion to avoid issues with its default policy.

See w3c/trusted-types#248,
w3c/trusted-types#176

The actual logic of the string validation calls into HTML, and will be something similar to https://w3c.github.io/trusted-types/dist/spec/#html-validate-the-string-in-context.

At least two implementers are interested (and none opposed):
- Chromium
- Gecko
Tests are written and can be reviewed and commented upon at:
- …
Implementation bugs are filed:
- Chromium: Shipping
- Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1508286 - General trusted types issue
- WebKit: https://bugs.webkit.org/show_bug.cgi?id=266630 - General Trusted Types issue
- Deno: …
- Node.js: …
- webidl2.js: …
- widlparser: …
MDN issue is filed: Trusted types itself is documented and the specifics of this IDL attribute probably aren't relevant.
The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

(DOM|USV)String. This is to hook up the Trusted Types validation during the ES->IDL type conversion to avoid funky issues with its default policy. See w3c/trusted-types#248, w3c/trusted-types#176

…and regular argument types explicitly.

lukewarlow · 2024-03-11T18:16:05Z

Fork of #841 will address review comments here

index.bs

lukewarlow · 2024-03-12T15:34:17Z

index.bs

+                [{{StringContext}}] extended attribute, then set |V| to the result of performing
+                [=validate the string in context=], passing [=this=], |V|, the {{StringContext}}
+                extended attribute [=identifier=], and the [=identifier=]
+                of the [=operation=] or [=extended attribute=] of the first entry in |S|.


Because we only need the identifier which is the same for all overloads we don't actually need to worry about which entry in the overload set is used.

But what if that overload doesn't have a StringContext annotated type?

I guess what this means is that you cannot meaningfully have overloaded StringContext types (or at least not all variants one might imagine to be possible). That seems reasonable, but I wonder if we need to make that more explicit in some way.

Yeah based on my understanding you can't really have overloads. Like where you need the type to be different in a sub class we just switch back to using a union type and handling it manually rather than using this attribute. cc @koto in case I'm wrong.

Happy to add a note or something to make it clearer, not really sure what it should say though?

I guess I would add a paragraph to where StringContext is defined that outlines the limitations.

I've added a paragraph hopefully it makes sense. Happy to adjust it if you'd like.

annevk

I think this looks reasonable, although I think we probably should note the overload limitations more explicitly.

@EdgarChen @petervanderbeken @yuki3 any of you want to take a look as well?

index.bs

annevk · 2024-03-13T12:09:16Z

index.bs

+                [{{StringContext}}] extended attribute, then set |V| to the result of performing
+                [=validate the string in context=], passing [=this=], |V|, the {{StringContext}}
+                extended attribute [=identifier=], and the [=identifier=]
+                of the [=operation=] or [=extended attribute=] of the first entry in |S|.


But what if that overload doesn't have a StringContext annotated type?

I guess what this means is that you cannot meaningfully have overloaded StringContext types (or at least not all variants one might imagine to be possible). That seems reasonable, but I wonder if we need to make that more explicit in some way.

yuki3 · 2024-03-14T07:31:03Z

I think this looks reasonable, although I think we probably should note the overload limitations more explicitly.

@EdgarChen @petervanderbeken @yuki3 any of you want to take a look as well?

Looks good to me, too.

index.bs

…e overloads of an overloaded operation.

yuki3 · 2024-03-15T04:44:39Z

cc @natechapin as he's taking over the Blink-V8 bindings work (including interop work) from me.

index.bs

mbrodesser-Igalia · 2024-03-26T10:40:12Z

index.bs

+a [=regular operation=] argument. A type annotated with the [{{StringContext}}]
+extended attribute must not appear in a [=read only=] attribute.
+
+A type that is not {{DOMString}} or {{USVString}} must not be [=extended attributes associated with|associated with=]


In the future nullable types might require support too. At least Chromium doesn't need them now.

https://w3c.github.io/trusted-types/dist/spec/#setting-slot-values - does require use of ScriptString? so this might need updating to account for it.

I've updated the TT spec to just use a union type there rather than the extended attribute, because that solves dealing with nullable types.

https://w3c.github.io/trusted-types/dist/spec/#setting-slot-values - does require use of ScriptString? so this might need updating to account for it.

Btw, Chromium doesn't reflect the spec here (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_script_element.idl;l=24-52;drc=d1cf902be6fc9ae8654fe5a6c466dfb51f782197).

Yeah that section specifically is going through a rewrite, Chrome doesn't match it currently and won't in its new form either so that is one change to the chromium implementation that will be needed. (Very minor web facing changes that shouldn't impact anyone)

I've updated the TT spec to just use a union type there rather than the extended attribute, because that solves dealing with nullable types.

Where was the TT spec updated? This patch still adds StringContext extended attribute.

w3c/trusted-types#484 - in this PR the textContent one specifically was updated to use a union. I've done a follow up commit that changes them all to unions (which I can easily revert if we decide against that)

annevk

I think this is mostly good now, but on closer inspection I think the "validate" aspect needs a bit more flushing out.

index.bs

annevk · 2024-04-03T14:27:19Z

index.bs

+
+The algorithm returns an ECMAScript value, or [=JavaScript/throws=] a <l spec=ecmascript>{{TypeError}}</l>.
+
+Note: The HTML Standard defines how the validation is performed. [[!HTML]]


I looked into this a bit more and this description appears insufficient.

It's not clear it always ends up throwing a TypeError. Doesn't this involve userland for some bits?

It seems this algorithm might also end up reporting violations per step 6.1 of https://w3c.github.io/trusted-types/dist/spec/#abstract-opdef-get-trusted-type-compliant-string.

I don't think "validate" is a sufficiently clear term given the implications. "Validate" makes me expect a side-effect-free-boolean-returning operation, not something that can result in network traffic (and events?).

Apart from the above, we'll need some test coverage to ensure that this is invoked at the correct time relative to other exceptions that can be thrown. This will be observable if this can throw non-TypeError exceptions, but even if this always throws a TypeError (and we ignore the message field) it's observable through network traffic.

It's not clear it always ends up throwing a TypeError. Doesn't this involve userland for some bits?

Yeah you're correct this can throw any error type.

It seems this algorithm might also end up reporting violations per step 6.1

Also correct it can report CSP violations.

I don't think "validate" is a sufficiently clear term given the implications. "Validate" makes me expect a side-effect-free-boolean-returning operation, not something that can result in network traffic (and events?).

Maybe verify instead? I'm not sure what prior art there is for this sort of thing.

Apart from the above, we'll need some test coverage to ensure that this is invoked at the correct time relative to other exceptions that can be thrown. This will be observable if this can throw non-TypeError exceptions, but even if this always throws a TypeError (and we ignore the message field) it's observable through network traffic.

Additional tests to ensure timing wrt to other errors makes sense here. cc @mbrodesser-Igalia

As for the network traffic aspect, I'm not familiar with the details of how CSP violation reports work and whether their timing is an observable concern.

The string can also be modified, right? It says it returns an ECMAScript value, but doesn't it really return a string?

I think "verify" is okay. I guess in theory we want this to be able to be used for non-Trusted Types purposes.

There's various things that matter with respect to timing:

Does it all happen at the correct point in time relative to other exceptions that may be thrown. I.e., do implementations handle the checks in the correct order.

What further side effects may arise from this check happening. It seems that https://w3c.github.io/webappsec-csp/#report-violation always queues a task so we don't have to worry about that for CSP. And I think we don't have to worry in the general case because this happens before any specification algorithms are involved. Although if we add more of these utilities around the edges of IDL we'll run into conflicts at some point of course.

doesn't it really return a string?

Doesn't it return the argument directly in step 2?

It overwrites it first in 1.2 as far as I can tell. At least judging from https://w3c.github.io/trusted-types/dist/spec/#html-validate-the-string-in-context.

If you don't expect the code path to be used then you need to use Assert.

But given @petervanderbeken's comments I'm starting to wonder if having this as an extended attribute is a good after all. I guess I'm back at #841 (comment) where I wonder if we should put most of the complexity in the specification algorithms.

cc @domenic @koto

Also cc @mbrodesser-Igalia as it will impact his implementation in Gecko

After discussion in the matrix I'm on board with the idea of just using union types and updating algorithms, and closing this PR out

@koto or @otherdaniel do you forsee chrome having any issues with this proposed change? It'd be good to get the specs and webkit implementation.

Tldr drop the IDL extended attribute and just use unions everywhere we need enforcement (updating the call site directly).

If it helps the discussion I've got a draft commit to update the WebKIt implementation to use union types instead of IDL magic. This doesn't account for DOM timers (webkit impl is different to spec). But otherwise replaces all existing usages of IDL magic.

https://github.com/webkit/webkit/compare/lukewarlow:trusted-types-use-union-over-attribute

petervanderbeken · 2024-04-04T11:45:00Z

index.bs

+the [{{StringContext}}] extended attribute.
+
+The [{{StringContext}}] extended attribute must not be used on only some of the [=overloaded|overloads=] of an
+[=overloaded=] [=operation=].


I find this sentence a bit weird, because it's used on arguments, not on the operation itself. Does this mean that in the case of overloads, either all DOMString/USVString arguments need to be annotated or none? Or just that if the overloads have a DOMString/USVString argument in the same position it should be annotated the same way in all overloads?

Off the top of my head I think it's that if they're in the same position they have to always or never be annotated. It might also be the case that you can't have an overload where it's an annotated string in one branch, and a different type entirely in another.

The attribute has a side effect and that means these limitations have to be in place.

I'll be honest I'm not super familar with the IDL overloading so I might be missing the point entirely.

@annevk can probably explain it better.

Happy to reword this paragraph to make more sense :)

petervanderbeken · 2024-04-04T12:37:00Z

I do think that we need to make [StringContext]-annotated strings not distinguishable from object, interface-like, callback function, dictionary-like and sequence-like. So we probably need to add another letter in the table, and use it in the string types row to explain that these are only distinguishable if the string type doesn't have a [StringContext] extended attribute.

lukewarlow · 2024-04-15T12:05:37Z

Based on discussions here, in Matrix and a video call we had with Koto and Daniel I'm going to go ahead and close this PR. If Daniel discovers a reason why the observable timing differences between IDL and call site aren't web compatible we can revisit this.

koto and others added 3 commits March 11, 2024 18:13

Added a definition of {{StringContext}} extended attribute for

f9fbdc2

(DOM|USV)String. This is to hook up the Trusted Types validation during the ES->IDL type conversion to avoid funky issues with its default policy. See w3c/trusted-types#248, w3c/trusted-types#176

Mentioned that {{StringContext}} may apply only to regular operation …

bf5bea1

…and regular argument types explicitly.

Fix reference errors

c31e59c

lukewarlow added 5 commits March 12, 2024 11:53

Copy String context handling to attribute setter steps.

550b967

Copy String context handling to js overload resolution algorithm

331a76a

Fix attribute setter

29b25b6

Remove StringContext handling from DOMString conversion algorithm

831c461

Minor changes

49a7361

lukewarlow force-pushed the string-context-xattr branch from 184e827 to 49a7361 Compare March 12, 2024 12:37

lukewarlow changed the title ~~Add StringContext attribute~~ Add `{{StringContext attribute Mar 12, 2024

lukewarlow changed the title ~~Add `{{StringContext attribute~~ Add {{StringContext}} extended attribute Mar 12, 2024

Change String context indentifiers

000abfe

lukewarlow requested review from Ms2ger, domenic and annevk March 12, 2024 13:13

annevk reviewed Mar 12, 2024

View reviewed changes

index.bs Show resolved Hide resolved

Remove related construct

86a6e50

lukewarlow commented Mar 12, 2024

View reviewed changes

lukewarlow requested a review from annevk March 12, 2024 15:34

lukewarlow marked this pull request as ready for review March 13, 2024 11:42

annevk reviewed Mar 13, 2024

View reviewed changes

Add comma

27d5b5e

This was referenced Mar 13, 2024

Replace WebIDL section with a link to new PR w3c/trusted-types#475

Merged

[Meta] Upstream changes w3c/trusted-types#476

Open

Ms2ger reviewed Mar 14, 2024

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

lukewarlow added 2 commits March 14, 2024 12:50

Small fixups

808cef7

Add paragraph explaining that StringContext can't be used on only som…

36ddffb

…e overloads of an overloaded operation.

whatwg deleted a comment from Jhonatanbb Mar 15, 2024

mbrodesser-Igalia reviewed Mar 18, 2024

View reviewed changes

index.bs Show resolved Hide resolved

mbrodesser-Igalia mentioned this pull request Mar 21, 2024

"Validate the string in context" takes any value and calls "Get Trusted Type compliant string" which requires a TrustedType or a string w3c/trusted-types#488

Closed

mbrodesser-Igalia reviewed Mar 26, 2024

View reviewed changes

annevk reviewed Apr 3, 2024

View reviewed changes

petervanderbeken reviewed Apr 4, 2024

View reviewed changes

lukewarlow closed this Apr 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `{{StringContext}}` extended attribute #1392

Add `{{StringContext}}` extended attribute #1392

lukewarlow commented Mar 11, 2024 •

edited by pr-preview bot

Loading

lukewarlow commented Mar 11, 2024

lukewarlow Mar 12, 2024

annevk Mar 13, 2024

lukewarlow Mar 13, 2024

annevk Mar 14, 2024

lukewarlow Mar 14, 2024

annevk left a comment

annevk Mar 13, 2024

yuki3 commented Mar 14, 2024

yuki3 commented Mar 15, 2024

mbrodesser-Igalia Mar 26, 2024 •

edited

Loading

lukewarlow Mar 28, 2024

lukewarlow Apr 4, 2024

mbrodesser-Igalia Apr 8, 2024

mbrodesser-Igalia Apr 8, 2024

lukewarlow Apr 8, 2024

mbrodesser-Igalia Apr 8, 2024

lukewarlow Apr 8, 2024

annevk left a comment

annevk Apr 3, 2024

lukewarlow Apr 3, 2024

annevk Apr 4, 2024

Ms2ger Apr 4, 2024

annevk Apr 4, 2024

annevk Apr 4, 2024

lukewarlow Apr 4, 2024

lukewarlow Apr 4, 2024

lukewarlow Apr 8, 2024

lukewarlow Apr 8, 2024

petervanderbeken Apr 4, 2024

lukewarlow Apr 4, 2024

petervanderbeken commented Apr 4, 2024

lukewarlow commented Apr 15, 2024


		The algorithm returns an ECMAScript value, or [=JavaScript/throws=] a <l spec=ecmascript>{{TypeError}}</l>.

		Note: The HTML Standard defines how the validation is performed. [[!HTML]]

Add {{StringContext}} extended attribute #1392

Add {{StringContext}} extended attribute #1392

Conversation

lukewarlow commented Mar 11, 2024 • edited by pr-preview bot Loading

lukewarlow commented Mar 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuki3 commented Mar 14, 2024

yuki3 commented Mar 15, 2024

mbrodesser-Igalia Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petervanderbeken commented Apr 4, 2024

lukewarlow commented Apr 15, 2024

Add `{{StringContext}}` extended attribute #1392

Add `{{StringContext}}` extended attribute #1392

lukewarlow commented Mar 11, 2024 •

edited by pr-preview bot

Loading

mbrodesser-Igalia Mar 26, 2024 •

edited

Loading