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

Update Trusted Types enforcement for document.write/writeln #10328

Merged
merged 1 commit into from
May 28, 2024
Merged

Update Trusted Types enforcement for document.write/writeln #10328

merged 1 commit into from
May 28, 2024

Conversation

lukewarlow
Copy link
Member

@lukewarlow lukewarlow commented May 7, 2024 •
edited by pr-preview bot
Loading

Update Trusted Types enforcement for document.write/writeln

This changes from using HTMLString to a TrustedHTML or DOMString union.

This also changes the timing of the default policy call.

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

/dom.html ( diff )
/dynamic-markup-insertion.html ( diff )
/infrastructure.html ( diff )

@lukewarlow
Copy link
Member Author

One thing to note is that the behaviour difference for default policy call is actually aligning with the shipping implementation (Chromium)

@lukewarlow lukewarlow mentioned this pull request May 7, 2024
Closed
lukewarlow
lukewarlow commented May 7, 2024
View reviewed changes
source Outdated Show resolved Hide resolved
annevk
annevk reviewed May 7, 2024
View reviewed changes
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
annevk
annevk reviewed May 7, 2024
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

All comments apply to both algorithms, though I've made some only on one of them. Apologies for the slight mess.

source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@lukewarlow
Copy link
Member Author

Thanks for the reviews so far hopefully it's looking like you expect now. Definitely cleaner than the first draft.

annevk
annevk reviewed May 7, 2024
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Yeah this looks good, thanks. Although now I wonder if we should have a shared operation as they are essentially copies? Maybe pass text to the document write steps and add an argument for a trailing LF? I guess @otherdaniel should agree to this as well at least.

@lukewarlow lukewarlow marked this pull request as ready for review May 7, 2024 15:13
@lukewarlow lukewarlow mentioned this pull request May 7, 2024
Merged
@otherdaniel
Copy link
Contributor

Thanks! We're happy with this, too.

@lukewarlow
Copy link
Member Author

lukewarlow commented May 8, 2024 •
edited
Loading

@annevk am I okay to say this has webkit implementor interest?

The webkit pr itself is being merged but I can do follow ups to change it. Mainly want it in to move away from the IDL attribute.

webkit-commit-queue pushed a commit to lukewarlow/WebKit that referenced this pull request May 8, 2024
@lukewarlow
Update implementation of TT enforcement for document.write(ln)
e84b70e 
https://bugs.webkit.org/show_bug.cgi?id=273819

Reviewed by Ryosuke Niwa.

This patch switches document.write and writeln to use a union IDL type and single call to default policy.

This brings it close to chromium behaviour.

See whatwg/html#10328

* LayoutTests/imported/w3c/web-platform-tests/trusted-types/Document-write-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/trusted-types/Document-write.html:
* LayoutTests/imported/w3c/web-platform-tests/trusted-types/block-string-assignment-to-Document-write-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/trusted-types/block-string-assignment-to-Document-write.html:
* Source/WebCore/dom/Document+HTML.idl:
* Source/WebCore/dom/Document.cpp:
(WebCore::Document::write):
(WebCore::Document::writeln):
* Source/WebCore/dom/Document.h:

Canonical link: https://commits.webkit.org/278501@main
@annevk
Copy link
Member

annevk commented May 8, 2024

I think the deduplication still needs to happen.

As for implementer interest, it's a bit tricky. WebKit hasn't established a position as of yet in part due to the many outstanding issues with Trusted Types. This refactoring on its own seems okay, but I would not want that to be taken as WebKit being okay with it in general.

@lukewarlow
Copy link
Member Author

I've refactored this to move things into the document write steps.

@lukewarlow lukewarlow requested a review from annevk May 9, 2024 13:38
@lukewarlow lukewarlow mentioned this pull request May 14, 2024
Merged
zcorpan
zcorpan reviewed May 15, 2024
View reviewed changes
source Show resolved Hide resolved
@lukewarlow lukewarlow mentioned this pull request May 28, 2024
Merged
5 tasks
@lukewarlow
Update Trusted Types enforcement for document.write/writeln
ae1f8f1 
This changes from using HTMLString to a TrustedHTML or DOMString union.

This also changes the timing of the default policy call.
@zcorpan
Copy link
Member

zcorpan commented May 28, 2024

No need for any change to MDN here as far as I can tell. Everything seems in order, so merging.

@zcorpan zcorpan merged commit feb379d into whatwg:main May 28, 2024
2 checks passed
webkit-commit-queue pushed a commit to lukewarlow/WebKit that referenced this pull request Jun 11, 2024
@lukewarlow
Update implementation of TT enforcement for document.write(ln)
cfe83d0 
https://bugs.webkit.org/show_bug.cgi?id=273819

Reviewed by Darin Adler.

This patch switches document.write and writeln to use a union IDL type and single call to default policy.

See whatwg/html#10328

* LayoutTests/imported/w3c/web-platform-tests/trusted-types/block-string-assignment-to-Document-write-expected.txt:
* Source/WebCore/dom/DOMImplementation.cpp:
(WebCore::DOMImplementation::createHTMLDocument):
* Source/WebCore/dom/Document+HTML.idl:
* Source/WebCore/dom/Document.cpp:
(WebCore::Document::write):
(WebCore::Document::writeln):
* Source/WebCore/dom/Document.h:
* Source/WebKitLegacy/mac/DOM/DOMHTMLDocument.mm:
(-[DOMHTMLDocument write:]):
(-[DOMHTMLDocument writeln:]):

Canonical link: https://commits.webkit.org/279904@main
mnutt pushed a commit to movableink/webkit that referenced this pull request Aug 27, 2024
@lukewarlow @mnutt
Update implementation of TT enforcement for document.write(ln)
1e4e17c 
https://bugs.webkit.org/show_bug.cgi?id=273819

Reviewed by Darin Adler.

This patch switches document.write and writeln to use a union IDL type and single call to default policy.

See whatwg/html#10328

* LayoutTests/imported/w3c/web-platform-tests/trusted-types/block-string-assignment-to-Document-write-expected.txt:
* Source/WebCore/dom/DOMImplementation.cpp:
(WebCore::DOMImplementation::createHTMLDocument):
* Source/WebCore/dom/Document+HTML.idl:
* Source/WebCore/dom/Document.cpp:
(WebCore::Document::write):
(WebCore::Document::writeln):
* Source/WebCore/dom/Document.h:
* Source/WebKitLegacy/mac/DOM/DOMHTMLDocument.mm:
(-[DOMHTMLDocument write:]):
(-[DOMHTMLDocument writeln:]):

Canonical link: https://commits.webkit.org/279904@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zcorpan zcorpan zcorpan approved these changes

@annevk annevk Awaiting requested review from annevk

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lukewarlow @otherdaniel @annevk @zcorpan