From a2e935d1d8cca11f63c9c610611b418b3077984d Mon Sep 17 00:00:00 2001 From: Edwin van den Houdt Date: Thu, 27 Aug 2026 10:48:04 +0200 Subject: [PATCH] fix(uploads): authorize the document-content and status endpoints (RB-01) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /uploads/{documentId}/content took only (string documentId) — no HttpContext, so no authorization was possible. It streams diploma and identity scans, protected by GUID unguessability alone, while DELETE on the same resource has always been owner-scoped. GET /uploads/status had the same shape and confirmed whether any client-chosen localId exists, plus its documentId. Both now take HttpContext. Content is readable by the owning ZorgverlenerCaller or a caller passing Authz.CanBeoordelen — matched on the caller kind rather than branched on a boolean, because ctx.Zorgverlener() throws for a MedewerkerCaller and the behandelportal's beoordeling screen is a legitimate reader. Status is scoped to ctx.Zorgverlener().Bsn via a new owner parameter on DocumentStore.ByLocalIds (one call site). 404, not 403, on both: a foreign document id must not be distinguishable from one that never existed, and a foreign localId reads back as "unknown". Residual, recorded in the implementation note: both callers reach the URL as a plain browser navigation ( / previewUrl), which carries no identity header and no interceptor, so StubIdentityProvider resolves it to the seeded citizen. That is BIO-002 and belongs to RB-09; the links keep working today only because one citizen owns every document in the POC. Co-Authored-By: Claude Opus 5 --- .../src/BigRegister.Api/Data/DocumentStore.cs | 4 +- backend/src/BigRegister.Api/Program.cs | 18 +++- .../BigRegister.Tests/UploadAccessTests.cs | 83 +++++++++++++++++++ .../refactor-backlog/implementation/rb-01.md | 57 +++++++++++++ 4 files changed, 156 insertions(+), 6 deletions(-) create mode 100644 backend/tests/BigRegister.Tests/UploadAccessTests.cs create mode 100644 docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-01.md diff --git a/backend/src/BigRegister.Api/Data/DocumentStore.cs b/backend/src/BigRegister.Api/Data/DocumentStore.cs index 1f2faa0..4b60054 100644 --- a/backend/src/BigRegister.Api/Data/DocumentStore.cs +++ b/backend/src/BigRegister.Api/Data/DocumentStore.cs @@ -73,13 +73,13 @@ public static class DocumentStore /// Status for the poll-on-return pattern: a known localId is "complete" (it /// arrived), an unknown one is still in flight / never started. - public static IReadOnlyList ByLocalIds(IEnumerable localIds) + public static IReadOnlyList ByLocalIds(IEnumerable localIds, string owner) { var set = localIds.ToHashSet(); lock (_gate) { using var db = Db.Create(); - return db.Documents.Where(d => set.Contains(d.LocalId)).ToList(); + return db.Documents.Where(d => set.Contains(d.LocalId) && d.Owner == owner).ToList(); } } diff --git a/backend/src/BigRegister.Api/Program.cs b/backend/src/BigRegister.Api/Program.cs index 5dd830d..e390131 100644 --- a/backend/src/BigRegister.Api/Program.cs +++ b/backend/src/BigRegister.Api/Program.cs @@ -228,10 +228,18 @@ api.MapPost("/uploads", async (HttpRequest request, HttpContext ctx, IDocumentSo // Serve stored bytes so a re-opened wizard can preview/download an upload. Inline // for pdf/image (browser renders it), attachment otherwise (download). -api.MapGet("/uploads/{documentId}/content", (string documentId) => +// Scoped like DELETE on the same resource (RB-01/BIO-004): the owning citizen, or a +// behandelaar reading an aanvraag's linked documents. A foreign id 404s rather than +// 403s, so the endpoint never confirms that a document id exists. +api.MapGet("/uploads/{documentId}/content", (string documentId, HttpContext ctx) => { var doc = DocumentStore.Get(documentId); - if (doc is null) return Results.NotFound(); + var allowed = ctx.Caller() switch + { + ZorgverlenerCaller z => doc?.Owner == z.Bsn, + var caller => Authz.CanBeoordelen(caller), + }; + if (doc is null || !allowed) return Results.NotFound(); var inline = doc.ContentType == "application/pdf" || doc.ContentType.StartsWith("image/"); return Results.File(doc.Content, doc.ContentType, fileDownloadName: inline ? null : doc.FileName); }) @@ -239,10 +247,12 @@ api.MapGet("/uploads/{documentId}/content", (string documentId) => .Produces(StatusCodes.Status404NotFound); // Poll-on-return: which of these client localIds have arrived at the BFF. -api.MapGet("/uploads/status", (string? localIds) => +api.MapGet("/uploads/status", (string? localIds, HttpContext ctx) => { var ids = (localIds ?? "").Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); - var found = DocumentStore.ByLocalIds(ids).ToDictionary(d => d.LocalId); + // Owner-scoped (RB-01/BIO-004): someone else's localId reads back as "unknown", the + // same answer an id that never existed gets. + var found = DocumentStore.ByLocalIds(ids, ctx.Zorgverlener().Bsn).ToDictionary(d => d.LocalId); var results = ids.Select(id => found.TryGetValue(id, out var d) ? new UploadStatusItemDto(id, "complete", d.DocumentId) : new UploadStatusItemDto(id, "unknown", null)).ToList(); diff --git a/backend/tests/BigRegister.Tests/UploadAccessTests.cs b/backend/tests/BigRegister.Tests/UploadAccessTests.cs new file mode 100644 index 0000000..54e094f --- /dev/null +++ b/backend/tests/BigRegister.Tests/UploadAccessTests.cs @@ -0,0 +1,83 @@ +using System.Net; +using System.Net.Http.Headers; +using System.Net.Http.Json; +using BigRegister.Api.Contracts; +using Microsoft.AspNetCore.Mvc.Testing; + +namespace BigRegister.Tests; + +/// RB-01/BIO-004: GET /uploads/{id}/content and /uploads/status used to take no +/// HttpContext at all — a diploma or identity scan was protected by GUID +/// unguessability alone, while DELETE on the same resource was owner-scoped. +public class UploadAccessTests(TestWebApplicationFactory factory) : IClassFixture +{ + private readonly HttpClient _client = factory.CreateClient(); + + private const string OtherCitizen = "999999990"; + + private async Task UploadAsOwner() + { + var form = new MultipartFormDataContent(); + var file = new ByteArrayContent(new byte[] { 1, 2, 3 }); + file.Headers.ContentType = new MediaTypeHeaderValue("application/pdf"); + form.Add(file, "file", "diploma.pdf"); + form.Add(new StringContent("diploma"), "categoryId"); + form.Add(new StringContent("local-rb01"), "localId"); + form.Add(new StringContent("registratie"), "wizardId"); + var res = await _client.PostAsync("/api/v1/uploads", form); + Assert.Equal(HttpStatusCode.Created, res.StatusCode); + return (await res.Content.ReadFromJsonAsync())!.DocumentId; + } + + private Task Get(string path, params (string Name, string Value)[] headers) + { + var req = new HttpRequestMessage(HttpMethod.Get, path); + foreach (var (name, value) in headers) req.Headers.Add(name, value); + return _client.SendAsync(req); + } + + [Fact] + public async Task The_owner_can_read_the_bytes() + { + var id = await UploadAsOwner(); + Assert.Equal(HttpStatusCode.OK, (await Get($"/api/v1/uploads/{id}/content")).StatusCode); + } + + [Fact] + public async Task Another_citizen_gets_404_not_403() + { + var id = await UploadAsOwner(); + // 404, not 403: a foreign id must not be distinguishable from one that never existed. + Assert.Equal(HttpStatusCode.NotFound, + (await Get($"/api/v1/uploads/{id}/content", ("X-Subject", OtherCitizen))).StatusCode); + } + + [Fact] + public async Task A_behandelaar_can_read_a_linked_document() + { + var id = await UploadAsOwner(); + Assert.Equal(HttpStatusCode.OK, + (await Get($"/api/v1/uploads/{id}/content", ("X-Medewerker", "medewerker-1"))).StatusCode); + } + + [Fact] + public async Task A_medewerker_without_the_behandelaar_rol_does_not() + { + var id = await UploadAsOwner(); + Assert.Equal(HttpStatusCode.NotFound, + (await Get($"/api/v1/uploads/{id}/content", + ("X-Medewerker", "medewerker-1"), ("X-Rollen", "geen"))).StatusCode); + } + + [Fact] + public async Task Status_reports_another_citizens_localId_as_unknown() + { + await UploadAsOwner(); + var res = await Get("/api/v1/uploads/status?localIds=local-rb01", ("X-Subject", OtherCitizen)); + res.EnsureSuccessStatusCode(); + var status = (await res.Content.ReadFromJsonAsync())!; + var item = Assert.Single(status.Results); + Assert.Equal("unknown", item.Status); + Assert.Null(item.DocumentId); + } +} diff --git a/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-01.md b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-01.md new file mode 100644 index 0000000..27ee3a9 --- /dev/null +++ b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-01.md @@ -0,0 +1,57 @@ +# RB-01 — authorize `GET /uploads/{id}/content` and `/uploads/status` + +Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-004 · `99-backlog.md` RB-01 + +## What was wrong + +`GET /uploads/{documentId}/content` took `(string documentId)` — no `HttpContext`, so no +authorization was possible at all. It streams diploma and identity scans; the only +protection was the unguessability of the document GUID. `DELETE` on the same resource has +been owner-scoped (`DocumentStore.DeleteOwned`) since it was written. + +`GET /uploads/status?localIds=` had the same shape, and leaks less but still confirms +whether a given client-chosen `localId` exists anywhere in the store, plus its documentId. + +## What changed + +| File | Change | +| ------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------- | +| `Program.cs` `/uploads/{documentId}/content` | takes `HttpContext`; allowed for the owning `ZorgverlenerCaller` or a caller passing `Authz.CanBeoordelen`; else `404` | +| `Program.cs` `/uploads/status` | takes `HttpContext`; scoped to `ctx.Zorgverlener().Bsn` | +| `Data/DocumentStore.cs` `ByLocalIds` | second parameter `owner`; filters on it (the only call site is the endpoint above) | +| `tests/BigRegister.Tests/UploadAccessTests.cs` | **new** — 5 cases | + +The two actor kinds are matched, not branched on a boolean, because `ctx.Zorgverlener()` +**throws** for a `MedewerkerCaller` — a behandelaar reading an aanvraag's linked documents +(`beoordeling-documenten.component.ts`) is a legitimate caller here: + +```csharp +var allowed = ctx.Caller() switch +{ + ZorgverlenerCaller z => doc?.Owner == z.Bsn, + var caller => Authz.CanBeoordelen(caller), +}; +``` + +**404, not 403**, per the ticket: a foreign document id must not be distinguishable from +one that never existed. `doc is null || !allowed` collapses both to the same answer, and +`/uploads/status` reports a foreign `localId` as `"unknown"` — the same word an id that +never existed gets. + +## Known residual — this endpoint is reached without identity headers + +Both callers link to the URL directly (`` in `beoordeling-documenten.component.ts`, +`previewUrl` in `libs/shared/src/upload/upload.adapter.ts`), so the request is a plain +browser navigation that carries **no** `X-Medewerker` / `X-Subject` header and never passes +through an Angular interceptor. `StubIdentityProvider` therefore resolves it to the seeded +citizen, which owns every document in the POC, so both links keep working — by coincidence, +not by authorization. That coincidence **is** BIO-002, and it is fixed by **RB-09** (making +`IIdentityProvider` able to express "no identity"), not here. RB-09 will need this endpoint +to receive a real credential — a signed URL or a cookie — rather than the ambient default. + +## Verification + +`dotnet build` clean. `dotnet test`: **250 passed, 1 failed** — the failure is +`OpenZaakIntegrationTests.Admin_cases_returns_the_seeded_zaak_mapped_through_real_HTTP_and_JWT`, +which needs a live OpenZaak container and **fails identically on a stashed tree**, i.e. it +pre-dates this change.