Skip to content

feat: add admin flag bypass - #2312

Open
epipav wants to merge 2 commits into
feat/api-gateway-member-tiersfrom
feat/api-gateway-admin-flag
Open

epipav wants to merge 2 commits into
feat/api-gateway-member-tiersfrom
feat/api-gateway-admin-flag

Conversation

@epipav

@epipav epipav commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

LFIDs individually targeted with true on a LaunchDarkly boolean flag skip member-tiers and get the admin x-org-id and x-tier.

  • The Worker fetches the flag's server-side config from LD_FLAG_URL with LD_SDK_KEY and reads individual targets only; rules and segments are ignored, and a flag that is off yields no admins.
  • The list is reused for 60 seconds. A LaunchDarkly failure keeps the last list, or falls back to member-tiers.
  • Admin values are provisional (lfx-admin / admin) until the API's tier collapse defines an admin pool.

Stacked on #2311.

Copilot AI balanced review requested due to automatic review settings September 25, 2026 15:31
@cursor

cursor Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

PR Summary

High Risk
Changes who receives admin org/tier headers on the API gateway entitlement path; mis-targeting or stale cached admin lists could grant elevated access until refresh.

Overview
Adds a LaunchDarkly-driven admin bypass on entitlement cache misses: after PAT→JWT exchange, LFIDs individually targeted with boolean true on the flag at LD_FLAG_URL receive fixed x-org-id / x-tier (lfx-admin / admin) and skip the member-tiers lookup; everyone else still uses pickOrgTier as before.

New flags.ts loads server-side flag JSON with LD_SDK_KEY, derives usernames only from targets and user contextTargets (rules/segments ignored; flag off ⇒ no admins), caches the list for 60s, and on fetch errors retains the previous list or returns empty so resolution falls back to member-tiers. Worker Env, .dev.vars.example, and README document LD_SDK_KEY and LD_FLAG_URL; fetchAdminUsernames is injectable via Deps for tests.

Reviewed by Cursor Bugbot for commit eda1b1f. Bugbot is set up for automated code reviews on this repo. Configure here.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Concurrent cache refreshes can cause periodic LaunchDarkly request stampedes.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Adds a LaunchDarkly-controlled admin bypass to the API gateway’s entitlement resolution.

Changes:

  • Parses individually targeted admin LFIDs and caches them for 60 seconds.
  • Applies provisional admin organization and tier headers.
  • Adds environment configuration, documentation, and tests.
File Description
.dev.vars.example Adds LaunchDarkly variables.
README.md Documents admin bypass behavior.
src/​env.ts Types LaunchDarkly configuration.
src/​flags.ts Fetches, parses, and caches admin targets.
src/​index.ts Integrates admin entitlement resolution.
tests/​flags.test.ts Tests flag parsing and caching.
tests/​gateway.test.ts Tests admin header forwarding.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread workers/api-gateway/src/flags.ts
@epipav
epipav force-pushed the feat/api-gateway-admin-flag branch from 5d6007d to 0b6ad41 Compare September 25, 2026 16:37
Copilot AI review requested due to automatic review settings September 25, 2026 16:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Admin authorization currently relies on the noncanonical username claim rather than the immutable JWT subject.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment thread workers/api-gateway/src/index.ts Outdated
Copilot AI review requested due to automatic review settings September 25, 2026 16:46
@epipav
epipav force-pushed the feat/api-gateway-admin-flag branch from 0b6ad41 to 0fc6699 Compare September 25, 2026 16:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The LaunchDarkly request and response handling do not match its polling API, preventing the admin list from loading.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment thread workers/api-gateway/src/flags.ts
@epipav
epipav requested a review from gaspergrom September 25, 2026 18:56
@epipav
epipav added this pull request to stack #2313 September 25, 2026 18:56
@epipav epipav assigned epipav and themarolt and unassigned themarolt Sep 25, 2026

@gaspergrom gaspergrom left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question on the flag fetch before this merges, left inline.

try {
const response = await fetch(env.LD_FLAG_URL, { headers: { authorization: env.LD_SDK_KEY } });
if (!response.ok) throw new Error(`LaunchDarkly flag request failed: ${response.status}`);
cached = { usernames: targetedUsernames(await response.json()), fetchedAt: Date.now() };

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does LD_FLAG_URL map to a real LaunchDarkly endpoint? SDK-key polling normally returns the whole environment's flags and segments, not one flag's on/variations/targets shape. If LD_FLAG_URL points there, flag.on stays undefined and the feature excludes every admin with no error. Does a proxy sit in front of LD and return just this flag, or should this pull the flag by key from the polling response first?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LD_FLAG_URL is the per-flag polling path /sdk/latest-flags/insights-public-api, which returns that one flag config (older server SDKs fetch by key this way). tests use that path now in e3dfc40, will confirm against the real env once the SDK key is set

Copilot AI review requested due to automatic review settings September 25, 2026 19:43
@epipav
epipav force-pushed the feat/api-gateway-admin-flag branch from 0fc6699 to e3dfc40 Compare September 25, 2026 19:43
@epipav
epipav requested a review from gaspergrom September 25, 2026 19:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The previously identified LaunchDarkly polling endpoint mismatch remains unresolved and prevents the configured flag from being parsed correctly.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: None

Resolved since last review (1)

Signed-off-by: anilb <epipav@gmail.com>
Signed-off-by: anilb <epipav@gmail.com>
Copilot AI review requested due to automatic review settings September 25, 2026 19:52
@epipav
epipav force-pushed the feat/api-gateway-admin-flag branch from e3dfc40 to eda1b1f Compare September 25, 2026 19:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The LaunchDarkly request needs a timeout so stalled lookups cannot block entitlement-cache misses.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)

export async function fetchAdminUsernames(env: Env): Promise<string[]> {
if (cached && Date.now() - cached.fetchedAt < REFRESH_MS) return cached.usernames;
try {
const response = await fetch(env.LD_FLAG_URL, { headers: { authorization: env.LD_SDK_KEY } });

@gaspergrom gaspergrom left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, thanks for the fixes.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants