fix(routines): reject HMAC webhook replays (#9994)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Routines allow external systems to start recurring work through authenticated public webhooks > - Timestamped HMAC authentication currently verifies authenticity and age but does not remember an accepted delivery > - An exact signed request can therefore be reused within its replay window, including through simultaneous duplicate delivery > - Replay rejection must be atomic with run creation so concurrent copies cannot both succeed > - This pull request derives a non-secret replay identity from each valid timestamped HMAC delivery and claims it under the existing routine transaction lock > - The benefit is at-most-once acceptance of an exact HMAC delivery without changing ordinary caller-supplied idempotency semantics ## Linked Issues or Issue Description Fixes: #9993 ## What Changed - Derive a stable, non-secret idempotency key after a timestamped HMAC signature has been validated. - Reject a previously claimed HMAC delivery with a conflict while preserving coalescing for existing non-HMAC idempotency keys. - Apply the same atomic replay claim when automatic worktree execution is suppressed. - Add regression coverage for sequential, concurrent, and suppressed-run replays. ## Verification - `pnpm exec vitest run server/src/__tests__/routines-service.test.ts` — 59 tests passed. - `pnpm typecheck` — all workspace packages passed. - The sequential test was observed failing on unmodified `master`: the second identical request resolved and a second run was created. - The concurrent regression test verifies exactly one request succeeds and only one routine run exists. ## Risks - Low migration risk: no schema change is required; the existing nullable routine-run idempotency field is reused. - The routine row lock serializes replay claims, adding a small amount of contention only while a routine run is being created. - Replay rejection applies only to `hmac_sha256`, which carries the timestamp needed for a bounded replay policy. Existing `github_hmac`, bearer, and unauthenticated trigger semantics are unchanged. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used OpenAI Codex (GPT-5 family) with reasoning, repository inspection, shell execution, and test tooling. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] Documentation does not require an update because this restores the documented replay-window security behavior without changing configuration or APIs - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge
This commit is contained in:
Jonathan Reyes
GitHub
parent
fc5c6ffed2
commit
676e20a894
|
|
@ -2012,6 +2012,124 @@ describeEmbeddedPostgres("routine service live-execution coalescing", () => {
|
|||
expect(run.linkedIssueId).toBeTruthy();
|
||||
});
|
||||
|
||||
it("rejects an HMAC webhook replay inside the accepted timestamp window", async () => {
|
||||
const { routine, svc } = await seedFixture();
|
||||
const { trigger, secretMaterial } = await svc.createTrigger(
|
||||
routine.id,
|
||||
{
|
||||
kind: "webhook",
|
||||
signingMode: "hmac_sha256",
|
||||
replayWindowSec: 300,
|
||||
},
|
||||
{},
|
||||
);
|
||||
|
||||
const payload = { event: "acceptance" };
|
||||
const rawBody = Buffer.from(JSON.stringify(payload));
|
||||
const timestampSeconds = String(Math.floor(Date.now() / 1000));
|
||||
const signature = `sha256=${createHmac("sha256", secretMaterial!.webhookSecret)
|
||||
.update(`${timestampSeconds}.`)
|
||||
.update(rawBody)
|
||||
.digest("hex")}`;
|
||||
const request = {
|
||||
signatureHeader: signature,
|
||||
timestampHeader: timestampSeconds,
|
||||
rawBody,
|
||||
payload,
|
||||
};
|
||||
|
||||
await expect(svc.firePublicTrigger(trigger.publicId!, request)).resolves.toMatchObject({
|
||||
source: "webhook",
|
||||
status: "issue_created",
|
||||
});
|
||||
await expect(svc.firePublicTrigger(trigger.publicId!, request)).rejects.toThrow(
|
||||
"Webhook replay detected",
|
||||
);
|
||||
|
||||
const runs = await db
|
||||
.select({ id: routineRuns.id })
|
||||
.from(routineRuns)
|
||||
.where(eq(routineRuns.triggerId, trigger.id));
|
||||
expect(runs).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("serializes concurrent HMAC webhook replays", async () => {
|
||||
const { routine, svc } = await seedFixture();
|
||||
const { trigger, secretMaterial } = await svc.createTrigger(
|
||||
routine.id,
|
||||
{
|
||||
kind: "webhook",
|
||||
signingMode: "hmac_sha256",
|
||||
replayWindowSec: 300,
|
||||
},
|
||||
{},
|
||||
);
|
||||
|
||||
const payload = { event: "concurrent" };
|
||||
const rawBody = Buffer.from(JSON.stringify(payload));
|
||||
const timestampSeconds = String(Math.floor(Date.now() / 1000));
|
||||
const signature = `sha256=${createHmac("sha256", secretMaterial!.webhookSecret)
|
||||
.update(`${timestampSeconds}.`)
|
||||
.update(rawBody)
|
||||
.digest("hex")}`;
|
||||
const request = {
|
||||
signatureHeader: signature,
|
||||
timestampHeader: timestampSeconds,
|
||||
rawBody,
|
||||
payload,
|
||||
};
|
||||
|
||||
const results = await Promise.allSettled([
|
||||
svc.firePublicTrigger(trigger.publicId!, request),
|
||||
svc.firePublicTrigger(trigger.publicId!, request),
|
||||
]);
|
||||
|
||||
expect(results.filter((result) => result.status === "fulfilled")).toHaveLength(1);
|
||||
const rejected = results.find((result) => result.status === "rejected");
|
||||
expect(rejected).toMatchObject({ status: "rejected" });
|
||||
expect((rejected as PromiseRejectedResult).reason).toMatchObject({
|
||||
message: "Webhook replay detected",
|
||||
});
|
||||
expect(await db.select().from(routineRuns).where(eq(routineRuns.triggerId, trigger.id))).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("rejects an HMAC webhook replay when automatic execution is suppressed", async () => {
|
||||
const runtimeEnv = { PAPERCLIP_IN_WORKTREE: "yes", PAPERCLIP_INSTANCE_ID: "worktree-routines-test" };
|
||||
const { routine, svc } = await seedFixture({ runtimeEnv });
|
||||
const { trigger, secretMaterial } = await svc.createTrigger(
|
||||
routine.id,
|
||||
{
|
||||
kind: "webhook",
|
||||
signingMode: "hmac_sha256",
|
||||
replayWindowSec: 300,
|
||||
},
|
||||
{},
|
||||
);
|
||||
|
||||
const payload = { event: "suppressed" };
|
||||
const rawBody = Buffer.from(JSON.stringify(payload));
|
||||
const timestampSeconds = String(Math.floor(Date.now() / 1000));
|
||||
const signature = `sha256=${createHmac("sha256", secretMaterial!.webhookSecret)
|
||||
.update(`${timestampSeconds}.`)
|
||||
.update(rawBody)
|
||||
.digest("hex")}`;
|
||||
const request = {
|
||||
signatureHeader: signature,
|
||||
timestampHeader: timestampSeconds,
|
||||
rawBody,
|
||||
payload,
|
||||
};
|
||||
|
||||
await expect(svc.firePublicTrigger(trigger.publicId!, request)).resolves.toMatchObject({
|
||||
status: "skipped",
|
||||
failureReason: "worktree_execution_cutoff",
|
||||
});
|
||||
await expect(svc.firePublicTrigger(trigger.publicId!, request)).rejects.toThrow(
|
||||
"Webhook replay detected",
|
||||
);
|
||||
expect(await db.select().from(routineRuns).where(eq(routineRuns.triggerId, trigger.id))).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("uses the configured provider for generated webhook trigger secrets", async () => {
|
||||
process.env.PAPERCLIP_SECRETS_PROVIDER = "aws_secrets_manager";
|
||||
const originalGetSecretProvider = providerRegistry.getSecretProvider;
|
||||
|
|
|
|||
|
|
@ -1359,10 +1359,40 @@ export function routineService(
|
|||
reason: string;
|
||||
nextRunAt?: Date | null;
|
||||
details?: Record<string, unknown> | null;
|
||||
idempotencyKey?: string | null;
|
||||
rejectIdempotencyReplay?: boolean;
|
||||
}) {
|
||||
const triggeredAt = new Date();
|
||||
const run = await db.transaction(async (tx) => {
|
||||
const txDb = tx as unknown as Db;
|
||||
await tx.execute(
|
||||
sql`select id from ${routines} where ${routines.id} = ${input.routine.id} and ${routines.companyId} = ${input.routine.companyId} for update`,
|
||||
);
|
||||
|
||||
if (input.idempotencyKey) {
|
||||
const existing = await txDb
|
||||
.select()
|
||||
.from(routineRuns)
|
||||
.where(
|
||||
and(
|
||||
eq(routineRuns.companyId, input.routine.companyId),
|
||||
eq(routineRuns.routineId, input.routine.id),
|
||||
eq(routineRuns.source, input.source),
|
||||
eq(routineRuns.idempotencyKey, input.idempotencyKey),
|
||||
eq(routineRuns.triggerId, input.trigger.id),
|
||||
),
|
||||
)
|
||||
.orderBy(desc(routineRuns.createdAt))
|
||||
.limit(1)
|
||||
.then((rows) => rows[0] ?? null);
|
||||
if (existing) {
|
||||
if (input.rejectIdempotencyReplay) {
|
||||
throw conflict("Webhook replay detected");
|
||||
}
|
||||
return existing;
|
||||
}
|
||||
}
|
||||
|
||||
const [createdRun] = await txDb
|
||||
.insert(routineRuns)
|
||||
.values({
|
||||
|
|
@ -1378,6 +1408,7 @@ export function routineService(
|
|||
routineRevisionId: input.routine.latestRevisionId,
|
||||
responsibleUserId: input.routine.responsibleUserId ?? null,
|
||||
triggerPayload: input.details ?? null,
|
||||
idempotencyKey: input.idempotencyKey ?? null,
|
||||
})
|
||||
.returning();
|
||||
await updateRoutineTouchedState({
|
||||
|
|
@ -1636,6 +1667,7 @@ export function routineService(
|
|||
projectWorkspaceId?: string | null;
|
||||
assigneeAgentId?: string | null;
|
||||
idempotencyKey?: string | null;
|
||||
rejectIdempotencyReplay?: boolean;
|
||||
executionWorkspaceId?: string | null;
|
||||
executionWorkspacePreference?: string | null;
|
||||
executionWorkspaceSettings?: Record<string, unknown> | null;
|
||||
|
|
@ -1723,7 +1755,12 @@ export function routineService(
|
|||
.orderBy(desc(routineRuns.createdAt))
|
||||
.limit(1)
|
||||
.then((rows) => rows[0] ?? null);
|
||||
if (existing) return existing;
|
||||
if (existing) {
|
||||
if (input.rejectIdempotencyReplay) {
|
||||
throw conflict("Webhook replay detected");
|
||||
}
|
||||
return existing;
|
||||
}
|
||||
}
|
||||
|
||||
const triggeredAt = new Date();
|
||||
|
|
@ -2814,6 +2851,7 @@ export function routineService(
|
|||
if (!routine) throw notFound("Routine not found");
|
||||
if (!trigger.enabled || routine.status !== "active") throw conflict("Routine trigger is not active");
|
||||
|
||||
let hmacReplayKey: string | null = null;
|
||||
if (trigger.signingMode === "none") {
|
||||
// No authentication — the publicId in the URL acts as a shared secret.
|
||||
} else if (trigger.signingMode === "github_hmac") {
|
||||
|
|
@ -2870,6 +2908,10 @@ export function routineService(
|
|||
normalizedSignature.length === expectedHmac.length &&
|
||||
crypto.timingSafeEqual(Buffer.from(normalizedSignature), Buffer.from(expectedHmac));
|
||||
if (!valid) throw unauthorized();
|
||||
hmacReplayKey = `webhook-hmac:${crypto
|
||||
.createHash("sha256")
|
||||
.update(`${trigger.id}:${providedTimestamp}:${expectedHmac}`)
|
||||
.digest("hex")}`;
|
||||
}
|
||||
|
||||
const eligibility = await getAutomaticRoutineDispatchEligibility(routine);
|
||||
|
|
@ -2879,6 +2921,8 @@ export function routineService(
|
|||
trigger,
|
||||
source: "webhook",
|
||||
reason: "worktree_execution_cutoff",
|
||||
idempotencyKey: hmacReplayKey ?? input.idempotencyKey,
|
||||
rejectIdempotencyReplay: hmacReplayKey !== null,
|
||||
});
|
||||
}
|
||||
|
||||
|
|
@ -2890,7 +2934,8 @@ export function routineService(
|
|||
variables: isPlainRecord(input.payload) && isPlainRecord(input.payload.variables)
|
||||
? input.payload.variables
|
||||
: null,
|
||||
idempotencyKey: input.idempotencyKey,
|
||||
idempotencyKey: hmacReplayKey ?? input.idempotencyKey,
|
||||
rejectIdempotencyReplay: hmacReplayKey !== null,
|
||||
});
|
||||
},
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue