fix(uploads): finish the sweeps round 1 left half-done
Deep-review round 2, on round 1's own fix commit. It found MORE than round 1 did (28 raw, against 13), and most of it was mine -- a new piece of UI attracts findings a backend-only diff never does. The pattern in almost all of them is the same: a principle applied to the site that prompted it and not to its siblings. * `display_text` reached three of the FIVE writers of a user-supplied name. `POST /api/downloads` and `POST /api/downloads/edit` still handed Postgres an unbounded string, so a 300-character name pasted into the Add-download dialog or the editor was a bare 500 (main.py re-raises every DataError but the NUL case). Four of the seven reviewers found it independently. The bound now lives in `models.py`, the three columns are DECLARED with it, and the guard for the next writer is parsed rather than remembered: every `display_name` / `display_uploader` assignment under `app/routes/` must go through the helper. That guard immediately found two more -- the Google sign-in callback writes the profile name into the same 255-wide column -- so it is six, not five. * The connection release reached one of the THREE routes that read their own body. The screenshot route (10 MiB, from a phone) and the client log sink both still held a pooled connection idle-in-transaction for the length of the upload, which is the invariant db.py sizes the pool on. All three now call `db.release_read_connection`, whose precondition -- nothing pending on the shared session -- is CHECKED rather than described, because a whole-session rollback discarding a caller's staged write is a mistake this repo has already made once. `test_body_limit.py` holds the rule for the next such route. * The AST drift registry keyed its sites by (module, limit expression), so a second route copied from the first collapsed onto the same entry and went uncovered -- the exact hole the registry exists to close. Keyed per call site now, and it carries the release requirement too. * The dialog's own refusal was announced once and never again: two over-cap picks leave an identical node with identical text, and an untouched live region says nothing. It is keyed per pick now, the drop zone reads as invalid and points at the sentence, and the reason is reachable from the control rather than only from a one-shot announcement. * Two tests rebuilt the blind spot they were written to remove: the dialog's cap test sat nowhere near the boundary (a `>` loosened to `>=` survived it, which is the mutation round 1 fixed on the server side), and the header-precedence test only covered an exact-case duplicate while a differently-cased one would have been COMBINED onto the wire rather than replaced. Both fixed, and the merge now dedupes case-insensitively because header names are. Also: the upload dialog's unstubbed usage query was firing a real request from jsdom in a neighbouring suite and leaking a delayed "connection lost" toast across test boundaries; `GET /api/downloads/usage` had no test at all though the whole client-side check hangs off it; and the comment claiming the edge 413 is readable now says what was measured -- readable to 200 MiB over loopback, a bare `onerror` through a proxy -- so the pre-send check is named as the mechanism and that branch as the backstop it is. Every test added or changed here was mutation-checked. One mutation SURVIVED and is recorded rather than papered over: the `tooBig` term inside `submit()` cannot be reached, because `ui/Button` swallows the activation itself, so it is marked as belt and braces instead of being described as the guard.
This commit is contained in:
@@ -53,7 +53,12 @@ class FakeXHR {
|
||||
}
|
||||
open() {}
|
||||
setRequestHeader(k: string, v: string) {
|
||||
this.headers[k] = v;
|
||||
// A real XHR COMBINES values for a name it already has, case-insensitively, rather than
|
||||
// overwriting — so a plain assignment here would hide exactly the duplicate-header bug the
|
||||
// merge in `uploadReq` has to prevent.
|
||||
const existing = Object.keys(this.headers).find((h) => h.toLowerCase() === k.toLowerCase());
|
||||
if (existing === undefined) this.headers[k] = v;
|
||||
else this.headers[existing] = `${this.headers[existing]}, ${v}`;
|
||||
}
|
||||
send(body?: unknown) {
|
||||
this.sent = true;
|
||||
@@ -152,11 +157,17 @@ describe("uploadReq", () => {
|
||||
setDebugSession("0123456789abcdef");
|
||||
try {
|
||||
void uploadReq("/u", body(), {
|
||||
headers: { "X-Debug-Session": "deadbeefdeadbeef", "X-Upload-Filename": "a.bin" },
|
||||
// Lower-case on purpose: header names are case-insensitive and the codebase's own upload
|
||||
// headers are spelled that way on the server. A case-sensitive merge would not replace
|
||||
// this one, it would ADD it, and the wire would carry both values comma-joined.
|
||||
headers: { "x-debug-session": "deadbeefdeadbeef", "X-Upload-Filename": "a.bin" },
|
||||
});
|
||||
expect(FakeXHR.last.headers["X-Debug-Session"]).toBe("0123456789abcdef");
|
||||
const sent = Object.entries(FakeXHR.last.headers);
|
||||
const at = (n: string) => sent.filter(([k]) => k.toLowerCase() === n).map(([, v]) => v);
|
||||
// Exactly one, and it is the identity layer's — not two, and not the caller's.
|
||||
expect(at("x-debug-session")).toEqual(["0123456789abcdef"]);
|
||||
// ...while a header that is genuinely the caller's still rides.
|
||||
expect(FakeXHR.last.headers["X-Upload-Filename"]).toBe("a.bin");
|
||||
expect(at("x-upload-filename")).toEqual(["a.bin"]);
|
||||
} finally {
|
||||
setDebugSession(null);
|
||||
}
|
||||
|
||||
@@ -120,11 +120,17 @@ function localizeDetail(d: unknown): string {
|
||||
return t("downloads.errors.upload.tooLarge", { size: formatBytes(o.limit) });
|
||||
if (o?.code === "upload_empty") return t("downloads.errors.upload.empty");
|
||||
// The EDGE ceiling's refusal (`bodylimit.TOO_LARGE_BODY`), shared by every route and carrying no
|
||||
// number. Not a theoretical case: on a DEFAULT install it is what an over-cap upload actually
|
||||
// gets, because the ceiling and the route's own cap are the same figure and the edge answers
|
||||
// first — the route's numbered `upload_too_large` above is the answer only once an admin has
|
||||
// LOWERED the knob (see bodylimit.ROUTE_MAX_BODY_BYTES). Without this branch that refusal fell
|
||||
// through to the generic "couldn't carry out that action", the one message no retry can fix.
|
||||
// number. Not a theoretical case: on a DEFAULT install it is what an over-cap upload gets,
|
||||
// because the ceiling and the route's own cap are the same figure and the edge answers first —
|
||||
// the route's numbered `upload_too_large` above is the answer only once an admin has LOWERED the
|
||||
// knob (see bodylimit.ROUTE_MAX_BODY_BYTES). Without this branch that refusal fell through to
|
||||
// the generic "couldn't carry out that action", the one message no retry can fix.
|
||||
//
|
||||
// A BACKSTOP, not the plan. The middleware answers without draining, so whether the browser gets
|
||||
// to read this at all depends on the client having finished sending: measured readable up to
|
||||
// 200 MiB over loopback, and measured as a bare `onerror` through a dev proxy. That is why the
|
||||
// upload dialog checks the file against `upload_max_bytes` BEFORE sending — this branch is what
|
||||
// happens when that check could not run.
|
||||
if (o?.code === "too_large") return t("errors.tooLarge");
|
||||
// SEC-12: a download URL the server refuses to fetch. The reasons are deliberately distinct —
|
||||
// "we could not look that host up" is a different thing to tell someone than "we will not fetch
|
||||
@@ -409,11 +415,18 @@ export function uploadReq<T = unknown>(
|
||||
const requestId = newRequestId();
|
||||
const started = performance.now();
|
||||
xhr.setRequestHeader(REQUEST_ID_HEADER, requestId);
|
||||
// `identityHeaders()` LAST: who the request is from is this layer's to state, not the caller's.
|
||||
// Merged the other way round, a caller's header of the same name would silently re-attribute
|
||||
// the upload to another account — the one thing in here that must not be overridable.
|
||||
for (const [name, value] of Object.entries({ ...cfg.headers, ...identityHeaders() }))
|
||||
xhr.setRequestHeader(name, value);
|
||||
// `identityHeaders()` LAST, and LOWER-CASED on the way in: who the request is from is this
|
||||
// layer's to state, not the caller's. Merged the other way round a caller could silently
|
||||
// re-attribute the upload; merged case-sensitively, a caller spelling the same header in
|
||||
// another case would not be replaced but APPENDED — `setRequestHeader` combines values for a
|
||||
// name that already exists, so the wire would carry both, comma-joined. Header names are
|
||||
// case-insensitive, so the merge has to be too.
|
||||
const merged = new Map<string, [string, string]>(); // lower-cased name -> [as spelled, value]
|
||||
for (const [name, value] of Object.entries(cfg.headers ?? {}))
|
||||
merged.set(name.toLowerCase(), [name, value]);
|
||||
for (const [name, value] of Object.entries(identityHeaders()))
|
||||
merged.set(name.toLowerCase(), [name, value]);
|
||||
for (const [name, value] of merged.values()) xhr.setRequestHeader(name, value);
|
||||
if (cfg.onProgress) {
|
||||
xhr.upload.onprogress = (e) => {
|
||||
if (e.lengthComputable) cfg.onProgress?.(Math.round((e.loaded / e.total) * 100));
|
||||
|
||||
@@ -0,0 +1,38 @@
|
||||
import { readFileSync } from "node:fs";
|
||||
import { describe, expect, it } from "vitest";
|
||||
|
||||
// A bound that has to hold in two languages at once. The backend truncates a user-supplied display
|
||||
// name to `DISPLAY_TEXT_MAX` on its way into a varchar column of the same width (the columns are
|
||||
// declared with it); the upload dialog stops the user typing at the same number, so the truncation
|
||||
// is never a silent surprise. Neither side can see the other's copy, so the gate reads both.
|
||||
//
|
||||
// Widening one side alone is the failure this catches: the column and the Python constant move
|
||||
// together, the input keeps clipping at the old number, and the field simply stops accepting
|
||||
// keystrokes with no message — visible only by reading two files in two languages.
|
||||
//
|
||||
// Read as TEXT, like `i18n/backendIds.contract.test.ts` does, rather than by importing the module:
|
||||
// the api slice pulls in i18n, which touches localStorage at load and would drag a DOM into a check
|
||||
// that is pure string comparison.
|
||||
const src = (u: string) => readFileSync(new URL(u, import.meta.url), "utf-8");
|
||||
|
||||
describe("display-name bound", () => {
|
||||
it("matches the backend's DISPLAY_TEXT_MAX", () => {
|
||||
const backend = src("../../../../backend/app/models.py").match(
|
||||
/^DISPLAY_TEXT_MAX\s*=\s*(\d+)$/m,
|
||||
);
|
||||
const frontend = src("./downloads.ts").match(/^export const DISPLAY_NAME_MAX = (\d+);$/m);
|
||||
expect(backend, "backend/app/models.py no longer defines DISPLAY_TEXT_MAX").toBeTruthy();
|
||||
expect(frontend, "lib/api/downloads.ts no longer exports DISPLAY_NAME_MAX").toBeTruthy();
|
||||
expect(Number(frontend![1])).toBe(Number(backend![1]));
|
||||
});
|
||||
|
||||
it("is the width the columns are actually declared with", () => {
|
||||
// The mutation guard for the check above: both constants could agree on a number neither
|
||||
// column uses. `String(DISPLAY_TEXT_MAX)` is how the three display columns are declared, so a
|
||||
// literal creeping back into one of them fails here.
|
||||
const models = src("../../../../backend/app/models.py");
|
||||
const declared = [...models.matchAll(/display_(?:name|uploader): Mapped.*?String\((.*?)\)/g)];
|
||||
expect(declared).toHaveLength(3);
|
||||
for (const m of declared) expect(m[1]).toBe("DISPLAY_TEXT_MAX");
|
||||
});
|
||||
});
|
||||
@@ -116,6 +116,12 @@ export interface ShareLink {
|
||||
created_at: string | null;
|
||||
}
|
||||
|
||||
// The width of every user-supplied display name on a job, mirroring the backend's DISPLAY_TEXT_MAX
|
||||
// (backend/app/models.py) so an input can stop the user rather than letting the server truncate in
|
||||
// silence. The two are held together by `downloads.contract.test.ts` — a number living in two
|
||||
// languages is exactly the drift that CLAUDE.md's comment-vs-code rule is about.
|
||||
export const DISPLAY_NAME_MAX = 255;
|
||||
|
||||
export interface DownloadUsage {
|
||||
footprint_bytes: number;
|
||||
active_jobs: number;
|
||||
|
||||
Reference in New Issue
Block a user