feat(auth): remove catch-all LegacyApiKeyHandler; S2S is the replacement (SITES-34224)

[ravverma]

Summary

Removes the catch-all LegacyApiKeyHandler from AUTH_HANDLERS in src/index.js. S2S authentication is now the supported path for all service integrations.

What changes:

• LegacyApiKeyHandler removed from the AUTH_HANDLERS array and import
• RouteScopedLegacyApiKeyHandler remains — it is locked to exactly two routes whose external callers cannot be onboarded as IMS S2S consumers:
  • POST /event/fulfillment (external fulfillment webhook, e.g. SFDC/commerce provisioning)
  • POST /slack/channels/invite-by-user-id (external Slack integration)
  • This list is frozen — no new routes will be added; every new service must use S2S
• auth-handlers-order.test.js updated: test now asserts LegacyApiKeyHandler is absent from AUTH_HANDLERS (regression guard so it cannot be re-added accidentally)
• Stale TODO comment removed from route-scoped-legacy-api-key-handler.js

Why now: SITES-34224 tracks removal of legacy API key auth as a security requirement. S2S is fully operational and is the replacement for all consumers. The RouteScopedLegacyApiKeyHandler decision is documented in platform/decisions/route-scoped-legacy-api-key-handler.md.

Related

• Security ticket: https://jira.corp.adobe.com/browse/SITES-34224
• Decision doc: mysticat-architecture/platform/decisions/route-scoped-legacy-api-key-handler.md
• Companion PR: adobe/spacecat-auth-service feat/remove-legacy-api-key-handler

Test plan

• auth-handlers-order.test.js — asserts LegacyApiKeyHandler absent, RouteScopedLegacyApiKeyHandler present
• route-scoped-legacy-api-key-handler.test.js — existing tests unchanged (scoped handler behaviour is identical)
• POST /event/fulfillment and POST /slack/channels/invite-by-user-id still authenticate via legacy API key through the scoped handler
• All other routes reject legacy API key requests (no handler in chain accepts them)

🤖 Generated with Claude Code

[github-actions]

This PR will trigger a minor release when merged.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[MysticatBot-Dev]

Hey @ravverma,

Strengths

• Excellent change discipline (src/index.js): The diff is tightly scoped - removes exactly one handler from the array, updates comments to reflect the new state, and nothing else. No scope creep.
• Regression guard (test/auth-handlers-order.test.js:77-88): Converting the ordering test into an absence assertion (legacyIdx === -1) is the right pattern. The assertion message includes the ticket number (SITES-34224) for future maintainers.
• Attack surface reduction (src/index.js:421): Removing the catch-all eliminates the broadest auth vector. Any valid legacy API key could previously authenticate to any route - now auth is constrained to JWT, IMS, scoped API key, or the two frozen route-scoped paths.
• Stale TODO cleanup (src/support/route-scoped-legacy-api-key-handler.js:49-52): Removing the "TODO: Once the parent is removed" comment in the same PR that performs the removal. No TODO rot.
• Frozen-list documentation (src/index.js:404-412): The updated comment block for RouteScopedLegacyApiKeyHandler is clear, states the policy, and names the two routes explicitly.
• Test fix (test/index.test.js:24,55): Adding the authWrapperStub is the correct fix so esmock loads without the real auth handler import tree.

Issues

Important (Should Fix)

CLAUDE.md "Authentication precedence" is now stale. The repo's CLAUDE.md states:

"Authentication precedence (checked in order): 1. JWT with scopes, 2. Adobe IMS, 3. Scoped API Key (fine-grained permissions), 4. Legacy API Key (backward compatibility)"

After this PR, the catch-all LegacyApiKeyHandler no longer exists. Item 4 implies a global fallback that no longer applies to arbitrary routes. A reader (human or AI agent) following this guidance would incorrectly believe any route can authenticate via a legacy API key.

How to fix: Update the CLAUDE.md "Authentication precedence" list to either remove item 4 or replace it with "4. Route-Scoped Legacy API Key (POST /event/fulfillment and POST /slack/channels/invite-by-user-id only)" to reflect the frozen scope.

Minor (Nice to Have)

src/support/route-scoped-legacy-api-key-handler.js:48 - The class JSDoc references "LegacyApiKeyHandler sets the handler name to 'legacyApiKey'" without clarifying that this is the parent class used only for inheritance. Now that the parent is no longer in the handler chain itself, a one-word clarification ("the parent class LegacyApiKeyHandler") would prevent confusion - though the extends LegacyApiKeyHandler on the next line makes it clear enough.

Recommendations

• The auth handler chain now has six handlers plus the outer s2sAuthWrapper, with four being legacy paths with documented removal timelines. Consider maintaining a lightweight "auth handler lifecycle" table in the decision doc or index.js comments that tracks each handler's purpose, planned end-of-life trigger, and the owning ticket.
• Once this lands and stabilizes, consider rotating or revoking legacy API keys held by consumers now on S2S. A compromised legacy key still authenticates the two frozen routes; revoking migrated consumers' keys reduces residual exposure. (Out of scope for this PR.)

Assessment

Ready to merge? With fixes
Reasoning: This is a clean, security-positive removal with proper regression tests and passing CI. The only blocking concern is that the CLAUDE.md authentication documentation is now actively misleading - a one-line fix that should ship with this PR to avoid stale guidance.

Next Steps

1. Update the CLAUDE.md "Authentication precedence" section to reflect that legacy API key auth is now route-scoped, not global.
2. The Minor JSDoc note is optional.

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 46s | Cost: $3.48 | Commit: 06e76dcac1e63f726237cbc93d2d631a3cc33d8e
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[MysticatBot]

Hey @ravverma,

Strengths

• Excellent change discipline (src/index.js): The diff removes exactly one handler from the array, updates all surrounding comments to reflect the new state, removes the stale import, and nothing else. No scope creep.
• Attack surface reduction (src/index.js:421): Removing the catch-all eliminates the broadest auth vector in the system. Previously any valid legacy API key granted access to every route; now legacy keys are confined to exactly two endpoints.
• Regression guard (test/auth-handlers-order.test.js:77-88): The absence assertion (legacyIdx === -1) with ticket reference prevents accidental re-addition. This is the right structural enforcement pattern.
• Frozen-list policy documented at point of use (src/index.js:404-412): The updated comment block names the two routes, states the policy, and references the ticket. Future engineers do not need a separate design doc to understand the constraint.
• Stale TODO removed (src/support/route-scoped-legacy-api-key-handler.js:44-52): The TODO referencing this removal as a "planned follow-up" is cleaned up in the same PR that performs the removal. No TODO rot.
• Test infrastructure fix (test/index.test.js:24,55): Adding the authWrapperStub as a passthrough is the correct minimal fix for esmock now that the import tree changes.
• Previously flagged issues addressed: CLAUDE.md authentication precedence updated to reflect route-scoped-only (commit 098d1cc). JSDoc in route-scoped-legacy-api-key-handler.js clarified to name the parent class explicitly.

Issues

Minor (Nice to Have)

src/support/route-scoped-legacy-api-key-handler.js:44 - The JSDoc mentions sub-path matching ("e.g. POST /event/fulfillment/xxxx"). If isScopedRoute uses prefix matching, it would be worth confirming the route matching is normalized and does not allow path traversal sequences (e.g. POST /event/fulfillment/../admin/something) to extend scope beyond the two documented routes. Not a finding against this PR (the scoped handler is unchanged), but worth a quick verification given this handler is now the sole remaining legacy-key surface.

Recommendations

• The RouteScopedLegacyApiKeyHandler still inherits from LegacyApiKeyHandler (the class). Now that it is the only consumer, consider in a future PR whether collapsing the inheritance into a standalone handler would simplify the class hierarchy. Not a blocker here.
• Consider adding a structured log line when RouteScopedLegacyApiKeyHandler successfully authenticates a request. This gives visibility into legacy key usage on the two frozen routes and supports a future decision about when those routes can also migrate.

Assessment

Ready to merge? Yes
Reasoning: This is a clean, security-positive removal with proper regression tests and passing CI. The architecture is strictly simpler after the change - fewer handlers in the chain, tighter scope on legacy auth, documentation updated in lockstep with code. All prior review findings have been addressed.

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 33s | Cost: $2.24 | Commit: ace1a5184e514d0faa093b36160c43babe7bff1e
If this code review was useful, please react with 👍. Otherwise, react with 👎.}