-
Notifications
You must be signed in to change notification settings - Fork 901
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
[Improvement] Add integrity tags to all 3rd party assets #5558
Comments
This would also be a good opportunity for us to bump the dependency versions. It's been a while since we last did that. |
@tabacitu thank you for opening the issue based on my email. In addition to singing the includes I still woud appreciate if the old behavior could still be used via some kind of switch. It is harder to review and ship such CDN contents than locally build packages. |
@tabacitu What's the status on this? This blocks us from upgrading |
Thanks for the reminder @amenk !
We do not plan on adding back the old behaviour - that would consider a HUGE breaking change, and a step back for us.
I agree - we did plan a v2 for Basset that included Regarding actually implementing SRI in Basset, here are the possible solutions we are considering: SOLUTION 1. Add the integrity hashes to all our assets, manually. For example instead of: @basset('https://cdn.datatables.net/1.13.1/css/dataTables.bootstrap5.min.css') we can do: @basset('https://cdn.datatables.net/1.13.1/css/dataTables.bootstrap5.min.css', true, [
'integrity' => 'GADhaOJCr6lsUqdHJnYcH/QaARzVT92beGzAYxLTSoxUorHjQZci1FW+X9BqbnE3%',
'crossorigin' => 'anonymous',
]) This will output: <link href="/storage/basset/cdn.datatables.net/1.13.1/css/dataTables.bootstrap5.min.css?187e21de716e"
rel="stylesheet"
type="text/css"
integrity="GADhaOJCr6lsUqdHJnYcH/QaARzVT92beGzAYxLTSoxUorHjQZci1FW+X9BqbnE3%"
crossorigin="anonymous" /> THE PROBLEM with this solution is that it's manual. It requires us to MANUALLY add the hash for each asset we use. And to update it, every time we update the asset. This isn't sustainable. And it would 100% break, when loading local assets from userland, that the developer can change (eg. Then again... we can speed this up by using a service like https://www.srihash.org/ that generates the hash... so it wouldn't be THAT much of a trouble the first time we do it. It would still be unsustainable long-term... but not a HUGE deal. SOLUTION 2. NOT AN ACTUAL OPTION. We could generate a hash on-the-fly, when SOLUTION 3. We could modify the I would say:
That being said, SOLUTION 3 doesn't render SOLUTION 1 worthless. In fact, for the assets where the vendor has published a has in the docs page, it's preferrable to have that hash written along with the URL. Then Basset won't have to generate a hash for it, we just test against it. What do you think @amenk ? Do you see SOLUTION 1 or SOLUTION 3 as being rock-solid? If not, any other solution you have in mind? |
I actually see SOLUTION 1 as being quite okay. That's what I had in mind. Yes it's manual - but that's the whole point: We have to consciously choose a version, eventually do a short review for known issues and then calculate the hash. And such updates don't seem to happen so often in backpack - so this should be fine. We can always add a artisan scaffolding tool
yes, I would say so - this is a much narrower attack vector. I think SRI is mostly meant to 3rd party stuff. |
Thanks for the suggestion @amenk We are implementing SRI hashes for 3rd party content in backpack for the next version 👍 Will be closing this as most of the work is already done! Stay tuned 🥳 Cheers |
A user just suggested on email that we should add integrity tags to our assets.
I think it's a small change that would go a long way towards preventing some attacks. A bang-for-buck change.
Especially now that supply chain attacks are more common (see polyfill incident in July 2024), we can do a little more to prevent this kind of attacks.
I suggest we go through all our repos, one by one, and add integrity tags. Afaik Basset already supports it, it's just a matter of actually copy-pasting them to our code as well.
What do you guys think, isn't this an easy and impactful thing we can finish by the end of the month?
The text was updated successfully, but these errors were encountered: