fix(jira): repair webhook admission and secret rotation - #710
fix(jira): repair webhook admission and secret rotation#710ayushtr-aws wants to merge 8 commits into
Conversation
scottschreckengaust
left a comment
scottschreckengaust
left a comment
There was a problem hiding this comment.
1. Verdict
Request changes — the functional fix is sound and well-tested, but the PR silently trades away a documented ADR-015 tenet (per-tenant signature binding / multi-tenant support for Jira) without updating that ADR, and it ships a documentation/comment set that now contradicts itself in three places. Neither is a large amount of work; the code changes themselves are close to mergeable.
2. Vision alignment
Mostly aligned, with one undocumented tenet trade.
Aligned:
- Bounded blast radius — making unmapped/removed Jira projects a true no-op for every event type (
cdk/src/handlers/jira-webhook-processor.ts:303-317,getActiveProjectMappingat:1047) is exactly right. A site-wide admin-console webhook that posts ABCA comments into projects that never opted in is unbounded blast radius by any reading, and the oldsafeReportIssueFailureon the unmapped path was that bug. Good fix, and the tests pin it (cdk/test/handlers/jira-webhook-processor.test.ts:385-407,:864-873). - Fire-and-forget — no change to the async path;
update-webhook-secretis an operator-plane command, not a task-plane one. - Reviewable outcomes — the added attribution diagnostics (
jira_account_source,jira_identity_lookup_key) plus the new troubleshooting section are a real observability win over the previous "isn't linked to a platform user, runbgagent jira link" dead-end, which did not tell the operator which account to link.
Trade-off that needs an ADR update (see Blocking #1): ADR-015 §Multi-tenant signature binding states the design intent explicitly: the stack-wide secret "is not copied into later tenants' bundles", and the per-tenant secret "proves which tenant signed a delivery". This PR inverts that: bgagent jira setup now refuses outright to onboard a second active Jira tenant (cli/src/commands/jira.ts:701-703). That is a defensible decision — one webhook URL genuinely cannot select among N secrets when the payload omits cloudId — but it converts "multi-tenant with per-tenant binding" into "single-tenant by hard constraint," and ADR-015 (lines 44, 49-51, 63) still asserts the opposite. Per the review process, an undocumented tenet trade is a blocking concern.
3. Blocking issues
B1 — ADR-015 not updated for the single-active-tenant constraint (cli/src/commands/jira.ts:701)
setup now throws multiTenantWebhookError whenever any other active tenant exists in the registry. That is a hard product constraint on the Jira channel, and it directly contradicts three ADR-015 statements that remain unedited on this branch:
- line 44: "The stack-wide secret is seeded only once (from the first tenant) for single-tenant back-compat — it is not copied into later tenants' bundles"
- lines 49-51 (§Multi-tenant signature binding): describes the receiver as preserving "the fail-closed multi-tenant guarantee"
- line 63: "(+) Per-tenant credential isolation, signature binding, and the changelog-diff trigger keep the trust and re-trigger semantics correct for multi-tenant installs"
Also stale on line 68: "(!) rotating it in Jira without re-running bgagent jira setup causes silent 401s" — the remedy is now update-webhook-secret.
Risk: the next contributor reads ADR-015, believes multi-tenant Jira is supported and intentionally per-tenant-bound, and either re-adds the second-tenant path or builds on the assumption. The guide note added at docs/guides/JIRA_SETUP_GUIDE.md:150 is good but a guide does not override an ADR.
Fix: amend ADR-015 in this PR (a Superseded by #709 / Revision section is fine, no new ADR needed) to state (a) admin-console webhooks omit cloudId, (b) the Jira channel therefore supports exactly one active tenant, (c) the stack-wide secret is now a synchronized copy of that tenant's secret rather than a first-tenant-only seed, and (d) the rotation command. Then mise //docs:sync.
B2 — The multi-tenant guard in setup is completely untested (cli/src/commands/jira.ts:699-704)
The rotation command's identical guard has four dedicated tests (cli/test/commands/jira.test.ts:471, :493, :539, :561). The setup guard — which is the security-relevant one, because it is what prevents an operator from onboarding tenant B and thereby silently overwriting tenant A's stack-wide verifier — has none. The only new setup test (cli/test/commands/jira.test.ts:1044) mocks the Scan to return exactly [cloud-123], i.e. the happy path.
This matters more than a normal coverage gap because of the guard's placement: it runs after the OAuth dance and before the secret/registry writes. A future refactor that moves the makeDocClient/Scan below the upsertOauthSecret call would leave tenant B's OAuth bundle and registry row written while the command throws — a half-onboarded tenant with an active registry row. Nothing in the suite would catch that reordering.
Fix: add two setup tests — (a) a Scan returning a different active tenant rejects with /multiple tenant secrets/ and asserts smSend was never called (so the ordering invariant is pinned, matching the expect(smSend).not.toHaveBeenCalled() assertion already used at cli/test/commands/jira.test.ts:490); (b) a Scan returning [] for a first-ever install still proceeds (the otherActiveTenantIds filter makes this pass today — worth pinning, since the rotation path treats empty as fatal and the asymmetry is easy to "fix" wrongly later).
B3 — Comments and type docs now contradict the implemented behavior (3 sites)
The PR carefully rewrote the CDK-side comments but missed the mirror copies, leaving the codebase asserting both the old and new models:
cli/src/jira-oauth.ts:114-124—StoredJiraOauthToken.webhook_signing_secretstill reads "Webhook subscriptions are tenant-scoped, so a single stack-wide signing secret cannot verify events from multiple tenants" and "the receiver falls back to the stack-wideJIRA_WEBHOOK_SECRET_ARN". The CDK twin atcdk/src/handlers/shared/jira-oauth-resolver.ts:93-102was updated in this diff to say the opposite. These two type docs describe the same wire field and are now in direct conflict. (The repo already treats CLI/CDK shared-shape drift as a first-class hazard — see the AGENTS.md routing table entry forshared/types.ts↔cli/src/types.ts.)cdk/src/constructs/jira-integration.ts:52-56— the new comment claims "The explicit JSON placeholder can never accidentally match a valid operator HMAC secret." WithisWebhookSecretPlaceholderdeleted, nothing in the codebase reads the marker key any more; the placeholder's recognizability is now dead information. The comment describes a property no consumer depends on, and a reader will hunt for the (now nonexistent) recognizer. Say instead that the JSON shape is a non-verifiable initial value thatsetupunconditionally overwrites.cdk/test/constructs/jira-integration.test.ts:61-70— untouched by this PR, and its rationale is now false: "the webhook secret MUST seed an explicit JSON placeholder so the CLI can distinguish 'never configured' from an operator-set value." The CLI no longer distinguishes anything —setupalways overwrites. The test still asserts a real property (the seeded value isn't a bare random string an attacker could brute-force-shape), but the stated reason is the deleted #368 heuristic. A test whose comment justifies it by a deleted mechanism is how the assertion gets deleted next.
Fix: update all three to the synchronize-always model. Keep the assertion in (3); rewrite only its why.
4. Non-blocking suggestions / nits
cdk/src/handlers/jira-webhook-processor.ts:391—jira_actor_display_nameis new PII in CloudWatch. Every other identity field added in this hunk is an opaque Atlassian accountId;displayNameis a human name. It is the only PII-shaped field logged by any Jira handler (verified:grepfordisplay_name:acrosscdk/src/handlers/returns exactly this one line). The repo has an explicit PII-redaction posture elsewhere (docs/design/SECURITY.md:170,deny-reason-scanner.ts). The accountId +jira_identity_lookup_keyalready give the operator everything needed to runinvite-user, so the display name buys little. Suggest dropping it, or at minimum note in the PR why it is needed.cli/src/commands/jira.ts:311-336— the rollback is best-effort by construction and worth saying so. If the process is killed between the tenantPutSecretValueand the stack-wide one, the two copies diverge with no rollback. That is acceptable (the remedy is re-runningupdate-webhook-secret, and the guide already says "Keep the Jira webhook disabled until the command succeeds"), but the function's docstring reads as if two-phase durability is guaranteed. One sentence — "not atomic; a crash between writes leaves the copies divergent, re-run to converge" — would prevent a future reader trusting it too far.cli/src/commands/jira.ts:998+:776—GetSecretValue→PutSecretValueread-modify-write on the shared OAuth bundle. BothsynchronizeJiraWebhookSecretscall sites re-read the bundle and write it back whole. This races the Lambda-side token refresher (cdk/src/handlers/shared/jira-oauth-resolver.ts, which holdsPutSecretValueand rotatesrefresh_tokenon every use — ADR-015 line 55 warns that losing a rotated refresh token bricks the tenant). The window is a few hundred ms of an interactive operator command, and the pre-existinginvite-userpath (:1067-1096) has the identical pattern, so this is not newly introduced and not blocking. Butsetupnow performs an extra read-back at:776that did not exist before, widening the window slightly. If you want to close it,PutSecretValueaccepts aClientRequestToken, or aVersionId-conditioned write would make it CAS-like.ConsistentRead: trueblanket application. Correct for the read-your-writes bugs this fixes (jira-link.ts:70,lookupPlatformUser,getActiveProjectMapping, the registry rows) and cheap on PAY_PER_REQUEST at this volume. Note it does double the RCU cost of the registryScaninresolveSoleTenantCloudId(cdk/src/handlers/jira-webhook-processor.ts:145) on every admin-console delivery. Given the table is one row per tenant that is genuinely negligible; flagging only so it is a known cost rather than an accident.lookupPlatformUsertightened fromstatus !== 'pending'tostatus !== 'active'(cdk/src/handlers/jira-webhook-processor.ts:1036-1043). Fail-closed and correct — the only writer of a non-pending row isjira-link.ts:105, which always setsstatus: 'active'(confirmed via history: that field has been written since the original #302 landing), so there is no legacy row shape this silently un-links. Good change; calling it out because a reader might worry about back-compat.cli/src/commands/jira.ts:1115—invite-user's "already linked" pre-checkGetdid not getConsistentRead: truewhile every otherGetin the file did. It is only an advisory warning so a stale read is harmless, but the inconsistency will read as an oversight.- Branch name
fix/709-jira-webhook-admissionmatches the convention. PR body is genuinely good — it explains why, and the live-acceptance evidence (401 on bad HMAC, matching SHA-256 hashes, silent unmapped-project behavior, idempotent replay) is the kind of verification this repo should ask for more often.
5. Documentation
Updated: docs/guides/JIRA_SETUP_GUIDE.md (single-active-tenant callout, rotation command, admission-silence rule, new "Linking succeeds but a trigger says the Jira user is unlinked" section), cli/README.md (subcommand list + usage block), and the Starlight mirror docs/src/content/docs/using/Jira-setup-guide.md.
Mirror sync: verified clean. I ran node scripts/sync-starlight.mjs from docs/ at the head SHA and git status --porcelain came back empty — the mirror is regenerated and byte-consistent, with only the expected link-rewrite deltas vs. the source. This will not trip CI's "Fail build on mutation".
Missing: ADR-015 (blocking, B1). The new troubleshooting section is well-targeted at the actual customer failure — it correctly calls out that "the name shown above an ABCA comment is not proof that the same account triggered the event," which is the non-obvious part.
Issue tracking: #709 exists, carries the approved label, and its six acceptance criteria map 1:1 onto the diff. Governance satisfied.
6. Tests & CI
CI: all 8 checks green at 4dfe3e1 (CodeQL ×3 + aggregate, dead-code advisory, secrets/deps/workflow scan, PR-title lint, build (agentcore) 11m46s). Notably the "Secrets, deps, and workflow scan" is passing here — the PR body's caveat about dependency advisories is stale, since #711/#718 landed those fixes on main and this branch merged main three times (last at 9bd1bc25). Base is current: df1ebac6 is an ancestor of HEAD, so nothing is being reasoned about against a stale base. mergeStateStatus: BLOCKED reflects the missing approval, not a check failure.
Coverage — strong on the paths that were broken, one hole:
- Silent-admission behavior: thoroughly pinned, including the negative assertions that matter (
expect(reportIssueFailureMock).not.toHaveBeenCalled()) across unmapped, removed, and no-project-key states for both the label path and the comment path. The twosafeReportIssueFailureresilience tests were correctly re-pointed at the surviving report path (:949-970) rather than deleted when their old trigger became silent — that is the right instinct. ConsistentRead: asserted at each of the ~8 call sites rather than assumed.synchronizeJiraWebhookSecrets: all three branches covered — success, rollback-on-stack-wide-failure, and rollback-also-fails (cli/test/commands/jira.test.ts:1544-1620). The rollback test asserting the restored payload.toEqual(stored)is exactly the right assertion.- Hole: the
setupmulti-tenant guard (B2).
Bootstrap synth-coverage: not applicable. No new CFN resource types — the AWS::SecretsManager::Secret in jira-integration.ts is pre-existing and only its comments changed; cdk/src/bootstrap/** and BOOTSTRAP_VERSION are correctly untouched.
Test performance: no CDK synth changes; no test re-enables aws:cdk:bundling-stacks, and no new new App() + Template.fromStack() per-test patterns (#366 clean).
7. Review agents run
I must be transparent about a process gap here.
/security-review— RUN. In scope (secrets handling, webhook input gateway, HMAC admission). Findings folded in above: no HIGH/MEDIUM exploitable vulnerability introduced. Specifically checked and cleared: (a) the stack-wide secret is now equal to the sole tenant's secret rather than an independent credential, which does not weaken the receiver —jira-webhook-processor.ts:285-297still ignores a body-suppliedcloudIdon stack-wide-verified deliveries and binds to the sole active tenant, so the ADR-015 fail-closed property survives the change in substance even as its wording goes stale; (b)verifyJiraSignatureretainsisUsableHmacSecret(empty/whitespace rejected) andtimingSafeEqual; (c) the.trim()added to the prompted secret atcli/src/commands/jira.ts:768is a correctness fix, not a weakening — an untrimmed trailing newline was a plausible root cause of the reported 401s; (d)getActiveProjectMappingfails closed on a missing/non-active row. Items surfaced as non-blocking: PII in logs (nit 1), non-atomic dual-secret write (nit 2), read-modify-write race on the OAuth bundle (nit 3).pr-review-toolkit:code-reviewer— NOT RUN.pr-review-toolkit:silent-failure-hunter— NOT RUN. Clearly in scope (this PR is largely about converting loud failures to silent skips, plus a new try/catch rollback).pr-review-toolkit:type-design-analyzer— NOT RUN. In scope (getActiveProjectMappingreturnsRecord<string, unknown> | null;synchronizeJiraWebhookSecretsis a new exported contract).pr-review-toolkit:comment-analyzer— NOT RUN. In scope, and would very likely have found B3 independently.pr-review-toolkit:pr-test-analyzer— NOT RUN. In scope, and would very likely have found B2 independently.
Reason for the omissions (not a judgment that the diff was too simple): no subagent-dispatch tool was exposed in my execution context — I searched the available tool surface for Task/Agent/dispatch entry points and none resolved, so the five toolkit agents were not invocable. To compensate I hand-verified their specific scopes: I traced every error path this PR made silent against its callers, checked the new exported type contracts against both the CLI and CDK twins, grepped for stale references to every deleted symbol (isWebhookSecretConfigured, JIRA_WEBHOOK_SECRET_PLACEHOLDER_KEY, #368) across .ts/.md, and diffed the test file's negative assertions against the new control flow. B2 and B3 are the findings that surfaced. Treat this section as a known gap in this review rather than a clean bill from those five agents, and re-run them if your environment has them wired.
8. Human heuristics
- Proportionality — pass. The diff deletes more mechanism than it adds: the
isWebhookSecretPlaceholder/isWebhookSecretConfiguredheuristic pair (~70 lines plus 9 tests) goes away, replaced by "always overwrite," which is the simpler and more correct rule.getActiveProjectMappingis a genuine two-call-site extraction, not a speculative abstraction.update-webhook-secretis a real operator need (rotation without re-running OAuth), not a knob. No new factory/engine/indirection. The Scan inlistActiveJiraTenantIdsduplicatesresolveSoleTenantCloudId's logic across the CLI/CDK boundary, which is unavoidable given the package split. - Coherence — concern. The code is coherent; the prose layer is not, and it is spread across four files that now disagree about whether the stack-wide secret is a "fallback," a "synchronized copy," or a back-compat seed (B3, plus ADR-015 in B1). The repo's own term for this is drift, and it is the specific thing AGENTS.md flags for CLI/CDK shared shapes. Also:
cli/src/commands/linear.ts:176still exports the realisWebhookSecretConfiguredfor Linear, so the name now means something in one integration and nothing in the other — fine, but worth a sentence in the Jira construct comment so a reader does not go looking for the Jira twin. - Clarity — pass, with nit 1.
accountSourceas an explicit discriminator beats re-deriving the precedence at the log site. Error messages are actionable and name the real command with real positionals (bgagent jira invite-user <cloudId> <accountId>), and the test at:874-878asserts on that exact string — good, because the previous message pointed atbgagent jira link <code>which the operator could not run without a code.multiTenantWebhookErrorhandles the zero-tenant case with a distinct message rather than a confusing plural. The one clarity regression isjira_actor_display_name(nit 1) and the over-strong "can never accidentally match" claim (B3.2). - Appropriateness — pass. This is the dimension the PR is strongest on. The author verified against real Atlassian behavior on a live deployment (
bgagent-linear-vercel, us-east-1) rather than only against self-written mocks — 200 on a correctly-signed delivery, 401 on a bad HMAC, matching SHA-256 hashes across both secret locations, a real task ID and a real PR, a real unmapped-project silence check, and the other two webhooks explicitly disabled to isolate the test. That directly answers AI001, which is exactly the failure mode webhook-integration code is prone to. The tests assert what the code should do (the negativenot.toHaveBeenCalled()assertions are the load-bearing ones) rather than merely recording current behavior.
To unblock: amend ADR-015 (B1) + mise //docs:sync, add the two setup-guard tests (B2), and fix the three contradictory comments (B3). No implementation changes required — the runtime behavior in this PR is, as far as I can determine, correct.
2 participants
Fix Jira admin-console webhook verification, admission, and user-link diagnostics reported by a customer deployment.
Area
cdk— infrastructure, handlers, constructsagent— Python runtime / Docker imagecli—bgagentclientdocs— guides or design sources (docs/guides/,docs/design/)tooling— rootmise.toml, scripts, CI workflowsRelated
Closes #709
Changes
bgagent jira update-webhook-secret <cloud-id>for rotation without repeating OAuth, including rollback if the second secret write fails.cloudId.comment_created, from unmapped or removed Jira projects completely silent.Verification
Local automated checks:
mise run build: passed, including 173 CDK suites / 3,477 tests.linear-vercelfull build: passed (3,626 CDK, 670 CLI, and 1,421 agent tests, plus docs, synth, compile, and lint).linear-vercelAgentStack tests: 57 passed.Live acceptance checks on
bgagent-linear-vercelin account336879875486,us-east-1:01KZ6ZGMHP9GPY92SZA8KEAZVMand chore(validation): add Jira integration e2e test validation note ayushtr-aws/abca-testing#21.10122with success, cost, turn count, duration, and PR link.TGproduced zero Jira comments and zero tasks; logs recorded a silent onboarding skip.SCRUMissue without trigger criteria produced zero Jira comments and zero tasks.backgroundagent-devandbackgroundagent-dev2) were disabled during isolation testing and received neither test event.All current CI checks pass. This PR changes no dependency or lockfile.
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.