This PR will trigger a minor release when merged.

Self reviewed with review kit

All three agents completed. Here's the consolidated review:

PR #1595 Review Summary

Critical (fix before merge)

[security] Body fallback activates on param name mismatch, not just "no params in route"
read-only-admin-wrapper.js:52-53

params.siteId ?? context.data?.siteId fires whenever params.siteId is absent — including when the route has a path param but it's named :id, :site, or anything other than :siteId. An RO admin who owns site A could call PATCH /sites/B (a route using :id) with body { siteId: "A" } and the ownership check would pass for A while the handler writes to B.

Fix: only fall back to context.data when the route had zero path params — i.e. gate on Object.keys(params).length === 0:

const siteId = Object.keys(params).length === 0
  ? context.data?.siteId
  : params.siteId;
const organizationId = Object.keys(params).length === 0
  ? context.data?.organizationId
  : params.organizationId;

Important (should fix)

[observability] evaluateFeatureFlag catch is completely silent
read-only-admin-wrapper.js:108-110 — LD failures block all RO admin access with zero log output. Add log.error before return false.

[observability] Missing dataAccess returns false with no log
read-only-admin-wrapper.js:46-48 — A misconfigured wrapper chain produces a flood of unexplained 403s. Add log.error with a message pointing at wrapper ordering.

[observability] No-ID-found case is silent and indistinguishable from ownership failure
read-only-admin-wrapper.js:80 — When neither path nor body has a recognisable ID, return false fires with no log. Operators can't tell if a 403 came from "user doesn't own this" vs "no ID was found at all". Add a log.warn.

[audit] Audit log omits resolved resource ID and whether body fallback was used
read-only-admin-wrapper.js:177-183 — The ro-admin-audit log entry doesn't record which siteId/organizationId was used to authorise a write, making post-incident reconstruction impossible. Include the resolved IDs and source ('path' vs 'body').

[reliability] extractRouteParams called outside any try-catch
read-only-admin-wrapper.js:162 — If it throws, the exception escapes the wrapper as an unhandled rejection rather than a controlled 403. Wrap it.

Suggestions (nice to have)

[test] getOrganization() throwing is not covered — the catch block is exercised via findById throwing but not from a second async step inside the same try block.

[test] Organization.findById throwing is not covered — only the site lookup throw path is tested; the org path hits the same catch but isn't explicitly exercised.

[test] dataAccess present but Site/Organization property absent not tested — the optional chaining dataAccess.Site?.findById silently returns undefined; no test exercises this with context.dataAccess = {}.

[docs] JSDoc should document that context.data must be the parsed body — if readOnlyAdminWrapper runs before the body parser in the .with() chain, body-fallback routes will always 403 with no diagnostic.

Strengths

• Fail-closed is correctly implemented and well-tested for the main paths
• Path params take precedence over body data via ?? — correct priority
• Ownership check is lazy (only runs on write routes)
• 309 tests, 100% lines/statements, 98.18% branches
• isOwnerOfResource catch block correctly logs the error before returning false
• extractRouteParams returns {} (never null) — prevents null-dereference downstream

Pushed. Here's a summary of everything addressed from the review:

Critical (security)

• Body fallback now gates on Object.keys(params).length === 0 — it only activates when the matched route has no path params at all, closing the bypass where an attacker on a :id-named route could supply a body.siteId for a site they own

Observability / logging (4 gaps fixed)

• evaluateFeatureFlag catch now calls log.error with the exception before returning false
• isOwnerOfResource logs log.error when dataAccess is absent (with a pointer to wrapper ordering)
• isOwnerOfResource logs log.warn when no ID can be resolved from path or body
• Allowed writes now emit a dedicated ro-admin-write log entry including resolved IDs and idSource: 'path' | 'body'

Reliability

• extractRouteParams call wrapped in try/catch — exceptions now produce a controlled 403 + log.error instead of an unhandled rejection

New tests (9 added)

• log.error assertions on LD SDK throw and isFlagEnabledForIMSOrg throw
• log.error on missing dataAccess; log.warn on no-ID-found
• getOrganization() throwing (fail-closed + log)
• Organization.findById throwing (fail-closed + log)
• dataAccess present but Site property absent
• Security test: body siteId ignored when route uses :id param name
• ro-admin-write log assertions for both path and body ID source
• extractRouteParams throwing via esmock

Hey @ravverma,

Thanks for the careful design and the detailed self-review trail. The fail-closed posture is solid, body-fallback bypass via :id-style routes is correctly closed and locked in by tests, and the audit fields (resolvedSiteId, resolvedOrgId, idSource) are exactly what an on-call needs. A few concerns worth tightening before merge.

Strengths

• read-only-admin-wrapper.js:50-99 isOwnerOfResource fail-closes consistently across every error path (missing dataAccess, entity not found, missing org, lookup throws, no resolvable ID) and structured-logs each pathway.
• read-only-admin-wrapper.js:62-66 correctly gates body fallback on Object.keys(params).length === 0. The regression test "blocks write and ignores body siteId when route has path params with a different name" (read-only-admin-wrapper.test.js:614-629) locks this in.
• route-utils.js:76-103 extractRouteParams reuses the existing matchRoute helper, so the matcher remains single-sourced.
• Coverage is thorough on the explicit cases: site/org/spaceCatId path params, body fallback, path-vs-body precedence, db throws on both branches, missing Site accessor, and getOrganization throws.

Issues

Important (Should Fix)

1. read-only-admin-wrapper.js:69, 77 - hasOrganization(undefined) would raise TypeError. auth-info.js:95-100 does orgId.split('@') with no guard. If org.getImsOrgId() returns falsy (misconfigured org row, missing IMS provisioning), the call throws and the outer try/catch at line 88 catches it as a generic "Error checking resource ownership for RO admin". Fail-closed posture is preserved but the log message is misleading and masks a real data-integrity bug. Fix: guard getImsOrgId() explicitly before passing to hasOrganization, with a dedicated log.warn like "Owning organization has no imsOrgId; denying RO admin write". Optional but recommended: also guard hasOrganization itself in auth-info.js against null/undefined input.

2. read-only-admin-wrapper.test.js:673-699 - two tests claim to cover "unmapped routes" but actually exercise the parameterized-match path. Suffix /sites/some-unmapped-action matches PATCH /sites/:siteId in the routeMap, so extractRouteParams returns { siteId: 'some-unmapped-action' } and the body siteId: 'abc-123' is never consulted. The tests pass only because findById is a permissive stub. As a result, truly unmapped routes are not test-covered. Fix: use a suffix that genuinely fails to match (e.g. /some/other/path against a routeMap containing only PATCH /sites/:siteId) and assert findById.calledWith('abc-123') to lock in body-fallback semantics on unmapped routes.

3. read-only-admin-wrapper.js:212-240 - unmapped routes reach the body-fallback authorization path. When a request hits a URL whose pattern is not in routeCapabilities at all (extractRouteParams returns {} because no key matches), control falls into the !hasPathParams else branch, resolveRouteCapability returns null, the action is undefined (not 'read'), and isOwnerOfResource consults context.data.siteId. If the body carries an owned siteId, the request is allowed. Net effect: a new admin endpoint shipped in a consumer service without a corresponding routeCapabilities entry becomes write-reachable for owners. The previous safe default was "unmapped means deny." Fix options: (a) require resolveRouteCapability(...) to return non-null before letting body-fallback authorize, or (b) introduce an opt-in flag per route (e.g. capability suffix :bodyOwnership) so consumers must declare which body-driven writes are sanctioned.

4. read-only-admin-wrapper.js:178-211 - non-owner reads on parameterized routes now trigger isOwnerOfResource, which performs a DB lookup before the read can proceed. Two concrete consequences: (a) every parameterized-route read by an RO admin now does an extra Site.findById + getOrganization() (or Organization.findById) even though reads were always permitted by capability; (b) in any caller setup where dataAccess is not yet on the context (e.g. some legacy/test wirings), this emits log.error("dataAccess not on context...") per read, polluting the noise floor. Fix: in the hasPathParams branch, resolve capability first; if the action is read/readAll, skip the ownership check entirely. Reserve ownership-first for write actions.

5. read-only-admin-wrapper.js:184-249 - double info log per allowed RO admin request. Both ro-admin-access and ro-admin-audit are emitted for the same request with overlapping fields. This doubles audit log volume on a privileged code path and breaks downstream consumers that expect one log line per request. Pick one tag (the richer ro-admin-access subsumes ro-admin-audit), or skip the audit log when the access log was already emitted.

6. read-only-admin-wrapper.js:172-243 - confused-deputy shape on body-fallback writes. The wrapper authorizes based on a single siteId/organizationId plucked from the body, but the handler may mutate other resources that the wrapper never validated. Concrete example: POST /preflight/jobs with body { siteId: "owned-A", targets: ["unowned-B"] } is allowed because the user owns site A; the handler may then operate on unowned-B. The wrapper has no visibility into the handler's actual action surface. Fix: document the contract explicitly on readOnlyAdminWrapper's JSDoc that handlers behind body-fallback authorization MUST only mutate the declared siteId/organizationId. Consider an explicit per-route opt-in (issue 3) so consumers must surface this contract per endpoint.

7. route-utils.test.js:83-128 - extractRouteParams has no direct unit test for :spaceCatId extraction even though the wrapper special-cases it. Coverage is only transitive through read-only-admin-wrapper.test.js. Add one direct case: route 'PATCH /v2/orgs/:spaceCatId', suffix /v2/orgs/org-456, expect { spaceCatId: 'org-456' }.

8. read-only-admin-wrapper.test.js:599-612 - the body-fallback success test asserts the ro-admin-access log but not the trailing ro-admin-audit log. The path-fallback test at line 579 asserts both. Add the matching ro-admin-audit assertion for symmetry; otherwise a future refactor that accidentally early-returns after ro-admin-access on the body path would silently drop the audit log.

9. read-only-admin-wrapper.js:166-208 - denial-reason ambiguity in audit logs. The same tag: 'ro-admin' and 'Read-only admin blocked from route' message is emitted on FF-disabled denials (line 166), parameterized non-owner-write denials (line 202), and collection-write no-owner denials (line 220). A SIEM cannot distinguish "feature flag off" from "not the owner" from "no resource ID in request" - exactly the events that warrant separate detection rules. Add a reason field (e.g. 'feature-flag-disabled' | 'not-owner-and-no-read-capability' | 'no-resource-id').

Minor (Nice to Have)

• read-only-admin-wrapper.js:62-66 - dead-code in the no-path-params branch. When hasPathParams === false, params.siteId / params.organizationId / params.spaceCatId are guaranteed undefined, so params.X ?? context.data?.X collapses to context.data?.X. Cleaner:

  const siteId = hasPathParams ? params.siteId : context.data?.siteId;
  const organizationId = hasPathParams
    ? (params.organizationId ?? params.spaceCatId)
    : (context.data?.organizationId ?? context.data?.spaceCatId);

• read-only-admin-wrapper.js:175 - extractRouteParams is called outside any try/catch. Today the function is pure string ops on pathInfo.suffix so a throw is unlikely, but a single wrapping try/catch around the whole authorization block would be consistent with the fail-closed posture the rest of the wrapper enforces.

• read-only-admin-wrapper.js:204-205, 235-236 - resolvedSiteId/resolvedOrgId in the body-fallback log are re-derived from context.data rather than reflecting what isOwnerOfResource actually used. If isOwnerOfResource's precedence ever changes (or aliases expand), the log will drift from the policy. Consider having isOwnerOfResource return { owns, resolvedSiteId, resolvedOrgId, idSource } so the wrapper logs the same value the policy used.

• read-only-admin-wrapper.js:55-66, 191, 236 - spaceCatId is referenced only in this PR within spacecat-shared. The convention may come from spacecat-api-service LLMO routes (e.g. :spaceCatId for org identifiers), but a new maintainer reading the code in 12 months will reasonably wonder if it is a typo. Add a brief comment at the first alias site naming the consuming convention.

• read-only-admin-wrapper.js:140-156 - JSDoc on readOnlyAdminWrapper does not state two load-bearing wiring requirements: (a) dataAccessWrapper must run before this wrapper, (b) the request body parser must run before this wrapper for routes that depend on body fallback. Mis-ordered .with() chains will silently deny legitimate requests. Add both to the JSDoc.

• route-utils.test.js:83-128 - missing edge cases for extractRouteParams: trailing slash in suffix (/sites/abc/), method-mismatch (route is PATCH, request is POST), lowercase request method (pathInfo.method = 'patch'), longer request than route pattern (/sites/abc/extra vs /sites/:siteId). The implementation appears correct via routeSegments.length !== requestSegments.length and toUpperCase(), but each is a load-bearing assumption worth locking in.

Recommendations

• Pick a single authorization contract for body-fallback routes (issue 3 above) and document it in the routeCapabilities map convention. The two viable options are explicit opt-in per route, or "must be in routeCapabilities with a non-null capability." The current behavior of "body fallback authorizes any route" is the easiest mode to maintain but the riskiest as the consumer set grows.
• Consider returning the resolved entity from isOwnerOfResource and stashing it on context (e.g. context.authzResource = { site, org }). This both eliminates a duplicate fetch by the handler and gives the handler a verified resource to operate on - which slightly tightens the confused-deputy shape from issue 6.

Assessment

Ready to merge? With fixes.

Reasoning: The security model and fail-closed posture are correct; the body-fallback bypass concern raised earlier in self-review is properly closed and tested. The remaining items fall into two clusters: (1) policy decisions that should be tightened before this lands as the library-wide pattern (unmapped-route body authorization, ownership check on every read, confused-deputy doc/contract); and (2) test/observability hygiene that's straightforward to address (test renames, audit log split, denial-reason field, :spaceCatId unit coverage).

Next Steps

1. Address items 1-3 first (TypeError path, misleading test names, unmapped-route authorization) - these are the load-bearing correctness/policy items.
2. Pick a fix for items 4-6 (read fast-path, log doubling, confused-deputy contract).
3. Items 7-9 are small, mechanical test/log changes.
4. Minor items optional but high-leverage for future maintainers.

Hey @ravverma,

Re-review on commit f4488a52. All previously flagged Important findings are addressed; the pushed-back policy on unmapped routes is documented and defensible. One residual contract-legibility concern and a small batch of observability/test polish notes.

Strengths

Previously flagged issues now resolved:

• hasOrganization(undefined) TypeError guard: explicit imsOrgId null check in both site (read-only-admin-wrapper.js:78-83) and org paths (:91-96), each with dedicated log.warn and dedicated tests.
• Misleading "unmapped route" tests fixed: suffix changed to POST /jobs/preflight; findById.calledWith('abc-123') now locks in body-fallback semantics (read-only-admin-wrapper.test.js:738-768).
• Read fast-path implemented (read-only-admin-wrapper.js:217). No more Site.findById + getOrganization per parameterized read.
• Double-log suppressed via accessLogged flag; test asserts ro-admin-audit is NOT emitted when ro-admin-access fired (read-only-admin-wrapper.test.js:643-662).
• Confused-deputy contract now explicit in the wrapper JSDoc (read-only-admin-wrapper.js:179-183): handlers behind body-fallback MUST only mutate the declared siteId/organizationId.
• :spaceCatId direct unit test at route-utils.test.js:133-137.
• Body-fallback path-param spoofing remains blocked (:62) and tested at read-only-admin-wrapper.test.js:679-694.
• JSDoc on readOnlyAdminWrapper now documents wiring requirements (dataAccessWrapper first, body-parser first for body-fallback routes).
• Outer try/catch around the whole authorization block (read-only-admin-wrapper.js:209-244); tested via esmock at read-only-admin-wrapper.test.js:770-793.
• extractRouteParams edge cases (method mismatch, lowercase method, longer-than-pattern, trailing slash) all covered.
• Pushed-back item (unmapped-route ownership policy): the author's counter-argument is reasonable given the dual gating (IMS is_read_only_admin claim + FT_READ_ONLY_ORG per-org LD flag) and the comprehensive ro-admin-access audit log. Policy is documented at read-only-admin-wrapper.js:213-216. Original concern resolved by author response; see Important item 1 for the residual contract-legibility note.

Issues

Important (Should Fix)

1. Drift-detection signal needed when an unmapped route is allowed via ownership (read-only-admin-wrapper.js:213-244). The policy ("ownership is the governing invariant, capability mapping is advisory") is defensible at the wrapper level, but the contract becomes invisible to consumers: a maintainer in spacecat-api-service or spacecat-audit-worker reading the routeCapabilities map sees what looks like an explicit allowlist, while routes omitted from the map are silently allowed if ownership resolves. The concrete failure mode is a maintainer removing a route from routeCapabilities thinking it is dead - behavior silently changes from "denied" to "allowed if owner", and local unit tests cannot detect this because the map is consumer-supplied.

   Fix (minimal): emit a log.warn when ownership grants access to a route that was NOT in routeCapabilities:

   if (capability === null) {
     log.warn({ tag: 'ro-admin', reason: 'unmapped-route-allowed', method, suffix }, 'RO admin allowed on unmapped route via ownership');
   }

   Consumer maintainers can grep for this in production logs to detect drift.

   Fix (better, separate PR): add a sentinel value in routeCapabilities for routes that intentionally rely on body-fallback ownership (e.g. 'POST /preflight/jobs': 'preflight:body-ownership') so the contract stays a single-source-of-truth lookup.

Minor (Nice to Have)

1. Denial-reason field is misleading for the "no resolvable ID" path. read-only-admin-wrapper.js:103-107 + :235-242. When isOwnerOfResource finds no siteId and no organizationId, it returns false and the wrapper logs reason: 'not-owner', which is factually wrong. Either return a structured result from isOwnerOfResource ({ owner, reason: 'no-id' | 'not-owner' | 'lookup-failed' }) or differentiate at the call site.

2. FF-disabled denial lacks reason field. read-only-admin-wrapper.js:192-196 still uses generic tag: 'ro-admin' without a reason, while the not-owner denial carries reason: 'not-owner'. Add reason: 'feature-flag-disabled' for SIEM filterability.

3. Outer-try test missing body and audit-log assertions. read-only-admin-wrapper.test.js:770-793 asserts result.status === 403 and the error log, but not body.message === 'Forbidden' and not that ro-admin-audit is NOT emitted on the catch path. Add both for symmetry.

4. Sibling block tests should assert reason: 'not-owner'. read-only-admin-wrapper.test.js:256-266 (POST write-route) and :294-304 (unmapped route) flow through the same not-owner branch as the PATCH test at :277-280 but only assert { tag: 'ro-admin' }. Extend both to include reason: 'not-owner'.

5. resolvedSiteId/resolvedOrgId in the log are re-derived from context.data rather than tracked through isOwnerOfResource. read-only-admin-wrapper.js:226-232. Resolution rules in isOwnerOfResource and in the log block must stay synchronized; today they are literal duplicates. Have isOwnerOfResource return { owner, resolvedSiteId, resolvedOrgId, idSource }.

6. extractRouteParams matches paths with double internal/leading slashes. Because filter(Boolean) collapses empty segments, //sites/abc-123 and /sites//abc-123 match the parameterized route. Behavior is currently fine but the trailing-slash test at route-utils.test.js:155-158 only pins one such case. Add one paranoia test for double-slash inputs.

Recommendations

• Return the resolved entity from isOwnerOfResource so handlers can reuse the verified site/org instead of re-fetching.
• Per-route opt-in for body-fallback authorization (sentinel value or richer entry shape). Tracking only, separate PR.
• Consider replacing email with a stable IMS user ID in the ro-admin-access/ro-admin-audit logs - email is PII at four log call sites.

Assessment

Ready to merge? With fixes.

Reasoning: The new commit substantively addresses every previously-flagged Important finding and the pushed-back policy is documented and defensible. The remaining Important item is operational: add a one-line drift-detection log. Minor items are mechanical observability and test-symmetry tightening.

Note: CI Test check is currently pending - re-verify before merging.

Next Steps

1. Pick up the Important item first (one log line at the unmapped-route allow path).
2. Minor items 1-2 (denial-reason on FF-disabled and no-id paths) are the highest-signal of the remaining items for forensics.
3. Minor items 3-4 are mechanical test additions.

Hey @ravverma,

Re-review on commit 6134ad6c. The prior Important finding (drift-detection log for unmapped-route ownership) is addressed. Implementation is faithful to the recommended fix; placement, severity, structured fields, and message text are all correct. One small test-symmetry suggestion and two observability follow-ups. No merge blockers.

Strengths

Previously flagged issue now resolved:

• Drift-detection signal for unmapped-route-via-ownership (read-only-admin-wrapper.js:219-227): exactly the recommended fix. Gated on capability === null so it fires only on the allow-path for unmapped routes. log.warn severity is right for a policy-drift event. Structured fields (tag, reason: 'unmapped-route-allowed', method, suffix) are SIEM-filterable and symmetric with the existing reason: 'not-owner' denial signal. Message names the remediation. No PII or token leakage beyond what the neighbouring ro-admin-access log already emits.
• Test coverage (read-only-admin-wrapper.test.js:754-772): positive assertion via calledWithMatch on the exact payload and message, using a realistic body-fallback path (POST /jobs/preflight).

Issues

Minor (Nice to Have)

1. No negative assertion that the drift warn is suppressed on mapped writes. The production gate is if (capability === null). The existing mapped-write owner-allowed test (around read-only-admin-wrapper.test.js:643) is the natural home for expect(logStub.warn.calledWithMatch({ reason: 'unmapped-route-allowed' })).to.be.false. Without it, a future flip of the condition (dropping the === null, or changing to == null) would not be caught. One line; locks the gate.

2. authInfo.getTenantIds?.()[0] shape on the new log line (read-only-admin-wrapper.js:224). If getTenantIds is absent the optional chain yields undefined and [0] throws TypeError; the outer try/catch would absorb it and the drift signal would be silently lost. Note: this pattern is pre-existing on lines 195, 226, 241, 261, so the new line just adds another instance. Consider hardening to authInfo.getTenantIds?.()?.[0] across all five call sites (or tracking as a follow-up; not blocking for this commit).

Recommendations

• Fold reason: 'unmapped-route-allowed' (or a capabilityMapped: false boolean) into the existing ro-admin-access log.info entry rather than emitting a separate warn. SIEM and dashboards can then detect drift from a single log stream per request instead of correlating two lines.
• Treat the drift warn as a closeable signal in consumer-service dashboards: explicit goal of "zero unmapped-route-allowed events sustained for N days". Once empirically empty, the wrapper contract simplifies from "ownership is the invariant, capability is advisory" to "capability mapping is required", which is a strictly smaller surface to explain. Worth a follow-up ADR.

The previously raised minor items from the last re-review (denial-reason wording, FF-disabled reason field, outer-try test polish, sibling not-owner test reason assertion, resolvedSiteId/resolvedOrgId re-derivation, double-slash path matching) remain unresolved but are not gating; they were not picked up by this commit and can land in a separate cleanup.

Assessment

Ready to merge? Yes.

Reasoning: The sole gating Important item from the prior re-review is addressed precisely as suggested. The new branch is fully covered by the new positive test; the one Minor on negative assertion is a one-line test addition and not blocking. The pre-existing getTenantIds?.()[0] pattern is a separate hygiene fix worth tracking but not gating.

Note: CI Test check was pending at review time, re-verify before merging.

🎉 This PR is included in version @adobe/spacecat-shared-http-utils-v1.27.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

@solaris007 Sorry I have merged it accidentally, let me fix the issue next PR