Skip to content

Commit 3d54323

Browse files
JLLeitschuhclaude
andcommitted
refactor: address review feedback (IntegrityError, single algorithm map, README reorg)
Per review from @grosser: - Add CssParser::IntegrityError < RemoteFileError, raised specifically on an SRI mismatch, so callers can distinguish it from other fetch failures (404, SSRF rejection, timeout) while remaining catchable by existing `rescue RemoteFileError` code. Required a small fix to the method's catch-all rescue, which previously downgraded any exception raised inside it (including the new IntegrityError) back to a plain RemoteFileError -- now only IntegrityError specifically is let through unchanged, preserving existing behavior/tests for other error paths. - Merge INTEGRITY_ALGORITHM_PRIORITY and the inline digest-class lookup into one INTEGRITY_ALGORITHMS hash (algorithm => Digest class, ordered strongest first), so there's a single structure to keep in sync instead of two. - Move the Subresource Integrity usage examples out of the main Usage code block into their own README section, since it's a less-common, advanced option. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 7f5e236 commit 3d54323

3 files changed

Lines changed: 80 additions & 39 deletions

File tree

README.md

Lines changed: 33 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -23,26 +23,6 @@ parser.load_uri!('file://home/user/styles/style.css')
2323
# load a remote file, setting the base_uri and media_types
2424
parser.load_uri!('../style.css', {base_uri: 'http://example.com/styles/inc/', media_types: [:screen, :handheld]})
2525

26-
# load a remote file, verifying it against a Subresource Integrity value
27-
# (https://www.w3.org/TR/SRI/) before parsing -- e.g. the value of an
28-
# HTML <link integrity="..."> attribute. Raises CssParser::RemoteFileError
29-
# (or, with io_exceptions: false, loads nothing) when the fetched body
30-
# doesn't match.
31-
parser.load_uri!('http://example.com/styles/style.css', integrity: 'sha384-oqVuAfXRKap7fdgcCY5uykM6+R9GqQ8K/uxy9rx7HNQlGYl1kPzQho1wx4JwY8wC')
32-
33-
# `integrity:` also accepts several space-separated values, exactly like the
34-
# HTML attribute does. When more than one hash algorithm is present, only the
35-
# strongest one is checked (sha512 > sha384 > sha256) and every weaker value is
36-
# ignored; multiple values for that same strongest algorithm are treated as
37-
# alternatives -- matching any one of them is enough.
38-
parser.load_uri!(
39-
'http://example.com/styles/style.css',
40-
integrity: 'sha256-Br6tO8uuFyBAw2O0eUNdXVyuS/POLb5jpHxXaxIq6Q0= sha384-0gCPKBW0n+VzQzZu5gzP+YMxy9QTLyn1y/O/TMvLTpVajzRKAx6d7TiPB5W7DnDn'
41-
)
42-
# ^ only the sha384 value is actually checked here; the sha256 one is present
43-
# (e.g. for browsers/tools that only understand sha256) but ignored by this
44-
# library since a stronger algorithm is also listed.
45-
4626
# load a local file, setting the base_dir and media_types
4727
parser.load_file!('print.css', '~/styles/', :print)
4828

@@ -91,6 +71,39 @@ content_rule.offset
9171
#=> 0..21
9272
```
9373

74+
# Subresource Integrity
75+
76+
`Parser#load_uri!` accepts an `integrity:` option that verifies a fetched remote stylesheet
77+
against a [Subresource Integrity](https://www.w3.org/TR/SRI/) value before it's parsed -- the
78+
same value an HTML `<link integrity="...">` attribute carries.
79+
80+
```Ruby
81+
parser.load_uri!(
82+
'http://example.com/styles/style.css',
83+
integrity: 'sha384-oqVuAfXRKap7fdgcCY5uykM6+R9GqQ8K/uxy9rx7HNQlGYl1kPzQho1wx4JwY8wC'
84+
)
85+
```
86+
87+
When the fetched body doesn't match, `CssParser::IntegrityError` (a subclass of
88+
`CssParser::RemoteFileError`, so existing `rescue RemoteFileError` code is unaffected) is
89+
raised if `io_exceptions` is enabled, or nothing is loaded otherwise.
90+
91+
`integrity:` also accepts several space-separated values, exactly like the HTML attribute
92+
does. When more than one hash algorithm is present, only the strongest one is checked
93+
(sha512 > sha384 > sha256) and every weaker value is ignored; multiple values for that same
94+
strongest algorithm are treated as alternatives -- matching any one of them is enough (useful
95+
during a stylesheet rotation, when a CDN may still serve the old version for a while):
96+
97+
```Ruby
98+
parser.load_uri!(
99+
'http://example.com/styles/style.css',
100+
integrity: 'sha256-Br6tO8uuFyBAw2O0eUNdXVyuS/POLb5jpHxXaxIq6Q0= sha384-0gCPKBW0n+VzQzZu5gzP+YMxy9QTLyn1y/O/TMvLTpVajzRKAx6d7TiPB5W7DnDn'
101+
)
102+
# only the sha384 value is actually checked here; the sha256 one is present
103+
# (e.g. for browsers/tools that only understand sha256) but ignored by this
104+
# library since a stronger algorithm is also listed.
105+
```
106+
94107
# Testing
95108

96109
```Bash

lib/css_parser/parser.rb

Lines changed: 36 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,13 @@ module CssParser
88
# Exception class used for any errors encountered while downloading remote files.
99
class RemoteFileError < IOError; end
1010

11+
# Exception class used when a fetched remote file fails Subresource Integrity
12+
# verification (see the `:integrity` option on `Parser#load_uri!`). A subclass of
13+
# `RemoteFileError` so existing `rescue RemoteFileError` callers are unaffected;
14+
# callers that want to distinguish an integrity failure from other fetch failures
15+
# (404, SSRF rejection, timeout, etc.) can rescue this class specifically.
16+
class IntegrityError < RemoteFileError; end
17+
1118
# Exception class used if a request is made to load a CSS file more than once.
1219
class CircularReferenceError < StandardError; end
1320

@@ -42,12 +49,17 @@ class Parser
4249
# was GHSA-9pmc-p236-855h.
4350
REMOTE_ALLOWED_SCHEMES = %w[http https].freeze
4451

45-
# Subresource Integrity hash algorithms this library can verify,
46-
# strongest first. Mirrors the SRI spec's "agility" rule
47-
# (https://www.w3.org/TR/SRI/#agility): when a caller-supplied
48-
# `integrity` value lists more than one algorithm, only the
49-
# strongest one present is checked.
50-
INTEGRITY_ALGORITHM_PRIORITY = %w[sha512 sha384 sha256].freeze
52+
# Subresource Integrity hash algorithms this library can verify, mapped to their
53+
# Digest class, ordered strongest first. A single structure (rather than a
54+
# separate priority list and digest-class lookup) so the two can't drift out of
55+
# sync. Mirrors the SRI spec's "agility" rule (https://www.w3.org/TR/SRI/#agility):
56+
# when a caller-supplied `integrity` value lists more than one algorithm, only
57+
# the strongest one present is checked.
58+
INTEGRITY_ALGORITHMS = {
59+
'sha512' => Digest::SHA512,
60+
'sha384' => Digest::SHA384,
61+
'sha256' => Digest::SHA256
62+
}.freeze
5163

5264
# Array of CSS files that have been loaded.
5365
attr_reader :loaded_uris
@@ -508,8 +520,9 @@ def parse_block_into_rule_sets!(block, options = {}) # :nodoc:
508520
# Subresource Integrity value (https://www.w3.org/TR/SRI/) -- e.g. the value of an
509521
# HTML <tt><link integrity="..."></tt> attribute -- and, for http(s) URIs, verifies the
510522
# fetched response body against it before the CSS is parsed. When the digest does not
511-
# match, the fetch is treated as a failure: an exception is raised if <tt>io_exceptions</tt>
512-
# is enabled, otherwise nothing is loaded. Ignored for <tt>file://</tt> URIs.
523+
# match, <tt>CssParser::IntegrityError</tt> (a subclass of <tt>RemoteFileError</tt>) is
524+
# raised if <tt>io_exceptions</tt> is enabled, otherwise nothing is loaded. Ignored for
525+
# <tt>file://</tt> URIs.
513526
#
514527
# Deprecated: originally accepted three params: `uri`, `base_uri` and `media_types`
515528
def load_uri!(uri, options = {}, deprecated = nil)
@@ -733,7 +746,7 @@ def read_remote_file(uri, integrity: nil) # :nodoc:
733746
end
734747

735748
if integrity && !integrity_matches?(res.body, integrity)
736-
raise RemoteFileError, uri.to_s if @options[:io_exceptions]
749+
raise IntegrityError, uri.to_s if @options[:io_exceptions]
737750

738751
return nil, nil
739752
end
@@ -743,6 +756,16 @@ def read_remote_file(uri, integrity: nil) # :nodoc:
743756
src.encode!('UTF-8', charset) if charset
744757

745758
[src, charset]
759+
rescue IntegrityError
760+
# Let the IntegrityError raised above propagate with its specific
761+
# class intact, rather than being downgraded to a generic
762+
# RemoteFileError by the catch-all below. Other RemoteFileErrors
763+
# raised within this method (e.g. from fetch_via_net_http on a
764+
# cross-scheme redirect) are intentionally still caught by the
765+
# catch-all: it discards their (potentially redirect-target-scoped)
766+
# message in favor of this method's own `uri`, which several
767+
# existing tests depend on.
768+
raise
746769
rescue
747770
raise RemoteFileError, uri.to_s if @options[:io_exceptions]
748771

@@ -761,18 +784,14 @@ def read_remote_file(uri, integrity: nil) # :nodoc:
761784
def integrity_matches?(body, integrity) # :nodoc:
762785
candidates = integrity.to_s.split.filter_map do |token|
763786
algorithm, value = token.split('-', 2)
764-
[algorithm, value] if algorithm && value && INTEGRITY_ALGORITHM_PRIORITY.include?(algorithm)
787+
[algorithm, value] if algorithm && value && INTEGRITY_ALGORITHMS.key?(algorithm)
765788
end
766789
return true if candidates.empty?
767790

768-
algorithm = candidates.map(&:first).min_by { |a| INTEGRITY_ALGORITHM_PRIORITY.index(a) }
791+
algorithms_present = candidates.map(&:first)
792+
algorithm = INTEGRITY_ALGORITHMS.each_key.find { |a| algorithms_present.include?(a) }
769793
expected_values = candidates.select { |a, _v| a == algorithm }.map { |_a, v| v }
770-
771-
digest_class = {
772-
'sha512' => Digest::SHA512,
773-
'sha384' => Digest::SHA384,
774-
'sha256' => Digest::SHA256
775-
}.fetch(algorithm)
794+
digest_class = INTEGRITY_ALGORITHMS.fetch(algorithm)
776795

777796
expected_values.include?(Base64.strict_encode64(digest_class.digest(body)))
778797
end

test/test_css_parser_integrity.rb

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,15 @@ def test_matching_sha512_integrity_loads_normally
7777
end
7878

7979
def test_mismatched_integrity_is_refused
80+
tampered = "#{sha('sha384')[0, 15]}not-the-real-digest-at-all=="
81+
assert_raises(CssParser::IntegrityError) do
82+
cp.load_uri!("#{@uri_base}/simple.css", integrity: tampered)
83+
end
84+
end
85+
86+
def test_mismatched_integrity_raises_a_subclass_of_remote_file_error
87+
# CssParser::IntegrityError must remain catchable by existing
88+
# `rescue RemoteFileError` callers.
8089
tampered = "#{sha('sha384')[0, 15]}not-the-real-digest-at-all=="
8190
assert_raises(CssParser::RemoteFileError) do
8291
cp.load_uri!("#{@uri_base}/simple.css", integrity: tampered)
@@ -96,7 +105,7 @@ def test_strongest_algorithm_wins_when_multiple_present_and_it_fails
96105
# here) is authoritative, so a right-but-weaker value must not mask
97106
# a wrong-but-stronger one.
98107
value = "#{sha('sha256')} sha512-#{Base64.strict_encode64('not the real digest')}"
99-
assert_raises(CssParser::RemoteFileError) do
108+
assert_raises(CssParser::IntegrityError) do
100109
cp.load_uri!("#{@uri_base}/simple.css", integrity: value)
101110
end
102111
end
@@ -122,7 +131,7 @@ def test_multiple_values_for_the_same_algorithm_accepts_any_match
122131
end
123132

124133
def test_unrecognized_algorithm_only_value_is_unverifiable_and_passes
125-
# md5 is not in INTEGRITY_ALGORITHM_PRIORITY. A value naming only an
134+
# md5 is not in INTEGRITY_ALGORITHMS. A value naming only an
126135
# unsupported algorithm can't be checked either way, so it's treated
127136
# as unverifiable rather than failing every such fetch.
128137
parser = cp

0 commit comments

Comments
 (0)