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

Jump to bottom

Add attribute validate steps. #809

Closed
wants to merge 4 commits into from
Closed

Add attribute validate steps. #809

wants to merge 4 commits into from

Conversation

koto
Copy link

@koto koto commented Dec 10, 2019 •
edited by pr-preview bot
Loading

When handling attribute change, validate the value before setting it.

Attribute append now also takes a value, and sets it only after handling the changes (this should be side-effects free, as no callbacks in handle attribute change get passed the attribute node - whose value is not yet set).

See #789 for the background. This PR doesn't allow the validate steps to change the value, only to possibly reject setting it.

💥 Error: 500 Internal Server Error 💥

PR Preview failed to build. (Last tried on Jan 15, 2021, 7:33 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 CSS Spec Preprocessor - CSS Spec Preprocessor is the web service used to build Bikeshed specs.

🔗 Related URL

<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>500 Internal Server Error</title>
</head><body>
<h1>Internal Server Error</h1>
<p>The server encountered an internal error or
misconfiguration and was unable to complete
your request.</p>
<p>Please contact the server administrator at 
 [no address given] to inform them of the time this error occurred,
 and the actions you performed just before this error.</p>
<p>More information about this error may be available
in the server error log.</p>
<hr>
<address>Apache/2.4.10 (Debian) Server at api.csswg.org Port 443</address>
</body></html>

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@koto koto mentioned this pull request Dec 10, 2019
Open
@koto
Copy link
Author

koto commented Dec 12, 2019

Rebased now that #805 is merged.

@koto koto changed the title Add attribute validate steps on top of #805. Add attribute validate steps. Dec 12, 2019
@koto koto mentioned this pull request Dec 12, 2019
Closed
@koto koto marked this pull request as ready for review December 12, 2019 15:06
@koto
Copy link
Author

koto commented Jan 2, 2020

From my pov this is ready for a review, see discussion in #789.

@koto
Copy link
Author

koto commented Jan 17, 2020

Friendly ping, this seems to be the most efficient way we can prevent attribute node bypasses in Trusted Types by rejecting the value without potentially calling the default policy on all attr mutations.

@annevk annevk added needs implementer interest Moving the issue forward requires implementers to express interest needs tests Moving the issue forward requires someone to write tests labels Feb 11, 2020
@annevk
Copy link
Member

annevk commented Feb 11, 2020

I think this makes sense, but we should add a note explaining this setup somewhere close to where it's relevant.

This was referenced Mar 2, 2020
Closed
Closed
Base automatically changed from master to main January 15, 2021 07:32
@lukewarlow
Copy link
Member

This should have implementor interest now given Mozilla's positive standards position on trusted types as a whole.

@annevk
Copy link
Member

annevk commented Jan 15, 2024 •
edited
Loading

I wonder if we want an arbitrary extension point for this or just call into Trusted Types directly. With an arbitrary extension point we have the risk of running into ordering issues so we tend to avoid those these days, even though the DOM has some precedent for it (mainly historical).

@annevk annevk removed the needs implementer interest Moving the issue forward requires implementers to express interest label Jan 15, 2024
@koto
Copy link
Author

koto commented Jan 15, 2024

I wonder if we want an arbitrary extension point for this or just call into Trusted Types directly.

I don't have a strong preference here. As long as it is possible to guard attribute mutations done by the DOM APIs, the actual algorithms can be defined in either spec. This PR was the only necessary integration point of TT with DOM.

What might be problematic is not technical, i.e. can DOM call out to specs that are currently at FPWD? Waiting until CR is probably not very useful to the authors here, especially with shipped implementations.

@annevk
Copy link
Member

annevk commented Jan 15, 2024

That's not really an issue, especially for integration points. And in fact, we tend to prefer referencing the latest editor's draft.

@domenic
Copy link
Member

domenic commented Jan 15, 2024

Definite +1 for a direct call out.

koto added a commit to koto/dom that referenced this pull request Jan 23, 2024
@koto
Draft integration with Trusted Types, take 2.
4585427 
See w3c/trusted-types#418 and whatwg#789.
Supercedes PR whatwg#809.
@koto koto mentioned this pull request Jan 23, 2024
Closed
5 tasks
@koto
Copy link
Author

koto commented Jan 23, 2024

#1247 and w3c/trusted-types#418 supercede this one. Unfortunately handling the attr mutation in DOM happens now after setting the new value, so I had to add an explicit new step to the algorithms.

koto added a commit to koto/dom that referenced this pull request Feb 26, 2024
@koto
Draft integration with Trusted Types, take 2.
5bcc0af 
See w3c/trusted-types#418 and whatwg#789.
Supercedes PR whatwg#809.
@lukewarlow lukewarlow closed this Mar 13, 2024
@lukewarlow lukewarlow mentioned this pull request Mar 27, 2024
Open
5 tasks
lukewarlow pushed a commit to lukewarlow/dom that referenced this pull request Apr 11, 2024
@koto @lukewarlow
Draft integration with Trusted Types, take 2.
10729d5 
See w3c/trusted-types#418 and whatwg#789.
Supercedes PR whatwg#809.
lukewarlow pushed a commit to lukewarlow/dom that referenced this pull request Apr 22, 2024
@koto @lukewarlow
Draft integration with Trusted Types, take 2.
321883a 
See w3c/trusted-types#418 and whatwg#789.
Supercedes PR whatwg#809.
lukewarlow pushed a commit to lukewarlow/dom that referenced this pull request May 16, 2024
@koto @lukewarlow
Draft integration with Trusted Types, take 2.
6d4b27d 
See w3c/trusted-types#418 and whatwg#789.
Supercedes PR whatwg#809.
lukewarlow pushed a commit to lukewarlow/dom that referenced this pull request Aug 13, 2024
@koto @lukewarlow
Draft integration with Trusted Types, take 2.
184fbb3 
See w3c/trusted-types#418 and whatwg#789.
Supercedes PR whatwg#809.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
needs tests Moving the issue forward requires someone to write tests
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@koto @annevk @lukewarlow @domenic