Files
chatwoot/spec/lib/safe_fetch_spec.rb
T
Shivam MishraandGitHub 8712879681 test: Stabilize SafeFetch spec against constant-identity flake (#14454)
`spec/lib/safe_fetch_spec.rb` has been flaking intermittently under
full-suite runs with errors like:

```
expected SafeFetch::FileTooLargeError, got #<SafeFetch::FileTooLargeError: exceeded 1048576 bytes>
```

The class name on both sides is identical — yet RSpec reports a
mismatch. This PR replaces the constant-identity assertions in this spec
with a name-based matcher so the comparison stops depending on the live
Class object's identity. We have made a similar fix earlier, but that
wasn't addressing the core of the issue in #14139.


**How to reproduce:** The flake only surfaces under load. Running the
spec in isolation almost always passes, here's a [run failing in
CI](https://github.com/chatwoot/chatwoot/actions/runs/25852294516/job/75961520248?pr=14370)

## What's actually going on

Three facts combine:

1. **Test env reloads classes.** `config/environments/test.rb` sets
`cache_classes = false` so Zeitwerk reloads autoloadable code on demand.
2. **`lib/` is on the reloadable autoload tree.**
`config/application.rb` adds `lib` to `eager_load_paths`, which (with
`eager_load = false` in test) makes it lazily loaded by Zeitwerk's
*main* (reloading) autoloader. `lib/safe_fetch/` lives under that
umbrella.
3. **RSpec's `raise_error(Klass)` snapshots the Class object.**
`raise_error` matcher captures `Klass` (a specific `Class` instance)
when the matcher is built. At raise time it compares with `Module#===`,
which is identity-based.

When the executor between examples triggers
`Rails.application.reloader.reload!`, Zeitwerk does
`remove_const(:SafeFetch)` and re-installs an autoload trigger. The next
access produces a **fresh** `Class` object for
`SafeFetch::FileTooLargeError` — same name, different identity. The
matcher's snapshot now points at the dead Class, and the live raise
produces the new one. Identity fails, even though the error is
semantically correct.

This bites SafeFetch specifically because:

- SafeFetch is a *namespace* with 7 nested error classes — one reload
invalidates all of them.
- The spec contains 14+ `raise_error(SafeFetch::Foo)` assertions — many
chances to land in a reload window.
- SafeFetch is exercised by request-driven code (webhook delivery,
avatar fetch). Earlier specs in the suite warm up the reloader
machinery, which then fires during this spec.

Other custom-error specs don't visibly flake because their consumers
`rescue ConstName => e` (dynamic class lookup at raise time, walks the
ancestor chain via `Module#===` *at the moment of raise*, with no
captured snapshot), rather than RSpec's snapshot-then-compare pattern.

## What this PR does

Adds a small local helper to the spec that matches by class name string,
not by Class identity:

```ruby
def safe_fetch_error(name, message_pattern = nil)
  satisfy("raise SafeFetch::#{name}#{" matching #{message_pattern.inspect}" if message_pattern}") do |error|
    error.class.name == "SafeFetch::#{name}" &&
      (message_pattern.nil? || message_pattern.match?(error.message))
  end
end
```

Every `raise_error(described_class::FooError)` becomes
`raise_error(safe_fetch_error('FooError'))`. Class names are strings;
string equality survives any number of reloads. The semantic assertion
("this raised the right kind of error") is preserved.

This is the pattern CLAUDE.md already endorses for this codebase:

> Specs in parallel/reloading environments: prefer comparing
`error.class.name` over constant class equality when asserting raised
errors.

## Why this approach (even though it's suboptimal)

To be honest with reviewers: **this is a workaround, not a root-cause
fix.** Future spec authors who use `raise_error(SafeFetch::Foo)` in
other files will hit the same flake. The "real" fix removes the failure
mode at the source rather than dodging it per-spec.

Here's the menu of options we considered and the tradeoffs:

### Option A — Spec-side name matcher (this PR)

- **What:** the helper above.
- **Pro:** one-file change, zero blast radius beyond the spec.
- **Pro:** matches CLAUDE.md's documented stance.
- **Con:** every new spec touching reloadable error classes needs to
remember this pattern. It's a discipline tax, not a structural fix.
- **Verdict:** chosen for this PR.

### Option B — Pin SafeFetch outside Zeitwerk

```ruby
# config/application.rb
Rails.autoloaders.main.ignore(
  Rails.root.join('lib/safe_fetch.rb'),
  Rails.root.join('lib/safe_fetch'),
)
require Rails.root.join('lib/safe_fetch')
```

- **Pro:** real root-cause fix. `SafeFetch::*` constants become
process-lifetime stable. The spec helper becomes unnecessary, every
assertion in any spec works correctly.
- **Pro:** preserves the public API exactly.
- **Con:** loses hot-reload for SafeFetch in dev (need server restart to
see edits).
- **Con:** modifies `application.rb`, which has cross-team review
weight.
- **Verdict:** rejected for scope reasons in this PR; a sensible
follow-up.

### Option C — Move error classes to a non-reloadable location

Define top-level error classes in
`config/initializers/safe_fetch_errors.rb`. Initializers run once at
boot, constants are never reloaded.

- **Pro:** identity-stable errors, no spec helper needed.
- **Con:** API change — `SafeFetch::FileTooLargeError` becomes
`SafeFetchFileTooLargeError` (or similar). Every consumer's `rescue`
clause has to update.
- **Con:** namespace pollution at top-level.
- **Verdict:** rejected. The constraint of touching initializers is what
motivated the simpler PR.

### Option D — Vendor SafeFetch as a path gem

Move `lib/safe_fetch/` → `vendor/gems/safe_fetch/` with a gemspec.
Bundler `require`s gems once; Zeitwerk has zero involvement.

- **Pro:** structurally the most correct fix. Same identity stability as
Option B, no Zeitwerk plumbing required.
- **Pro:** zero API change for consumers.
- **Con:** larger refactor (6+ files moved, gemspec authored,
Gemfile/Gemfile.lock updated).
- **Con:** SafeFetch becomes harder to iterate on in dev (gem-style
edit-then-restart loop).
- **Verdict:** rejected for scope in this PR; the cleanest long-term
home for a security primitive.

### Option E — Drop custom errors, return a `Result`

Refactor SafeFetch to yield a result hash (`{ ok: true, ... }` / `{ ok:
false, kind: :unsafe_url, ... }`) instead of raising for anticipated
outcomes. Failure kinds become symbols, which are interned for the
process lifetime.

- **Pro:** eliminates the failure mode at its true root — there are no
custom exception classes to reload, anywhere.
- **Pro:** forces explicit, exhaustive handling at every call site — a
feature for a security primitive.
- **Pro:** spec assertions become data assertions (`expect(result).to
match(ok: false, kind: :too_large)`), which are robust against any
reload, any reordering.
- **Con:** paradigm shift away from the exception-driven style used
everywhere else in the codebase.
- **Con:** every consumer rewrites their `rescue` block into
pattern-matched handling.
- **Verdict:** rejected for scope in this PR; the architecturally
cleanest answer if we ever revisit SafeFetch's API.

## Why we're shipping A despite knowing B–E are better

This flake has been chewing CI time intermittently and previously took a
partial fix (#14139). We need it stable *now*. Options B–E are real
refactors with broader review surface (`application.rb`, consumer code,
or the lib's public contract). Option A:

- Costs nothing — one helper, mechanical replacements.
- Doesn't preclude any of B/C/D/E later. The helper goes away cleanly
once a root-cause fix lands.
- Aligns with the project's documented guidance for this scenario.

When SafeFetch next gets a substantive change, that's the right moment
to fold in B or D. Until then, the spec is stable and CI gets its time
back.

## What changed

- `spec/lib/safe_fetch_spec.rb`: added `safe_fetch_error(name,
message_pattern = nil)` helper; converted all 14
`raise_error(described_class::FooError[, /regex/])` assertions to
`raise_error(safe_fetch_error('FooError'[, /regex/]))`.
2026-05-14 16:48:14 +05:30

456 lines
16 KiB
Ruby

require 'rails_helper'
# `SafeFetch.fetch` is a custom method that requires a block (it yields a Result);
# it is NOT `Hash#fetch`, so RuboCop's autocorrect to `fetch(url, nil)` would break the API.
# rubocop:disable Style/RedundantFetchBlock
RSpec.describe SafeFetch do
let(:url) { 'http://example.com/image.png' }
before do
allow(Resolv).to receive(:getaddresses).and_call_original
allow(Resolv).to receive(:getaddresses).with('example.com').and_return(['93.184.216.34'])
end
describe '.fetch' do
context 'with a valid public URL serving an image' do
before do
stub_request(:get, url).to_return(
status: 200,
body: File.new(Rails.root.join('spec/assets/avatar.png')),
headers: { 'Content-Type' => 'image/png' }
)
end
it 'yields a Result with tempfile, filename, and content_type' do
described_class.fetch(url) do |result|
expect(result.tempfile).to be_a(Tempfile)
expect(result.filename).to eq('image.png')
expect(result.content_type).to eq('image/png')
expect(result.tempfile.size).to be > 0
end
end
it 'closes the tempfile after the block returns' do
captured = nil
described_class.fetch(url) { |result| captured = result.tempfile }
expect(captured.closed?).to be true
end
it 'closes the tempfile even when the block raises' do
captured = nil
expect do
described_class.fetch(url) do |result|
captured = result.tempfile
raise 'boom'
end
end.to raise_error('boom')
expect(captured.closed?).to be true
end
it 'defaults the filename to a unique "download-<timestamp>-<hex>" when the URL has no path' do
bare_url = 'http://example.com'
stub_request(:get, bare_url).to_return(
status: 200,
body: File.new(Rails.root.join('spec/assets/avatar.png')),
headers: { 'Content-Type' => 'image/png' }
)
described_class.fetch(bare_url) do |result|
expect(result.filename).to match(/\Adownload-\d+-[a-f0-9]{8}\z/)
end
end
it 'requires a block' do
expect { described_class.fetch(url) }.to raise_error(ArgumentError, /block required/)
end
end
context 'with embedded basic auth credentials' do
it 'passes decoded credentials to the request' do
authenticated_url = 'http://user+avatar%40example.com:p%40ss+word%3A1@example.com/image.png'
stub_request(:get, url)
.with(headers: { 'Authorization' => 'Basic dXNlcithdmF0YXJAZXhhbXBsZS5jb206cEBzcyt3b3JkOjE=' })
.to_return(
status: 200,
body: File.new(Rails.root.join('spec/assets/avatar.png')),
headers: { 'Content-Type' => 'image/png' }
)
described_class.fetch(authenticated_url) do |result|
expect(result.content_type).to eq('image/png')
end
end
it 'preserves embedded credentials after a same-origin redirect removes userinfo' do
authenticated_url = 'http://user:pass@example.com/protected.png'
initial_url = 'http://example.com/protected.png'
redirect_url = 'http://example.com/public.png'
redirected_headers = nil
stub_request(:get, initial_url)
.with(headers: { 'Authorization' => 'Basic dXNlcjpwYXNz' })
.to_return(
status: 302,
headers: { 'Location' => '/public.png' }
)
stub_request(:get, redirect_url)
.with do |request|
redirected_headers = request.headers.transform_keys(&:downcase)
true
end
.to_return(
status: 200,
body: File.new(Rails.root.join('spec/assets/avatar.png')),
headers: { 'Content-Type' => 'image/png' }
)
described_class.fetch(authenticated_url) do |result|
expect(result.content_type).to eq('image/png')
end
expect(redirected_headers).to include('authorization' => 'Basic dXNlcjpwYXNz')
end
it 'strips embedded credentials on cross-origin redirects' do
authenticated_url = 'http://user:pass@example.com/protected.png'
initial_url = 'http://example.com/protected.png'
redirect_url = 'https://example.com/public.png'
redirected_headers = nil
stub_request(:get, initial_url)
.with(headers: { 'Authorization' => 'Basic dXNlcjpwYXNz' })
.to_return(
status: 302,
headers: { 'Location' => redirect_url }
)
stub_request(:get, redirect_url)
.with do |request|
redirected_headers = request.headers.transform_keys(&:downcase)
true
end
.to_return(
status: 200,
body: File.new(Rails.root.join('spec/assets/avatar.png')),
headers: { 'Content-Type' => 'image/png' }
)
described_class.fetch(authenticated_url) do |result|
expect(result.content_type).to eq('image/png')
end
expect(redirected_headers).not_to include('authorization')
end
end
context 'with URL validation' do
it 'raises InvalidUrlError for javascript: URLs' do
expect { described_class.fetch('javascript:alert(1)') { nil } }.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::InvalidUrlError')
end
end
it 'raises InvalidUrlError for mailto: URLs' do
expect { described_class.fetch('mailto:test@example.com') { nil } }.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::InvalidUrlError')
end
end
it 'raises InvalidUrlError for data: URLs' do
expect { described_class.fetch('data:text/html,<x>') { nil } }.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::InvalidUrlError')
end
end
it 'raises InvalidUrlError for ftp: URLs' do
expect { described_class.fetch('ftp://example.com/file') { nil } }.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::InvalidUrlError')
end
end
it 'raises InvalidUrlError for malformed URLs' do
expect { described_class.fetch('not_a_url') { nil } }.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::InvalidUrlError')
end
end
it 'raises InvalidUrlError when host is missing' do
expect { described_class.fetch('http:///path') { nil } }.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::InvalidUrlError')
end
end
end
context 'with SSRF protection (integration with ssrf_filter)' do
it 'raises UnsafeUrlError for private IP literals (10.x.x.x)' do
expect { described_class.fetch('http://10.0.0.1/secret') { nil } }.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::UnsafeUrlError')
end
end
it 'raises UnsafeUrlError for loopback addresses' do
expect { described_class.fetch('http://127.0.0.1/secret') { nil } }.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::UnsafeUrlError')
end
end
it 'raises UnsafeUrlError for AWS metadata IP (169.254.169.254)' do
expect { described_class.fetch('http://169.254.169.254/latest/meta-data/') { nil } }.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::UnsafeUrlError')
end
end
it 'raises UnsafeUrlError when hostname resolves to a private IP (DNS rebinding)' do
allow(Resolv).to receive(:getaddresses).with('evil.example.com').and_return(['10.0.0.1'])
expect { described_class.fetch('http://evil.example.com/secret') { nil } }.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::UnsafeUrlError')
end
end
end
context 'with content-type allowlist' do
it 'rejects text/html responses' do
stub_request(:get, url).to_return(
status: 200,
body: '<html></html>',
headers: { 'Content-Type' => 'text/html' }
)
expect { described_class.fetch(url) { nil } }.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::UnsupportedContentTypeError')
end
end
it 'rejects application/octet-stream responses' do
stub_request(:get, url).to_return(
status: 200,
body: 'x',
headers: { 'Content-Type' => 'application/octet-stream' }
)
expect { described_class.fetch(url) { nil } }.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::UnsupportedContentTypeError')
end
end
it 'allows video/mp4 responses' do
stub_request(:get, url).to_return(
status: 200,
body: File.new(Rails.root.join('spec/assets/avatar.png')),
headers: { 'Content-Type' => 'video/mp4' }
)
expect { described_class.fetch(url) { nil } }.not_to raise_error
end
it 'normalizes parameters and casing before yielding content_type' do
stub_request(:get, url).to_return(
status: 200,
body: 'x',
headers: { 'Content-Type' => 'IMAGE/PNG; charset=binary' }
)
described_class.fetch(url) do |result|
expect(result.content_type).to eq('image/png')
end
end
it 'allows exact content-type matches when prefixes are empty' do
pdf_url = 'http://example.com/file.pdf'
stub_request(:get, pdf_url).to_return(
status: 200,
body: 'pdf-data',
headers: { 'Content-Type' => 'application/pdf' }
)
expect do
described_class.fetch(
pdf_url,
allowed_content_type_prefixes: [],
allowed_content_types: ['application/pdf']
) { nil }
end.not_to raise_error
end
it 'rejects exact content-type mismatches when prefixes are empty' do
stub_request(:get, url).to_return(
status: 200,
body: 'x',
headers: { 'Content-Type' => 'image/webp' }
)
expect { described_class.fetch(url, allowed_content_type_prefixes: [], allowed_content_types: ['image/png']) { nil } }
.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::UnsupportedContentTypeError')
end
end
it 'rejects when the content-type header is missing' do
stub_request(:get, url).to_return(status: 200, body: 'x', headers: {})
expect { described_class.fetch(url) { nil } }.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::UnsupportedContentTypeError')
end
end
end
context 'with custom request options' do
let(:post_body) { { hello: 'world' }.to_json }
let(:headers) do
{
'Authorization' => 'Bearer test-token',
'Content-Type' => 'application/json'
}
end
it 'supports POST requests with custom headers when content-type validation is disabled' do
stub_request(:post, url)
.with(body: post_body, headers: headers)
.to_return(status: 200, body: '', headers: {})
expect do
described_class.fetch(
url,
method: :post,
body: post_body,
headers: headers,
validate_content_type: false
) { nil }
end.not_to raise_error
end
it 'preserves non-credential headers on cross-origin redirects' do
redirect_url = 'https://example.com/image.png'
redirected_headers = nil
headers = {
'Authorization' => 'Bearer test-token',
'Cookie' => 'session=test',
'Content-Type' => 'application/json',
'X-Chatwoot-Delivery' => 'test-uuid',
'X-Chatwoot-Signature' => 'sha256=test-signature'
}
stub_request(:post, url).to_return(
status: 307,
headers: { 'Location' => redirect_url }
)
stub_request(:post, redirect_url)
.with do |request|
redirected_headers = request.headers.transform_keys(&:downcase)
true
end
.to_return(status: 200, body: '', headers: {})
described_class.fetch(
url,
method: :post,
body: post_body,
headers: headers,
validate_content_type: false
) { nil }
expect(redirected_headers).to include(
'content-type' => 'application/json',
'x-chatwoot-delivery' => 'test-uuid',
'x-chatwoot-signature' => 'sha256=test-signature'
)
expect(redirected_headers).not_to include('authorization', 'cookie')
end
it 'raises UnsupportedMethodError for unsupported HTTP methods' do
expect { described_class.fetch(url, method: :options) { nil } }.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::UnsupportedMethodError')
end
end
end
context 'with body size cap' do
it 'honours a custom max_bytes argument' do
stub_request(:get, url).to_return(
status: 200,
body: 'xxxxx',
headers: { 'Content-Type' => 'image/png' }
)
expect { described_class.fetch(url, max_bytes: 2) { nil } }.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::FileTooLargeError')
end
end
it 'reads the default cap from GlobalConfigService MAXIMUM_FILE_UPLOAD_SIZE (matching Attachment#validate_file_size)' do
allow(GlobalConfigService).to receive(:load).and_call_original
allow(GlobalConfigService).to receive(:load).with('MAXIMUM_FILE_UPLOAD_SIZE', 40).and_return('1')
oversize = 'x' * (1.megabyte + 1)
stub_request(:get, url).to_return(
status: 200,
body: oversize,
headers: { 'Content-Type' => 'image/png' }
)
expect { described_class.fetch(url) { nil } }.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::FileTooLargeError')
end
end
it 'falls back to 40 MB when GlobalConfigService returns a non-positive value' do
allow(GlobalConfigService).to receive(:load).and_call_original
allow(GlobalConfigService).to receive(:load).with('MAXIMUM_FILE_UPLOAD_SIZE', 40).and_return('-10')
# 1 MB body should pass under the 40 MB fallback
stub_request(:get, url).to_return(
status: 200,
body: 'x' * 1.megabyte,
headers: { 'Content-Type' => 'image/png' }
)
expect { described_class.fetch(url) { nil } }.not_to raise_error
end
it 'allows uploads between the old hardcoded 10 MB and the configured limit (regression check)' do
# Default config is 40 MB; a 15 MB upload must succeed.
# This is the exact regression scenario: with the old hardcoded 10 MB cap,
# this would have failed even though direct file uploads of the same size succeed.
allow(GlobalConfigService).to receive(:load).and_call_original
allow(GlobalConfigService).to receive(:load).with('MAXIMUM_FILE_UPLOAD_SIZE', 40).and_return('40')
stub_request(:get, url).to_return(
status: 200,
body: 'x' * (15 * 1024 * 1024),
headers: { 'Content-Type' => 'image/png' }
)
expect { described_class.fetch(url) { nil } }.not_to raise_error
end
end
context 'with network failures' do
it 'maps Net::ReadTimeout to FetchError' do
stub_request(:get, url).to_raise(Net::ReadTimeout)
expect { described_class.fetch(url) { nil } }.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::FetchError')
end
end
it 'maps SocketError to FetchError' do
stub_request(:get, url).to_raise(SocketError.new('connection refused'))
expect { described_class.fetch(url) { nil } }.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::FetchError')
end
end
end
context 'with non-2xx upstream responses' do
it 'raises HttpError on non-2xx responses' do
stub_request(:get, url).to_return(status: 404, body: '', headers: {})
expect { described_class.fetch(url) { nil } }.to raise_error do |error|
expect(error.class.name).to eq('SafeFetch::HttpError')
end
end
end
end
end
# rubocop:enable Style/RedundantFetchBlock