Skip to main content

max / makenotwork

security: seal git-push CSRF, close 7z/RAR polyglot evasion, exhaustive sanitizer allowlists Drive Security to A (ultra-fuzz Run 7). UX-S1: git receive-pack (push) is merged outside the CsrfRouter/origin_gate tree; authorize_push now requires a push-scoped PAT (token_push == Some(true)) and rejects session-cookie auth, so a cross-origin cookie POST can't drive a write. Carve-out comment corrected to record the enforcement. Regression test: git_session_cookie_cannot_push. Sec-S1: the R6 7z/RAR rejection only matched magic at offset 0, so a [image header][7z payload] polyglot sniffed as an image and fell through to ClamAV FailOpen. detect_unsupported_container now window-scans the whole buffer (mirroring has_zip_eocd), gated on file_type != Download so it never scans a large download buffer. Regression tests for prefixed 7z + RAR. UX-S2: the CssRule at-rule match had a `_ => None` fail-open arm that emitted an unknown at-rule unfiltered into <style>. The match is now exhaustive (no wildcard), so a lightningcss upgrade adding a variant fails to compile until triaged. UX-S3: permissive markdown sanitization rested implicitly on ammonia defaults. Route it through a named permissive_builder() and add regression tests pinning that iframe/srcdoc/style/event-handler/javascript:/object/embed are stripped. Sec-M1: record_granted_scopes did a non-atomic read-modify-write; run it under a per-(user,app) pg_advisory_xact_lock in one transaction so a concurrent consent can't lose a scope. Sec-M2: media_files::delete folds ownership into SQL (WHERE id AND user_id); get_license_key_by_id renamed _unchecked with a contract doc; set_item_listed given an explicit caller-must-own contract. Sec-M3: replace the four hand-rolled constant-time loops in email/tokens.rs with helpers::constant_time_compare (also removes their length-truncation foot-gun). guest_download now gates on scan_status == Clean, like the authenticated download path, instead of skipping the quarantine check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-24 22:16 UTC
Commit: 240a4caa15bb806f3d5c9e175fa489a5c5dae4ab
Parent: d69642c
17 files changed, +339 insertions, -141 deletions
A server/.sqlx/query-51c328291c2dc9a1db7bd802afce2125fbd5673bb7cc5997c4fa569e6d10d217.json +77
@@ -0,0 +1,77 @@
1 + {
2 + "db_name": "PostgreSQL",
3 + "query": "DELETE FROM media_files WHERE id = $1 AND user_id = $2\n RETURNING id AS \"id: MediaFileId\", user_id AS \"user_id: UserId\", folder, filename,\n s3_key, content_type, file_size_bytes, media_type, scan_status,\n created_at AS \"created_at: chrono::DateTime<chrono::Utc>\"",
4 + "describe": {
5 + "columns": [
6 + {
7 + "ordinal": 0,
8 + "name": "id: MediaFileId",
9 + "type_info": "Uuid"
10 + },
11 + {
12 + "ordinal": 1,
13 + "name": "user_id: UserId",
14 + "type_info": "Uuid"
15 + },
16 + {
17 + "ordinal": 2,
18 + "name": "folder",
19 + "type_info": "Varchar"
20 + },
21 + {
22 + "ordinal": 3,
23 + "name": "filename",
24 + "type_info": "Varchar"
25 + },
26 + {
27 + "ordinal": 4,
28 + "name": "s3_key",
29 + "type_info": "Varchar"
30 + },
31 + {
32 + "ordinal": 5,
33 + "name": "content_type",
34 + "type_info": "Varchar"
35 + },
36 + {
37 + "ordinal": 6,
38 + "name": "file_size_bytes",
39 + "type_info": "Int8"
40 + },
41 + {
42 + "ordinal": 7,
43 + "name": "media_type",
44 + "type_info": "Varchar"
45 + },
46 + {
47 + "ordinal": 8,
48 + "name": "scan_status",
49 + "type_info": "Text"
50 + },
51 + {
52 + "ordinal": 9,
53 + "name": "created_at: chrono::DateTime<chrono::Utc>",
54 + "type_info": "Timestamptz"
55 + }
56 + ],
57 + "parameters": {
58 + "Left": [
59 + "Uuid",
60 + "Uuid"
61 + ]
62 + },
63 + "nullable": [
64 + false,
65 + false,
66 + false,
67 + false,
68 + false,
69 + false,
70 + false,
71 + false,
72 + false,
73 + false
74 + ]
75 + },
76 + "hash": "51c328291c2dc9a1db7bd802afce2125fbd5673bb7cc5997c4fa569e6d10d217"
77 + }
D server/.sqlx/query-adddea436ff8f51ba54a26b4c02670cd25adc39ec7b6b24e627b178809ee8b89.json -76
@@ -1,76 +0,0 @@
1 - {
2 - "db_name": "PostgreSQL",
3 - "query": "DELETE FROM media_files WHERE id = $1\n RETURNING id AS \"id: MediaFileId\", user_id AS \"user_id: UserId\", folder, filename,\n s3_key, content_type, file_size_bytes, media_type, scan_status,\n created_at AS \"created_at: chrono::DateTime<chrono::Utc>\"",
4 - "describe": {
5 - "columns": [
6 - {
7 - "ordinal": 0,
8 - "name": "id: MediaFileId",
9 - "type_info": "Uuid"
10 - },
11 - {
12 - "ordinal": 1,
13 - "name": "user_id: UserId",
14 - "type_info": "Uuid"
15 - },
16 - {
17 - "ordinal": 2,
18 - "name": "folder",
19 - "type_info": "Varchar"
20 - },
21 - {
22 - "ordinal": 3,
23 - "name": "filename",
24 - "type_info": "Varchar"
25 - },
26 - {
27 - "ordinal": 4,
28 - "name": "s3_key",
29 - "type_info": "Varchar"
30 - },
31 - {
32 - "ordinal": 5,
33 - "name": "content_type",
34 - "type_info": "Varchar"
35 - },
36 - {
37 - "ordinal": 6,
38 - "name": "file_size_bytes",
39 - "type_info": "Int8"
40 - },
41 - {
42 - "ordinal": 7,
43 - "name": "media_type",
44 - "type_info": "Varchar"
45 - },
46 - {
47 - "ordinal": 8,
48 - "name": "scan_status",
49 - "type_info": "Text"
50 - },
51 - {
52 - "ordinal": 9,
53 - "name": "created_at: chrono::DateTime<chrono::Utc>",
54 - "type_info": "Timestamptz"
55 - }
56 - ],
57 - "parameters": {
58 - "Left": [
59 - "Uuid"
60 - ]
61 - },
62 - "nullable": [
63 - false,
64 - false,
65 - false,
66 - false,
67 - false,
68 - false,
69 - false,
70 - false,
71 - false,
72 - false
73 - ]
74 - },
75 - "hash": "adddea436ff8f51ba54a26b4c02670cd25adc39ec7b6b24e627b178809ee8b89"
76 - }
M server/src/csrf.rs +7 -3
@@ -588,10 +588,14 @@ where
588 588 ///
589 589 /// Carve-out (ultra-fuzz Run 4): routers merged into the app OUTSIDE the
590 590 /// `CsrfRouter` tree (git smart-HTTP, SSO, embed) are not wrapped by this
591 - /// `origin_gate`. That is sound today because those surfaces are GET-only or
591 + /// `origin_gate`. That is sound because those surfaces are GET-only or
592 592 /// authenticated by a PAT/bearer rather than a session cookie (so they are
593 - /// legitimately CSRF-exempt) — but any future cookie-authed POST added to one
594 - /// of them must route through a `CsrfRouter`, not be merged raw.
593 + /// legitimately CSRF-exempt). The one mutating route among them — git
594 + /// `receive-pack` (push) — ENFORCES this: `authorize_push` requires a
595 + /// push-scoped PAT (`token_push == Some(true)`) and rejects session-cookie
596 + /// auth, so a cross-origin cookie POST cannot drive a write (UX-S1, Run 7).
597 + /// Any future cookie-authed POST added to one of these surfaces must route
598 + /// through a `CsrfRouter`, not be merged raw.
595 599 pub fn finalize(self) -> Router<S> {
596 600 self.0.layer(from_fn(origin_gate))
597 601 }
M server/src/custom_pages/css_sanitizer.rs +18 -1
@@ -275,7 +275,24 @@ impl<'i, 'p> Visitor<'i> for CssSanitizer<'p> {
275 275 CssRule::StartingStyle(_) => Some("@starting-style"),
276 276 CssRule::ViewTransition(_) => Some("@view-transition"),
277 277 CssRule::Unknown(_) => Some("unknown at-rule"),
278 - _ => None,
278 + // Explicitly allowed: plain style rules and the safe grouping/at-rules.
279 + // Enumerated with NO wildcard arm (UX-S2, Run 7) so a future
280 + // lightningcss upgrade that adds a `CssRule` variant fails to COMPILE
281 + // here until it is triaged into allow-or-block — instead of the old
282 + // `_ => None` silently emitting an unknown at-rule unfiltered into the
283 + // `<style>` block.
284 + CssRule::Media(_)
285 + | CssRule::Style(_)
286 + | CssRule::Keyframes(_)
287 + | CssRule::FontFace(_)
288 + | CssRule::Page(_)
289 + | CssRule::Supports(_)
290 + | CssRule::Nesting(_)
291 + | CssRule::NestedDeclarations(_)
292 + | CssRule::LayerStatement(_)
293 + | CssRule::LayerBlock(_)
294 + | CssRule::Ignored
295 + | CssRule::Custom(_) => None,
279 296 };
280 297
281 298 if let Some(name) = blocked_name {
M server/src/db/bundles.rs +4 -1
@@ -301,7 +301,10 @@ pub async fn is_bundle_member(pool: &PgPool, bundle_id: ItemId, child_id: ItemId
301 301 Ok(exists)
302 302 }
303 303
304 - /// Set the `listed` flag on an item.
304 + /// Set the `listed` flag on an item. UNSCOPED: takes no owner and updates by id
305 + /// alone, so every caller MUST have already verified the item belongs to the
306 + /// acting user (the bundle/project ownership check upstream) before calling — the
307 + /// flag write itself enforces no ownership (Sec-M2).
305 308 #[tracing::instrument(skip_all, fields(%item_id, listed))]
306 309 pub async fn set_item_listed(pool: &PgPool, item_id: ItemId, listed: bool) -> Result<()> {
307 310 sqlx::query("UPDATE items SET listed = $2 WHERE id = $1")
M server/src/db/license_keys.rs +4 -2
@@ -140,9 +140,11 @@ pub async fn get_license_key_by_transaction_id(
140 140 Ok(key)
141 141 }
142 142
143 - /// Get a license key by ID.
143 + /// Get a license key by ID. UNSCOPED: returns any user's key, so the caller MUST
144 + /// authorize against the returned `owner_id` / `item_id` before acting on it
145 + /// (the `_unchecked` suffix makes that contract legible at the call site — Sec-M2).
144 146 #[tracing::instrument(skip_all)]
145 - pub async fn get_license_key_by_id(pool: &PgPool, id: LicenseKeyId) -> Result<Option<DbLicenseKey>> {
147 + pub async fn get_license_key_by_id_unchecked(pool: &PgPool, id: LicenseKeyId) -> Result<Option<DbLicenseKey>> {
146 148 let key = sqlx::query_as!(
147 149 DbLicenseKey,
148 150 r#"
M server/src/db/media_files.rs +7 -2
@@ -127,19 +127,24 @@ pub async fn get_by_id(pool: &PgPool, id: MediaFileId) -> Result<Option<DbMediaF
127 127 Ok(row)
128 128 }
129 129
130 - /// Delete a media file by ID.
130 + /// Delete a media file owned by `user_id`. Ownership is scoped IN the SQL
131 + /// (`WHERE id = $1 AND user_id = $2`) so the delete can't touch another user's
132 + /// row even if a caller forgot to pre-check (Sec-M2); returns `None` when the row
133 + /// doesn't exist or isn't owned by `user_id`.
131 134 #[tracing::instrument(skip_all)]
132 135 pub async fn delete<'e>(
133 136 executor: impl sqlx::PgExecutor<'e>,
134 137 id: MediaFileId,
138 + user_id: UserId,
135 139 ) -> Result<Option<DbMediaFile>> {
136 140 let row = sqlx::query_as!(
137 141 DbMediaFile,
138 - r#"DELETE FROM media_files WHERE id = $1
142 + r#"DELETE FROM media_files WHERE id = $1 AND user_id = $2
139 143 RETURNING id AS "id: MediaFileId", user_id AS "user_id: UserId", folder, filename,
140 144 s3_key, content_type, file_size_bytes, media_type, scan_status,
141 145 created_at AS "created_at: chrono::DateTime<chrono::Utc>""#,
142 146 id as MediaFileId,
147 + user_id as UserId,
143 148 )
144 149 .fetch_optional(executor)
145 150 .await?;
M server/src/db/oauth.rs +27 -2
@@ -285,9 +285,33 @@ pub async fn record_granted_scopes(
285 285 app_id: SyncAppId,
286 286 scope: &crate::oauth_scope::GrantedScopes,
287 287 ) -> Result<()> {
288 - let mut merged = get_granted_scopes(pool, user_id, app_id).await?;
288 + let mut tx = pool.begin().await?;
289 +
290 + // Serialize concurrent consent recordings for this (user, app) so the
291 + // read-modify-write union below can't lose a just-granted scope to a lost
292 + // update (Sec-M1). Keyed on the pair, so it only contends with this user's
293 + // own concurrent consents for this app and auto-releases at commit. Mirrors
294 + // the per-reporter lock in db::reports::create_report_within_daily_limit.
295 + sqlx::query("SELECT pg_advisory_xact_lock(hashtextextended($1::text || ':' || $2::text, 0))")
296 + .bind(user_id)
297 + .bind(app_id)
298 + .execute(&mut *tx)
299 + .await?;
300 +
301 + // Read the standing grant INSIDE the lock + transaction.
302 + let existing: Option<String> = sqlx::query_scalar(
303 + "SELECT scopes FROM oauth_granted_scopes WHERE user_id = $1 AND app_id = $2",
304 + )
305 + .bind(user_id)
306 + .bind(app_id)
307 + .fetch_optional(&mut *tx)
308 + .await?;
309 + let mut merged = existing
310 + .map(|s| crate::oauth_scope::GrantedScopes::parse(&s))
311 + .unwrap_or_default();
289 312 merged.union_with(scope);
290 313 let scopes = merged.to_string();
314 +
291 315 sqlx::query!(
292 316 r#"
293 317 INSERT INTO oauth_granted_scopes (user_id, app_id, scopes)
@@ -299,7 +323,8 @@ pub async fn record_granted_scopes(
299 323 app_id as SyncAppId,
300 324 scopes,
301 325 )
302 - .execute(pool)
326 + .execute(&mut *tx)
303 327 .await?;
328 + tx.commit().await?;
304 329 Ok(())
305 330 }
M server/src/email/tokens.rs +7 -38
@@ -138,16 +138,9 @@ pub fn verify_login_token(token: &str, stored_hash: &str) -> bool {
138 138 hasher.update(token.as_bytes());
139 139 let computed_hash = hex::encode(hasher.finalize());
140 140
141 - // Constant-time comparison
142 - if computed_hash.len() != stored_hash.len() {
143 - return false;
144 - }
145 -
146 - let mut result = 0u8;
147 - for (a, b) in computed_hash.bytes().zip(stored_hash.bytes()) {
148 - result |= a ^ b;
149 - }
150 - result == 0
141 + // Constant-time comparison via the shared helper (Sec-M3); it also handles the
142 + // length-mismatch case, so no separate early-return length check is needed.
143 + crate::helpers::constant_time_compare(&computed_hash, stored_hash)
151 144 }
152 145
153 146 /// Verify email verification signature
@@ -173,15 +166,7 @@ pub fn verify_email_signature(
173 166 mac.update(message.as_bytes());
174 167
175 168 let expected = hex::encode(mac.finalize().into_bytes());
176 - if expected.len() != signature.len() {
177 - return false;
178 - }
179 -
180 - let mut result = 0u8;
181 - for (a, b) in expected.bytes().zip(signature.bytes()) {
182 - result |= a ^ b;
183 - }
184 - result == 0
169 + crate::helpers::constant_time_compare(&expected, signature)
185 170 }
186 171
187 172 /// Generate an HMAC-signed unsubscribe URL.
@@ -231,16 +216,7 @@ pub fn verify_unsubscribe_signature(
231 216 mac.update(message.as_bytes());
232 217
233 218 let expected = hex::encode(mac.finalize().into_bytes());
234 -
235 - // Constant-time comparison
236 - if expected.len() != signature.len() {
237 - return false;
238 - }
239 - let mut result = 0u8;
240 - for (a, b) in expected.bytes().zip(signature.bytes()) {
241 - result |= a ^ b;
242 - }
243 - result == 0
219 + crate::helpers::constant_time_compare(&expected, signature)
244 220 }
245 221
246 222 /// Generate account deletion URL
@@ -334,15 +310,8 @@ pub fn parse_issue_reply_token(
334 310 let hash = mac.finalize().into_bytes();
335 311 let expected = &URL_SAFE_NO_PAD.encode(&hash[..12])[..16];
336 312
337 - // Constant-time comparison
338 - if expected.len() != sig.len() {
339 - return None;
340 - }
341 - let mut result = 0u8;
342 - for (a, b) in expected.bytes().zip(sig.bytes()) {
343 - result |= a ^ b;
344 - }
345 - if result != 0 {
313 + // Constant-time comparison via the shared helper (Sec-M3).
314 + if !crate::helpers::constant_time_compare(expected, sig) {
346 315 return None;
347 316 }
348 317
M server/src/routes/api/guest_checkout.rs +9
@@ -260,6 +260,15 @@ pub(super) async fn guest_download(
260 260 .await?
261 261 .ok_or(AppError::NotFound)?;
262 262
263 + // Gate on scan status, exactly as the authenticated download path does
264 + // (downloads.rs). A guest is never the creator, so there is no preview
265 + // exemption: only Clean content is downloadable — Pending/HeldForReview/
266 + // Quarantined are withheld until the scan clears (Sec NOTE, Run 7; the guest
267 + // path previously skipped this quarantine check entirely).
268 + if item.scan_status != db::FileScanStatus::Clean {
269 + return Err(AppError::NotFound);
270 + }
271 +
263 272 // Get the S3 key for the content
264 273 let s3_key = item.audio_s3_key.as_deref()
265 274 .or(item.video_s3_key.as_deref())
M server/src/routes/api/internal/content.rs +1 -1
@@ -481,7 +481,7 @@ pub(super) async fn revoke_license_key(
481 481 Path(key_id): Path<LicenseKeyId>,
482 482 Json(req): Json<RevokeKeyRequest>,
483 483 ) -> Result<impl IntoResponse> {
484 - let key = db::license_keys::get_license_key_by_id(&state.db, key_id)
484 + let key = db::license_keys::get_license_key_by_id_unchecked(&state.db, key_id)
485 485 .await?
486 486 .ok_or(AppError::NotFound)?;
487 487
M server/src/routes/api/license_keys.rs +1 -1
@@ -453,7 +453,7 @@ pub(super) async fn revoke_key(
453 453 ) -> Result<Response> {
454 454 user.check_not_suspended()?;
455 455
456 - let key = db::license_keys::get_license_key_by_id(&state.db, key_id)
456 + let key = db::license_keys::get_license_key_by_id_unchecked(&state.db, key_id)
457 457 .await?
458 458 .ok_or(AppError::NotFound)?;
459 459
M server/src/routes/git/raw.rs +14 -5
@@ -160,17 +160,26 @@ pub(super) async fn smart_http_info_refs(
160 160 }
161 161
162 162 /// Authorize a push (receive-pack) for the resolved principal. Requires a
163 - /// principal (no anonymous push); a token-authed principal must carry push
164 - /// scope (`token_push == Some(true)`); and the user must be the repo owner or a
165 - /// push-collaborator — the same write model SSH enforces in `git_ssh.rs`.
163 + /// personal access token carrying push scope (`token_push == Some(true)`), and
164 + /// the user must be the repo owner or a push-collaborator — the same write model
165 + /// SSH enforces in `git_ssh.rs`.
166 + ///
167 + /// A session cookie (`token_push == None`) is rejected for push (UX-S1): browsers
168 + /// never run git-receive-pack, and these routes are merged OUTSIDE the
169 + /// `CsrfRouter`/`origin_gate` tree, so accepting a cookie here would let a
170 + /// cross-origin POST drive a write with only git wire-format + CORS-preflight
171 + /// friction standing in for the CSRF seal the rest of the mutating surface has.
172 + /// The git CLI always authenticates over HTTP with a PAT, so requiring one does
173 + /// not regress any real push path.
166 174 async fn authorize_push(
167 175 state: &AppState,
168 176 resolved: &super::ResolvedRepo,
169 177 principal: Option<&super::GitHttpPrincipal>,
170 178 ) -> Result<()> {
171 179 let principal = principal.ok_or(AppError::Unauthorized)?;
172 - if principal.token_push == Some(false) {
173 - // Authenticated by a read-only token.
180 + if principal.token_push != Some(true) {
181 + // Reject session-cookie auth (None) and read-only tokens (Some(false)):
182 + // only a push-scoped PAT may push over smart-HTTP.
174 183 return Err(AppError::Forbidden);
175 184 }
176 185 let is_owner = resolved.db_user.id == principal.user_id;
M server/src/routes/storage/media.rs +1 -1
@@ -488,7 +488,7 @@ pub(super) async fn media_delete(
488 488 // delete_version adopted). The refund + enqueue use the DELETE's own returned
489 489 // row (`deleted`), not the pre-tx `get_by_id` read.
490 490 let mut tx = state.db.begin().await?;
491 - let deleted = db::media_files::delete(&mut *tx, id).await?;
491 + let deleted = db::media_files::delete(&mut *tx, id, user.id).await?;
492 492 if let Some(ref row) = deleted {
493 493 db::creator_tiers::decrement_storage_used(&mut *tx, user.id, row.file_size_bytes).await?;
494 494 db::pending_s3_deletions::enqueue_deletions(
M server/src/scanning/archive.rs +61 -7
@@ -70,18 +70,35 @@ fn detect_kind(magic: &[u8]) -> Option<ArchiveKind> {
70 70 /// (7z, RAR). Returns the container label so the caller can reject them where
71 71 /// they aren't a legitimate download payload, rather than passing them through
72 72 /// on the ClamAV (FailOpen) layer alone (ultra-fuzz Run 6 R6-Sec-L2).
73 - fn detect_unsupported_container(magic: &[u8]) -> Option<&'static str> {
74 - if magic.starts_with(&[0x37, 0x7A, 0xBC, 0xAF, 0x27, 0x1C]) {
73 + ///
74 + /// Scans the WHOLE buffer for the signature, not just offset 0 (Sec-S1, Run 7):
75 + /// a prefixed/polyglot container — e.g. `[PNG header][7z payload]` — sniffs as an
76 + /// image (passing the content-type layer) and would otherwise carry its 7z magic
77 + /// past offset 0, skipping this rejection and falling through to ClamAV (FailOpen)
78 + /// alone. This mirrors `has_zip_eocd`'s window scan for prefixed ZIPs. The 6-byte
79 + /// signatures (with the unusual `BC AF` / `1A 07` bytes) make a false match on
80 + /// benign image data astronomically unlikely; a false positive only costs a
81 + /// held-for-review, never a bypass. The caller gates this on `file_type` so the
82 + /// scan never runs over a large Download buffer.
83 + fn detect_unsupported_container(data: &[u8]) -> Option<&'static str> {
84 + // 7z: "7z\xBC\xAF\x27\x1C".
85 + const SEVENZ: &[u8] = &[0x37, 0x7A, 0xBC, 0xAF, 0x27, 0x1C];
86 + // RAR4 ("Rar!\x1a\x07\x00") and RAR5 ("Rar!\x1a\x07\x01\x00") share this prefix.
87 + const RAR: &[u8] = &[0x52, 0x61, 0x72, 0x21, 0x1A, 0x07];
88 + if contains_window(data, SEVENZ) {
75 89 Some("7z")
76 - } else if magic.starts_with(&[0x52, 0x61, 0x72, 0x21, 0x1A, 0x07]) {
77 - // RAR4 ("Rar!\x1a\x07\x00") and RAR5 ("Rar!\x1a\x07\x01\x00") share this
78 - // 6-byte prefix.
90 + } else if contains_window(data, RAR) {
79 91 Some("RAR")
80 92 } else {
81 93 None
82 94 }
83 95 }
84 96
97 + /// True if `needle` appears anywhere in `haystack`.
98 + fn contains_window(haystack: &[u8], needle: &[u8]) -> bool {
99 + haystack.windows(needle.len()).any(|w| w == needle)
100 + }
101 +
85 102 /// ZIP end-of-central-directory signature (`PK\x05\x06`). A valid ZIP always
86 103 /// ends with this record (plus an optional trailing comment), even when bytes
87 104 /// are prepended ahead of the first local header (self-extracting archives).
@@ -196,8 +213,10 @@ pub fn inspect_archive(
196 213 // through to detect_kind == None and rely on ClamAV (FailOpen) alone. Reject
197 214 // them (FailClosed → held for review) anywhere they aren't a legitimate
198 215 // download payload; Download keeps the ClamAV backstop (R6-Sec-L2).
199 - if let Some(container) = detect_unsupported_container(data)
200 - && file_type != FileType::Download
216 + // Gate on file_type FIRST so the whole-buffer signature scan never runs over a
217 + // large Download buffer (Download keeps the ClamAV backstop, R6-Sec-L2).
218 + if file_type != FileType::Download
219 + && let Some(container) = detect_unsupported_container(data)
201 220 {
202 221 let detail = format!(
203 222 "{container} archives are not accepted for this upload type (cannot be decompression-bomb inspected)"
@@ -1355,4 +1374,39 @@ mod tests {
1355 1374 let result = check_archive_safety(&data, FileType::Download);
1356 1375 assert_eq!(result.verdict, LayerVerdict::Fail, "detail: {:?}", result.detail);
1357 1376 }
1377 +
1378 + #[test]
1379 + fn prefixed_7z_polyglot_rejected_for_non_download() {
1380 + // [PNG header][7z magic][junk]: sniffs as a PNG (passing the content-type
1381 + // layer) but carries a 7z payload past offset 0. The offset-0-only magic
1382 + // check missed this and fell through to ClamAV FailOpen (Sec-S1); the
1383 + // whole-buffer window scan now rejects it for non-Download uploads.
1384 + let mut data = vec![0x89, b'P', b'N', b'G', 0x0D, 0x0A, 0x1A, 0x0A];
1385 + data.extend_from_slice(&[0x37, 0x7A, 0xBC, 0xAF, 0x27, 0x1C]); // 7z magic
1386 + data.extend_from_slice(&[0u8; 64]);
1387 +
1388 + // Cover / image: held for review (Error), not passed to ClamAV alone.
1389 + let cover = check_archive_safety(&data, FileType::Cover);
1390 + assert_eq!(cover.verdict, LayerVerdict::Error, "detail: {:?}", cover.detail);
1391 + assert!(cover.detail.unwrap().contains("7z"));
1392 +
1393 + // Download keeps the ClamAV backstop — this layer must not reject it.
1394 + let download = check_archive_safety(&data, FileType::Download);
1395 + assert_ne!(
1396 + download.verdict,
1397 + LayerVerdict::Error,
1398 + "Download must keep the ClamAV backstop, not be rejected by the container check"
1399 + );
1400 + }
1401 +
1402 + #[test]
1403 + fn prefixed_rar_polyglot_rejected_for_non_download() {
1404 + // Same evasion shape with a RAR signature embedded after a JPEG header.
1405 + let mut data = vec![0xFF, 0xD8, 0xFF, 0xE0]; // JPEG SOI + APP0
1406 + data.extend_from_slice(&[0x52, 0x61, 0x72, 0x21, 0x1A, 0x07]); // "Rar!\x1a\x07"
1407 + data.extend_from_slice(&[0u8; 64]);
1408 + let cover = check_archive_safety(&data, FileType::Cover);
1409 + assert_eq!(cover.verdict, LayerVerdict::Error, "detail: {:?}", cover.detail);
1410 + assert!(cover.detail.unwrap().contains("RAR"));
1411 + }
1358 1412 }
M server/tests/workflows/git_browser.rs +41
@@ -765,6 +765,47 @@ async fn git_push_token_gets_receive_pack_advertisement() {
765 765 assert!(resp.text.contains("# service=git-receive-pack"), "missing receive-pack banner: {}", resp.text);
766 766 }
767 767
768 + // UX-S1: git push (receive-pack) must reject session-cookie auth, even for the
769 + // repo OWNER. These routes are merged outside the CsrfRouter/origin_gate tree, so
770 + // a cookie-authed push would be drivable cross-origin with only git wire-format
771 + // friction. Push now requires a push-scoped PAT; the owner's browser session does
772 + // not authorize a write.
773 + #[tokio::test]
774 + async fn git_session_cookie_cannot_push() {
775 + let tmp = tempfile::TempDir::new().unwrap();
776 + make_test_repo(tmp.path());
777 + let mut h = setup_git_harness(&tmp).await; // signs up + logs in testowner
778 +
779 + h.client.get("/git/testowner/testrepo").await; // auto-register
780 +
781 + // Owner is logged in (session cookie tracked by the client) and CAN read.
782 + h.login("testowner", "password123").await;
783 +
784 + // Read advertisement (upload-pack) works with the cookie — owner has read.
785 + let upload = "/git/testowner/testrepo.git/info/refs?service=git-upload-pack";
786 + let resp = h.client.get(upload).await;
787 + assert!(resp.status.is_success(), "owner cookie should read: {} {}", resp.status, resp.text);
788 +
789 + // Push advertisement (receive-pack) must be REFUSED for cookie auth — no PAT.
790 + let recv = "/git/testowner/testrepo.git/info/refs?service=git-receive-pack";
791 + let resp = h.client.get(recv).await;
792 + assert_eq!(
793 + resp.status, 403,
794 + "a session cookie must not authorize git push, even for the owner (UX-S1): {} {}",
795 + resp.status, resp.text
796 + );
797 +
798 + // The receive-pack POST (the actual push) is likewise refused for cookie auth.
799 + let recv_post = "/git/testowner/testrepo.git/git-receive-pack";
800 + let resp = h.client.request_with_headers("POST", recv_post, Some("0000"),
801 + &[("Content-Type", "application/x-git-receive-pack-request")]).await;
802 + assert_eq!(
803 + resp.status, 403,
804 + "cookie-authed receive-pack POST must be refused (UX-S1): {} {}",
805 + resp.status, resp.text
806 + );
807 + }
808 +
768 809 // ── Smart HTTP (git clone) ──
769 810
770 811 /// Tip commit sha of refs/heads/main from the on-disk bare repo.
M shared/docengine/src/sanitize.rs +60 -1
@@ -12,10 +12,25 @@ pub enum SanitizePreset {
12 12 Minimal,
13 13 }
14 14
15 + /// The single in-repo authority for permissive (creator long-form) sanitization.
16 + ///
17 + /// `ammonia::clean` is exactly `Builder::default().clean()`, so the XSS guarantee
18 + /// for all creator markdown rested implicitly on ammonia's evolving defaults with
19 + /// nothing in-repo pinning it (UX-S3). Routing through a named builder gives one
20 + /// explicit construction point, and the regression tests in this module pin the
21 + /// guarantee — `<script>`/`<iframe>`/`<style>`/`on*` handlers/`javascript:` URLs
22 + /// must stay stripped — so a floating ammonia minor that weakened a default would
23 + /// fail CI here rather than silently opening an injection hole.
24 + pub(crate) fn permissive_builder() -> ammonia::Builder<'static> {
25 + ammonia::Builder::default()
26 + }
27 +
15 28 impl SanitizePreset {
16 29 pub(crate) fn clean(&self, html: &str) -> String {
17 30 match self {
18 - SanitizePreset::Permissive | SanitizePreset::Standard => ammonia::clean(html),
31 + SanitizePreset::Permissive | SanitizePreset::Standard => {
32 + permissive_builder().clean(html).to_string()
33 + }
19 34 SanitizePreset::Strict => ammonia::Builder::default()
20 35 .link_rel(Some("noopener noreferrer nofollow"))
21 36 .clean(html)
@@ -77,4 +92,48 @@ mod tests {
77 92 assert!(result.contains("<code>"));
78 93 assert!(result.contains("<pre>"));
79 94 }
95 +
96 + // UX-S3: pin the permissive XSS guarantee so a future ammonia default change
97 + // can't silently weaken it. These cover the high-value injection vectors that
98 + // all creator long-form content relies on being stripped.
99 +
100 + #[test]
101 + fn permissive_strips_iframe_and_srcdoc() {
102 + let html = r#"<p>x</p><iframe srcdoc="<script>alert(1)</script>"></iframe>"#;
103 + let result = SanitizePreset::Permissive.clean(html);
104 + assert!(!result.contains("<iframe"), "iframe must be stripped: {result}");
105 + assert!(!result.contains("srcdoc"), "srcdoc must be stripped: {result}");
106 + assert!(!result.contains("<script"), "script must be stripped: {result}");
107 + }
108 +
109 + #[test]
110 + fn permissive_strips_style_element() {
111 + let html = "<style>body{background:url(javascript:alert(1))}</style><p>x</p>";
112 + let result = SanitizePreset::Permissive.clean(html);
113 + assert!(!result.contains("<style"), "style element must be stripped: {result}");
114 + }
115 +
116 + #[test]
117 + fn permissive_strips_event_handlers() {
118 + let html = r#"<p onclick="alert(1)" onmouseover="alert(2)">x</p><img src="x" onerror="alert(3)">"#;
119 + let result = SanitizePreset::Permissive.clean(html);
120 + assert!(!result.contains("onclick"), "onclick must be stripped: {result}");
121 + assert!(!result.contains("onmouseover"), "onmouseover must be stripped: {result}");
122 + assert!(!result.contains("onerror"), "onerror must be stripped: {result}");
123 + }
124 +
125 + #[test]
126 + fn permissive_strips_javascript_url() {
127 + let html = r#"<a href="javascript:alert(1)">click</a>"#;
128 + let result = SanitizePreset::Permissive.clean(html);
129 + assert!(!result.contains("javascript:"), "javascript: URL must be stripped: {result}");
130 + }
131 +
132 + #[test]
133 + fn permissive_strips_object_and_embed() {
134 + let html = r#"<object data="evil.swf"></object><embed src="evil.swf">"#;
135 + let result = SanitizePreset::Permissive.clean(html);
136 + assert!(!result.contains("<object"), "object must be stripped: {result}");
137 + assert!(!result.contains("<embed"), "embed must be stripped: {result}");
138 + }
80 139 }