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

Revive require-sri-for for scripts #129

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

yoavweiss
Copy link

@yoavweiss yoavweiss commented Feb 6, 2025

This ~reverts https://github.com/w3c/webappsec-subresource-integrity/pull/82/files, which reverted require-sri-for.

It only includes the script token, as it has a clear use case.

I believe the reasons for removing require-sri-for are losing ground:

Integrity checks for workers are still missing, but there's work underway to enable importmaps in workers which will give us that, at least for some assets.

require-sri-for CSP directive

Subresource-Integrity (SRI) enables developers to make sure the assets they intend to load are indeed the assets they are loading. But there's no current way for developers to be sure that all of their scripts are validated using SRI.

The require-sri-for CSP directive gives developers the ability to assert that every resource of a given type needs to be integrity checked. If a resource of that type is attempted to be loaded without integrity metadata, that attempt will fail and trigger a CSP violation report.

Example usage

A developer that wants to validate that all of their script resources have integrity checks will be able to add a CSP header similar to:

Content-Security-Policy: require-sri-for 'script' 

(Or the equivalent <meta> tag)

From that point, any external script that is fetched without a valid integrity attribute (that is, one that translates into non-empty integrity metadata) or with a "no-cors" request mode, will not be loaded.
It will also trigger a "securitypolicyviolation" event, as well as a CSP report.


Preview | Diff

@yoavweiss yoavweiss marked this pull request as draft February 6, 2025 07:50
@yoavweiss
Copy link
Author

Further work on this would require:

  • Ensuring data/blob URLs get a free pass
  • Ensuring Signature-based SRI is considered "integrity metadata".

We'd also need tests for the above

aarongable pushed a commit to chromium/chromium that referenced this pull request Feb 13, 2025
`require-sri-for` would enable documents to enforce SRI on all resources
they load (of a certain type). This CL revives a previous attempt [1]
at this that ended up being removed. It only adds the 'script' part of
it, as this has a clear use case [2].

Intent-to-Prototype: https://groups.google.com/a/chromium.org/g/blink-dev/c/CdLp5BM2FCQ/m/t9ae0Do_AAAJ

Spec PR: w3c/webappsec-subresource-integrity#129


[1] https://chromium-review.googlesource.com/c/chromium/src/+/2199260
[2] https://docs.google.com/document/d/1RcUpbpWPxXTyW0Qwczs9GCTLPD3-LcbbhL4ooBUevTM/edit?tab=t.0

Change-Id: I66acc12b073174cb33cf594b714e803e24656d27
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5877633
Reviewed-by: Antonio Sartori <[email protected]>
Commit-Queue: Yoav Weiss (@Shopify) <[email protected]>
Reviewed-by: Arthur Sonzogni <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1419883}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 13, 2025
`require-sri-for` would enable documents to enforce SRI on all resources
they load (of a certain type). This CL revives a previous attempt [1]
at this that ended up being removed. It only adds the 'script' part of
it, as this has a clear use case [2].

Intent-to-Prototype: https://groups.google.com/a/chromium.org/g/blink-dev/c/CdLp5BM2FCQ/m/t9ae0Do_AAAJ

Spec PR: w3c/webappsec-subresource-integrity#129

[1] https://chromium-review.googlesource.com/c/chromium/src/+/2199260
[2] https://docs.google.com/document/d/1RcUpbpWPxXTyW0Qwczs9GCTLPD3-LcbbhL4ooBUevTM/edit?tab=t.0

Change-Id: I66acc12b073174cb33cf594b714e803e24656d27
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5877633
Reviewed-by: Antonio Sartori <[email protected]>
Commit-Queue: Yoav Weiss (@Shopify) <[email protected]>
Reviewed-by: Arthur Sonzogni <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1419883}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 13, 2025
`require-sri-for` would enable documents to enforce SRI on all resources
they load (of a certain type). This CL revives a previous attempt [1]
at this that ended up being removed. It only adds the 'script' part of
it, as this has a clear use case [2].

Intent-to-Prototype: https://groups.google.com/a/chromium.org/g/blink-dev/c/CdLp5BM2FCQ/m/t9ae0Do_AAAJ

Spec PR: w3c/webappsec-subresource-integrity#129

[1] https://chromium-review.googlesource.com/c/chromium/src/+/2199260
[2] https://docs.google.com/document/d/1RcUpbpWPxXTyW0Qwczs9GCTLPD3-LcbbhL4ooBUevTM/edit?tab=t.0

Change-Id: I66acc12b073174cb33cf594b714e803e24656d27
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5877633
Reviewed-by: Antonio Sartori <[email protected]>
Commit-Queue: Yoav Weiss (@Shopify) <[email protected]>
Reviewed-by: Arthur Sonzogni <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1419883}
@yoavweiss yoavweiss marked this pull request as ready for review February 14, 2025 05:19
@yoavweiss
Copy link
Author

  • Ensuring data/blob URLs get a free pass

Done

  • Ensuring Signature-based SRI is considered "integrity metadata".

The relevant Fetch parts mean that any string is considered a non-empty integrity metadata.
At the same time, we explicitly test that unknown signatures cause a request to be blocked.

@yoavweiss
Copy link
Author

The relevant Fetch parts mean that any string is considered a non-empty integrity metadata.
At the same time, we explicitly test that unknown signatures cause a request to be blocked.

done!

Comment on lines +360 to +374
### Parsing `require-sri-for` ### {#parse-require-sri-for}

Given a string (|token list|), this algorithm returns a list of resource
types which will require integrity checks:

1. Let |protected resource types| be the empty <a for=/>set</a>.

2. For each |token| in the result of <a lt="split a string on ascii whitespace">
splitting |token list| on spaces</a>, if token matches the grammar
for <a>require-sri-for</a> and is a <a>ASCII case-insensitive match</a>
for any of the <a>known token</a>s, add |token| to |protected resource types|.
Otherwise, ignore the token.

3. Return the set of |protected resource types|.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How likely is it that we expand the list of known tokens? Otherwise a simple string-equals match would suffice.

Copy link
Author

Choose a reason for hiding this comment

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

Previously the list also included "style". As I don't have a clear use case for it, I didn't add it here.

We could drop this to be a simple equality check now, and expand to a list if/when the time comes.

Copy link
Member

Choose a reason for hiding this comment

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

What should the behavior be in user agents which don't understand the new type?

This suggestion would make them not enforce anything at all, which seems suboptimal?

Copy link
Author

Choose a reason for hiding this comment

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

I think this is the way CSP directives are currently defined, and that enabled us to expand them over time.

Not enforcing anything on unknown types would enable us to expand the list overtime if needed. What's the alternative you see?

Copy link
Author

Choose a reason for hiding this comment

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

Thinking through @annevk's other comment, workers may be a good candidate for near-future expansion of the list.

@mozfreddyb
Copy link
Collaborator

With my editor hat on, I think this passes review.
With my Mozilla dinosaur hat on: We may want to align such that we have more than one implementation in planning before we merge this. How fitting that the topic is on the webappsec agenda for this week.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 18, 2025
Automatic update from web-platform-tests
require-sri-for: 'script'

`require-sri-for` would enable documents to enforce SRI on all resources
they load (of a certain type). This CL revives a previous attempt [1]
at this that ended up being removed. It only adds the 'script' part of
it, as this has a clear use case [2].

Intent-to-Prototype: https://groups.google.com/a/chromium.org/g/blink-dev/c/CdLp5BM2FCQ/m/t9ae0Do_AAAJ

Spec PR: w3c/webappsec-subresource-integrity#129

[1] https://chromium-review.googlesource.com/c/chromium/src/+/2199260
[2] https://docs.google.com/document/d/1RcUpbpWPxXTyW0Qwczs9GCTLPD3-LcbbhL4ooBUevTM/edit?tab=t.0

Change-Id: I66acc12b073174cb33cf594b714e803e24656d27
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5877633
Reviewed-by: Antonio Sartori <[email protected]>
Commit-Queue: Yoav Weiss (@Shopify) <[email protected]>
Reviewed-by: Arthur Sonzogni <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1419883}

--

wpt-commits: 5f473f1dcc4b545448d84c380b4a6715db612fd1
wpt-pr: 50681
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Feb 19, 2025
Automatic update from web-platform-tests
require-sri-for: 'script'

`require-sri-for` would enable documents to enforce SRI on all resources
they load (of a certain type). This CL revives a previous attempt [1]
at this that ended up being removed. It only adds the 'script' part of
it, as this has a clear use case [2].

Intent-to-Prototype: https://groups.google.com/a/chromium.org/g/blink-dev/c/CdLp5BM2FCQ/m/t9ae0Do_AAAJ

Spec PR: w3c/webappsec-subresource-integrity#129

[1] https://chromium-review.googlesource.com/c/chromium/src/+/2199260
[2] https://docs.google.com/document/d/1RcUpbpWPxXTyW0Qwczs9GCTLPD3-LcbbhL4ooBUevTM/edit?tab=t.0

Change-Id: I66acc12b073174cb33cf594b714e803e24656d27
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5877633
Reviewed-by: Antonio Sartori <[email protected]>
Commit-Queue: Yoav Weiss (@Shopify) <[email protected]>
Reviewed-by: Arthur Sonzogni <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1419883}

--

wpt-commits: 5f473f1dcc4b545448d84c380b4a6715db612fd1
wpt-pr: 50681
@mikewest
Copy link
Member

On the process question @mozfreddyb raised above:

We discussed this in the WebAppSec WG yesterday (minutes at https://github.com/w3c/webappsec/blob/main/meetings/2025/2025-02-19-minutes.md#reviving-require-sri-for). Generally, folks on the call (and folks I've talked to elsewhere) are on board with this functionality.

I'd like to wait to land this in the spec until we've kicked off the adoption process for this work; it's quite clearly covered by our current charter, but I'd like to formally get the ball rolling on an SRI Level 2 we can continually update under the new(ish) W3C publication process. Once that's in place, adding this as a feature to that new workstream should be straightforward.

@simoneonofri is going to send out an email to the list to that effect based on yesterday's conversation, and if all goes well, I think we can get this in by March.

Does that work for you, @mozfreddyb and @yoavweiss?

@yoavweiss
Copy link
Author

yoavweiss commented Feb 20, 2025

Does that work for you?

Yeah, thank you! :)

@mozfreddyb
Copy link
Collaborator

Yes, but specifically, once we move to a Living Standard I would like us to ensure that we have more than 1 implementation interested and with a positive standards position before merging any changes here and elsewhere

@mikewest
Copy link
Member

@mozfreddyb: I suspect we'd generally want to end up with something like https://whatwg.org/working-mode, which sets different bars for different kinds of changes.

That said, you're editing this spec, and I think that gives you a reasonable amount of freedom to interpret when something is supported-enough to land.

jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Feb 25, 2025
Automatic update from web-platform-tests
require-sri-for: 'script'

`require-sri-for` would enable documents to enforce SRI on all resources
they load (of a certain type). This CL revives a previous attempt [1]
at this that ended up being removed. It only adds the 'script' part of
it, as this has a clear use case [2].

Intent-to-Prototype: https://groups.google.com/a/chromium.org/g/blink-dev/c/CdLp5BM2FCQ/m/t9ae0Do_AAAJ

Spec PR: w3c/webappsec-subresource-integrity#129

[1] https://chromium-review.googlesource.com/c/chromium/src/+/2199260
[2] https://docs.google.com/document/d/1RcUpbpWPxXTyW0Qwczs9GCTLPD3-LcbbhL4ooBUevTM/edit?tab=t.0

Change-Id: I66acc12b073174cb33cf594b714e803e24656d27
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5877633
Reviewed-by: Antonio Sartori <[email protected]>
Commit-Queue: Yoav Weiss (@Shopify) <[email protected]>
Reviewed-by: Arthur Sonzogni <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1419883}

--

wpt-commits: 5f473f1dcc4b545448d84c380b4a6715db612fd1
wpt-pr: 50681
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.

4 participants