Skip to content

Add {{StringContext}} extended attribute #1392

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.

Sign up for GitHub

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

Closed
wants to merge 13 commits into from
Closed

Add {{StringContext}} extended attribute #1392

Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f9fbdc2
Added a definition of {{StringContext}} extended attribute for
koto Feb 10, 2020
bf5bea1
Mentioned that {{StringContext}} may apply only to regular operation …
koto Feb 14, 2020
c31e59c
Fix reference errors
lukewarlow Mar 11, 2024
550b967
Copy String context handling to attribute setter steps.
lukewarlow Mar 12, 2024
331a76a
Copy String context handling to js overload resolution algorithm
lukewarlow Mar 12, 2024
29b25b6
Fix attribute setter
lukewarlow Mar 12, 2024
831c461
Remove StringContext handling from DOMString conversion algorithm
lukewarlow Mar 12, 2024
49a7361
Minor changes
lukewarlow Mar 12, 2024
000abfe
Change String context indentifiers
lukewarlow Mar 12, 2024
86a6e50
Remove related construct
lukewarlow Mar 12, 2024
27d5b5e
Add comma
lukewarlow Mar 13, 2024
808cef7
Small fixups
lukewarlow Mar 14, 2024
36ddffb
Add paragraph explaining that StringContext can't be used on only som…
lukewarlow Mar 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 77 additions & 8 deletions index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6353,8 +6353,10 @@ The following extended attributes are <dfn for="extended attributes">applicable
[{{AllowResizable}}],
[{{AllowShared}}],
[{{Clamp}}],
[{{EnforceRange}}], and
[{{LegacyNullToEmptyString}}].
[{{EnforceRange}}],
[{{LegacyNullToEmptyString}}], and
[{{StringContext}}].


<div algorithm>
The <dfn for="IDL type" lt="extended attribute associated with|extended attributes associated with">extended attributes associated with</dfn>
Expand Down Expand Up @@ -10259,6 +10261,36 @@ that does specify [{{SecureContext}}].
</pre>
</div>

<h4 id="StringContext" extended-attribute lt="StringContext">[StringContext]</h4>

If the [{{StringContext}}] [=extended attribute=] appears on {{DOMString}} or {{USVString}}, it
modifies how the value is converted to the IDL type, causing additional value validation to
adhere to the context the string is used in.

The [{{StringContext}}] extended attribute must [=takes an identifier|take an identifier=]. The [=identifier=]
must be one of "<code>TrustedHTML</code>", "<code>TrustedScript</code>", and "<code>TrustedScriptURL</code>".

[{{StringContext}}] extended attribute may only annotate a type of a [=regular attribute=] or
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=]
Copy link

@ghost ghost Mar 26, 2024 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

CC @koto ^

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

the [{{StringContext}}] extended attribute.

<div class="example" id="example-7b5c2e9f">

In the following [=IDL fragment=],
a [=variadic=] [=operation=] is declared
that uses the [{{StringContext}}] [=extended attribute=]
on all its arguments:

<pre highlight="webidl">
interface Document {
void write([StringContext=TrustedHTML] DOMString... text);
};
</pre>
</div>


<h4 id="Unscopable" extended-attribute lt="Unscopable">[Unscopable]</h4>

Expand Down Expand Up @@ -11056,6 +11088,21 @@ allowed. The security check takes the following three inputs:

Note: The HTML Standard defines how a security check is performed. [[!HTML]]

Certain algorithms are defined to
<dfn id="dfn-validate-the-string-in-context" export>validate the string in context</dfn> on a given
value. This check is used to determine whether a given value
is appropriate for its {{StringContext}}. This validation takes the following four inputs:

1. the [=platform object=] on
which the operation invocation or attribute access is being done,
1. the value to validate,
1. the {{StringContext}} [=identifier=], and
1. the [=identifier=] of the operation or attribute.

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]]
Copy link
Member

Choose a reason for hiding this comment

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

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

  1. It's not clear it always ends up throwing a TypeError. Doesn't this involve userland for some bits?
  2. 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. 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.
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

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

doesn't it really return a string?

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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



<h3 id="js-overloads" oldids="es-overloads">Overload resolution algorithm</h3>

Expand Down Expand Up @@ -11092,8 +11139,16 @@ Note: The HTML Standard defines how a security check is performed. [[!HTML]]
1. If the argument at index |i| is declared with a [=optional argument/default value=],
then append to |values| that default value.
1. Otherwise, append to |values| the special value “missing”.
1. Otherwise, append to |values| the result of [=converted to an IDL value|converting=]
|V| to IDL type |type|.
1. Otherwise:
1. If |type| is an IDL type [=extended attribute associated with|associated with=] the
[{{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|.
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


Note: That algorithm may [=JavaScript/throw=] a <l spec=ecmascript>{{TypeError}}</l>.
1. Append to |values| the result of [=converted to an IDL value|converting=]
|V| to IDL type |type|.
1. Set |i| to |i| + 1.
1. If |i| = |d|, then:
1. Let |V| be |args|[|i|].
Expand Down Expand Up @@ -11292,8 +11347,16 @@ Note: The HTML Standard defines how a security check is performed. [[!HTML]]
1. If the argument at index |i| is declared with a [=optional argument/default value=],
then append to |values| that default value.
1. Otherwise, append to |values| the special value “missing”.
1. Otherwise, append to |values| the result of
[=converted to an IDL value|converting=] |V| to IDL type |type|.
1. Otherwise:
1. If |type| is an IDL type [=extended attribute associated with|associated with=] the
[{{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 |callable|.

Note: That algorithm may [=JavaScript/throw=] a <l spec=ecmascript>{{TypeError}}</l>.
1. Append to |values| the result of [=converted to an IDL value|converting=]
|V| to IDL type |type|.
1. Set |i| to |i| + 1.
1. While |i| is less than the number of arguments |callable| is declared to take:
1. If |callable|'s argument at index |i| is declared with a [=optional argument/default value=],
Expand Down Expand Up @@ -11982,8 +12045,14 @@ in which case they are exposed on every object that [=implements=] the interface

<dt>Otherwise</dt>
<dd>
|idlValue| is the result of [=converted to an IDL value|converting=] |V| to an
IDL value of |attribute|'s type.
1. If |attribute|'s type is [=extended attribute associated with|associated with=] the
[{{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 |id|.

Note: That algorithm may [=JavaScript/throw=] a <l spec=ecmascript>{{TypeError}}</l>.
1. |idlValue| is the result of [=converted to an IDL value|converting=] |V| to an
IDL value of |attribute|'s type.
</dd>
</dl>

Expand Down