feat(launchdarkly): add admin-only flags summary endpoint - #3236
feat(launchdarkly): add admin-only flags summary endpoint#3236sandsinh wants to merge 12 commits into
Conversation
GET /tools/launchdarkly/flags proxies LaunchDarkly's REST API for the experience-success-studio project, returning the raw response as-is so consumers (e.g. backoffice) can inspect real flag data without vendoring a static export snapshot into a repo. Co-Authored-By: Claude <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
This PR will trigger a minor release when merged. |
…eeds
Raw LD flag objects carry ~17 fields per flag and the "list flags" API
paginates at 20/page by default; the endpoint now follows `_links.next`
until exhausted and reduces each flag to { key, value } — the variation-0
value is the org/site targeting map that experience-success-studio-backoffice's
featureFlagParser.js (and the existing plg-onboarding LD code) actually reads.
Everything else (targeting rules, metadata, tags, etc.) was unused.
Co-Authored-By: Claude <noreply@anthropic.com>
…ly-flags-endpoint
…dpoint' into feat/admin-launchdarkly-flags-endpoint
There was a problem hiding this comment.
Hey @sandsinh,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Request changes - two blocking issues to address before merge.
Complexity: HIGH - medium diff with API surface and FACS classification changes.
Changes: Adds an admin-only GET /tools/launchdarkly/flags endpoint that fetches and summarizes feature flags from the LaunchDarkly REST API (7 files).
Must fix before merge
- [Important] PR description contradicts the implementation - claims raw passthrough and query-string forwarding but code reshapes through
toFlagSummary()and hardcodes params -src/controllers/launchdarkly.js:112(details inline) - [Important] Pagination follows
_links.next.hrefwithout path-prefix validation - defense-in-depth gap -src/controllers/launchdarkly.js:57(details inline)
Non-blocking (4): minor issues and suggestions
- nit:
response.json()is called before theresponse.okcheck - if LD returns a non-JSON error page (e.g. an HTML 502), the SyntaxError loses the HTTP status context in the logged error -src/controllers/launchdarkly.js:45 - nit: pagination loop exits silently at MAX_PAGES (2500 flags) without logging when more pages remain -
src/controllers/launchdarkly.js:38 - suggestion: the
valuefield intoFlagSummaryreturnsvariations[0].value(a variation definition, not the evaluated value for any environment) - the JSDoc documents this choice clearly, but the bare field name may mislead future consumers who expect live flag state -src/controllers/launchdarkly.js:76 - suggestion: the codebase already uses
@adobe/spacecat-shared-launchdarkly-clientinsrc/controllers/plg/plg-onboarding/launchdarkly.jsfor the same project and token - consider whether the shared client supports flag listing to centralize LD API interaction
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 8m 4s | Cost: $6.47 | Commit: 978a18133b0197d40a72f0a8ad2905ac6bf74b01
If this code review was useful, please react with 👍. Otherwise, react with 👎.
…r status Addresses ai-pr-review blocking findings on PR #3236: - Validate LD's `_links.next.href` starts with the requested flags path before following it, so the LD API token is never sent to an unexpected host/path if a response were ever tampered with. - Check response.ok before calling response.json() on the error branch -- an upstream non-JSON error body (e.g. an HTML 502) no longer masks the real HTTP status behind a JSON.parse failure. - Log a warning if pagination hits the MAX_PAGES safety cap with more pages remaining, instead of silently truncating. Co-Authored-By: Claude <noreply@anthropic.com>
…ly-flags-endpoint
…dpoint' into feat/admin-launchdarkly-flags-endpoint
There was a problem hiding this comment.
Hey @sandsinh,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Request changes - one blocking issue with the response shape contract.
Complexity: HIGH - medium diff with API surface and FACS classification changes.
Changes: Adds an admin-only GET /tools/launchdarkly/flags endpoint that fetches and summarizes feature flags from the LaunchDarkly REST API (7 files).
Note: CI checks are still pending (build, it-postgres) - resolve before merge.
Must fix before merge
- [Important]
toFlagSummaryreturnsundefinedfor empty/missing variations, silently dropping thevaluekey from the serialized response -src/controllers/launchdarkly.js:98(details inline)
Non-blocking (2): minor issues and suggestions
- nit: comments in
src/routes/facs-capabilities.js:206andsrc/routes/required-capabilities.js:183say "raw flag passthrough" but the implementation reshapes flags throughtoFlagSummary()- the PR description was already corrected, but these inline comments were not updated to match - suggestion: pagination path check (
startsWith(LD_FLAGS_PATH)) does not block same-host path traversal via../segments in the next link - consider comparing against the resolved pathname after URL construction -src/controllers/launchdarkly.js:72
Previously flagged, now resolved
- PR description mismatch with implementation (corrected)
- Pagination next-link validation added (prefix check on
LD_FLAGS_PATH) response.oknow checked beforeresponse.json()- MAX_PAGES cap now logs a warning when more pages remain
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 14m 14s | Cost: $8.06 | Commit: 20bf3723343ffa1d4b1028a620602f0bd65e69e8
If this code review was useful, please react with 👍. Otherwise, react with 👎.
|
|
||
| /** | ||
| * Admin-only LaunchDarkly flags endpoint for the `experience-success-studio` project. | ||
| * Fetches every flag (paginating through LD's REST API) and returns only the fields |
There was a problem hiding this comment.
issue (blocking): toFlagSummary returns { key, value: undefined } when variations is empty or absent. JSON.stringify silently drops undefined values, so the serialized response becomes { "key": "..." } with no value field, while flags with variations produce { "key": "...", "value": ... }. This inconsistent shape forces every consumer to handle the field being present or absent.
The fix:
value: flag.variations?.[0]?.value ?? null,This keeps the shape uniform and makes "no variations" explicit. The test at line 159 documents the current behavior and would need updating to expect { key: 'FF_no-variations', value: null }.
There was a problem hiding this comment.
Fixed in 62fd065 — value now falls back to null instead of undefined, so the response shape is uniform (value key always present) regardless of whether a flag has variations. Also addressed both non-blocking nits from the same review: pagination-link validation now resolves the href against our base URL and checks the resolved origin+pathname (closes the ../ traversal / different-origin gap), and the stale 'raw flag passthrough' comments in facs-capabilities.js and required-capabilities.js were updated to describe the actual reshaped response.
…heck Addresses ai-pr-review round 2 on PR #3236: - toFlagSummary now returns value: null (not undefined) for a flag with no variations, so the response shape is uniform instead of the `value` key silently disappearing under JSON serialization for some flags. - Pagination link validation now resolves the next href against our own base URL and checks the resolved origin + pathname, instead of a raw string prefix check -- closes a `../` path-traversal gap and an absolute different-origin URL, either of which could have sent the LD API token to an unexpected destination. - Updated stale "raw flag passthrough" comments in facs-capabilities.js and required-capabilities.js to match the actual reshaped response. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Hey @sandsinh,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Approve - all prior blocking findings addressed; no new blocking issues.
Complexity: HIGH - medium diff with API surface and FACS classification changes.
Changes: Adds an admin-only GET /tools/launchdarkly/flags endpoint that fetches and summarizes feature flags from the LaunchDarkly REST API (7 files).
Note: CI checks are still pending (build, it-postgres) - resolve before merge.
Non-blocking (4): minor issues and suggestions
- nit: JSDoc
@paramforlaunchDarklyControllerinsrc/routes/index.js:205still says "passthrough controller" - the same terminology was corrected infacs-capabilities.jsandrequired-capabilities.jsbut missed here - suggestion: the
Authorizationheader receives the raw LD API token without aBearerprefix (correct for LD's API), but a future maintainer may "fix" this by adding one - a one-line comment on the header noting LD expects the raw token would prevent that -src/controllers/launchdarkly.js:50 - suggestion: the catch block logs
e.messagebut note.status(which the code sets explicitly for LD API errors on line 57) - includinge.statusin the log line would tell on-call whether to rotate the token (401) or back off (429) without reproducing the call -src/controllers/launchdarkly.js:146 - suggestion: no test exercises a response body where
body.itemsisundefined(vs empty array) - the?? []fallback on line 63 is doing real work and deserves a test that proves it -test/controllers/launchdarkly.test.js
Previously flagged, now resolved
toFlagSummaryreturnsvalue: nullinstead ofundefinedfor empty variations (uniform response shape)- Pagination link validation upgraded from naive
startsWithon raw string to resolved-URL origin+pathname check (closes path-traversal and different-origin vectors) - Stale "raw flag passthrough" comments updated to "flags summary" in
facs-capabilities.jsandrequired-capabilities.js
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 2m 3s | Cost: $6.66 | Commit: 62fd065f225c55c793e8d930b082192d6a3e4dc9
If this code review was useful, please react with 👍. Otherwise, react with 👎.
Please ensure your pull request adheres to the following guidelines:
describe here the problem you're solving.
If the PR is changing the API specification:
yet. Ideally, return a 501 status code with a message explaining the feature is not implemented yet.
If the PR is changing the API implementation or an entity exposed through the API:
If the PR is introducing a new audit type:
Related Issues
No tracked issue — internal/admin tooling endpoint requested to inspect real LaunchDarkly flag data for the
experience-success-studioproject without vendoring a static export snapshot (ff-export.json) into experience-success-studio-backoffice.Summary
GET /tools/launchdarkly/flagsendpoint, admin-only (AccessControlUtil.hasAdminAccess()).experience-success-studioLD project, paginating through LD's REST API (GET /api/v2/flags/:project, 20/page default) by following_links.next.hrefuntil exhausted, capped atMAX_PAGES(50) as a safety bound.{ key, value }—valueisvariations[0].value, the same fieldplg-onboarding/launchdarkly.jsalready reads, and exactly whatexperience-success-studio-backoffice'sfeatureFlagParser.jsexpects. This is not a raw passthrough and does not forward caller query params — an earlier revision did both, and this description has been corrected to match the current implementation.nextlink is validated to stay within the requested flags path before being followed, so the LD API token can't be sent to an unexpected host/path if a response were ever tampered with.INTERNAL_ROUTESin bothrequired-capabilities.jsandfacs-capabilities.js— not exposed to S2S JWT consumers or FACS ReBAC, same bucket asGET /tools/proxyandGET /monitoring/drs-bp-pg-audit.GET /tools/proxy(same internal-tooling class) is also undocumented there.@adobe/spacecat-shared-launchdarkly-client(already used for this project/token inplg-onboarding/launchdarkly.js) could serve this: it only exposes single-flag reads/writes (getFeatureFlag,updateFallthroughVariation,updateVariationValue), no "list all flags" method, so this endpoint calls LD's REST API directly instead.Test plan
npm run lint— cleannpm test— full suite green, includingtest/controllers/launchdarkly.test.js(admin gate, missing token, single-page and multi-page pagination, pagination-path validation, page-cap warning, no-variations flag, LD error status, fetch throwing) and updatedtest/routes/index.test.jswiringThanks for contributing!