From 07c36d24a2f3ac5bd7eb78e221aef1f0dd46494f Mon Sep 17 00:00:00 2001 From: Jacob Hagstedt Date: Sat, 24 Oct 2026 11:03:19 +0200 Subject: [PATCH] L-10: E-16 answers not_found for another user's device (D-156) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A member naming the other user's device now gets the same 404 not_found body as an unknown id, so E-16 no longer reveals whose a device id is. The admin keeps revoking any device except the current one (AC-3.2, §8.4). contracts/openapi.yaml drops the 403 on E-16 and states the rule; web/src/api/schema.d.ts regenerated. The forbidden test becomes a not_found test, plus one asserting the body equals the unknown-id body and that nothing was revoked. Also bumps ctutils 0.4.3 -> 0.4.4 (lockfile only) for RUSTSEC-2026-0385, published 2026-10-23, which failed cargo audit on main. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TQd3pEP9UeN2uifbHknQ7B --- contracts/openapi.yaml | 5 +-- server/Cargo.lock | 4 +- server/crates/kvitt-server/src/api/devices.rs | 11 +++-- .../kvitt-server/tests/core_api/devices.rs | 42 +++++++++++++++++-- web/src/api/schema.d.ts | 4 +- 5 files changed, 48 insertions(+), 18 deletions(-) diff --git a/contracts/openapi.yaml b/contracts/openapi.yaml index 00049d1..e1d3c52 100644 --- a/contracts/openapi.yaml +++ b/contracts/openapi.yaml @@ -626,7 +626,7 @@ paths: delete: operationId: revokeDevice summary: E-16 Revoke a device - description: Revoking an already revoked device is 204. Admin any device except the current one; member own devices except the current one (SPEC §8.4). + description: Revoking an already revoked device is 204. Admin any device except the current one; member own devices except the current one, and a device of the other user is 404 `not_found`, the same answer as an unknown id (SPEC §8.4, D-156). parameters: - $ref: '#/components/parameters/DeviceId' - $ref: '#/components/parameters/Protocol' @@ -641,9 +641,6 @@ paths: $ref: '#/components/headers/ServerVersion' '401': $ref: '#/components/responses/Unauthorized' - '403': - $ref: '#/components/responses/Forbidden' - description: '`forbidden`.' '404': $ref: '#/components/responses/NotFound' description: '`not_found`.' diff --git a/server/Cargo.lock b/server/Cargo.lock index b9b5650..c9f02fc 100644 --- a/server/Cargo.lock +++ b/server/Cargo.lock @@ -496,9 +496,9 @@ dependencies = [ [[package]] name = "ctutils" -version = "0.4.3" +version = "0.4.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "03bb0e1cc970d482d121d9a1744999169b69a07470b3d644a7894e53fcaf4574" +checksum = "12218662ccac37d9714f2191252553d67585b97df8c839cbe49e0bde278dc865" dependencies = [ "cmov", ] diff --git a/server/crates/kvitt-server/src/api/devices.rs b/server/crates/kvitt-server/src/api/devices.rs index 30cf515..23bdafe 100644 --- a/server/crates/kvitt-server/src/api/devices.rs +++ b/server/crates/kvitt-server/src/api/devices.rs @@ -11,7 +11,7 @@ use crate::api::{ApiJson, IdPath, JsonBody, rows}; use crate::auth::AuthDevice; use crate::db; use crate::error::{AppError, ErrorCode}; -use crate::openapi::responses::{BadRequest, Conflict, Forbidden, NotFound}; +use crate::openapi::responses::{BadRequest, Conflict, NotFound}; use crate::wire::{DeviceList, PushTokenRequest, Role, UuidSchema}; /// Longest `fcm_token` stored, in characters after trimming (D-114; a Firebase Installation ID has 22). @@ -41,13 +41,12 @@ pub async fn list_devices( path = "/devices/{device_id}", operation_id = "revokeDevice", summary = "E-16 Revoke a device", - description = "Revoking an already revoked device is 204. Admin any device except the current one; member own devices except the current one (SPEC §8.4).", + description = "Revoking an already revoked device is 204. Admin any device except the current one; member own devices except the current one, and a device of the other user is 404 `not_found`, the same answer as an unknown id (SPEC §8.4, D-156).", params( ("device_id" = UuidSchema, Path, description = "Not a UUID is 404 `not_found`."), ), responses( (status = 204, description = "Revoked."), - (status = 403, response = Forbidden), (status = 404, response = NotFound), (status = 409, response = Conflict), ) @@ -58,12 +57,12 @@ pub async fn revoke_device( IdPath(device_id): IdPath, ) -> Result { let mut tx = db::begin_immediate(&state.db).await?; + // D-156 (security review L-10): for a member, a device of the other user is the same `not_found` as an unknown + // id, so the answer does not tell whether the id belongs to someone. let target = rows::device(&mut tx, device_id, device.device_id) .await? + .filter(|target| device.role == Role::Admin || target.user_id == device.user_id) .ok_or_else(AppError::not_found)?; - if device.role != Role::Admin && target.user_id != device.user_id { - return Err(AppError::forbidden()); - } if target.is_current { return Err(AppError::new( ErrorCode::CannotRevokeCurrentDevice, diff --git a/server/crates/kvitt-server/tests/core_api/devices.rs b/server/crates/kvitt-server/tests/core_api/devices.rs index d35a3a5..8c6b2b5 100644 --- a/server/crates/kvitt-server/tests/core_api/devices.rs +++ b/server/crates/kvitt-server/tests/core_api/devices.rs @@ -77,9 +77,9 @@ async fn e16_admin_revokes_another_device_which_then_gets_device_revoked() { } #[tokio::test] -async fn e16_forbidden_not_found_and_cannot_revoke_current_device() { +async fn e16_not_found_and_cannot_revoke_current_device() { let app = TestApp::household().await; - // A member may revoke only their own devices. + // A member may revoke only their own devices; another user's device is not_found (D-156). let reply = app .call( Method::DELETE, @@ -88,7 +88,7 @@ async fn e16_forbidden_not_found_and_cannot_revoke_current_device() { None, ) .await; - assert_error(&reply, 403, "forbidden"); + assert_error(&reply, 404, "not_found"); let reply = app .call( Method::DELETE, @@ -109,6 +109,42 @@ async fn e16_forbidden_not_found_and_cannot_revoke_current_device() { } } +#[tokio::test] +async fn e16_member_naming_the_other_users_device_gets_the_unknown_id_answer() { + // D-156 (security review L-10): the answer does not tell whether the id belongs to a device. + let app = TestApp::household().await; + let unknown = app + .call( + Method::DELETE, + &format!("/devices/{UNKNOWN_ID}"), + &member_token(), + None, + ) + .await; + assert_error(&unknown, 404, "not_found"); + for id in [ADMIN_DEVICE_ID, ADMIN_OLD_DEVICE_ID] { + let reply = app + .call( + Method::DELETE, + &format!("/devices/{id}"), + &member_token(), + None, + ) + .await; + assert_eq!(reply.status, unknown.status); + assert_eq!(reply.json, unknown.json); + } + // Nothing was revoked: the admin's current device still works. + let me = app.call(Method::GET, "/me", &admin_token(), None).await; + assert_eq!(me.status, 200); + let revoked: Option = sqlx::query_scalar("SELECT revoked_at FROM devices WHERE id = ?") + .bind(ADMIN_DEVICE_ID) + .fetch_one(&app.state.db) + .await + .unwrap(); + assert_eq!(revoked, None); +} + #[tokio::test] async fn e16_member_revokes_own_other_device() { let app = TestApp::household().await; diff --git a/web/src/api/schema.d.ts b/web/src/api/schema.d.ts index 7e4a98c..94d37f9 100644 --- a/web/src/api/schema.d.ts +++ b/web/src/api/schema.d.ts @@ -239,7 +239,7 @@ export interface paths { post?: never; /** * E-16 Revoke a device - * @description Revoking an already revoked device is 204. Admin any device except the current one; member own devices except the current one (SPEC §8.4). + * @description Revoking an already revoked device is 204. Admin any device except the current one; member own devices except the current one, and a device of the other user is 404 `not_found`, the same answer as an unknown id (SPEC §8.4, D-156). */ delete: operations["revokeDevice"]; options?: never; @@ -1887,8 +1887,6 @@ export interface operations { content?: never; }; 401: components["responses"]["Unauthorized"]; - /** @description `forbidden`. */ - 403: components["responses"]["Forbidden"]; /** @description `not_found`. */ 404: components["responses"]["NotFound"]; /** @description `cannot_revoke_current_device`. */