Issues with SRI implementation

[Tugzrida]

I was happy to see SRI added in #3256, however when I tested it out on my site, I noticed a few issues.

On my site I have the jquery collection set to load from a CDN. Because this is an external path, the file_get_contents call that was added fails (it tries to load /var/www/mysitehttps://cdn.example/path/to/jquery according to the error) and causes a crash.

To fix this, I'd suggest just dropping SRI calculation for remote resources, as this is actually not useful from a security perspective. If someone compromises the remote resource, then Grav calculating a hash on the fly will not protect against the compromise, which essentially negates the prime benefit of SRI. The current method is still fine for local resources, as if someone had access to the server running Grav to modify them, they'd assumedly also be able to just remove SRI entirely.

As a follow up to that, it'd be cool to be able to add SRI to assets in a collection (namely remote assets, were you to implement my suggestion above). For individual assets, you can add the integrity attribute either directly in HTML, or in the addCss/addJs call, but for collections, which may be addJs'd in any number of locations, it'd be great to be able to add the integrity attribute at a collection level. Just looking at the options available when adding a collection, I'd guess that this would require a fair bit of reworking, but it'd be a great improvement.

[w00fz]

This is now implemented, remote assets are not going to be checked any longer. While fixing this I also found out of an issue with local paths when Grav instance in sub folder which is now resolved as well. All of this should be available in the next Grav release.

Regarding the collection, I'm not entirely sure I am following your suggestion. The integrity can only be applied to individual assets, and as you probably already know, you can add integrity to a remote asset via addJs/addCss:

{% do assets.addJs('https://cdn.jsdelivr.net/npm/jquery@3.6.0/dist/jquery.min.js', {
  loading: 'defer',
  integrity: 'sha256-/xUj+3OJU5yExlq6GSYGSHk7tPXikynS7ogEvDej/m4=',
  crossorigin: 'anonymous'
}) %}

[Tugzrida]

Thanks for fixing the remote asset issue! And sorry, I perhaps didn't explain the collections issue well enough.

In my system.yaml, I have this:

assets:
  collections:
    jquery:
      1: 'https://cdnjs.cloudflare.com/ajax/libs/jquery/3.5.1/jquery.min.js'

Which means that anywhere throughout the site, jquery can be included with assets.addJs('jquery'). As the addJs call can be used any number of times throughout the site, it'd be nicer to be able to set SRI hashes on the assets at the collection level, rather than having to find every addJs call referencing a given collection and add it there.

Also, as a collection can contain multiple assets, each asset would have their own hash and just adding one for the whole collection in the addJs call wouldn't work.

If there's already a way to do this, then I couldn't find it in the Asset Manager docs.

[w00fz]

Oh yeah that’s a valid point, I had forgotten about collections in the assets manager (not used it much!).

It will definitely require some adjustments to also guarantee backward compatibility. In fact I think ideally it would be nicer to be able to specify any property, and not just the integrity.

I reopened this so we won’t forget.

[Tugzrida]

be able to specify any property, and not just the integrity.

I'd been thinking that too. Would allow for async/deferred scripts, etc. :)

[w00fz]

Hey @Tugzrida , this is now implemented. I also updated the docs with some examples: https://learn.getgrav.org/17/themes/asset-manager#collections-with-attributes (not sure why the anchor doesn't scroll down, it is under the Named Assets and Collections section though).

Thanks for the suggestion!

[Tugzrida]

Wow thank you!

I've done some testing and unfortunately there is a bug with the plain add function. addJs and addCss work fine by my testing.

The add function seems to be getting confused that it's getting an array that isn't just a list or URLs. This is probably something I could look at making a fix for myself, though I'll probably only be able to get to it nearing the end of next week.

[w00fz]

@Tugzrida can you share an example of how you used add(), just add('name') instead of addJs(...) ?

[w00fz]

Actually @Tugzrida I think i was able to reproduce, mind giving this a try again?

[Tugzrida]

Hmm, that's giving me an error. Pretty sure it's coming from the array_replace_recursive you added in the latest commit.

My YAML:

assets:
  collections:
    testcollection:
      'https://example/script.js':
        integrity: scripthash
      'https://example/style.css':
        integrity: stylehash

and twig:

{% do assets.add('testcollection',101) %}

I think it's to do with the priority number there, as it's not an array of options and array_replace_recursive expects arrays. The docs here mention that an integer is accepted as the second argument.

It's probably also worth noting that when adding the collection through the Admin panel, the below YAML is generated, where each collection parses to an array containing one dict for each asset, containing another dict of attributes, rather than a dict for the collection with a dict for each asset's attributes. I don't know how many different styles of syntax is appropriate to accept, seeing as YAML can be so flexible. The below YAML does raise the same error as the above one so I think it may not require any additional parsing, however I don't have time to get into it today.

assets:
  collections:
    testcollection:
      -
        'https://example/script.js': { integrity: scripthash }
      -
        'https://example/style.css': { integrity: stylehash }

Sorry this has ballooned into a complex problem 😅

[w00fz]

Indeed I did not test the case with the priority as second case.
I’ll fix that too, unfortunately the original syntax definition is not really friendly with what we are trying to do here (multi array with params definition), and I’m trying to implement it while maintaining backward compatibility, so yes it is being a bit challenging but I think we are close.

Thank you for the valuable feedbacks, really helpful and appreciated. I think I’ll need to also add some new unit tests for this because they have been all passing when clearly shouldn’t have.

[w00fz]

@Tugzrida I have now fixed the priority case and added unit tests for every case discussed here.
Good thing I added tests because while doing so I found an additional bug in assets caused by how the arguments array for assets was getting manipulated via array_shift which is mutable and causing a lot of problems with unwanted inherited parameters. I have now made it immutable and works a lot more reliably now.

Have a look and let me know if this seems to work reliably for you now. Thanks!

[Tugzrida]

Looks pretty good, however I did find that the following YAML (note the leading dash on the asset url key)

assets:
  collections:
    jquery:
      - https://cdnjs.cloudflare.com/ajax/libs/jquery/3.6.0/jquery.min.js:
          integrity: sha512-894YE6QWD5I59HgZOGReFYm4dnWc1Qt5NtvYSaNcOP+u1T9qYdvdihz0PPSiiqn/+/3e7Jo4EaG7TubfWGUrMQ==
          crossorigin: anonymous

causes this erroneous HTML output:

<script src="/0" https://cdnjs.cloudflare.com/ajax/libs/jquery/3.6.0/jquery.min.js="sha512-894YE6QWD5I59HgZOGReFYm4dnWc1Qt5NtvYSaNcOP+u1T9qYdvdihz0PPSiiqn/+/3e7Jo4EaG7TubfWGUrMQ== anonymous"></script>

Most other sensible YAML worked, so I don't know if this is worth fixing, as it would presumably only effect people who are already changing their settings.

Thanks again for your persistence! :)

[w00fz]

That breaks because you are making the array definition an extra level deep by adding the dash (-).

assets[collections][jquery][0][…cdn…] = [ … ]

Vs the expected (no dash)

assets[collections][jquery][…cdn…] = [ … ]

It now also makes it indexed as opposed to key named, which is why you see that 0 and everything shifted.

I say this is not a supported case and not documented either so most likely it’s not something that will ever trip people up. That extra nested level is not going to ever be needed either now that any param can be passed to an asset in a collection.

Thank you for helping in testing this, I think now you confirmed it works we will get this out rather soon!

[w00fz]

FYI Grav v1.7.17 has been released with these enhancements in it.