Implement enforcement of require-trusted-types-for CSP directive
#23412
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
EWS run on previous version of this PR (hash 0ddfb1a) |
lukewarlow
force-pushed
the
trusted-types-require-trusted-types-for-csp-violation
branch
from
0ddfb1a to
a87a721
Compare
lukewarlow
added
the
WebCore Misc.
Collaborator
|
EWS run on previous version of this PR (hash a87a721) |
lukewarlow
commented
Jan 29, 2024
lukewarlow
commented
Jan 29, 2024
lukewarlow
commented
Jan 29, 2024
webkit-ews-buildbot
added
the
merging-blocked
lukewarlow
commented
Jan 29, 2024
lukewarlow
commented
Jan 29, 2024
lukewarlow
commented
Jan 29, 2024
lukewarlow
force-pushed
the
trusted-types-require-trusted-types-for-csp-violation
branch
from
a87a721 to
3291302
Compare
Collaborator
|
EWS run on previous version of this PR (hash 3291302) |
lukewarlow
force-pushed
the
trusted-types-require-trusted-types-for-csp-violation
branch
from
3291302 to
ad81fce
Compare
Collaborator
|
EWS run on previous version of this PR (hash ad81fce) |
lukewarlow
force-pushed
the
trusted-types-require-trusted-types-for-csp-violation
branch
from
ad81fce to
4fa428e
Compare
Collaborator
|
EWS run on previous version of this PR (hash 4fa428e) |
lukewarlow
removed
the
merging-blocked
lukewarlow
changed the title
lukewarlow
force-pushed
the
trusted-types-require-trusted-types-for-csp-violation
branch
from
4fa428e to
f953c9b
Compare
Collaborator
|
EWS run on current version of this PR (hash f953c9b) |
🧪 bindings
Collaborator
|
EWS run on previous version of this PR (hash f953c9b)
|
lukewarlow
changed the title
require-trusted-types-for CSP directive
lukewarlow
changed the title
require-trusted-types-for CSP directive
lukewarlow
force-pushed
the
trusted-types-require-trusted-types-for-csp-violation
branch
from
f953c9b to
b797455
Compare
lukewarlow
changed the title
require-trusted-types-for CSP directive
Collaborator
|
EWS run on previous version of this PR (hash b797455)
|
webkit-ews-buildbot
added
the
merging-blocked
lukewarlow
force-pushed
the
trusted-types-require-trusted-types-for-csp-violation
branch
from
4117d54 to
9a795ba
Compare
Collaborator
|
EWS run on previous version of this PR (hash 9a795ba)
|
webkit-ews-buildbot
added
the
merging-blocked
lukewarlow
removed
the
merging-blocked
darinadler
requested changes
Feb 29, 2024
Member
darinadler
left a comment
darinadler
left a comment
There was a problem hiding this comment.
Did a first pass and found some things to improve.
| webkit.org/b/261849 imported/w3c/web-platform-tests/css/css-scroll-anchoring/start-edge-in-block-layout-direction.html [ Skip ] | ||
|
|
||
| # Trusted Types aren't implemented yet | ||
| # Trusted Types aren't fully implemented yet |
Member
There was a problem hiding this comment.
Why is it best to skip these tests instead of expecting failures? Normally we would only want to skip a test if it hangs or runs very slowly. It’s much better to just expect a failure.
Member
Author
There was a problem hiding this comment.
I think all of these are timeouts because they timeout in the failure case.
| ExceptionOr<Ref<TrustedHTML>> createHTML(const String& input, FixedVector<JSC::Strong<JSC::Unknown>>&&); | ||
| ExceptionOr<Ref<TrustedScript>> createScript(const String& input, FixedVector<JSC::Strong<JSC::Unknown>>&&); | ||
| ExceptionOr<Ref<TrustedScriptURL>> createScriptURL(const String& input, FixedVector<JSC::Strong<JSC::Unknown>>&&); | ||
| ExceptionOr<String> getPolicyValue(const TrustedType trustedTypeName, const String& input, FixedVector<JSC::Strong<JSC::Unknown>>&&, bool throwIfMissing = true); |
Member
There was a problem hiding this comment.
Should just be TrustedType not const TrustedType.
We try to avoid mystery bool arguments like this throwIfMissing one. Can we find a way to not have that, perhaps by having more than one function? I think one function can return null string if missing and the one that throws if missing can just call the other and post process it.
Member
Author
There was a problem hiding this comment.
The problem with returning nullString and post processing is that the callbacks themselves can return null and they shouldn't throw in this situation.
I'll make it a boolean enum instead so it's more explicit.
Member
There was a problem hiding this comment.
I think you can split this into two separate functions. Whether internally you want to use a bool argument to a common function or can find a way to have one call the other is something we can tinker with. I am almost certain you can have one call the other but I can propose a specific way once I see your revised patch. For example if you can distinguish the specific exception you can convert the exception to a null string.
Member
Author
There was a problem hiding this comment.
Doing something like in a GetPolicyValueForProcessDefault function could work? But it doesn't feel great, unfortunately the ExceptionCode alone isn't enough to distinguish because TypeErrors are thrown in other cases (such as from userland code)
auto policyValue = getPolicyValue(trustedTypeName, input, WTFMove(arguments));
if (policyValue.hasException()) {
if (policyValue.exception().message().contains("TrustedTypePolicyOptions did not specify"_s))
return String(nullString());
}
return policyValue;
Member
There was a problem hiding this comment.
Yes clearly wrong if it needs to check the string.
Member
Author
There was a problem hiding this comment.
I've changed it to an enum so it's at least not a magic bool.
lukewarlow
force-pushed
the
trusted-types-require-trusted-types-for-csp-violation
branch
from
9a795ba to
192efe1
Compare
Collaborator
|
EWS run on previous version of this PR (hash 192efe1)
|
Member
Author
|
@darinadler Thanks for the review I've addressed or replied to all your comments |
webkit-ews-buildbot
added
the
merging-blocked
lukewarlow
removed
the
merging-blocked
lukewarlow
force-pushed
the
trusted-types-require-trusted-types-for-csp-violation
branch
from
192efe1 to
49d817e
Compare
Collaborator
|
EWS run on previous version of this PR (hash 49d817e)
|
webkit-ews-buildbot
added
the
merging-blocked
lukewarlow
removed
the
merging-blocked
lukewarlow
force-pushed
the
trusted-types-require-trusted-types-for-csp-violation
branch
from
49d817e to
8b6337d
Compare
Collaborator
|
EWS run on previous version of this PR (hash 8b6337d)
|
darinadler
approved these changes
Mar 3, 2024
lukewarlow
force-pushed
the
trusted-types-require-trusted-types-for-csp-violation
branch
from
8b6337d to
6f76254
Compare
Collaborator
|
EWS run on current version of this PR (hash 6f76254) |
lukewarlow
added
the
safe-merge-queue
webkit-ews-buildbot
added
merge-queue
Collaborator
|
Safe-Merge-Queue: Build #13879. |
https://bugs.webkit.org/show_bug.cgi?id=267685 Reviewed by Darin Adler. This patch implements the StringContext idl attribute to check the `require-trusted-types-for` CSP and enforce trusted types accordingly. This patch also makes use of the StringContext IDL attribute on an initial set of sinks. More complicated sinks such as setAttribute, execCommand, eval and timer functions will be addressed in follow ups. Spec: https://w3c.github.io/trusted-types/dist/spec/#integrations * LayoutTests/TestExpectations: * LayoutTests/imported/w3c/web-platform-tests/content-security-policy/reporting/report-clips-sample.https-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/HTMLElement-generic-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/HTMLScriptElement-internal-slot-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/WorkerGlobalScope-importScripts-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/block-string-assignment-to-DOMParser-parseFromString-expected.txt: * 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-Element-insertAdjacentHTML-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/block-string-assignment-to-Element-outerHTML-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/block-string-assignment-to-Element-setAttribute-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/block-string-assignment-to-HTMLElement-generic-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/block-string-assignment-to-Range-createContextualFragment-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/block-text-node-insertion-into-script-element-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/default-policy-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/default-policy-report-only-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/empty-default-policy-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/empty-default-policy-report-only-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/no-require-trusted-types-for-report-only-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/require-trusted-types-for-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/require-trusted-types-for-report-only-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/trusted-types-createHTMLDocument-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/trusted-types-report-only-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/trusted-types-reporting-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/trusted-types-source-file-path-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/trusted-types-svg-script-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/worker-constructor.https-expected.txt: * Source/WebCore/Headers.cmake: * Source/WebCore/Sources.txt: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/bindings/IDLTypes.h: * Source/WebCore/bindings/js/JSDOMConvertStrings.cpp: (WebCore::trustedTypeCompliantString): * Source/WebCore/bindings/js/JSDOMConvertStrings.h: (WebCore::Converter<IDLStringContextTrustedHTMLAdaptor<T>>::convert): (WebCore::JSConverter<IDLStringContextTrustedHTMLAdaptor<T>>::convert): (WebCore::Converter<IDLLegacyNullToEmptyStringStringContextTrustedHTMLAdaptor<T>>::convert): (WebCore::JSConverter<IDLLegacyNullToEmptyStringStringContextTrustedHTMLAdaptor<T>>::convert): (WebCore::Converter<IDLStringContextTrustedScriptAdaptor<T>>::convert): (WebCore::JSConverter<IDLStringContextTrustedScriptAdaptor<T>>::convert): (WebCore::Converter<IDLLegacyNullToEmptyStringStringContextTrustedScriptAdaptor<T>>::convert): (WebCore::JSConverter<IDLLegacyNullToEmptyStringStringContextTrustedScriptAdaptor<T>>::convert): (WebCore::Converter<IDLStringContextTrustedScriptURLAdaptor<T>>::convert): (WebCore::JSConverter<IDLStringContextTrustedScriptURLAdaptor<T>>::convert): (WebCore::Converter<IDLLegacyNullToEmptyStringStringContextTrustedScriptURLAdaptor<T>>::convert): (WebCore::JSConverter<IDLLegacyNullToEmptyStringStringContextTrustedScriptURLAdaptor<T>>::convert): (WebCore::Converter<IDLAtomStringStringContextTrustedHTMLAdaptor<T>>::convert): (WebCore::JSConverter<IDLAtomStringStringContextTrustedHTMLAdaptor<T>>::convert): (WebCore::Converter<IDLAtomStringStringContextTrustedScriptAdaptor<T>>::convert): (WebCore::JSConverter<IDLAtomStringStringContextTrustedScriptAdaptor<T>>::convert): (WebCore::Converter<IDLAtomStringStringContextTrustedScriptURLAdaptor<T>>::convert): (WebCore::JSConverter<IDLAtomStringStringContextTrustedScriptURLAdaptor<T>>::convert): * Source/WebCore/bindings/scripts/CodeGeneratorJS.pm: (GenerateParametersCheck): (IsAnnotatedType): (GetAnnotatedIDLType): (JSValueToNative): * Source/WebCore/bindings/scripts/IDLAttributes.json: * Source/WebCore/bindings/scripts/test/BindingTestGlobalConstructors.idl: * Source/WebCore/bindings/scripts/test/JS/JSTestGlobalObject.cpp: (WebCore::jsTestGlobalObject_TestStringContextConstructorGetter): (WebCore::JSC_DEFINE_CUSTOM_GETTER): * Source/WebCore/bindings/scripts/test/JS/JSTestStringContext.cpp: Added. (WebCore::JSTestStringContextDOMConstructor::prototypeForStructure): (WebCore::JSTestStringContextDOMConstructor::initializeProperties): (WebCore::JSTestStringContextPrototype::finishCreation): (WebCore::JSTestStringContext::JSTestStringContext): (WebCore::JSTestStringContext::createPrototype): (WebCore::JSTestStringContext::prototype): (WebCore::JSTestStringContext::getConstructor): (WebCore::JSTestStringContext::destroy): (WebCore::JSC_DEFINE_CUSTOM_GETTER): (WebCore::jsTestStringContext_attributeWithStringContextTrustedHTMLGetter): (WebCore::setJSTestStringContext_attributeWithStringContextTrustedHTMLSetter): (WebCore::JSC_DEFINE_CUSTOM_SETTER): (WebCore::jsTestStringContext_attributeWithStringContextTrustedScriptGetter): (WebCore::setJSTestStringContext_attributeWithStringContextTrustedScriptSetter): (WebCore::jsTestStringContext_attributeWithStringContextTrustedScriptURLGetter): (WebCore::setJSTestStringContext_attributeWithStringContextTrustedScriptURLSetter): (WebCore::jsTestStringContext_attributeWithStringContextTrustedHTMLAndLegacyNullToEmptyStringGetter): (WebCore::setJSTestStringContext_attributeWithStringContextTrustedHTMLAndLegacyNullToEmptyStringSetter): (WebCore::jsTestStringContext_attributeWithStringContextTrustedScriptAndLegacyNullToEmptyStringGetter): (WebCore::setJSTestStringContext_attributeWithStringContextTrustedScriptAndLegacyNullToEmptyStringSetter): (WebCore::jsTestStringContext_attributeWithStringContextTrustedScriptURLAndLegacyNullToEmptyStringGetter): (WebCore::setJSTestStringContext_attributeWithStringContextTrustedScriptURLAndLegacyNullToEmptyStringSetter): (WebCore::jsTestStringContext_reflectedAttributeWithStringContextTrustedHTMLGetter): (WebCore::setJSTestStringContext_reflectedAttributeWithStringContextTrustedHTMLSetter): (WebCore::jsTestStringContext_reflectedAttributeWithStringContextTrustedScriptGetter): (WebCore::setJSTestStringContext_reflectedAttributeWithStringContextTrustedScriptSetter): (WebCore::jsTestStringContext_reflectedAttributeWithStringContextTrustedScriptURLGetter): (WebCore::setJSTestStringContext_reflectedAttributeWithStringContextTrustedScriptURLSetter): (WebCore::jsTestStringContext_reflectedUrlAttributeWithStringContextTrustedHTMLGetter): (WebCore::setJSTestStringContext_reflectedUrlAttributeWithStringContextTrustedHTMLSetter): (WebCore::jsTestStringContext_reflectedUrlAttributeWithStringContextTrustedScriptGetter): (WebCore::setJSTestStringContext_reflectedUrlAttributeWithStringContextTrustedScriptSetter): (WebCore::jsTestStringContext_reflectedUrlAttributeWithStringContextTrustedScriptURLGetter): (WebCore::setJSTestStringContext_reflectedUrlAttributeWithStringContextTrustedScriptURLSetter): (WebCore::jsTestStringContextPrototypeFunction_methodWithStringContextTrustedHTMLBody): (WebCore::JSC_DEFINE_HOST_FUNCTION): (WebCore::jsTestStringContextPrototypeFunction_methodWithStringContextTrustedScriptBody): (WebCore::jsTestStringContextPrototypeFunction_methodWithStringContextTrustedScriptURLBody): (WebCore::jsTestStringContextPrototypeFunction_methodWithStringContextTrustedHTMLAndLegacyNullToEmptyStringBody): (WebCore::jsTestStringContextPrototypeFunction_methodWithStringContextTrustedScriptAndLegacyNullToEmptyStringBody): (WebCore::jsTestStringContextPrototypeFunction_methodWithStringContextTrustedScriptURLAndLegacyNullToEmptyStringBody): (WebCore::JSTestStringContext::subspaceForImpl): (WebCore::JSTestStringContext::analyzeHeap): (WebCore::JSTestStringContextOwner::isReachableFromOpaqueRoots): (WebCore::JSTestStringContextOwner::finalize): (WebCore::toJSNewlyCreated): (WebCore::toJS): (WebCore::JSTestStringContext::toWrapped): * Source/WebCore/bindings/scripts/test/JS/JSTestStringContext.h: Added. (WebCore::JSTestStringContext::create): (WebCore::JSTestStringContext::createStructure): (WebCore::JSTestStringContext::subspaceFor): (WebCore::wrapperOwner): (WebCore::wrapperKey): (WebCore::toJS): (WebCore::toJSNewlyCreated): * Source/WebCore/bindings/scripts/test/SupplementalDependencies.dep: * Source/WebCore/bindings/scripts/test/TestStringContext.idl: Added. * Source/WebCore/dom/Document+HTML.idl: * Source/WebCore/dom/Document.idl: * Source/WebCore/dom/Element+DOMParsing.idl: * Source/WebCore/dom/InnerHTML.idl: * Source/WebCore/dom/Range+DOMParsing.idl: * Source/WebCore/dom/TrustedType.cpp: Added. (WebCore::TrustedTypeVisitor::operator()): (WebCore::trustedTypeToString): (WebCore::trustedTypeToCallbackName): (WebCore::processValueWithDefaultPolicy): (WebCore::trustedTypeCompliantString): * Source/WebCore/dom/TrustedType.h: Copied from Source/WebCore/workers/Worker.idl. * Source/WebCore/dom/TrustedTypePolicy.cpp: (WebCore::TrustedTypePolicy::createHTML): (WebCore::TrustedTypePolicy::createScript): (WebCore::TrustedTypePolicy::createScriptURL): (WebCore::TrustedTypePolicy::getPolicyValue): * Source/WebCore/dom/TrustedTypePolicy.h: * Source/WebCore/html/HTMLEmbedElement.idl: * Source/WebCore/html/HTMLIFrameElement.idl: * Source/WebCore/html/HTMLObjectElement.idl: * Source/WebCore/html/HTMLScriptElement.idl: * Source/WebCore/page/csp/ContentSecurityPolicy.cpp: (WebCore::ContentSecurityPolicy::requireTrustedTypesForSinkGroup const): (WebCore::ContentSecurityPolicy::allowMissingTrustedTypesForSinkGroup const): (WebCore::ContentSecurityPolicy::reportViolation const): * Source/WebCore/page/csp/ContentSecurityPolicy.h: * Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp: (WebCore::ContentSecurityPolicyDirectiveList::shouldReportSample const): * Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.h: (WebCore::ContentSecurityPolicyDirectiveList::requiresTrustedTypesForScript const): * Source/WebCore/workers/Worker.idl: * Source/WebCore/workers/WorkerGlobalScope.idl: * Source/WebCore/workers/service/ServiceWorkerContainer.idl: * Source/WebCore/workers/shared/SharedWorker.idl: * Source/WebCore/xml/DOMParser.idl: Canonical link: https://commits.webkit.org/275607@main
webkit-commit-queue
force-pushed
the
trusted-types-require-trusted-types-for-csp-violation
branch
from
6f76254 to
caf4cd7
Compare
Collaborator
|
Committed 275607@main (caf4cd7): https://commits.webkit.org/275607@main Reviewed commits have been landed. Closing PR #23412 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
caf4cd7
6f76254