Fix CSP nonce validation and violation reporting for external scripts#40956
Merged
TimvdLippe merged 3 commits intoservo:mainfrom Feb 27, 2026
Merged
Fix CSP nonce validation and violation reporting for external scripts#40956TimvdLippe merged 3 commits intoservo:mainfrom
TimvdLippe merged 3 commits intoservo:mainfrom
Conversation
Contributor
Author
|
marked as draft because I created a couple wpt failures https://github.com/dyegoaurelio/servo/actions/runs/19787946509 |
dyegoaurelio
force-pushed
the
nonce-external-scripts
branch
from
0fef75f to
074e678
Compare
servo-highfive
added
the
S-awaiting-review
|
🔨 Triggering try run (#19796121125) for Linux (WPT) |
|
Test results for linux-wpt from try job (#19796121125): Flaky unexpected result (29)
Stable unexpected results that are known to be intermittent (21)
|
|
✨ Try run (#19796121125) succeeded. |
TimvdLippe
requested changes
Nov 30, 2025
Contributor
TimvdLippe
left a comment
TimvdLippe
left a comment
There was a problem hiding this comment.
Nice fixes! I do think we can improve the handling the source position since the is_inline boolean seems duplicate information to me.
| }; | ||
| // Determine source location based on violation type and whether position was provided | ||
| let (source_file, line_number, column_number) = | ||
| if source_position_was_provided || is_inline { |
Contributor
There was a problem hiding this comment.
Shouldn't these be both true? E.g. the source position should have only been provided if it was inline. Therefore, I think we should assert that if the resource is URL, no source position is provided. Then only in the URL case we set the filename and for the other ones we use the source position.
Contributor
Author
There was a problem hiding this comment.
It added a couple wpt failures
https://github.com/dyegoaurelio/servo/actions/runs/19801782531
Contributor
Author
There was a problem hiding this comment.
The URL assertion broke the report for images. I reverted the assertion and removed the source position parameter from all URL reports aside from the html image element one
servo-highfive
added
S-needs-code-changes
dyegoaurelio
force-pushed
the
nonce-external-scripts
branch
from
074e678 to
b28b229
Compare
servo-highfive
added
S-awaiting-review
dyegoaurelio
force-pushed
the
nonce-external-scripts
branch
from
b28b229 to
d666287
Compare
dyegoaurelio
force-pushed
the
nonce-external-scripts
branch
2 times, most recently
from
051ada5 to
4f41daa
Compare
TimvdLippe
reviewed
Dec 2, 2025
| source_position.column_number, | ||
| ) | ||
| } else { | ||
| (self.get_url().to_string(), 0, 0) |
Contributor
There was a problem hiding this comment.
I feel like we are over-complicating things here and we would be better off fixing the callers of this method. Moreover, since self.get_url() is the URL for the globalscope, not the resource. Therefore, I think in almost all cases this URL is wrong since we expect the URL of the blocked resource, not the global it is part of?
Instead, can we update all caller-sides to provide the relevant information. If I understand you correctly, in the following cases we should set the filename, but not the line numbers:
- Link elements
- Image elements
And for script elements, it depends whether it is inline or external.
If that's the case, can we revert the changes made in this function and update the elements to pass in the right source position based on these expectations?
Additionally, it might be good to split up the PR as it fixes two distinct issues. One is related to nonce values, the other one is related to the source position. Therefore, I suggest to split it up in those two, to ease reviewing.
Lastly, it would help if WPT tests start passing, yet none are with this PR thus far. I understand that's blocked on the other mentioned issue in the HTML parser, so that's a bit unfortunate. It could be good to write a WPT test that only tests for one thing (e.g. the URL without line and column number) and then we defer passing the other tests until the HTML parser is updated.
Does that help you further?
Contributor
Author
There was a problem hiding this comment.
Makes sense. Thanks for the review!
It could be good to write a WPT test that only tests for one thing (e.g. the URL without line and column number) and then we defer passing the other tests until the HTML parser is updated.
I didn't realize I could just add a new wpt test. I thought they were strictly pulled from upstream.
I agree that it's much better this way. I'll add a new test
If I understand you correctly, in the following > cases we should set the filename, but not the line numbers:
Link elements
Image elements
TBH, I'm not sure about the correct behavior here.
All the changes I made on this regard were because of test assertions:
-
I've removed the line number from the script sources because
expects line number not to be truthy in order for it to check theblockedURI -
after your feedback I reworked it to not allow url resources (eb1d86a). But then It broke
Because this one expects both to be defined. That's why I reverted the assertion and added the line numbers back for the img reports.
Also, I just noticed that the img test is checking the event body.blockedURL while the script one is checking e.blockedURI
Maybe they require slightly different handling. I'm not sure.
I'll try to read the spec more thoughtfully later to get a better understanding of the correct way to build these objects.
Contributor
There was a problem hiding this comment.
Thanks for the investigation! Note that for the second test, only Chrome passes it: https://wpt.fyi/results/content-security-policy/reporting-api/reporting-api-sends-reports-on-violation.https.sub.html?label=experimental&label=master&aligned
If we can't figure out a reasonable behaviour that passes all tests, we can also file an issue with the spec. For example, I am confused why that line expectation is 53, yet that's the line that has the script tag. Safari doesn't report a line number at all. Shouldn't it be line 49?
Contributor
Author
There was a problem hiding this comment.
As Per the spec:
If the user agent is currently executing script, and can extract a source file's URL, line number, and column number from the global, set violation's source file, line number, and column number accordingly.
For declarative HTML loads (external scripts, links, preloads), no script is executing, so source position should be empty per spec. I removed the explicit source_position from those fetch handlers
However, the spec's definition of "source file" is acknowledged as underspecified, and Chrome populates source position for element-initiated image loads. So I kept the image element's source_position to avoid regressing report-to-directive-allowed-in-meta.https.sub.html.
We're also passing the trusted-types/navigate-to-javascript-url-001.html test now due to the column number fix
dyegoaurelio
force-pushed
the
nonce-external-scripts
branch
from
e80d72a to
2e7c6be
Compare
|
🔨 Triggering try run (#22476966400) for Linux (WPT) |
|
Test results for linux-wpt from try job (#22476966400): Flaky unexpected result (32)
Stable unexpected results that are known to be intermittent (17)
|
|
✨ Try run (#22476966400) succeeded. |
TimvdLippe
approved these changes
Feb 27, 2026
Contributor
TimvdLippe
left a comment
TimvdLippe
left a comment
There was a problem hiding this comment.
Nice work, thanks for cleaning this up!
servo-highfive
removed
the
S-awaiting-review
servo-highfive
added
the
S-awaiting-merge
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
servo-highfive
added
S-tests-failed
External `<script>` elements with malformed attributes (e.g., `<script src="evil.js" <script nonce="abc">`) bypass the "is element nonceable" check. Step 24 of "prepare the script element" was reading `[[CryptographicNonce]]` directly via `nonce_value()`, skipping the malformed-attribute check. Add a nonceability guard at step 24: if the element has a nonce content attribute but is not nonceable (malformed attributes per CSP §3.5), strip the nonce. Elements without a nonce content attribute (e.g. JS-created via `.nonce = "abc"`) use the internal slot directly, since the nonceable check only applies to parser-created elements. Spec: https://www.w3.org/TR/CSP/#is-element-nonceable Signed-off-by: Dyego Aurélio <dyegoaurelio@gmail.com>
…etches Fix compute_scripted_caller_source_position to return column 0 (not 1) when no script is on the stack, by matching on Ok/Err from describe_scripted_caller instead of using unwrap_or_default which unconditionally applied the +1 offset. Remove explicit source_position from script, link, and preload fetch response CSP handlers. Image element keeps its source_position to match chrome behavior asserted by report-to-directive-allowed-in-meta.https.sub.html. Also fix the Convert<CSPViolationReportBody> impl to conditionalize sourceFile/lineNumber/columnNumber on source_file being non-empty, matching the existing report-uri serialization. Spec: https://www.w3.org/TR/CSP/#create-violation-for-global Signed-off-by: Dyego Aurélio <dyegoaurelio@gmail.com>
Add a custom servo WPT test that verifies nonce enforcement for both inline and external scripts with malformed attributes. The test covers the subset of nonce-enforce-blocked.html cases that Servo can currently enforce without html5ever changes (duplicate-attribute detection, SVG script support). Remove expected failure for trusted-types/navigate-to-javascript-url-001 which now passes due to the column number fix in compute_scripted_caller_source_position. Signed-off-by: Dyego Aurélio <dyegoaurelio@gmail.com>
dyegoaurelio
force-pushed
the
nonce-external-scripts
branch
from
2e7c6be to
9bdc621
Compare
servo-highfive
added
S-awaiting-review
servo-highfive
added
the
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
This PR fixes two related issues with Content Security Policy (CSP) nonce validation for external scripts:
This makes servo closer to passing the
nonce-enforce-blockedwpt test.The remaining failures are blocked by required changes in the html parser.
the html parser needs to provide this flag, as mentioned on the original commit message (4821bc0)
I've also created a PR to implement the duplicate attrs flag on html5ever servo/html5ever#695
Testing: doesn't fixes the aforementioned wpt test yet.
Fixes: part of #36437