Skip to main content

max / makenotwork

audit Run 16 Phase 1: Security axis A- -> A Fix the net-new Security cluster from the Run 16 audit: - receipt page: require an authenticated viewer before the buyer/seller check so an anonymous caller can no longer match a guest transaction's NULL buyer_id via None == None (live info-disclosure). - verify-email: check the HMAC signature before revealing any account state; unify not-found / already-verified / bad-sig into one generic response (removes the account-enumeration oracle). - oauth: add an explicit `sync` scope and is_sync_request(); scope=sync is the opt-in for the 7-day sync token, empty scope still works but logs a deprecation warn (flip to reject once first-party clients are updated). - git raw serving: add X-Content-Type-Options: nosniff. - git resolve_repo: auto-register a row-less bare repo only for the owner; anonymous/third-party requests get 404 (no anon DB write, no private repo republished at the public default). - ota: reject an empty/implausible release signature at creation so the updater is never handed an un-installable release. - validation: add reject_control_chars_multiline and wire it into every multi-line body/description validator; strict reject on single-line tier_name/version_number/machine_id. - internal git authorize: route the client-supplied owner through Username::new instead of from_trusted. Build-runner sandbox deferred to a standalone infra task (Max's call). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-07-02 13:15 UTC
Commit: fb85bab14d81a0b59c53657f1f9c85871b570a24
Parent: 5268535
13 files changed, +171 insertions, -43 deletions
M server/src/oauth_scope.rs +34
@@ -19,6 +19,10 @@ pub enum OAuthScope {
19 19 PerksRead,
20 20 /// Issue a refresh token on the authorization-code grant (OIDC name).
21 21 Offline,
22 + /// Full SyncKit-API access — the desktop-app pairing flow. Distinct from the
23 + /// read-only userinfo scopes: a token carrying `sync` authenticates the
24 + /// entire sync API, so it is the explicit opt-in for the 7-day sync token.
25 + Sync,
22 26 }
23 27
24 28 impl OAuthScope {
@@ -27,6 +31,7 @@ impl OAuthScope {
27 31 OAuthScope::ProfileRead => "profile:read",
28 32 OAuthScope::PerksRead => "perks:read",
29 33 OAuthScope::Offline => "offline_access",
34 + OAuthScope::Sync => "sync",
30 35 }
31 36 }
32 37 }
@@ -38,6 +43,7 @@ impl FromStr for OAuthScope {
38 43 "profile:read" => Ok(OAuthScope::ProfileRead),
39 44 "perks:read" => Ok(OAuthScope::PerksRead),
40 45 "offline_access" => Ok(OAuthScope::Offline),
46 + "sync" => Ok(OAuthScope::Sync),
41 47 _ => Err(()),
42 48 }
43 49 }
@@ -78,6 +84,20 @@ impl GrantedScopes {
78 84 self.0.is_empty()
79 85 }
80 86
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.
91 + ///
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.
97 + pub fn is_sync_request(&self) -> bool {
98 + self.0.is_empty() || self.contains(OAuthScope::Sync)
99 + }
100 +
81 101 /// True if every scope in `self` is also in `other`. The downgrade-only
82 102 /// invariant for refresh: a refresh request may narrow but never widen the
83 103 /// scope it was originally granted. Also the prompt=none consent gate: a
@@ -165,4 +185,18 @@ mod tests {
165 185 assert!(GrantedScopes::parse("perks:read").subset_of(&a));
166 186 assert!(!GrantedScopes::parse("offline_access").subset_of(&a));
167 187 }
188 +
189 + #[test]
190 + fn sync_request_detection() {
191 + // Explicit opt-in and the deprecated empty scope both mint a sync token.
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.
197 + assert!(!GrantedScopes::parse("profile:read").is_sync_request());
198 + assert!(!GrantedScopes::parse("profile:read perks:read").is_sync_request());
199 + // `sync` round-trips through the canonical string form.
200 + assert_eq!(GrantedScopes::parse("sync").to_string(), "sync");
201 + }
168 202 }
M server/src/routes/api/internal/git.rs +9 -7
@@ -137,13 +137,15 @@ pub(super) async fn git_authorize(
137 137 .as_deref()
138 138 .ok_or_else(|| AppError::ServiceUnavailable("Git hosting is not configured".to_string()))?;
139 139
140 - // Look up the namespace owner
141 - let owner_user = db::users::get_user_by_username(
142 - &state.db,
143 - &Username::from_trusted(req.owner.clone()),
144 - )
145 - .await?
146 - .ok_or(AppError::NotFound)?;
140 + // Look up the namespace owner. `req.owner` is a client-supplied field, so
141 + // validate it through `Username::new` rather than `from_trusted` — the
142 + // newtype's contract is "this string already passed validation", and an
143 + // unvalidated owner defeats it (a malformed owner simply can't name a real
144 + // user, so it maps to the same NotFound).
145 + let owner = Username::new(&req.owner).map_err(|_| AppError::NotFound)?;
146 + let owner_user = db::users::get_user_by_username(&state.db, &owner)
147 + .await?
148 + .ok_or(AppError::NotFound)?;
147 149
148 150 let repo = match db::git_repos::get_repo_by_user_and_name(
149 151 &state.db,
M server/src/routes/git/mod.rs +9 -1
@@ -178,7 +178,15 @@ pub(crate) async fn resolve_repo(
178 178 let db_repo = match db::git_repos::get_repo_by_user_and_name(&state.db, db_user.id, repo_name).await? {
179 179 Some(r) => r,
180 180 None => {
181 - // Auto-register: repo exists on disk but not in DB
181 + // Auto-register: repo exists on disk but not in DB. Only the owner
182 + // may trigger this. Otherwise an anonymous or third-party GET would
183 + // (a) issue a DB write on an unauthenticated request, and (b)
184 + // materialize the row at `create_repo`'s default visibility —
185 + // re-publishing a private repo that lost its row. Non-owners get the
186 + // same 404 as a genuinely missing repo.
187 + if session_user_id != Some(db_user.id) {
188 + return Err(AppError::NotFound);
189 + }
182 190 let disk_dir = owner;
183 191 if git::open_repo(&root, disk_dir, repo_name).is_err() {
184 192 return Err(AppError::NotFound);
M server/src/routes/git/raw.rs +5
@@ -71,6 +71,11 @@ pub(super) async fn raw_file(
71 71
72 72 Response::builder()
73 73 .header(header::CONTENT_TYPE, content_type)
74 + // Repo contents are attacker-controlled and served from the primary
75 + // origin. `nosniff` stops a browser from MIME-sniffing an
76 + // `application/octet-stream` (or otherwise unlisted) file into active
77 + // HTML/script — a stored-XSS vector when raw files share the app origin.
78 + .header(header::X_CONTENT_TYPE_OPTIONS, "nosniff")
74 79 .header(
75 80 header::CONTENT_DISPOSITION,
76 81 format!(
M server/src/routes/oauth.rs +23 -9
@@ -342,13 +342,16 @@ async fn authorize_get(
342 342 Some(user) => {
343 343 // Silent re-auth may only mint a code for scopes the user has
344 344 // ALREADY consented to for this app; anything broader requires
345 - // interactive approval (ultra-fuzz Run 6 R6-Sec-L5). Empty scope
346 - // (legacy sync client) is a subset of any grant, so it still
347 - // passes. The check reads consent purely from the DB, so a
348 - // client-supplied scope can never silently widen a grant.
345 + // interactive approval (ultra-fuzz Run 6 R6-Sec-L5). The sync
346 + // pairing flow (explicit `scope=sync`, or the deprecated empty
347 + // scope) is gated by PKCE + interactive pairing rather than the
348 + // userinfo consent ledger, so it bypasses the subset check
349 + // exactly as empty scope always has. The check reads consent
350 + // purely from the DB, so a client-supplied userinfo scope can
351 + // never silently widen a grant.
349 352 let granted =
350 353 db::oauth::get_granted_scopes(&state.db, user.id, app.id).await?;
351 - if !scope.subset_of(&granted) {
354 + if !scope.is_sync_request() && !scope.subset_of(&granted) {
352 355 return Ok(redirect_with_error(
353 356 redirect_uri,
354 357 state_param,
@@ -716,10 +719,21 @@ async fn token_authorization_code(
716 719
717 720 let scope = GrantedScopes::parse(&oauth_code.scope);
718 721
719 - // Empty scope = a legacy sync client (desktop apps via synckit-client): mint
720 - // the full 7-day sync token exactly as before. This is the only path that
721 - // still issues a sync-API-capable token from /oauth/token.
722 - if scope.is_empty() {
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.
725 + 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 + }
723 737 let token = synckit_auth::create_sync_token(
724 738 secret,
725 739 oauth_code.user_id,
M server/src/routes/ota.rs +13 -1
@@ -205,8 +205,20 @@ async fn create_release(
205 205 verify_app_owner(&state, &sync_user, app_id).await?;
206 206 validate_semver(&req.version)?;
207 207
208 + // The signature is served verbatim to the Tauri updater, which verifies the
209 + // artifact against it with minisign. An empty/implausible signature (the
210 + // serde default) produces a release the updater advertises but can never
211 + // install — reject it here rather than shipping an un-installable update. A
212 + // real base64-encoded minisign signature is well over 40 chars.
213 + let signature = req.signature.trim();
214 + if signature.len() < 40 {
215 + return Err(AppError::BadRequest(
216 + "signature is required (base64-encoded minisign signature)".to_string(),
217 + ));
218 + }
219 +
208 220 let release =
209 - db::ota::create_release(&state.db, app_id, &req.version, &req.notes, &req.signature)
221 + db::ota::create_release(&state.db, app_id, &req.version, &req.notes, signature)
210 222 .await?;
211 223
212 224 Ok((
M server/src/routes/pages/email_actions/links.rs +26 -21
@@ -70,25 +70,30 @@ pub(super) async fn verify_email_handler(
70 70 }
71 71 };
72 72
73 - // Get user
73 + // Generic failure response reused for every pre-signature outcome so this
74 + // endpoint is not an account-enumeration oracle: "user not found", "already
75 + // verified", and "bad signature" must be indistinguishable to a caller
76 + // without a valid HMAC. UserIds are visible in `/feed/{id}` URLs, so any
77 + // observable difference would leak account existence + verified-state.
78 + let invalid_link = || {
79 + error_page(
80 + "Email Verification Failed",
81 + "Verification link has expired or is invalid. Please request a new one.",
82 + "/dashboard",
83 + "Go to dashboard",
84 + )
85 + };
86 +
87 + // Get user. A missing user yields the same generic response as a bad
88 + // signature (the signature is keyed on the user's email, so a genuine link
89 + // can only exist for a real user anyway).
74 90 let user = match db::users::get_user_by_id(&state.db, user_id).await {
75 91 Ok(Some(u)) => u,
76 - _ => {
77 - return Ok(error_page(
78 - "Email Verification Failed",
79 - "User not found",
80 - "/dashboard",
81 - "Go to dashboard",
82 - ))
83 - }
92 + _ => return Ok(invalid_link()),
84 93 };
85 94
86 - // Check if already verified
87 - if user.email_verified {
88 - return Ok(Redirect::to("/dashboard").into_response());
89 - }
90 -
91 - // Verify HMAC signature
95 + // Verify the HMAC signature (and embedded expiry) BEFORE revealing any
96 + // account state such as the already-verified redirect below.
92 97 if !email::verify_email_signature(
93 98 user_id,
94 99 expires,
@@ -96,12 +101,12 @@ pub(super) async fn verify_email_handler(
96 101 &sig,
97 102 &state.config.signing_secret,
98 103 ) {
99 - return Ok(error_page(
100 - "Email Verification Failed",
101 - "Verification link has expired or is invalid. Please request a new one.",
102 - "/dashboard",
103 - "Go to dashboard",
104 - ));
104 + return Ok(invalid_link());
105 + }
106 +
107 + // Signature is valid — only now is it safe to reveal verified state.
108 + if user.email_verified {
109 + return Ok(Redirect::to("/dashboard").into_response());
105 110 }
106 111
107 112 // Mark email as verified
M server/src/routes/pages/public/content/mod.rs +10 -4
@@ -248,10 +248,16 @@ pub(super) async fn receipt_page(
248 248 .await?
249 249 .ok_or(AppError::NotFound)?;
250 250
251 - // Only the buyer or the seller can view a receipt
252 - let viewer_id = maybe_user.as_ref().map(|u| u.id);
253 - let is_buyer = viewer_id == tx.buyer_id;
254 - let is_seller = viewer_id == tx.seller_id;
251 + // Only the buyer or the seller can view a receipt. An anonymous viewer must
252 + // never match: guest transactions persist `buyer_id = NULL`, so comparing an
253 + // `Option` viewer directly (`None == tx.buyer_id`) would let any anonymous
254 + // caller read a guest receipt. Require an authenticated viewer, then compare
255 + // against the concrete `Some(id)`.
256 + let Some(viewer_id) = maybe_user.as_ref().map(|u| u.id) else {
257 + return Err(AppError::Forbidden);
258 + };
259 + let is_buyer = tx.buyer_id == Some(viewer_id);
260 + let is_seller = tx.seller_id == Some(viewer_id);
255 261 if !is_buyer && !is_seller {
256 262 return Err(AppError::Forbidden);
257 263 }
M server/src/validation/items.rs +19
@@ -26,6 +26,7 @@ pub fn validate_item_description(description: &str) -> Result<(), AppError> {
26 26 limits::ITEM_DESCRIPTION_MAX
27 27 )));
28 28 }
29 + super::reject_control_chars_multiline("Description", description)?;
29 30 Ok(())
30 31 }
31 32
@@ -52,6 +53,7 @@ pub fn validate_item_text_body(body: &str) -> Result<(), AppError> {
52 53 limits::ITEM_TEXT_BODY_MAX
53 54 )));
54 55 }
56 + super::reject_control_chars_multiline("Text body", body)?;
55 57 Ok(())
56 58 }
57 59
@@ -105,6 +107,7 @@ pub fn validate_version_number(version: &str) -> Result<(), AppError> {
105 107 limits::VERSION_NUMBER_MAX
106 108 )));
107 109 }
110 + super::reject_control_chars("Version number", version)?;
108 111 Ok(())
109 112 }
110 113
@@ -116,6 +119,7 @@ pub fn validate_changelog(changelog: &str) -> Result<(), AppError> {
116 119 limits::CHANGELOG_MAX
117 120 )));
118 121 }
122 + super::reject_control_chars_multiline("Changelog", changelog)?;
119 123 Ok(())
120 124 }
121 125
@@ -133,6 +137,7 @@ pub fn validate_waitlist_pitch(pitch: &str) -> Result<(), AppError> {
133 137 limits::WAITLIST_PITCH_MAX
134 138 )));
135 139 }
140 + super::reject_control_chars_multiline("Pitch", pitch)?;
136 141 Ok(())
137 142 }
138 143
@@ -164,6 +169,7 @@ pub fn validate_blog_post_body(body: &str) -> Result<(), AppError> {
164 169 limits::BLOG_POST_BODY_MAX
165 170 )));
166 171 }
172 + super::reject_control_chars_multiline("Blog post body", body)?;
167 173 Ok(())
168 174 }
169 175
@@ -216,6 +222,7 @@ pub fn validate_collection_description(description: &str) -> Result<(), AppError
216 222 limits::COLLECTION_DESCRIPTION_MAX
217 223 )));
218 224 }
225 + super::reject_control_chars_multiline("Collection description", description)?;
219 226 Ok(())
220 227 }
221 228
@@ -243,6 +250,7 @@ pub fn validate_section_body(body: &str) -> Result<(), AppError> {
243 250 limits::SECTION_BODY_MAX
244 251 )));
245 252 }
253 + super::reject_control_chars_multiline("Section body", body)?;
246 254 Ok(())
247 255 }
248 256
@@ -255,6 +263,17 @@ mod tests {
255 263 use super::*;
256 264
257 265 #[test]
266 + fn multiline_bodies_reject_control_bytes_but_allow_newlines() {
267 + // Multi-line fields keep newlines/tabs (legitimate formatting)...
268 + assert!(validate_item_description("line one\nline two\twith tab").is_ok());
269 + assert!(validate_item_text_body("para one\n\npara two").is_ok());
270 + // ...but reject NUL and other non-whitespace control bytes.
271 + assert!(validate_item_description("bad\0null").is_err());
272 + assert!(validate_item_text_body("esc\u{1b}[31m").is_err());
273 + assert!(validate_changelog("fixed\u{7f}del").is_err());
274 + }
275 +
276 + #[test]
258 277 fn test_validate_item_title() {
259 278 assert!(validate_item_title("My Song").is_ok());
260 279 assert!(validate_item_title("").is_err()); // empty
M server/src/validation/mod.rs +14
@@ -31,6 +31,20 @@ pub fn reject_control_chars(field: &str, value: &str) -> Result<(), AppError> {
31 31 Ok(())
32 32 }
33 33
34 + /// Reject control characters in a multi-line field while permitting normal
35 + /// whitespace (`\n`, `\r`, `\t`). Descriptions, bodies, and changelogs are
36 + /// legitimately multi-line but should never carry NUL/escape/etc. — those
37 + /// corrupt logs and tooling and can smuggle bytes into downstream contexts.
38 + /// Mirrors `validate_bio`.
39 + pub fn reject_control_chars_multiline(field: &str, value: &str) -> Result<(), AppError> {
40 + if value.chars().any(|c| c.is_control() && !matches!(c, '\n' | '\r' | '\t')) {
41 + return Err(AppError::validation(format!(
42 + "{field} cannot contain control characters"
43 + )));
44 + }
45 + Ok(())
46 + }
47 +
34 48 /// Maximum lengths for various fields
35 49 pub mod limits {
36 50 pub const DISPLAY_NAME_MAX: usize = 100;
M server/src/validation/payments.rs +4
@@ -35,6 +35,9 @@ pub fn validate_tier_name(name: &str) -> Result<(), AppError> {
35 35 limits::TIER_NAME_MAX
36 36 )));
37 37 }
38 + // Single-line: a tier name can reach an email subject (Postmark rejects
39 + // CRLF, but reject at source), so no line breaks or control chars.
40 + super::reject_control_chars("Tier name", name)?;
38 41 Ok(())
39 42 }
40 43
@@ -46,6 +49,7 @@ pub fn validate_tier_description(description: &str) -> Result<(), AppError> {
46 49 limits::TIER_DESCRIPTION_MAX
47 50 )));
48 51 }
52 + super::reject_control_chars_multiline("Tier description", description)?;
49 53 Ok(())
50 54 }
51 55
M server/src/validation/projects.rs +4
@@ -26,6 +26,7 @@ pub fn validate_project_description(description: &str) -> Result<(), AppError> {
26 26 limits::PROJECT_DESCRIPTION_MAX
27 27 )));
28 28 }
29 + super::reject_control_chars_multiline("Project description", description)?;
29 30 Ok(())
30 31 }
31 32
@@ -57,6 +58,7 @@ pub fn validate_issue_body(body: &str) -> Result<(), AppError> {
57 58 limits::ISSUE_BODY_MAX
58 59 )));
59 60 }
61 + super::reject_control_chars_multiline("Issue body", body)?;
60 62 Ok(())
61 63 }
62 64
@@ -71,6 +73,7 @@ pub fn validate_issue_comment_body(body: &str) -> Result<(), AppError> {
71 73 limits::ISSUE_COMMENT_BODY_MAX
72 74 )));
73 75 }
76 + super::reject_control_chars_multiline("Comment", body)?;
74 77 Ok(())
75 78 }
76 79
@@ -118,6 +121,7 @@ pub fn validate_repo_description(desc: &str) -> Result<(), AppError> {
118 121 limits::REPO_DESCRIPTION_MAX
119 122 )));
120 123 }
124 + super::reject_control_chars_multiline("Repository description", desc)?;
121 125 Ok(())
122 126 }
123 127
M server/src/validation/users.rs +1
@@ -148,6 +148,7 @@ pub fn validate_machine_id(machine_id: &str) -> Result<(), AppError> {
148 148 limits::MACHINE_ID_MAX
149 149 )));
150 150 }
151 + super::reject_control_chars("Machine ID", machine_id)?;
151 152 Ok(())
152 153 }
153 154