feat(http-utils): add anonymousEndpoints override and drop GET /slack/events from the anonymous auth bypass - #1920
Conversation
…LN-39365) `ANONYMOUS_ENDPOINTS` is a library-level authentication bypass that every consumer of `authWrapper` inherits. It listed both GET and POST `/slack/events`. `GET /slack/events` is removed. Slack only ever POSTs events and interactive payloads, and a GET carries no body to sign, so that entry could never be backed by a Slack signature check -- it was purely an unauthenticated entry point. `POST /slack/events` stays, deliberately. The Slack signature check in the consuming service (spacecat-api-service's `slackSignatureWrapper`, added for VULN-39365) is mounted outside this wrapper and runs first, and Slack presents no SpaceCat credential, so removing the entry would 401 all legitimate Slack traffic. The security contract is now documented on the constant: an entry here means this library authenticates nothing, so the consumer MUST authenticate the route by other means. Also adds an optional `opts.anonymousEndpoints` override so a service that does not verify Slack request signatures can pass `[]` and opt out of the inherited bypass entirely. Defaults to the existing list, so this is backwards compatible. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Follow-up to the VULN-39365 change, addressing an architecture review finding. `anonymousEndpoints` previously fell back to the default list whenever the supplied value was not an array -- and a test locked that behaviour in. That is the wrong failure mode for security-sensitive configuration: a service passing a malformed value while trying to DISABLE the bypass would silently re-enable an unauthenticated `POST /slack/events` instead of failing. It now throws at wrapper-construction time when the option is present but is not an array of strings, so the misconfiguration surfaces at boot rather than as a silently widened attack surface. Omitting the option is unchanged and still yields the documented default. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
This PR will trigger a patch release when merged. |
dzehnder
left a comment
There was a problem hiding this comment.
Hey @JayKid,
Verdict: Request changes - correct, minimal, well-tested library fix; one security-contract doc line overstates what the opt-out actually delivers.
Complexity: MEDIUM - small 2-file diff, but a library-level auth-bypass surface every consumer inherits.
Changes: removes GET /slack/events from authWrapper's default anonymous bypass and adds an optional anonymousEndpoints override (2 files).
Must fix before merge
- [Important] JSDoc claims
anonymousEndpoints: []"disables the bypass entirely" -auth-wrapper.js:46(details inline). ThePOST /hooks/site-detection/*prefix andOPTIONSbypasses are separate unconditional clauses the option never touches, so a consumer that passes[]for lockdown still ships two unauthenticated bypasses. Doc-only fix acceptable. Flagged independently by 4 reviewers.
Non-blocking (5): suggestions and nits
- suggestion: add tests pinning that
anonymousEndpoints: []still bypassesOPTIONSandPOST /hooks/site-detection/*, and that a non-empty override drops the defaultPOST /slack/events(-> 401) - these are exactly the behaviors the corrected doc most needs to guarantee, and thePOST /hooks/site-detection/true-branch appears unexercised today. - suggestion: consider
feat(http-utils):rather thanfix:- this adds a new public option, and this package's changelog precedent (#1693, #1870, #1883) tags new wrapper paramsfeat; semantic-release bumps the version off the type prefix. - nit:
anonymousEndpointsvalidates array-of-strings but not the'METHOD /path'uppercase-method shape - a malformed entry like'post /slack/events'passes validation yet silently never matches (fails closed) -auth-wrapper.js:60. - nit:
src/auth/readme.mdand the packageREADME.mdversion-callout convention were not updated for the new option. - nit:
anonymousEndpoints = opts.anonymousEndpointsretains the caller's array reference; a defensive copy ([...]) after validation removes a mutation footgun.
FYI (verified): among the 4 consumers of this wrapper, only spacecat-api-service serves a /slack/events route, so removing GET from the default is backward-safe; companion adobe/spacecat-api-service#3229 removes that route there. The throw-at-construction validation on bad config is the right call for security-sensitive config.
| * @param {object} [opts] - options. | ||
| * @param {Array} [opts.authHandlers] - the authentication handler classes to try, in order. | ||
| * @param {string[]} [opts.anonymousEndpoints] - overrides the default set of routes that bypass | ||
| * authentication, as `'METHOD /path'` strings. Pass `[]` to disable the bypass entirely. A |
There was a problem hiding this comment.
[Important] This line reads "Pass [] to disable the bypass entirely," and the ANONYMOUS_ENDPOINTS contract above says an entry means the library "performs NO authentication for that route." But the runtime guard is:
if (anonymousEndpoints.includes(route)
|| route.startsWith('POST /hooks/site-detection/')
|| method.toUpperCase() === 'OPTIONS') {
return fn(request, context);anonymousEndpoints only governs the first clause. A service that passes [] believing it has locked down all anonymous access still exposes POST /hooks/site-detection/* and every OPTIONS request unauthenticated, and neither is reachable through this option.
On a VULN-remediation PR whose value is an accurate mental model of the auth surface, a doc that implies full lockdown is itself a small hazard. Suggest either:
- reword to: "Pass
[]to remove the route-based anonymous entries. Note:OPTIONSrequests andPOST /hooks/site-detection/*bypass authentication unconditionally and are not affected by this option." (and soften the "performs NO authentication" contract to name the full surface), or - route the hooks-prefix + OPTIONS bypasses through the same override so
[]truly means "authenticate everything."
…docs Addresses review feedback from @dzehnder on #1920. Must-fix: the JSDoc claimed `anonymousEndpoints: []` "disables the bypass entirely", but the option governs only the first clause of the guard. `OPTIONS` requests and `POST /hooks/site-detection/*` are separate unconditional clauses the option never touches, so a consumer passing `[]` for lockdown still ships two unauthenticated bypasses. On a VULN-remediation change whose whole value is an accurate mental model of the auth surface, a doc implying full lockdown is itself a hazard. The docs now name the complete unauthenticated surface, on both the ANONYMOUS_ENDPOINTS contract and the option itself. Behaviour is unchanged: routing the OPTIONS and hooks bypasses through the override would alter semantics for every consumer, which does not belong in a security fix. Also from the review: - Pin the corrected contract with tests: `anonymousEndpoints: []` still bypasses OPTIONS and `POST /hooks/site-detection/*` (the latter's true branch was previously unexercised), and a non-empty override drops the default `POST /slack/events`. - Defensively copy the supplied array so a caller mutating it after construction cannot widen the bypass, with a test. - Document the option in `src/auth/readme.md`, including that `[]` does not mean "authenticate everything" and that malformed config throws. 532 passing, 100% coverage on auth-wrapper.js; lint clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Entries are compared verbatim against `${METHOD.toUpperCase()} ${suffix}`, so an entry like
'post /slack/events' (lower-case method) or 'POST slack/events' (no leading slash) would
pass the array-of-strings check, never match anything, and silently fail closed. A service
that believed it had allowed a route would instead get 401s with no signal as to why.
That is the same class of silent misconfiguration the throw-on-non-array check already
guards against, so validate the shape too and name the offending entries in the error.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks @dzehnder — and noted that four reviewers landed on the same doc line independently, which is a fair signal it was actively misleading. Pushed in f6a8ef0 + 85b8010. Must fix — done (doc route)You offered two options; I took the reword rather than routing the OPTIONS and hooks bypasses through the override. Reasoning: making The docs now name the full unauthenticated surface in both places — the Non-blocking — addressed
Thanks also for checking the four consumers and confirming only spacecat-api-service serves a 534 passing, 100% statements/branches/functions/lines on |
dzehnder
left a comment
There was a problem hiding this comment.
Hey @JayKid,
Verdict: Approve - the blocking doc-contract finding is resolved and every non-blocking note was addressed. Re-review over 2 new commits since my last pass.
Complexity: MEDIUM - small auth-bypass surface, unchanged.
Previously flagged, now resolved
- [Important] The
anonymousEndpoints: []"disables the bypass entirely" overstatement is fixed - theANONYMOUS_ENDPOINTSdocblock now carries an explicit IMPORTANT note thatOPTIONSandPOST /hooks/site-detection/*are unconditional and not overridable, the JSDoc reads "Pass[]to remove the route-based entries... this does NOT authenticate everything," andsrc/auth/readme.mddocuments the option with the same two caveats. - Route-shape validation added (
ANONYMOUS_ROUTE_SHAPE) - a lower-case method or a missing leading slash now throws at construction instead of silently failing closed. - Defensive copy of
opts.anonymousEndpoints, so a caller mutating the array afterwards cannot widen the bypass. - Test coverage added for exactly the behaviors the contract needed pinned:
[]still bypassesOPTIONSandPOST /hooks/site-detection/*; a non-empty override drops the defaultPOST /slack/events; post-construction mutation cannot widen; and both malformed-shape cases throw.
Non-blocking and optional: the fix: vs feat: commit-type note still stands (this adds a new public option), but that is a changelog-signal preference, not a blocker.
Fixes the Slack integration to authenticate inbound requests. Details of
the underlying
report are in **VULN-39365** (Jira) — deliberately not restated here, as
this is a public
repository and the fix is not yet deployed.
## Problem
`/slack/events` was not verifying that a request actually came from
Slack:
- `POST|GET /slack/events` is in `ANONYMOUS_ENDPOINTS` in
spacecat-shared's `authWrapper`,
so the authentication manager never runs for it.
- Bolt's signature check lives in its **HTTP receiver**, which this
service bypasses by
calling `app.processEvent({ body, ack })` on an already-parsed body. The
`signingSecret`
passed to `new App(...)` in `src/controllers/slack.js` was therefore
configured but never
exercised.
Net effect: any unauthenticated caller could reach every registered
Slack command, action
and view handler with a payload of their choosing.
## Fix
**1. `slackSignatureWrapper`
(`src/support/slack/signature-wrapper.js`)** — verifies Slack's
v0 HMAC-SHA256 signature over the raw body plus a ±5 minute timestamp
window, with a
timing-safe compare.
Placement is the load-bearing detail. It is declared immediately before
`enrichPathInfo` in
the `.with()` chain, which (helix-shared-wrap runs the last `.with()`
outermost) puts it
directly **after** `enrichPathInfo` — so `pathInfo.headers` is populated
— and **before**
`multipartFormData`/`bodyData`, so the body is still unread. Verified
execution order:
```
enrichPathInfo -> slackSignatureWrapper -> multipartFormData -> bodyData
```
It reads the body via `request.clone().text()`, never `request.text()`,
because `bodyData`
consumes the body downstream. A test asserts `context.data` is still
populated afterwards.
Fails closed on every path: missing, malformed, stale, future-dated,
oversized or
mismatching input, **and** on a missing `SLACK_SIGNING_SECRET`. Bodies
over 1 MiB are
rejected before any HMAC work (pre-auth exhaustion guard, mirroring
`github-webhook-hmac-handler.js`).
**2. Slack file-host allowlist (`src/utils/slack/file-url.js`)** —
defence in depth. Two
call sites attach the bot token to an outbound fetch of a
payload-supplied URL
(`utils/slack/base.js#fetchFile`,
`slack/commands/toggle-site-audit.js`). Both now require a
Slack-owned https host first. Note `support/url-safety.js#isSafeDomain`
is a *denylist* of
private ranges and does not stop an attacker-controlled public host, so
a positive allowlist
is required here.
**3. `GET /slack/events` removed** — Slack only ever POSTs, and a GET
carries no body to
sign, so it could never be verified. Removed from `routes/index.js`,
`required-capabilities.js` and `facs-capabilities.js` (the route-drift
guards caught the
last two).
## Testing
- 25 new tests for the wrapper: valid pass-through (JSON,
form-urlencoded, multi-byte
UTF-8), forged/absent/malformed signature, tampered body, replay window
edges, oversized
body, missing secret, unreadable body, non-Slack route pass-through, and
a check that
nothing sensitive is logged.
- 24 new tests for the allowlist, including userinfo
(`https://files.slack.com@evil.test/`)
and suffix-confusion (`files.slack.com.evil.test`) bypasses.
- Test fixtures that pointed `url_private` at `example.com` now use
`files.slack.com`, which
is what Slack actually sends.
- Full suite: **18152 passing, 0 failing**. Lint, `type-check:base` and
`type-check:strict`
all clean.
## Deploy notes
⚠️ **`SLACK_BOT_TOKEN` and `SLACK_SIGNING_SECRET` must be rotated in
Vault.** The report
shows the token was used against internal channels, so it is compromised
independently of
this fix. Rotating the signing secret is safe with this change deployed
(the wrapper reads
it per request from `context.env`).
**Fastly header pass-through: verified.** Checked the VCL for service
`DlxppS2VoAizEqJ9bPasq6`
(`spacecat-infrastructure/fastly/vcl/aso-prod/`). The only `unset
req.http.*` in the whole
service is `X-Spacecat-Cross-Product-Context` (anti-spoofing on an
internal header) — there is
no header allowlist and no stripping of client headers, so
`X-Slack-Signature` and
`X-Slack-Request-Timestamp` reach the origin intact. Nothing in the VCL
mutates the request
body (which would otherwise break the HMAC), and `recv/200` does
`return(pass)` for this path,
so Slack traffic bypasses caching entirely. Caveat: that VCL is a backup
last synced
2026-07-30 (aso-prod v120), so a live re-check is cheap insurance.
Still worth watching 401s on `POST /slack/events` immediately after
deploy.
Depends on / related: adobe/spacecat-shared#1920
## Review follow-ups
A multi-persona review (architect / security / DBA / tester agents, run
independently against
the full diff) produced no Critical findings and no bypass of the
signature check or the file
allowlist. Six Major findings were raised and **all six are fixed in
this PR** — the notable
ones: a third anonymous-route registry (`AccessControlUtil`) still
listed the removed `GET`
route; `OPTIONS` preflight was being turned into a 401; and there was no
test proving the
wrapper is actually *wired into* `main` (a mis-ordered wrapper would
have shipped green).
Two findings are **deliberately deferred**, tracked, and should inform
the merge decision:
| Ticket | Finding | Why not here |
|---|---|---|
| [SITES-51204](https://jira.corp.adobe.com/browse/SITES-51204) |
**Signature ≠ authorization.** Any Slack workspace user who can see an
approval button can still trigger `approveOrg` and reassign a site's IMS
org (`approve-org.js:34-51` uses `body.user` only for the reply text). |
Choosing a policy (named approver / admin-only / usergroup) is a product
decision; guessing risks breaking the ops team's daily Slack workflow.
This PR still reduces the population from *the entire internet,
unauthenticated* to *your Slack workspace*. |
| [SITES-51205](https://jira.corp.adobe.com/browse/SITES-51205) |
**Replay within the 5-minute window** can repeat non-idempotent writes
(`set-live-status` toggles rather than setting a target state). |
Pre-existing, not a regression; replay now requires capturing a
genuinely signed request. Needs a durable TTL'd store — too much to land
unreviewed in a blocker. |
The ±300s window matches Slack's own Bolt SDK
(`@slack/bolt/dist/receivers/verify-request.js`,
`requestTimestampMaxDeltaMin = 5`), and is stricter in two ways: it also
rejects future-dated
timestamps, and its `/^\d+$/` check is effective where Bolt's
`Number.isNaN(string)` guard is not.
Also noted for incident response (pre-existing, not introduced here):
`approveOrg` does not call
`reparentSiteProject`, so a pre-fix forged call could have left a site
whose `organizationId`
differs from its project's. Worth auditing for that mismatch.
## Change Management
```yaml
cm-assessment: v1
changeType: standard
impact: unnoticeable
risk: minor
scope: multi-repo
relatedPRs: ["adobe/spacecat-shared#1920"]
rationale: "Adds Slack request-signature verification to a previously unauthenticated endpoint, plus a file-host allowlist. Fails closed and only ever restricts access, so it cannot widen exposure. The realistic failure mode is over-rejection, which would break the internal SpaceCat Slack bot -- internal ops tooling, no customer-facing surface -- and is immediately reversible by redeploying the previous release. No schema or data operation, no external API contract change."
recommendations: "Rotate SLACK_BOT_TOKEN and SLACK_SIGNING_SECRET in Vault (required, independent of this fix). Fastly header pass-through verified against the aso-prod VCL (no header stripping, no body mutation). Watch 401 rate on POST /slack/events after deploy."
backout: "Redeploy the previous release; no data migration or state change to unwind."
```
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## [@adobe/spacecat-shared-http-utils-v1.37.0](https://github.com/adobe/spacecat-shared/compare/@adobe/spacecat-shared-http-utils-v1.36.0...@adobe/spacecat-shared-http-utils-v1.37.0) (2026-09-08) ### Features * **http-utils:** add anonymousEndpoints override and drop GET /slack/events from the anonymous auth bypass ([#1920](#1920)) ([041fa0c](041fa0c)), closes [adobe/spacecat-api-service#3229](adobe/spacecat-api-service#3229) [adobe/spacecat-api-service#3229](adobe/spacecat-api-service#3229)
|
🎉 This PR is included in version @adobe/spacecat-shared-http-utils-v1.37.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [1.798.0](v1.797.1...v1.798.0) (2026-09-08) ### Bug Fixes * **security:** scope AsyncJob readers to the caller (SEC-5 IDOR) ([#3203](#3203)) ([#3222](#3222)) ([54f2c34](54f2c34)), closes [Hi#level](https://github.com/Hi/issues/level) * **security:** verify Slack request signatures on /slack/events ([#3229](#3229)) ([933d0e8](933d0e8)), closes [adobe/spacecat-shared#1920](adobe/spacecat-shared#1920) * **serenity:** brand-scope topic prompts and topics with CBF_brand | LLMO-7443 ([#3230](#3230)) ([00f64ae](00f64ae)) * **serenity:** CSV re-import updates prompt tags instead of stacking them | LLMO-7422 ([#3219](#3219)) ([e5b9cd9](e5b9cd9)), closes [#3210](#3210) * set CloudFront connector presign TTL to 12h | LLMO-7074 ([#3211](#3211)) ([1592259](1592259)) ### Features * **llmo-akamai:** single-marker failover cleanup for multi-hop OAE routing ([#3209](#3209)) ([e195346](e195346))
Companion to adobe/spacecat-api-service#3229. Context is in VULN-39365 (Jira) —
deliberately not restated here, as this is a public repository and the fix is not yet
deployed.
Problem
ANONYMOUS_ENDPOINTSinauthWrapperis a library-level authentication bypass that everyconsumer of this wrapper inherits. It listed both
GETandPOST /slack/events, so anyservice mounting
authWrappergets an unauthenticated/slack/eventswhether or not itdefends that route by other means.
Changes
1.
GET /slack/eventsremoved. Slack only ever POSTs events and interactive payloads,and a GET carries no body to sign — so that entry could never be backed by a signature check
and existed purely as an unauthenticated entry point.
2.
POST /slack/eventsdeliberately stays. Worth being explicit, since the obviousinstinct is to remove both:
slackSignatureWrapper)is mounted outside this wrapper and therefore runs before it.
Slack request 401.
The security contract is now documented on the constant: an entry here means this library
authenticates nothing, so the consumer MUST authenticate the route by other means.
3. New optional
opts.anonymousEndpoints. Lets a consumer override the default list —[]opts out of the inherited bypass entirely. A service that does not verify Slacksignatures should pass
[]rather than silently inheriting a route it is not defending.Defaults to the existing list, so this is backwards compatible.
Deploy ordering
None required — the two PRs are independent in either order:
GET /slack/eventsis already gone from its route table (404).authentication manager and 401s, which is strictly safer than today.
Testing
npm test -w packages/spacecat-shared-http-utils: 527 passing,auth-wrapper.jsat100% statements / branches / functions / lines. Lint clean.
New tests cover:
POST /slack/eventsstill anonymous;GET /slack/eventsnow reaches theauth manager and 401s; an
anonymousEndpoints: []override disabling the bypass; anoverride naming a different route; and a non-array override falling back to the default.
Change Management