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

fix(spec): add missing subresource integrity checks for css from cdns #34

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DerekNonGeneric
Copy link

This fixes a bug that causes ci failure for folks with codeql actions enabled for their proposal template repos.

I thought handling all cases where this comes up locally would suffice, but then that fixup commit comes and messes it all up again DerekNonGeneric/proposal-assertion-error#14, so maybe fixing it here would be preferable.

/cc @ljharb @bakkot

index.html Outdated
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/highlight.js/11.0.1/styles/base16/solarized-light.min.css"><link rel="stylesheet" href="./spec.css">
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/highlight.js/8.4/styles/github.min.css">
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/highlight.js/11.0.1/styles/base16/solarized-light.min.css" integrity="sha512-1l+yD1g5lzzMJrdmD8L5N6FblzkAnd9dzjw03oYJkhe8NpmyzkqahigJDRhs1SuxYvZAsHIY50pqa/rzrWzArA==" crossorigin="anonymous"><link rel="stylesheet" href="./spec.css">
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/highlight.js/8.4/styles/github.min.css" integrity="sha512-iDO4iL6PUvPt54yn8f29JSkoMrScHRIMArsHJtOTGg2yTHWT7dfvziWeXIneGy+wGwbrhGMg1a1ntA+sNA+atg==" crossorigin="anonymous">
Copy link
Member

Choose a reason for hiding this comment

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

why would we need to add this?

Copy link
Author

Choose a reason for hiding this comment

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

Well, this ensures that the webpage author receives the file that they expect to receive from the CDNs. CDNs can get attacked and start serving malicious files to everyone visiting your page.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but this is a spec proposal. Why is that important?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think it matters what the particular webpage is about, it's more to do w/ being sure that you are getting what you expect to be getting.

Regarding the crossorigin="anonymous", maybe read about on mdn, but i haven't had to think about cors in too long anymore.

See: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/crossorigin

Copy link
Member

Choose a reason for hiding this comment

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

Right, but i mean, there's nothing to compromise here - this would just be security for its own sake, but there's no damage to prevent.

@ljharb ljharb force-pushed the fix/css-on-cdn-missing-integrity branch from b3fcc84 to 3447a12 Compare May 31, 2023 19:41
@ljharb ljharb marked this pull request as draft May 31, 2023 19:41
@ljharb ljharb force-pushed the fix/css-on-cdn-missing-integrity branch from 3447a12 to ddc6b54 Compare May 31, 2023 19:52
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.

2 participants