Match If-None-Match per RFC 7232: honor '*', comma-separated lists, and weak comparison - #2517

If-None-Match is compared byte-for-byte, ignoring *, lists, and weak comparison

Verification caveat, stated up front: I could not run this. Ruby is not installed on the machine I worked on (ruby, gem, and bundle are all absent, and I checked the usual install locations), so the regression specs below have not been executed and I have no before/after test counts. The defect is provable by reading Rack::ConditionalGet — I trace it below — but ruby -Ilib -Itest test/spec_conditional_get.rb still needs to be run before this is trusted. I've written the specs regardless.

Description

Rack::ConditionalGet#etag_matches? compares the whole If-None-Match field value against the response ETag with plain String#==:

def etag_matches?(none_match, headers)
  headers[ETAG] == none_match
end

Why that's wrong — three RFC 7232 §3.2 violations

1. If-None-Match: * is never honoured. * is a legal field value meaning "matches any current representation". Rack evaluates "W/\"abc\"" == "*" → false, so it returns 200 with the full body instead of 304. Every HTTP cache revalidating a resource for which it stored no validator hits this. Since ConditionalGet only reaches this code when status == 200 (conditional_get.rb:33), a current representation is guaranteed to exist, so * is unconditionally a match.

2. If-None-Match is a list. The grammar is #entity-tag or a comma-separated list. For If-None-Match: "4321", W/"1234" against ETag: W/"1234", Rack evaluates "W/\"1234\"" == "\"4321\", W/\"1234\"" → false. Only an accidental exact whole-header match ever yields a 304.

3. Strong comparison is used where weak is required. RFC 7232 §3.2: "A recipient MUST use the weak comparison function when comparing entity-tags for If-None-Match." Rack compares strings byte-for-byte, so If-None-Match: W/"abc" does not match ETag: "abc".

The in-code comment at line 53 already says "per RFC 7232", and CHANGELOG.md:774 records that maintainers treat this file as RFC-7232-conformant, so the current behaviour contradicts the project's own stated intent.

The fix

def etag_matches?(none_match, headers)
  # "*" matches as long as the server has a current representation for the
  # target resource, which is guaranteed here since the status is 200.
  return true if none_match.strip == '*'

  etag = headers[ETAG]
  return false if etag.nil?

  none_match.split(',').any? { |tag| weak_etag_match?(tag.strip, etag) }
end

# Weak comparison of two entity-tags, which only compares the opaque values
# and ignores the weakness prefix.
def weak_etag_match?(a, b)
  strip_weak_prefix(a) == strip_weak_prefix(b)
end

def strip_weak_prefix(etag)
  etag.start_with?('W/') ? etag[2..-1] : etag
end

Tests

Six new it blocks in test/spec_conditional_get.rb: * with an ETag, * with no ETag, an entity-tag inside a comma-separated list, weak comparison, a negative case where no list member matches (→ 200), and a negative case with no response ETag and not * (→ 200).

I also checked the four pre-existing If-None-Match specs still hold under the new logic: '1234' vs ETag: "1234" is weak-equal → 304, and '4321' vs ETag: "1234" → 200. Please confirm this when the suite is run — it is the main compatibility risk in the change.

Upstream status

gh search prs/issues "conditional get" --repo rack/rack --state open returns 0 results, and the 50 open issues and 26 open PRs contain nothing for conditional GET.

Two real neighbouring defects I left alone: Rack::MediaType.params raising NoMethodError on "text/plain;;charset=utf8" is already open upstream as PR #2508, and tag_multipart_encoding comparing TEXT_PLAIN == type_subtype case-sensitively is adjacent to open PR #2514. I also noted that Rack::Utils.q_values returns [nil, 1.0] for an empty Accept element, which makes Rack::ShowExceptions#accepts_html? — the 500 path — crash on an attacker-controlled Accept header; that is a genuine robustness bug but a separate change.