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.

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

add content security policy header to SVG responses #10642

Merged
merged 1 commit into from
Oct 27, 2024

Conversation

chris48s
Copy link
Member

This PR does not resolve an open security issue. However, it does proactively add another layer of protection on top of what we already do to prevent cross-site scripting issue.

SVGs can contain javascript.
When they're embedded with an <img> tag, SVGs are rendered in secure static mode which disables any embedded javascript, along with some other restrictions.
When they're embedded with a <object> tag or loaded directly, this is not the case.

While we take steps to prevent XSS issues, we have on at least one occasion discovered an escaping issue, which made it possible for someone to inject script into a badge image. Reference: #3511

In general, I think our escaping game is pretty solid these days.

That said, it is not impossible that someone might find a way around the mechanisms we have in place or we might introduce an escaping bug in some future refactor.

For this reason, I suggest we serve our SVG images with the Content-Security-Policy: script-src 'none'; header. This is a content security policy header that basically says "no javascript allowed on this response" and offers a strong browser-level protection. This header is respected when the image is loaded directly, but also crucially when embedded with an <object> tag. Essentially, this would mean that even if someone found a way to sneak a <script> tag into a SVG response served from shields.io, any modern browser would refuse to run that script.

@chris48s chris48s added core Server, BaseService, GitHub auth, Shared helpers security Refer to our SECURITY.md policy before opening pull requests that address a security vulnerability labels Oct 27, 2024
Copy link
Contributor

Messages
📖 ✨ Thanks for your contribution to Shields, @chris48s!

Generated by 🚫 dangerJS against 8977fe7

Copy link
Member

@PyvesB PyvesB left a comment

Choose a reason for hiding this comment

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

Makes sense, let's do this!

@chris48s chris48s added this pull request to the merge queue Oct 27, 2024
Merged via the queue into badges:master with commit 6243039 Oct 27, 2024
24 checks passed
@chris48s chris48s deleted the csp branch October 27, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Server, BaseService, GitHub auth, Shared helpers security Refer to our SECURITY.md policy before opening pull requests that address a security vulnerability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants