From b1276938d4f059e0325d4c24a9896d397f1e6c4f Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Fri, 17 Jul 2026 01:21:03 +0200 Subject: [PATCH] security(upload): add permission to upload_file_streaming() --- src/application/ports/file_ports.rs | 19 ++++ .../services/file_upload_service.rs | 38 ++++++++ src/common/stubs.rs | 11 +++ .../api/handlers/chunked_upload_handler.rs | 21 ++++- tests/api/grants.hurl | 90 +++++++++++++++++++ 5 files changed, 175 insertions(+), 4 deletions(-) diff --git a/src/application/ports/file_ports.rs b/src/application/ports/file_ports.rs index fe9bac7a..86539185 100644 --- a/src/application/ports/file_ports.rs +++ b/src/application/ports/file_ports.rs @@ -60,6 +60,25 @@ pub trait FileUploadUseCase: Send + Sync + 'static { caller_id: Uuid, ) -> Result; + /// `_with_perms` variant of `upload_file_streaming` — enforces + /// `Create` on the target folder before registering the row. + /// + /// AuthZ audit #17 (2026-07-12): the chunked-upload `complete` + /// path called plain `upload_file_streaming` at finalize; a grant + /// revoked between session open and finalize stayed effective + /// until the caller landed the final chunk (up to 24h JWT TTL, + /// forever with app-passwords). Handlers now call this variant + /// so the engine re-checks at finalize regardless of how long + /// the session was open. + async fn upload_file_streaming_with_perms( + &self, + name: String, + folder_id: Option, + content_type: String, + blob: StoredBlob, + caller_id: Uuid, + ) -> Result; + /// Replace the content of the file at `path` with an already-ingested /// blob, or create the file when it doesn't exist (WebDAV/WOPI PUT). /// diff --git a/src/application/services/file_upload_service.rs b/src/application/services/file_upload_service.rs index f3eb48f7..f73d1b76 100644 --- a/src/application/services/file_upload_service.rs +++ b/src/application/services/file_upload_service.rs @@ -457,6 +457,44 @@ impl FileUploadUseCase for FileUploadService { Ok(dto) } + /// AuthZ audit #17 — `Create` on target folder is re-verified here + /// so mid-session grant revocations take effect at finalize. When + /// `folder_id` is `None` the write lands at drive-root; the drive + /// resolution for that case isn't plumbed through the chunked- + /// upload session (`UploadSession.folder_id` alone), so we fall + /// back to the pre-audit behaviour there. That drive-root path is + /// tracked separately as part of the D0 folder-id-walking work; + /// closing it here would require session-scoped drive_id. + async fn upload_file_streaming_with_perms( + &self, + name: String, + folder_id: Option, + content_type: String, + blob: StoredBlob, + caller_id: Uuid, + ) -> Result { + if let Some(fid) = folder_id.as_deref() { + let Some(authz) = &self.authorization else { + return Err(DomainError::internal_error( + "FileUpload", + "upload_file_streaming_with_perms called without authorization engine wired", + )); + }; + let folder_uuid = Uuid::parse_str(fid) + .map_err(|_| DomainError::not_found("Folder", fid.to_string()))?; + authz + .require( + Subject::User(caller_id), + Permission::Create, + Resource::Folder(folder_uuid), + ) + .await?; + } + + self.upload_file_streaming(name, folder_id, content_type, blob, caller_id) + .await + } + /// Swap the content of the file at `path` to an already-ingested blob, /// creating the file when it doesn't exist (WebDAV/NextCloud/WOPI PUT). /// diff --git a/src/common/stubs.rs b/src/common/stubs.rs index 4d72d9d4..2d028f43 100644 --- a/src/common/stubs.rs +++ b/src/common/stubs.rs @@ -511,6 +511,17 @@ impl FileUploadUseCase for StubFileUploadUseCase { ) -> Result { Ok(FileDto::default()) } + + async fn upload_file_streaming_with_perms( + &self, + _name: String, + _folder_id: Option, + _content_type: String, + _blob: StoredBlob, + _caller_id: Uuid, + ) -> Result { + Ok(FileDto::default()) + } } // --------------------------------------------------------------------------- diff --git a/src/interfaces/api/handlers/chunked_upload_handler.rs b/src/interfaces/api/handlers/chunked_upload_handler.rs index ea339cc1..630c2a57 100644 --- a/src/interfaces/api/handlers/chunked_upload_handler.rs +++ b/src/interfaces/api/handlers/chunked_upload_handler.rs @@ -188,9 +188,14 @@ impl ChunkedUploadHandler { // ── Permission pre-check: caller must have Create on the target // folder BEFORE we allocate a session and accept chunks. The - // upload service re-checks at finalize time, but failing here - // avoids wasting client+server resources on chunks that will be - // rejected. None = caller's root namespace, no check needed. + // upload service re-checks at finalize via + // `upload_file_streaming_with_perms` (AuthZ audit #17 fix, + // 2026-07-16) so a grant revoked mid-session is caught. This + // pre-check is the fail-fast: it avoids wasting client+server + // resources on chunks that will be rejected anyway. `None` + // means the write lands at drive-root — that path is currently + // unchecked (session doesn't carry `drive_id`; tracked with the + // folder-id-walking follow-up). if let Some(ref fid) = request.folder_id && let Err(err) = state .applications @@ -441,9 +446,17 @@ impl ChunkedUploadHandler { } // Register the file row against the ingested blob. + // + // AuthZ audit #17 (2026-07-12): swapped `upload_file_streaming` → + // `upload_file_streaming_with_perms` so `Create` on the target + // folder is re-verified at finalize. Session creation already + // pre-checked (line ~198), but that was potentially hours or + // days ago; app-passwords keep sessions valid indefinitely. + // Without the finalize re-check, a grant revoked mid-session + // stayed effective until the last chunk landed. let size = ingested.size; match upload_service - .upload_file_streaming( + .upload_file_streaming_with_perms( parts.filename.clone(), parts.folder_id.clone(), ingested.content_type.clone(), diff --git a/tests/api/grants.hurl b/tests/api/grants.hurl index d0affa9f..a292b7fe 100644 --- a/tests/api/grants.hurl +++ b/tests/api/grants.hurl @@ -818,6 +818,96 @@ Authorization: Bearer {{adam_token}} HTTP 204 +# ── Regression pin for AuthZ audit #17 (2026-07-12). ───────── +# The chunked-upload `complete` handler used to call plain +# `upload_file_streaming` at finalize — no `_with_perms` check. +# A grant revoked between session-open and finalize stayed +# effective until the last chunk landed (up to 24h JWT TTL, +# forever with app-passwords). Fix: swap to +# `upload_file_streaming_with_perms` so `authz.require(Create, +# Folder)` re-runs at complete time. +# +# Sequence: +# 1. Adam (Editor) opens a session — pre-check passes. +# 2. Adam PATCHes the single chunk (chunk upload is unauth'd, +# always allowed). +# 3. Alice DEMOTES Adam to Viewer (Viewer bundle has Read but +# no Create). +# 4. Adam POST /complete → 403 (pre-fix: 201 + file created). +# 5. Cleanup: cancel the orphaned session + re-promote Adam +# to Editor so the following steps aren't disturbed. + +# 1 — Open session while Editor. +POST {{base_url}}/api/uploads +Authorization: Bearer {{adam_token}} +Content-Type: application/json +{ + "filename": "audit17-post-revoke.mp4", + "folder_id": "{{perm_folder_id}}", + "content_type": "video/mp4", + "total_size": 2760653, + "chunk_size": 3000000 +} + +HTTP 201 +[Captures] +audit17_upload_id: jsonpath "$.upload_id" + + +# 2 — Send the single chunk (session pre-authorised). +PATCH {{base_url}}/api/uploads/{{audit17_upload_id}}?chunk_index=0 +Authorization: Bearer {{adam_token}} +Content-Type: application/octet-stream +file,fixtures/free_video_over_1MB.mp4; + +HTTP 200 + + +# 3 — Alice demotes Adam Editor → Viewer (Create removed). +PUT {{base_url}}/api/grants/role +Authorization: Bearer {{alice_token}} +Content-Type: application/json +{ + "subject": { "type": "user", "id": "{{adam_user_id}}" }, + "resource": { "type": "folder", "id": "{{perm_folder_id}}" }, + "role": "viewer" +} + +HTTP 200 + + +# 4 — Finalize now fails: engine re-checks Create at complete +# time. Adam still has Read (viewer role) → graduated denial +# returns 403; pre-fix returned 201 with a phantom file. +POST {{base_url}}/api/uploads/{{audit17_upload_id}}/complete +Authorization: Bearer {{adam_token}} + +HTTP 403 + + +# 5a — The session is orphaned (chunks on disk, no completion). +# Cancel it as Adam (still owns the session, so the `_with_perms` +# gate on DELETE-session lets him through). +DELETE {{base_url}}/api/uploads/{{audit17_upload_id}} +Authorization: Bearer {{adam_token}} + +HTTP 204 + + +# 5b — Restore Adam to Editor so subsequent steps behave as +# before this regression pin was inserted. +PUT {{base_url}}/api/grants/role +Authorization: Bearer {{alice_token}} +Content-Type: application/json +{ + "subject": { "type": "user", "id": "{{adam_user_id}}" }, + "resource": { "type": "folder", "id": "{{perm_folder_id}}" }, + "role": "editor" +} + +HTTP 200 + + # ── Delete still denied (Editor excludes Delete). Editor has # Read → graduated denial returns 403. DELETE {{base_url}}/api/files/{{perm_file_id}}