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

fix: integrity attribute added for rel icon #326

Closed
dargmuesli opened this issue Dec 11, 2023 · 10 comments · Fixed by #328
Closed

fix: integrity attribute added for rel icon #326

dargmuesli opened this issue Dec 11, 2023 · 10 comments · Fixed by #328
Assignees
Labels
bug Something isn't working

Comments

@dargmuesli
Copy link
Contributor

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 dargmuesli added the bug Something isn't working label Dec 11, 2023
@dargmuesli
Copy link
Contributor Author

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

@Baroshem
Copy link
Owner

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
Copy link
Collaborator

vejja commented Dec 11, 2023

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 ?

@vejja vejja self-assigned this Dec 11, 2023
@Baroshem
Copy link
Owner

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
Copy link
Contributor Author

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?

if (rel === 'stylesheet' && hashStyles) {

@vejja
Copy link
Collaborator

vejja commented Dec 11, 2023

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?

if (rel === 'stylesheet' && hashStyles) {

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
Copy link
Owner

@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
Copy link
Contributor Author

For me 1.0.1 is fine

@vejja
Copy link
Collaborator

vejja commented Dec 12, 2023

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

@vejja vejja linked a pull request Dec 12, 2023 that will close this issue
6 tasks
@Baroshem
Copy link
Owner

Should be fixed in 1.0.0 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants