-
-
Notifications
You must be signed in to change notification settings - Fork 196
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
Check only stylesheet link rels rather than whitelisting other rels #529
Conversation
Codecov Report
@@ Coverage Diff @@
## master #529 +/- ##
==========================================
+ Coverage 98.38% 98.53% +0.14%
==========================================
Files 30 30
Lines 1924 1979 +55
==========================================
+ Hits 1893 1950 +57
+ Misses 31 29 -2
Continue to review full report at Codecov.
|
|
||
def check_sri(line, content) | ||
return if IGNORABE_REL.include?(@link.rel) | ||
return unless SRI_REL_TYPES.include?(@link.rel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this but I think I have to think on it a bit more before approving. For example, script
and link
will also need to be checked. I'm not sure if there are more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this code should only apply to link
tags - script
is handled by another check, in https://github.com/gjtorikian/html-proofer/blob/master/lib/html-proofer/check/scripts.rb#L33, so is unaffected. As I mentioned, as far as I can tell from the spec, the only thing that is proactively loaded in by browsers from link
tags are rel='stylesheet'
, which this should cover.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right! My mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 looks good to me
@gjtorikian Does this additional documentation resolve the issue? 277e1c9 |
Thanks y'all! |
This is the alternative solution for the addition in #528. The SRI spec only applies to
<link rel='stylesheet'>
, so this PR only applies the check to those. This means we don't need an ever-expanding whitelist of rel types, but can easily add more into the check.