Skip to content

feat(aso-overlay): accept AEM CS preview-tier -prev service suffix#2832

Merged
miakobchuk merged 4 commits into
mainfrom
fix/sites-48140-aso-overlay-preview-tier
Jul 16, 2026
Merged

feat(aso-overlay): accept AEM CS preview-tier -prev service suffix#2832
miakobchuk merged 4 commits into
mainfrom
fix/sites-48140-aso-overlay-preview-tier

Conversation

@abhishekgarg18

@abhishekgarg18 abhishekgarg18 commented Jul 16, 2026

Copy link
Copy Markdown
Member

Defense-in-depth positioning (2026-07-16). Cornel Isbiceanu (Skyline dispatcher team) noted on Slack that in steady state preview pods aren't supposed to be polling this endpoint — the ASO API key is only injected in publish pods, not preview. This PR is not meant to normalize preview traffic; it's meant to make api-service resilient to whatever URL shape the dispatcher sends. Rationale:

  1. Auth is still required. Accepting the -prev URL shape at the route regex does not weaken auth: AsoOverlayKeyHandler still requires a valid X-ASO-API-Key regardless of URL shape. Preview pods without the key are still rejected — just as 401 from the origin handler instead of Fastly edge, or 401 from Fastly edge for canonical URLs. Same posture, different layer.
  2. Fastly edge remains the primary signal. Skyline's diagnostic surface for "preview pods are misconfigured" is the Fastly edge log (401 rate by URL). That signal is unchanged by this PR — we only affect what happens if a request reaches origin. Preview pods still get 401'd at edge today because they lack the key; that alarm still fires.
  3. Zero-cost when preview eventually gets the key. If Skyline / AEM CS ever provision the API key on preview pods (planned or not), this code just works with no follow-up patch. Symmetric to how we already handle the same overlay across dev/stage/prod tiers.
  4. Future-proof against dispatcher changes. If Skyline's URL construction changes to strip -prev upstream, this PR becomes a no-op. If it doesn't, origin behavior is deterministic instead of a shape-based 400.

This is a resilience-hardening PR, not a functional change.

Summary

AEM Cloud Service preview publish pods send AEM_SERVICE=cm-pN-eN-prev. Skyline dispatcher builds the overlay URL from that env var verbatim, so preview traffic hits us with /config/cm-pN-eN-prev/redirects.txt. Two api-service regexes reject the shape today:

  1. AsoOverlayKeyHandler.OVERLAY_ROUTE does not match -prev, so the request falls through the auth chain and 401s at authWrapper before the controller runs.
  2. Even with correct edge auth, RedirectsController.SERVICE_RE would 400 the shape.

Mystique writes exactly one overlay per (program, environment) — publish and preview tiers share it. This PR accepts the suffix at both boundaries and strips it before all downstream lookups (Site lookup, S3 key, Surrogate-Key). Origin-side stripping avoids requiring a Fastly VCL change and lands the fix on our side alone.

Auth model is preserved end-to-end. AsoOverlayKeyHandler still requires X-ASO-API-Key regardless of URL shape — accepting -prev at the route regex does not weaken any authentication guarantee. A future auth-chain regression that accidentally auto-allowed keyless overlay-route requests would be the actual security problem, and that risk exists for cm-pN-eN as much as for cm-pN-eN-prev.

Fastly cache footprint

Fastly caches on raw URL, so preview and publish create two separate cache entries per tenant. Trade-off is acceptable because:

  1. Both entries share Surrogate-Key: aso-overlay-<canonical>, so mystique's purge-on-Deploy invalidates both in a single call — no divergence risk.
  2. Both fetch the same S3 object (canonical), so upstream is not doubled beyond initial per-URL fills — subsequent polls are cache hits.
  3. At 10k tenants × ~20% preview coverage the extra entries are cheap.
  4. No cache-poisoning risk — publish and preview have distinct cache keys.

A future normalization at Fastly VCL (regsub in vcl_recv before cache-key derivation) would collapse to one entry per tenant, but that requires Fastly-admin territory. Origin-side is the pragmatic bridge.

Changes

  • SERVICE_RE (src/controllers/redirects.js:43): accept optional trailing -prev, case-sensitive
  • RedirectsController.getRedirects: derive canonicalService from captured program/environment ids; use it for S3 key + Surrogate-Key on both 200 and 304 branches
  • AsoOverlayKeyHandler.OVERLAY_ROUTE (src/support/aso-overlay-key-handler.js:33): accept -prev so the auth handler doesn't 401 preview traffic before it reaches the controller
  • s3DurationEmitted idempotency guard (audit follow-up): fix double-emission of AsoOverlayS3ReadDurationMs when the body stream throws post-send(). Test in redirects.test.js asserts exactly one emission with SUCCESS result (first-writer-wins).
  • OpenAPI Surrogate-Key documentation (audit follow-up): document the header on both 200 and 304 response schemas so the mystique purge-on-Deploy contract is discoverable
  • Unit tests: -prev happy path, 304 on -prev, malformed variants still 400 (-preview, -PREV, -prev-2, trailing hyphen), auth handler accepts -prev
  • IT test: -prev end-to-end against the seeded canonical overlay
  • OpenAPI: pattern + description updated to document optional -prev

Jira: SITES-48140

Test plan

  • 58 unit tests in redirects.test.js pass, all auth handler tests green
  • Full suite green (2 pre-existing flakes unrelated to this PR; pass in isolation)
  • Lint + type-check clean
  • OpenAPI validates + docs rebuilt
  • Post-deploy: verify -prev URL returns the same canonical body + surrogate-key: aso-overlay-cm-pN-eN header via curl on dev
  • Post-deploy: verify Fastly edge 401 rate for -prev URLs is unchanged (the edge signal for upstream misconfig is preserved)

Post-review audit findings

Self-review by staff-engineer-level reviewer flagged:

  • I1 (Important): double-emit of AsoOverlayS3ReadDurationMsfixed in this PR via s3DurationEmitted guard
  • I4 (Important): OpenAPI missing Surrogate-Key doc — fixed in this PR
  • I3 (Important): resolveEnvironment silent fallback — split out to feat(metrics-emf): warn once when AWS_ENV/ENV missing #2835 (independent concern)
  • I2 + M1-M5 (Minor): tracked as follow-up tasks; not blocking merge

🤖 Generated with Claude Code

AEM Cloud Service injects `-prev` into the `AEM_SERVICE` env var on preview
publish pods (e.g. `cm-p154709-e1629980-prev`) for its own internal tier
routing. Skyline dispatcher builds the overlay URL from that env var
verbatim, so preview traffic hits us with `/config/cm-pN-eN-prev/redirects.txt`.

Current state on the aso-overlay Splunk board (2026-07-16): ~273/hr 401s
for `-prev` URLs — the Fastly VCL regex `[^/]+` accepts the suffix, but our
`AsoOverlayKeyHandler.OVERLAY_ROUTE` regex and `RedirectsController.SERVICE_RE`
both reject anything not exactly `cm-pN-eN`, so:
  1. auth handler doesn't match the path → falls through → 401 at authWrapper
  2. even with correct edge auth, controller `SERVICE_RE` would 400

Mystique writes exactly one overlay per (program, environment) — publish and
preview tiers share it. So we accept the suffix at both the auth + controller
boundaries and strip it before all downstream lookups (Site lookup, S3 key,
Surrogate-Key). Origin-side stripping avoids requiring a Fastly VCL change
(dispatcher-team territory) and lands the fix on our side alone.

Fastly cache footprint doubles per tenant (preview URL and publish URL become
separate cache entries under raw-URL keying). Acceptable because:
  1. Both share `Surrogate-Key: aso-overlay-<canonical>`, so mystique's
     purge-on-Deploy invalidates both in a single call.
  2. Both fetch the same S3 object (canonical), so upstream is not doubled
     beyond initial per-URL fills.
  3. At 10k tenants × ~20% preview coverage the extra entries are cheap.

Changes:
- `SERVICE_RE`: accept optional trailing `-prev`, keep case-sensitive
- `RedirectsController.getRedirects`: derive `canonicalService` from
  captured program/environment ids; use it for S3 key + Surrogate-Key on
  both 200 and 304 branches
- `AsoOverlayKeyHandler.OVERLAY_ROUTE`: accept the `-prev` suffix so the
  auth handler doesn't 401 preview traffic before it reaches the controller
- Unit tests: -prev happy path, 304 on -prev, malformed variants still 400,
  auth handler accepts -prev
- IT test: -prev end-to-end against the seeded canonical overlay
- OpenAPI: pattern + description updated

SITES-48140

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Regression fix from post-review audit on this PR:

**Bug:** On the 200 path, `emitS3Duration(SUCCESS)` fires right after
`s3Client.send()` resolves. If the subsequent `response.Body.transformToString()`
throws (SDK stream error, aborted download, decode failure), the outer catch
falls into the UNEXPECTED branch and calls `emitS3Duration(UNEXPECTED)` for
the same request — producing two `AsoOverlayS3ReadDurationMs` samples with
contradictory `S3Result` dimensions. On a rollout where a fraction of requests
fail mid-body, the S3-tier histogram would over-count and the SUCCESS bucket
would carry noise from what were actually failures.

**Fix:** Wrap `emitS3Duration` in an idempotency flag — first call wins. The
caller-observable outcome is still correctly recorded by `emitFinal` on the
final status, so the request-total chart is unaffected. The change is scoped:
one flag, one guard, no path re-shuffling.

Also update OpenAPI to document the `Surrogate-Key` response header on both
200 and 304, since Mystique's purge-on-Deploy contract depends on it and it
was previously undocumented for consumers reading the spec.

Regression test asserts exactly one `AsoOverlayS3ReadDurationMs` envelope is
emitted when the body stream throws post-send, and that the SUCCESS result
wins (first-writer semantics).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

This PR will trigger a patch release when merged.

@abhishekgarg18

Copy link
Copy Markdown
Member Author

Converting to Draft based on Cornel's Slack feedback earlier today (quoted at the top of the PR body): preview pods aren't supposed to be polling /config/*/redirects.txt — they have no API key. The 401s at Fastly edge are working-as-designed rejection of misconfigured preview pods, not a bug in this service.

Self-review of this PR agreed with Cornel's read and recommended closing rather than merging. The concern is two-fold:

  1. Silences the signal. The 401 rate is what Skyline uses to see the upstream misconfiguration. Merging this PR would hide it.
  2. Security posture. Without this PR, the route regex is the first line of defense — preview URLs don't even match. With it, the route matches, so any future auth-chain change that accidentally auto-allows keyless overlay-route requests would serve overlays to unauthenticated preview traffic.

Leaving the PR as Draft (not closed) so it can be re-marked ready in the future if:

  • Preview pods eventually get provisioned with the API key, OR
  • The decision reverses and shared publish/preview overlay is a supported pattern

Both audit-follow-up fixes (I1 metric double-emit + I4 OpenAPI Surrogate-Key doc) are still valuable independently and could be extracted into their own PR if this one stays parked long. The I3 env-fallback warn is already split out in #2835.

Next step: I'll ping @boston and @Slavic per Cornel's suggestion to align on direction.

@abhishekgarg18
abhishekgarg18 marked this pull request as ready for review July 16, 2026 07:43
@abhishekgarg18

abhishekgarg18 commented Jul 16, 2026

Copy link
Copy Markdown
Member Author

Reframed and un-drafted. See the top of the PR body for the defense-in-depth positioning.

Short version: this PR does not weaken auth. AsoOverlayKeyHandler still requires X-ASO-API-Key regardless of the URL shape. Accepting -prev at the route regex means preview pods without the key still get 401'd (just at the handler instead of the route-mismatch level), and preview pods with the key one day would just work.

The concern raised in the earlier review — "future auth-chain regression could auto-allow keyless overlay traffic" — applies equally to the canonical cm-pN-eN shape and to cm-pN-eN-prev. The regex boundary isn't the auth boundary today; the handler is. So the marginal security surface added by accepting -prev is effectively nil.

Fastly edge remains the primary diagnostic surface for "preview pods are misconfigured": the 401 rate by URL there is unchanged by this PR (we only change origin behavior). Skyline can still see the signal.

Bundled audit follow-ups still land here regardless:

  • Metric double-emit guard for AsoOverlayS3ReadDurationMs
  • OpenAPI Surrogate-Key documentation on 200/304

…-overlay-preview-tier

# Conflicts:
#	docs/index.html
@abhishekgarg18 abhishekgarg18 changed the title fix(aso-overlay): accept AEM CS preview-tier -prev service suffix feat(aso-overlay): accept AEM CS preview-tier -prev service suffix Jul 16, 2026
@miakobchuk
miakobchuk merged commit 4890495 into main Jul 16, 2026
20 checks passed
@miakobchuk
miakobchuk deleted the fix/sites-48140-aso-overlay-preview-tier branch July 16, 2026 16:53
solaris007 pushed a commit that referenced this pull request Jul 16, 2026
# [1.660.0](v1.659.1...v1.660.0) (2026-07-16)

### Features

* **aso-overlay:** accept AEM CS preview-tier -prev service suffix ([#2832](#2832)) ([4890495](4890495))
* **referral-traffic:** plumb categoryName through controller | LLMO-6026 ([#2790](#2790)) ([a77092f](a77092f)), closes [Hi#level](https://github.com/Hi/issues/level)
@solaris007

Copy link
Copy Markdown
Member

🎉 This PR is included in version 1.660.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants