Skip to content

script: Implement base-uri CSP check#42272

Merged
TimvdLippe merged 1 commit intoservo:mainfrom
WaterWhisperer:base-uri-CSP-check
Feb 1, 2026
Merged

script: Implement base-uri CSP check#42272
TimvdLippe merged 1 commit intoservo:mainfrom
WaterWhisperer:base-uri-CSP-check

Conversation

@WaterWhisperer
Copy link
Copy Markdown
Contributor

Testing: ./mach test-wpt /content-security-policy/base-uri
Fixes: #42261

@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 1, 2026
Copy link
Copy Markdown
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on! Some nits, but generally LGTM.

P.S. You have picked up a few starter issues now and have proven you are capable of contributing features to Servo. For the next project, do you mind taking on a slightly bigger one? Then we can leave the starter issues for those folks unfamiliar with Servo. Looking forward to hearing your interest and we can help you figure out an appropriate task!

@servo-highfive servo-highfive added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 1, 2026
@WaterWhisperer
Copy link
Copy Markdown
Contributor Author

P.S. You have picked up a few starter issues now and have proven you are capable of contributing features to Servo. For the next project, do you mind taking on a slightly bigger one? Then we can leave the starter issues for those folks unfamiliar with Servo. Looking forward to hearing your interest and we can help you figure out an appropriate task!

Thanks! I'd definitely be interested in taking on something more challenging. I really, really like Servo. It's so interesting and has taught me a lot, and I hope to contribute more.

That said, I should mention that I'm currently still a student, so my availability can be a bit unpredictable. And when completing some past issues, I often actually spent more time than it appeared to understand and learn those new knowledge (if there was a clear direction to let me know what to learn systematically, it would be great). I don't want to commit to something and then end up blocking progress.

So if there's a task that's not super time-sensitive, I'd really be happy to give it a shot.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The reason for the failure here seems to be that the port_part_match function in the content-security-policy crate performs a direct string comparison, which results in "08000" != "8000"

https://github.com/rust-ammonia/rust-content-security-policy/blob/90a0221fa0b1834c7072da4f690326c420e9b397/src/lib.rs#L1977

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if my understanding is correct, but according to the spec the function points to, the port number seems to should be interpreted as a decimal number?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yup I think you found a bug in their code! Feel free to send them a PR with a fix (and a test preferably). When we update the CSP crate we can also update this test expectation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

okay!

@servo-highfive servo-highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Feb 1, 2026
Copy link
Copy Markdown
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

No worries about time-sensitive work. We have plenty of contributors that work for a long time on projects and that's all fine. We appreciate everybody's input, regardless of how long it takes. In fact, taking time to learn things is a great use of your time. So that's encouraged as well.

I recommend you check out the issues with label E-candidate-for-mentoring (but not those with E-very-complex) and let us know. Then we can help you further along.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yup I think you found a bug in their code! Feel free to send them a PR with a fix (and a test preferably). When we update the CSP crate we can also update this test expectation.

@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Feb 1, 2026
@TimvdLippe TimvdLippe added this pull request to the merge queue Feb 1, 2026
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 1, 2026
Merged via the queue into servo:main with commit f405dde Feb 1, 2026
32 checks passed
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 1, 2026
@WaterWhisperer
Copy link
Copy Markdown
Contributor Author

No worries about time-sensitive work. We have plenty of contributors that work for a long time on projects and that's all fine. We appreciate everybody's input, regardless of how long it takes. In fact, taking time to learn things is a great use of your time. So that's encouraged as well.

I recommend you check out the issues with label E-candidate-for-mentoring (but not those with E-very-complex) and let us know. Then we can help you further along.

Understood, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement base-uri CSP check

3 participants