Skip to content

Add Subresource Integrity value to manifest #238

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

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

Conversation

zcei
Copy link
Contributor

@zcei zcei commented Jun 4, 2025

Hello maintainers,

this is a first draft on how we could alter the manifest format to account for more information than just the logical path -> digested path mapping without dropping support for existing manifests.

In #56 this was postponed with the option to revisit at a later time. With Rails 8 shipping propshaft by default I feel like it's a good time to open up this topic again.

The need / right place for this?

Is this something you're willing to have within this library?

I would love to see this in here, as it for me is part of "Digest stamping" functionality, where we provide another layer of security that the assets we loaded and processed are in fact the ones we expect for the digest.

One could argue it could live outside, but I fear it would replicate a lot from propshaft to basically take file inputs and build a manifest which only does file path -> integrity value mappings.

If you are strongly against it, we can stop the discussion here and could at least leave a written trail for anyone else looking for this feature in the future.

Evolving the Manifest

Right now the manifest cannot be evolved at all without some sort of backwards incompatibility. This is because there was never a "namespace" reserved to work in there.

Currently: Simple Manifest

The root of the JSON is directly the Record<LogicalPath, DigestedPath> mapping.

For the sake of this discussion I will call this the "simple manifest".

{ "one.txt": "one-f2e1ec14.txt" }

We cannot introduce a key on the root-level of JSON because it would be treated as a LogicalPath key.

Upcoming: Extensible Manifest

What we can do, though, is to alter the value type:

Manifest = Record<LogicalPath, DigestedPath | AssetEntry>
AssetEntry = { 
  digested_path: DigestedPath
  // ... more keys, e.g.
  integrity: String
}
{
  "one.txt": {
    "digested_path": "one-f2e1ec14.txt",
    "integrity": "sha384-LdS8l2QTAF8bD8WPb8QSQv0skTWHhmcnS2XU5LBkVQneGzqIqnDRskQtJvi7ADMe"
  }
}

This allows to account for additional values that can come up in the future, so for the sake of this discussion I will call this the "extensible manifest".

Using the new integrity check value

This PR by itself won't be of much help for anyone using propshaft via Rails. So I'd love to hear from you how you envision this to work in combination with Rails and the javascript_include_tag / stylesheet_link_tag helpers.

Likely the resolvers would have a way to read (static) or calculate (dynamic) the integrity value for the requested asset. For not this is kept backwards compatible "as-is" and only resolves to the digested file path.

(For my use-case it's fine to have it only on the extensible manifest as we would use this right away. So I don't have opinions here.)

Moving forward with the new manifest format

I was thinking about moving the logic into an actual Manifest class to encapsulate format and access for both the simple and extensible manifest, where we internally use the extensible format by mapping the DigestedPath from the simple manifest to the key of the extensible manifest AssetEntry, but for my proposal I opted for an "inline change" where you can clearly see where moving pieces are in this approach.

For example, we only opt-in into the extensible manifest when we we use the Subresource Integrity Check feature. This will cause the least friction for a transition process, but eventually a breaking semver will be necessary if we want to move everyone to the new format.

Code Refactorings

The whole "with integrity" naming feels off to me, but is clear and on-point for the moment. If we decide on how the different manifests will be called, I'll do another pass on streamlining the naming.

@rafaelfranca
Copy link
Member

The extensible manifest makes sense to me. I'd extract the logic to a manifest class.

For javascript_include_tag and similar, we will need to basically have the same code as sprockets to calculate/reuse the integrity.

I's just avoid having two different formats of manifest. After this version of propshaft I'd always write the extensible manifest format, while still being able to read the old format. This will allow us to possibly remove the old format in a later version.

Copy link

@andheiberg andheiberg left a comment

Choose a reason for hiding this comment

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

LGTM 👍 fit's with our needs, allows propshaft to evolve and support an important modern security standard.

Rafaels suggests seem sensible to me 👍

@zcei zcei force-pushed the feat/integrity branch from 129a6bd to 4f8196d Compare June 22, 2025 21:48
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.

3 participants