fix v2 hardening review findings

apps/workshop/tests/entrypoint-url.mjs

@@ -10,6 +10,11 @@ const validate = (value) =>
     env: { ...process.env, WORKSHOP_URL_VALIDATOR_PATH: validator },
   });
 
+const validateDocsBaseUrl = (value) =>
+  spawnSync("sh", [entrypoint, "--validate-docs-base-url", value], {
+    encoding: "utf8",
+  });
+
 for (const [value, expected] of [
   ["http://127.0.0.1:8090/", "http://127.0.0.1:8090"],
   ["https://example.com/docs/", "https://example.com/docs"],
@@ -30,6 +35,8 @@ for (const value of [
   "https://example.com:65536",
   "https://::1",
   "https://[:::1]",
+  "http://[:1:2:3:4:5:6:7:8]",
+  "http://[1:2:3:4:5:6:7:8:]",
   "https://1.2.3.4.5",
   "http://09.0.0.1",
   "http://099.0.0.1",
@@ -46,4 +53,20 @@ for (const value of [
   if (result.status === 0) throw new Error(`Expected invalid runtime URL to be rejected: ${JSON.stringify(value)}`);
 }
 
+for (const [value, expected] of [
+  ["/", "/"],
+  ["/docs/", "/docs"],
+  ["/docs..preview", "/docs..preview"],
+]) {
+  const result = validateDocsBaseUrl(value);
+  if (result.status !== 0 || result.stdout.trim() !== expected) {
+    throw new Error(`Expected valid DOCS_BASE_URL ${value}: ${result.stderr || result.stdout}`);
+  }
+}
+
+for (const value of ["docs", "/../docs", "/docs/../private", "/./docs", "/docs?preview=1"]) {
+  const result = validateDocsBaseUrl(value);
+  if (result.status === 0) throw new Error(`Expected invalid DOCS_BASE_URL to be rejected: ${JSON.stringify(value)}`);
+}
+
 console.log("Workshop runtime URL validation passed");

apps/workshop/tests/workshop-contracts.mjs

@@ -106,12 +106,29 @@ for (const operation of ["create", "update", "delete"]) {
   }
 }
 if (!hardeningMigration.includes("users:update")) throw new Error("Workshop user file updates must be rate limited");
+for (const required of [
+  "writeRateLimits",
+  "managedRules(settings.rateLimits.rules)",
+  "settings.rateLimits.enabled = state.enabled",
+  "...cloneJson(state.managedRules)",
+]) {
+  if (!hardeningMigration.includes(required)) {
+    throw new Error(`Workshop write rate-limit rollback is missing persistent state restoration: ${required}`);
+  }
+}
 
 const accessHardeningMigration = await read("apps/workshop/pb_migrations/1784240002_workshop_access_hardening.js");
+const rateLimitCompatibilityMigration = await read(
+  "apps/workshop/pb_migrations/1784240003_workshop_rate_limit_rollback_compatibility.js",
+);
+for (const required of ["writeRateLimits", "legacyCreateRules", "rate-limit compatibility state is unavailable"]) {
+  if (!rateLimitCompatibilityMigration.includes(required)) {
+    throw new Error(`Workshop rate-limit compatibility migration is missing: ${required}`);
+  }
+}
 for (const protectedField of [
   "email:changed",
   "displayName:changed",
-  "avatar:changed",
   "avatarUrl:changed",
   "githubUsername:changed",
   "githubProfileUrl:changed",
@@ -120,6 +137,16 @@ for (const protectedField of [
     throw new Error(`Workshop OAuth identity field remains caller-controlled: ${protectedField}`);
   }
 }
+if (accessHardeningMigration.includes("@request.body.avatar:changed")) {
+  throw new Error("Workshop access rules must not use the unsupported :changed modifier for file fields");
+}
+if (
+  !hook.includes('getUnsavedFiles("avatar")') ||
+  !hook.includes('original().getString("avatar")') ||
+  !hook.includes("Workshop avatars are managed by GitHub OAuth")
+) {
+  throw new Error("Workshop avatar updates must be rejected by the user update request hook");
+}
 for (const protectedField of [
   "title:changed",
   "description:changed",

apps/workshop/tests/workshop-oauth-hook.mjs

@@ -6,6 +6,7 @@ const nodeRequire = createRequire(import.meta.url);
 const workshopUtils = nodeRequire("../pb_hooks/workshop-utils.js");
 const hookSource = await readFile("apps/workshop/pb_hooks/workshop.pb.js", "utf8");
 let oauthHook;
+let userUpdateHook;
 
 const ignoreHook = () => {};
 vm.runInNewContext(hookSource, {
@@ -15,7 +16,9 @@ vm.runInNewContext(hookSource, {
   onRecordAfterCreateSuccess: ignoreHook,
   onRecordCreateRequest: ignoreHook,
   onRecordDeleteRequest: ignoreHook,
-  onRecordUpdateRequest: ignoreHook,
+  onRecordUpdateRequest: (handler, collection) => {
+    if (collection === "users") userUpdateHook = handler;
+  },
   onRecordAuthWithOAuth2Request: (handler, collection) => {
     if (collection !== "users") throw new Error(`OAuth hook targets unexpected collection: ${collection}`);
     oauthHook = handler;
@@ -24,11 +27,17 @@ vm.runInNewContext(hookSource, {
     if (specifier !== "/pb_hooks/workshop-utils.js") {
       throw new Error(`OAuth hook loaded unexpected dependency: ${specifier}`);
     }
-    return workshopUtils;
+    return {
+      ...workshopUtils,
+      rejectRequest: (message) => {
+        throw new Error(message);
+      },
+    };
   },
 });
 
 if (typeof oauthHook !== "function") throw new Error("Workshop OAuth request hook was not registered");
+if (typeof userUpdateHook !== "function") throw new Error("Workshop user update hook was not registered");
 
 const oauth2User = {
   id: "provider-user-123",
@@ -108,4 +117,52 @@ if (ignoredNextCalls !== 1 || Object.keys(ignoredCreateData).length !== 0) {
   throw new Error("Non-GitHub OAuth providers must pass through without GitHub identity synchronization");
 }
 
+let unchangedAvatarNextCalls = 0;
+userUpdateHook({
+  next: () => {
+    unchangedAvatarNextCalls += 1;
+  },
+  record: {
+    getString: () => "provider-avatar.png",
+    getUnsavedFiles: () => [],
+    original: () => ({ getString: () => "provider-avatar.png" }),
+  },
+});
+if (unchangedAvatarNextCalls !== 1) {
+  throw new Error("User updates that leave the provider avatar unchanged must pass through");
+}
+
+for (const avatarChange of [
+  {
+    name: "upload",
+    current: "provider-avatar.png",
+    original: "provider-avatar.png",
+    unsavedFiles: [{ name: "forged-avatar.png" }],
+  },
+  {
+    name: "removal",
+    current: "",
+    original: "provider-avatar.png",
+    unsavedFiles: [],
+  },
+]) {
+  let nextCalls = 0;
+  try {
+    userUpdateHook({
+      next: () => {
+        nextCalls += 1;
+      },
+      record: {
+        getString: () => avatarChange.current,
+        getUnsavedFiles: () => avatarChange.unsavedFiles,
+        original: () => ({ getString: () => avatarChange.original }),
+      },
+    });
+    throw new Error(`Workshop user avatar ${avatarChange.name} was accepted`);
+  } catch (error) {
+    if (!String(error).includes("managed by GitHub OAuth")) throw error;
+  }
+  if (nextCalls !== 0) throw new Error(`Rejected avatar ${avatarChange.name} continued the update request`);
+}
+
 console.log("Workshop OAuth hook contract passed");

apps/workshop/tests/workshop-rate-limit-migration.mjs

@@ -0,0 +1,205 @@
+import assert from "node:assert/strict";
+import { readFile } from "node:fs/promises";
+import vm from "node:vm";
+
+const normalized = (value) => JSON.parse(JSON.stringify(value));
+const managedLabels = [
+  "submissions:create",
+  "submissions:update",
+  "submissions:delete",
+  "votes:create",
+  "votes:update",
+  "votes:delete",
+  "comments:create",
+  "comments:update",
+  "comments:delete",
+  "reports:create",
+  "reports:update",
+  "reports:delete",
+  "users:update",
+];
+const originalManagedRules = managedLabels.map((label, index) => ({
+  label,
+  audience: index % 2 === 0 ? "@auth" : "",
+  duration: index + 1,
+  maxRequests: index + 10,
+}));
+const originalUnmanagedRule = {
+  label: "unrelated:create",
+  audience: "",
+  duration: 41,
+  maxRequests: 42,
+};
+const stateCollection = { id: "workshop_migration_state" };
+let stateSnapshot = JSON.stringify({ marker: "retain-existing-migration-state" });
+const stateRecord = {
+  getString(name) {
+    assert.equal(name, "snapshot");
+    return stateSnapshot;
+  },
+  set(name, value) {
+    assert.equal(name, "snapshot");
+    stateSnapshot = value;
+  },
+};
+const settings = {
+  rateLimits: {
+    enabled: false,
+    rules: [originalUnmanagedRule, ...originalManagedRules],
+  },
+};
+const app = {
+  findCollectionByNameOrId(name) {
+    if (name !== stateCollection.id) throw new Error(`Missing collection ${name}`);
+    return stateCollection;
+  },
+  findFirstRecordByFilter(collectionId) {
+    if (collectionId !== stateCollection.id) throw new Error(`Missing state record ${collectionId}`);
+    return stateRecord;
+  },
+  save() {},
+  settings() {
+    return settings;
+  },
+};
+
+const loadMigration = async (path) => {
+  let loaded;
+  const source = await readFile(path, "utf8");
+  vm.runInNewContext(source, {
+    console,
+    migrate: (up, down) => {
+      loaded = { up, down };
+    },
+  });
+  if (!loaded) throw new Error(`Workshop migration did not register: ${path}`);
+  return loaded;
+};
+
+const migration = await loadMigration("apps/workshop/pb_migrations/1784240001_workshop_write_rate_limits.js");
+
+migration.up(app);
+const savedState = JSON.parse(stateSnapshot);
+assert.equal(savedState.marker, "retain-existing-migration-state");
+assert.deepEqual(savedState.writeRateLimits, {
+  enabled: false,
+  managedRules: originalManagedRules,
+});
+assert.equal(settings.rateLimits.enabled, true);
+assert.deepEqual(
+  normalized(settings.rateLimits.rules.filter((rule) => !managedLabels.includes(rule.label))),
+  [originalUnmanagedRule],
+  "Applying Workshop rate limits changed unrelated rules",
+);
+assert.deepEqual(
+  new Set(settings.rateLimits.rules.map((rule) => rule.label)),
+  new Set([...managedLabels, originalUnmanagedRule.label]),
+  "Applying Workshop rate limits did not replace every managed label",
+);
+
+const laterUnmanagedRule = {
+  label: "added-after-migration:update",
+  audience: "@auth",
+  duration: 51,
+  maxRequests: 52,
+};
+settings.rateLimits.rules = [
+  laterUnmanagedRule,
+  ...settings.rateLimits.rules.filter((rule) => managedLabels.includes(rule.label)),
+];
+migration.down(app);
+
+assert.equal(settings.rateLimits.enabled, false, "Rollback did not restore the original enabled flag");
+assert.deepEqual(
+  normalized(settings.rateLimits.rules.filter((rule) => !managedLabels.includes(rule.label))),
+  [laterUnmanagedRule],
+  "Rollback changed unrelated rules added after the migration",
+);
+assert.deepEqual(
+  normalized(settings.rateLimits.rules.filter((rule) => managedLabels.includes(rule.label))),
+  originalManagedRules,
+  "Rollback did not restore every original managed-label rule",
+);
+
+assert.throws(
+  () =>
+    migration.up({
+      ...app,
+      findCollectionByNameOrId() {
+        throw new Error("missing");
+      },
+    }),
+  /migration state is unavailable/u,
+  "Migration must fail closed when its persistent rollback snapshot is unavailable",
+);
+
+const legacyOriginalRules = [
+  { label: "unrelated:create", audience: "", duration: 71, maxRequests: 72 },
+  { label: "submissions:create", audience: "", duration: 73, maxRequests: 74 },
+  { label: "comments:update", audience: "@auth", duration: 75, maxRequests: 76 },
+];
+const legacyStateCollection = { id: "workshop_migration_state" };
+let legacyStateSnapshot = JSON.stringify({
+  marker: "already-applied-old-0001",
+  rateLimits: { enabled: false, rules: legacyOriginalRules },
+});
+const legacyStateRecord = {
+  getString() {
+    return legacyStateSnapshot;
+  },
+  set(_name, value) {
+    legacyStateSnapshot = value;
+  },
+};
+const legacySettings = {
+  rateLimits: {
+    enabled: true,
+    rules: [
+      { label: "unrelated:create", audience: "", duration: 71, maxRequests: 72 },
+      ...managedLabels.map((label) => ({ label, audience: "@auth", duration: 60, maxRequests: 30 })),
+    ],
+  },
+};
+const legacyApp = {
+  findCollectionByNameOrId() {
+    return legacyStateCollection;
+  },
+  findFirstRecordByFilter() {
+    return legacyStateRecord;
+  },
+  save() {},
+  settings() {
+    return legacySettings;
+  },
+};
+const compatibilityMigration = await loadMigration(
+  "apps/workshop/pb_migrations/1784240003_workshop_rate_limit_rollback_compatibility.js",
+);
+compatibilityMigration.up(legacyApp);
+compatibilityMigration.down(legacyApp);
+
+const backfilledState = JSON.parse(legacyStateSnapshot);
+assert.equal(backfilledState.marker, "already-applied-old-0001");
+assert.deepEqual(backfilledState.writeRateLimits, {
+  enabled: true,
+  managedRules: [
+    { label: "comments:update", audience: "@auth", duration: 75, maxRequests: 76 },
+    { label: "submissions:create", audience: "@auth", duration: 60, maxRequests: 10 },
+    { label: "votes:create", audience: "@auth", duration: 10, maxRequests: 20 },
+    { label: "comments:create", audience: "@auth", duration: 60, maxRequests: 20 },
+    { label: "reports:create", audience: "@auth", duration: 60, maxRequests: 5 },
+  ],
+});
+
+migration.down(legacyApp);
+assert.equal(legacySettings.rateLimits.enabled, true);
+assert.deepEqual(
+  normalized(legacySettings.rateLimits.rules),
+  [
+    { label: "unrelated:create", audience: "", duration: 71, maxRequests: 72 },
+    ...backfilledState.writeRateLimits.managedRules,
+  ],
+  "An already-applied legacy migration could not be rolled back after the compatibility backfill",
+);
+
+console.log("Workshop write rate-limit migration restoration passed");

apps/workshop/tests/workshop.integration.mjs

@@ -98,11 +98,37 @@ for (const [field, value] of [
     404,
   );
 }
+const forgedAvatar = new FormData();
+forgedAvatar.set(
+  "avatar",
+  new Blob(
+    [
+      Buffer.from(
+        "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=",
+        "base64",
+      ),
+    ],
+    { type: "image/png" },
+  ),
+  "forged-avatar.png",
+);
+const avatarResponse = await fetch(`${baseUrl}/api/collections/users/records/${author.id}`, {
+  method: "PATCH",
+  headers: authorSession.headers,
+  body: forgedAvatar,
+});
+const avatarResponseBody = await avatarResponse.json().catch(() => null);
+if (avatarResponse.status !== 400 || !JSON.stringify(avatarResponseBody).includes("managed by GitHub OAuth")) {
+  throw new Error(
+    `Workshop user avatar upload bypassed the OAuth identity hook: ${avatarResponse.status} ${JSON.stringify(avatarResponseBody)}`,
+  );
+}
 const unchangedAuthor = await request(`/api/collections/users/records/${author.id}`, {
   headers: authorSession.headers,
 });
 if (
   unchangedAuthor.displayName !== "Widget Author" ||
+  unchangedAuthor.avatar ||
   unchangedAuthor.avatarUrl ||
   unchangedAuthor.githubUsername ||
   unchangedAuthor.githubProfileUrl