From 7c87dfa7e7ccc8571beebf53360ad0d6052d1f38 Mon Sep 17 00:00:00 2001 From: Andy Pai Date: Tue, 23 Jun 2026 01:28:57 -0400 Subject: [PATCH 1/2] harden(serve): close webhook-secret fail-open, tighten env interval, share int parser Follow-up to the serve+webhooks QA work (PR #76), addressing the three out-of-scope observations the xhigh code-review recorded (OBS-1/2/3). OBS-2 (security): assertTunnelSecurity hardcoded a jira/linear -> secret-env ternary that fell through to null for any other provider, so a future webhook-capable provider could start a public tunnel with no signing secret enforced. Add a single source of truth `WEBHOOK_SECRET_ENV: Record< TrackerProvider, string | null>` plus a `trackerProviderFromEnv()` normalizer in tracker-config.ts; the guard now looks both up. The Record type makes adding a provider a compile error until its secret is declared, and the normalizer derives recognized providers from the map's own keys so it stays in lockstep. OBS-1 (strictness): resolvePollingSyncIntervalMs parsed KANBAN_SYNC_INTERVAL_MS with Number(), accepting hex/scientific notation. Tighten to digits-only + Number.isSafeInteger, matching the strict --sync-interval-ms CLI flag (env path keeps INVALID_CONFIG; CLI path keeps INVALID_ARGUMENT). OBS-3 (dedup): extract `parseBoundedInt` as the single digits-only integer parser; parsePositiveInt, parsePort, and parseSyncIntervalMs all delegate to it, so a future hardening fix lands in one place. Independent codex review: APPROVE. +16 test cases; full suite 491 pass / 27 skip / 0 fail; check clean. Claude-Session: https://claude.ai/code/session_01LQjR5poSV2M3nxt8h5hNad --- src/__tests__/sync-config.test.ts | 15 ++++- .../tracker-config-webhook-secret.test.ts | 55 +++++++++++++++++ src/__tests__/transport-input.test.ts | 40 +++++++++++- src/index.ts | 61 ++++++------------- src/sync-config.ts | 8 ++- src/tracker-config.ts | 35 ++++++++++- src/transport-input.ts | 45 ++++++++++---- 7 files changed, 199 insertions(+), 60 deletions(-) create mode 100644 src/__tests__/tracker-config-webhook-secret.test.ts diff --git a/src/__tests__/sync-config.test.ts b/src/__tests__/sync-config.test.ts index 6e7a647..32483ef 100644 --- a/src/__tests__/sync-config.test.ts +++ b/src/__tests__/sync-config.test.ts @@ -17,7 +17,20 @@ describe('sync config', () => { }) test('rejects invalid or too-aggressive intervals', () => { - for (const raw of ['999', '5s', '1000.5', '0']) { + // OBS-1: digits-only — hex/scientific notation (which Number() would accept) + // and over-long digit strings (precision loss / Infinity) are now rejected + // for the env var too, matching the strict --sync-interval-ms CLI flag. + for (const raw of [ + '999', + '5s', + '1000.5', + '0', + '0x3e8', + '1e3', + '1_000', + '9007199254740993', + '9'.repeat(309), + ]) { let err: unknown try { resolvePollingSyncIntervalMs(raw) diff --git a/src/__tests__/tracker-config-webhook-secret.test.ts b/src/__tests__/tracker-config-webhook-secret.test.ts new file mode 100644 index 0000000..0542c21 --- /dev/null +++ b/src/__tests__/tracker-config-webhook-secret.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, test } from 'bun:test' +import { WEBHOOK_SECRET_ENV, trackerProviderFromEnv } from '../tracker-config' + +// OBS-2: the provider -> webhook-secret-env mapping is a single source of truth. +// WEBHOOK_SECRET_ENV is typed Record, so adding a new +// provider to the union is a COMPILE error until its secret env is declared here +// — that compile-time exhaustiveness is the real guard against assertTunnelSecurity +// silently failing open for a new webhook-capable provider. These runtime tests +// pin the map contents and the env normalization the guard relies on. + +describe('WEBHOOK_SECRET_ENV (single source of truth)', () => { + test('maps each known provider to its secret env (local has none)', () => { + expect(WEBHOOK_SECRET_ENV).toEqual({ + local: null, + linear: 'LINEAR_WEBHOOK_SECRET', + jira: 'JIRA_WEBHOOK_SECRET', + }) + }) + + test('every non-local provider points at a *_WEBHOOK_SECRET env name', () => { + for (const [provider, envName] of Object.entries(WEBHOOK_SECRET_ENV)) { + if (provider === 'local') { + expect(envName).toBeNull() + } else { + expect(envName).toMatch(/_WEBHOOK_SECRET$/) + } + } + }) +}) + +describe('trackerProviderFromEnv', () => { + test('normalizes known providers (case/whitespace-insensitive)', () => { + expect(trackerProviderFromEnv({ KANBAN_PROVIDER: 'jira' })).toBe('jira') + expect(trackerProviderFromEnv({ KANBAN_PROVIDER: 'Linear' })).toBe('linear') + expect(trackerProviderFromEnv({ KANBAN_PROVIDER: ' JIRA ' })).toBe('jira') + expect(trackerProviderFromEnv({ KANBAN_PROVIDER: 'local' })).toBe('local') + }) + + test('falls back to local for unset / blank / unrecognized values', () => { + expect(trackerProviderFromEnv({})).toBe('local') + expect(trackerProviderFromEnv({ KANBAN_PROVIDER: '' })).toBe('local') + // A future provider not yet wired in resolves to local (no webhooks) rather + // than an unmapped value; once added to TrackerProvider it must gain a + // WEBHOOK_SECRET_ENV entry (compile-enforced). + expect(trackerProviderFromEnv({ KANBAN_PROVIDER: 'github' })).toBe('local') + }) + + test('inherited Object property names do not leak through the map lookup', () => { + // The normalizer derives from WEBHOOK_SECRET_ENV's OWN keys, so prototype + // names must not be treated as providers. + for (const name of ['constructor', 'toString', 'hasOwnProperty', '__proto__']) { + expect(trackerProviderFromEnv({ KANBAN_PROVIDER: name })).toBe('local') + } + }) +}) diff --git a/src/__tests__/transport-input.test.ts b/src/__tests__/transport-input.test.ts index 9dcab6a..9cb811c 100644 --- a/src/__tests__/transport-input.test.ts +++ b/src/__tests__/transport-input.test.ts @@ -1,5 +1,43 @@ import { describe, expect, test } from 'bun:test' -import { parsePositiveInt } from '../transport-input' +import { ErrorCode, KanbanError } from '../errors' +import { parseBoundedInt, parsePositiveInt } from '../transport-input' + +describe('parseBoundedInt', () => { + test('accepts in-range digit strings (inclusive bounds)', () => { + expect(parseBoundedInt('0', { min: 0, max: 65535, field: 'port' })).toBe(0) + expect(parseBoundedInt('65535', { min: 0, max: 65535, field: 'port' })).toBe(65535) + expect(parseBoundedInt('1000', { min: 1000, field: 'iv' })).toBe(1000) + expect(parseBoundedInt(' 42 ', { min: 1, max: 100, field: 'n' })).toBe(42) + }) + + test.each([ + ['65536', { min: 0, max: 65535, field: 'port' }], + ['999', { min: 1000, field: 'iv' }], + ['-1', { min: 0, max: 65535, field: 'port' }], + ['3.5', { min: 0, max: 65535, field: 'port' }], + ['0x10', { min: 0, max: 65535, field: 'port' }], + ['1e3', { min: 1000, field: 'iv' }], + ['', { min: 0, max: 65535, field: 'port' }], + ['9007199254740993', { min: 1000, field: 'iv' }], // past MAX_SAFE_INTEGER → precision loss + ['9'.repeat(309), { min: 1000, field: 'iv' }], // Number() → Infinity + ] as const)('rejects %p as INVALID_ARGUMENT', (value, opts) => { + expect(() => parseBoundedInt(value, opts)).toThrow(KanbanError) + try { + parseBoundedInt(value, opts) + } catch (err) { + expect((err as KanbanError).code).toBe(ErrorCode.INVALID_ARGUMENT) + } + }) + + test('message form depends on whether max is bounded', () => { + expect(() => parseBoundedInt('x', { min: 0, max: 65535, field: 'port' })).toThrow( + /port must be an integer between 0 and 65535/, + ) + expect(() => parseBoundedInt('x', { min: 1000, field: '--sync-interval-ms' })).toThrow( + /--sync-interval-ms must be an integer >= 1000/, + ) + }) +}) describe('parsePositiveInt', () => { test('returns undefined for absent values', () => { diff --git a/src/index.ts b/src/index.ts index e13f0b7..2b4bd5d 100755 --- a/src/index.ts +++ b/src/index.ts @@ -9,11 +9,11 @@ import { boardInit, boardReset } from './commands/board' import { columnAdd, columnDelete, columnList, columnRename, columnReorder } from './commands/column' import { bulkClearDoneCmd, bulkMoveAllCmd } from './commands/bulk' import { getConfigPath, loadConfig, saveConfig } from './config' -import { parsePositiveInt } from './transport-input' +import { parseBoundedInt, parsePositiveInt } from './transport-input' import type { CliOutput, Priority, ProviderCapabilities } from './types' import { unsupportedOperation } from './providers/errors' import { openKanbanRuntime } from './provider-runtime' -import { trackerConfigFromEnv } from './tracker-config' +import { WEBHOOK_SECRET_ENV, trackerConfigFromEnv, trackerProviderFromEnv } from './tracker-config' import type { KanbanProvider } from './providers/types' import { MIN_POLLING_SYNC_INTERVAL_MS } from './sync-config' import { normalizeCreateTaskInput } from './use-cases' @@ -511,44 +511,22 @@ export function parseServeArgs(argv: string[]): ServeOptions { } } +// Strict digits-only CLI contract via the shared parseBoundedInt (rejects +// hex/scientific, the Number() overflow/precision-loss of an over-long digit +// string, and values below the minimum — all as INVALID_ARGUMENT). The shared +// resolvePollingSyncIntervalMs (sync-config.ts) is the env-path parser; this +// flag is validated strictly here. function parseSyncIntervalMs(raw: string): number { - // Strict digits-only CLI contract (matching --port): reject non-digit input, - // the Number() overflow/precision-loss of an over-long digit string - // (isSafeInteger also excludes Infinity and values past MAX_SAFE_INTEGER), and - // values below the minimum — all as INVALID_ARGUMENT. The shared - // resolvePollingSyncIntervalMs (sync-config.ts) is intentionally more lenient - // (Number() accepts hex/scientific; isInteger accepts past MAX_SAFE_INTEGER) - // and reports INVALID_CONFIG, so validate here and return the parsed value - // directly rather than re-parsing the same string through it. - const trimmed = raw.trim() - const parsed = Number(trimmed) - if ( - !/^\d+$/.test(trimmed) || - !Number.isSafeInteger(parsed) || - parsed < MIN_POLLING_SYNC_INTERVAL_MS - ) { - throw new KanbanError( - ErrorCode.INVALID_ARGUMENT, - `--sync-interval-ms must be an integer >= ${MIN_POLLING_SYNC_INTERVAL_MS}`, - ) - } - return parsed + return parseBoundedInt(raw, { min: MIN_POLLING_SYNC_INTERVAL_MS, field: '--sync-interval-ms' }) } // `parseInt` silently accepts `123abc` (→123) and `-1`, and yields NaN for -// non-numeric input, so validate the port explicitly: digits only, 0–65535 -// (0 lets the OS pick an ephemeral port). Throws so a bad value surfaces as the -// structured INVALID_ARGUMENT envelope rather than booting on a garbage port. +// non-numeric input, so validate the port explicitly via parseBoundedInt: digits +// only, 0–65535 (0 lets the OS pick an ephemeral port). Throws so a bad value +// surfaces as the structured INVALID_ARGUMENT envelope rather than booting on a +// garbage port. function parsePort(raw: string, label: string): number { - const trimmed = raw.trim() - const port = Number(trimmed) - if (!/^\d+$/.test(trimmed) || port > 65535) { - throw new KanbanError( - ErrorCode.INVALID_ARGUMENT, - `${label} must be an integer between 0 and 65535`, - ) - } - return port + return parseBoundedInt(raw, { min: 0, max: 65535, field: label }) } /** @@ -573,13 +551,12 @@ export function assertTunnelSecurity( 'Set KANBAN_API_TOKEN or pass --token .', ) } - const providerType = (env['KANBAN_PROVIDER'] ?? 'local').trim().toLowerCase() - const webhookSecretEnv = - providerType === 'jira' - ? 'JIRA_WEBHOOK_SECRET' - : providerType === 'linear' - ? 'LINEAR_WEBHOOK_SECRET' - : null + const providerType = trackerProviderFromEnv(env) + // Single source of truth (WEBHOOK_SECRET_ENV is Record), so + // a new webhook-capable provider can't be added without declaring its secret + // here — closing the fail-open hole where an unmapped provider would start a + // public tunnel with no signature secret enforced. + const webhookSecretEnv = WEBHOOK_SECRET_ENV[providerType] if (webhookSecretEnv && !env[webhookSecretEnv]) { throw new KanbanError( ErrorCode.INVALID_ARGUMENT, diff --git a/src/sync-config.ts b/src/sync-config.ts index 092a09f..737db30 100644 --- a/src/sync-config.ts +++ b/src/sync-config.ts @@ -10,8 +10,12 @@ export function resolvePollingSyncIntervalMs( const value = raw?.trim() if (!value) return DEFAULT_POLLING_SYNC_INTERVAL_MS - const parsed = Number(value) - if (!Number.isInteger(parsed) || parsed < MIN_POLLING_SYNC_INTERVAL_MS) { + // Plain decimal digits only: reject hex/scientific notation (`0x3e8`, `1e3`) + // that `Number()` would otherwise coerce, and use `isSafeInteger` so an + // over-long digit string rounded past MAX_SAFE_INTEGER / overflowed to + // Infinity is rejected rather than silently accepted with precision loss. + const parsed = /^\d+$/.test(value) ? Number(value) : NaN + if (!Number.isSafeInteger(parsed) || parsed < MIN_POLLING_SYNC_INTERVAL_MS) { throw new KanbanError( ErrorCode.INVALID_CONFIG, `${opts.label ?? 'KANBAN_SYNC_INTERVAL_MS'} must be an integer >= ${MIN_POLLING_SYNC_INTERVAL_MS}`, diff --git a/src/tracker-config.ts b/src/tracker-config.ts index 9e86e6d..031dce9 100644 --- a/src/tracker-config.ts +++ b/src/tracker-config.ts @@ -4,6 +4,39 @@ import { resolvePollingSyncIntervalMs } from './sync-config' export type TrackerProvider = 'local' | 'linear' | 'jira' +/** + * Single source of truth for which env var holds each provider's webhook signing + * secret (`null` = the provider has no webhook ingestion). Typed as + * `Record`, so adding a new provider to the union is a + * compile error here until its secret env is declared — preventing a new + * webhook-capable provider from silently slipping past the tunnel-security gate + * (assertTunnelSecurity) with no secret enforced. + */ +export const WEBHOOK_SECRET_ENV: Record = { + local: null, + linear: 'LINEAR_WEBHOOK_SECRET', + jira: 'JIRA_WEBHOOK_SECRET', +} + +/** + * Normalize the raw `KANBAN_PROVIDER` env value to a known `TrackerProvider`, + * falling back to `'local'` for anything unset/unrecognized — matching how + * trackerConfigFromEnv resolves the provider. Centralized so the security gate + * and the config loader agree on a single derivation. + */ +export function trackerProviderFromEnv( + env: Record = process.env, +): TrackerProvider { + const raw = (env['KANBAN_PROVIDER'] ?? 'local').trim().toLowerCase() + // Recognize exactly the providers declared in WEBHOOK_SECRET_ENV — own keys + // only, so inherited names like "constructor"/"toString" don't match — and + // fall back to local otherwise. Deriving from the map keeps this normalizer in + // lockstep with it, so a newly-added provider is picked up here automatically. + return Object.prototype.hasOwnProperty.call(WEBHOOK_SECRET_ENV, raw) + ? (raw as TrackerProvider) + : 'local' +} + export interface LocalTrackerConfig { provider: 'local' defaultColumns?: string[] @@ -34,7 +67,7 @@ export type TrackerConfig = LocalTrackerConfig | LinearTrackerConfig | JiraTrack export function trackerConfigFromEnv( env: Record = process.env, ): TrackerConfig { - const provider = (env['KANBAN_PROVIDER'] ?? 'local').trim().toLowerCase() + const provider = trackerProviderFromEnv(env) if (provider === 'linear') { const apiKey = env['LINEAR_API_KEY'] diff --git a/src/transport-input.ts b/src/transport-input.ts index 750e042..02e7269 100644 --- a/src/transport-input.ts +++ b/src/transport-input.ts @@ -1,13 +1,35 @@ import { ErrorCode, KanbanError } from './errors' /** - * Parse an optional positive integer supplied through a transport boundary - * (HTTP query string, CLI flag). Returns `undefined` when the value is absent, - * and throws a `KanbanError(INVALID_ARGUMENT)` for anything that is not a - * positive integer — `NaN`, negatives, zero, decimals, and trailing-garbage - * strings like `"5abc"`. Centralizing this keeps invalid limits from leaking - * into provider logic, where they would otherwise produce inconsistent - * behavior across SQL `LIMIT`, `Array.prototype.slice`, and Postgres. + * Parse a required integer supplied through a transport boundary (HTTP query + * string, CLI flag) into the inclusive range `[min, max]` (max defaults to + * `MAX_SAFE_INTEGER`). Accepts plain decimal digits only and rejects signs, + * decimals, and exponent/hex forms like `"1e100"` / `"0x10"` that `Number()` + * would otherwise coerce to an integer. `isSafeInteger` also rejects an + * over-long digit string that `Number()` rounds past `MAX_SAFE_INTEGER` or + * overflows to `Infinity`, so the parsed value stays exact. Throws + * `KanbanError(INVALID_ARGUMENT)` on any violation. This is the single + * digits-only integer parser the CLI/HTTP boundary parsers build on. + */ +export function parseBoundedInt( + value: string, + opts: { min: number; max?: number; field: string }, +): number { + const trimmed = value.trim() + const parsed = /^\d+$/.test(trimmed) ? Number(trimmed) : NaN + const max = opts.max ?? Number.MAX_SAFE_INTEGER + if (!Number.isSafeInteger(parsed) || parsed < opts.min || parsed > max) { + const range = opts.max === undefined ? `>= ${opts.min}` : `between ${opts.min} and ${opts.max}` + throw new KanbanError(ErrorCode.INVALID_ARGUMENT, `${opts.field} must be an integer ${range}`) + } + return parsed +} + +/** + * Parse an optional positive integer supplied through a transport boundary. + * Returns `undefined` when the value is absent, otherwise delegates to + * {@link parseBoundedInt} (min 1) and re-frames the error as "positive integer" + * so invalid limits don't leak into provider logic (SQL `LIMIT`, `slice`, PG). */ export function parsePositiveInt( value: string | null | undefined, @@ -16,15 +38,12 @@ export function parsePositiveInt( if (value === null || value === undefined) return undefined const trimmed = value.trim() if (trimmed === '') return undefined - // Plain decimal digits only — reject signs, decimals, and exponent forms like - // "1e100" that Number() would otherwise treat as integers and let escape into - // SQL `LIMIT`. Cap at MAX_SAFE_INTEGER so the parsed value stays exact. - const parsed = /^\d+$/.test(trimmed) ? Number(trimmed) : NaN - if (!Number.isInteger(parsed) || parsed <= 0 || parsed > Number.MAX_SAFE_INTEGER) { + try { + return parseBoundedInt(trimmed, { min: 1, field }) + } catch { throw new KanbanError( ErrorCode.INVALID_ARGUMENT, `${field} must be a positive integer (received '${value}')`, ) } - return parsed } From 2bbe55d81720d3cb9097443d00b165bc3e9a15fc Mon Sep 17 00:00:00 2001 From: Andy Pai Date: Tue, 23 Jun 2026 05:50:52 -0400 Subject: [PATCH 2/2] harden(serve): resolve webhook secret via WEBHOOK_SECRET_ENV (close gate/enforcement drift) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OBS-2 made WEBHOOK_SECRET_ENV the single source of truth for the tunnel gate (assertTunnelSecurity), but the runtime signature enforcement in jira-core/linear-core still hardcoded process.env['JIRA_WEBHOOK_SECRET'] / ['LINEAR_WEBHOOK_SECRET']. The map only guaranteed the gate was exhaustive, not that the gate and enforcement read the same env name — so a future provider whose verification env diverged from its map entry could pass the gate while verification read an unset var, accepting unsigned writes over a public tunnel (fail-open). Route both handlers through a new tracker-config webhookSecretFromEnv() that resolves the secret via WEBHOOK_SECRET_ENV, so gate and enforcement share one source of truth and cannot drift. Also collapses the per-handler double process.env read into a single resolution. Rendered messages are unchanged. +4 tests pinning that the helper reads exactly the env name the map declares and never a sibling provider's env. Suite 495 pass / 0 fail, check clean. Claude-Session: https://claude.ai/code/session_01LQjR5poSV2M3nxt8h5hNad --- .../tracker-config-webhook-secret.test.ts | 36 ++++++++++++++++++- src/providers/jira-core.ts | 12 +++++-- src/providers/linear-core.ts | 12 +++++-- src/tracker-config.ts | 17 +++++++++ 4 files changed, 70 insertions(+), 7 deletions(-) diff --git a/src/__tests__/tracker-config-webhook-secret.test.ts b/src/__tests__/tracker-config-webhook-secret.test.ts index 0542c21..0c3f764 100644 --- a/src/__tests__/tracker-config-webhook-secret.test.ts +++ b/src/__tests__/tracker-config-webhook-secret.test.ts @@ -1,5 +1,10 @@ import { describe, expect, test } from 'bun:test' -import { WEBHOOK_SECRET_ENV, trackerProviderFromEnv } from '../tracker-config' +import { + WEBHOOK_SECRET_ENV, + trackerProviderFromEnv, + webhookSecretFromEnv, + type TrackerProvider, +} from '../tracker-config' // OBS-2: the provider -> webhook-secret-env mapping is a single source of truth. // WEBHOOK_SECRET_ENV is typed Record, so adding a new @@ -53,3 +58,32 @@ describe('trackerProviderFromEnv', () => { } }) }) + +describe('webhookSecretFromEnv', () => { + // The runtime signature enforcement in jira-core/linear-core resolves its secret + // through this helper, and the assertTunnelSecurity tunnel gate resolves the same + // env name through WEBHOOK_SECRET_ENV. These tests pin that they read the SAME env + // name, so the gate and enforcement can't drift into a fail-open. + test('reads exactly the env name WEBHOOK_SECRET_ENV declares for each provider', () => { + for (const [provider, envName] of Object.entries(WEBHOOK_SECRET_ENV)) { + if (envName === null) continue + const secret = `secret-for-${envName}` + expect(webhookSecretFromEnv(provider as TrackerProvider, { [envName]: secret })).toBe(secret) + } + }) + + test('returns undefined when the declared secret env is unset (open dev mode)', () => { + expect(webhookSecretFromEnv('jira', {})).toBeUndefined() + expect(webhookSecretFromEnv('linear', {})).toBeUndefined() + }) + + test('local has no webhook secret env, so it never resolves a secret', () => { + expect(webhookSecretFromEnv('local', { JIRA_WEBHOOK_SECRET: 'x' })).toBeUndefined() + }) + + test('does not read a different env than the one the provider declares', () => { + // Setting Linear's secret must not satisfy Jira's lookup, and vice versa. + expect(webhookSecretFromEnv('jira', { LINEAR_WEBHOOK_SECRET: 'l' })).toBeUndefined() + expect(webhookSecretFromEnv('linear', { JIRA_WEBHOOK_SECRET: 'j' })).toBeUndefined() + }) +}) diff --git a/src/providers/jira-core.ts b/src/providers/jira-core.ts index fc4f348..77aa522 100644 --- a/src/providers/jira-core.ts +++ b/src/providers/jira-core.ts @@ -47,6 +47,7 @@ import type { UpdateTaskInput, } from './types' import { DEFAULT_POLLING_SYNC_INTERVAL_MS } from '../sync-config' +import { WEBHOOK_SECRET_ENV, webhookSecretFromEnv } from '../tracker-config' import { warnOnce } from './warn-once' import { applyTaskFilters, forEachWithConcurrency, SyncGate, syncStatusFromMeta } from './sync-core' @@ -872,14 +873,19 @@ export class JiraProviderCore implements KanbanProvider { // Shared webhook dispatch. Postgres wraps this with webhook-event auditing; // SQLite calls it directly via the default handleWebhook above. protected async handleWebhookCore(payload: WebhookRequest): Promise { - if (!process.env['JIRA_WEBHOOK_SECRET']) { + // Resolve the signing secret through WEBHOOK_SECRET_ENV so the runtime + // enforcement here and the assertTunnelSecurity tunnel gate read the same env + // name — they can't drift into a fail-open where the gate requires one env and + // verification reads another (unset) one. + const secret = webhookSecretFromEnv('jira') + if (!secret) { warnOnce( 'jira-webhook-open-dev-mode', - '[jira] JIRA_WEBHOOK_SECRET is not set — accepting webhook without signature verification (open dev mode)', + `[jira] ${WEBHOOK_SECRET_ENV['jira']} is not set — accepting webhook without signature verification (open dev mode)`, ) } const auth = authorizeWebhook({ - secret: process.env['JIRA_WEBHOOK_SECRET'], + secret, rawBody: payload.rawBody, signature: headerLower(payload.headers, 'x-hub-signature'), verify: verifySha256HmacSignatureHeader, diff --git a/src/providers/linear-core.ts b/src/providers/linear-core.ts index 6189191..5630489 100644 --- a/src/providers/linear-core.ts +++ b/src/providers/linear-core.ts @@ -40,6 +40,7 @@ import type { UpdateTaskInput, } from './types' import { DEFAULT_POLLING_SYNC_INTERVAL_MS } from '../sync-config' +import { WEBHOOK_SECRET_ENV, webhookSecretFromEnv } from '../tracker-config' import { warnOnce } from './warn-once' import { applyTaskFilters, @@ -596,14 +597,19 @@ export class LinearProviderCore implements KanbanProvider { // Shared webhook dispatch. Postgres wraps this with webhook-event auditing; // SQLite calls it directly via the default handleWebhook above. protected async handleWebhookCore(payload: WebhookRequest): Promise { - if (!process.env['LINEAR_WEBHOOK_SECRET']) { + // Resolve the signing secret through WEBHOOK_SECRET_ENV so the runtime + // enforcement here and the assertTunnelSecurity tunnel gate read the same env + // name — they can't drift into a fail-open where the gate requires one env and + // verification reads another (unset) one. + const secret = webhookSecretFromEnv('linear') + if (!secret) { warnOnce( 'linear-webhook-open-dev-mode', - '[linear] LINEAR_WEBHOOK_SECRET is not set — accepting webhook without signature verification (open dev mode)', + `[linear] ${WEBHOOK_SECRET_ENV['linear']} is not set — accepting webhook without signature verification (open dev mode)`, ) } const auth = authorizeWebhook({ - secret: process.env['LINEAR_WEBHOOK_SECRET'], + secret, rawBody: payload.rawBody, signature: headerLower(payload.headers, 'linear-signature'), verify: verifyHmacSha256, diff --git a/src/tracker-config.ts b/src/tracker-config.ts index 031dce9..3afc64e 100644 --- a/src/tracker-config.ts +++ b/src/tracker-config.ts @@ -37,6 +37,23 @@ export function trackerProviderFromEnv( : 'local' } +/** + * Resolve a provider's webhook signing secret from `env` via WEBHOOK_SECRET_ENV + * (the single source of truth), returning `undefined` when the provider declares + * no secret env or the env var is unset — the case where the webhook handler + * falls back to open dev mode. The runtime signature enforcement (the provider + * webhook handlers) and the assertTunnelSecurity tunnel gate both resolve the + * secret env name through this same map, so the gate cannot require one env name + * while enforcement reads another (a fail-open if they drifted). + */ +export function webhookSecretFromEnv( + provider: TrackerProvider, + env: Record = process.env, +): string | undefined { + const envName = WEBHOOK_SECRET_ENV[provider] + return envName ? env[envName] : undefined +} + export interface LocalTrackerConfig { provider: 'local' defaultColumns?: string[]