Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Self-review
Risks / notes
Verification
|
Code Review — Anti-Abuse GuardsOverallSolid work. The scope is right — reserved namespaces, rate limits, size caps, and stale reclaim. All the things the discussion called out as missing from MVP. Test coverage is thorough (7 new integration tests). Approved with minor notesGood decisions:
Minor observations (not blocking):
SummaryShip it. The implementation matches the project plan's "Phase 1 Controls" section exactly. The namespace index removal is a bonus win. |
There was a problem hiding this comment.
Pull request overview
Adds first-pass hosted anti-abuse protections to the pubmd hosted write surface by enforcing reserved namespaces, markdown size caps, claim/publish rate limits (persisted in storage), and reclaiming stale unpublished namespace claims.
Changes:
- Enforce reserved namespaces, markdown max size, and service-layer claim/publish rate limits (with persisted rate-limit buckets).
- Add reclaim-on-claim behavior for stale unpublished namespaces.
- Expand HTTP integration coverage for abuse/edge cases and update project planning docs.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/blob-store.test.ts | Extends Blob mock to support listing blobs for new list-based page indexing. |
| tests/integration/test-server.ts | Exposes dataDir from the test server for reclaim behavior tests that edit on-disk records. |
| tests/integration/server.test.ts | Adds integration tests for reserved namespaces, rate limits, oversize markdown, reclaim behavior, wrong-token publish, and slug conflicts. |
| src/server/app.ts | Plumbs request IP into claim/publish flows and maps new abuse-control errors to HTTP status codes. |
| src/core/repository.ts | Introduces abuse-control error types and repository methods for persisting rate-limit records and overwriting namespace records. |
| src/core/publish-service.ts | Implements reserved namespace checks, markdown size caps, claim/publish rate limiting, and reclaim-on-claim logic. |
| src/core/markdown.ts | Small CSS tweak for blockquote strong styling (inherit). |
| src/core/file-store.ts | Persists rate-limit buckets on disk and supports namespace record overwrites for reclaim behavior. |
| src/core/blob-store.ts | Persists rate-limit buckets in Blob storage and changes page listing to be derived from lookup blobs. |
| docs/project-plan.md | Documents cost model and the anti-abuse control plan; marks milestones complete. |
| docs/progress.md | Records completion of the anti-abuse plan, implementation, and integration coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tokenHash, | ||
| createdAt: new Date().toISOString(), | ||
| }, | ||
| "wx", | ||
| ); |
There was a problem hiding this comment.
claimNamespace relies on writeFile(..., { flag: "wx" }) throwing on existing files. If two claim requests race, this will surface as a raw EEXIST filesystem error (currently mapped to a generic 400), instead of a consistent 409 NamespaceExistsError. Consider catching EEXIST here (or in the service) and throwing NamespaceExistsError so concurrent claims behave predictably.
| async function listPages(namespace: string): Promise<StoredPage[]> { | ||
| const index = await readJsonBlob( | ||
| namespaceIndexPath(namespace), | ||
| NamespacePageIndexSchema, | ||
| ); | ||
| const pages = index?.pages ?? []; | ||
| const lookupResults = await list({ | ||
| limit: 1000, | ||
| prefix: `${lookupPrefix(namespace)}/`, | ||
| token: metadataToken, | ||
| }); |
There was a problem hiding this comment.
listPages hard-codes limit: 1000 and ignores pagination (hasMore/cursor). Namespaces with >1000 pages will be silently truncated. Consider iterating through all pages using the list API’s pagination fields so listing remains correct for larger namespaces.
| return pages | ||
| .filter((page): page is StoredPage => page !== null) | ||
| .sort((left, right) => right.updatedAt.localeCompare(left.updatedAt)); |
There was a problem hiding this comment.
listPages can return duplicates if multiple lookup blobs point at the same pageId (e.g., if cleanup of an old slug lookup fails after a rename). Since the method maps lookups -> findPageById without deduping, the same page can appear multiple times. Consider deduping by pageId (or slug) after resolving pages.
| return pages | |
| .filter((page): page is StoredPage => page !== null) | |
| .sort((left, right) => right.updatedAt.localeCompare(left.updatedAt)); | |
| const nonNullPages = pages.filter( | |
| (page): page is StoredPage => page !== null, | |
| ); | |
| const uniquePagesById = new Map<string, StoredPage>(); | |
| for (const page of nonNullPages) { | |
| // Deduplicate by page identifier to avoid returning the same page multiple times | |
| if (!uniquePagesById.has(page.id)) { | |
| uniquePagesById.set(page.id, page); | |
| } | |
| } | |
| return Array.from(uniquePagesById.values()).sort((left, right) => | |
| right.updatedAt.localeCompare(left.updatedAt), | |
| ); |
| tokenHash, | ||
| createdAt: new Date().toISOString(), | ||
| }, | ||
| false, | ||
| ); |
There was a problem hiding this comment.
When allowOverwrite is false, put() will fail if the namespace blob already exists (including race conditions between getNamespace and claimNamespace). That failure currently propagates as a storage error and will be mapped to a generic HTTP 400/500. Consider translating overwrite-conflict errors into NamespaceExistsError to keep claim behavior stable under concurrency.
| function requestIp(context: Context): string | undefined { | ||
| return ( | ||
| context.req.header("x-real-ip") ?? | ||
| context.req.header("cf-connecting-ip") ?? | ||
| undefined |
There was a problem hiding this comment.
requestIp trusts x-real-ip / cf-connecting-ip directly. In deployments not sitting behind a proxy that strips/overwrites these headers, clients can spoof them and bypass IP-based rate limits. Consider either (a) only trusting these headers when trustProxy is enabled / the immediate peer is a known proxy, or (b) using the runtime’s remote address as the primary source and falling back to headers only in trusted environments.
Why
The hosted pubmd instance is live and usable, but it still lacks the basic protections needed for a free public write surface. This change adds a first-pass set of controls so namespace claiming and publishing are harder to abuse without adding a heavyweight auth system.
What
Test plan