Skip to main content

max / makenotwork

server: refuse git over HTTPS for suspended and deactivated accounts resolve_git_http_principal authenticated a personal access token on its hash and expiry alone, and never loaded the user, so a token minted before a suspension kept working until it expired or was deleted by hand. The session-cookie branch was unchecked on the same terms. SSH already refuses both states in git_ssh::dispatch, so one account was locked out of one transport and served by the other. The check goes in the funnel rather than in authorize_push, so it covers reads as well as pushes, and rather than in git_access_tokens::resolve_active, so it covers the cookie branch that query never sees. Reads are refused alongside pushes: SSH already answers it that way, and allowing HTTPS clones would leave the two transports disagreeing, which is the bug. Getting data out of a suspended account is a separate question. Drops the redundant check in the notes API write path, which reached for the raw timestamp fields; the principal is known good by the time it runs.
Co-Authored-By
Claude Opus 5 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-08-09 13:39 UTC
Signed with PGP, not checked
Commit: ac5d54a6ceb747136b99ed32106edc08bbd71e7d
Parent: e1063b5
3 files changed, +177 insertions, -21 deletions
M server/tests/workflows/git_browser.rs +147
@@ -993,6 +993,153 @@
993 993 );
994 994 }
995 995
996 + // Suspension has to mean the same thing over HTTPS that it means over SSH.
997 + // `git_ssh::dispatch` loads the user and refuses a suspended or deactivated
998 + // account; the HTTPS funnel used to authenticate a token on its hash and expiry
999 + // alone, so a token minted before a suspension kept working. Both credential
1000 + // branches are covered here: the PAT and the session cookie.
1001 + #[tokio::test]
1002 + async fn suspended_account_loses_git_over_https() {
1003 + let tmp = tempfile::TempDir::new().unwrap();
1004 + make_test_repo(tmp.path());
1005 + let mut h = setup_git_harness(&tmp).await;
1006 +
1007 + h.client.get("/git/testowner/testrepo").await; // auto-register
1008 + sqlx::query("UPDATE git_repos SET visibility = 'private' WHERE name = 'testrepo'")
1009 + .execute(&h.db)
1010 + .await
1011 + .unwrap();
1012 +
1013 + h.login("testowner", "password123").await;
1014 + h.client.fetch_csrf_token().await;
1015 + let resp = h
1016 + .client
1017 + .post_form("/api/users/me/git-tokens", "name=ci&can_push=on")
1018 + .await;
1019 + assert_eq!(resp.status, 200, "create push token: {}", resp.text);
1020 + let token = resp.text.trim().to_string();
1021 +
1022 + let upload = "/git/testowner/testrepo.git/info/refs?service=git-upload-pack";
1023 + let recv = "/git/testowner/testrepo.git/info/refs?service=git-receive-pack";
1024 +
1025 + // Baseline: the token reads and pushes, the cookie reads. The token half
1026 + // runs with the cookies cleared, since the funnel prefers a session and a
1027 + // session is not a push credential.
1028 + h.client.clear_cookies();
1029 + let resp = h
1030 + .client
1031 + .request_with_headers(
1032 + "GET",
1033 + upload,
1034 + None,
1035 + &[("Authorization", &basic_auth(&token))],
1036 + )
1037 + .await;
1038 + assert_eq!(
1039 + resp.status, 200,
1040 + "token read before suspension: {}",
1041 + resp.text
1042 + );
1043 + let resp = h
1044 + .client
1045 + .request_with_headers("GET", recv, None, &[("Authorization", &basic_auth(&token))])
1046 + .await;
1047 + assert_eq!(
1048 + resp.status, 200,
1049 + "token push advert before suspension: {}",
1050 + resp.text
1051 + );
1052 + h.login("testowner", "password123").await;
1053 + let resp = h.client.get(upload).await;
1054 + assert_eq!(
1055 + resp.status, 200,
1056 + "cookie read before suspension: {}",
1057 + resp.text
1058 + );
1059 +
1060 + sqlx::query("UPDATE users SET suspended_at = now() WHERE username = 'testowner'")
1061 + .execute(&h.db)
1062 + .await
1063 + .unwrap();
1064 +
1065 + // The cookie branch: the session survives, the account does not.
1066 + let resp = h.client.get(upload).await;
1067 + assert_eq!(
1068 + resp.status, 404,
1069 + "a suspended account must not read over a session cookie: {}",
1070 + resp.text
1071 + );
1072 +
1073 + // The token branch, read and push both.
1074 + h.client.clear_cookies();
1075 + let resp = h
1076 + .client
1077 + .request_with_headers(
1078 + "GET",
1079 + upload,
1080 + None,
1081 + &[("Authorization", &basic_auth(&token))],
1082 + )
1083 + .await;
1084 + assert_eq!(
1085 + resp.status, 404,
1086 + "a suspended account must not clone over HTTPS: {}",
1087 + resp.text
1088 + );
1089 + let resp = h
1090 + .client
1091 + .request_with_headers("GET", recv, None, &[("Authorization", &basic_auth(&token))])
1092 + .await;
1093 + assert_eq!(
1094 + resp.status, 404,
1095 + "a suspended account must not push over HTTPS: {}",
1096 + resp.text
1097 + );
1098 +
1099 + // Deactivation is refused on the same terms, the way SSH pairs them.
1100 + sqlx::query(
1101 + "UPDATE users SET suspended_at = NULL, deactivated_at = now() WHERE username = 'testowner'",
1102 + )
1103 + .execute(&h.db)
1104 + .await
1105 + .unwrap();
1106 + let resp = h
1107 + .client
1108 + .request_with_headers(
1109 + "GET",
1110 + upload,
1111 + None,
1112 + &[("Authorization", &basic_auth(&token))],
1113 + )
1114 + .await;
1115 + assert_eq!(
1116 + resp.status, 404,
1117 + "a deactivated account must not clone over HTTPS: {}",
1118 + resp.text
1119 + );
1120 +
1121 + // Lifting the suspension restores the same token, so it is account standing
1122 + // being enforced and not the token being invalidated.
1123 + sqlx::query("UPDATE users SET deactivated_at = NULL WHERE username = 'testowner'")
1124 + .execute(&h.db)
1125 + .await
1126 + .unwrap();
1127 + let resp = h
1128 + .client
1129 + .request_with_headers(
1130 + "GET",
1131 + upload,
1132 + None,
1133 + &[("Authorization", &basic_auth(&token))],
1134 + )
1135 + .await;
1136 + assert_eq!(
1137 + resp.status, 200,
1138 + "token should work again once the account is in good standing: {}",
1139 + resp.text
1140 + );
1141 + }
1142 +
996 1143 // UX-S1: git push (receive-pack) must reject session-cookie auth, even for the
997 1144 // repo OWNER. These routes are merged outside the CsrfRouter/origin_gate tree, so
998 1145 // a cookie-authed push would be drivable cross-origin with only git wire-format
M server/src/routes/api/git_notes.rs +2 -5
@@ -518,14 +518,11 @@
518 518 let principal = crate::routes::git::resolve_git_http_principal(db, headers, None).await;
519 519 let principal = require_push_token(principal.as_ref())?;
520 520
521 + // Account standing is already settled: `resolve_git_http_principal` refuses
522 + // a suspended or deactivated user. This load is for the note identity.
521 523 let user = db::users::get_user_by_id(db, principal.user_id)
522 524 .await?
523 525 .ok_or(AppError::Unauthorized)?;
524 - // The browser path refuses a suspended account, and a token minted before a
525 - // suspension would otherwise be the way around it.
526 - if user.suspended_at.is_some() || user.deactivated_at.is_some() {
527 - return Err(AppError::Forbidden);
528 - }
529 526
530 527 let resolved = resolve_repo(db, config, owner, repo_name, Some(principal.user_id)).await?;
531 528 if !notes_write::can_write_notes(db, &resolved, principal.user_id).await? {
M server/src/routes/git/mod.rs +28 -16
@@ -309,29 +309,41 @@
309 309 /// password; the username is ignored). Returns `None` for anonymous, unknown,
310 310 /// or expired credentials, callers treat that as unauthenticated, and a
311 311 /// private repo then 404s exactly as it does for an anonymous browser request.
312 + ///
313 + /// Whichever branch produced the id, the account behind it has to be in good
314 + /// standing: a suspended or deactivated user resolves to `None`. Without that,
315 + /// a token minted before a suspension keeps working until it expires, and
316 + /// suspension means suspended over SSH (`git_ssh::dispatch`) but not over HTTPS.
317 + /// The check sits here rather than in `authorize_push` so it covers reads too,
318 + /// and rather than in `db::git_access_tokens::resolve_active` so it covers the
319 + /// session-cookie branch, which never touches that query.
312 320 pub(crate) async fn resolve_git_http_principal(
313 321 db: &PgPool,
314 322 headers: &axum::http::HeaderMap,
315 323 session_user_id: Option<UserId>,
316 324 ) -> Option<GitHttpPrincipal> {
317 - if let Some(user_id) = session_user_id {
318 - return Some(GitHttpPrincipal {
319 - user_id,
320 - token_push: None,
321 - });
325 + let (user_id, token_push) = match session_user_id {
326 + Some(user_id) => (user_id, None),
327 + None => {
328 + let header = headers
329 + .get(axum::http::header::AUTHORIZATION)?
330 + .to_str()
331 + .ok()?;
332 + let token = parse_basic_auth_token(header)?;
333 + let hash = crate::crypto::git_token_hash(&token);
334 + let resolved = db::git_access_tokens::resolve_active(db, &hash)
335 + .await
336 + .ok()??;
337 + (resolved.user_id, Some(resolved.can_push))
338 + }
339 + };
340 + let user = db::users::get_user_by_id(db, user_id).await.ok()??;
341 + if user.is_suspended() || user.is_deactivated() {
342 + return None;
322 343 }
323 - let header = headers
324 - .get(axum::http::header::AUTHORIZATION)?
325 - .to_str()
326 - .ok()?;
327 - let token = parse_basic_auth_token(header)?;
328 - let hash = crate::crypto::git_token_hash(&token);
329 - let resolved = db::git_access_tokens::resolve_active(db, &hash)
330 - .await
331 - .ok()??;
332 344 Some(GitHttpPrincipal {
333 - user_id: resolved.user_id,
334 - token_push: Some(resolved.can_push),
345 + user_id,
346 + token_push,
335 347 })
336 348 }
337 349