Skip to main content

max / makenotwork

server: flip OAuth empty-scope to userinfo; validate build signing_key_path OAuth empty-scope escalation: an /oauth/token request that omitted `scope` was treated as a legacy sync client and minted the broadest 7-day full-sync token. is_sync_request() now requires the explicit `sync` scope; an omitted or unrecognized scope takes the least-privilege userinfo path. synckit-client's build_authorize_url now sends `scope=sync` so first-party sync clients (rebuilt af/go/mnw-cli) keep working; already-deployed builds that send no scope fall to userinfo until updated (accepted rollout cost). Removed the now-dead empty-scope warn branch at the token endpoint; updated the oauth workflow tests to assert no-scope -> userinfo (not sync) and scope=sync -> sync. Build signing_key_path was the one build-config field bypassing validate_build_config_fields; route it through validate_artifact_path (when non-empty) so a stored `../` / absolute path can't become a cross-tenant key-read foothold once the runner wires up signing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-07-02 16:52 UTC
Commit: 3bcd9e380ce1e3cec34521c1c49084dc52fd161d
Parent: bd33ce7
5 files changed, +71 insertions, -40 deletions
M server/src/oauth_scope.rs +18 -14
@@ -85,17 +85,17 @@ impl GrantedScopes {
85 85 }
86 86
87 87 /// True when this is a full-sync request rather than the read-only userinfo
88 - /// RP flow. A request is "sync" if it explicitly carries the `sync` scope,
89 - /// or — for backward compatibility with desktop clients that predate the
90 - /// explicit scope — if it carries no recognized scope at all.
88 + /// RP flow. A request is "sync" **only** if it explicitly carries the `sync`
89 + /// scope.
91 90 ///
92 - /// The empty-scope path is deprecated: it exists only so already-shipped
93 - /// clients keep working during the transition. Once the first-party clients
94 - /// (GoingsOn, AudioFiles, mnw-cli) send `scope=sync`, the empty path should
95 - /// be rejected outright so a userinfo client that merely forgets its `scope`
96 - /// param can no longer be silently escalated to a sync-capable token.
91 + /// An empty / unrecognized scope set is NOT sync: it takes the least-
92 + /// privilege userinfo path. This closes the escalation where a relying party
93 + /// that merely omitted `scope` was silently granted the broadest 7-day
94 + /// sync-API token (audit Run 17 Security). First-party clients now send
95 + /// `scope=sync` via `synckit-client`'s authorize URL; a client that predates
96 + /// that build and sends no scope gets a userinfo token, not sync.
97 97 pub fn is_sync_request(&self) -> bool {
98 - self.0.is_empty() || self.contains(OAuthScope::Sync)
98 + self.contains(OAuthScope::Sync)
99 99 }
100 100
101 101 /// True if every scope in `self` is also in `other`. The downgrade-only
@@ -188,12 +188,16 @@ mod tests {
188 188
189 189 #[test]
190 190 fn sync_request_detection() {
191 - // Explicit opt-in and the deprecated empty scope both mint a sync token.
191 + // Only the explicit `sync` scope mints a sync token.
192 192 assert!(GrantedScopes::parse("sync").is_sync_request());
193 - assert!(GrantedScopes::parse("").is_sync_request());
194 - assert!(GrantedScopes::parse(" ").is_sync_request());
195 - // A userinfo request (which is what a client that *forgot* nothing sends)
196 - // must NOT be treated as a sync request — this is the escalation guard.
193 + // Empty / whitespace-only scope is NO LONGER treated as sync — it takes
194 + // the least-privilege userinfo path. This is the Run 17 escalation fix:
195 + // a client that omits `scope` can no longer be upgraded to a sync token.
196 + assert!(!GrantedScopes::parse("").is_sync_request());
197 + assert!(!GrantedScopes::parse(" ").is_sync_request());
198 + // Unrecognized-only scopes parse to empty → also not sync.
199 + assert!(!GrantedScopes::parse("admin:everything").is_sync_request());
200 + // A userinfo request must NOT be treated as a sync request.
197 201 assert!(!GrantedScopes::parse("profile:read").is_sync_request());
198 202 assert!(!GrantedScopes::parse("profile:read perks:read").is_sync_request());
199 203 // `sync` round-trips through the canonical string form.
M server/src/routes/builds.rs +18 -4
@@ -42,12 +42,26 @@ fn validate_targets(targets: &[String]) -> Result<()> {
42 42 Ok(())
43 43 }
44 44
45 - /// Validate build_command and artifact_path for shell safety.
46 - fn validate_build_config_fields(build_command: &str, artifact_path: &str) -> Result<()> {
45 + /// Validate build_command, artifact_path, and signing_key_path for shell +
46 + /// traversal safety. `signing_key_path` is optional (empty = unsigned); when
47 + /// set it names a path the build runner will read a signing key from, so it
48 + /// must pass the same relative-path/no-`..`/no-metacharacter check as
49 + /// `artifact_path` — otherwise a stored `../` or absolute path becomes a
50 + /// cross-tenant key-read foothold the moment the runner wires up signing
51 + /// (audit Run 17 Security: the one build-config field that bypassed validation).
52 + fn validate_build_config_fields(
53 + build_command: &str,
54 + artifact_path: &str,
55 + signing_key_path: &str,
56 + ) -> Result<()> {
47 57 crate::build_runner::validate_build_command(build_command)
48 58 .map_err(|e| AppError::validation(format!("build_command: {e}")))?;
49 59 crate::build_runner::validate_artifact_path(artifact_path)
50 60 .map_err(|e| AppError::validation(format!("artifact_path: {e}")))?;
61 + if !signing_key_path.is_empty() {
62 + crate::build_runner::validate_artifact_path(signing_key_path)
63 + .map_err(|e| AppError::validation(format!("signing_key_path: {e}")))?;
64 + }
51 65 Ok(())
52 66 }
53 67
@@ -206,7 +220,7 @@ async fn create_config(
206 220 ) -> Result<impl IntoResponse> {
207 221 verify_app_owner(&state, &sync_user, app_id).await?;
208 222 validate_targets(&req.targets)?;
209 - validate_build_config_fields(&req.build_command, &req.artifact_path)?;
223 + validate_build_config_fields(&req.build_command, &req.artifact_path, &req.signing_key_path)?;
210 224
211 225 // Verify repo ownership
212 226 let repo = db::git_repos::get_repo_by_id(&state.db, req.repo_id)
@@ -260,7 +274,7 @@ async fn update_config(
260 274 ) -> Result<impl IntoResponse> {
261 275 verify_app_owner(&state, &sync_user, app_id).await?;
262 276 validate_targets(&req.targets)?;
263 - validate_build_config_fields(&req.build_command, &req.artifact_path)?;
277 + validate_build_config_fields(&req.build_command, &req.artifact_path, &req.signing_key_path)?;
264 278
265 279 let existing = db::builds::get_build_config_by_app(&state.db, app_id)
266 280 .await?
M server/src/routes/oauth.rs +4 -14
@@ -719,21 +719,11 @@ async fn token_authorization_code(
719 719
720 720 let scope = GrantedScopes::parse(&oauth_code.scope);
721 721
722 - // A sync request (explicit `scope=sync`, or — deprecated — no scope at all)
723 - // mints the full 7-day sync token. This is the only path that issues a
724 - // sync-API-capable token from /oauth/token.
722 + // Only an explicit `scope=sync` mints the full 7-day sync token — the sole
723 + // path that issues a sync-API-capable token from /oauth/token. An omitted or
724 + // unrecognized scope now falls through to the least-privilege userinfo path
725 + // below rather than being escalated to sync (audit Run 17 Security).
725 726 if scope.is_sync_request() {
726 - // The empty-scope path is a backward-compatibility bridge only: a
727 - // userinfo client that merely forgets its `scope` param lands here and
728 - // is silently upgraded to a sync-capable token. Log every use so the
729 - // remaining empty-scope clients can be identified and migrated to
730 - // `scope=sync`, after which this branch should reject empty scope.
731 - if scope.is_empty() {
732 - tracing::warn!(
733 - app_id = %oauth_code.app_id,
734 - "oauth: full-sync token issued for empty scope (deprecated; client should send scope=sync)"
735 - );
736 - }
737 727 let token = synckit_auth::create_sync_token(
738 728 secret,
739 729 oauth_code.user_id,
M server/tests/workflows/oauth.rs +22 -7
@@ -94,7 +94,7 @@ async fn authorize(
94 94 let resp = h
95 95 .client
96 96 .get(&format!(
97 - "/oauth/authorize?response_type=code&client_id={}&redirect_uri={}&state={}&code_challenge={}&code_challenge_method=S256",
97 + "/oauth/authorize?response_type=code&client_id={}&redirect_uri={}&state={}&code_challenge={}&code_challenge_method=S256&scope=sync",
98 98 urlencoding::encode(client_id),
99 99 urlencoding::encode(redirect_uri),
100 100 state_param,
@@ -112,7 +112,7 @@ async fn authorize(
112 112
113 113 // POST credentials (CSRF goes in form body as _csrf)
114 114 let body = format!(
115 - "client_id={}&redirect_uri={}&state={}&code_challenge={}&code_challenge_method=S256&login={}&password={}&_csrf={}",
115 + "client_id={}&redirect_uri={}&state={}&code_challenge={}&code_challenge_method=S256&scope=sync&login={}&password={}&_csrf={}",
116 116 urlencoding::encode(client_id),
117 117 urlencoding::encode(redirect_uri),
118 118 state_param,
@@ -171,6 +171,13 @@ async fn oauth_full_flow() {
171 171 assert!(token.expires_in > 0);
172 172 assert_eq!(token.user_id, user_id);
173 173 assert_eq!(token.app_id, app_id);
174 +
175 + // `scope=sync` (sent by the authorize helper, mirroring synckit-client) mints
176 + // a sync-capable token that authenticates the sync API.
177 + h.client.set_bearer_token(&token.access_token);
178 + let resp = h.client.get("/api/v1/sync/status").await;
179 + assert_ne!(resp.status.as_u16(), 401, "scope=sync token should authenticate the sync API");
180 + h.client.clear_bearer_token();
174 181 }
175 182
176 183 #[tokio::test]
@@ -655,18 +662,26 @@ async fn oauth_userinfo_requires_perks_scope() {
655 662 }
656 663
657 664 #[tokio::test]
658 - async fn oauth_no_scope_yields_sync_token_without_refresh() {
659 - // The legacy/desktop path: omitting scope mints a full sync token (works on
660 - // the sync API) and no refresh token.
665 + async fn oauth_no_scope_yields_userinfo_token_not_sync() {
666 + // Run 17 security flip: omitting `scope` now mints a LEAST-PRIVILEGE userinfo
667 + // token, not a sync token. A relying party that merely forgot its `scope`
668 + // param can no longer be silently escalated to the 7-day full-sync token; a
669 + // client that wants sync must send `scope=sync` explicitly (synckit-client
670 + // does). No scope also means no refresh token (no offline_access).
661 671 let mut h = TestHarness::new().await;
662 672 h.signup("legacyuser", "legacy@test.com", "Password1!").await;
663 673 let token = obtain_scoped_token(&mut h, "legacyuser", "Password1!", "").await;
664 674 assert!(token.refresh_token.is_none(), "no scope => no refresh token");
665 - assert_eq!(token.scope, "");
666 675
676 + // The omitted-scope token must NOT authenticate the sync API — the escalation
677 + // guard. (Contrast oauth_full_flow, which sends scope=sync and succeeds.)
667 678 h.client.set_bearer_token(&token.access_token);
668 679 let resp = h.client.get("/api/v1/sync/status").await;
669 - assert_ne!(resp.status.as_u16(), 401, "legacy sync token should authenticate the sync API");
680 + assert_eq!(
681 + resp.status.as_u16(),
682 + 401,
683 + "an omitted-scope token must NOT authenticate the sync API"
684 + );
670 685 }
671 686
672 687 #[tokio::test]
M shared/synckit-client/src/client/auth.rs +9 -1
@@ -118,6 +118,11 @@ impl SyncKitClient {
118 118 ///
119 119 /// The caller is responsible for generating the PKCE verifier/challenge,
120 120 /// starting the localhost callback server, and opening the browser.
121 + ///
122 + /// Sends `scope=sync` explicitly: this is the SyncKit pairing flow and the
123 + /// full sync-API token is what it needs. The server treats an omitted scope
124 + /// as least-privilege userinfo (not sync), so the scope must be sent for the
125 + /// token exchange to yield a sync-capable token.
121 126 pub fn build_authorize_url(
122 127 &self,
123 128 redirect_port: u16,
@@ -125,7 +130,7 @@ impl SyncKitClient {
125 130 code_challenge: &str,
126 131 ) -> String {
127 132 format!(
128 - "{}/oauth/authorize?response_type=code&client_id={}&redirect_uri={}&state={}&code_challenge={}&code_challenge_method=S256",
133 + "{}/oauth/authorize?response_type=code&client_id={}&redirect_uri={}&state={}&code_challenge={}&code_challenge_method=S256&scope=sync",
129 134 self.config.server_url,
130 135 urlencoding::encode(&self.config.api_key),
131 136 urlencoding::encode(&format!("http://127.0.0.1:{}/", redirect_port)),
@@ -269,6 +274,9 @@ mod tests {
269 274 assert!(url.contains("state=random-state"));
270 275 assert!(url.contains("code_challenge=challenge123"));
271 276 assert!(url.contains("code_challenge_method=S256"));
277 + // The pairing flow must request the sync scope explicitly — the server
278 + // treats an omitted scope as least-privilege userinfo, not sync.
279 + assert!(url.contains("scope=sync"));
272 280 }
273 281
274 282 #[test]