Skip to content

Commit 7ef21dc

Browse files
fix(cli): linear setup must not clobber a workspace's webhook signing secret (aws-samples#611) (aws-samples#612)
* fix(cli): linear setup must not clobber a workspace's webhook secret (aws-samples#611) Re-running `bgagent linear setup <slug>` on an already-installed workspace overwrote that workspace's per-workspace webhook signing secret with the STACK-WIDE fallback — which belongs to whichever workspace installed first. In a multi-workspace deployment this silently breaks HMAC signature verification for every workspace after the first: the receiver rejects Linear's deliveries with 401 {"error":"Invalid signature"} (live-hit on demo-abca after a re-auth). Root cause (linear.ts setup action): the mirror was guarded only on `stackWideAlreadyConfigured`, and the OAuth bundle is re-upserted fresh before that block, so the existing per-workspace `webhook_signing_secret` was already dropped and then replaced with the wrong (stack-wide) value. Fix: - New pure helper `resolveWebhookSecretAction(existingPerWorkspaceSecret, stackWideConfigured)` → preserve | mirror-stackwide | prompt. An existing valid per-workspace secret always wins (rotation stays `update-webhook-secret`'s job); else mirror the stack-wide secret (with a warning to verify for an additional workspace); else prompt. 5 unit tests incl. the multi-workspace re-run case. - `setup` reads the existing per-workspace secret BEFORE the OAuth overwrite and preserves it; the mirror path now warns that an additional workspace's Linear secret differs. cli build green (610 tests, compile + eslint clean). * fix(cli): address aws-samples#612 review — fail-closed webhook pre-read + tests + nits B1 (fail-open re-armed the clobber): the pre-read catch swallowed EVERY GetSecretValue error and treated it as 'first install — nothing to preserve'. That only holds for ResourceNotFoundException; for AccessDenied / KMSAccessDenied / Throttling / network / a corrupt bundle — all on a secret that EXISTS — existingWebhookSecret stayed undefined → resolveWebhookSecretAction mirrors the stack-wide (workspace #1's) secret over this workspace's real one, re-arming the aws-samples#611 401 clobber silently behind a green 'Setup complete'. Extracted the pre-read into readExistingWebhookSecret() which FAILS CLOSED: only ResourceNotFound → 'none to preserve'; a corrupt bundle (JSON.parse) or any other SM error propagates, and setup re-throws an actionable CliError with an IAM/KMS hint (mirroring the sibling isWebhookSecretConfigured). B2 (fix was untested end-to-end): the 5 prior tests only exercised the pure resolveWebhookSecretAction. Added 7 tests for readExistingWebhookSecret covering the load-bearing seam — preserve-not-clobber, ResourceNotFound→undefined, malformed/absent secret, and the fail-closed THROW on AccessDenied / KMS / Throttling / corrupt JSON (the exact B1 path). N1 (stale comment): the Step-6 header still described the REMOVED behavior ('the stored value is this workspace's secret — lift it in'). Rewrote it to describe the preserve → mirror-stackwide → prompt decision so a maintainer can't revert the fix. N2 (double-write + failure window): fold the preserved secret into the INITIAL bundle so the OAuth-secret write lands it in ONE PutSecretValue; the preserve path skips the later re-write (guarded by !== stored.webhook_signing_secret). Closes the window where a throw between the two writes left the bundle persisted without the secret (401 until update-webhook-secret). Full cli build green (616 tests). * test(cli): end-to-end setup regression for the second-workspace webhook-secret clobber (aws-samples#612 review B2 / aws-samples#611 AC#3) Scott's re-review approved with one should-fix follow-up (B2): the safety seam (readExistingWebhookSecret fail-closed) and the pure decision (resolveWebhookSecretAction) were unit-tested, but nothing drove the `setup` ACTION end-to-end to prove issue aws-samples#611's AC#3 against the final PutSecretValue payload — linear.ts's secretAction branch + mirror-back guard were uncovered. New test drives `bgagent linear setup demo --no-browser` through parseAsync with the OAuth flow mocked (awaitOauthCallback echoes the wizard's generated state, scraped from the printed auth URL; exchangeAuthorizationCode + fetch + SecretsManager + CFN + DynamoDB stubbed), for the second-workspace re-run: - prior per-workspace bundle carries lin_wh_thisWorkspace, - stack-wide holds a DIFFERENT lin_wh_otherWorkspace (the aws-samples#611 clobber source). Asserts the final per-workspace PutSecretValue payload keeps lin_wh_thisWorkspace (preserve, folded into the initial bundle — never the stack-wide other), that the stack-wide ARN is never overwritten by setup, and that the registry row is still written (setup ran to completion). A refactor that re-broke the wiring now fails a test, not just inspection. Partial-mocks linear-oauth (keeps the real readExistingWebhookSecret / resolveWebhookSecretAction under test; stubs only exchangeAuthorizationCode). cli gate green: 621 tests, compile + eslint clean. --------- Co-authored-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
1 parent 6b40f5f commit 7ef21dc

4 files changed

Lines changed: 512 additions & 13 deletions

File tree

cli/src/commands/linear.ts

Lines changed: 82 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@ import {
4343
generatePkce,
4444
LINEAR_OAUTH_SECRET_PREFIX,
4545
linearOauthSecretName,
46+
readExistingWebhookSecret,
47+
resolveWebhookSecretAction,
4648
StoredLinearOauthToken,
4749
} from '../linear-oauth';
4850
import { awaitOauthCallback, CALLBACK_URL } from '../oauth-callback-server';
@@ -601,6 +603,42 @@ export function makeLinearCommand(): Command {
601603
process.stdout.write(' → Storing OAuth token...');
602604
const sm = new SecretsManagerClient({ region });
603605
const now = new Date().toISOString();
606+
// Preserve any EXISTING per-workspace webhook signing secret before the
607+
// OAuth overwrite below. Re-running `setup` on an already-installed
608+
// workspace must NOT clobber its working signing secret with the
609+
// stack-wide fallback (which belongs to whichever workspace installed
610+
// first) — that silently breaks signature verification (401 "Invalid
611+
// signature") for every workspace after the first in a multi-workspace
612+
// deployment. Rotation stays the job of `update-webhook-secret`.
613+
// Fail CLOSED (#612 review B1): read this workspace's existing signing
614+
// secret before the overwrite. Only ResourceNotFoundException is a clean
615+
// first-install; any other SM error (AccessDenied, KMSAccessDenied,
616+
// Throttling, network) — or a corrupt bundle — must surface, NOT default
617+
// to undefined (which would mirror the stack-wide secret over a working
618+
// per-workspace one → the #611 401 clobber, silently, behind a green
619+
// "Setup complete"). Extracted to readExistingWebhookSecret + unit-tested.
620+
let existingWebhookSecret: string | undefined;
621+
try {
622+
existingWebhookSecret = await readExistingWebhookSecret(
623+
async () => {
624+
const prior = await sm.send(new GetSecretValueCommand({ SecretId: linearOauthSecretName(slug) }));
625+
return prior.SecretString;
626+
},
627+
(err) => (err as { name?: string }).name === 'ResourceNotFoundException',
628+
);
629+
} catch (err) {
630+
const errorName = (err as { name?: string }).name;
631+
const message = err instanceof Error ? err.message : String(err);
632+
throw new CliError(
633+
`Failed to read the existing Linear webhook secret for '${slug}' before re-write: `
634+
+ `${errorName ?? 'Error'}: ${message}. Refusing to proceed — continuing could clobber `
635+
+ 'this workspace\'s webhook signing secret with the stack-wide value (a 401 on every '
636+
+ 'delivery). Likely an IAM/KMS gap: confirm your CLI principal has '
637+
+ '`secretsmanager:GetSecretValue` (and kms:Decrypt if a CMK) on '
638+
+ `${linearOauthSecretName(slug)}, or run \`bgagent linear update-webhook-secret ${slug}\` `
639+
+ 'after fixing access.',
640+
);
641+
}
604642
const stored: StoredLinearOauthToken = {
605643
access_token: tokenResponse.access_token,
606644
refresh_token: tokenResponse.refresh_token ?? '',
@@ -615,6 +653,14 @@ export function makeLinearCommand(): Command {
615653
installed_at: now,
616654
updated_at: now,
617655
installed_by_platform_user_id: cognitoSub,
656+
// Fold the preserved secret into the INITIAL bundle (#612 review N2):
657+
// the OAuth-secret write below then lands the correct bundle in ONE
658+
// PutSecretValue, and the preserve path skips the later re-write. This
659+
// also closes a narrow window — if any fallible step between the two
660+
// writes threw, the bundle was left persisted WITHOUT the secret (401
661+
// until update-webhook-secret). Only set for a real `lin_wh_` value;
662+
// mirror-stackwide / prompt cases add it in the mirror-back below.
663+
...(existingWebhookSecret ? { webhook_signing_secret: existingWebhookSecret } : {}),
618664
};
619665
if (!stored.refresh_token) {
620666
throw new CliError(
@@ -676,19 +722,40 @@ export function makeLinearCommand(): Command {
676722
// without re-onboarding. Multi-workspace installs need each
677723
// workspace to own its own per-workspace signing secret — only
678724
// the FIRST install can populate the stack-wide one usefully.
679-
// If stack-wide is already populated, this is either a re-run
680-
// of setup on the SAME workspace or the FIRST workspace of a
681-
// future multi-workspace install. Either way the stored value
682-
// is this workspace's signing secret — lift it into the
683-
// per-workspace bundle without prompting (auto-migration to
684-
// the new shape). Rotation is not setup's job: use
685-
// `bgagent linear update-webhook-secret <slug>` to rotate the
686-
// signing secret without re-running OAuth.
725+
//
726+
// resolveWebhookSecretAction picks one of three outcomes from
727+
// (existingWebhookSecret, stackWideAlreadyConfigured):
728+
// • preserve — this workspace ALREADY has its own `lin_wh_`
729+
// secret (a re-run). Keep it; do NOT overwrite from
730+
// the stack-wide fallback, which is a DIFFERENT
731+
// workspace's secret once >1 is installed. This is
732+
// the #611 clobber fix — the stack-wide value is NOT
733+
// necessarily this workspace's.
734+
// • mirror-stackwide — no per-workspace secret yet but stack-wide is
735+
// set: mirror it (correct for the first/only
736+
// workspace; warn for an additional one).
737+
// • prompt — nothing to go on: ask for the signing secret.
738+
// Rotation is not setup's job: use
739+
// `bgagent linear update-webhook-secret <slug>` to rotate.
687740
const stackWideAlreadyConfigured = await isWebhookSecretConfigured(sm, webhookSecretArn!);
688741
let webhookSigningSecret: string | undefined;
689742

690-
if (stackWideAlreadyConfigured) {
691-
console.log(' ✓ Webhook signing secret already configured stack-wide (mirroring to per-workspace)');
743+
const secretAction = resolveWebhookSecretAction(existingWebhookSecret, stackWideAlreadyConfigured);
744+
if (secretAction.kind === 'preserve') {
745+
// This workspace already had its own signing secret — keep it. Do NOT
746+
// overwrite from the stack-wide fallback (a DIFFERENT workspace's
747+
// secret once >1 is installed). Re-run case; rotation is
748+
// `update-webhook-secret`'s job, not setup's.
749+
console.log(' ✓ Preserving this workspace\'s existing webhook signing secret (re-run — not overwriting)');
750+
webhookSigningSecret = secretAction.secret;
751+
} else if (secretAction.kind === 'mirror-stackwide') {
752+
// No per-workspace secret yet, but the stack-wide one is set. Safe to
753+
// mirror ONLY when this is the first/only workspace — for a genuinely
754+
// new ADDITIONAL workspace the stack-wide secret is the wrong one, so
755+
// warn that the operator should verify (or run `update-webhook-secret`).
756+
console.log(' ✓ No per-workspace secret yet; mirroring the stack-wide signing secret');
757+
console.log(' (if this is an ADDITIONAL workspace, its Linear webhook secret differs —');
758+
console.log(` run \`bgagent linear update-webhook-secret ${slug}\` with this workspace's secret.)`);
692759
try {
693760
const value = await sm.send(new GetSecretValueCommand({ SecretId: webhookSecretArn! }));
694761
if (value.SecretString && value.SecretString.startsWith('lin_wh_')) {
@@ -725,9 +792,11 @@ export function makeLinearCommand(): Command {
725792
webhookSigningSecret = webhookSecret;
726793
}
727794

728-
// Mirror into the per-workspace OAuth secret so the receiver can
729-
// look it up by orgId. Re-upsert with the merged payload.
730-
if (webhookSigningSecret) {
795+
// Mirror into the per-workspace OAuth secret so the receiver can look it
796+
// up by orgId. In the PRESERVE case the secret is already in `stored`
797+
// (folded above) and was written by the OAuth-secret upsert — no re-write
798+
// needed. Only mirror-stackwide / prompt produce a NEW secret to persist.
799+
if (webhookSigningSecret && webhookSigningSecret !== stored.webhook_signing_secret) {
731800
const merged: StoredLinearOauthToken = {
732801
...stored,
733802
webhook_signing_secret: webhookSigningSecret,

cli/src/linear-oauth.ts

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,3 +293,88 @@ function isLinearTokenResponse(value: unknown): value is LinearTokenResponse {
293293
export function computeExpiresAt(expiresInSeconds: number, now: Date = new Date()): string {
294294
return new Date(now.getTime() + expiresInSeconds * 1000).toISOString();
295295
}
296+
297+
/** What `bgagent linear setup` should do about the webhook signing secret. */
298+
export type WebhookSecretAction =
299+
/** This workspace already has its own signing secret — keep it (re-run). */
300+
| { readonly kind: 'preserve'; readonly secret: string }
301+
/** No per-workspace secret; mirror the stack-wide one (safe for the first
302+
* workspace, ambiguous for an additional one — caller should warn). */
303+
| { readonly kind: 'mirror-stackwide' }
304+
/** No secret anywhere — prompt the operator for it (first install). */
305+
| { readonly kind: 'prompt' };
306+
307+
/**
308+
* Decide which webhook signing secret `setup` should use, WITHOUT clobbering a
309+
* working per-workspace secret.
310+
*
311+
* The bug this fixes: re-running `setup` on an already-installed workspace used
312+
* to lift the *stack-wide* signing secret into the per-workspace bundle. That's
313+
* correct only for the FIRST workspace — the stack-wide secret belongs to
314+
* whichever workspace installed first, so for any additional workspace it
315+
* overwrites the correct per-workspace secret with the wrong one, silently
316+
* breaking signature verification (webhook 401 "Invalid signature").
317+
*
318+
* Rule: an existing valid per-workspace secret always wins (rotation is
319+
* `update-webhook-secret`'s job); else mirror the stack-wide secret if present;
320+
* else prompt. Pure — the caller supplies what it read from Secrets Manager.
321+
*
322+
* @param existingPerWorkspaceSecret the `webhook_signing_secret` on this
323+
* workspace's OAuth bundle BEFORE the setup rewrite (undefined if none).
324+
* @param stackWideConfigured whether the stack-wide fallback secret is set.
325+
*/
326+
export function resolveWebhookSecretAction(
327+
existingPerWorkspaceSecret: string | undefined,
328+
stackWideConfigured: boolean,
329+
): WebhookSecretAction {
330+
if (existingPerWorkspaceSecret?.startsWith('lin_wh_')) {
331+
return { kind: 'preserve', secret: existingPerWorkspaceSecret };
332+
}
333+
if (stackWideConfigured) {
334+
return { kind: 'mirror-stackwide' };
335+
}
336+
return { kind: 'prompt' };
337+
}
338+
339+
/**
340+
* Read this workspace's EXISTING `webhook_signing_secret` before `setup`
341+
* overwrites the OAuth bundle — FAIL CLOSED (#612 review B1).
342+
*
343+
* `fetchSecretString` fetches the prior per-workspace OAuth `SecretString`
344+
* (typically a `SecretsManagerClient.send(GetSecretValueCommand)` wrapper). The
345+
* value fed to {@link resolveWebhookSecretAction} decides whether the existing
346+
* secret is preserved or the stack-wide one is mirrored over it — so a wrong
347+
* `undefined` here re-arms the #611 clobber. Therefore:
348+
*
349+
* • secret missing (`ResourceNotFoundException`) → genuine first install,
350+
* return undefined (nothing to preserve).
351+
* • secret present with a valid `lin_wh_…` `webhook_signing_secret` → return it.
352+
* • secret present but no/malformed secret → return undefined (nothing valid
353+
* to preserve; the caller mirrors/prompts).
354+
* • ANY OTHER error (AccessDenied, KMSAccessDenied, Throttling, network, or a
355+
* corrupt-JSON bundle) → THROW. Swallowing it would leave undefined and
356+
* silently clobber a working secret behind a green "Setup complete".
357+
*
358+
* `isNotFound` classifies the fetch error; callers pass an
359+
* `name === 'ResourceNotFoundException'` check. On a throw the caller re-raises
360+
* a CliError with an IAM/KMS hint (mirroring `isWebhookSecretConfigured`).
361+
*/
362+
export async function readExistingWebhookSecret(
363+
fetchSecretString: () => Promise<string | undefined>,
364+
isNotFound: (err: unknown) => boolean,
365+
): Promise<string | undefined> {
366+
let raw: string | undefined;
367+
try {
368+
raw = await fetchSecretString();
369+
} catch (err) {
370+
if (isNotFound(err)) return undefined; // genuine first install
371+
throw err; // fail closed — caller wraps with an actionable CliError
372+
}
373+
if (!raw) return undefined;
374+
// A corrupt-but-present bundle must surface (JSON.parse throws → propagates),
375+
// NOT silently become "nothing to preserve" → mirror-stackwide.
376+
const bundle = JSON.parse(raw) as Partial<StoredLinearOauthToken>;
377+
return bundle.webhook_signing_secret?.startsWith('lin_wh_')
378+
? bundle.webhook_signing_secret
379+
: undefined;
380+
}

0 commit comments

Comments
 (0)