search

Baroshem / nuxt-security

🛡 Automatically configure your app to follow OWASP security patterns and principles by using HTTP Headers and Middleware
https://nuxt-security.vercel.app/
MIT License
743 stars 56 forks source link

fix: integrity attribute added for rel icon #326

Closed dargmuesli closed 6 months ago

dargmuesli commented 6 months ago

Version

nuxt-security: 1.0.0-rc.5 nuxt: 3.8.2

Reproduction Link

https://stackblitz.com/edit/github-bln8kp?file=nuxt.config.ts

Steps to reproduce

Use @nuxtseo/module and nuxt-security.

What is Expected?

The head includes

<link rel="icon" href="/favicon.ico" sizes="any" nonce="geuUTI/dj85sYyy6R++z1w==">

As rel="icon" should be excluded from having integrity added.

What is actually happening?

The head includes

<link rel="icon" href="/favicon.ico" sizes="any" integrity="sha384-udbcbVSoJ0jynxYo+FKdhmcYDst1ze6s6rkgFExSYfpX6tAuGn5whsHNjmcRr4eU" nonce="geuUTI/dj85sYyy6R++z1w==">
dargmuesli commented 6 months ago

As usual, u need to download the stackblitz and execute it locally, it seems.

Baroshem commented 6 months ago

Hey @dargmuesli

Thanks for reporting this issue. @vejja do you think that we should fix it for 1.0.0 or for the patch like 1.0.1?

vejja commented 6 months ago

Hi @dargmuesli Does it actually triggers a bug on your side ? i.e. is it a problem for you if the integrity attribute is included with the icon?

My understanding is that the W3C spec (https://www.w3.org/TR/SRI/) is quite loose;

It is true that all examples in the document are about stylesheets. Also section 3.8 only describes links for stylesheets. There is an overall feeling that SRI was initially intended for stylesheets only (in the case of links).

However that section 3.8 seems to be restricted to required modifications of the HTML fetch protocol for SRI. In addition, section 3.4 applies to all <link> elements, irrespective of their rel attribute. Same for 3.6.1, which extends the <link> interface for all elements.

In addition 3.4 specifically states:

A future revision of this specification is likely to include integrity support for all possible subresources, i.e., a, audio, embed, iframe, img, link, object, script, source, track, and video elements.

Therefore my view was that we can cover all links elements if we have the hash. We offer more protection with no downside, and we keep future compatibility options open, in case some browsers implement SRI on additional subresources?

Edit: I am testing on Chrome, which browser do you use ?

Baroshem commented 6 months ago

From my side, if it does not break anything right now, we could proceed with 1.0.0 and release a fix for 1.0.1. Or if we decide not to change it to support future cases to just leave it as it is :)

dargmuesli commented 6 months ago

Well it's not urgent, so you can release v1, but it makes integration with Nuxt module html-validator fail.

error  "integrity" attribute cannot be used on <link> in this context: "rel" attribute must be "stylesheet", "preload" or "modulepreload"  attribute-misuse

rule

Isn't there already the logic that should filter the tags on which integrity is to be added in the following place? https://github.com/Baroshem/nuxt-security/blob/b9450405cade209ab28fb679f0ccc7a6087d7193/src/runtime/nitro/plugins/04-cspSsgHashes.ts#L74

vejja commented 6 months ago

Well it's not urgent, so you can release v1, but it makes integration with Nuxt module html-validator fail.

error  "integrity" attribute cannot be used on <link> in this context: "rel" attribute must be "stylesheet", "preload" or "modulepreload"  attribute-misuse

rule

Well you're correct. In fact the HTML5 standard says that the integrity attribute is only valid on these 3 rels, while the SRI specs say that it is valid on any rel. But at the end of the day our document is an HTML one, so I believe you're correct and we should patch to conform to the standard.

Isn't there already the logic that should filter the tags on which integrity is to be added in the following place?

https://github.com/Baroshem/nuxt-security/blob/b9450405cade209ab28fb679f0ccc7a6087d7193/src/runtime/nitro/plugins/04-cspSsgHashes.ts#L74

Yes, although that part is CSP-related. It extracts the integrity hashes from HTML to copy them into the CSP header. So here I made sure that I was only extracting the hashes when valid from an HTML perspective.

Baroshem commented 6 months ago

@vejja @dargmuesli

Thanks for additional info guys.

Do you think it should be added to 1.0.0 or can be added as 1.0.1?

dargmuesli commented 6 months ago

For me 1.0.1 is fine

vejja commented 6 months ago

Fixed by #328 @baroshem if it's not too late it can go into 1.0.0

Baroshem commented 6 months ago

Should be fixed in 1.0.0 :)