Captain custom tools configured with API key authentication now send the
configured key in the requested HTTP header. Existing tools begin
working without needing to be recreated or reconfigured, while
credentials remain protected across redirects.
## How to reproduce
1. Create a Captain custom tool using API Key authentication.
2. Configure `X-API-Key` as the header name and save the tool.
3. Invoke the tool and inspect the incoming request.
4. Before this change, the API key header is absent; after this change,
the configured endpoint receives it.
## What changed
The UI persists API key authentication as `name` and `key`, but the
request builder also required an unused `location: header` property. The
request builder now treats API key authentication as header-based,
matching the only mode exposed by the UI.
Custom authentication headers are also registered as sensitive with
`SafeFetch`. They are retained for the configured endpoint and
same-origin redirects, but stripped when a redirect crosses origins to
prevent credential leakage. Factory, request, and redirect specs cover
the real UI payload and both public and private-network fetch paths.
Self-hosted installations can now opt SafeFetch into private-network
access after SSRF hardening. The default remains unchanged: private IP
destinations are blocked unless the instance owner explicitly enables
private-network requests with `SAFE_FETCH_ALLOW_PRIVATE_NETWORK=true`.
Fixes https://linear.app/chatwoot/issue/CW-7131
Fixes https://github.com/chatwoot/chatwoot/issues/14489
Fixes https://github.com/chatwoot/chatwoot/issues/14494
## How to use
For self-hosted installations that need API inbox webhooks, or other
SafeFetch-backed requests, to call trusted private services, enable
private-network access with a single environment variable:
```bash
SAFE_FETCH_ALLOW_PRIVATE_NETWORK=true
```
This is disabled by default. Enable it only when the instance owner
controls the deployment network and trusts the configured URLs.
`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/]))`.
This routes external downloads used by webhook fetch used by macros and
acutomations through SafeFetch. It closes the SSRF exposure from raw
Down.download paths, preserves provider-specific auth and header flows,
and adds regression coverage for blocked internal URLs plus
authenticated downloads.
Fixes # (issue):
[CW-6940](https://linear.app/chatwoot/issue/CW-6940/ssrf-via-webhooksautomationmacros-non-upload-non-avatar)
This routes external downloads used by avatar sync through SafeFetch. It closes the SSRF exposure from raw Down.download paths, preserves provider-specific auth and header flows, and adds regression coverage
for blocked internal URLs plus authenticated downloads.
Fixes # (issue): [CW-6931](https://linear.app/chatwoot/issue/CW-6931/avatarwidget-url-ssrf-downdownload-unprotected-unauth)
