fix(skills): bound upload expiry checks

steipete · steipete · commit 522da25932b3 · 2026-05-30T13:00:52.000-04:00
diff --git a/src/skills/lifecycle/upload-store.test.ts b/src/skills/lifecycle/upload-store.test.ts
@@ -3,6 +3,7 @@ import fs from "node:fs/promises";
 import os from "node:os";
 import path from "node:path";
 import { afterEach, beforeEach, describe, expect, it } from "vitest";
+import { MAX_DATE_TIMESTAMP_MS } from "../../shared/number-coercion.js";
 import {
   createSkillUploadStore,
   MAX_ACTIVE_SKILL_UPLOADS,
@@ -272,6 +273,60 @@ describe("skill upload store", () => {
     );
   });
 
+  it("rejects new uploads when the clock cannot produce a valid expiry", async () => {
+    const rootDir = await makeTempDir();
+    const invalidClockStore = createSkillUploadStore({
+      rootDir,
+      now: () => Number.NaN,
+    });
+    await expectUploadError(
+      invalidClockStore.begin({
+        kind: "skill-archive",
+        slug: "invalid-clock",
+        sizeBytes: 1,
+      }),
+      "invalid upload expiry",
+    );
+
+    const overflowStore = createSkillUploadStore({
+      rootDir,
+      now: () => MAX_DATE_TIMESTAMP_MS,
+    });
+    await expectUploadError(
+      overflowStore.begin({
+        kind: "skill-archive",
+        slug: "overflow-clock",
+        sizeBytes: 1,
+      }),
+      "invalid upload expiry",
+    );
+  });
+
+  it("does not count uploads with invalid stored expiry as active", async () => {
+    const rootDir = await makeTempDir();
+    const store = createSkillUploadStore({ rootDir });
+    const begin = await store.begin({
+      kind: "skill-archive",
+      slug: "invalid-expiry",
+      sizeBytes: 1,
+      idempotencyKey: "invalid-expiry",
+    });
+    const metadataPath = path.join(rootDir, begin.uploadId, "metadata.json");
+    const metadata = JSON.parse(await fs.readFile(metadataPath, "utf8")) as Record<string, unknown>;
+    metadata.expiresAt = null;
+    await fs.writeFile(metadataPath, `${JSON.stringify(metadata, null, 2)}\n`, "utf8");
+
+    const repeated = await store.begin({
+      kind: "skill-archive",
+      slug: "invalid-expiry",
+      sizeBytes: 1,
+      idempotencyKey: "invalid-expiry",
+    });
+
+    expect(repeated.uploadId).not.toBe(begin.uploadId);
+    await expectMissingPath(path.join(rootDir, begin.uploadId));
+  });
+
   it("expires unfinished and committed uploads", async () => {
     let now = 1000;
     const rootDir = await makeTempDir();
diff --git a/src/skills/lifecycle/upload-store.ts b/src/skills/lifecycle/upload-store.ts
@@ -6,6 +6,11 @@ import { resolveStateDir } from "../../config/paths.js";
 import { DEFAULT_MAX_ARCHIVE_BYTES_ZIP } from "../../infra/archive.js";
 import { formatErrorMessage } from "../../infra/errors.js";
 import { createAsyncLock, readDurableJsonFile, writeJsonAtomic } from "../../infra/json-files.js";
+import {
+  asDateTimestampMs,
+  isFutureDateTimestampMs,
+  resolveExpiresAtMsFromDurationMs,
+} from "../../shared/number-coercion.js";
 import { validateRequestedSkillSlug } from "./archive-install.js";
 
 export const SKILL_UPLOAD_TTL_MS = 60 * 60 * 1000;
@@ -209,10 +214,14 @@ async function assertNotExpired(
   record: SkillUploadRecord,
   now: number,
 ): Promise<void> {
-  if (record.expiresAt <= now) {
+  const validNow = asDateTimestampMs(now);
+  if (validNow !== undefined && !isFutureDateTimestampMs(record.expiresAt, { nowMs: validNow })) {
     await removeRecordFiles(rootDir, record);
     throw new SkillUploadRequestError("upload has expired");
   }
+  if (validNow === undefined) {
+    throw new SkillUploadRequestError("upload has expired");
+  }
 }
 
 async function computeFileSha256(filePath: string): Promise<string> {
@@ -286,7 +295,12 @@ async function cleanupExpiredUploads(
     }
     await withLock(`${rootDir}:upload:${uploadId}`, async () => {
       const record = await readRecordIfPresent(rootDir, uploadId).catch(() => null);
-      if (record && record.expiresAt <= nowMs) {
+      const validNow = asDateTimestampMs(nowMs);
+      if (
+        record &&
+        validNow !== undefined &&
+        !isFutureDateTimestampMs(record.expiresAt, { nowMs: validNow })
+      ) {
         await removeRecordFiles(rootDir, record);
       }
     });
@@ -297,7 +311,7 @@ async function countActiveUploads(rootDir: string, nowMs: number): Promise<numbe
   let count = 0;
   for (const uploadId of await listUploadIds(rootDir)) {
     const record = await readRecordIfPresent(rootDir, uploadId).catch(() => null);
-    if (record && record.expiresAt > nowMs) {
+    if (record && isFutureDateTimestampMs(record.expiresAt, { nowMs })) {
       count += 1;
     }
   }
@@ -395,7 +409,7 @@ export function createSkillUploadStore(options?: {
               `${rootDir}:upload:${existingUploadId}`,
               async () => {
                 const record = await readRecordIfPresent(rootDir, existingUploadId);
-                if (record && record.expiresAt > now()) {
+                if (record && isFutureDateTimestampMs(record.expiresAt, { nowMs: now() })) {
                   return {
                     uploadId: record.uploadId,
                     receivedBytes: record.receivedBytes,
@@ -424,6 +438,10 @@ export function createSkillUploadStore(options?: {
         const uploadDir = resolveUploadDir(rootDir, uploadId);
         const archivePath = resolveArchivePath(rootDir, uploadId);
         const createdAt = now();
+        const expiresAt = resolveExpiresAtMsFromDurationMs(ttlMs, { nowMs: createdAt });
+        if (expiresAt === undefined) {
+          throw new SkillUploadRequestError("invalid upload expiry");
+        }
         const record: SkillUploadRecord = {
           version: 1,
           kind: params.kind,
@@ -435,7 +453,7 @@ export function createSkillUploadStore(options?: {
           receivedBytes: 0,
           archivePath,
           createdAt,
-          expiresAt: createdAt + ttlMs,
+          expiresAt,
           committed: false,
           ...(keyHash ? { idempotencyKeyHash: keyHash } : {}),
         };
