fix: prevent race conditions with KV locks in dispatch and group provisioning

This commit is contained in:
RhenCloud 2026-08-15 12:04:37 +08:00
parent 17eefd3c8e
commit 7ebd9aa63f
No known key found for this signature in database
GPG key ID: A574A617378C4E0B
4 changed files with 194 additions and 79 deletions

View file

@ -197,60 +197,131 @@ export async function dispatchEvent(
if (message.updateKey) { if (message.updateKey) {
const groupPrefix = route.groupId ? `${route.groupId}:` : ""; const groupPrefix = route.groupId ? `${route.groupId}:` : "";
const kvKey = `msg:${groupPrefix}${route.id}:${message.updateKey}:${targetStr}`; const kvKey = `msg:${groupPrefix}${route.id}:${message.updateKey}:${targetStr}`;
const existingId = await env.KV.get(kvKey); const lockKey = `msg:lock:${kvKey}`;
if (existingId) {
result = await driver.edit(message, target, env, existingId); // Acquire a short-lived lock so concurrent events for the same
if (result.ok) { // updateKey don't race (both read null, both send, the later put
attempts.push({ // overwrites). KV is eventually consistent so the lock is
groupId: route.groupId, // best-effort; a retry loop further shrinks the window.
routeId: route.id, let locked = false;
routeName: route.name, for (let attempt = 0; attempt < 3; attempt++) {
target: targetStr, const holder = await env.KV.get(lockKey);
ok: true, if (holder) {
}); const existing = await env.KV.get(kvKey);
await recordSend(env.DB, { if (existing) {
...base, result = await driver.edit(message, target, env, existing);
ok: true, if (result.ok || /not modified/i.test(result.error ?? "")) {
status: result.status, const ok = result.ok || /not modified/i.test(result.error ?? "");
messageId: existingId, attempts.push({
platform: driver.id, groupId: route.groupId,
attempts: result.attempts, routeId: route.id,
durationMs: Date.now() - started, routeName: route.name,
errorCode: result.errorCode, target: targetStr,
}); ok: true,
});
await recordSend(env.DB, {
...base,
ok: true,
status: result.status,
messageId: existing,
platform: driver.id,
attempts: result.attempts,
durationMs: Date.now() - started,
errorCode: result.errorCode,
});
if (!ok) await env.KV.delete(kvKey);
continue;
}
await env.KV.delete(kvKey);
break;
}
await new Promise((r) => setTimeout(r, 50 * (attempt + 1)));
continue; continue;
} }
if (/not modified/i.test(result.error ?? "")) { await env.KV.put(lockKey, "1", { expirationTtl: 60 });
attempts.push({ locked = true;
groupId: route.groupId, break;
routeId: route.id,
routeName: route.name,
target: targetStr,
ok: true,
});
await recordSend(env.DB, {
...base,
ok: true,
status: result.status,
messageId: existingId,
platform: driver.id,
attempts: result.attempts,
durationMs: Date.now() - started,
errorCode: result.errorCode,
});
continue;
}
await env.KV.delete(kvKey);
} }
result = await driver.send(message, target, env);
if (result.ok && result.messageId) { try {
await env.KV.put(kvKey, result.messageId, { expirationTtl: 604800 }); const existingId = await env.KV.get(kvKey);
if (existingId) {
result = await driver.edit(message, target, env, existingId);
if (result.ok) {
attempts.push({
groupId: route.groupId,
routeId: route.id,
routeName: route.name,
target: targetStr,
ok: true,
});
await recordSend(env.DB, {
...base,
ok: true,
status: result.status,
messageId: existingId,
platform: driver.id,
attempts: result.attempts,
durationMs: Date.now() - started,
errorCode: result.errorCode,
});
continue;
}
if (/not modified/i.test(result.error ?? "")) {
attempts.push({
groupId: route.groupId,
routeId: route.id,
routeName: route.name,
target: targetStr,
ok: true,
});
await recordSend(env.DB, {
...base,
ok: true,
status: result.status,
messageId: existingId,
platform: driver.id,
attempts: result.attempts,
durationMs: Date.now() - started,
errorCode: result.errorCode,
});
continue;
}
await env.KV.delete(kvKey);
}
result = await driver.send(message, target, env);
if (result.ok && result.messageId) {
await env.KV.put(kvKey, result.messageId, { expirationTtl: 604800 });
}
} finally {
if (locked) await env.KV.delete(lockKey);
} }
} else { } else {
result = await driver.send(message, target, env); result = await driver.send(message, target, env);
} }
const durationMs = Date.now() - started; const durationMs = Date.now() - started;
if (!result.ok) throw new Error(result.error ?? "Send failed"); if (!result.ok) {
const error = result.error ?? "Send failed";
attempts.push({
groupId: route.groupId,
routeId: route.id,
routeName: route.name,
target: targetStr,
ok: false,
error,
});
await recordSend(env.DB, {
...base,
ok: false,
error,
durationMs,
status: result.status,
platform: driver.id,
attempts: result.attempts,
errorCode: result.errorCode,
});
continue;
}
attempts.push({ attempts.push({
groupId: route.groupId, groupId: route.groupId,
routeId: route.id, routeId: route.id,

View file

@ -33,6 +33,19 @@ const GITHUB_COMMENT_RE =
/github\.com\/([^/\s]+)\/([^/\s]+)\/(?:issues|pull)\/\d+#issuecomment-(\d+)/; /github\.com\/([^/\s]+)\/([^/\s]+)\/(?:issues|pull)\/\d+#issuecomment-(\d+)/;
const GITHUB_ISSUE_RE = /github\.com\/([^/\s]+)\/([^/\s]+)\/(?:issues|pull)\/(\d+)/; const GITHUB_ISSUE_RE = /github\.com\/([^/\s]+)\/([^/\s]+)\/(?:issues|pull)\/(\d+)/;
/**
* Split a pipe-delimited custom-id suffix into [owner, repo, number]. The
* number is always the last segment (arbitrary-length owner/repo can contain
* pipes on non-GitHub forges), so we split from the right.
*/
function splitThree(rest: string): [string, string, string] {
const parts = rest.split("|");
const number = parts.pop();
const repo = parts.pop();
const owner = parts.join("|");
return [owner ?? "", repo ?? "", number ?? ""];
}
interface Interaction { interface Interaction {
id: string; id: string;
token: string; token: string;
@ -101,7 +114,8 @@ export async function handleInteractionRequest(request: Request, env: Env): Prom
); );
return new Response("Invalid signature", { status: 401 }); return new Response("Invalid signature", { status: 401 });
} }
if (Math.abs(Math.floor(Date.now() / 1000) - Number(timestamp)) > TIMESTAMP_TOLERANCE_SECONDS) { const ts = Number(timestamp);
if (!Number.isFinite(ts) || Math.abs(Math.floor(Date.now() / 1000) - ts) > TIMESTAMP_TOLERANCE_SECONDS) {
return new Response("Invalid signature", { status: 401 }); return new Response("Invalid signature", { status: 401 });
} }
@ -287,7 +301,7 @@ async function handleButton(
return; return;
} }
const [owner, repo, number] = rest.split("|"); const [owner, repo, number] = splitThree(rest);
if (!owner || !repo || !number) return; if (!owner || !repo || !number) return;
// Acknowledge first (deferred, ephemeral) so the clicker sees a spinner // Acknowledge first (deferred, ephemeral) so the clicker sees a spinner
@ -494,7 +508,7 @@ async function modalSubmit(
// ghc|add|owner|repo|issueNumber // ghc|add|owner|repo|issueNumber
if (customId.startsWith(MODAL_ADD)) { if (customId.startsWith(MODAL_ADD)) {
const [owner, repo, number] = customId.slice(MODAL_ADD.length).split("|"); const [owner, repo, number] = splitThree(customId.slice(MODAL_ADD.length));
if (!owner || !repo || !number) if (!owner || !repo || !number)
return respond(env, id, token, "内部错误:无法解析目标 issue。"); return respond(env, id, token, "内部错误:无法解析目标 issue。");
try { try {
@ -514,7 +528,7 @@ async function modalSubmit(
// ghc|edit|owner|repo|commentId // ghc|edit|owner|repo|commentId
if (customId.startsWith(MODAL_EDIT)) { if (customId.startsWith(MODAL_EDIT)) {
const [owner, repo, commentId] = customId.slice(MODAL_EDIT.length).split("|"); const [owner, repo, commentId] = splitThree(customId.slice(MODAL_EDIT.length));
if (!owner || !repo || !commentId) if (!owner || !repo || !commentId)
return respond(env, id, token, "内部错误:无法解析目标评论。"); return respond(env, id, token, "内部错误:无法解析目标评论。");
try { try {

View file

@ -120,36 +120,57 @@ export async function ensureInstallationGroup(
installationId: number, installationId: number,
accountLogin: string, accountLogin: string,
): Promise<Group | null> { ): Promise<Group | null> {
const groups = await loadGroups(kv); // A per-installation lock serializes concurrent `installation.created`
const existing = groups.find((g) => g.installationId === installationId); // webhooks: without it two requests both pass the "no group bound" check,
if (existing) return existing; // both create `inst-{id}`, and the second save overwrites the first.
const lockKey = `inst:lock:${installationId}`;
const login = accountLogin.trim().toLowerCase(); for (let attempt = 0; attempt < 5; attempt++) {
const candidates = groups.filter( const holder = await kv.get(lockKey);
(g) => if (!holder) break;
g.installationId == null && const groups = await loadGroups(kv);
login.length > 0 && const existing = groups.find((g) => g.installationId === installationId);
(g.owners ?? []).some((o) => o.trim().toLowerCase() === login), if (existing) return existing;
); await new Promise((r) => setTimeout(r, 40 * (attempt + 1)));
if (candidates.length > 0) {
const next = groups.map((g) => (candidates.includes(g) ? { ...g, installationId } : g));
await saveGroups(kv, next);
return next.find((g) => g.id === candidates[0]!.id) ?? null;
} }
const gid = `inst-${installationId}`; let locked = false;
const dedicated = groups.find((g) => g.id === gid); try {
const group: Group = { await kv.put(lockKey, "1", { expirationTtl: 60 });
id: gid, locked = true;
name: accountLogin.trim() || `Installation ${installationId}`,
adminIds: [], const groups = await loadGroups(kv);
installationId, const existing = groups.find((g) => g.installationId === installationId);
}; if (existing) return existing;
await saveGroups(
kv, const login = accountLogin.trim().toLowerCase();
dedicated ? groups.map((g) => (g.id === gid ? { ...g, ...group } : g)) : [...groups, group], const candidates = groups.filter(
); (g) =>
return group; g.installationId == null &&
login.length > 0 &&
(g.owners ?? []).some((o) => o.trim().toLowerCase() === login),
);
if (candidates.length > 0) {
const next = groups.map((g) => (candidates.includes(g) ? { ...g, installationId } : g));
await saveGroups(kv, next);
return next.find((g) => g.id === candidates[0]!.id) ?? null;
}
const gid = `inst-${installationId}`;
const dedicated = groups.find((g) => g.id === gid);
const group: Group = {
id: gid,
name: accountLogin.trim() || `Installation ${installationId}`,
adminIds: [],
installationId,
};
await saveGroups(
kv,
dedicated ? groups.map((g) => (g.id === gid ? { ...g, ...group } : g)) : [...groups, group],
);
return group;
} finally {
if (locked) await kv.delete(lockKey);
}
} }
export interface AccessScope { export interface AccessScope {

View file

@ -50,7 +50,16 @@ export async function processWebhook(
return { status: 400, body: { error: "Unknown webhook provider" } }; return { status: 400, body: { error: "Unknown webhook provider" } };
} }
if (!(await provider.verify(body, headers, effectiveEnv))) { let verified = false;
try {
verified = await provider.verify(body, headers, effectiveEnv);
} catch (err) {
// A malformed secret or an unavailable crypto implementation must fail as
// a clean 401, never an uncaught 500.
log.warn({ provider: provider.id, err: String(err) }, "Webhook signature verification failed");
return { status: 401, body: { error: "Invalid signature" } };
}
if (!verified) {
// Log the actual cause: a missing provider secret is a deployment problem, // Log the actual cause: a missing provider secret is a deployment problem,
// while a mismatched signature usually means the sender used the wrong secret. // while a mismatched signature usually means the sender used the wrong secret.
const secret = const secret =