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.

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

Handle null policyValue in Create a Trusted Type #527

Merged
merged 1 commit into from
Jun 19, 2024
Merged

Handle null policyValue in Create a Trusted Type #527

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions spec/index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -981,6 +981,7 @@ a string |value| and a list |arguments|, execute the following steps:
1. Let |policyValue| be the result of executing [$Get Trusted Type policy value$] with the same arguments as this algorithm and additionally true as |throwIfMissing|.
1. If the algorithm threw an error, rethrow the error and abort the following steps.
1. Let |dataString| be the result of stringifying |policyValue|.
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if stringifying is even defined to mean a specific thing, this spec in general probably needs checking over for this sort of thing but leave that for #472

1. If |policyValue| is null or undefined, set |dataString| to the empty string.
Copy link

@petervanderbeken petervanderbeken Jun 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand how this solves the issue? Stringifying policyValue would still happen with policyValue potentially being null?

Seems to me like this should be

1. If |policyValue| is null or undefined, set |dataString| to the empty string. Otherwise, let |dataString| be the result of stringifying |policyValue|.

Copy link
Member Author

@lukewarlow lukewarlow Jun 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It stringifies it which would set dataString to "null" (presumably) and then dataString is set to an emptyString in the case the original value was null (or undefined). Just feels cleaner to read imo.

Copy link

Choose a reason for hiding this comment

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

Looking more into it, I find it quite confusing. I'm going to assume that Get Trusted Type policy value actually calls https://webidl.spec.whatwg.org/#invoke-a-callback-function, since it's technically invoking a WebIDL callback, not an ECMAScript function. After invoking the callback we have a nullable DOMString WebIDL value. Stringifying a value of that type is not specified anywhere afaik? Having undefined mentioned also doesn't make sense for a value of that type.

Isn't it enough to then just replace these 2 steps with:

1. If |policyValue| is null, set |policyValue| to the empty string.

and replace dataString with policyValue in the following step?

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 we need to look more into the spec as a whole it's probably missing correct infra syntax in many places. This change was largely just to make it clear what to do in that specific case.

Cc @koto would you have any spare capacity to spend some time making the spec more correct from an editorial perspective?

Copy link
Member

Choose a reason for hiding this comment

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

Cc @koto would you have any spare capacity to spend some time making the spec more correct from an editorial perspective?

Sorry, I don't have enough capacity to help with this for now :/

1. Return a new instance of an interface with a type
name |trustedTypeName|, with its associated data value set to |dataString|.

Expand Down
Loading