From 86d168ea2e4c95b8f613b8c4589a04e36d69572f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 00:29:27 +0000 Subject: [PATCH] Admin users: a PATCH applies all of its fields or none `PATCH /admin/users/{id}` committed a service account's `description` and `realm` in one transaction and then applied `active` in another: `{"active": false, "realm": "lb"}` left the account active in its new realm for a moment, and a failure in the second step left the first applied. The request is now checked first (the account is a service account, the description and realm are accepted) and then applied in one transaction: the description, the realm, and `active` under the last-admin guard. `find_user_by_id` takes any executor, so the unchanged account is read in that transaction too. The test makes the deactivation fail in the database and checks that nothing else was applied (on the old code the account kept its new realm). The service account page sent `realm` with every save, so once the realm was renamed or deleted after the page loaded, saving a new description alone failed with 400. It now sends only the fields that changed; the fake admin API of the web tests refuses an unknown realm like the server. `parse_description` refused control characters (Cc) but accepted format characters (Cf) such as U+202E, which made a description read otherwise in the admin list than it is. It now refuses them too, all but the zero-width non-joiner and joiner that emoji and several scripts need. Fixes #585 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012e6DJZaXD2mBH9tURsdutM --- crates/db/src/user_dao.rs | 7 +- crates/server/src/admin/users.rs | 228 +++++++++++++----- crates/server/tests/service_accounts.rs | 79 ++++++ docs/changes/2.2.0/585.fixed.md | 11 + docs/deployment.md | 8 +- .../routes/admin/service-accounts/Edit.tsx | 13 +- .../service-accounts/serviceAccounts.test.tsx | 29 ++- web/src/test/fakeRbacApi.ts | 8 + 8 files changed, 307 insertions(+), 76 deletions(-) create mode 100644 docs/changes/2.2.0/585.fixed.md diff --git a/crates/db/src/user_dao.rs b/crates/db/src/user_dao.rs index 4de70e30..e3b894d6 100644 --- a/crates/db/src/user_dao.rs +++ b/crates/db/src/user_dao.rs @@ -399,7 +399,10 @@ pub async fn find_user_by_username(pool: &PgPool, username: &str) -> sqlx::Resul } /// Find a user by id. -pub async fn find_user_by_id(pool: &PgPool, id: i64) -> sqlx::Result> { +pub async fn find_user_by_id<'e, E>(executor: E, id: i64) -> sqlx::Result> +where + E: sqlx::PgExecutor<'e>, +{ sqlx::query_as!( AppUser, r#" @@ -411,7 +414,7 @@ pub async fn find_user_by_id(pool: &PgPool, id: i64) -> sqlx::Result) -> Result, Response> { let Some(description) = description.map(str::trim).filter(|d| !d.is_empty()) else { return Ok(None); @@ -242,9 +245,52 @@ fn parse_description(description: Option<&str>) -> Result, Respon ) .into_response()); } + if let Some(c) = description.chars().find(|&c| is_format_character(c)) { + return Err(( + StatusCode::BAD_REQUEST, + format!( + "description must not contain invisible formatting characters such as U+{:04X}", + c as u32 + ), + ) + .into_response()); + } Ok(Some(description.to_string())) } +/// A Unicode format character (general category `Cf`): the bidirectional +/// embeddings, overrides and isolates (U+202E reverses what follows it), +/// zero-width and other invisible characters, and tags. Not the zero-width +/// non-joiner and joiner (U+200C, U+200D), which emoji and several scripts +/// need. +fn is_format_character(c: char) -> bool { + matches!( + c as u32, + 0x00AD + | 0x0600..=0x0605 + | 0x061C + | 0x06DD + | 0x070F + | 0x0890..=0x0891 + | 0x08E2 + | 0x180E + | 0x200B + | 0x200E..=0x200F + | 0x202A..=0x202E + | 0x2060..=0x2064 + | 0x2066..=0x206F + | 0xFEFF + | 0xFFF9..=0xFFFB + | 0x110BD + | 0x110CD + | 0x13430..=0x1343F + | 0x1BCA0..=0x1BCA3 + | 0x1D173..=0x1D17A + | 0xE0001 + | 0xE0020..=0xE007F + ) +} + /// The id of the trusted-header realm named `name`, which a service account /// may belong to, or the **400** that refuses the name. async fn service_account_realm(state: &AppState, name: &str) -> Result { @@ -788,9 +834,10 @@ pub async fn revoke_invitation(State(state): State, Path(id): Path, Path(id): Path, @@ -804,28 +851,27 @@ pub async fn update( ) .into_response(); } - if body.description.is_some() || body.realm.is_some() { - if let Err(resp) = update_service_account(&state, id, &body).await { - return resp; + let changes = if body.description.is_some() || body.realm.is_some() { + match service_account_changes(&state, id, &body).await { + Ok(changes) => Some(changes), + Err(resp) => return resp, } - } - let Some(active) = body.active else { - forge_core::authz::cache::invalidate_user(&state.cache, id).await; - return match user_dao::find_user_by_id(&state.pool, id).await { - Ok(Some(user)) => match UserView::load(&state, &user).await { - Ok(view) => Json(view).into_response(), - Err(resp) => resp, - }, - Ok(None) => (StatusCode::NOT_FOUND, "user not found").into_response(), - Err(e) => db_error(e), - }; + } else { + None }; - let user = match set_active_guarded(&state, id, active).await { + let user = match apply_update(&state, id, changes.as_ref(), body.active).await { Ok(Some(u)) => u, Ok(None) => return (StatusCode::NOT_FOUND, "user not found").into_response(), Err(resp) => return resp, }; - if !active { + if changes.is_some() { + tracing::info!( + user_id = user.id, + username = %user.username, + "service account updated" + ); + } + if body.active == Some(false) { if let Err(resp) = revoke_sessions_and_verdicts(&state, &user).await { return resp; } @@ -843,14 +889,21 @@ pub async fn update( } } -/// Set the `description` and `realm` a PATCH carries on the service account -/// `id`, in one transaction: **404** for no account, **409** for a user's, -/// **400** for a description or realm that is refused. -async fn update_service_account( +/// The `description` and `realm` a PATCH sets on a service account, each +/// `None` when the request leaves it as it is. +struct ServiceAccountChanges { + description: Option>, + realm_id: Option>, +} + +/// The `description` and `realm` a PATCH carries for the service account +/// `id`, checked: **404** for no account, **409** for a user's, **400** for +/// a description or realm that is refused. +async fn service_account_changes( state: &AppState, id: i64, body: &UpdateUser, -) -> Result<(), Response> { +) -> Result { let user = match user_dao::find_user_by_id(&state.pool, id).await { Ok(Some(u)) => u, Ok(None) => return Err((StatusCode::NOT_FOUND, "user not found").into_response()), @@ -873,24 +926,54 @@ async fn update_service_account( Some(None) => Some(None), None => None, }; + Ok(ServiceAccountChanges { + description, + realm_id, + }) +} + +/// Apply a PATCH to the account `id` in one transaction: a service +/// account's `changes`, then `active` under the last-admin guard (only a +/// deactivation can remove an admin). `Ok(None)` when no such account +/// exists. +async fn apply_update( + state: &AppState, + id: i64, + changes: Option<&ServiceAccountChanges>, + active: Option, +) -> Result, Response> { let mut tx = forge_db::begin(&state.pool).await.map_err(db_error)?; - if let Some(description) = &description { - user_dao::set_service_account_description(&mut *tx, id, description.as_deref()) - .await - .map_err(db_error)?; + if let Some(changes) = changes { + if let Some(description) = &changes.description { + user_dao::set_service_account_description(&mut *tx, id, description.as_deref()) + .await + .map_err(db_error)?; + } + if let Some(realm_id) = changes.realm_id { + user_dao::set_service_account_realm(&mut *tx, id, realm_id) + .await + .map_err(db_error)?; + } } - if let Some(realm_id) = realm_id { - user_dao::set_service_account_realm(&mut *tx, id, realm_id) + let user = match active { + Some(active) => { + if !active + && authz_dao::would_leave_no_admin(&mut tx, id) + .await + .map_err(db_error)? + { + return Err(last_admin_conflict()); + } + user_dao::set_active(&mut *tx, id, active) + .await + .map_err(db_error)? + } + None => user_dao::find_user_by_id(&mut *tx, id) .await - .map_err(db_error)?; - } + .map_err(db_error)?, + }; tx.commit().await.map_err(db_error)?; - tracing::info!( - user_id = id, - username = %user.username, - "service account updated" - ); - Ok(()) + Ok(user) } /// `DELETE /admin/users/{id}` — delete the user (sessions, tokens, role @@ -1034,28 +1117,6 @@ pub(crate) async fn purge_converted( Ok(()) } -/// Set `active` in a transaction under the last-admin guard (only a -/// deactivation can remove an admin). `Ok(None)` when no such user exists. -async fn set_active_guarded( - state: &AppState, - id: i64, - active: bool, -) -> Result, Response> { - let mut tx = forge_db::begin(&state.pool).await.map_err(db_error)?; - if !active - && authz_dao::would_leave_no_admin(&mut tx, id) - .await - .map_err(db_error)? - { - return Err(last_admin_conflict()); - } - let user = user_dao::set_active(&mut *tx, id, active) - .await - .map_err(db_error)?; - tx.commit().await.map_err(db_error)?; - Ok(user) -} - /// Delete the user in a transaction under the last-admin guard. `Ok(false)` /// when no such user exists. async fn delete_guarded(state: &AppState, id: i64) -> Result { @@ -1094,3 +1155,48 @@ async fn revoke_sessions_and_verdicts(state: &AppState, user: &AppUser) -> Resul fn is_unique_violation(e: &sqlx::Error) -> bool { matches!(e, sqlx::Error::Database(db) if db.code().as_deref() == Some("23505")) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn descriptions_refuse_what_reads_otherwise_than_it_is() { + let parsed = |d: &str| parse_description(Some(d)).ok().flatten(); + assert_eq!( + parsed(" CI of the web team ").as_deref(), + Some("CI of the web team") + ); + assert_eq!( + parsed("owner:\n\tplatform").as_deref(), + Some("owner:\n\tplatform") + ); + // Emoji and scripts keep their joiners. + assert!(parsed("runners \u{1F468}\u{200D}\u{1F4BB}").is_some()); + assert!( + parsed("\u{0645}\u{06CC}\u{200C}\u{062E}\u{0648}\u{0627}\u{0647}\u{0645}").is_some() + ); + assert_eq!(parse_description(Some(" ")).ok(), Some(None)); + for c in [ + '\u{7}', + '\u{202E}', + '\u{202D}', + '\u{2066}', + '\u{2069}', + '\u{200E}', + '\u{200F}', + '\u{061C}', + '\u{200B}', + '\u{2060}', + '\u{FEFF}', + '\u{00AD}', + '\u{E0041}', + ] { + assert!( + parse_description(Some(&format!("build {c}agent"))).is_err(), + "U+{:04X}", + c as u32 + ); + } + } +} diff --git a/crates/server/tests/service_accounts.rs b/crates/server/tests/service_accounts.rs index 55d8c015..c052a1bf 100644 --- a/crates/server/tests/service_accounts.rs +++ b/crates/server/tests/service_accounts.rs @@ -300,6 +300,85 @@ async fn the_admin_api_creates_lists_and_updates_service_accounts() { assert_eq!(view["roles"], json!(["writer"])); } +/// A PATCH applies its fields in one transaction (#585): when deactivating +/// fails, the description and realm it carries are not applied either; when +/// it succeeds, the account is inactive in its new realm at once. A +/// description that would read otherwise than it is (a right-to-left +/// override, a zero-width space) is refused, on create and on update. +#[tokio::test] +async fn a_patch_applies_all_of_its_fields_or_none() { + let srv = server().await; + let realm = create_realm(&srv, false).await; + let name = format!("lb{}@example.com", common::unique()); + let id = create_service_account(&srv, &name, None).await["id"] + .as_i64() + .unwrap(); + + // The database refuses the deactivation, after the realm and the + // description were written in the same request. + sqlx::raw_sql(sqlx::AssertSqlSafe(format!( + "CREATE FUNCTION refuse_deactivation() RETURNS trigger LANGUAGE plpgsql AS $$ \ + BEGIN IF NEW.id = {id} AND NOT NEW.active THEN RAISE EXCEPTION 'refused'; END IF; \ + RETURN NEW; END $$; \ + CREATE TRIGGER refuse_deactivation BEFORE UPDATE ON app_user \ + FOR EACH ROW EXECUTE FUNCTION refuse_deactivation();" + ))) + .execute(&srv.pool) + .await + .unwrap(); + let (status, _) = patch( + &srv, + id, + json!({"active": false, "description": "the load balancer", "realm": realm.name}), + ) + .await; + assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR); + let view = |users: Vec| users.into_iter().find(|u| u["id"] == json!(id)).unwrap(); + let account = view(list(&srv, "?kind=service").await); + assert_eq!(account["active"], json!(true), "{account}"); + assert_eq!(account["realm"], Value::Null, "{account}"); + assert_eq!( + account["description"], + json!("The build agents; owner: platform"), + "{account}" + ); + sqlx::raw_sql( + "DROP TRIGGER refuse_deactivation ON app_user; DROP FUNCTION refuse_deactivation();", + ) + .execute(&srv.pool) + .await + .unwrap(); + + let (status, body) = patch( + &srv, + id, + json!({"active": false, "description": "the load balancer", "realm": realm.name}), + ) + .await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body["active"], json!(false)); + assert_eq!(body["realm"], json!(realm.name)); + assert_eq!(body["description"], json!("the load balancer")); + + for description in ["admin \u{202E}lanigiro", "build\u{200B}agents"] { + let (status, body) = patch(&srv, id, json!({"description": description})).await; + assert_eq!(status, StatusCode::BAD_REQUEST, "{body}"); + assert!(body.as_str().unwrap().contains("formatting"), "{body}"); + let (status, body) = create( + &srv, + json!({ + "username": format!("svc{}@example.com", common::unique()), + "kind": "service", + "description": description, + }), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST, "{body}"); + } + let account = view(list(&srv, "?kind=service").await); + assert_eq!(account["description"], json!("the load balancer")); +} + #[tokio::test] async fn nobody_signs_in_to_a_service_account_but_its_tokens_work() { let srv = server().await; diff --git a/docs/changes/2.2.0/585.fixed.md b/docs/changes/2.2.0/585.fixed.md new file mode 100644 index 00000000..913006de --- /dev/null +++ b/docs/changes/2.2.0/585.fixed.md @@ -0,0 +1,11 @@ +`PATCH /admin/users/{id}` applied a service account's `description` and +`realm` in one transaction and `active` in another: `{"active": false, +"realm": "lb"}` left the account active in its new realm for a moment, and +a failure in the second step left the first applied. All of a request's +fields are now applied in one transaction, or none. The service account +page sent the realm with every save, so once the realm had been renamed or +deleted since the page loaded, saving a new description alone failed with +400; it now sends only what was changed. A description is also refused +when it holds an invisible formatting character, such as a right-to-left +override (U+202E) that made it read otherwise in the admin list than it +is; zero-width joiners, which emoji and some scripts need, stay allowed. diff --git a/docs/deployment.md b/docs/deployment.md index e3861109..cb4c0a51 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -507,8 +507,12 @@ or `writer`), and the optional `realm` (the name of a trusted-header realm). `GET /admin/users?kind=service` lists the service accounts; every user view carries `kind`, `description`, `realm` and `last_used_at`. `PATCH /admin/users/{id}` sets a service account's `description` and `realm` -(`null` clears either) besides `active`. Tokens are -`POST /admin/users/{id}/tokens`, roles `PUT /admin/users/{id}/roles`. +(`null` clears either) besides `active`, all of them in one transaction. +A description holds at most 1,000 characters, and no control character but +a newline or a tab, nor an invisible formatting character such as a +right-to-left override (U+202E), with which it would read otherwise than it +is. Tokens are `POST /admin/users/{id}/tokens`, roles `PUT +/admin/users/{id}/roles`. ### Usernames diff --git a/web/src/routes/admin/service-accounts/Edit.tsx b/web/src/routes/admin/service-accounts/Edit.tsx index b7d710f6..1f722a90 100644 --- a/web/src/routes/admin/service-accounts/Edit.tsx +++ b/web/src/routes/admin/service-accounts/Edit.tsx @@ -102,7 +102,7 @@ function ServiceAccountPage() { /** * The account's description and trusted-header realm (`PATCH - * /admin/users/{id}`), and its state: deactivating stops its tokens and its + * /admin/users/{id}`, with the fields that changed), and its state: deactivating stops its tokens and its * realm from authenticating it (the tokens are suspended, not revoked); * reactivating lets both work again. Each after a confirmation. */ @@ -116,18 +116,21 @@ function ServiceAccountSettings({ account }: { account: UserView }) { const [realm, setRealm] = useState(account.realm ?? '') const [confirming, setConfirming] = useState<'deactivate' | 'reactivate' | null>(null) const [done, setDone] = useState(null) - const changed = - description.trim() !== (account.description ?? '') || realm !== (account.realm ?? '') + const descriptionChanged = description.trim() !== (account.description ?? '') + const realmChanged = realm !== (account.realm ?? '') + const changed = descriptionChanged || realmChanged const liveTokens = tokens.data?.filter((t) => tokenStatus(t) === 'active').length + // Only what was changed is sent: a realm renamed or deleted since the page + // loaded would refuse the save of a new description. function save(e: React.FormEvent) { e.preventDefault() setDone(null) update.mutate( { id: account.id, - description: description.trim() || null, - realm: realm || null, + ...(descriptionChanged && { description: description.trim() || null }), + ...(realmChanged && { realm: realm || null }), }, { onSuccess: () => setDone(`Saved ${account.username}.`) }, ) diff --git a/web/src/routes/admin/service-accounts/serviceAccounts.test.tsx b/web/src/routes/admin/service-accounts/serviceAccounts.test.tsx index b67b8751..bf197539 100644 --- a/web/src/routes/admin/service-accounts/serviceAccounts.test.tsx +++ b/web/src/routes/admin/service-accounts/serviceAccounts.test.tsx @@ -14,7 +14,8 @@ import { renderApp } from '~/test/renderApp' * them out; * - the create page POSTs `kind: 'service'` with the description, realm * and access level, offers no admin level and takes no email; - * - the account page saves its description and realm, never offers the + * - the account page saves what changed of its description and realm (a + * realm deleted since is not sent again), never offers the * admin role or level, issues a token whose secret shows once, and * deactivates it after a confirmation. */ @@ -159,14 +160,11 @@ describe('admin service accounts', () => { expect(await within(settings).findByText('Saved ci@example.com.')).toBeInTheDocument() await waitFor(() => expect(screen.getByText(/^Realm:/)).toHaveTextContent('edge-lb')) - // Back to no realm: the realm is sent as null. + // Back to no realm: the realm alone is sent, as null. await u.selectOptions(within(settings).getByLabelText('Trusted-header realm'), '') await u.click(within(settings).getByRole('button', { name: 'Save' })) await waitFor(() => - expect(api.sent('PATCH', /^\/admin\/users\/4$/)[1]?.body).toEqual({ - description: 'CI of the web team', - realm: null, - }), + expect(api.sent('PATCH', /^\/admin\/users\/4$/)[1]?.body).toEqual({ realm: null }), ) // The access level has no admin, and neither do the roles to assign. @@ -182,6 +180,25 @@ describe('admin service accounts', () => { expect(within(assign).queryByRole('option', { name: 'admin' })).toBeNull() }) + it('saves a description alone once its realm is gone, without re-sending the realm', async () => { + // The account's realm was deleted (or renamed) since it was assigned. + api.state.realms = [] + const u = userEvent.setup() + await renderApp('/manage/service-accounts/3') + + const settings = await screen.findByRole('form', { name: 'Service account' }) + const field = within(settings).getByLabelText(/Description/) + await u.clear(field) + await u.type(field, 'The office network') + await u.click(within(settings).getByRole('button', { name: 'Save' })) + await waitFor(() => + expect(api.sent('PATCH', /^\/admin\/users\/3$/)[0]?.body).toEqual({ + description: 'The office network', + }), + ) + expect(await within(settings).findByText('Saved office-lan@example.com.')).toBeInTheDocument() + }) + it('issues a token for a service account, showing the secret once', async () => { const u = userEvent.setup() await renderApp('/manage/service-accounts/4') diff --git a/web/src/test/fakeRbacApi.ts b/web/src/test/fakeRbacApi.ts index 4ee42c78..b8dcf4ab 100644 --- a/web/src/test/fakeRbacApi.ts +++ b/web/src/test/fakeRbacApi.ts @@ -367,6 +367,14 @@ export class FakeRbacApi { u.active = body.active u.status = body.active ? 'active' : 'inactive' } + if ( + typeof body.realm === 'string' && + !(s.realms as { kind?: string; name?: string }[]).some( + (r) => r.kind === 'trusted_header' && r.name === body.realm, + ) + ) { + return text(`no trusted-header realm is named "${body.realm}"`, 400) + } if (body.description !== undefined) u.description = body.description if (body.realm !== undefined) u.realm = body.realm return json(this.userView(u))