Give every parked attachment an owner, a ceiling, and the server's own limit #111

Closed

robbertbos wants to merge 1 commit from fase-2-bijlagen into fase-1-fundament

robbertbos commented

2026-07-12 15:58:00 +00:00

Owner

Fase 2a van "nieuw bericht opstellen en plannen". Base: fase-1-fundament (PR #110).

Nog geen feature: dit maakt de bijlagen-laag veilig genoeg om er straks de kabel op aan te sluiten. Drie dingen die prima waren zolang je bijlagen niet kon plannen, en dat niet meer zijn zodra refs de enige munteenheid worden waarin ze rondgaan.

Een ref had geen eigenaar

Een ref is een kale UUID. DELETE /api/outgoing-files/{ref} controleerde niets, en _file_entries_from_refs valideerde alleen dát het bestand bestond. Gebruiker A kon dus het geparkeerde bestand van gebruiker B aan zijn eigen bericht hangen - en het verwijder-endpoint was een destructieve IDOR die lag te wachten op de dag dat refs echt iets zouden dragen.

Elke lees-, schrijf- en verwijderactie noemt nu de eigenaar. Afgedwongen op één plek - dezelfde waar elke route met file_refs toch al doorheen gaat - in plaats van op vijf plekken die het elk apart moeten onthouden. Een zijbestand zónder user_id leest als niet-gevonden: fail closed, geen gedoogregel, want een tak die zonder eigenaar tóch toegang geeft gaat nooit meer dicht.

Geen 403 maar een 404: die vertelt een aanvaller niets wat een 403 niet ook zou verklappen.

Er was geen plafond

Meerdere gebruikers delen één pod en één volume. De schijf-vol-check weigert de láátste schrijfactie; hij voorkomt het volschrijven niet. Wie de schijf volzet, neemt de bijlagen van alle anderen mee.

1 GB per gebruiker (WAGGLE_MAX_PARKED_BYTES_PER_USER), geteld over de bytes op schijf en niet over de bytes die aan een rij hangen. Een ref bereikt pas een rij zodra de gedebouncede concept-opslag eraan toekomt, dus een rij-gebaseerde som telt een verse upload als nul - en een lus die uploadt zonder ooit aan te hechten is precies waar het plafond voor bestaat.

De bestandslimiet was gegokt, twee keer, verschillend

cards.py zei 55 MB ("MM's 50 MB-default plus overhead"), outgoing_files.py zei 100 MB. Allebei waren ooit waar: Mattermost verhoogde zijn eigen default van 50 naar 100 MB (MM-31277) en één comment liep achter.

Maar het getal is niet van ons. FileSettings.MaxFileSize is serverconfiguratie en een beheerder kan hem verzetten. Mattermost vertelt hem gewoon aan elke ingelogde client (server/config/client.go, GenerateClientConfig), dus we vragen het bij het koppelen en bewaren het op het account.

Best-effort: een server die het niet wil zeggen, is nog steeds een server die je kunt koppelen - dan vallen we terug op Mattermosts eigen default. EnableFileAttachments komt mee, zodat een server met bijlagen uit ons kan tegenhouden er een paperclip op te richten.

De les is niet "kies het juiste getal", maar: bezit geen getal dat van iemand anders is.

Verder

De upload-route verliest zijn kaart. Het card_id deed daar niets anders dan een eigenaarscheck die require_user al doet, en een geparkeerde ref legt geen kaartrelatie vast. Wordt POST /api/outgoing-files.
wipe_done verwijdert refs niet meer onvoorwaardelijk. Een gepland bericht bewerken annuleert de rij en hangt dezelfde refs aan het nieuwe concept, dus een terminale en een levende rij kunnen hetzelfde bestand delen. "Wachtrij opschonen" wiste dan de bijlage waar je op dat moment mee aan het schrijven was. _delete_orphan_refs kent die regel, en is nu de enige die verwijdert.
507 lekt geen servergetallen meer. Er stond letterlijk het aantal vrije bytes op de server in. Foutmeldingen zijn Nederlands en zeggen wat je eraan kunt doen.
docs/deployment.md legt de volume-eis vast, mét grootte. Die stond alleen in een comment in de Containerfile - te dun voor iets waar bijlagen van afhangen.

Poorten

1358 tests groen, 100,00% backend-dekking, pre-commit groen.

Wat hierna komt (fase 2b)

De kabel écht aansluiten: de frontend van MM-file_ids naar refs, het antwoord-endpoint van file_ids naar file_refs met een gedeelde upload-helper, en de wees-opruimer. Pas dán kun je een bijlage bij een gepland bericht meesturen.

Fase 2a van "nieuw bericht opstellen en plannen". Base: `fase-1-fundament` (PR #110). Nog geen feature: dit maakt de bijlagen-laag veilig genoeg om er straks de kabel op aan te sluiten. Drie dingen die prima waren zolang je bijlagen **niet** kon plannen, en dat niet meer zijn zodra refs de enige munteenheid worden waarin ze rondgaan. ## Een ref had geen eigenaar Een `ref` is een kale UUID. `DELETE /api/outgoing-files/{ref}` controleerde **niets**, en `_file_entries_from_refs` valideerde alleen dát het bestand bestond. Gebruiker A kon dus het geparkeerde bestand van gebruiker B aan zijn eigen bericht hangen - en het verwijder-endpoint was een destructieve IDOR die lag te wachten op de dag dat refs echt iets zouden dragen. Elke lees-, schrijf- en verwijderactie noemt nu de eigenaar. Afgedwongen op **één** plek - dezelfde waar elke route met `file_refs` toch al doorheen gaat - in plaats van op vijf plekken die het elk apart moeten onthouden. Een zijbestand zónder `user_id` leest als niet-gevonden: fail closed, geen gedoogregel, want een tak die zonder eigenaar tóch toegang geeft gaat nooit meer dicht. Geen 403 maar een 404: die vertelt een aanvaller niets wat een 403 niet ook zou verklappen. ## Er was geen plafond Meerdere gebruikers delen één pod en één volume. De schijf-vol-check weigert de láátste schrijfactie; hij voorkomt het volschrijven niet. Wie de schijf volzet, neemt de bijlagen van alle anderen mee. **1 GB per gebruiker** (`WAGGLE_MAX_PARKED_BYTES_PER_USER`), geteld over de bytes **op schijf** en niet over de bytes die aan een rij hangen. Een ref bereikt pas een rij zodra de gedebouncede concept-opslag eraan toekomt, dus een rij-gebaseerde som telt een verse upload als nul - en een lus die uploadt zonder ooit aan te hechten is precies waar het plafond voor bestaat. ## De bestandslimiet was gegokt, twee keer, verschillend `cards.py` zei 55 MB ("MM's 50 MB-default plus overhead"), `outgoing_files.py` zei 100 MB. **Allebei waren ooit waar**: Mattermost verhoogde zijn eigen default van 50 naar 100 MB ([MM-31277](https://github.com/mattermost/mattermost/pull/16668)) en één comment liep achter. Maar het getal is niet van ons. `FileSettings.MaxFileSize` is serverconfiguratie en een beheerder kan hem verzetten. Mattermost vertelt hem gewoon aan elke ingelogde client (`server/config/client.go`, `GenerateClientConfig`), dus we vragen het bij het koppelen en bewaren het op het account. Best-effort: een server die het niet wil zeggen, is nog steeds een server die je kunt koppelen - dan vallen we terug op Mattermosts eigen default. `EnableFileAttachments` komt mee, zodat een server met bijlagen uit ons kan tegenhouden er een paperclip op te richten. De les is niet "kies het juiste getal", maar: bezit geen getal dat van iemand anders is. ## Verder - **De upload-route verliest zijn kaart.** Het `card_id` deed daar niets anders dan een eigenaarscheck die `require_user` al doet, en een geparkeerde ref legt geen kaartrelatie vast. Wordt `POST /api/outgoing-files`. - **`wipe_done` verwijdert refs niet meer onvoorwaardelijk.** Een gepland bericht bewerken annuleert de rij en hangt dezelfde refs aan het nieuwe concept, dus een terminale en een levende rij kunnen hetzelfde bestand delen. "Wachtrij opschonen" wiste dan de bijlage waar je op dat moment mee aan het schrijven was. `_delete_orphan_refs` kent die regel, en is nu de enige die verwijdert. - **507 lekt geen servergetallen meer.** Er stond letterlijk het aantal vrije bytes op de server in. Foutmeldingen zijn Nederlands en zeggen wat je eraan kunt doen. - `docs/deployment.md` legt de volume-eis vast, mét grootte. Die stond alleen in een comment in de Containerfile - te dun voor iets waar bijlagen van afhangen. ## Poorten 1358 tests groen, **100,00% backend-dekking**, pre-commit groen. ## Wat hierna komt (fase 2b) De kabel écht aansluiten: de frontend van MM-`file_id`s naar refs, het antwoord-endpoint van `file_ids` naar `file_refs` met een gedeelde upload-helper, en de wees-opruimer. Pas dán kun je een bijlage bij een gepland bericht meesturen.

robbertbos added 1 commit

2026-07-12 15:58:00 +00:00

Give every parked attachment an owner, a ceiling, and the server's own limit

CI / pre-commit (pull_request) Successful in 1m10s

Details

CI / frontend-test (pull_request) Successful in 4m17s

Details

CI / release-scripts (pull_request) Successful in 9s

Details

CI / backend-test (pull_request) Successful in 6m14s

Details

CI / backend-test-postgres (pull_request) Successful in 8m35s

Details

CI / e2e (pull_request) Successful in 8m1s

Details

8e07788b0f

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.

robbertbos referenced this pull request

2026-07-12 18:38:15 +00:00

Connect the attachment cable end-to-end (fase 2b) #113

robbertbos closed this pull request

2026-07-19 20:31:52 +00:00

robbertbos referenced this pull request

2026-07-20 06:44:36 +00:00

Give every parked attachment an owner, a ceiling, and the server's own limit #140