Give every parked attachment an owner, a ceiling, and the server's own limit #140
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fase-2-bijlagen-v2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Reopened from #111 (fase 2a of "nieuw bericht opstellen + plannen"), rebased onto current main now that #123 (fase 1) is merged. See #111 for the full rationale: ownership on parked attachment refs (the delete endpoint was a latent IDOR), a 1 GB per-user ceiling counted on bytes on disk, and the max file size asked from the Mattermost server instead of guessed.
Changes vs #111: the superpowers plan/spec documents that were accidentally committed are no longer part of the branch; the CHANGELOG entry is rebased into the current Unreleased section.
Verified on this branch: backend suite 1379 passed with the 100% coverage gate, frontend tsc + build + 505 vitest green, pre-commit clean.
Three things that were fine while attachments could not be scheduled, and stop being fine the moment refs become the only currency they travel in. **A ref had no owner.** It is a bare uuid, and DELETE /api/outgoing-files/{ref} checked nothing: a destructive IDOR waiting for the day refs actually carry something. _file_entries_from_refs checked only that the file existed, so a row could point at somebody else's bytes. Every read, write and delete now names the owner, enforced in one place - the one every route with file_refs already goes through - rather than in five places that each have to remember. A sidecar with no user_id reads as missing: fail closed, no grandfathering, because a branch that grants access without an owner never closes again. **There was no ceiling.** Several users share one pod and one volume. The disk-full guard refuses the last write; it does not stop the filling, and the user who fills the disk takes everyone else's attachments with them. 1 GB per user, counted over bytes on disk rather than bytes attached to a row - a ref only reaches a row when the debounced draft-save gets round to it, so a row-based sum counts a fresh upload as zero, and a loop that uploads without ever attaching is exactly what the ceiling is for. **The file-size limit was guessed, twice, differently.** cards.py said 55 MB ("MM's 50 MB default plus overhead"), outgoing_files.py said 100 MB. Both were true once: Mattermost raised its own default from 50 to 100 MB (MM-31277) and one comment never caught up. But the number is not ours - FileSettings.MaxFileSize is server configuration and an admin can move it. Mattermost hands it to every logged-in client, so we ask at link time and store it on the account. Best-effort: a server that will not tell us is still a server you can link, and we fall back to Mattermost's own default. EnableFileAttachments comes along, so a server with attachments switched off can stop us pointing a paperclip at it. The upload route loses its card. The card_id did nothing there but an ownership check that require_user already does, and a parked ref records no relationship to a card. POST /api/outgoing-files. wipe_done no longer deletes refs unconditionally. Editing a scheduled post cancels the row and hangs the same refs on the new draft, so a terminal row and a live one can share a file; _delete_orphan_refs knows that rule and is now the only place that deletes.Four things the adversarial review found, all real. A DoS I introduced. Replacing the fixed 100 MB cap with "ask the server" lost the ceiling: the advertised MaxFileSize is attacker-influenced (link a public server you control, have it advertise 10 GB), and the handler buffered the whole body into memory before rejecting it. absolute_max_upload_bytes (200 MB, operator-owned) now clamps the dynamic value, which may only ever go lower, and the read is bounded to cap+1 so a lied-about Content-Length cannot force a bigger buffer. The bug I fixed in wipe_done, still live in _settle_sent. A sent scheduled post deleted its refs unconditionally, but scheduling does not remove the card's draft, so a draft and the sent row can share a ref - and the send would wipe the file the draft still holds. Now routes through _delete_orphan_refs, same as wipe_done; the now-terminal SENT row is excluded, so only a genuine orphan goes. The frontend still pointed at the deleted route. uploadOutgoingFile POSTed to the old /api/cards/{id}/outgoing-files, which would 404 the instant phase 2b wired it. Now /api/outgoing-files, without the card argument it never needed. The actual attack was only unit-tested. Every ownership test called outgoing_files directly; nothing drove a second logged-in user against the real endpoints, so removing the guards left the whole suite green bar one file. Added a two-independent-client IDOR test (draft, schedule, delete all refused, Alice's bytes intact), an end-to-end quota test, a hard-cap test, and the shared-ref send-cleanup test. Each kills its mutation - verified by hand.42af473679a4dbf3e4f8nginx -t: fails as uid 101 in buildah/CI21b5d125fce8dd7c1a0drobbertbos referenced this pull request2026-07-21 04:26:35 +00:00