yuzpxzopsouqmerged#6
fix: 500 error because of database is patched
Mdocker/Dockerfile+8−3
| @@ −66,10 +66,15 @@ | |||
| 66 | 66 | # Installed into each repository's hooks/ directory at creation. | |
| 67 | 67 | COPY --from=builder /build/target/release/dogfood-hook /usr/local/bin/dogfood-hook | |
| 68 | 68 | EXPOSE 8080 | |
| 69 | − | # The container is healthy when it can serve; readiness (database reachable) is | |
| 70 | − | # checked separately by the orchestrator via /readyz. | |
| 69 | + | # /readyz, not /healthz: /healthz is an unconditional 200, so it reported this | |
| 70 | + | # container healthy through the two hours of 2026-08-03 in which every request | |
| 71 | + | # 500ed on an exhausted connection pool. Docker does not act on this status by | |
| 72 | + | # itself — compose restarts on exit, not on unhealthy — so this buys visibility, | |
| 73 | + | # not recovery. The 5s timeout is deliberately below the pool's 10s | |
| 74 | + | # acquire_timeout, so a pool that can no longer hand out connections fails the | |
| 75 | + | # check instead of slowly answering it. | |
| 71 | 76 | HEALTHCHECK --interval=30s --timeout=5s --start-period=20s --retries=3 \ | |
| 72 | − | CMD wget -qO- http://127.0.0.1:8080/healthz >/dev/null || exit 1 | |
| 77 | + | CMD wget -qO- http://127.0.0.1:8080/readyz >/dev/null || exit 1 | |
| 73 | 78 | ENTRYPOINT ["/usr/local/bin/dogfood-web"] | |
| 74 | 79 | ||
| 75 | 80 | # ─── worker ────────────────────────────────────────────────────────────────── | |
Mcrates/df-auth/src/session.rs+142−29
| @@ −3,17 +3,67 @@ | |||
| 3 | 3 | //! "Sessions are server-side rows with an opaque ID in a cookie: HttpOnly, | |
| 4 | 4 | //! Secure, SameSite=Lax, 14-day sliding expiry. Store nothing in the cookie but | |
| 5 | 5 | //! the ID." | |
| 6 | + | //! | |
| 7 | + | //! The cookie carries a [`generate_token`] value — 32 bytes from the OS CSPRNG, | |
| 8 | + | //! hex-encoded — and never the row's `id`. The two are deliberately different | |
| 9 | + | //! things: `id` is a UUIDv7, which is time-ordered and carries a counter that | |
| 10 | + | //! only reseeds once per millisecond, so ids minted close together share their | |
| 11 | + | //! leading bits and any *other* id from the same generator (a comment id, say, | |
| 12 | + | //! which is rendered into review pages) narrows the search space for the rest. | |
| 13 | + | //! That is fine for a primary key and disqualifying for a bearer secret. | |
| 14 | + | //! | |
| 15 | + | //! Only the SHA-256 of the token is stored, so a leaked dump of this table | |
| 16 | + | //! contains nothing that can be presented back to the server. SHA-256 rather | |
| 17 | + | //! than Argon2 on purpose: the token has 256 bits of entropy, so there is no | |
| 18 | + | //! guessing attack for a slow hash to frustrate, and this runs on every request. | |
| 6 | 19 | ||
| 7 | 20 | use anyhow::Result; | |
| 8 | 21 | use chrono::{DateTime, Duration, Utc}; | |
| 9 | 22 | use df_db::ids::new_id; | |
| 10 | 23 | use df_db::models::{Session, User}; | |
| 24 | + | use rand::RngCore; | |
| 25 | + | use sha2::{Digest, Sha256}; | |
| 11 | 26 | use sqlx::PgPool; | |
| 12 | 27 | use uuid::Uuid; | |
| 13 | 28 | ||
| 14 | 29 | pub const COOKIE_NAME: &str = "dogfood_session"; | |
| 15 | 30 | ||
| 16 | − | /// Create a session row and return its id, which becomes the cookie value. | |
| 31 | + | /// Entropy in a session token. 32 bytes is the usual bar for a bearer secret | |
| 32 | + | /// and leaves no meaningful margin for a birthday collision across the table. | |
| 33 | + | const TOKEN_BYTES: usize = 32; | |
| 34 | + | ||
| 35 | + | /// Length of the hex form, which is what the cookie carries. | |
| 36 | + | const TOKEN_HEX_LEN: usize = TOKEN_BYTES * 2; | |
| 37 | + | ||
| 38 | + | /// A freshly created session. The token exists only here and in the cookie. | |
| 39 | + | pub struct NewSession { | |
| 40 | + | pub id: Uuid, | |
| 41 | + | /// The cookie value. Not recoverable from the database afterwards. | |
| 42 | + | pub token: String, | |
| 43 | + | pub expires_at: DateTime<Utc>, | |
| 44 | + | } | |
| 45 | + | ||
| 46 | + | /// Mint a session token. | |
| 47 | + | pub fn generate_token() -> String { | |
| 48 | + | let mut bytes = [0u8; TOKEN_BYTES]; | |
| 49 | + | rand::thread_rng().fill_bytes(&mut bytes); | |
| 50 | + | hex::encode(bytes) | |
| 51 | + | } | |
| 52 | + | ||
| 53 | + | /// What the database stores in place of the token. | |
| 54 | + | fn hash_token(token: &str) -> String { | |
| 55 | + | hex::encode(Sha256::digest(token.as_bytes())) | |
| 56 | + | } | |
| 57 | + | ||
| 58 | + | /// Reject anything that is not shaped like one of our tokens. | |
| 59 | + | /// | |
| 60 | + | /// Cheap, and it means a request carrying a junk cookie — the shape a flood | |
| 61 | + | /// takes — is refused before it costs a database round trip. | |
| 62 | + | fn well_formed(token: &str) -> bool { | |
| 63 | + | token.len() == TOKEN_HEX_LEN && token.bytes().all(|b| b.is_ascii_hexdigit()) | |
| 64 | + | } | |
| 65 | + | ||
| 66 | + | /// Create a session row and return the token that addresses it. | |
| 17 | 67 | pub async fn create( | |
| 18 | 68 | db: &PgPool, | |
| 19 | 69 | user_id: Uuid, | |
| @@ −21,16 +71,18 @@ | |||
| 21 | 71 | user_agent: Option<&str>, | |
| 22 | 72 | ip: Option<std::net::IpAddr>, | |
| 23 | 73 | id_token: Option<&str>, | |
| 24 | − | ) -> Result<(Uuid, DateTime<Utc>)> { | |
| 74 | + | ) -> Result<NewSession> { | |
| 25 | 75 | let id = new_id(); | |
| 76 | + | let token = generate_token(); | |
| 26 | 77 | let expires_at = Utc::now() + Duration::days(ttl_days); | |
| 27 | 78 | ||
| 28 | 79 | sqlx::query( | |
| 29 | − | "INSERT INTO sessions (id, user_id, expires_at, user_agent, ip, id_token) | |
| 30 | − | VALUES ($1, $2, $3, $4, $5, $6)", | |
| 80 | + | "INSERT INTO sessions (id, user_id, token_hash, expires_at, user_agent, ip, id_token) | |
| 81 | + | VALUES ($1, $2, $3, $4, $5, $6, $7)", | |
| 31 | 82 | ) | |
| 32 | 83 | .bind(id) | |
| 33 | 84 | .bind(user_id) | |
| 85 | + | .bind(hash_token(&token)) | |
| 34 | 86 | .bind(expires_at) | |
| 35 | 87 | // Truncated: a hostile client can send a multi-kilobyte User-Agent and | |
| 36 | 88 | // there is no reason to store it. | |
| @@ −40,36 +92,26 @@ | |||
| 40 | 92 | .execute(db) | |
| 41 | 93 | .await?; | |
| 42 | 94 | ||
| 43 | − | Ok((id, expires_at)) | |
| 44 | − | } | |
| 45 | − | ||
| 46 | − | /// The ID token stashed at login, for `id_token_hint` on RP-Initiated Logout. | |
| 47 | − | /// | |
| 48 | − | /// `None` both when the session predates this column and when the login that | |
| 49 | − | /// created it had no token to store (the pending-handle path) — callers treat | |
| 50 | − | /// both the same way: fall back to clearing the local session only. | |
| 51 | − | pub async fn id_token(db: &PgPool, session_id: Uuid) -> Result<Option<String>> { | |
| 52 | − | let row: Option<(Option<String>,)> = | |
| 53 | − | sqlx::query_as("SELECT id_token FROM sessions WHERE id = $1") | |
| 54 | − | .bind(session_id) | |
| 55 | − | .fetch_optional(db) | |
| 56 | − | .await?; | |
| 57 | − | Ok(row.and_then(|r| r.0)) | |
| 95 | + | Ok(NewSession { id, token, expires_at }) | |
| 58 | 96 | } | |
| 59 | 97 | ||
| 60 | − | /// Load the user for a session id, if the session exists and has not expired. | |
| 98 | + | /// Load the user for a session token, if the session exists and has not expired. | |
| 61 | 99 | /// | |
| 62 | 100 | /// Returns the session so the caller can decide whether to slide the expiry. | |
| 63 | − | pub async fn load(db: &PgPool, session_id: Uuid) -> Result<Option<(Session, User)>> { | |
| 101 | + | pub async fn load(db: &PgPool, token: &str) -> Result<Option<(Session, User)>> { | |
| 102 | + | if !well_formed(token) { | |
| 103 | + | return Ok(None); | |
| 104 | + | } | |
| 105 | + | ||
| 64 | 106 | let row = sqlx::query_as::<_, (Uuid, Uuid, DateTime<Utc>, Uuid, String, String, Option<String>, Option<String>, Option<String>, bool, DateTime<Utc>)>( | |
| 65 | 107 | "SELECT s.id, s.user_id, s.expires_at, | |
| 66 | 108 | u.id, u.subject, u.handle, u.display_name, u.email, u.avatar_url, | |
| 67 | 109 | u.is_admin, u.created_at | |
| 68 | 110 | FROM sessions s | |
| 69 | 111 | JOIN users u ON u.id = s.user_id | |
| 70 | − | WHERE s.id = $1 AND s.expires_at > now()", | |
| 112 | + | WHERE s.token_hash = $1 AND s.expires_at > now()", | |
| 71 | 113 | ) | |
| 72 | − | .bind(session_id) | |
| 114 | + | .bind(hash_token(token)) | |
| 73 | 115 | .fetch_optional(db) | |
| 74 | 116 | .await?; | |
| 75 | 117 | ||
| @@ −113,12 +155,26 @@ | |||
| 113 | 155 | remaining < Duration::days(ttl_days) / 2 | |
| 114 | 156 | } | |
| 115 | 157 | ||
| 116 | − | pub async fn destroy(db: &PgPool, session_id: Uuid) -> Result<()> { | |
| 117 | − | sqlx::query("DELETE FROM sessions WHERE id = $1") | |
| 118 | − | .bind(session_id) | |
| 119 | − | .execute(db) | |
| 120 | − | .await?; | |
| 121 | − | Ok(()) | |
| 158 | + | /// Destroy a session, returning the ID token stashed at login. | |
| 159 | + | /// | |
| 160 | + | /// One statement rather than a read followed by a delete: logout needs the | |
| 161 | + | /// token for `id_token_hint` on RP-Initiated Logout, and doing it atomically | |
| 162 | + | /// means two logouts racing cannot both believe they ended the session. | |
| 163 | + | /// | |
| 164 | + | /// `None` covers every "nothing to hand the provider" case — an unknown token, | |
| 165 | + | /// a session created down the pending-handle path with no token to store — and | |
| 166 | + | /// callers treat them the same way: clear the local session and go home. | |
| 167 | + | pub async fn destroy(db: &PgPool, token: &str) -> Result<Option<String>> { | |
| 168 | + | if !well_formed(token) { | |
| 169 | + | return Ok(None); | |
| 170 | + | } | |
| 171 | + | ||
| 172 | + | let row: Option<(Option<String>,)> = | |
| 173 | + | sqlx::query_as("DELETE FROM sessions WHERE token_hash = $1 RETURNING id_token") | |
| 174 | + | .bind(hash_token(token)) | |
| 175 | + | .fetch_optional(db) | |
| 176 | + | .await?; | |
| 177 | + | Ok(row.and_then(|r| r.0)) | |
| 122 | 178 | } | |
| 123 | 179 | ||
| 124 | 180 | /// Invalidate every session for a user. Used when an account is disabled. | |
| @@ −159,4 +215,61 @@ | |||
| 159 | 215 | // should_slide in practice; asserted so the interaction stays obvious. | |
| 160 | 216 | assert!(should_slide(Utc::now() - Duration::days(1), 14)); | |
| 161 | 217 | } | |
| 218 | + | ||
| 219 | + | // ─── the token is a bearer secret, not an identifier ───────────────────── | |
| 220 | + | ||
| 221 | + | #[test] | |
| 222 | + | fn tokens_are_full_length_hex_and_unique() { | |
| 223 | + | use std::collections::HashSet; | |
| 224 | + | ||
| 225 | + | let set: HashSet<String> = (0..1000).map(|_| generate_token()).collect(); | |
| 226 | + | assert_eq!(set.len(), 1000, "tokens must not repeat"); | |
| 227 | + | ||
| 228 | + | for t in &set { | |
| 229 | + | assert_eq!(t.len(), TOKEN_HEX_LEN); | |
| 230 | + | assert!(t.bytes().all(|b| b.is_ascii_hexdigit()), "not hex: {t}"); | |
| 231 | + | assert!(well_formed(t)); | |
| 232 | + | } | |
| 233 | + | } | |
| 234 | + | ||
| 235 | + | #[test] | |
| 236 | + | fn successive_tokens_share_no_prefix() { | |
| 237 | + | // The property a UUIDv7 does not have, and the whole reason for this | |
| 238 | + | // type: consecutive ids from a v7 generator agree in their leading ~96 | |
| 239 | + | // bits, so one of them narrows the search space for its neighbours. | |
| 240 | + | let a = generate_token(); | |
| 241 | + | let b = generate_token(); | |
| 242 | + | let shared = a | |
| 243 | + | .bytes() | |
| 244 | + | .zip(b.bytes()) | |
| 245 | + | .take_while(|(x, y)| x == y) | |
| 246 | + | .count(); | |
| 247 | + | assert!(shared < 8, "tokens share a {shared}-character prefix: {a} / {b}"); | |
| 248 | + | } | |
| 249 | + | ||
| 250 | + | #[test] | |
| 251 | + | fn the_stored_hash_is_not_the_token() { | |
| 252 | + | let t = generate_token(); | |
| 253 | + | let h = hash_token(&t); | |
| 254 | + | assert_ne!(h, t, "storing the token itself defeats the point"); | |
| 255 | + | assert_eq!(h, hash_token(&t), "hashing must be deterministic to look up"); | |
| 256 | + | assert_ne!(h, hash_token(&generate_token())); | |
| 257 | + | } | |
| 258 | + | ||
| 259 | + | #[test] | |
| 260 | + | fn malformed_cookies_are_refused_before_the_database() { | |
| 261 | + | // Each of these would otherwise be a query per request, which is the | |
| 262 | + | // work an unauthenticated flood gets for free. | |
| 263 | + | for bad in [ | |
| 264 | + | "", | |
| 265 | + | "not-hex", | |
| 266 | + | &"a".repeat(TOKEN_HEX_LEN - 1), | |
| 267 | + | &"a".repeat(TOKEN_HEX_LEN + 1), | |
| 268 | + | &"g".repeat(TOKEN_HEX_LEN), | |
| 269 | + | // The old cookie value: a session id must no longer authenticate. | |
| 270 | + | &Uuid::now_v7().to_string(), | |
| 271 | + | ] { | |
| 272 | + | assert!(!well_formed(bad), "must reject {bad:?}"); | |
| 273 | + | } | |
| 274 | + | } | |
| 162 | 275 | } | |
Mcrates/df-ssh/src/repo.rs+21−2
| @@ −133,6 +133,9 @@ | |||
| 133 | 133 | .env("HOME", "/nonexistent") | |
| 134 | 134 | .env("GIT_CONFIG_NOSYSTEM", "1") | |
| 135 | 135 | .env("GIT_TERMINAL_PROMPT", "0") | |
| 136 | + | // So a git that is still running when the runtime tears the task down | |
| 137 | + | // is killed rather than orphaned. | |
| 138 | + | .kill_on_drop(true) | |
| 136 | 139 | // Consumed by the pre-receive hook. | |
| 137 | 140 | .env("DOGFOOD_REPO_ID", repo_id.to_string()) | |
| 138 | 141 | .env("DOGFOOD_PUSHER_ID", user_id.to_string()) | |
| @@ −154,12 +157,22 @@ | |||
| 154 | 157 | let handle = handle.clone(); | |
| 155 | 158 | tokio::spawn(async move { | |
| 156 | 159 | let mut buf = vec![0u8; 32 * 1024]; | |
| 160 | + | // Once the client is gone we keep reading, discarding what we read. | |
| 161 | + | // Stopping instead would leave git blocked writing into a pipe | |
| 162 | + | // nobody drains — and since the reaper below waits on the child, it | |
| 163 | + | // would wait forever. The pack is already being produced; draining | |
| 164 | + | // it costs a copy and lets the process exit. | |
| 165 | + | let mut client_gone = false; | |
| 157 | 166 | loop { | |
| 158 | 167 | match stdout.read(&mut buf).await { | |
| 159 | 168 | Ok(0) | Err(_) => break, | |
| 160 | 169 | Ok(n) => { | |
| 170 | + | if client_gone { | |
| 171 | + | continue; | |
| 172 | + | } | |
| 161 | 173 | if handle.data(channel, buf[..n].to_vec().into()).await.is_err() { | |
| 162 | − | break; | |
| 174 | + | tracing::debug!("ssh client went away mid-stream; draining git"); | |
| 175 | + | client_gone = true; | |
| 163 | 176 | } | |
| 164 | 177 | } | |
| 165 | 178 | } | |
| @@ −171,10 +184,16 @@ | |||
| 171 | 184 | let handle = handle.clone(); | |
| 172 | 185 | tokio::spawn(async move { | |
| 173 | 186 | let mut buf = vec![0u8; 8 * 1024]; | |
| 187 | + | // Drained past a dead channel for the same reason as stdout: a | |
| 188 | + | // full stderr pipe blocks git just as thoroughly as a full stdout. | |
| 189 | + | let mut client_gone = false; | |
| 174 | 190 | loop { | |
| 175 | 191 | match stderr.read(&mut buf).await { | |
| 176 | 192 | Ok(0) | Err(_) => break, | |
| 177 | 193 | Ok(n) => { | |
| 194 | + | if client_gone { | |
| 195 | + | continue; | |
| 196 | + | } | |
| 178 | 197 | // Stream 1 is stderr; this is where the pre-receive | |
| 179 | 198 | // hook's rejection messages reach the user. | |
| 180 | 199 | if handle | |
| @@ −182,7 +201,7 @@ | |||
| 182 | 201 | .await | |
| 183 | 202 | .is_err() | |
| 184 | 203 | { | |
| 185 | − | break; | |
| 204 | + | client_gone = true; | |
| 186 | 205 | } | |
| 187 | 206 | } | |
| 188 | 207 | } | |
Mcrates/df-store/tests/git_store.rs+25−0
| @@ −357,6 +357,31 @@ | |||
| 357 | 357 | assert!(!d.truncated); | |
| 358 | 358 | } | |
| 359 | 359 | ||
| 360 | + | /// The tree walk reports the directories it descends through as well as the | |
| 361 | + | /// files inside them. A directory is not a changed file: it has no content, so | |
| 362 | + | /// it reached the renderer as a hunkless "binary file" and padded the file | |
| 363 | + | /// count of every change that touched a new subdirectory. | |
| 364 | + | #[tokio::test] | |
| 365 | + | async fn a_diff_lists_files_and_not_the_directories_holding_them() { | |
| 366 | + | for case in ["basic", "stack", "renames", "mixed", "hostile"] { | |
| 367 | + | let Some(f) = load(case) else { continue }; | |
| 368 | + | let rev = head(&f).await; | |
| 369 | + | let d = f | |
| 370 | + | .store | |
| 371 | + | .diff_from_parent(f.id, &rev, Default::default()) | |
| 372 | + | .await | |
| 373 | + | .expect("diff"); | |
| 374 | + | ||
| 375 | + | let paths: Vec<&str> = d.files.iter().map(|x| x.path.as_str()).collect(); | |
| 376 | + | for p in &paths { | |
| 377 | + | assert!( | |
| 378 | + | !paths.iter().any(|q| q.starts_with(&format!("{p}/"))), | |
| 379 | + | "{case}: {p:?} is a directory, not a changed file: {paths:#?}" | |
| 380 | + | ); | |
| 381 | + | } | |
| 382 | + | } | |
| 383 | + | } | |
| 384 | + | ||
| 360 | 385 | #[tokio::test] | |
| 361 | 386 | async fn diff_of_a_revision_against_itself_is_empty() { | |
| 362 | 387 | let Some(f) = load("basic") else { return }; | |
Mcrates/df-web/assets/app.css853 lines+673−38
| @@ −2427,6 +2427,64 @@ | |||
| 2427 | 2427 | } | |
| 2428 | 2428 | } | |
| 2429 | 2429 | ||
| 2430 | + | /* ─── one commit ─── */ | |
| 2431 | + | ||
| 2432 | + | /* The header of the commit page: the message, who wrote it, and where the | |
| 2433 | + | commit sits in history. Three bands separated by rules, because they answer | |
| 2434 | + | three different questions and a reader is usually after exactly one. */ | |
| 2435 | + | .commit-meta { | |
| 2436 | + | padding: 0; | |
| 2437 | + | margin-bottom: 10px; | |
| 2438 | + | } | |
| 2439 | + | ||
| 2440 | + | .commit-msg { | |
| 2441 | + | padding: 12px 14px; | |
| 2442 | + | } | |
| 2443 | + | ||
| 2444 | + | .commit-msg h1 { | |
| 2445 | + | margin: 0; | |
| 2446 | + | font-size: var(--text-lg); | |
| 2447 | + | line-height: 1.35; | |
| 2448 | + | } | |
| 2449 | + | ||
| 2450 | + | /* The trailing body of a commit message is preformatted text — wrapped rather | |
| 2451 | + | than scrolled, since a paragraph someone hard-wrapped at 72 columns is still | |
| 2452 | + | prose and should read like it. */ | |
| 2453 | + | .commit-body { | |
| 2454 | + | margin: 8px 0 0; | |
| 2455 | + | font-family: var(--font-mono); | |
| 2456 | + | font-size: var(--text-sm); | |
| 2457 | + | line-height: 1.55; | |
| 2458 | + | color: var(--text-dim); | |
| 2459 | + | white-space: pre-wrap; | |
| 2460 | + | overflow-wrap: anywhere; | |
| 2461 | + | } | |
| 2462 | + | ||
| 2463 | + | .commit-byline, | |
| 2464 | + | .commit-ids { | |
| 2465 | + | display: flex; | |
| 2466 | + | align-items: center; | |
| 2467 | + | gap: 8px; | |
| 2468 | + | flex-wrap: wrap; | |
| 2469 | + | padding: 8px 14px; | |
| 2470 | + | border-top: 1px solid var(--border); | |
| 2471 | + | font-size: var(--text-sm); | |
| 2472 | + | color: var(--text-dim); | |
| 2473 | + | } | |
| 2474 | + | ||
| 2475 | + | .commit-ids { | |
| 2476 | + | background: var(--surface-raised); | |
| 2477 | + | border-radius: 0 0 var(--radius) var(--radius); | |
| 2478 | + | } | |
| 2479 | + | ||
| 2480 | + | .commit-parent { | |
| 2481 | + | font-size: var(--text-xs); | |
| 2482 | + | color: var(--text-faint); | |
| 2483 | + | white-space: nowrap; | |
| 2484 | + | } | |
| 2485 | + | ||
| 2486 | + | .commit-parent:hover { color: var(--action); } | |
| 2487 | + | ||
| 2430 | 2488 | /* The bordered container shared by the file listing, the bookmarks table and | |
| 2431 | 2489 | the README panel. */ | |
| 2432 | 2490 | .filelist { | |
| @@ −2441,10 +2499,9 @@ | |||
| 2441 | 2499 | } | |
| 2442 | 2500 | ||
| 2443 | 2501 | /* Fixed layout is what makes the `max-width: 0` ellipsis trick on | |
| 2444 | − | `.filelist-name` / `.bookmark-title` actually hold: under the default auto | |
| 2445 | − | layout the browser sizes columns from content instead, so the name column | |
| 2446 | − | never truncates and the icon column drifts away from it as the viewport | |
| 2447 | − | narrows. */ | |
| 2502 | + | `.filelist-message` / `.bookmark-title` actually hold: under the default auto | |
| 2503 | + | layout the browser sizes columns from content instead, so a long commit | |
| 2504 | + | message pushes the row wider rather than truncating. */ | |
| 2448 | 2505 | .filelist table { | |
| 2449 | 2506 | width: 100%; | |
| 2450 | 2507 | table-layout: fixed; | |
| @@ −2479,16 +2536,6 @@ | |||
| 2479 | 2536 | padding: 0 8px; | |
| 2480 | 2537 | height: 28px; | |
| 2481 | 2538 | vertical-align: middle; | |
| 2482 | − | } | |
| 2483 | − | ||
| 2484 | − | /* A literal pixel width, not the old auto-layout `1px` shrink-to-content | |
| 2485 | − | hack: `table-layout: fixed` takes column widths at face value, so a hint | |
| 2486 | − | width no longer holds and has to be the icon's real footprint (8px | |
| 2487 | − | padding-left + 16px icon). */ | |
| 2488 | − | .filelist-icon { | |
| 2489 | − | width: 28px; | |
| 2490 | − | padding-right: 0 !important; | |
| 2491 | − | line-height: 0; | |
| 2492 | 2539 | } | |
| 2493 | 2540 | ||
| 2494 | 2541 | .icon-dir { | |
| @@ −2499,39 +2546,67 @@ | |||
| 2499 | 2546 | color: var(--text-faint); | |
| 2500 | 2547 | } | |
| 2501 | 2548 | ||
| 2549 | + | /* The icon shares the name's cell rather than owning a column of its own. | |
| 2550 | + | As its own column it was a fixed 28px, and any slack the table had to hand | |
| 2551 | + | out widened it, so the icon stayed pinned left while the filename started | |
| 2552 | + | further and further right — worst below the breakpoints that drop the | |
| 2553 | + | message and change columns. In one cell the two are always 8px apart. */ | |
| 2502 | 2554 | .filelist-name { | |
| 2503 | − | max-width: 0; | |
| 2504 | − | width: 32%; | |
| 2555 | + | width: calc(32% + 28px); | |
| 2556 | + | } | |
| 2557 | + | ||
| 2558 | + | .filelist-entry { | |
| 2559 | + | display: flex; | |
| 2560 | + | align-items: center; | |
| 2561 | + | min-width: 0; | |
| 2505 | 2562 | } | |
| 2506 | 2563 | ||
| 2507 | − | .filelist-name a { | |
| 2564 | + | .filelist-entry-link { | |
| 2565 | + | display: flex; | |
| 2566 | + | align-items: center; | |
| 2567 | + | gap: 10px; | |
| 2568 | + | min-width: 0; | |
| 2508 | 2569 | color: var(--text); | |
| 2570 | + | } | |
| 2571 | + | ||
| 2572 | + | .filelist-entry-link svg { | |
| 2573 | + | flex: none; | |
| 2574 | + | } | |
| 2575 | + | ||
| 2576 | + | /* `line-height: 1` here, not just on `.filelist-entry-link`: the body default | |
| 2577 | + | of 1.5 gives the name span extra leading that JetBrains Mono splits | |
| 2578 | + | unevenly above/below the glyphs, so the text visibly hangs below the | |
| 2579 | + | icon's center instead of sitting beside it. */ | |
| 2580 | + | .filelist-entry-name { | |
| 2509 | 2581 | overflow: hidden; | |
| 2510 | 2582 | text-overflow: ellipsis; | |
| 2511 | 2583 | white-space: nowrap; | |
| 2512 | − | display: inline-block; | |
| 2513 | − | max-width: 100%; | |
| 2514 | − | vertical-align: bottom; | |
| 2584 | + | line-height: 1; | |
| 2515 | 2585 | } | |
| 2516 | 2586 | ||
| 2517 | − | .filelist-name a.is-dir { | |
| 2587 | + | .filelist-entry-link.is-dir { | |
| 2518 | 2588 | font-weight: 500; | |
| 2519 | 2589 | } | |
| 2520 | 2590 | ||
| 2521 | − | .filelist tr:hover .filelist-name a { | |
| 2591 | + | /* The underline goes on the text, not the link: on the link the rule would | |
| 2592 | + | run under the icon as well. */ | |
| 2593 | + | .filelist tr:hover .filelist-entry-link { | |
| 2522 | 2594 | color: var(--action); | |
| 2595 | + | } | |
| 2596 | + | ||
| 2597 | + | .filelist tr:hover .filelist-entry-name { | |
| 2523 | 2598 | text-decoration: underline; | |
| 2524 | 2599 | } | |
| 2525 | 2600 | ||
| 2526 | 2601 | .filelist-note { | |
| 2602 | + | flex: none; | |
| 2527 | 2603 | margin-left: 8px; | |
| 2528 | 2604 | font-size: var(--text-xs); | |
| 2529 | 2605 | } | |
| 2530 | 2606 | ||
| 2531 | − | /* The commit message column. `max-width: 0` is the same "shrink to let the | |
| 2532 | − | sibling ellipsis rule take over" trick `.filelist-name` uses — without it | |
| 2533 | − | an auto-layout table lets a long message push the row wider instead of | |
| 2534 | − | truncating. */ | |
| 2607 | + | /* The commit message column. `max-width: 0` is the "shrink to let the sibling | |
| 2608 | + | ellipsis rule take over" trick — without it an auto-layout table lets a long | |
| 2609 | + | message push the row wider instead of truncating. */ | |
| 2535 | 2610 | .filelist-message { | |
| 2536 | 2611 | max-width: 0; | |
| 2537 | 2612 | font-size: var(--text-xs); | |
| @@ −2568,6 +2643,12 @@ | |||
| 2568 | 2643 | color: var(--text-faint); | |
| 2569 | 2644 | } | |
| 2570 | 2645 | ||
| 2646 | + | /* The blanket `thead th` rule left the header hugging the left edge of a | |
| 2647 | + | column whose ages are set flush right. Follow the cells. */ | |
| 2648 | + | .filelist thead th.filelist-when { | |
| 2649 | + | text-align: right; | |
| 2650 | + | } | |
| 2651 | + | ||
| 2571 | 2652 | /* Below the wide breakpoint the listing keeps only what a filename needs. */ | |
| 2572 | 2653 | @media (max-width: 1120px) { | |
| 2573 | 2654 | .filelist-change { | |
| @@ −2579,6 +2660,13 @@ | |||
| 2579 | 2660 | .filelist-message { | |
| 2580 | 2661 | display: none; | |
| 2581 | 2662 | } | |
| 2663 | + | ||
| 2664 | + | /* The message column was the one soaking up the table's leftover width; | |
| 2665 | + | with it gone, hand the slack to the name instead of letting fixed layout | |
| 2666 | + | split it across every remaining column. */ | |
| 2667 | + | .filelist-name { | |
| 2668 | + | width: auto; | |
| 2669 | + | } | |
| 2582 | 2670 | } | |
| 2583 | 2671 | ||
| 2584 | 2672 | /* ─── the repository sidebar ───────────────────────────────────────────────── */ | |
| @@ −2651,6 +2739,10 @@ | |||
| 2651 | 2739 | white-space: nowrap; | |
| 2652 | 2740 | } | |
| 2653 | 2741 | ||
| 2742 | + | .bookmark-table thead th:nth-child(4) { | |
| 2743 | + | text-align: right; | |
| 2744 | + | } | |
| 2745 | + | ||
| 2654 | 2746 | .bookmark-when { | |
| 2655 | 2747 | width: 72px; | |
| 2656 | 2748 | text-align: right; | |
| @@ −3477,10 +3569,375 @@ | |||
| 3477 | 3569 | ||
| 3478 | 3570 | /* ─── diffs and review ─────────────────────────────────────────────────────── */ | |
| 3479 | 3571 | ||
| 3572 | + | /* The diff is read in three passes, and the layout is built for that order: | |
| 3573 | + | the bar says what is being compared and how big it is, the index says which | |
| 3574 | + | files, and only then the code. Each of the first two stays out of the way of | |
| 3575 | + | the third — the bar is one line, the index folds. */ | |
| 3576 | + | ||
| 3577 | + | /* ─── the sticky diff bar ─── */ | |
| 3578 | + | ||
| 3579 | + | /* `top` clears the 46px masthead. z-index sits under the masthead (30) and over | |
| 3580 | + | the file headers (10), which is the order they overlap in. */ | |
| 3581 | + | .diffbar { | |
| 3582 | + | position: sticky; | |
| 3583 | + | top: 46px; | |
| 3584 | + | z-index: 20; | |
| 3585 | + | display: flex; | |
| 3586 | + | align-items: center; | |
| 3587 | + | gap: 10px; | |
| 3588 | + | flex-wrap: wrap; | |
| 3589 | + | padding: 7px 12px; | |
| 3590 | + | margin-bottom: 8px; | |
| 3591 | + | border: 1px solid var(--border); | |
| 3592 | + | border-radius: var(--radius); | |
| 3593 | + | background: var(--surface-raised); | |
| 3594 | + | font-size: var(--text-sm); | |
| 3595 | + | color: var(--text-dim); | |
| 3596 | + | } | |
| 3597 | + | ||
| 3598 | + | /* Opening the compare form makes the bar two rows tall, which would leave the | |
| 3599 | + | file headers sticking to a height that no longer exists. While it is open the | |
| 3600 | + | bar is an ordinary block and scrolls away. */ | |
| 3601 | + | .diffbar:has(.cmpbox[open]) { | |
| 3602 | + | position: static; | |
| 3603 | + | } | |
| 3604 | + | ||
| 3605 | + | .diffbar-stat { | |
| 3606 | + | display: flex; | |
| 3607 | + | align-items: center; | |
| 3608 | + | gap: 8px; | |
| 3609 | + | } | |
| 3610 | + | ||
| 3611 | + | .diffbar-stat strong { | |
| 3612 | + | color: var(--text); | |
| 3613 | + | } | |
| 3614 | + | ||
| 3615 | + | .diffbar-link { | |
| 3616 | + | font-size: var(--text-sm); | |
| 3617 | + | white-space: nowrap; | |
| 3618 | + | } | |
| 3619 | + | ||
| 3620 | + | /* The compare control: closed it is a label reading "Comparing v2 → v3", which | |
| 3621 | + | is information the reviewer needs on every screen. Open it is the form. */ | |
| 3622 | + | .cmpbox > summary { | |
| 3623 | + | cursor: pointer; | |
| 3624 | + | list-style: none; | |
| 3625 | + | display: flex; | |
| 3626 | + | align-items: baseline; | |
| 3627 | + | gap: 6px; | |
| 3628 | + | padding: 2px 8px; | |
| 3629 | + | border: 1px solid var(--border-strong); | |
| 3630 | + | border-radius: var(--radius); | |
| 3631 | + | color: var(--text); | |
| 3632 | + | white-space: nowrap; | |
| 3633 | + | } | |
| 3634 | + | ||
| 3635 | + | .cmpbox > summary::-webkit-details-marker { display: none; } | |
| 3636 | + | .cmpbox > summary:hover { border-color: var(--action); color: var(--action); } | |
| 3637 | + | ||
| 3638 | + | .cmpform { | |
| 3639 | + | display: flex; | |
| 3640 | + | align-items: center; | |
| 3641 | + | gap: 8px; | |
| 3642 | + | flex-wrap: wrap; | |
| 3643 | + | padding: 10px 0 2px; | |
| 3644 | + | } | |
| 3645 | + | ||
| 3646 | + | /* ─── the changed-file tree, beside the diff ─── */ | |
| 3647 | + | ||
| 3648 | + | /* Two columns: the tree, and the diff. The tree is fixed-width because it is a | |
| 3649 | + | reference the eye returns to at a known place, and `minmax(0, 1fr)` is what | |
| 3650 | + | stops a long unbroken code line from widening the grid track and pushing the | |
| 3651 | + | tree off the screen. */ | |
| 3652 | + | .difflayout { | |
| 3653 | + | display: grid; | |
| 3654 | + | grid-template-columns: 250px minmax(0, 1fr); | |
| 3655 | + | gap: 12px; | |
| 3656 | + | align-items: start; | |
| 3657 | + | } | |
| 3658 | + | ||
| 3659 | + | /* A diff with no files renders no tree, and a one-column grid should not leave | |
| 3660 | + | a 250px hole where it would have been. */ | |
| 3661 | + | .difflayout:not(:has(.difftree)) { | |
| 3662 | + | grid-template-columns: minmax(0, 1fr); | |
| 3663 | + | } | |
| 3664 | + | ||
| 3665 | + | .diffmain { | |
| 3666 | + | min-width: 0; | |
| 3667 | + | } | |
| 3668 | + | ||
| 3669 | + | /* `top` clears the masthead (46px) and the sticky diff bar under it, so the | |
| 3670 | + | tree comes to rest just below the bar rather than behind it. Scrolls | |
| 3671 | + | independently once it outgrows the viewport — a 200-file commit must not | |
| 3672 | + | make the sidebar taller than the page. */ | |
| 3673 | + | .difftree { | |
| 3674 | + | position: sticky; | |
| 3675 | + | top: 92px; | |
| 3676 | + | max-height: calc(100vh - 104px); | |
| 3677 | + | overflow-y: auto; | |
| 3678 | + | border: 1px solid var(--border); | |
| 3679 | + | border-radius: var(--radius); | |
| 3680 | + | background: var(--surface); | |
| 3681 | + | font-size: var(--text-sm); | |
| 3682 | + | } | |
| 3683 | + | ||
| 3684 | + | .dt-head { | |
| 3685 | + | position: sticky; | |
| 3686 | + | top: 0; | |
| 3687 | + | padding: 8px 10px; | |
| 3688 | + | border-bottom: 1px solid var(--border); | |
| 3689 | + | background: var(--surface-raised); | |
| 3690 | + | color: var(--text-dim); | |
| 3691 | + | font-size: var(--text-xs); | |
| 3692 | + | text-transform: uppercase; | |
| 3693 | + | letter-spacing: 0.04em; | |
| 3694 | + | } | |
| 3695 | + | ||
| 3696 | + | .dt-body { | |
| 3697 | + | padding: 6px 8px 8px; | |
| 3698 | + | } | |
| 3699 | + | ||
| 3700 | + | /* Each level indents by one step and draws a rule down its own children, which | |
| 3701 | + | is what makes the nesting readable at a glance rather than countable. */ | |
| 3702 | + | .dt-kids { | |
| 3703 | + | margin-left: 5px; | |
| 3704 | + | padding-left: 7px; | |
| 3705 | + | border-left: 1px solid var(--border); | |
| 3706 | + | } | |
| 3707 | + | ||
| 3708 | + | .dt-row, | |
| 3709 | + | .dt-file { | |
| 3710 | + | display: flex; | |
| 3711 | + | align-items: center; | |
| 3712 | + | gap: 6px; | |
| 3713 | + | padding: 2px 4px; | |
| 3714 | + | border-radius: 3px; | |
| 3715 | + | font-family: var(--font-mono); | |
| 3716 | + | line-height: 1.5; | |
| 3717 | + | text-decoration: none; | |
| 3718 | + | color: var(--text); | |
| 3719 | + | } | |
| 3720 | + | ||
| 3721 | + | .dt-row { | |
| 3722 | + | cursor: pointer; | |
| 3723 | + | list-style: none; | |
| 3724 | + | color: var(--text-dim); | |
| 3725 | + | } | |
| 3726 | + | ||
| 3727 | + | .dt-row::-webkit-details-marker { display: none; } | |
| 3728 | + | .dt-row:hover, | |
| 3729 | + | .dt-file:hover { background: var(--surface-raised); } | |
| 3730 | + | .dt-file:hover .dt-name { color: var(--action); } | |
| 3731 | + | ||
| 3732 | + | .dt-caret { | |
| 3733 | + | width: 0; | |
| 3734 | + | height: 0; | |
| 3735 | + | flex: none; | |
| 3736 | + | border-left: 5px solid var(--text-faint); | |
| 3737 | + | border-top: 4px solid transparent; | |
| 3738 | + | border-bottom: 4px solid transparent; | |
| 3739 | + | transition: transform 90ms ease; | |
| 3740 | + | } | |
| 3741 | + | ||
| 3742 | + | details[open] > .dt-row .dt-caret { | |
| 3743 | + | transform: rotate(90deg); | |
| 3744 | + | } | |
| 3745 | + | ||
| 3746 | + | /* The directory name is context and the file name is the thing, so only the | |
| 3747 | + | directory dims — the same rule the file index and the file headers follow. */ | |
| 3748 | + | .dt-dirname, | |
| 3749 | + | .dt-name { | |
| 3750 | + | min-width: 0; | |
| 3751 | + | overflow: hidden; | |
| 3752 | + | text-overflow: ellipsis; | |
| 3753 | + | white-space: nowrap; | |
| 3754 | + | } | |
| 3755 | + | ||
| 3756 | + | .dt-dirname { | |
| 3757 | + | font-size: var(--text-xs); | |
| 3758 | + | } | |
| 3759 | + | ||
| 3760 | + | /* The counts are a magnitude, not a figure to read: small, and never allowed to | |
| 3761 | + | squeeze the name they sit beside. */ | |
| 3762 | + | .dt-file .cl-add, | |
| 3763 | + | .dt-file .cl-del { | |
| 3764 | + | flex: none; | |
| 3765 | + | font-size: var(--text-xs); | |
| 3766 | + | } | |
| 3767 | + | ||
| 3768 | + | /* Under about 900px there is no room for a column beside the diff, so the tree | |
| 3769 | + | becomes a capped block above it rather than disappearing. */ | |
| 3770 | + | @media (max-width: 900px) { | |
| 3771 | + | .difflayout { | |
| 3772 | + | grid-template-columns: minmax(0, 1fr); | |
| 3773 | + | } | |
| 3774 | + | ||
| 3775 | + | /* Stacked, the tree is no longer a column and stretching it the full width | |
| 3776 | + | of the page strands the counts a screen away from their filenames. */ | |
| 3777 | + | .difftree { | |
| 3778 | + | position: static; | |
| 3779 | + | max-width: 560px; | |
| 3780 | + | max-height: 260px; | |
| 3781 | + | margin-bottom: 10px; | |
| 3782 | + | } | |
| 3783 | + | } | |
| 3784 | + | ||
| 3785 | + | /* On a change's files tab the diff already has the reviewers aside to its | |
| 3786 | + | right, so the tree makes three columns and needs the room for it. It gives | |
| 3787 | + | up its column earlier than on the commit page — the media query measures the | |
| 3788 | + | viewport, but what is actually short here is the main column inside it. */ | |
| 3789 | + | @media (max-width: 1280px) { | |
| 3790 | + | .columns-main .difflayout { | |
| 3791 | + | grid-template-columns: minmax(0, 1fr); | |
| 3792 | + | } | |
| 3793 | + | ||
| 3794 | + | .columns-main .difftree { | |
| 3795 | + | position: static; | |
| 3796 | + | max-width: 560px; | |
| 3797 | + | max-height: 260px; | |
| 3798 | + | margin-bottom: 10px; | |
| 3799 | + | } | |
| 3800 | + | } | |
| 3801 | + | ||
| 3802 | + | /* ─── the file index ─── */ | |
| 3803 | + | ||
| 3804 | + | .fileindex { | |
| 3805 | + | border: 1px solid var(--border); | |
| 3806 | + | border-radius: var(--radius); | |
| 3807 | + | background: var(--surface); | |
| 3808 | + | margin-bottom: 10px; | |
| 3809 | + | } | |
| 3810 | + | ||
| 3811 | + | .fileindex > summary { | |
| 3812 | + | cursor: pointer; | |
| 3813 | + | list-style: none; | |
| 3814 | + | display: flex; | |
| 3815 | + | align-items: center; | |
| 3816 | + | gap: 8px; | |
| 3817 | + | padding: 8px 12px; | |
| 3818 | + | font-size: var(--text-sm); | |
| 3819 | + | color: var(--text-dim); | |
| 3820 | + | } | |
| 3821 | + | ||
| 3822 | + | .fileindex > summary::-webkit-details-marker { display: none; } | |
| 3823 | + | ||
| 3824 | + | .fx-caret, | |
| 3825 | + | .fd-caret { | |
| 3826 | + | width: 0; | |
| 3827 | + | height: 0; | |
| 3828 | + | border-left: 5px solid var(--text-faint); | |
| 3829 | + | border-top: 4px solid transparent; | |
| 3830 | + | border-bottom: 4px solid transparent; | |
| 3831 | + | transition: transform 90ms ease; | |
| 3832 | + | flex: none; | |
| 3833 | + | } | |
| 3834 | + | ||
| 3835 | + | details[open] > summary .fx-caret, | |
| 3836 | + | details[open] > summary .fd-caret { | |
| 3837 | + | transform: rotate(90deg); | |
| 3838 | + | } | |
| 3839 | + | ||
| 3840 | + | /* Capped and scrolled: a forty-file index should not push the diff off the | |
| 3841 | + | first screen, which is the thing it exists to get you to. */ | |
| 3842 | + | .fx-list { | |
| 3843 | + | max-height: 300px; | |
| 3844 | + | overflow-y: auto; | |
| 3845 | + | border-top: 1px solid var(--border); | |
| 3846 | + | } | |
| 3847 | + | ||
| 3848 | + | .fx-row { | |
| 3849 | + | display: flex; | |
| 3850 | + | align-items: center; | |
| 3851 | + | gap: 8px; | |
| 3852 | + | padding: 3px 12px; | |
| 3853 | + | font-size: var(--text-sm); | |
| 3854 | + | color: var(--text); | |
| 3855 | + | text-decoration: none; | |
| 3856 | + | border-bottom: 1px solid var(--border); | |
| 3857 | + | } | |
| 3858 | + | ||
| 3859 | + | .fx-row:last-child { border-bottom: 0; } | |
| 3860 | + | .fx-row:hover { background: var(--surface-raised); } | |
| 3861 | + | ||
| 3862 | + | /* The directory is context, the basename is the thing. Dimming the leading path | |
| 3863 | + | is what makes a column of deep paths scannable, and truncating only the | |
| 3864 | + | directory means the basename — the part being looked for — never disappears | |
| 3865 | + | into an ellipsis. */ | |
| 3866 | + | .fx-path, | |
| 3867 | + | .fd-path { | |
| 3868 | + | display: flex; | |
| 3869 | + | min-width: 0; | |
| 3870 | + | font-family: var(--font-mono); | |
| 3871 | + | font-size: var(--text-xs); | |
| 3872 | + | white-space: nowrap; | |
| 3873 | + | } | |
| 3874 | + | ||
| 3875 | + | .fx-dir, | |
| 3876 | + | .fd-dir { | |
| 3877 | + | color: var(--text-faint); | |
| 3878 | + | overflow: hidden; | |
| 3879 | + | text-overflow: ellipsis; | |
| 3880 | + | } | |
| 3881 | + | ||
| 3882 | + | .fx-name, | |
| 3883 | + | .fd-name { | |
| 3884 | + | color: var(--text); | |
| 3885 | + | flex: none; | |
| 3886 | + | } | |
| 3887 | + | ||
| 3888 | + | .fx-bin, | |
| 3889 | + | .fd-note { | |
| 3890 | + | font-size: var(--text-xs); | |
| 3891 | + | color: var(--text-faint); | |
| 3892 | + | white-space: nowrap; | |
| 3893 | + | } | |
| 3894 | + | ||
| 3895 | + | /* ─── the status letter ─── */ | |
| 3896 | + | ||
| 3897 | + | .fkind { | |
| 3898 | + | flex: none; | |
| 3899 | + | width: 16px; | |
| 3900 | + | height: 16px; | |
| 3901 | + | display: inline-grid; | |
| 3902 | + | place-items: center; | |
| 3903 | + | border-radius: var(--radius-sm); | |
| 3904 | + | font-family: var(--font-mono); | |
| 3905 | + | font-size: 10px; | |
| 3906 | + | font-weight: 600; | |
| 3907 | + | line-height: 1; | |
| 3908 | + | color: var(--on-action); | |
| 3909 | + | } | |
| 3910 | + | ||
| 3911 | + | .fkind.is-add { background: var(--diff-add-text); } | |
| 3912 | + | .fkind.is-mod { background: var(--identity); } | |
| 3913 | + | .fkind.is-del { background: var(--diff-del-text); } | |
| 3914 | + | .fkind.is-ren { background: var(--merged); color: #fff; } | |
| 3915 | + | ||
| 3916 | + | /* ─── the add/delete ratio bar ─── */ | |
| 3917 | + | ||
| 3918 | + | .statbar { | |
| 3919 | + | flex: none; | |
| 3920 | + | display: flex; | |
| 3921 | + | width: 44px; | |
| 3922 | + | height: 7px; | |
| 3923 | + | border-radius: 1px; | |
| 3924 | + | overflow: hidden; | |
| 3925 | + | background: var(--border); | |
| 3926 | + | } | |
| 3927 | + | ||
| 3928 | + | .statbar-add { background: var(--diff-add-text); } | |
| 3929 | + | .statbar-del { background: var(--diff-del-text); } | |
| 3930 | + | ||
| 3931 | + | /* ─── one file ─── */ | |
| 3932 | + | ||
| 3933 | + | /* The base box is shared with the interdiff and the conflict view, which build | |
| 3934 | + | it out of plain divs and scroll horizontally. Only the foldable version in | |
| 3935 | + | the files tab gets the sticky, clickable header below. */ | |
| 3480 | 3936 | .filediff { | |
| 3481 | 3937 | border: 1px solid var(--border); | |
| 3482 | 3938 | border-radius: var(--radius); | |
| 3483 | − | margin-bottom: 14px; | |
| 3939 | + | margin-bottom: 10px; | |
| 3940 | + | background: var(--surface); | |
| 3484 | 3941 | overflow-x: auto; | |
| 3485 | 3942 | } | |
| 3486 | 3943 | ||
| @@ −3490,34 +3947,187 @@ | |||
| 3490 | 3947 | border-bottom: 1px solid var(--border); | |
| 3491 | 3948 | } | |
| 3492 | 3949 | ||
| 3950 | + | /* ─── the foldable file, files tab only ─── */ | |
| 3951 | + | ||
| 3952 | + | details.filediff { | |
| 3953 | + | /* Wrapping the code removes the reason to scroll, and a scroll container | |
| 3954 | + | would stop the header sticking to the viewport. */ | |
| 3955 | + | overflow-x: visible; | |
| 3956 | + | /* Jumping from the index must not land the header under the sticky bar. */ | |
| 3957 | + | scroll-margin-top: 92px; | |
| 3958 | + | } | |
| 3959 | + | ||
| 3960 | + | /* Sticky, so the path stays readable a thousand lines into the file. It is the | |
| 3961 | + | summary itself rather than a child, because `details` is the containing | |
| 3962 | + | block here and a sticky grandchild would stop at its padding box. */ | |
| 3963 | + | summary.filediff-head { | |
| 3964 | + | position: sticky; | |
| 3965 | + | top: 84px; | |
| 3966 | + | z-index: 10; | |
| 3967 | + | cursor: pointer; | |
| 3968 | + | list-style: none; | |
| 3969 | + | display: flex; | |
| 3970 | + | align-items: center; | |
| 3971 | + | gap: 8px; | |
| 3972 | + | padding: 7px 10px; | |
| 3973 | + | border-radius: var(--radius) var(--radius) 0 0; | |
| 3974 | + | } | |
| 3975 | + | ||
| 3976 | + | summary.filediff-head::-webkit-details-marker { display: none; } | |
| 3977 | + | summary.filediff-head:hover .fd-name { color: var(--action); } | |
| 3978 | + | details.filediff:not([open]) > .filediff-head { border-bottom-color: transparent; } | |
| 3979 | + | ||
| 3980 | + | /* A folded large file still has to look like something you can open, not like | |
| 3981 | + | something that failed to render. */ | |
| 3982 | + | details.filediff.is-big:not([open]) > .filediff-head { | |
| 3983 | + | border-left: 2px solid var(--identity); | |
| 3984 | + | } | |
| 3985 | + | ||
| 3986 | + | .fd-old { | |
| 3987 | + | font-size: var(--text-xs); | |
| 3988 | + | color: var(--text-faint); | |
| 3989 | + | white-space: nowrap; | |
| 3990 | + | } | |
| 3991 | + | ||
| 3992 | + | /* "View file" — only the commit page sets it, where the revision is one the | |
| 3993 | + | browse routes can serve. Quiet until the header is hovered: it is the answer | |
| 3994 | + | to a question most hunks never raise. */ | |
| 3995 | + | .fd-view { | |
| 3996 | + | font-size: var(--text-xs); | |
| 3997 | + | color: var(--text-faint); | |
| 3998 | + | white-space: nowrap; | |
| 3999 | + | } | |
| 4000 | + | ||
| 4001 | + | summary.filediff-head:hover .fd-view { color: var(--action); } | |
| 4002 | + | ||
| 4003 | + | /* ─── the lines ─── */ | |
| 4004 | + | ||
| 3493 | 4005 | .difftable { | |
| 3494 | 4006 | width: 100%; | |
| 3495 | 4007 | border-collapse: collapse; | |
| 4008 | + | font-size: 12.5px; | |
| 4009 | + | line-height: 19px; | |
| 3496 | 4010 | } | |
| 3497 | 4011 | ||
| 4012 | + | /* Two number columns and a sign column, all fixed and all unselectable, so a | |
| 4013 | + | copy of the diff is a copy of the code. */ | |
| 3498 | 4014 | .difftable .lineno { | |
| 4015 | + | position: relative; | |
| 3499 | 4016 | width: 1%; | |
| 4017 | + | min-width: 44px; | |
| 3500 | 4018 | text-align: right; | |
| 3501 | 4019 | padding: 0 8px; | |
| 3502 | 4020 | user-select: none; | |
| 3503 | 4021 | white-space: nowrap; | |
| 4022 | + | color: var(--text-faint); | |
| 4023 | + | font-size: var(--text-xs); | |
| 4024 | + | background: color-mix(in srgb, var(--bg) 60%, transparent); | |
| 4025 | + | } | |
| 4026 | + | ||
| 4027 | + | .difftable .lineno + .lineno { | |
| 4028 | + | border-right: 1px solid var(--border); | |
| 3504 | 4029 | } | |
| 3505 | 4030 | ||
| 4031 | + | .difftable .dl-mark { | |
| 4032 | + | width: 1%; | |
| 4033 | + | padding: 0 0 0 6px; | |
| 4034 | + | user-select: none; | |
| 4035 | + | text-align: center; | |
| 4036 | + | font-weight: 600; | |
| 4037 | + | } | |
| 4038 | + | ||
| 3506 | 4039 | .difftable .codeline { | |
| 3507 | − | padding: 0 10px; | |
| 4040 | + | padding: 0 10px 0 4px; | |
| 4041 | + | /* Wrap rather than scroll: a horizontal scrollbar per file makes a long diff | |
| 4042 | + | unreadable, and a scroll container would also break the sticky header. | |
| 4043 | + | Continuation lines are hung two characters in so a wrap is visibly a wrap | |
| 4044 | + | and not a new line of code. */ | |
| 3508 | 4045 | white-space: pre-wrap; | |
| 3509 | − | word-break: break-all; | |
| 4046 | + | overflow-wrap: anywhere; | |
| 4047 | + | word-break: normal; | |
| 4048 | + | text-indent: -2ch; | |
| 4049 | + | padding-left: calc(4px + 2ch); | |
| 3510 | 4050 | } | |
| 3511 | 4051 | ||
| 3512 | 4052 | .difftable .hunkhead td { | |
| 3513 | − | padding: 2px 10px; | |
| 4053 | + | padding: 3px 10px; | |
| 3514 | 4054 | color: var(--text-faint); | |
| 3515 | 4055 | background: var(--bg); | |
| 4056 | + | border-top: 1px solid var(--border); | |
| 4057 | + | border-bottom: 1px solid var(--border); | |
| 4058 | + | font-size: var(--text-xs); | |
| 3516 | 4059 | } | |
| 3517 | 4060 | ||
| 3518 | − | .line-add { background: var(--diff-add-bg); color: var(--diff-add-text); } | |
| 3519 | − | .line-del { background: var(--diff-del-bg); color: var(--diff-del-text); } | |
| 4061 | + | .difftable tbody tr:first-child .hunkhead td, | |
| 4062 | + | .difftable .hunkhead:first-child td { | |
| 4063 | + | border-top: 0; | |
| 4064 | + | } | |
| 4065 | + | ||
| 4066 | + | /* In the commentable table the wash carries the add/delete signal on its own | |
| 4067 | + | and the text stays body colour. Colouring whole lines green and red — as this | |
| 4068 | + | used to — costs contrast on every character and drowns the word-level | |
| 4069 | + | emphasis that says what actually changed on the line. The 2px rule is what | |
| 4070 | + | survives a monochrome rendering. | |
| 4071 | + | ||
| 4072 | + | The `.diffline` variant (interdiff, design specimens) keeps the coloured text | |
| 4073 | + | it had: it is read in short bursts, and it has no gutter to carry the colour | |
| 4074 | + | instead. */ | |
| 4075 | + | .diffline.line-add { background: var(--diff-add-bg); color: var(--diff-add-text); } | |
| 4076 | + | .diffline.line-del { background: var(--diff-del-bg); color: var(--diff-del-text); } | |
| 3520 | 4077 | ||
| 4078 | + | .diff-sign { | |
| 4079 | + | display: inline-block; | |
| 4080 | + | width: 1ch; | |
| 4081 | + | } | |
| 4082 | + | ||
| 4083 | + | .line-add > .lineno, | |
| 4084 | + | .line-add > .dl-mark, | |
| 4085 | + | .line-add > .codeline { background: var(--diff-add-bg); } | |
| 4086 | + | .line-del > .lineno, | |
| 4087 | + | .line-del > .dl-mark, | |
| 4088 | + | .line-del > .codeline { background: var(--diff-del-bg); } | |
| 4089 | + | ||
| 4090 | + | .line-add > .dl-mark { color: var(--diff-add-text); } | |
| 4091 | + | .line-del > .dl-mark { color: var(--diff-del-text); } | |
| 4092 | + | ||
| 4093 | + | .line-add > .lineno:first-child { box-shadow: inset 2px 0 0 var(--diff-add-text); } | |
| 4094 | + | .line-del > .lineno:first-child { box-shadow: inset 2px 0 0 var(--diff-del-text); } | |
| 4095 | + | ||
| 4096 | + | .dl:hover > .lineno { color: var(--text-dim); } | |
| 4097 | + | ||
| 4098 | + | /* The comment affordance. Absolutely positioned so it costs no layout, hidden | |
| 4099 | + | until the row is hovered or it is focused — which is what lets the diff be a | |
| 4100 | + | wall of code again instead of a wall of "Comment on line N". */ | |
| 4101 | + | .dl-add { | |
| 4102 | + | position: absolute; | |
| 4103 | + | left: 2px; | |
| 4104 | + | top: 50%; | |
| 4105 | + | transform: translateY(-50%); | |
| 4106 | + | width: 15px; | |
| 4107 | + | height: 15px; | |
| 4108 | + | display: grid; | |
| 4109 | + | place-items: center; | |
| 4110 | + | border-radius: var(--radius-sm); | |
| 4111 | + | background: var(--action); | |
| 4112 | + | color: var(--on-action); | |
| 4113 | + | font-size: 12px; | |
| 4114 | + | line-height: 1; | |
| 4115 | + | text-decoration: none; | |
| 4116 | + | opacity: 0; | |
| 4117 | + | pointer-events: none; | |
| 4118 | + | } | |
| 4119 | + | ||
| 4120 | + | .dl:hover .dl-add, | |
| 4121 | + | .dl-add:focus-visible { | |
| 4122 | + | opacity: 1; | |
| 4123 | + | pointer-events: auto; | |
| 4124 | + | } | |
| 4125 | + | ||
| 4126 | + | @media (hover: none) { | |
| 4127 | + | /* No hover to reveal it with, so it is always there, just quiet. */ | |
| 4128 | + | .dl-add { opacity: 0.45; pointer-events: auto; } | |
| 4129 | + | } | |
| 4130 | + | ||
| 3521 | 4131 | /* A diff line outside the commentable table: the interdiff and the design | |
| 3522 | 4132 | sheet's specimen rows. A grid rather than a table, because there is no | |
| 3523 | 4133 | comment column to align against — just a gutter and the code. | |
| @@ −3575,15 +4185,29 @@ | |||
| 3575 | 4185 | white-space: normal; | |
| 3576 | 4186 | } | |
| 3577 | 4187 | ||
| 4188 | + | /* The comment form exists in the markup for every commentable line and is shown | |
| 4189 | + | for exactly one: the one named in the fragment. `:target` does the work, so | |
| 4190 | + | there is no per-line chrome in the resting diff, no script, and an open form | |
| 4191 | + | has a URL that can be linked at somebody. */ | |
| 3578 | 4192 | .inline-form { | |
| 3579 | − | padding: 4px 14px 10px; | |
| 4193 | + | display: none; | |
| 4194 | + | } | |
| 4195 | + | ||
| 4196 | + | .inline-form:target { | |
| 4197 | + | display: table-row; | |
| 4198 | + | scroll-margin-top: 120px; | |
| 4199 | + | } | |
| 4200 | + | ||
| 4201 | + | .inline-form > td { | |
| 4202 | + | padding: 10px 14px; | |
| 3580 | 4203 | background: var(--surface); | |
| 4204 | + | border-top: 2px solid var(--action); | |
| 4205 | + | border-bottom: 1px solid var(--border); | |
| 3581 | 4206 | white-space: normal; | |
| 3582 | 4207 | } | |
| 3583 | 4208 | ||
| 3584 | − | .inline-form summary { | |
| 3585 | − | cursor: pointer; | |
| 3586 | − | font-size: 12px; | |
| 4209 | + | .inline-form textarea { | |
| 4210 | + | width: 100%; | |
| 3587 | 4211 | } | |
| 3588 | 4212 | ||
| 3589 | 4213 | /* A comment is a boxed object; an event is a line. That difference is the | |
| @@ −4529,10 +5153,11 @@ | |||
| 4529 | 5153 | .sidebar-header { | |
| 4530 | 5154 | display: flex; | |
| 4531 | 5155 | align-items: center; | |
| 4532 | − | gap: 6px; | |
| 5156 | + | gap: 8px; | |
| 4533 | 5157 | padding: 10px 12px; | |
| 4534 | 5158 | border-bottom: 1px solid var(--border); | |
| 4535 | 5159 | font-size: var(--text-sm); | |
| 5160 | + | line-height: 1; | |
| 4536 | 5161 | } | |
| 4537 | 5162 | ||
| 4538 | 5163 | .file-tree { | |
| @@ −4540,10 +5165,14 @@ | |||
| 4540 | 5165 | flex-direction: column; | |
| 4541 | 5166 | } | |
| 4542 | 5167 | ||
| 5168 | + | /* `line-height: 1`: without it the body default of 1.5 gives the label extra | |
| 5169 | + | leading that JetBrains Mono splits unevenly above/below the glyphs, so the | |
| 5170 | + | text hangs below the icon instead of sitting beside it (same fix as | |
| 5171 | + | `.filelist-entry-name`). */ | |
| 4543 | 5172 | .file-tree-item { | |
| 4544 | 5173 | display: flex; | |
| 4545 | 5174 | align-items: center; | |
| 4546 | − | gap: 6px; | |
| 5175 | + | gap: 8px; | |
| 4547 | 5176 | padding: 5px 12px; | |
| 4548 | 5177 | font-size: var(--text-sm); | |
| 4549 | 5178 | font-family: var(--font-mono); | |
| @@ −4553,6 +5182,7 @@ | |||
| 4553 | 5182 | white-space: nowrap; | |
| 4554 | 5183 | overflow: hidden; | |
| 4555 | 5184 | text-overflow: ellipsis; | |
| 5185 | + | line-height: 1; | |
| 4556 | 5186 | } | |
| 4557 | 5187 | ||
| 4558 | 5188 | .file-tree-item:last-child { | |
| @@ −4574,6 +5204,11 @@ | |||
| 4574 | 5204 | color: var(--text-dim); | |
| 4575 | 5205 | } | |
| 4576 | 5206 | ||
| 5207 | + | .sidebar-header svg, | |
| 5208 | + | .file-tree-item svg { | |
| 5209 | + | flex: none; | |
| 5210 | + | } | |
| 5211 | + | ||
| 4577 | 5212 | .file-tree-parent { | |
| 4578 | 5213 | color: var(--text-dim); | |
| 4579 | 5214 | } | |
Mcrates/df-web/src/config.rs+44−0
| @@ −5,6 +5,7 @@ | |||
| 5 | 5 | //! boot rather than at the first request that needs it. | |
| 6 | 6 | ||
| 7 | 7 | use anyhow::{bail, Context, Result}; | |
| 8 | + | use sqlx::types::ipnetwork::IpNetwork; | |
| 8 | 9 | ||
| 9 | 10 | #[derive(Debug, Clone)] | |
| 10 | 11 | pub struct Config { | |
| @@ −28,6 +29,17 @@ | |||
| 28 | 29 | /// stray comma cannot widen access. | |
| 29 | 30 | pub allowlist: Vec<String>, | |
| 30 | 31 | ||
| 32 | + | /// Networks whose `X-Forwarded-For` header is believed. | |
| 33 | + | /// | |
| 34 | + | /// Empty means "believe nobody", which is the only safe default: `XFF` is a | |
| 35 | + | /// request header, so trusting it from an arbitrary peer lets any client | |
| 36 | + | /// name its own address and walk straight through the rate limiter. | |
| 37 | + | /// | |
| 38 | + | /// This must list the edge proxy, or every request appears to come from the | |
| 39 | + | /// proxy and all anonymous traffic shares one bucket — which is not a | |
| 40 | + | /// tighter limit but a global one, and a single client can spend it. | |
| 41 | + | pub trusted_proxies: Vec<IpNetwork>, | |
| 42 | + | ||
| 31 | 43 | pub session_secret: Vec<u8>, | |
| 32 | 44 | pub session_ttl_days: i64, | |
| 33 | 45 | ||
| @@ −63,6 +75,24 @@ | |||
| 63 | 75 | } | |
| 64 | 76 | } | |
| 65 | 77 | ||
| 78 | + | /// Parse `TRUSTED_PROXIES`: comma-separated CIDRs, or bare addresses meaning a | |
| 79 | + | /// single host. | |
| 80 | + | /// | |
| 81 | + | /// A malformed entry is fatal rather than skipped. Dropping one silently would | |
| 82 | + | /// mean the proxy is not trusted, every client looks like the proxy, and the | |
| 83 | + | /// rate limiter degrades to a single global bucket — a failure that presents as | |
| 84 | + | /// mysterious 429s rather than as a configuration error. | |
| 85 | + | fn parse_trusted_proxies(raw: &str) -> Result<Vec<IpNetwork>> { | |
| 86 | + | raw.split(',') | |
| 87 | + | .map(str::trim) | |
| 88 | + | .filter(|s| !s.is_empty()) | |
| 89 | + | .map(|s| { | |
| 90 | + | s.parse::<IpNetwork>() | |
| 91 | + | .with_context(|| format!("TRUSTED_PROXIES entry {s:?} is not an address or CIDR")) | |
| 92 | + | }) | |
| 93 | + | .collect() | |
| 94 | + | } | |
| 95 | + | ||
| 66 | 96 | impl Config { | |
| 67 | 97 | pub fn from_env() -> Result<Self> { | |
| 68 | 98 | let base_url = var_or("BASE_URL", "http://localhost:8080") | |
| @@ −94,6 +124,17 @@ | |||
| 94 | 124 | ); | |
| 95 | 125 | } | |
| 96 | 126 | ||
| 127 | + | let trusted_proxies = parse_trusted_proxies(&var_or("TRUSTED_PROXIES", ""))?; | |
| 128 | + | if trusted_proxies.is_empty() { | |
| 129 | + | tracing::warn!( | |
| 130 | + | "TRUSTED_PROXIES is empty; rate limits key on the TCP peer. \ | |
| 131 | + | Behind a reverse proxy that makes every anonymous request share \ | |
| 132 | + | one bucket — set it to the proxy's network." | |
| 133 | + | ); | |
| 134 | + | } else { | |
| 135 | + | tracing::info!(?trusted_proxies, "trusting X-Forwarded-For from these networks"); | |
| 136 | + | } | |
| 137 | + | ||
| 97 | 138 | let cfg = Config { | |
| 98 | 139 | database_url: var("DATABASE_URL")?, | |
| 99 | 140 | database_max_connections: parse_num("DATABASE_MAX_CONNECTIONS", 10)?, | |
| @@ −114,6 +155,7 @@ | |||
| 114 | 155 | oidc_scopes: var_or("OIDC_SCOPES", "openid profile email"), | |
| 115 | 156 | ||
| 116 | 157 | allowlist, | |
| 158 | + | trusted_proxies, | |
| 117 | 159 | session_secret, | |
| 118 | 160 | session_ttl_days: parse_num("SESSION_TTL_DAYS", 14i64)?, | |
| 119 | 161 | ||
| @@ −197,6 +239,7 @@ | |||
| 197 | 239 | oidc_redirect_url: "https://dogfood.test/auth/callback".into(), | |
| 198 | 240 | oidc_scopes: "openid profile email".into(), | |
| 199 | 241 | allowlist: vec![], | |
| 242 | + | trusted_proxies: vec![], | |
| 200 | 243 | session_secret: vec![7; 32], | |
| 201 | 244 | session_ttl_days: 14, | |
| 202 | 245 | hook_binary: "/usr/local/bin/dogfood-hook".into(), | |
| @@ −245,6 +288,7 @@ | |||
| 245 | 288 | oidc_redirect_url: format!("{base}/auth/callback"), | |
| 246 | 289 | oidc_scopes: "openid".into(), | |
| 247 | 290 | allowlist: vec![], | |
| 291 | + | trusted_proxies: vec![], | |
| 248 | 292 | session_secret: vec![0; 32], | |
| 249 | 293 | session_ttl_days: 14, | |
| 250 | 294 | hook_binary: "/usr/local/bin/dogfood-hook".into(), | |
Mcrates/df-web/src/main.rs+13−4
| @@ −189,6 +189,7 @@ | |||
| 189 | 189 | get(routes::edit::show).post(routes::edit::save), | |
| 190 | 190 | ) | |
| 191 | 191 | .route("/{owner}/{repo}/log", get(routes::repo::log)) | |
| 192 | + | .route("/{owner}/{repo}/commit/{rev}", get(routes::repo::commit)) | |
| 192 | 193 | .route( | |
| 193 | 194 | "/{owner}/{repo}/changes", | |
| 194 | 195 | get(routes::change::list).post(routes::change::create), | |
| @@ −246,11 +247,15 @@ | |||
| 246 | 247 | // (spec §7, §10). | |
| 247 | 248 | .route("/metrics", get(routes::metrics::metrics)) | |
| 248 | 249 | // Order matters. Layers run outermost-last in this builder, so the | |
| 249 | − | // effective order per request is: security → session → rate limit → | |
| 250 | − | // handler. The limiter runs *after* the session so an authenticated | |
| 250 | + | // effective order per request is: security → edge limit → session → | |
| 251 | + | // rate limit → handler. | |
| 252 | + | // | |
| 253 | + | // The fine-grained limiter runs *after* the session so an authenticated | |
| 251 | 254 | // request gets its own bucket rather than sharing its neighbours' | |
| 252 | − | // address, and *inside* the security layer so a 429 still carries the | |
| 253 | − | // security headers. | |
| 255 | + | // address. That leaves session resolution — a database round trip — | |
| 256 | + | // ahead of it, so a coarse per-address limit runs before the session to | |
| 257 | + | // bound what an unauthenticated flood can force. Both sit inside the | |
| 258 | + | // security layer, so a 429 still carries the security headers. | |
| 254 | 259 | .layer(axum::middleware::from_fn_with_state( | |
| 255 | 260 | state.clone(), | |
| 256 | 261 | ratelimit::layer, | |
| @@ −261,6 +266,10 @@ | |||
| 261 | 266 | )) | |
| 262 | 267 | .layer(axum::middleware::from_fn_with_state( | |
| 263 | 268 | state.clone(), | |
| 269 | + | ratelimit::edge_layer, | |
| 270 | + | )) | |
| 271 | + | .layer(axum::middleware::from_fn_with_state( | |
| 272 | + | state.clone(), | |
| 264 | 273 | middleware::security_layer, | |
| 265 | 274 | )) | |
| 266 | 275 | .layer(CatchPanicLayer::new()) | |
Mcrates/df-web/src/middleware.rs+14−15
| @@ −9,7 +9,6 @@ | |||
| 9 | 9 | use axum_extra::extract::cookie::{Cookie, SameSite}; | |
| 10 | 10 | use df_auth::{csrf, session}; | |
| 11 | 11 | use rand::RngCore; | |
| 12 | − | use uuid::Uuid; | |
| 13 | 12 | ||
| 14 | 13 | use crate::state::{AppState, CsrfToken, CurrentUser, Nonce}; | |
| 15 | 14 | ||
| @@ −27,23 +26,23 @@ | |||
| 27 | 26 | let mut current = CurrentUser(None); | |
| 28 | 27 | ||
| 29 | 28 | if let Some(raw) = jar.get(session::COOKIE_NAME).map(|c| c.value().to_owned()) { | |
| 30 | − | if let Ok(id) = raw.parse::<Uuid>() { | |
| 31 | − | match session::load(&state.db, id).await { | |
| 32 | − | Ok(Some((sess, user))) => { | |
| 33 | − | // Slide the expiry only past the halfway point, to avoid a | |
| 34 | − | // database write on every page view. | |
| 35 | − | if session::should_slide(sess.expires_at, state.config.session_ttl_days) { | |
| 36 | − | if let Err(e) = | |
| 37 | − | session::slide(&state.db, sess.id, state.config.session_ttl_days).await | |
| 38 | − | { | |
| 39 | − | tracing::warn!("sliding session failed: {e}"); | |
| 40 | − | } | |
| 29 | + | // The cookie is the session's token, not its id. `load` rejects | |
| 30 | + | // anything not shaped like one without touching the database. | |
| 31 | + | match session::load(&state.db, &raw).await { | |
| 32 | + | Ok(Some((sess, user))) => { | |
| 33 | + | // Slide the expiry only past the halfway point, to avoid a | |
| 34 | + | // database write on every page view. | |
| 35 | + | if session::should_slide(sess.expires_at, state.config.session_ttl_days) { | |
| 36 | + | if let Err(e) = | |
| 37 | + | session::slide(&state.db, sess.id, state.config.session_ttl_days).await | |
| 38 | + | { | |
| 39 | + | tracing::warn!("sliding session failed: {e}"); | |
| 41 | 40 | } | |
| 42 | − | current = CurrentUser(Some(Arc::new(user))); | |
| 43 | 41 | } | |
| 44 | − | Ok(None) => { /* expired or unknown: treated as signed out */ } | |
| 45 | − | Err(e) => tracing::error!("loading session failed: {e}"), | |
| 42 | + | current = CurrentUser(Some(Arc::new(user))); | |
| 46 | 43 | } | |
| 44 | + | Ok(None) => { /* expired or unknown: treated as signed out */ } | |
| 45 | + | Err(e) => tracing::error!("loading session failed: {e}"), | |
| 47 | 46 | } | |
| 48 | 47 | } | |
| 49 | 48 | ||
Mcrates/df-web/src/ratelimit.rs460 lines+360−27
| @@ −15,6 +15,17 @@ | |||
| 15 | 15 | //! team behind one NAT is not one attacker, and one attacker on a hundred | |
| 16 | 16 | //! addresses is still one account. | |
| 17 | 17 | //! | |
| 18 | + | //! "By IP" means [`client_ip`], not the TCP peer. Behind a reverse proxy the | |
| 19 | + | //! peer is the proxy for every request in the world, so keying on it does not | |
| 20 | + | //! produce a strict limit — it produces a *global* one, and a single client can | |
| 21 | + | //! spend the whole budget and lock every signed-out visitor out of the site. | |
| 22 | + | //! `TRUSTED_PROXIES` is what closes that, and an empty setting is loud about it. | |
| 23 | + | //! | |
| 24 | + | //! There are two token buckets in the request path, at different depths. | |
| 25 | + | //! Resolving a session is a database round trip and it happens *before* the | |
| 26 | + | //! limiter that knows who you are, so a coarse per-address bucket | |
| 27 | + | //! ([`edge_layer`]) runs ahead of it, and the real one ([`layer`]) runs after. | |
| 28 | + | //! | |
| 18 | 29 | //! In-memory, deliberately. A shared limiter would mean Redis, and this is a | |
| 19 | 30 | //! single-instance product (spec §1). The state is small, bounded, and reset by | |
| 20 | 31 | //! a restart — which is the correct behaviour for a limiter whose only job is to | |
| @@ −26,9 +37,10 @@ | |||
| 26 | 37 | use std::time::{Duration, Instant}; | |
| 27 | 38 | ||
| 28 | 39 | use axum::extract::{ConnectInfo, Request, State}; | |
| 29 | − | use axum::http::StatusCode; | |
| 40 | + | use axum::http::{HeaderMap, StatusCode}; | |
| 30 | 41 | use axum::middleware::Next; | |
| 31 | 42 | use axum::response::{IntoResponse, Response}; | |
| 43 | + | use sqlx::types::ipnetwork::IpNetwork; | |
| 32 | 44 | ||
| 33 | 45 | use crate::state::{AppState, CurrentUser}; | |
| 34 | 46 | ||
| @@ −50,6 +62,16 @@ | |||
| 50 | 62 | /// identity holding all of them. | |
| 51 | 63 | const MAX_CONCURRENT: u32 = 12; | |
| 52 | 64 | ||
| 65 | + | /// The coarse bucket applied before the session is resolved. | |
| 66 | + | /// | |
| 67 | + | /// Deliberately far above [`BURST`]: it is not a second rate limit, it is a | |
| 68 | + | /// bound on how much work an unauthenticated flood can force *ahead of* the | |
| 69 | + | /// real limiter. Session resolution is a database round trip and it runs first, | |
| 70 | + | /// so without this a client can spend a query per request no matter what the | |
| 71 | + | /// bucket below decides. | |
| 72 | + | const EDGE_REFILL_PER_SEC: f64 = 50.0; | |
| 73 | + | const EDGE_BURST: f64 = 200.0; | |
| 74 | + | ||
| 53 | 75 | /// Entries idle longer than this are dropped, so the map does not grow with | |
| 54 | 76 | /// every address that has ever connected. | |
| 55 | 77 | const IDLE_EVICT: Duration = Duration::from_secs(600); | |
| @@ −83,10 +105,35 @@ | |||
| 83 | 105 | struct Bucket { | |
| 84 | 106 | tokens: f64, | |
| 85 | 107 | auth_tokens: f64, | |
| 108 | + | edge_tokens: f64, | |
| 86 | 109 | in_flight: u32, | |
| 87 | 110 | last: Instant, | |
| 88 | 111 | } | |
| 89 | 112 | ||
| 113 | + | impl Bucket { | |
| 114 | + | fn full(now: Instant) -> Bucket { | |
| 115 | + | Bucket { | |
| 116 | + | tokens: BURST, | |
| 117 | + | auth_tokens: AUTH_BURST, | |
| 118 | + | edge_tokens: EDGE_BURST, | |
| 119 | + | in_flight: 0, | |
| 120 | + | last: now, | |
| 121 | + | } | |
| 122 | + | } | |
| 123 | + | ||
| 124 | + | /// Refill for the time that passed. | |
| 125 | + | /// | |
| 126 | + | /// `saturating_duration_since` because a clock that went backwards must not | |
| 127 | + | /// mint tokens. | |
| 128 | + | fn refill(&mut self, now: Instant) { | |
| 129 | + | let elapsed = now.saturating_duration_since(self.last).as_secs_f64(); | |
| 130 | + | self.tokens = (self.tokens + elapsed * REFILL_PER_SEC).min(BURST); | |
| 131 | + | self.auth_tokens = (self.auth_tokens + elapsed * AUTH_REFILL_PER_SEC).min(AUTH_BURST); | |
| 132 | + | self.edge_tokens = (self.edge_tokens + elapsed * EDGE_REFILL_PER_SEC).min(EDGE_BURST); | |
| 133 | + | self.last = now; | |
| 134 | + | } | |
| 135 | + | } | |
| 136 | + | ||
| 90 | 137 | impl Default for Limiter { | |
| 91 | 138 | fn default() -> Self { | |
| 92 | 139 | Self::new() | |
| @@ −117,19 +164,8 @@ | |||
| 117 | 164 | return None; | |
| 118 | 165 | } | |
| 119 | 166 | ||
| 120 | − | let bucket = inner.buckets.entry(key.clone()).or_insert(Bucket { | |
| 121 | − | tokens: BURST, | |
| 122 | − | auth_tokens: AUTH_BURST, | |
| 123 | − | in_flight: 0, | |
| 124 | − | last: now, | |
| 125 | − | }); | |
| 126 | − | ||
| 127 | − | // Refill for the time that passed. `saturating_duration_since` because a | |
| 128 | − | // clock that went backwards must not mint tokens. | |
| 129 | − | let elapsed = now.saturating_duration_since(bucket.last).as_secs_f64(); | |
| 130 | − | bucket.tokens = (bucket.tokens + elapsed * REFILL_PER_SEC).min(BURST); | |
| 131 | − | bucket.auth_tokens = (bucket.auth_tokens + elapsed * AUTH_REFILL_PER_SEC).min(AUTH_BURST); | |
| 132 | − | bucket.last = now; | |
| 167 | + | let bucket = inner.buckets.entry(key.clone()).or_insert(Bucket::full(now)); | |
| 168 | + | bucket.refill(now); | |
| 133 | 169 | ||
| 134 | 170 | if bucket.in_flight >= MAX_CONCURRENT { | |
| 135 | 171 | return None; | |
| @@ −150,6 +186,29 @@ | |||
| 150 | 186 | Some(Guard { limiter: self.clone(), key }) | |
| 151 | 187 | } | |
| 152 | 188 | ||
| 189 | + | /// Take one token from the coarse pre-session bucket. | |
| 190 | + | /// | |
| 191 | + | /// No concurrency slot: this runs before the handler is chosen and releases | |
| 192 | + | /// nothing, so it bounds arrival rate only. | |
| 193 | + | fn acquire_edge(&self, ip: IpAddr, now: Instant) -> bool { | |
| 194 | + | let key = Key::Addr(ip); | |
| 195 | + | let mut inner = self.0.lock().expect("rate limiter poisoned"); | |
| 196 | + | inner.sweep(now); | |
| 197 | + | ||
| 198 | + | if inner.buckets.len() >= MAX_TRACKED && !inner.buckets.contains_key(&key) { | |
| 199 | + | return false; | |
| 200 | + | } | |
| 201 | + | ||
| 202 | + | let bucket = inner.buckets.entry(key).or_insert(Bucket::full(now)); | |
| 203 | + | bucket.refill(now); | |
| 204 | + | ||
| 205 | + | if bucket.edge_tokens < 1.0 { | |
| 206 | + | return false; | |
| 207 | + | } | |
| 208 | + | bucket.edge_tokens -= 1.0; | |
| 209 | + | true | |
| 210 | + | } | |
| 211 | + | ||
| 153 | 212 | fn release(&self, key: &Key) { | |
| 154 | 213 | if let Ok(mut inner) = self.0.lock() { | |
| 155 | 214 | if let Some(b) = inner.buckets.get_mut(key) { | |
| @@ −205,6 +264,96 @@ | |||
| 205 | 264 | matches!(path, "/healthz" | "/readyz") || path.starts_with("/assets/") | |
| 206 | 265 | } | |
| 207 | 266 | ||
| 267 | + | /// Not in `http::header`, which only defines registered headers. | |
| 268 | + | const X_FORWARDED_FOR: &str = "x-forwarded-for"; | |
| 269 | + | ||
| 270 | + | /// One `X-Forwarded-For` entry as an address. | |
| 271 | + | /// | |
| 272 | + | /// Proxies vary: bare addresses, `addr:port`, and bracketed IPv6 all appear. | |
| 273 | + | /// Anything that does not parse is discarded rather than guessed at. | |
| 274 | + | fn parse_forwarded(entry: &str) -> Option<IpAddr> { | |
| 275 | + | let s = entry.trim(); | |
| 276 | + | if s.is_empty() { | |
| 277 | + | return None; | |
| 278 | + | } | |
| 279 | + | // `[::1]` or `[::1]:8080` | |
| 280 | + | if let Some(rest) = s.strip_prefix('[') { | |
| 281 | + | let (inner, _) = rest.split_once(']')?; | |
| 282 | + | return inner.parse().ok(); | |
| 283 | + | } | |
| 284 | + | if let Ok(ip) = s.parse::<IpAddr>() { | |
| 285 | + | return Some(ip); | |
| 286 | + | } | |
| 287 | + | // `1.2.3.4:5678`. Only IPv4 — a bare IPv6 has colons of its own and was | |
| 288 | + | // handled by the parse above. | |
| 289 | + | s.rsplit_once(':').and_then(|(host, _)| host.parse().ok()) | |
| 290 | + | } | |
| 291 | + | ||
| 292 | + | /// The address to attribute a request to. | |
| 293 | + | /// | |
| 294 | + | /// The TCP peer is the truth unless it is a proxy we were told to trust, in | |
| 295 | + | /// which case the client is the rightmost `X-Forwarded-For` entry that is not | |
| 296 | + | /// itself trusted — walking from the right because the entries an attacker can | |
| 297 | + | /// forge are on the left, appended before ours. | |
| 298 | + | /// | |
| 299 | + | /// With no trusted proxies configured the header is ignored entirely. That is | |
| 300 | + | /// the only safe default: `XFF` is client-controlled, so honouring it from an | |
| 301 | + | /// arbitrary peer would let anyone claim a fresh bucket per request. | |
| 302 | + | pub fn client_ip( | |
| 303 | + | headers: &HeaderMap, | |
| 304 | + | peer: Option<IpAddr>, | |
| 305 | + | trusted: &[IpNetwork], | |
| 306 | + | ) -> Option<IpAddr> { | |
| 307 | + | let peer = peer?; | |
| 308 | + | ||
| 309 | + | let is_trusted = |ip: IpAddr| trusted.iter().any(|n| n.contains(ip)); | |
| 310 | + | if !is_trusted(peer) { | |
| 311 | + | return Some(peer); | |
| 312 | + | } | |
| 313 | + | ||
| 314 | + | headers | |
| 315 | + | .get_all(X_FORWARDED_FOR) | |
| 316 | + | .iter() | |
| 317 | + | .filter_map(|v| v.to_str().ok()) | |
| 318 | + | .flat_map(|v| v.split(',')) | |
| 319 | + | .filter_map(parse_forwarded) | |
| 320 | + | .collect::<Vec<_>>() | |
| 321 | + | .into_iter() | |
| 322 | + | .rev() | |
| 323 | + | .find(|ip| !is_trusted(*ip)) | |
| 324 | + | // Every hop was a trusted proxy, or the header was absent: the peer is | |
| 325 | + | // the closest thing to a client we can honestly name. | |
| 326 | + | .or(Some(peer)) | |
| 327 | + | } | |
| 328 | + | ||
| 329 | + | /// The client address for this request, or the shared unknown-peer bucket. | |
| 330 | + | fn request_ip(state: &AppState, req: &Request) -> IpAddr { | |
| 331 | + | let peer = req | |
| 332 | + | .extensions() | |
| 333 | + | .get::<ConnectInfo<std::net::SocketAddr>>() | |
| 334 | + | .map(|c| c.0.ip()); | |
| 335 | + | client_ip(req.headers(), peer, &state.config.trusted_proxies).unwrap_or(UNKNOWN_PEER) | |
| 336 | + | } | |
| 337 | + | ||
| 338 | + | /// The coarse limiter, which runs *before* the session is resolved. | |
| 339 | + | /// | |
| 340 | + | /// Its only job is to stop an unauthenticated flood buying a database round trip | |
| 341 | + | /// per request: session resolution sits between this layer and [`layer`]. | |
| 342 | + | pub async fn edge_layer(State(state): State<AppState>, req: Request, next: Next) -> Response { | |
| 343 | + | let path = req.uri().path().to_owned(); | |
| 344 | + | if is_exempt(&path) { | |
| 345 | + | return next.run(req).await; | |
| 346 | + | } | |
| 347 | + | ||
| 348 | + | let ip = request_ip(&state, &req); | |
| 349 | + | if !state.limiter.acquire_edge(ip, Instant::now()) { | |
| 350 | + | tracing::warn!(%path, %ip, "rate limited at the edge"); | |
| 351 | + | return too_many(); | |
| 352 | + | } | |
| 353 | + | ||
| 354 | + | next.run(req).await | |
| 355 | + | } | |
| 356 | + | ||
| 208 | 357 | /// The rate-limiting middleware. | |
| 209 | 358 | /// | |
| 210 | 359 | /// `ConnectInfo` is optional so a missing peer address cannot turn every | |
| @@ −212,10 +361,6 @@ | |||
| 212 | 361 | /// `into_make_service_with_connect_info` — and its absence falls back to a | |
| 213 | 362 | /// single shared bucket, which is stricter than per-address, not looser. | |
| 214 | 363 | pub async fn layer(State(state): State<AppState>, req: Request, next: Next) -> Response { | |
| 215 | − | let addr = req | |
| 216 | − | .extensions() | |
| 217 | − | .get::<ConnectInfo<std::net::SocketAddr>>() | |
| 218 | − | .map(|c| c.0); | |
| 219 | 364 | let path = req.uri().path().to_owned(); | |
| 220 | 365 | if is_exempt(&path) { | |
| 221 | 366 | return next.run(req).await; | |
| @@ −226,22 +371,27 @@ | |||
| 226 | 371 | // neighbours' address. | |
| 227 | 372 | let key = match req.extensions().get::<CurrentUser>().and_then(|u| u.0.as_ref()) { | |
| 228 | 373 | Some(user) => Key::User(user.id), | |
| 229 | − | None => Key::Addr(addr.map(|a| a.ip()).unwrap_or(UNKNOWN_PEER)), | |
| 374 | + | None => Key::Addr(request_ip(&state, &req)), | |
| 230 | 375 | }; | |
| 231 | 376 | ||
| 232 | − | let Some(_guard) = state.limiter.acquire(key, is_auth_path(&path), Instant::now()) else { | |
| 233 | − | tracing::warn!(%path, peer = ?addr, "rate limited"); | |
| 234 | − | return ( | |
| 235 | − | StatusCode::TOO_MANY_REQUESTS, | |
| 236 | − | [(axum::http::header::RETRY_AFTER, "5")], | |
| 237 | − | "Too many requests. Try again in a moment.", | |
| 238 | − | ) | |
| 239 | − | .into_response(); | |
| 377 | + | let Some(_guard) = state.limiter.acquire(key.clone(), is_auth_path(&path), Instant::now()) | |
| 378 | + | else { | |
| 379 | + | tracing::warn!(%path, ?key, "rate limited"); | |
| 380 | + | return too_many(); | |
| 240 | 381 | }; | |
| 241 | 382 | ||
| 242 | 383 | next.run(req).await | |
| 243 | 384 | } | |
| 244 | 385 | ||
| 386 | + | fn too_many() -> Response { | |
| 387 | + | ( | |
| 388 | + | StatusCode::TOO_MANY_REQUESTS, | |
| 389 | + | [(axum::http::header::RETRY_AFTER, "5")], | |
| 390 | + | "Too many requests. Try again in a moment.", | |
| 391 | + | ) | |
| 392 | + | .into_response() | |
| 393 | + | } | |
| 394 | + | ||
| 245 | 395 | #[cfg(test)] | |
| 246 | 396 | mod tests { | |
| 247 | 397 | use super::*; | |
| @@ −250,6 +400,189 @@ | |||
| 250 | 400 | Key::Addr("10.0.0.1".parse().unwrap()) | |
| 251 | 401 | } | |
| 252 | 402 | ||
| 403 | + | // ─── attributing a request to a client ─────────────────────────────────── | |
| 404 | + | ||
| 405 | + | fn ip(s: &str) -> IpAddr { | |
| 406 | + | s.parse().unwrap() | |
| 407 | + | } | |
| 408 | + | ||
| 409 | + | fn nets(v: &[&str]) -> Vec<IpNetwork> { | |
| 410 | + | v.iter().map(|s| s.parse().unwrap()).collect() | |
| 411 | + | } | |
| 412 | + | ||
| 413 | + | fn xff(value: &str) -> HeaderMap { | |
| 414 | + | let mut h = HeaderMap::new(); | |
| 415 | + | h.insert(X_FORWARDED_FOR, value.parse().unwrap()); | |
| 416 | + | h | |
| 417 | + | } | |
| 418 | + | ||
| 419 | + | #[test] | |
| 420 | + | fn without_trusted_proxies_the_header_is_ignored() { | |
| 421 | + | // The spoofing case: believing this header from an arbitrary peer lets | |
| 422 | + | // any client mint a fresh bucket per request. | |
| 423 | + | let h = xff("1.2.3.4"); | |
| 424 | + | assert_eq!( | |
| 425 | + | client_ip(&h, Some(ip("203.0.113.9")), &[]), | |
| 426 | + | Some(ip("203.0.113.9")) | |
| 427 | + | ); | |
| 428 | + | } | |
| 429 | + | ||
| 430 | + | #[test] | |
| 431 | + | fn a_forged_header_from_an_untrusted_peer_is_ignored() { | |
| 432 | + | let h = xff("1.2.3.4"); | |
| 433 | + | let trusted = nets(&["172.23.0.0/16"]); | |
| 434 | + | assert_eq!( | |
| 435 | + | client_ip(&h, Some(ip("198.51.100.7")), &trusted), | |
| 436 | + | Some(ip("198.51.100.7")), | |
| 437 | + | "only the configured proxy may speak for a client" | |
| 438 | + | ); | |
| 439 | + | } | |
| 440 | + | ||
| 441 | + | #[test] | |
| 442 | + | fn behind_the_proxy_the_client_is_taken_from_the_header() { | |
| 443 | + | // The bug this exists to fix: without it every request looks like the | |
| 444 | + | // proxy and all anonymous traffic shares one bucket. | |
| 445 | + | let h = xff("203.0.113.9"); | |
| 446 | + | let trusted = nets(&["172.23.0.0/16"]); | |
| 447 | + | assert_eq!( | |
| 448 | + | client_ip(&h, Some(ip("172.23.0.2")), &trusted), | |
| 449 | + | Some(ip("203.0.113.9")) | |
| 450 | + | ); | |
| 451 | + | } | |
| 452 | + | ||
| 453 | + | #[test] | |
| 454 | + | fn a_client_cannot_prepend_its_way_to_a_fresh_bucket() { | |
| 455 | + | // A client that sends its own XFF has it *prepended* to by the proxy, | |
| 456 | + | // so the entries it controls are on the left. Reading from the right is | |
| 457 | + | // what makes them inert. | |
| 458 | + | let h = xff("9.9.9.9, 8.8.8.8, 203.0.113.9"); | |
| 459 | + | let trusted = nets(&["172.23.0.0/16"]); | |
| 460 | + | assert_eq!( | |
| 461 | + | client_ip(&h, Some(ip("172.23.0.2")), &trusted), | |
| 462 | + | Some(ip("203.0.113.9")), | |
| 463 | + | "the rightmost untrusted entry is the only honest one" | |
| 464 | + | ); | |
| 465 | + | } | |
| 466 | + | ||
| 467 | + | #[test] | |
| 468 | + | fn trusted_hops_are_skipped_from_the_right() { | |
| 469 | + | let h = xff("203.0.113.9, 172.23.0.5, 172.23.0.9"); | |
| 470 | + | let trusted = nets(&["172.23.0.0/16"]); | |
| 471 | + | assert_eq!( | |
| 472 | + | client_ip(&h, Some(ip("172.23.0.2")), &trusted), | |
| 473 | + | Some(ip("203.0.113.9")) | |
| 474 | + | ); | |
| 475 | + | } | |
| 476 | + | ||
| 477 | + | #[test] | |
| 478 | + | fn an_all_trusted_chain_falls_back_to_the_peer() { | |
| 479 | + | let h = xff("172.23.0.5"); | |
| 480 | + | let trusted = nets(&["172.23.0.0/16"]); | |
| 481 | + | assert_eq!( | |
| 482 | + | client_ip(&h, Some(ip("172.23.0.2")), &trusted), | |
| 483 | + | Some(ip("172.23.0.2")) | |
| 484 | + | ); | |
| 485 | + | } | |
| 486 | + | ||
| 487 | + | #[test] | |
| 488 | + | fn a_proxy_that_sends_no_header_falls_back_to_the_peer() { | |
| 489 | + | let trusted = nets(&["172.23.0.0/16"]); | |
| 490 | + | assert_eq!( | |
| 491 | + | client_ip(&HeaderMap::new(), Some(ip("172.23.0.2")), &trusted), | |
| 492 | + | Some(ip("172.23.0.2")) | |
| 493 | + | ); | |
| 494 | + | } | |
| 495 | + | ||
| 496 | + | #[test] | |
| 497 | + | fn forwarded_entries_parse_in_the_shapes_proxies_actually_send() { | |
| 498 | + | assert_eq!(parse_forwarded("1.2.3.4"), Some(ip("1.2.3.4"))); | |
| 499 | + | assert_eq!(parse_forwarded(" 1.2.3.4 "), Some(ip("1.2.3.4"))); | |
| 500 | + | assert_eq!(parse_forwarded("1.2.3.4:5678"), Some(ip("1.2.3.4"))); | |
| 501 | + | assert_eq!(parse_forwarded("::1"), Some(ip("::1"))); | |
| 502 | + | assert_eq!(parse_forwarded("[::1]"), Some(ip("::1"))); | |
| 503 | + | assert_eq!(parse_forwarded("[2001:db8::1]:443"), Some(ip("2001:db8::1"))); | |
| 504 | + | // Junk is discarded, never guessed at. | |
| 505 | + | assert_eq!(parse_forwarded(""), None); | |
| 506 | + | assert_eq!(parse_forwarded("unknown"), None); | |
| 507 | + | assert_eq!(parse_forwarded("_secret"), None); | |
| 508 | + | } | |
| 509 | + | ||
| 510 | + | #[test] | |
| 511 | + | fn a_missing_peer_yields_no_address() { | |
| 512 | + | assert_eq!(client_ip(&xff("1.2.3.4"), None, &nets(&["0.0.0.0/0"])), None); | |
| 513 | + | } | |
| 514 | + | ||
| 515 | + | #[test] | |
| 516 | + | fn distinct_clients_behind_one_proxy_get_distinct_buckets() { | |
| 517 | + | // The property the whole fix is for: two visitors must not be able to | |
| 518 | + | // spend each other's budget. | |
| 519 | + | let trusted = nets(&["172.23.0.0/16"]); | |
| 520 | + | let peer = Some(ip("172.23.0.2")); | |
| 521 | + | let a = client_ip(&xff("203.0.113.1"), peer, &trusted).unwrap(); | |
| 522 | + | let b = client_ip(&xff("203.0.113.2"), peer, &trusted).unwrap(); | |
| 523 | + | assert_ne!(a, b); | |
| 524 | + | ||
| 525 | + | let l = Limiter::new(); | |
| 526 | + | let now = Instant::now(); | |
| 527 | + | let mut held = Vec::new(); | |
| 528 | + | for _ in 0..MAX_CONCURRENT { | |
| 529 | + | held.push(l.acquire(Key::Addr(a), false, now).expect("under the cap")); | |
| 530 | + | } | |
| 531 | + | assert!( | |
| 532 | + | l.acquire(Key::Addr(a), false, now).is_none(), | |
| 533 | + | "the first client has spent its own budget" | |
| 534 | + | ); | |
| 535 | + | assert!( | |
| 536 | + | l.acquire(Key::Addr(b), false, now).is_some(), | |
| 537 | + | "one client must not be able to lock everyone else out" | |
| 538 | + | ); | |
| 539 | + | } | |
| 540 | + | ||
| 541 | + | // ─── the coarse pre-session bucket ─────────────────────────────────────── | |
| 542 | + | ||
| 543 | + | // Asserting on constants is the point: this pins a relationship between | |
| 544 | + | // them that a later edit could quietly break. | |
| 545 | + | #[test] | |
| 546 | + | #[allow(clippy::assertions_on_constants)] | |
| 547 | + | fn the_edge_bucket_is_far_looser_than_the_real_one() { | |
| 548 | + | // It must never be what stops ordinary traffic; the limiter after the | |
| 549 | + | // session is where policy lives. | |
| 550 | + | assert!(EDGE_BURST > BURST * 4.0); | |
| 551 | + | assert!(EDGE_REFILL_PER_SEC > REFILL_PER_SEC * 4.0); | |
| 552 | + | } | |
| 553 | + | ||
| 554 | + | #[test] | |
| 555 | + | fn the_edge_bucket_bites_eventually() { | |
| 556 | + | let l = Limiter::new(); | |
| 557 | + | let now = Instant::now(); | |
| 558 | + | for i in 0..EDGE_BURST as usize { | |
| 559 | + | assert!(l.acquire_edge(ip("10.0.0.1"), now), "request {i} of the burst"); | |
| 560 | + | } | |
| 561 | + | assert!(!l.acquire_edge(ip("10.0.0.1"), now), "the edge burst is spent"); | |
| 562 | + | assert!( | |
| 563 | + | l.acquire_edge(ip("10.0.0.2"), now), | |
| 564 | + | "and it is per-address, not global" | |
| 565 | + | ); | |
| 566 | + | } | |
| 567 | + | ||
| 568 | + | #[test] | |
| 569 | + | fn the_edge_bucket_takes_no_concurrency_slot() { | |
| 570 | + | // It runs before the handler is chosen and releases nothing, so it must | |
| 571 | + | // not consume the cap the real limiter enforces. | |
| 572 | + | let l = Limiter::new(); | |
| 573 | + | let now = Instant::now(); | |
| 574 | + | for _ in 0..50 { | |
| 575 | + | assert!(l.acquire_edge(ip("10.0.0.1"), now)); | |
| 576 | + | } | |
| 577 | + | let mut held = Vec::new(); | |
| 578 | + | for _ in 0..MAX_CONCURRENT { | |
| 579 | + | held.push( | |
| 580 | + | l.acquire(Key::Addr(ip("10.0.0.1")), false, now) | |
| 581 | + | .expect("the concurrency cap is untouched by the edge bucket"), | |
| 582 | + | ); | |
| 583 | + | } | |
| 584 | + | } | |
| 585 | + | ||
| 253 | 586 | #[test] | |
| 254 | 587 | fn a_burst_is_allowed_then_refused() { | |
| 255 | 588 | let l = Limiter::new(); | |
Mcrates/df-web/src/security_tests.rs+76−18
| @@ −178,11 +178,11 @@ | |||
| 178 | 178 | } | |
| 179 | 179 | ||
| 180 | 180 | /// A GET as a signed-in user. | |
| 181 | − | async fn get_as(&self, path: &str, session: &Uuid) -> Response { | |
| 182 | − | self.request(path, Some(*session)).await | |
| 181 | + | async fn get_as(&self, path: &str, session: &str) -> Response { | |
| 182 | + | self.request(path, Some(session)).await | |
| 183 | 183 | } | |
| 184 | 184 | ||
| 185 | − | async fn request(&self, path: &str, session: Option<Uuid>) -> Response { | |
| 185 | + | async fn request(&self, path: &str, session: Option<&str>) -> Response { | |
| 186 | 186 | let mut req = Request::builder().uri(path).method("GET"); | |
| 187 | 187 | if let Some(s) = session { | |
| 188 | 188 | req = req.header("cookie", format!("{}={s}", df_auth::session::COOKIE_NAME)); | |
| @@ −361,8 +361,11 @@ | |||
| 361 | 361 | }) | |
| 362 | 362 | } | |
| 363 | 363 | ||
| 364 | − | /// Seed a user and an active session, returning `(user_id, session_id)`. | |
| 365 | − | async fn user(db: &PgPool, handle: &str, admin: bool) -> (Uuid, Uuid) { | |
| 364 | + | /// Seed a user and an active session, returning `(user_id, session_token)`. | |
| 365 | + | /// | |
| 366 | + | /// The second element is the cookie value, which is the token and not the row | |
| 367 | + | /// id — the id is never accepted as a credential. | |
| 368 | + | async fn user(db: &PgPool, handle: &str, admin: bool) -> (Uuid, String) { | |
| 366 | 369 | let id = Uuid::now_v7(); | |
| 367 | 370 | sqlx::query( | |
| 368 | 371 | "INSERT INTO users (id, subject, handle, display_name, is_admin) | |
| @@ −376,15 +379,21 @@ | |||
| 376 | 379 | .await | |
| 377 | 380 | .expect("seeding a user"); | |
| 378 | 381 | ||
| 379 | − | let session = Uuid::now_v7(); | |
| 380 | − | sqlx::query("INSERT INTO sessions (id, user_id, expires_at) VALUES ($1, $2, now() + '14 days')") | |
| 381 | − | .bind(session) | |
| 382 | − | .bind(id) | |
| 383 | − | .execute(db) | |
| 384 | − | .await | |
| 385 | − | .expect("seeding a session"); | |
| 382 | + | // Minted the same way a real login does, so the tests exercise the token | |
| 383 | + | // path rather than a shape only the tests produce. | |
| 384 | + | let token = df_auth::session::generate_token(); | |
| 385 | + | sqlx::query( | |
| 386 | + | "INSERT INTO sessions (id, user_id, token_hash, expires_at) | |
| 387 | + | VALUES ($1, $2, encode(sha256($3::bytea), 'hex'), now() + '14 days')", | |
| 388 | + | ) | |
| 389 | + | .bind(Uuid::now_v7()) | |
| 390 | + | .bind(id) | |
| 391 | + | .bind(token.as_bytes()) | |
| 392 | + | .execute(db) | |
| 393 | + | .await | |
| 394 | + | .expect("seeding a session"); | |
| 386 | 395 | ||
| 387 | − | (id, session) | |
| 396 | + | (id, token) | |
| 388 | 397 | } | |
| 389 | 398 | ||
| 390 | 399 | /// Seed a repository owned by `owner`, returning its id. | |
| @@ −539,7 +548,7 @@ | |||
| 539 | 548 | "/alice/hidden/issues/1", | |
| 540 | 549 | "/alice/hidden/stacks/klxqnvpqlnlvtkmuqmtmxktlnvnomvwv", | |
| 541 | 550 | ] { | |
| 542 | − | for session in [None, Some(mallory)] { | |
| 551 | + | for session in [None, Some(mallory.as_str())] { | |
| 543 | 552 | let res = h.request(path, session).await; | |
| 544 | 553 | assert_eq!(res.status(), StatusCode::NOT_FOUND, "{path} was reachable"); | |
| 545 | 554 | let body = h.body(res).await; | |
| @@ −615,7 +624,7 @@ | |||
| 615 | 624 | "/bob/public-fork/issues/1", | |
| 616 | 625 | "/alice/private-parent", | |
| 617 | 626 | ] { | |
| 618 | − | for session in [None, Some(mallory)] { | |
| 627 | + | for session in [None, Some(mallory.as_str())] { | |
| 619 | 628 | let res = h.request(path, session).await; | |
| 620 | 629 | assert_eq!(res.status(), StatusCode::NOT_FOUND, "{path}"); | |
| 621 | 630 | let body = h.body(res).await; | |
| @@ −644,7 +653,7 @@ | |||
| 644 | 653 | issue(&h.db, hidden, 1, "widgetconfidential issue").await; | |
| 645 | 654 | change(&h.db, public, 1, "mmmmnvpqlnlvtkmuqmtmxktlnvnomvwv", "widgetpublic change").await; | |
| 646 | 655 | ||
| 647 | − | for session in [None, Some(mallory)] { | |
| 656 | + | for session in [None, Some(mallory.as_str())] { | |
| 648 | 657 | let res = h.request("/search?q=widget", session).await; | |
| 649 | 658 | assert_eq!(res.status(), StatusCode::OK); | |
| 650 | 659 | let body = h.body(res).await; | |
| @@ −722,7 +731,7 @@ | |||
| 722 | 731 | change(&h.db, hidden, 1, "klxqnvpqlnlvtkmuqmtmxktlnvnomvwv", "widgetconfidential change").await; | |
| 723 | 732 | change(&h.db, public, 1, "mmmmnvpqlnlvtkmuqmtmxktlnvnomvwv", "widgetpublic change").await; | |
| 724 | 733 | ||
| 725 | − | for session in [None, Some(mallory)] { | |
| 734 | + | for session in [None, Some(mallory.as_str())] { | |
| 726 | 735 | let res = h.request("/search?q=widget&fragment=1", session).await; | |
| 727 | 736 | assert_eq!(res.status(), StatusCode::OK); | |
| 728 | 737 | let body = h.body(res).await; | |
| @@ −756,7 +765,7 @@ | |||
| 756 | 765 | repo(&h.db, alice, "unlistable-repo", true).await; | |
| 757 | 766 | repo(&h.db, alice, "listable-repo", false).await; | |
| 758 | 767 | ||
| 759 | − | for session in [None, Some(mallory)] { | |
| 768 | + | for session in [None, Some(mallory.as_str())] { | |
| 760 | 769 | let res = h.request("/alice", session).await; | |
| 761 | 770 | assert_eq!(res.status(), StatusCode::OK); | |
| 762 | 771 | let body = h.body(res).await; | |
| @@ −1013,6 +1022,55 @@ | |||
| 1013 | 1022 | h.drop_schema().await; | |
| 1014 | 1023 | } | |
| 1015 | 1024 | ||
| 1025 | + | /// The session cookie is a bearer token, and the row id is not a credential. | |
| 1026 | + | /// | |
| 1027 | + | /// The cookie used to be `sessions.id`, a UUIDv7 — time-ordered, with a counter | |
| 1028 | + | /// that only reseeds once a millisecond, so ids minted together share their | |
| 1029 | + | /// leading bits and any other id from the same generator narrows the rest. The | |
| 1030 | + | /// id is still the primary key; what this pins is that presenting it no longer | |
| 1031 | + | /// signs anybody in. | |
| 1032 | + | #[tokio::test] | |
| 1033 | + | async fn a_session_row_id_is_not_a_credential() { | |
| 1034 | + | let Some(h) = harness("sessionid").await else { return }; | |
| 1035 | + | ||
| 1036 | + | let (alice, token) = user(&h.db, "alice", false).await; | |
| 1037 | + | repo(&h.db, alice, "hidden", true).await; | |
| 1038 | + | ||
| 1039 | + | // The token works. | |
| 1040 | + | assert_eq!( | |
| 1041 | + | h.get_as("/alice/hidden", &token).await.status(), | |
| 1042 | + | StatusCode::OK, | |
| 1043 | + | "the session token must sign alice in" | |
| 1044 | + | ); | |
| 1045 | + | ||
| 1046 | + | // The row id, which is what the old cookie carried, does not. | |
| 1047 | + | let id: Uuid = sqlx::query_scalar("SELECT id FROM sessions WHERE user_id = $1") | |
| 1048 | + | .bind(alice) | |
| 1049 | + | .fetch_one(&h.db) | |
| 1050 | + | .await | |
| 1051 | + | .expect("the seeded session"); | |
| 1052 | + | assert_eq!( | |
| 1053 | + | h.get_as("/alice/hidden", &id.to_string()).await.status(), | |
| 1054 | + | StatusCode::NOT_FOUND, | |
| 1055 | + | "a session id must not authenticate — it is an identifier, not a secret" | |
| 1056 | + | ); | |
| 1057 | + | ||
| 1058 | + | // Neither does the stored hash, which is what a database leak would yield. | |
| 1059 | + | let hash: String = sqlx::query_scalar("SELECT token_hash FROM sessions WHERE user_id = $1") | |
| 1060 | + | .bind(alice) | |
| 1061 | + | .fetch_one(&h.db) | |
| 1062 | + | .await | |
| 1063 | + | .expect("the seeded session"); | |
| 1064 | + | assert_ne!(hash, token, "the plaintext token must not be stored"); | |
| 1065 | + | assert_eq!( | |
| 1066 | + | h.get_as("/alice/hidden", &hash).await.status(), | |
| 1067 | + | StatusCode::NOT_FOUND, | |
| 1068 | + | "the stored hash must not be replayable as the token" | |
| 1069 | + | ); | |
| 1070 | + | ||
| 1071 | + | h.drop_schema().await; | |
| 1072 | + | } | |
| 1073 | + | ||
| 1016 | 1074 | /// An archived repository is read-only. The settings page promises that in so | |
| 1017 | 1075 | /// many words, so it has to be true on the wire — enforcing it only in the UI | |
| 1018 | 1076 | /// would leave the promise false for every Git client. | |
Mcrates/df-store/src/git/diff.rs+12−4
| @@ −87,9 +87,16 @@ | |||
| 87 | 87 | } | |
| 88 | 88 | ||
| 89 | 89 | use gix::object::tree::diff::Change; | |
| 90 | + | ||
| 91 | + | // Blobs only. The walk reports the directories on the way down as | |
| 92 | + | // well as the files inside them, and a tree entry has no content to | |
| 93 | + | // diff — it used to reach the renderer as a "binary file" with no | |
| 94 | + | // hunks, so a change touching four crates listed eleven phantom | |
| 95 | + | // files nobody edited. `is_blob` also excludes symlinks and | |
| 96 | + | // submodule gitlinks, which is what the old check was reaching for. | |
| 90 | 97 | let pc = match change { | |
| 91 | 98 | Change::Addition { entry_mode, id, .. } => { | |
| 92 | − | (!entry_mode.is_link() && !entry_mode.is_commit()).then(|| PathChange { | |
| 99 | + | entry_mode.is_blob().then(|| PathChange { | |
| 93 | 100 | path: path.clone(), | |
| 94 | 101 | old_path: None, | |
| 95 | 102 | kind: ChangeKind::Added, | |
| @@ −98,7 +105,7 @@ | |||
| 98 | 105 | }) | |
| 99 | 106 | } | |
| 100 | 107 | Change::Deletion { entry_mode, id, .. } => { | |
| 101 | − | (!entry_mode.is_link() && !entry_mode.is_commit()).then(|| PathChange { | |
| 108 | + | entry_mode.is_blob().then(|| PathChange { | |
| 102 | 109 | path: path.clone(), | |
| 103 | 110 | old_path: None, | |
| 104 | 111 | kind: ChangeKind::Deleted, | |
| @@ −111,7 +118,7 @@ | |||
| 111 | 118 | previous_id, | |
| 112 | 119 | id, | |
| 113 | 120 | .. | |
| 114 | − | } => (!entry_mode.is_link() && !entry_mode.is_commit()).then(|| PathChange { | |
| 121 | + | } => entry_mode.is_blob().then(|| PathChange { | |
| 115 | 122 | path: path.clone(), | |
| 116 | 123 | old_path: None, | |
| 117 | 124 | kind: ChangeKind::Modified, | |
| @@ −119,11 +126,12 @@ | |||
| 119 | 126 | new_id: Some(id.detach()), | |
| 120 | 127 | }), | |
| 121 | 128 | Change::Rewrite { | |
| 129 | + | entry_mode, | |
| 122 | 130 | source_location, | |
| 123 | 131 | source_id, | |
| 124 | 132 | id, | |
| 125 | 133 | .. | |
| 126 | − | } => Some(PathChange { | |
| 134 | + | } => entry_mode.is_blob().then(|| PathChange { | |
| 127 | 135 | path: path.clone(), | |
| 128 | 136 | old_path: Some(source_location.to_string()), | |
| 129 | 137 | kind: ChangeKind::Renamed, | |
Mcrates/df-web/src/git_http/mod.rs+9−1
| @@ −357,6 +357,11 @@ | |||
| 357 | 357 | .stdin(Stdio::piped()) | |
| 358 | 358 | .stdout(Stdio::piped()) | |
| 359 | 359 | .stderr(Stdio::piped()) | |
| 360 | + | // Without this the `GIT_TIMEOUT` below bounds how long we *wait*, not | |
| 361 | + | // how long git runs: tokio does not kill a child when its handle is | |
| 362 | + | // dropped unless asked, so a timed-out `upload-pack` on a hostile | |
| 363 | + | // repository would keep burning CPU with nothing left watching it. | |
| 364 | + | .kill_on_drop(true) | |
| 360 | 365 | // Never let a repository's own config influence what we run, and never | |
| 361 | 366 | // let git prompt for anything. | |
| 362 | 367 | .env_clear() | |
| @@ −436,7 +441,10 @@ | |||
| 436 | 441 | return Err(AppError::Internal(anyhow::anyhow!("running git: {e}"))); | |
| 437 | 442 | } | |
| 438 | 443 | Err(_) => { | |
| 439 | − | tracing::error!(?args, "git operation timed out"); | |
| 444 | + | // Dropping the timed-out future drops the `Child`, and | |
| 445 | + | // `kill_on_drop` above turns that into a SIGKILL. The process is | |
| 446 | + | // gone by the time this returns rather than orphaned. | |
| 447 | + | tracing::error!(?args, "git operation timed out; killing it"); | |
| 440 | 448 | return Err(AppError::Internal(anyhow::anyhow!("git timed out"))); | |
| 441 | 449 | } | |
| 442 | 450 | }; | |
Mcrates/df-web/src/routes/auth.rs+98−9
| @@ −198,6 +198,13 @@ | |||
| 198 | 198 | /// selection. Short-lived and self-contained, so no extra table is needed. | |
| 199 | 199 | const PENDING_COOKIE: &str = "dogfood_pending"; | |
| 200 | 200 | ||
| 201 | + | /// How long a pending identity stays usable. | |
| 202 | + | /// | |
| 203 | + | /// Enforced inside the signature, not only as the cookie's `Max-Age`: `Max-Age` | |
| 204 | + | /// is a request to the browser, and a copy of the cookie taken anywhere else | |
| 205 | + | /// would otherwise stay redeemable for an account forever. | |
| 206 | + | const PENDING_TTL_MINUTES: i64 = 15; | |
| 207 | + | ||
| 201 | 208 | fn stash_pending( | |
| 202 | 209 | jar: CookieJar, | |
| 203 | 210 | state: &AppState, | |
| @@ −208,6 +215,7 @@ | |||
| 208 | 215 | "email": identity.email, | |
| 209 | 216 | "name": identity.name, | |
| 210 | 217 | "id_token": identity.id_token, | |
| 218 | + | "exp": (chrono::Utc::now() + chrono::Duration::minutes(PENDING_TTL_MINUTES)).timestamp(), | |
| 211 | 219 | }) | |
| 212 | 220 | .to_string(); | |
| 213 | 221 | ||
| @@ −220,7 +228,7 @@ | |||
| 220 | 228 | .secure(state.config.secure_cookies()) | |
| 221 | 229 | .http_only(true) | |
| 222 | 230 | .same_site(SameSite::Lax) | |
| 223 | − | .max_age(time::Duration::minutes(15)) | |
| 231 | + | .max_age(time::Duration::minutes(PENDING_TTL_MINUTES)) | |
| 224 | 232 | .build(), | |
| 225 | 233 | )) | |
| 226 | 234 | } | |
| @@ −234,9 +242,16 @@ | |||
| 234 | 242 | } | |
| 235 | 243 | ||
| 236 | 244 | fn read_pending(secret: &[u8], jar: &CookieJar) -> Option<df_auth::Identity> { | |
| 245 | + | verify_pending(secret, jar.get(PENDING_COOKIE)?.value()) | |
| 246 | + | } | |
| 247 | + | ||
| 248 | + | /// Verify and decode a pending-identity cookie value. | |
| 249 | + | /// | |
| 250 | + | /// Split out from [`read_pending`] so the signature and expiry rules are | |
| 251 | + | /// testable without building a cookie jar. | |
| 252 | + | fn verify_pending(secret: &[u8], raw: &str) -> Option<df_auth::Identity> { | |
| 237 | 253 | use subtle::ConstantTimeEq; | |
| 238 | 254 | ||
| 239 | − | let raw = jar.get(PENDING_COOKIE)?.value(); | |
| 240 | 255 | let (payload_hex, mac) = raw.split_once('.')?; | |
| 241 | 256 | let payload = String::from_utf8(hex::decode(payload_hex).ok()?).ok()?; | |
| 242 | 257 | ||
| @@ −247,6 +262,15 @@ | |||
| 247 | 262 | } | |
| 248 | 263 | ||
| 249 | 264 | let v: serde_json::Value = serde_json::from_str(&payload).ok()?; | |
| 265 | + | ||
| 266 | + | // The signature proves we minted it; `exp` is what stops it being minted | |
| 267 | + | // once and redeemed indefinitely. A payload without one predates this and | |
| 268 | + | // is refused rather than grandfathered. | |
| 269 | + | let exp = v.get("exp").and_then(serde_json::Value::as_i64)?; | |
| 270 | + | if chrono::Utc::now().timestamp() > exp { | |
| 271 | + | return None; | |
| 272 | + | } | |
| 273 | + | ||
| 250 | 274 | Some(df_auth::Identity { | |
| 251 | 275 | subject: v.get("sub")?.as_str()?.to_string(), | |
| 252 | 276 | email: v.get("email").and_then(|e| e.as_str()).map(str::to_string), | |
| @@ −336,7 +360,7 @@ | |||
| 336 | 360 | .get(axum::http::header::USER_AGENT) | |
| 337 | 361 | .and_then(|v| v.to_str().ok()); | |
| 338 | 362 | ||
| 339 | − | let (id, expires) = session::create( | |
| 363 | + | let s = session::create( | |
| 340 | 364 | &state.db, | |
| 341 | 365 | user_id, | |
| 342 | 366 | state.config.session_ttl_days, | |
| @@ −345,8 +369,10 @@ | |||
| 345 | 369 | id_token, | |
| 346 | 370 | ) | |
| 347 | 371 | .await?; | |
| 372 | + | let expires = s.expires_at; | |
| 348 | 373 | ||
| 349 | − | let cookie = Cookie::build((session::COOKIE_NAME, id.to_string())) | |
| 374 | + | // The cookie carries the token. The row id never leaves the server. | |
| 375 | + | let cookie = Cookie::build((session::COOKIE_NAME, s.token)) | |
| 350 | 376 | .path("/") | |
| 351 | 377 | .secure(state.config.secure_cookies()) | |
| 352 | 378 | .http_only(true) | |
| @@ −368,11 +394,10 @@ | |||
| 368 | 394 | ) -> AppResult<Response> { | |
| 369 | 395 | let mut id_token_hint = None; | |
| 370 | 396 | if let Some(raw) = jar.get(session::COOKIE_NAME) { | |
| 371 | − | if let Ok(id) = raw.value().parse::<uuid::Uuid>() { | |
| 372 | − | id_token_hint = session::id_token(&state.db, id).await.unwrap_or_default(); | |
| 373 | − | if let Err(e) = session::destroy(&state.db, id).await { | |
| 374 | − | tracing::warn!("destroying session failed: {e}"); | |
| 375 | − | } | |
| 397 | + | // Deletes the row and hands back the stashed ID token in one statement. | |
| 398 | + | match session::destroy(&state.db, raw.value()).await { | |
| 399 | + | Ok(hint) => id_token_hint = hint, | |
| 400 | + | Err(e) => tracing::warn!("destroying session failed: {e}"), | |
| 376 | 401 | } | |
| 377 | 402 | } | |
| 378 | 403 | ||
| @@ −430,4 +455,68 @@ | |||
| 430 | 455 | // A changed payload must not verify against the old tag. | |
| 431 | 456 | assert_ne!(mac, sign_pending(secret, r#"{"sub":"attacker"}"#)); | |
| 432 | 457 | } | |
| 458 | + | ||
| 459 | + | const PENDING_SECRET: &[u8] = b"secret-key-for-pending-identity-cookie"; | |
| 460 | + | ||
| 461 | + | /// Build a cookie value the way `stash_pending` does, with a chosen expiry. | |
| 462 | + | fn pending_cookie(exp: i64) -> String { | |
| 463 | + | let payload = serde_json::json!({ | |
| 464 | + | "sub": "abc", | |
| 465 | + | "email": "a@b.c", | |
| 466 | + | "name": null, | |
| 467 | + | "id_token": null, | |
| 468 | + | "exp": exp, | |
| 469 | + | }) | |
| 470 | + | .to_string(); | |
| 471 | + | format!( | |
| 472 | + | "{}.{}", | |
| 473 | + | hex::encode(&payload), | |
| 474 | + | sign_pending(PENDING_SECRET, &payload) | |
| 475 | + | ) | |
| 476 | + | } | |
| 477 | + | ||
| 478 | + | #[test] | |
| 479 | + | fn a_live_pending_cookie_verifies() { | |
| 480 | + | let raw = pending_cookie(chrono::Utc::now().timestamp() + 600); | |
| 481 | + | let identity = verify_pending(PENDING_SECRET, &raw).expect("should verify"); | |
| 482 | + | assert_eq!(identity.subject, "abc"); | |
| 483 | + | } | |
| 484 | + | ||
| 485 | + | #[test] | |
| 486 | + | fn an_expired_pending_cookie_is_refused_even_though_it_is_signed() { | |
| 487 | + | // The attack `Max-Age` alone does not stop: the browser's copy is gone, | |
| 488 | + | // but a copy taken anywhere else still carries our signature. | |
| 489 | + | let raw = pending_cookie(chrono::Utc::now().timestamp() - 1); | |
| 490 | + | assert!(verify_pending(PENDING_SECRET, &raw).is_none()); | |
| 491 | + | } | |
| 492 | + | ||
| 493 | + | #[test] | |
| 494 | + | fn a_pending_cookie_without_an_expiry_is_refused() { | |
| 495 | + | // The pre-expiry format. Grandfathering it in would leave the old | |
| 496 | + | // indefinitely-redeemable cookie working. | |
| 497 | + | let payload = r#"{"sub":"abc","email":"a@b.c","name":null}"#; | |
| 498 | + | let raw = format!( | |
| 499 | + | "{}.{}", | |
| 500 | + | hex::encode(payload), | |
| 501 | + | sign_pending(PENDING_SECRET, payload) | |
| 502 | + | ); | |
| 503 | + | assert!(verify_pending(PENDING_SECRET, &raw).is_none()); | |
| 504 | + | } | |
| 505 | + | ||
| 506 | + | #[test] | |
| 507 | + | fn a_pending_cookie_cannot_have_its_expiry_extended() { | |
| 508 | + | let raw = pending_cookie(chrono::Utc::now().timestamp() - 1); | |
| 509 | + | let (payload_hex, mac) = raw.split_once('.').unwrap(); | |
| 510 | + | let payload = String::from_utf8(hex::decode(payload_hex).unwrap()).unwrap(); | |
| 511 | + | ||
| 512 | + | // Push the expiry out without re-signing, which is all an attacker can do. | |
| 513 | + | let forged = payload.replace( | |
| 514 | + | &format!("\"exp\":{}", chrono::Utc::now().timestamp() - 1), | |
| 515 | + | &format!("\"exp\":{}", chrono::Utc::now().timestamp() + 86_400), | |
| 516 | + | ); | |
| 517 | + | assert_ne!(forged, payload, "the test must actually change the expiry"); | |
| 518 | + | ||
| 519 | + | let tampered = format!("{}.{mac}", hex::encode(&forged)); | |
| 520 | + | assert!(verify_pending(PENDING_SECRET, &tampered).is_none()); | |
| 521 | + | } | |
| 433 | 522 | } | |
Mcrates/df-web/src/routes/health.rs+28−6
| @@ −1,9 +1,14 @@ | |||
| 1 | 1 | //! Liveness and readiness. | |
| 2 | 2 | //! | |
| 3 | 3 | //! `/healthz` answers whether the process is up; `/readyz` answers whether it | |
| 4 | − | //! can serve, which means the database is reachable. Container orchestration | |
| 5 | − | //! restarts on the former and withholds traffic on the latter, so conflating | |
| 6 | − | //! them turns a transient database blip into a restart loop. | |
| 4 | + | //! can serve, which means the database is reachable and the pool can still hand | |
| 5 | + | //! out a connection. | |
| 6 | + | //! | |
| 7 | + | //! The container healthcheck reads `/readyz`. It used to read `/healthz` on the | |
| 8 | + | //! reasoning that restarting on readiness turns a transient database blip into a | |
| 9 | + | //! restart loop — but nothing restarts on either signal here (compose restarts | |
| 10 | + | //! on exit), so all that bought was a container reporting healthy while it | |
| 11 | + | //! served nothing but 500s. | |
| 7 | 12 | ||
| 8 | 13 | use axum::extract::State; | |
| 9 | 14 | use axum::http::StatusCode; | |
| @@ −16,11 +21,28 @@ | |||
| 16 | 21 | } | |
| 17 | 22 | ||
| 18 | 23 | pub async fn readyz(State(state): State<AppState>) -> impl IntoResponse { | |
| 24 | + | // Reported either way: a pool that is at its ceiling with nothing idle is | |
| 25 | + | // the signature of connections stuck in the pool rather than of a database | |
| 26 | + | // that has gone away, and the two need different responses from whoever is | |
| 27 | + | // reading this. | |
| 28 | + | let open = state.db.size(); | |
| 29 | + | let idle = state.db.num_idle(); | |
| 30 | + | ||
| 19 | 31 | match df_db::ping(&state.db).await { | |
| 20 | − | Ok(()) => (StatusCode::OK, "ready\n"), | |
| 32 | + | Ok(()) => ( | |
| 33 | + | StatusCode::OK, | |
| 34 | + | format!("ready\npool: {idle} idle / {open} open\n"), | |
| 35 | + | ), | |
| 21 | 36 | Err(e) => { | |
| 22 | − | tracing::error!("readiness check failed: {e}"); | |
| 23 | − | (StatusCode::SERVICE_UNAVAILABLE, "database unreachable\n") | |
| 37 | + | tracing::error!( | |
| 38 | + | pool_open = open, | |
| 39 | + | pool_idle = idle, | |
| 40 | + | "readiness check failed: {e}" | |
| 41 | + | ); | |
| 42 | + | ( | |
| 43 | + | StatusCode::SERVICE_UNAVAILABLE, | |
| 44 | + | format!("database unreachable\npool: {idle} idle / {open} open\n"), | |
| 45 | + | ) | |
| 24 | 46 | } | |
| 25 | 47 | } | |
| 26 | 48 | } | |
Mcrates/df-web/src/routes/repo.rs+73−0
| @@ −626,6 +626,79 @@ | |||
| 626 | 626 | Ok(render(&ctx, "code", &csrf, &nonce, user.as_deref(), body)) | |
| 627 | 627 | } | |
| 628 | 628 | ||
| 629 | + | #[derive(Deserialize, Default)] | |
| 630 | + | pub struct CommitQuery { | |
| 631 | + | /// Fold every file. A link rather than a script, so the folded view is a | |
| 632 | + | /// URL and works with scripting off. | |
| 633 | + | pub collapse: Option<String>, | |
| 634 | + | } | |
| 635 | + | ||
| 636 | + | /// `GET /{owner}/{repo}/commit/{rev}` | |
| 637 | + | /// | |
| 638 | + | /// One commit and its patch against the first parent. A root commit has no | |
| 639 | + | /// parent and diffs against an empty tree, so the first commit in a repository | |
| 640 | + | /// renders as an all-additions patch rather than as an error. | |
| 641 | + | pub async fn commit( | |
| 642 | + | State(state): State<AppState>, | |
| 643 | + | UrlPath((owner, name, rev_spec)): UrlPath<(String, String, String)>, | |
| 644 | + | Query(q): Query<CommitQuery>, | |
| 645 | + | CurrentUser(user): CurrentUser, | |
| 646 | + | CsrfToken(csrf): CsrfToken, | |
| 647 | + | Nonce(nonce): Nonce, | |
| 648 | + | ) -> AppResult<Response> { | |
| 649 | + | let ctx = RepoContext::load(&state, &owner, &name, user.as_deref()).await?; | |
| 650 | + | ||
| 651 | + | // Bookmark names resolve too: `/commit/main` is a reasonable thing to type, | |
| 652 | + | // and it lands on whichever commit that name points at right now. | |
| 653 | + | let rev = state | |
| 654 | + | .store | |
| 655 | + | .resolve(ctx.store_id(), &rev_spec) | |
| 656 | + | .await | |
| 657 | + | .map_err(store_err)?; | |
| 658 | + | ||
| 659 | + | let revision = state | |
| 660 | + | .store | |
| 661 | + | .revision(ctx.store_id(), &rev) | |
| 662 | + | .await | |
| 663 | + | .map_err(store_err)?; | |
| 664 | + | ||
| 665 | + | let opts = df_store::DiffOpts { | |
| 666 | + | context_lines: 3, | |
| 667 | + | max_files: state.config.max_diff_files, | |
| 668 | + | max_lines: state.config.max_diff_lines, | |
| 669 | + | }; | |
| 670 | + | ||
| 671 | + | // Best-effort: a commit whose patch cannot be rendered still has a message, | |
| 672 | + | // an author and parents worth showing, and the view says so in place of the | |
| 673 | + | // diff rather than 500-ing the page. | |
| 674 | + | let diff = match state.store.diff_from_parent(ctx.store_id(), &rev, opts).await { | |
| 675 | + | Ok(d) => Some(d), | |
| 676 | + | Err(e) => { | |
| 677 | + | tracing::warn!("diffing {rev} failed: {e}"); | |
| 678 | + | None | |
| 679 | + | } | |
| 680 | + | }; | |
| 681 | + | ||
| 682 | + | let handles = handles_for_emails( | |
| 683 | + | &state, | |
| 684 | + | [revision.author.email.as_str(), revision.committer.email.as_str()], | |
| 685 | + | ) | |
| 686 | + | .await; | |
| 687 | + | ||
| 688 | + | let body = v::commit_view( | |
| 689 | + | &ctx, | |
| 690 | + | v::CommitPage { | |
| 691 | + | rev: &revision, | |
| 692 | + | author_handle: handles.get(&revision.author.email).map(String::as_str), | |
| 693 | + | committer_handle: handles.get(&revision.committer.email).map(String::as_str), | |
| 694 | + | diff: diff.as_ref(), | |
| 695 | + | collapsed: q.collapse.is_some(), | |
| 696 | + | }, | |
| 697 | + | ); | |
| 698 | + | ||
| 699 | + | Ok(render(&ctx, "code", &csrf, &nonce, user.as_deref(), body)) | |
| 700 | + | } | |
| 701 | + | ||
| 629 | 702 | /// `GET /{owner}/{repo}/bookmarks` | |
| 630 | 703 | pub async fn bookmarks( | |
| 631 | 704 | State(state): State<AppState>, | |
Mcrates/df-web/src/routes/review.rs+4−0
| @@ −329,6 +329,9 @@ | |||
| 329 | 329 | pub struct FilesQuery { | |
| 330 | 330 | pub rev: Option<String>, | |
| 331 | 331 | pub against: Option<String>, | |
| 332 | + | /// Fold every file. A link rather than a script, so the folded view is a | |
| 333 | + | /// URL and works with scripting off. | |
| 334 | + | pub collapse: Option<String>, | |
| 332 | 335 | } | |
| 333 | 336 | ||
| 334 | 337 | /// `GET /{owner}/{repo}/changes/{ref}/files` | |
| @@ −409,6 +412,7 @@ | |||
| 409 | 412 | rev: rev.as_deref().unwrap_or(""), | |
| 410 | 413 | against: against.as_deref(), | |
| 411 | 414 | revisions: &l.revisions, | |
| 415 | + | collapsed: q.collapse.is_some(), | |
| 412 | 416 | }))) | |
| 413 | 417 | }; | |
| 414 | 418 | ||
Mcrates/df-web/src/views/mod.rs+1−0
| @@ −5,6 +5,7 @@ | |||
| 5 | 5 | ||
| 6 | 6 | pub mod change; | |
| 7 | 7 | pub mod design; | |
| 8 | + | pub mod diff; | |
| 8 | 9 | pub mod edit; | |
| 9 | 10 | pub mod issue; | |
| 10 | 11 | pub mod layout; | |
Mcrates/df-web/src/views/repo.rs+305−19
| @@ −331,7 +331,6 @@ | |||
| 331 | 331 | caption .sr-only { "Files in this directory" } | |
| 332 | 332 | thead { | |
| 333 | 333 | tr { | |
| 334 | − | th .filelist-icon { span .sr-only { "Kind" } } | |
| 335 | 334 | th .filelist-name { "Name" } | |
| 336 | 335 | th .filelist-message { "Last change" } | |
| 337 | 336 | th .filelist-change { "Change" } | |
| @@ −341,31 +340,31 @@ | |||
| 341 | 340 | tbody { | |
| 342 | 341 | @if !path.is_empty() { | |
| 343 | 342 | tr { | |
| 344 | − | td .filelist-icon { | |
| 345 | − | a href=(parent_link(&base, rev_label, path)) aria-label="Parent directory" { | |
| 346 | − | (dir_icon()) | |
| 347 | − | } | |
| 348 | − | } | |
| 349 | 343 | td .filelist-name colspan="4" { | |
| 350 | − | a .mono href=(parent_link(&base, rev_label, path)) { ".." } | |
| 344 | + | span .filelist-entry { | |
| 345 | + | a .filelist-entry-link.mono | |
| 346 | + | href=(parent_link(&base, rev_label, path)) | |
| 347 | + | aria-label="Parent directory" { | |
| 348 | + | (dir_icon()) | |
| 349 | + | span .filelist-entry-name { ".." } | |
| 350 | + | } | |
| 351 | + | } | |
| 351 | 352 | } | |
| 352 | 353 | } | |
| 353 | 354 | } | |
| 354 | 355 | @for e in entries { | |
| 355 | 356 | tr { | |
| 356 | − | td .filelist-icon { | |
| 357 | − | a href=(entry_link(&base, rev_label, e)) tabindex="-1" aria-hidden="true" { | |
| 358 | − | @if e.kind == EntryKind::Directory { (dir_icon()) } @else { (file_icon()) } | |
| 359 | − | } | |
| 360 | − | } | |
| 361 | 357 | td .filelist-name { | |
| 362 | − | a .mono href=(entry_link(&base, rev_label, e)) | |
| 363 | − | .is-dir[e.kind == EntryKind::Directory] { | |
| 364 | − | (e.name) | |
| 358 | + | span .filelist-entry { | |
| 359 | + | a .filelist-entry-link.mono href=(entry_link(&base, rev_label, e)) | |
| 360 | + | .is-dir[e.kind == EntryKind::Directory] { | |
| 361 | + | @if e.kind == EntryKind::Directory { (dir_icon()) } @else { (file_icon()) } | |
| 362 | + | span .filelist-entry-name { (e.name) } | |
| 363 | + | } | |
| 364 | + | @if e.kind == EntryKind::Symlink { | |
| 365 | + | span .faint .filelist-note { "symlink" } | |
| 366 | + | } | |
| 365 | 367 | } | |
| 366 | − | @if e.kind == EntryKind::Symlink { | |
| 367 | − | span .faint .filelist-note { "symlink" } | |
| 368 | − | } | |
| 369 | 368 | } | |
| 370 | 369 | @match history.get(&e.name) { | |
| 371 | 370 | Some(h) => { | |
| @@ −852,7 +851,11 @@ | |||
| 852 | 851 | @for r in revisions { | |
| 853 | 852 | div style="padding:10px 0;border-bottom:1px solid var(--border)" { | |
| 854 | 853 | div .row { | |
| 855 | − | a href=(format!("{base}/tree/{}/", r.rev)) { (r.summary()) } | |
| 854 | + | // The message leads to the commit's own page — | |
| 855 | + | // what it changed is the question a log row | |
| 856 | + | // raises, and the tree at that revision is not | |
| 857 | + | // an answer to it. | |
| 858 | + | a href=(format!("{base}/commit/{}", r.rev)) { (r.summary()) } | |
| 856 | 859 | @if r.conflicted { | |
| 857 | 860 | span .badge.badge-conflict { "conflict" } | |
| 858 | 861 | } | |
| @@ −867,6 +870,10 @@ | |||
| 867 | 870 | } @else { | |
| 868 | 871 | span .chip title="authored with plain git" { "git" } | |
| 869 | 872 | } | |
| 873 | + | a .mono.faint href=(format!("{base}/commit/{}", r.rev)) | |
| 874 | + | title=(r.rev.as_str()) { | |
| 875 | + | (df_store::abbreviate_rev(r.rev.as_str())) | |
| 876 | + | } | |
| 870 | 877 | span .faint { | |
| 871 | 878 | (crate::views::person( | |
| 872 | 879 | handles.get(&r.author.email).map(String::as_str), | |
| @@ −883,6 +890,142 @@ | |||
| 883 | 890 | } | |
| 884 | 891 | } | |
| 885 | 892 | ||
| 893 | + | // ─── one commit ────────────────────────────────────────────────────────────── | |
| 894 | + | ||
| 895 | + | /// Everything the commit page shows about one revision. | |
| 896 | + | pub struct CommitPage<'a> { | |
| 897 | + | pub rev: &'a Revision, | |
| 898 | + | /// The account the author's email resolved to, when it matched one. | |
| 899 | + | pub author_handle: Option<&'a str>, | |
| 900 | + | /// The account the committer's email resolved to. Only rendered when the | |
| 901 | + | /// committer differs from the author — on a rebase or an amend they part | |
| 902 | + | /// company, and that is exactly when a reader wants to know. | |
| 903 | + | pub committer_handle: Option<&'a str>, | |
| 904 | + | /// The patch against the first parent. `None` when the diff exceeded the | |
| 905 | + | /// store's limits or could not be read. | |
| 906 | + | pub diff: Option<&'a df_store::Diff>, | |
| 907 | + | /// Fold every file, for skimming the shape of a large commit first. | |
| 908 | + | pub collapsed: bool, | |
| 909 | + | } | |
| 910 | + | ||
| 911 | + | /// One commit: what it says, who wrote it, where it sits in history, and what | |
| 912 | + | /// it changed. | |
| 913 | + | /// | |
| 914 | + | /// The diff is against the first parent, which is what makes a merge readable | |
| 915 | + | /// as "what this merge brought in" rather than as a second copy of both sides. | |
| 916 | + | pub fn commit_view(ctx: &RepoContext, c: CommitPage<'_>) -> Markup { | |
| 917 | + | use crate::views::diff as vd; | |
| 918 | + | ||
| 919 | + | let base = ctx.base(); | |
| 920 | + | let rev = c.rev.rev.as_str(); | |
| 921 | + | let blob_base = format!("{base}/blob/{rev}"); | |
| 922 | + | let now = chrono::Utc::now(); | |
| 923 | + | // The commit's own hash, not a bookmark name: a page about one commit must | |
| 924 | + | // keep pointing at that commit after the bookmark moves. | |
| 925 | + | let toggle = if c.collapsed { | |
| 926 | + | format!("{base}/commit/{rev}") | |
| 927 | + | } else { | |
| 928 | + | format!("{base}/commit/{rev}?collapse=1") | |
| 929 | + | }; | |
| 930 | + | ||
| 931 | + | // A different committer is worth a line; the usual case, where they are the | |
| 932 | + | // same person, is not. | |
| 933 | + | let amended = c.rev.committer.email != c.rev.author.email; | |
| 934 | + | ||
| 935 | + | html! { | |
| 936 | + | div .panel .commit-meta { | |
| 937 | + | div .commit-msg { | |
| 938 | + | h1 { (c.rev.summary()) } | |
| 939 | + | @if !c.rev.body().is_empty() { | |
| 940 | + | pre .commit-body { (c.rev.body()) } | |
| 941 | + | } | |
| 942 | + | } | |
| 943 | + | ||
| 944 | + | div .commit-byline { | |
| 945 | + | (avatar(c.author_handle.unwrap_or(&c.rev.author.name))) | |
| 946 | + | span { | |
| 947 | + | (crate::views::person(c.author_handle, Some(&c.rev.author.name))) | |
| 948 | + | " authored " | |
| 949 | + | span .faint title=(c.rev.author.when.format("%Y-%m-%d %H:%M UTC").to_string()) { | |
| 950 | + | (crate::views::relative_time(c.rev.author.when, now)) | |
| 951 | + | } | |
| 952 | + | } | |
| 953 | + | @if amended { | |
| 954 | + | span .faint { | |
| 955 | + | "· committed by " | |
| 956 | + | (crate::views::person(c.committer_handle, Some(&c.rev.committer.name))) | |
| 957 | + | " " | |
| 958 | + | span title=(c.rev.committer.when.format("%Y-%m-%d %H:%M UTC").to_string()) { | |
| 959 | + | (crate::views::relative_time(c.rev.committer.when, now)) | |
| 960 | + | } | |
| 961 | + | } | |
| 962 | + | } | |
| 963 | + | @if c.rev.conflicted { | |
| 964 | + | span .badge.badge-conflict { "conflict" } | |
| 965 | + | } | |
| 966 | + | } | |
| 967 | + | ||
| 968 | + | div .commit-ids { | |
| 969 | + | // The change id is the identity; the revision is a point in its | |
| 970 | + | // history (spec §4). Both are shown, and the identity links. | |
| 971 | + | @if let Some(id) = &c.rev.change_id { | |
| 972 | + | a .cid href=(format!("{base}/changes/{id}")) | |
| 973 | + | title=(format!("jj change id: {id}")) { (cid_parts(id)) } | |
| 974 | + | } @else { | |
| 975 | + | span .chip title="authored with plain git" { "git" } | |
| 976 | + | } | |
| 977 | + | span .mono.faint title=(rev) { (df_store::abbreviate_rev(rev)) } | |
| 978 | + | ||
| 979 | + | span .spacer {} | |
| 980 | + | ||
| 981 | + | @for p in &c.rev.parents { | |
| 982 | + | a .mono.commit-parent href=(format!("{base}/commit/{p}")) | |
| 983 | + | title=(format!("parent {p}")) { | |
| 984 | + | "parent " (df_store::abbreviate_rev(p.as_str())) | |
| 985 | + | } | |
| 986 | + | } | |
| 987 | + | a href=(format!("{base}/tree/{rev}/")) { "Browse files" } | |
| 988 | + | } | |
| 989 | + | } | |
| 990 | + | ||
| 991 | + | @match c.diff { | |
| 992 | + | None => div .panel { p .dim style="margin:0" { | |
| 993 | + | "This commit's diff could not be rendered. Fetch the revision with " | |
| 994 | + | code { "jj" } " to read it in full." | |
| 995 | + | } }, | |
| 996 | + | Some(d) => { | |
| 997 | + | div .diffbar { | |
| 998 | + | (vd::stat_summary(d)) | |
| 999 | + | span .spacer {} | |
| 1000 | + | @if !d.files.is_empty() { | |
| 1001 | + | a .diffbar-link href=(toggle) { | |
| 1002 | + | (if c.collapsed { "Expand all" } else { "Collapse all" }) | |
| 1003 | + | } | |
| 1004 | + | } | |
| 1005 | + | } | |
| 1006 | + | // The tree stands in for the flat file index here: it answers | |
| 1007 | + | // the same "which files" question and answers it better, and | |
| 1008 | + | // stacking both above the diff would make the reader scroll | |
| 1009 | + | // past the same thirty paths twice. | |
| 1010 | + | div .difflayout { | |
| 1011 | + | (vd::tree(d)) | |
| 1012 | + | div .diffmain { | |
| 1013 | + | (vd::files(&vd::DiffView { | |
| 1014 | + | collapsed: c.collapsed, | |
| 1015 | + | // A commit is a revision the browse routes can | |
| 1016 | + | // serve, so every file header reaches the whole | |
| 1017 | + | // file at this point in history — the step a hunk | |
| 1018 | + | // always raises. | |
| 1019 | + | blob_base: Some(&blob_base), | |
| 1020 | + | ..vd::DiffView::new(d) | |
| 1021 | + | })) | |
| 1022 | + | } | |
| 1023 | + | } | |
| 1024 | + | } | |
| 1025 | + | } | |
| 1026 | + | } | |
| 1027 | + | } | |
| 1028 | + | ||
| 886 | 1029 | /// Bookmark list. | |
| 887 | 1030 | /// The bookmarks page. | |
| 888 | 1031 | /// | |
| @@ −1083,4 +1226,147 @@ | |||
| 1083 | 1226 | assert_eq!(parent_link("/o/r", "main", "a/b/c"), "/o/r/tree/main/a/b"); | |
| 1084 | 1227 | assert_eq!(parent_link("/o/r", "main", "a"), "/o/r/tree/main/"); | |
| 1085 | 1228 | } | |
| 1229 | + | ||
| 1230 | + | // ─── the commit page ───────────────────────────────────────────────────── | |
| 1231 | + | ||
| 1232 | + | fn ctx() -> RepoContext { | |
| 1233 | + | RepoContext { | |
| 1234 | + | repo: df_db::models::Repo { | |
| 1235 | + | id: uuid::Uuid::nil(), | |
| 1236 | + | owner_kind: df_db::models::OwnerKind::User, | |
| 1237 | + | owner_user_id: None, | |
| 1238 | + | owner_org_id: None, | |
| 1239 | + | name: "r".into(), | |
| 1240 | + | description: None, | |
| 1241 | + | visibility: df_db::models::Visibility::Public, | |
| 1242 | + | default_bookmark: "main".into(), | |
| 1243 | + | fork_of_repo_id: None, | |
| 1244 | + | size_bytes: 0, | |
| 1245 | + | pushed_at: None, | |
| 1246 | + | archived: false, | |
| 1247 | + | created_at: chrono::Utc::now(), | |
| 1248 | + | }, | |
| 1249 | + | owner: "o".into(), | |
| 1250 | + | access: df_auth::RepoAccess::DENIED, | |
| 1251 | + | nav: crate::repo_ctx::RepoNav::default(), | |
| 1252 | + | } | |
| 1253 | + | } | |
| 1254 | + | ||
| 1255 | + | fn sig(name: &str) -> df_store::Signature { | |
| 1256 | + | df_store::Signature { | |
| 1257 | + | name: name.into(), | |
| 1258 | + | email: format!("{name}@example.test"), | |
| 1259 | + | when: chrono::Utc::now(), | |
| 1260 | + | } | |
| 1261 | + | } | |
| 1262 | + | ||
| 1263 | + | fn revision() -> Revision { | |
| 1264 | + | Revision { | |
| 1265 | + | rev: df_store::RevId::from_stored( | |
| 1266 | + | "0123456789abcdef0123456789abcdef01234567".to_string(), | |
| 1267 | + | ), | |
| 1268 | + | change_id: Some("kksontuqryot".into()), | |
| 1269 | + | parents: vec![df_store::RevId::from_stored( | |
| 1270 | + | "fedcba9876543210fedcba9876543210fedcba98".to_string(), | |
| 1271 | + | )], | |
| 1272 | + | author: sig("alice"), | |
| 1273 | + | committer: sig("alice"), | |
| 1274 | + | message: "fix the thing\n\nA longer explanation.\n".into(), | |
| 1275 | + | conflicted: false, | |
| 1276 | + | conflict_sides: Vec::new(), | |
| 1277 | + | conflict_bases: Vec::new(), | |
| 1278 | + | } | |
| 1279 | + | } | |
| 1280 | + | ||
| 1281 | + | fn page(rev: &Revision, diff: Option<&df_store::Diff>) -> String { | |
| 1282 | + | commit_view( | |
| 1283 | + | &ctx(), | |
| 1284 | + | CommitPage { | |
| 1285 | + | rev, | |
| 1286 | + | author_handle: Some("alice"), | |
| 1287 | + | committer_handle: Some("alice"), | |
| 1288 | + | diff, | |
| 1289 | + | collapsed: false, | |
| 1290 | + | }, | |
| 1291 | + | ) | |
| 1292 | + | .into_string() | |
| 1293 | + | } | |
| 1294 | + | ||
| 1295 | + | /// The three things a commit page is for: what it says, which commit it is, | |
| 1296 | + | /// and what it changed. | |
| 1297 | + | #[test] | |
| 1298 | + | fn a_commit_shows_its_message_its_ids_and_its_patch() { | |
| 1299 | + | let r = revision(); | |
| 1300 | + | let d = crate::views::diff::tests::fixture(6); | |
| 1301 | + | let html = page(&r, Some(&d)); | |
| 1302 | + | ||
| 1303 | + | assert!(html.contains("fix the thing")); | |
| 1304 | + | assert!(html.contains("A longer explanation.")); | |
| 1305 | + | // Abbreviation goes through the store (spec §3 rule 2) — never a slice. | |
| 1306 | + | assert!(html.contains("0123456789ab"), "{html:.800}"); | |
| 1307 | + | assert!(!html.contains("0123456789abcdef0123456789abcdef01234567<")); | |
| 1308 | + | // The change id is the identity, and it links to the change. | |
| 1309 | + | assert!(html.contains("href=\"/o/r/changes/kksontuqryot\"")); | |
| 1310 | + | assert!(html.contains("difftable")); | |
| 1311 | + | } | |
| 1312 | + | ||
| 1313 | + | /// The parent is the other half of "what changed": the diff is *against* it, | |
| 1314 | + | /// so walking back one commit has to be one click. | |
| 1315 | + | #[test] | |
| 1316 | + | fn parents_link_to_their_own_commit_pages() { | |
| 1317 | + | let r = revision(); | |
| 1318 | + | let html = page(&r, None); | |
| 1319 | + | assert!( | |
| 1320 | + | html.contains( | |
| 1321 | + | "href=\"/o/r/commit/fedcba9876543210fedcba9876543210fedcba98\"" | |
| 1322 | + | ), | |
| 1323 | + | "{html:.800}" | |
| 1324 | + | ); | |
| 1325 | + | } | |
| 1326 | + | ||
| 1327 | + | /// A commit whose patch could not be rendered still has a message, an | |
| 1328 | + | /// author and parents worth reading. | |
| 1329 | + | #[test] | |
| 1330 | + | fn a_commit_with_no_renderable_diff_still_renders() { | |
| 1331 | + | let html = page(&revision(), None); | |
| 1332 | + | assert!(html.contains("fix the thing")); | |
| 1333 | + | assert!(html.contains("could not be rendered")); | |
| 1334 | + | assert!(!html.contains("difftable")); | |
| 1335 | + | } | |
| 1336 | + | ||
| 1337 | + | /// The collapse link is a URL, so a folded commit can be pasted at | |
| 1338 | + | /// somebody — and it points back at the commit's own hash rather than at | |
| 1339 | + | /// whatever bookmark was typed to reach it. | |
| 1340 | + | #[test] | |
| 1341 | + | fn the_collapse_toggle_is_a_permalink() { | |
| 1342 | + | let r = revision(); | |
| 1343 | + | let d = crate::views::diff::tests::fixture(6); | |
| 1344 | + | let expanded = page(&r, Some(&d)); | |
| 1345 | + | assert!(expanded.contains(&format!("/o/r/commit/{}?collapse=1", r.rev))); | |
| 1346 | + | ||
| 1347 | + | let folded = commit_view( | |
| 1348 | + | &ctx(), | |
| 1349 | + | CommitPage { | |
| 1350 | + | rev: &r, | |
| 1351 | + | author_handle: None, | |
| 1352 | + | committer_handle: None, | |
| 1353 | + | diff: Some(&d), | |
| 1354 | + | collapsed: true, | |
| 1355 | + | }, | |
| 1356 | + | ) | |
| 1357 | + | .into_string(); | |
| 1358 | + | assert!(folded.contains("Expand all")); | |
| 1359 | + | assert!(!folded.contains("collapse=1")); | |
| 1360 | + | } | |
| 1361 | + | ||
| 1362 | + | /// An amend or a rebase parts the author from the committer, and that is | |
| 1363 | + | /// exactly when the second name is worth the line it costs. | |
| 1364 | + | #[test] | |
| 1365 | + | fn a_committer_is_only_named_when_they_differ_from_the_author() { | |
| 1366 | + | assert!(!page(&revision(), None).contains("committed by")); | |
| 1367 | + | ||
| 1368 | + | let mut r = revision(); | |
| 1369 | + | r.committer = sig("bob"); | |
| 1370 | + | assert!(page(&r, None).contains("committed by")); | |
| 1371 | + | } | |
| 1086 | 1372 | } | |
Mcrates/df-web/src/views/review.rs423 lines+206−146
| @@ −13,10 +13,13 @@ | |||
| 13 | 13 | use maud::{html, Markup, PreEscaped}; | |
| 14 | 14 | use uuid::Uuid; | |
| 15 | 15 | ||
| 16 | − | use df_store::{ConflictedFile, Diff, DiffLineKind, DiffSpan}; | |
| 16 | + | use df_store::{ConflictedFile, Diff, DiffLine, FileDiff}; | |
| 17 | 17 | ||
| 18 | 18 | use crate::repo_ctx::RepoContext; | |
| 19 | 19 | use crate::views::change::{change_chip, state_badge}; | |
| 20 | + | use crate::views::diff::{ | |
| 21 | + | self, line_class, marker, path_anchor, spans, stat_summary, DiffView, LineHooks, | |
| 22 | + | }; | |
| 20 | 23 | ||
| 21 | 24 | // ─── shared shape ──────────────────────────────────────────────────────────── | |
| 22 | 25 | ||
| @@ −690,42 +693,104 @@ | |||
| 690 | 693 | pub against: Option<&'a str>, | |
| 691 | 694 | /// Every revision of the change, for the compare selectors. | |
| 692 | 695 | pub revisions: &'a [(i32, String)], | |
| 696 | + | /// Fold every file, for skimming the shape of a large change first. | |
| 697 | + | pub collapsed: bool, | |
| 693 | 698 | } | |
| 694 | 699 | ||
| 695 | 700 | pub fn files(ctx: &RepoContext, c: &ChangeHead<'_>, f: FilesView<'_>) -> Markup { | |
| 696 | 701 | let base = format!("{}/changes/{}", ctx.base(), c.number); | |
| 697 | 702 | ||
| 698 | 703 | html! { | |
| 699 | − | div .panel { | |
| 704 | + | @match f.diff { | |
| 705 | + | None => div .panel { div .empty { h2 { "Nothing to show" } p { "This change has no revisions yet." } } }, | |
| 706 | + | Some(d) => { | |
| 707 | + | (diff_bar(d, &base, &f)) | |
| 708 | + | // The changed-file tree beside the diff, as on the commit page. | |
| 709 | + | // A reviewer's first question on a forty-file change is "what | |
| 710 | + | // did this touch", and the tree answers it in the shape the | |
| 711 | + | // code actually has rather than as forty near-identical paths. | |
| 712 | + | div .difflayout { | |
| 713 | + | (diff::tree(d)) | |
| 714 | + | div .diffmain { | |
| 715 | + | (diff_with_comments(d, f.comments, c, &base, f.rev, f.collapsed)) | |
| 716 | + | } | |
| 717 | + | } | |
| 718 | + | } | |
| 719 | + | } | |
| 720 | + | } | |
| 721 | + | } | |
| 722 | + | ||
| 723 | + | /// The bar that follows the reader down the diff: what is being compared, how | |
| 724 | + | /// big it is, and the two controls that change either. | |
| 725 | + | /// | |
| 726 | + | /// It stays on screen because the question it answers — *which* two revisions | |
| 727 | + | /// am I looking at — is the one a reviewer loses first when scrolling a long | |
| 728 | + | /// diff, and getting it wrong means reviewing the wrong code. | |
| 729 | + | fn diff_bar(d: &Diff, base: &str, f: &FilesView<'_>) -> Markup { | |
| 730 | + | let seq_of = |rev: &str| f.revisions.iter().find(|(_, r)| r == rev).map(|(s, _)| *s); | |
| 731 | + | let label = |rev: &str| match seq_of(rev) { | |
| 732 | + | Some(s) => format!("v{s}"), | |
| 733 | + | None => df_store::abbreviate_rev(rev).to_owned(), | |
| 734 | + | }; | |
| 735 | + | ||
| 736 | + | let comparing = match f.against { | |
| 737 | + | Some(a) => format!("{} → {}", label(a), label(f.rev)), | |
| 738 | + | None => format!("{} against its parent", label(f.rev)), | |
| 739 | + | }; | |
| 740 | + | ||
| 741 | + | // Collapse/expand is a link, not a script: it survives scripting being off | |
| 742 | + | // and it is a URL, so "here is the shape of it" can be pasted at somebody. | |
| 743 | + | let toggle = { | |
| 744 | + | let mut q = format!("?rev={}", f.rev); | |
| 745 | + | if let Some(a) = f.against { | |
| 746 | + | q.push_str(&format!("&against={a}")); | |
| 747 | + | } | |
| 748 | + | if !f.collapsed { | |
| 749 | + | q.push_str("&collapse=1"); | |
| 750 | + | } | |
| 751 | + | format!("{base}/files{q}") | |
| 752 | + | }; | |
| 753 | + | ||
| 754 | + | html! { | |
| 755 | + | div .diffbar { | |
| 756 | + | (stat_summary(d)) | |
| 757 | + | ||
| 758 | + | span .spacer {} | |
| 759 | + | ||
| 760 | + | @if !d.files.is_empty() { | |
| 761 | + | a .diffbar-link href=(toggle) { | |
| 762 | + | (if f.collapsed { "Expand all" } else { "Collapse all" }) | |
| 763 | + | } | |
| 764 | + | } | |
| 765 | + | ||
| 700 | 766 | // Revision-to-revision diffing (M4): compare any two revisions of | |
| 701 | 767 | // the change, which is what makes "what changed since I reviewed" | |
| 702 | − | // answerable at all. | |
| 703 | − | form method="get" action=(format!("{base}/files")) .row style="gap:10px;flex-wrap:wrap" { | |
| 704 | − | label for="against" .label-condensed { "Compare" } | |
| 705 | − | select id="against" name="against" { | |
| 706 | − | option value="" selected[f.against.is_none()] { "against the parent" } | |
| 707 | − | @for (seq, rev) in f.revisions { | |
| 708 | − | option value=(rev) selected[f.against == Some(rev.as_str())] { | |
| 709 | − | "v" (seq) " · " (df_store::abbreviate_rev(rev)) | |
| 768 | + | // answerable at all. Folded away by default — the answer matters on | |
| 769 | + | // every screen, the control only when you want a different one. | |
| 770 | + | details .cmpbox { | |
| 771 | + | summary { span .label-condensed { "Comparing" } (comparing) } | |
| 772 | + | form method="get" action=(format!("{base}/files")) .cmpform { | |
| 773 | + | label for="against" .label-condensed { "Compare" } | |
| 774 | + | select id="against" name="against" { | |
| 775 | + | option value="" selected[f.against.is_none()] { "against the parent" } | |
| 776 | + | @for (seq, rev) in f.revisions { | |
| 777 | + | option value=(rev) selected[f.against == Some(rev.as_str())] { | |
| 778 | + | "v" (seq) " · " (df_store::abbreviate_rev(rev)) | |
| 779 | + | } | |
| 710 | 780 | } | |
| 711 | 781 | } | |
| 712 | − | } | |
| 713 | − | label for="rev" .label-condensed { "of" } | |
| 714 | − | select id="rev" name="rev" { | |
| 715 | − | @for (seq, rev) in f.revisions { | |
| 716 | − | option value=(rev) selected[f.rev == rev] { | |
| 717 | − | "v" (seq) " · " (df_store::abbreviate_rev(rev)) | |
| 782 | + | label for="rev" .label-condensed { "of" } | |
| 783 | + | select id="rev" name="rev" { | |
| 784 | + | @for (seq, rev) in f.revisions { | |
| 785 | + | option value=(rev) selected[f.rev == rev] { | |
| 786 | + | "v" (seq) " · " (df_store::abbreviate_rev(rev)) | |
| 787 | + | } | |
| 718 | 788 | } | |
| 719 | 789 | } | |
| 790 | + | button .btn type="submit" { "Show" } | |
| 720 | 791 | } | |
| 721 | − | button .btn type="submit" { "Show" } | |
| 722 | 792 | } | |
| 723 | 793 | } | |
| 724 | − | ||
| 725 | − | @match f.diff { | |
| 726 | − | None => div .panel { div .empty { h2 { "Nothing to show" } p { "This change has no revisions yet." } } }, | |
| 727 | − | Some(d) => (diff_with_comments(d, f.comments, c, &base, f.rev)), | |
| 728 | − | } | |
| 729 | 794 | } | |
| 730 | 795 | } | |
| 731 | 796 | ||
| @@ −736,115 +801,79 @@ | |||
| 736 | 801 | c: &ChangeHead<'_>, | |
| 737 | 802 | base: &str, | |
| 738 | 803 | rev: &str, | |
| 804 | + | collapsed: bool, | |
| 739 | 805 | ) -> Markup { | |
| 740 | − | html! { | |
| 741 | − | div .panel { | |
| 742 | − | div .row style="margin-bottom:12px" { | |
| 743 | − | h2 style="margin:0" { "Files changed" } | |
| 744 | − | span .faint { (diff.files.len()) " file(s)" } | |
| 745 | − | span style="color:var(--diff-add-text)" { "+" (diff.total_additions) } | |
| 746 | − | span style="color:var(--diff-del-text)" { "−" (diff.total_deletions) } | |
| 747 | − | } | |
| 806 | + | // The comment affordance: a target link, so the form below opens with no | |
| 807 | + | // script and the open form has a URL of its own. | |
| 808 | + | let gutter = |file: &FileDiff, l: &DiffLine| -> Markup { | |
| 809 | + | let Some(n) = l.new_lineno else { return html! {} }; | |
| 810 | + | if !c.can_comment { | |
| 811 | + | return html! {}; | |
| 812 | + | } | |
| 813 | + | let id = comment_anchor(&file.path, n); | |
| 814 | + | html! { | |
| 815 | + | a .dl-add href=(format!("#{id}")) | |
| 816 | + | title=(format!("Comment on line {n}")) | |
| 817 | + | aria-label=(format!("Comment on line {n}")) | |
| 818 | + | { "+" } | |
| 819 | + | } | |
| 820 | + | }; | |
| 748 | 821 | ||
| 749 | − | @if diff.truncated { | |
| 750 | − | div .banner.banner-error role="alert" { | |
| 751 | − | "This diff exceeds the render limits and is shown in part. \ | |
| 752 | − | Fetch the revision with " code { "jj" } " to read it in full." | |
| 753 | − | } | |
| 754 | − | } | |
| 822 | + | // Threads anchored to this line, then the form to add one. Both are plain | |
| 823 | + | // HTML, and the form is laid out only when it is the fragment target — a | |
| 824 | + | // visible form per line is what made a large change unreadable. | |
| 825 | + | let under = |file: &FileDiff, l: &DiffLine| -> Markup { | |
| 826 | + | let Some(n) = l.new_lineno else { return html! {} }; | |
| 827 | + | let here: Vec<&CommentRow> = comments | |
| 828 | + | .iter() | |
| 829 | + | .filter(|cm| { | |
| 830 | + | cm.anchor_path.as_deref() == Some(file.path.as_str()) | |
| 831 | + | && cm.anchor_line == Some(n as i32) | |
| 832 | + | }) | |
| 833 | + | .collect(); | |
| 755 | 834 | ||
| 756 | − | @if diff.files.is_empty() { | |
| 757 | − | p .dim { "No changes." } | |
| 835 | + | html! { | |
| 836 | + | @if !here.is_empty() { | |
| 837 | + | tr { td colspan="4" .inline-thread { | |
| 838 | + | @for cm in here { (comment(cm, c, base, false)) } | |
| 839 | + | } } | |
| 758 | 840 | } | |
| 759 | − | ||
| 760 | − | @for file in &diff.files { | |
| 761 | − | div .filediff { | |
| 762 | − | div .row .filediff-head id=(format!("f-{}", path_anchor(&file.path))) { | |
| 763 | − | span .mono { (file.path) } | |
| 764 | − | @if let Some(old) = &file.old_path { span .faint { " ← " (old) } } | |
| 765 | − | span .faint style="margin-left:auto" { | |
| 766 | − | "+" (file.additions) " −" (file.deletions) | |
| 767 | − | } | |
| 768 | − | } | |
| 769 | − | ||
| 770 | − | @if file.binary { | |
| 771 | − | p .dim style="padding:10px" { "Binary file not shown." } | |
| 772 | − | } @else { | |
| 773 | − | table .mono .difftable { | |
| 774 | − | tbody { | |
| 775 | − | @for hunk in &file.hunks { | |
| 776 | − | tr .hunkhead { | |
| 777 | − | td colspan="3" { | |
| 778 | − | "@@ -" (hunk.old_start) "," (hunk.old_lines) | |
| 779 | − | " +" (hunk.new_start) "," (hunk.new_lines) " @@" | |
| 780 | − | } | |
| 781 | − | } | |
| 782 | − | @for l in &hunk.lines { | |
| 783 | − | tr .(line_class(l.kind)) { | |
| 784 | − | td .faint .lineno { @if let Some(n) = l.old_lineno { (n) } } | |
| 785 | − | td .faint .lineno { @if let Some(n) = l.new_lineno { (n) } } | |
| 786 | − | td .codeline { | |
| 787 | − | (marker(l.kind)) | |
| 788 | − | (spans(&l.spans, l.kind)) | |
| 789 | − | } | |
| 790 | − | } | |
| 791 | − | ||
| 792 | − | // Threads anchored to this line, then the | |
| 793 | − | // form to add one. Both are plain HTML. | |
| 794 | − | @if let Some(n) = l.new_lineno { | |
| 795 | − | @let here: Vec<&CommentRow> = comments | |
| 796 | − | .iter() | |
| 797 | − | .filter(|cm| { | |
| 798 | − | cm.anchor_path.as_deref() == Some(file.path.as_str()) | |
| 799 | − | && cm.anchor_line == Some(n as i32) | |
| 800 | − | }) | |
| 801 | − | .collect(); | |
| 802 | − | @if !here.is_empty() { | |
| 803 | − | tr { td colspan="3" .inline-thread { | |
| 804 | − | @for cm in here { (comment(cm, c, base, false)) } | |
| 805 | − | } } | |
| 806 | − | } | |
| 807 | − | @if c.can_comment { | |
| 808 | − | tr { td colspan="3" .inline-form { | |
| 809 | − | details { | |
| 810 | − | summary .faint { "Comment on line " (n) } | |
| 811 | − | form method="post" | |
| 812 | − | action=(format!("{base}/comments")) .stack { | |
| 813 | − | input type="hidden" name="_csrf" value=(c.csrf); | |
| 814 | − | input type="hidden" name="path" value=(file.path); | |
| 815 | − | input type="hidden" name="line" value=(n); | |
| 816 | − | input type="hidden" name="side" value="new"; | |
| 817 | − | input type="hidden" name="rev" value=(rev); | |
| 818 | − | input type="hidden" name="context" value=(l.content); | |
| 819 | − | textarea name="body" rows="3" required {} | |
| 820 | − | button .btn.btn-primary type="submit" { "Comment" } | |
| 821 | − | } | |
| 822 | − | } | |
| 823 | − | } } | |
| 824 | − | } | |
| 825 | − | } | |
| 826 | − | } | |
| 827 | − | } | |
| 841 | + | @if c.can_comment { | |
| 842 | + | tr .inline-form id=(comment_anchor(&file.path, n)) { | |
| 843 | + | td colspan="4" { | |
| 844 | + | form method="post" action=(format!("{base}/comments")) .stack { | |
| 845 | + | input type="hidden" name="_csrf" value=(c.csrf); | |
| 846 | + | input type="hidden" name="path" value=(file.path); | |
| 847 | + | input type="hidden" name="line" value=(n); | |
| 848 | + | input type="hidden" name="side" value="new"; | |
| 849 | + | input type="hidden" name="rev" value=(rev); | |
| 850 | + | input type="hidden" name="context" value=(l.content); | |
| 851 | + | div .label-condensed { "Comment on " (file.path) ":" (n) } | |
| 852 | + | textarea name="body" rows="3" required {} | |
| 853 | + | div .row { | |
| 854 | + | button .btn.btn-primary type="submit" { "Comment" } | |
| 855 | + | a .btn href="#" { "Cancel" } | |
| 828 | 856 | } | |
| 829 | 857 | } | |
| 830 | 858 | } | |
| 831 | 859 | } | |
| 832 | 860 | } | |
| 833 | 861 | } | |
| 834 | − | } | |
| 862 | + | }; | |
| 863 | + | ||
| 864 | + | diff::files(&DiffView { | |
| 865 | + | diff, | |
| 866 | + | collapsed, | |
| 867 | + | // A change's revision is not necessarily reachable through the browse | |
| 868 | + | // routes, so the file headers stay link-free here. | |
| 869 | + | blob_base: None, | |
| 870 | + | hooks: Some(LineHooks { gutter: &gutter, under: &under }), | |
| 871 | + | }) | |
| 835 | 872 | } | |
| 836 | 873 | ||
| 837 | − | /// Render a line's word-level spans (spec §8). | |
| 838 | − | fn spans(spans: &[DiffSpan], kind: DiffLineKind) -> Markup { | |
| 839 | − | html! { | |
| 840 | − | @for s in spans { | |
| 841 | − | @if s.emphasis && !matches!(kind, DiffLineKind::Context) { | |
| 842 | − | span .word-changed { (s.text) } | |
| 843 | − | } @else { | |
| 844 | − | (s.text) | |
| 845 | − | } | |
| 846 | − | } | |
| 847 | − | } | |
| 874 | + | /// The fragment a line's comment form answers to. | |
| 875 | + | fn comment_anchor(path: &str, line: u32) -> String { | |
| 876 | + | format!("c-{}-{line}", path_anchor(path)) | |
| 848 | 877 | } | |
| 849 | 878 | ||
| 850 | 879 | // ─── revisions ─────────────────────────────────────────────────────────────── | |
| @@ −1019,7 +1048,10 @@ | |||
| 1019 | 1048 | @if let Some(n) = l.new_lineno.or(l.old_lineno) { (n) } | |
| 1020 | 1049 | } | |
| 1021 | 1050 | span .diff-text { | |
| 1022 | − | (marker(l.kind)) (spans(&l.spans, l.kind)) | |
| 1051 | + | // Fixed-width, so a context line's | |
| 1052 | + | // absent sign still holds its column. | |
| 1053 | + | span .diff-sign { (marker(l.kind)) } | |
| 1054 | + | (spans(&l.spans, l.kind)) | |
| 1023 | 1055 | } | |
| 1024 | 1056 | } | |
| 1025 | 1057 | } | |
| @@ −1225,34 +1257,11 @@ | |||
| 1225 | 1257 | } | |
| 1226 | 1258 | ||
| 1227 | 1259 | // ─── helpers ───────────────────────────────────────────────────────────────── | |
| 1228 | − | ||
| 1229 | − | fn line_class(kind: DiffLineKind) -> &'static str { | |
| 1230 | − | match kind { | |
| 1231 | − | DiffLineKind::Added => "line-add", | |
| 1232 | − | DiffLineKind::Deleted => "line-del", | |
| 1233 | − | DiffLineKind::Context => "line-ctx", | |
| 1234 | − | } | |
| 1235 | − | } | |
| 1236 | − | ||
| 1237 | − | fn marker(kind: DiffLineKind) -> &'static str { | |
| 1238 | − | match kind { | |
| 1239 | − | DiffLineKind::Added => "+", | |
| 1240 | − | DiffLineKind::Deleted => "-", | |
| 1241 | − | DiffLineKind::Context => " ", | |
| 1242 | − | } | |
| 1243 | − | } | |
| 1244 | 1260 | ||
| 1245 | 1261 | fn first_line(s: &str) -> &str { | |
| 1246 | 1262 | s.lines().next().unwrap_or("").trim() | |
| 1247 | 1263 | } | |
| 1248 | 1264 | ||
| 1249 | − | /// A path turned into a fragment-safe anchor. | |
| 1250 | − | fn path_anchor(path: &str) -> String { | |
| 1251 | − | path.chars() | |
| 1252 | − | .map(|c| if c.is_ascii_alphanumeric() { c } else { '-' }) | |
| 1253 | − | .collect() | |
| 1254 | − | } | |
| 1255 | − | ||
| 1256 | 1265 | #[cfg(test)] | |
| 1257 | 1266 | mod tests { | |
| 1258 | 1267 | use super::*; | |
| @@ −1303,9 +1312,60 @@ | |||
| 1303 | 1312 | assert!(text.ends_with("0123456789ab"), "{text}"); | |
| 1304 | 1313 | } | |
| 1305 | 1314 | ||
| 1315 | + | use crate::views::diff::tests::fixture as diff_fixture; | |
| 1316 | + | ||
| 1317 | + | fn head() -> ChangeHead<'static> { | |
| 1318 | + | ChangeHead { | |
| 1319 | + | number: 3, | |
| 1320 | + | change_id: "kksontuqryot", | |
| 1321 | + | synthetic: false, | |
| 1322 | + | title: "t", | |
| 1323 | + | state: "open", | |
| 1324 | + | conflicted: false, | |
| 1325 | + | target_bookmark: "main", | |
| 1326 | + | author: None, | |
| 1327 | + | author_name: None, | |
| 1328 | + | revision_count: 1, | |
| 1329 | + | head_commit: None, | |
| 1330 | + | created_at: Utc::now(), | |
| 1331 | + | updated_at: Utc::now(), | |
| 1332 | + | file_count: Some(1), | |
| 1333 | + | comment_count: 0, | |
| 1334 | + | can_manage: false, | |
| 1335 | + | can_comment: true, | |
| 1336 | + | csrf: "tok", | |
| 1337 | + | } | |
| 1338 | + | } | |
| 1339 | + | ||
| 1340 | + | /// The resting diff must be code and nothing else. Every commentable line | |
| 1341 | + | /// carries a form, but a form that is laid out costs a row per line and is | |
| 1342 | + | /// what made a large change unreadable — so the markup is there and the | |
| 1343 | + | /// `:target` rule is what reveals exactly one. | |
| 1344 | + | #[test] | |
| 1345 | + | fn a_comment_form_exists_per_line_but_none_is_open_by_default() { | |
| 1346 | + | let d = diff_fixture(30); | |
| 1347 | + | let html = diff_with_comments(&d, &[], &head(), "/o/r/changes/3", "abc", false).into_string(); | |
| 1348 | + | ||
| 1349 | + | assert_eq!(html.matches("class=\"inline-form\"").count(), 30); | |
| 1350 | + | // Nothing renders the old always-visible summary chrome any more. | |
| 1351 | + | assert!(!html.contains("Comment on line 1<"), "per-line chrome is back"); | |
| 1352 | + | // The affordance and the form agree on the fragment. | |
| 1353 | + | let anchor = path_anchor("crates/df-web/src/views/review.rs"); | |
| 1354 | + | assert!(html.contains(&format!("href=\"#c-{anchor}-7\""))); | |
| 1355 | + | assert!(html.contains(&format!("id=\"c-{anchor}-7\""))); | |
| 1356 | + | } | |
| 1357 | + | ||
| 1358 | + | /// A reader with no comment rights gets the diff and nothing else — no | |
| 1359 | + | /// dead affordance, and none of the per-line form markup either. | |
| 1306 | 1360 | #[test] | |
| 1307 | − | fn path_anchors_cannot_break_out_of_a_fragment() { | |
| 1308 | − | assert_eq!(path_anchor("src/a b.rs"), "src-a-b-rs"); | |
| 1309 | − | assert_eq!(path_anchor("a\"><script>"), "a---script-"); | |
| 1361 | + | fn a_reader_who_cannot_comment_gets_no_forms() { | |
| 1362 | + | let d = diff_fixture(9); | |
| 1363 | + | let head = ChangeHead { can_comment: false, ..head() }; | |
| 1364 | + | let html = diff_with_comments(&d, &[], &head, "/b", "abc", false).into_string(); | |
| 1365 | + | ||
| 1366 | + | assert!(!html.contains("inline-form")); | |
| 1367 | + | assert!(!html.contains("dl-add")); | |
| 1368 | + | // The diff itself is still there. | |
| 1369 | + | assert!(html.contains("difftable")); | |
| 1310 | 1370 | } | |
| 1311 | 1371 | } | |
Acrates/df-web/src/views/diff.rs519 lines+519−0
| @@ −0,0 +1,519 @@ | |||
| 1 | + | //! One diff renderer, shared by every page that shows a patch. | |
| 2 | + | //! | |
| 3 | + | //! A change's files tab and a commit's page are the same reading task — which | |
| 4 | + | //! files, how big, and what changed in each — so they are the same markup and | |
| 5 | + | //! the same CSS. What differs is what hangs *off* the diff: the change view | |
| 6 | + | //! anchors comment threads to lines, the commit view links each file to its | |
| 7 | + | //! blob. Both arrive through [`DiffView`] rather than by growing a second | |
| 8 | + | //! renderer that drifts from this one. | |
| 9 | + | ||
| 10 | + | use std::collections::BTreeMap; | |
| 11 | + | ||
| 12 | + | use maud::{html, Markup}; | |
| 13 | + | ||
| 14 | + | use df_store::{ChangeKind, Diff, DiffLine, DiffLineKind, DiffSpan, FileDiff}; | |
| 15 | + | ||
| 16 | + | /// A file this long is folded by default. Past a few hundred lines a file is | |
| 17 | + | /// no longer something a reviewer reads on the way past — it is a destination — | |
| 18 | + | /// and leaving it open buries every file after it. | |
| 19 | + | pub const LARGE_FILE_LINES: usize = 400; | |
| 20 | + | ||
| 21 | + | /// Per-line markup a caller wants woven into the table. | |
| 22 | + | /// | |
| 23 | + | /// Both hooks are called for every rendered line, so an implementation that | |
| 24 | + | /// only cares about one side must check the line itself; a hook that returns | |
| 25 | + | /// empty markup costs nothing. | |
| 26 | + | pub struct LineHooks<'a> { | |
| 27 | + | /// Extra markup inside the new-side line-number cell — the comment | |
| 28 | + | /// affordance, in practice. | |
| 29 | + | pub gutter: &'a dyn Fn(&FileDiff, &DiffLine) -> Markup, | |
| 30 | + | /// Full-width `<tr>`s placed directly beneath the line. Anything returned | |
| 31 | + | /// here must be table rows, since that is where it lands. | |
| 32 | + | pub under: &'a dyn Fn(&FileDiff, &DiffLine) -> Markup, | |
| 33 | + | } | |
| 34 | + | ||
| 35 | + | /// A diff and the decisions a page makes about how to show it. | |
| 36 | + | pub struct DiffView<'a> { | |
| 37 | + | pub diff: &'a Diff, | |
| 38 | + | /// Fold every file, for skimming the shape of a large change first. | |
| 39 | + | pub collapsed: bool, | |
| 40 | + | /// `…/blob/{rev}` — when set, every file header links to the file as it | |
| 41 | + | /// stands at this revision. A change's files tab leaves it `None`: the | |
| 42 | + | /// revision under review is not necessarily one the browser can serve. | |
| 43 | + | pub blob_base: Option<&'a str>, | |
| 44 | + | pub hooks: Option<LineHooks<'a>>, | |
| 45 | + | } | |
| 46 | + | ||
| 47 | + | impl<'a> DiffView<'a> { | |
| 48 | + | /// A diff with no annotations — the commit page's default. | |
| 49 | + | pub fn new(diff: &'a Diff) -> Self { | |
| 50 | + | DiffView { diff, collapsed: false, blob_base: None, hooks: None } | |
| 51 | + | } | |
| 52 | + | } | |
| 53 | + | ||
| 54 | + | /// How much changed, as one line: the count, the two totals, and the ratio bar. | |
| 55 | + | pub fn stat_summary(d: &Diff) -> Markup { | |
| 56 | + | html! { | |
| 57 | + | div .diffbar-stat { | |
| 58 | + | strong { (d.files.len()) } | |
| 59 | + | " " (if d.files.len() == 1 { "file" } else { "files" }) | |
| 60 | + | span .cl-add { "+" (d.total_additions) } | |
| 61 | + | span .cl-del { "−" (d.total_deletions) } | |
| 62 | + | (stat_bar(d.total_additions, d.total_deletions)) | |
| 63 | + | } | |
| 64 | + | } | |
| 65 | + | } | |
| 66 | + | ||
| 67 | + | // ─── the changed-file tree ─────────────────────────────────────────────────── | |
| 68 | + | ||
| 69 | + | /// The changed files as the directory tree they actually live in. | |
| 70 | + | /// | |
| 71 | + | /// A flat list of thirty paths in a Rust workspace is thirty rows that all | |
| 72 | + | /// begin `crates/df-…/src/`, and the eye has to read to the end of each one to | |
| 73 | + | /// tell them apart. The tree factors that prefix out once, so what is left on | |
| 74 | + | /// each row is the part that differs — and the shape of the commit ("this | |
| 75 | + | /// touched the store and the web crate") becomes visible without reading at | |
| 76 | + | /// all. | |
| 77 | + | /// | |
| 78 | + | /// Directories are `<details>`, so folding one needs no script. | |
| 79 | + | pub fn tree(d: &Diff) -> Markup { | |
| 80 | + | if d.files.is_empty() { | |
| 81 | + | return html! {}; | |
| 82 | + | } | |
| 83 | + | ||
| 84 | + | let root = TreeNode::build(&d.files); | |
| 85 | + | ||
| 86 | + | html! { | |
| 87 | + | aside .difftree aria-label="Changed files" { | |
| 88 | + | div .dt-head { | |
| 89 | + | (d.files.len()) " " (if d.files.len() == 1 { "file" } else { "files" }) " changed" | |
| 90 | + | } | |
| 91 | + | nav .dt-body { (tree_level(&root)) } | |
| 92 | + | } | |
| 93 | + | } | |
| 94 | + | } | |
| 95 | + | ||
| 96 | + | /// One level of the changed-file tree. | |
| 97 | + | #[derive(Default)] | |
| 98 | + | struct TreeNode<'a> { | |
| 99 | + | /// Ordered by name, which is how every file browser lists a directory and | |
| 100 | + | /// therefore where a reader already expects to find things. | |
| 101 | + | dirs: BTreeMap<&'a str, TreeNode<'a>>, | |
| 102 | + | files: Vec<&'a FileDiff>, | |
| 103 | + | } | |
| 104 | + | ||
| 105 | + | impl<'a> TreeNode<'a> { | |
| 106 | + | fn build(files: &'a [FileDiff]) -> TreeNode<'a> { | |
| 107 | + | let mut root = TreeNode::default(); | |
| 108 | + | ||
| 109 | + | for f in files { | |
| 110 | + | let mut node = &mut root; | |
| 111 | + | let mut segments = f.path.split('/').filter(|s| !s.is_empty()).peekable(); | |
| 112 | + | ||
| 113 | + | while let Some(seg) = segments.next() { | |
| 114 | + | if segments.peek().is_none() { | |
| 115 | + | node.files.push(f); | |
| 116 | + | } else { | |
| 117 | + | node = node.dirs.entry(seg).or_default(); | |
| 118 | + | } | |
| 119 | + | } | |
| 120 | + | } | |
| 121 | + | ||
| 122 | + | root | |
| 123 | + | } | |
| 124 | + | ||
| 125 | + | /// Fold a chain of single-child directories into one row. | |
| 126 | + | /// | |
| 127 | + | /// `crates/df-web/src/views/` is one place, not four, and indenting it four | |
| 128 | + | /// times spends the whole width of the sidebar saying nothing. Returns the | |
| 129 | + | /// joined label and the first node that actually branches. | |
| 130 | + | fn collapse(&'a self, name: &'a str) -> (String, &'a TreeNode<'a>) { | |
| 131 | + | let mut label = name.to_string(); | |
| 132 | + | let mut node = self; | |
| 133 | + | ||
| 134 | + | while node.files.is_empty() && node.dirs.len() == 1 { | |
| 135 | + | let (child_name, child) = node.dirs.iter().next().expect("len == 1"); | |
| 136 | + | label.push('/'); | |
| 137 | + | label.push_str(child_name); | |
| 138 | + | node = child; | |
| 139 | + | } | |
| 140 | + | ||
| 141 | + | (label, node) | |
| 142 | + | } | |
| 143 | + | } | |
| 144 | + | ||
| 145 | + | /// Directories first, then files — both alphabetical. | |
| 146 | + | fn tree_level(node: &TreeNode<'_>) -> Markup { | |
| 147 | + | html! { | |
| 148 | + | @for (name, child) in &node.dirs { | |
| 149 | + | @let (label, child) = child.collapse(name); | |
| 150 | + | details .dt-dir open { | |
| 151 | + | summary .dt-row { | |
| 152 | + | span .dt-caret aria-hidden="true" {} | |
| 153 | + | span .dt-dirname { (label) } | |
| 154 | + | } | |
| 155 | + | div .dt-kids { (tree_level(child)) } | |
| 156 | + | } | |
| 157 | + | } | |
| 158 | + | @for f in &node.files { (tree_file(f)) } | |
| 159 | + | } | |
| 160 | + | } | |
| 161 | + | ||
| 162 | + | fn tree_file(f: &FileDiff) -> Markup { | |
| 163 | + | let (_, name) = split_path(&f.path); | |
| 164 | + | ||
| 165 | + | html! { | |
| 166 | + | // The full path in `title`, because the visible name is truncated and | |
| 167 | + | // two files can share a basename. | |
| 168 | + | a .dt-file href=(format!("#f-{}", path_anchor(&f.path))) title=(f.path) { | |
| 169 | + | (kind_glyph(f.kind)) | |
| 170 | + | span .dt-name { (name) } | |
| 171 | + | span .spacer {} | |
| 172 | + | @if f.binary { | |
| 173 | + | span .fx-bin { "bin" } | |
| 174 | + | } @else { | |
| 175 | + | span .cl-add { "+" (f.additions) } | |
| 176 | + | span .cl-del { "−" (f.deletions) } | |
| 177 | + | } | |
| 178 | + | } | |
| 179 | + | } | |
| 180 | + | } | |
| 181 | + | ||
| 182 | + | /// The diff itself: one foldable block per file, each a table of numbered | |
| 183 | + | /// lines. | |
| 184 | + | pub fn files(v: &DiffView<'_>) -> Markup { | |
| 185 | + | let d = v.diff; | |
| 186 | + | ||
| 187 | + | html! { | |
| 188 | + | @if d.truncated { | |
| 189 | + | div .banner.banner-error role="alert" { | |
| 190 | + | "This diff exceeds the render limits and is shown in part. \ | |
| 191 | + | Fetch the revision with " code { "jj" } " to read it in full." | |
| 192 | + | } | |
| 193 | + | } | |
| 194 | + | ||
| 195 | + | @if d.files.is_empty() { | |
| 196 | + | div .panel { p .dim style="margin:0" { "No changes." } } | |
| 197 | + | } | |
| 198 | + | ||
| 199 | + | @for file in &d.files { | |
| 200 | + | @let anchor = path_anchor(&file.path); | |
| 201 | + | @let len: usize = file.hunks.iter().map(|h| h.lines.len()).sum(); | |
| 202 | + | @let big = len > LARGE_FILE_LINES; | |
| 203 | + | ||
| 204 | + | details .filediff.is-big[big] id=(format!("f-{}", anchor)) | |
| 205 | + | open[!v.collapsed && !big && !file.binary] { | |
| 206 | + | summary .filediff-head { | |
| 207 | + | span .fd-caret aria-hidden="true" {} | |
| 208 | + | (kind_glyph(file.kind)) | |
| 209 | + | span .fd-path.mono { | |
| 210 | + | @let (dir, name) = split_path(&file.path); | |
| 211 | + | @if !dir.is_empty() { span .fd-dir { (dir) } } | |
| 212 | + | span .fd-name { (name) } | |
| 213 | + | } | |
| 214 | + | @if let Some(old) = &file.old_path { | |
| 215 | + | span .fd-old .mono { "← " (old) } | |
| 216 | + | } | |
| 217 | + | span .spacer {} | |
| 218 | + | // A deleted file has no blob at this revision to link to. | |
| 219 | + | @if let (Some(b), false) = (v.blob_base, file.kind == ChangeKind::Deleted) { | |
| 220 | + | a .fd-view href=(format!("{b}/{}", file.path)) { "View file" } | |
| 221 | + | } | |
| 222 | + | @if big { span .fd-note { (len) " lines" } } | |
| 223 | + | @if file.binary { | |
| 224 | + | span .fx-bin { "binary" } | |
| 225 | + | } @else { | |
| 226 | + | span .cl-add { "+" (file.additions) } | |
| 227 | + | span .cl-del { "−" (file.deletions) } | |
| 228 | + | (stat_bar(file.additions, file.deletions)) | |
| 229 | + | } | |
| 230 | + | } | |
| 231 | + | ||
| 232 | + | @if file.binary { | |
| 233 | + | p .dim style="padding:10px 12px;margin:0" { "Binary file not shown." } | |
| 234 | + | } @else { | |
| 235 | + | table .mono .difftable { | |
| 236 | + | tbody { | |
| 237 | + | @for hunk in &file.hunks { | |
| 238 | + | tr .hunkhead { | |
| 239 | + | td colspan="4" { | |
| 240 | + | span .hh-at { "@@ −" (hunk.old_start) "," (hunk.old_lines) | |
| 241 | + | " +" (hunk.new_start) "," (hunk.new_lines) " @@" } | |
| 242 | + | } | |
| 243 | + | } | |
| 244 | + | @for l in &hunk.lines { | |
| 245 | + | tr .dl.(line_class(l.kind)) { | |
| 246 | + | td .lineno { @if let Some(n) = l.old_lineno { (n) } } | |
| 247 | + | td .lineno { | |
| 248 | + | @if let Some(n) = l.new_lineno { (n) } | |
| 249 | + | @if let Some(h) = &v.hooks { ((h.gutter)(file, l)) } | |
| 250 | + | } | |
| 251 | + | td .dl-mark { (marker(l.kind)) } | |
| 252 | + | td .codeline { (spans(&l.spans, l.kind)) } | |
| 253 | + | } | |
| 254 | + | @if let Some(h) = &v.hooks { ((h.under)(file, l)) } | |
| 255 | + | } | |
| 256 | + | } | |
| 257 | + | } | |
| 258 | + | } | |
| 259 | + | } | |
| 260 | + | } | |
| 261 | + | } | |
| 262 | + | } | |
| 263 | + | } | |
| 264 | + | ||
| 265 | + | /// The add/delete ratio as a bar. Two numbers say how much; the bar says which | |
| 266 | + | /// way, which is the thing that reads at a glance down a list of forty files. | |
| 267 | + | pub fn stat_bar(add: usize, del: usize) -> Markup { | |
| 268 | + | let total = add + del; | |
| 269 | + | let pct = (add * 100).checked_div(total).unwrap_or(0); | |
| 270 | + | ||
| 271 | + | html! { | |
| 272 | + | span .statbar aria-hidden="true" { | |
| 273 | + | @if total > 0 { | |
| 274 | + | span .statbar-add style=(format!("width:{pct}%")) {} | |
| 275 | + | span .statbar-del style=(format!("width:{}%", 100 - pct)) {} | |
| 276 | + | } | |
| 277 | + | } | |
| 278 | + | } | |
| 279 | + | } | |
| 280 | + | ||
| 281 | + | /// A/M/D/R — the one-letter status every reviewer already reads without | |
| 282 | + | /// thinking, from `git status` onwards. | |
| 283 | + | pub fn kind_glyph(k: ChangeKind) -> Markup { | |
| 284 | + | let (letter, class, title) = match k { | |
| 285 | + | ChangeKind::Added => ("A", "is-add", "added"), | |
| 286 | + | ChangeKind::Modified => ("M", "is-mod", "modified"), | |
| 287 | + | ChangeKind::Deleted => ("D", "is-del", "deleted"), | |
| 288 | + | ChangeKind::Renamed => ("R", "is-ren", "renamed"), | |
| 289 | + | }; | |
| 290 | + | html! { span .fkind.(class) title=(title) { (letter) } } | |
| 291 | + | } | |
| 292 | + | ||
| 293 | + | /// `("crates/df-web/src/", "views.rs")` — the directory dims, the file does not. | |
| 294 | + | pub fn split_path(path: &str) -> (&str, &str) { | |
| 295 | + | match path.rfind('/') { | |
| 296 | + | Some(i) => (&path[..=i], &path[i + 1..]), | |
| 297 | + | None => ("", path), | |
| 298 | + | } | |
| 299 | + | } | |
| 300 | + | ||
| 301 | + | /// A path turned into a fragment-safe anchor. | |
| 302 | + | pub fn path_anchor(path: &str) -> String { | |
| 303 | + | path.chars() | |
| 304 | + | .map(|c| if c.is_ascii_alphanumeric() { c } else { '-' }) | |
| 305 | + | .collect() | |
| 306 | + | } | |
| 307 | + | ||
| 308 | + | /// Render a line's word-level spans (spec §8). | |
| 309 | + | pub(crate) fn spans(spans: &[DiffSpan], kind: DiffLineKind) -> Markup { | |
| 310 | + | html! { | |
| 311 | + | @for s in spans { | |
| 312 | + | @if s.emphasis && !matches!(kind, DiffLineKind::Context) { | |
| 313 | + | span .word-changed { (s.text) } | |
| 314 | + | } @else { | |
| 315 | + | (s.text) | |
| 316 | + | } | |
| 317 | + | } | |
| 318 | + | } | |
| 319 | + | } | |
| 320 | + | ||
| 321 | + | pub(crate) fn line_class(kind: DiffLineKind) -> &'static str { | |
| 322 | + | match kind { | |
| 323 | + | DiffLineKind::Added => "line-add", | |
| 324 | + | DiffLineKind::Deleted => "line-del", | |
| 325 | + | DiffLineKind::Context => "line-ctx", | |
| 326 | + | } | |
| 327 | + | } | |
| 328 | + | ||
| 329 | + | /// The sign in its own column, so the code starts at the same x on every line. | |
| 330 | + | /// Inlining it into the text — as this used to — shifts context lines one | |
| 331 | + | /// character against the lines around them, and that misalignment is most of | |
| 332 | + | /// why an unstyled diff is hard to read. | |
| 333 | + | pub(crate) fn marker(kind: DiffLineKind) -> &'static str { | |
| 334 | + | match kind { | |
| 335 | + | DiffLineKind::Added => "+", | |
| 336 | + | DiffLineKind::Deleted => "−", | |
| 337 | + | DiffLineKind::Context => "", | |
| 338 | + | } | |
| 339 | + | } | |
| 340 | + | ||
| 341 | + | #[cfg(test)] | |
| 342 | + | pub(crate) mod tests { | |
| 343 | + | use super::*; | |
| 344 | + | ||
| 345 | + | #[test] | |
| 346 | + | fn path_anchors_cannot_break_out_of_a_fragment() { | |
| 347 | + | assert_eq!(path_anchor("src/a b.rs"), "src-a-b-rs"); | |
| 348 | + | assert_eq!(path_anchor("a\"><script>"), "a---script-"); | |
| 349 | + | } | |
| 350 | + | ||
| 351 | + | #[test] | |
| 352 | + | fn a_path_splits_into_a_dimmable_directory_and_a_basename() { | |
| 353 | + | assert_eq!(split_path("crates/df-web/views.rs"), ("crates/df-web/", "views.rs")); | |
| 354 | + | assert_eq!(split_path("Cargo.toml"), ("", "Cargo.toml")); | |
| 355 | + | assert_eq!(split_path("a/"), ("a/", "")); | |
| 356 | + | } | |
| 357 | + | ||
| 358 | + | /// Shared by the tests here and in `review`, so both views are exercised | |
| 359 | + | /// against the same shape. | |
| 360 | + | pub(crate) fn fixture(lines: usize) -> Diff { | |
| 361 | + | let hunk = df_store::Hunk { | |
| 362 | + | old_start: 1, | |
| 363 | + | old_lines: lines as u32, | |
| 364 | + | new_start: 1, | |
| 365 | + | new_lines: lines as u32, | |
| 366 | + | lines: (0..lines) | |
| 367 | + | .map(|i| DiffLine { | |
| 368 | + | kind: if i % 3 == 0 { DiffLineKind::Added } else { DiffLineKind::Context }, | |
| 369 | + | old_lineno: Some(i as u32 + 1), | |
| 370 | + | new_lineno: Some(i as u32 + 1), | |
| 371 | + | content: format!("line {i}"), | |
| 372 | + | spans: vec![DiffSpan { text: format!("line {i}"), emphasis: false }], | |
| 373 | + | }) | |
| 374 | + | .collect(), | |
| 375 | + | }; | |
| 376 | + | Diff { | |
| 377 | + | files: vec![FileDiff { | |
| 378 | + | path: "crates/df-web/src/views/review.rs".into(), | |
| 379 | + | old_path: None, | |
| 380 | + | kind: ChangeKind::Modified, | |
| 381 | + | binary: false, | |
| 382 | + | additions: lines / 3, | |
| 383 | + | deletions: 0, | |
| 384 | + | hunks: vec![hunk], | |
| 385 | + | }], | |
| 386 | + | truncated: false, | |
| 387 | + | total_additions: lines / 3, | |
| 388 | + | total_deletions: 0, | |
| 389 | + | } | |
| 390 | + | } | |
| 391 | + | ||
| 392 | + | /// A diff of the given paths, with no content — enough for anything that | |
| 393 | + | /// only cares about the set of files. | |
| 394 | + | fn paths(paths: &[&str]) -> Diff { | |
| 395 | + | Diff { | |
| 396 | + | files: paths | |
| 397 | + | .iter() | |
| 398 | + | .map(|p| FileDiff { | |
| 399 | + | path: (*p).into(), | |
| 400 | + | old_path: None, | |
| 401 | + | kind: ChangeKind::Modified, | |
| 402 | + | binary: false, | |
| 403 | + | additions: 1, | |
| 404 | + | deletions: 0, | |
| 405 | + | hunks: Vec::new(), | |
| 406 | + | }) | |
| 407 | + | .collect(), | |
| 408 | + | truncated: false, | |
| 409 | + | total_additions: paths.len(), | |
| 410 | + | total_deletions: 0, | |
| 411 | + | } | |
| 412 | + | } | |
| 413 | + | ||
| 414 | + | /// The tree exists to factor the shared prefix out of a column of paths, | |
| 415 | + | /// and every leaf still has to reach its diff. | |
| 416 | + | #[test] | |
| 417 | + | fn the_tree_nests_directories_and_links_each_file_to_its_anchor() { | |
| 418 | + | let d = paths(&["crates/df-web/src/main.rs", "crates/df-store/src/lib.rs"]); | |
| 419 | + | let html = tree(&d).into_string(); | |
| 420 | + | ||
| 421 | + | // The shared prefix is one row, not two. | |
| 422 | + | assert_eq!(html.matches(">crates<").count(), 1, "{html}"); | |
| 423 | + | // Both crates branch under it, in name order — and each one's own | |
| 424 | + | // single-child chain has already folded into its row. | |
| 425 | + | let store = html.find(">df-store/src<").expect("df-store/src"); | |
| 426 | + | let web = html.find(">df-web/src<").expect("df-web/src"); | |
| 427 | + | assert!(store < web, "directories are not alphabetical"); | |
| 428 | + | // Only the basename shows; the anchor carries the whole path. | |
| 429 | + | assert!(html.contains(&format!( | |
| 430 | + | "href=\"#f-{}\"", | |
| 431 | + | path_anchor("crates/df-web/src/main.rs") | |
| 432 | + | ))); | |
| 433 | + | assert!(html.contains(">main.rs<")); | |
| 434 | + | // The full path stays reachable, since the visible name is truncated. | |
| 435 | + | assert!(html.contains("title=\"crates/df-web/src/main.rs\"")); | |
| 436 | + | } | |
| 437 | + | ||
| 438 | + | /// `crates/df-web/src/views/` is one place, not four. Indenting it four | |
| 439 | + | /// times spends the whole sidebar saying nothing. | |
| 440 | + | #[test] | |
| 441 | + | fn a_chain_of_single_child_directories_collapses_to_one_row() { | |
| 442 | + | let d = paths(&["crates/df-web/src/views/repo.rs"]); | |
| 443 | + | let html = tree(&d).into_string(); | |
| 444 | + | ||
| 445 | + | assert!(html.contains(">crates/df-web/src/views<"), "{html}"); | |
| 446 | + | assert_eq!(html.matches("<details").count(), 1, "still nested: {html}"); | |
| 447 | + | ||
| 448 | + | // A directory that branches must *not* be folded into its parent. | |
| 449 | + | let d = paths(&["a/b/one.rs", "a/c/two.rs"]); | |
| 450 | + | let html = tree(&d).into_string(); | |
| 451 | + | assert!(html.contains(">a<"), "{html}"); | |
| 452 | + | assert!(!html.contains(">a/b<")); | |
| 453 | + | } | |
| 454 | + | ||
| 455 | + | /// Directories first, then files — the order every file browser uses, and | |
| 456 | + | /// therefore where a reader already looks. | |
| 457 | + | #[test] | |
| 458 | + | fn root_files_sit_at_the_root_below_the_directories() { | |
| 459 | + | let d = paths(&["Cargo.toml", "crates/df-web/src/main.rs"]); | |
| 460 | + | let html = tree(&d).into_string(); | |
| 461 | + | ||
| 462 | + | let dir = html.find(">crates/df-web/src<").expect("crates/df-web/src"); | |
| 463 | + | let file = html.find(">Cargo.toml<").expect("Cargo.toml"); | |
| 464 | + | assert!(dir < file, "a root file came before the directories"); | |
| 465 | + | } | |
| 466 | + | ||
| 467 | + | #[test] | |
| 468 | + | fn an_empty_diff_gets_no_tree() { | |
| 469 | + | assert!(tree(&Diff::default()).into_string().is_empty()); | |
| 470 | + | } | |
| 471 | + | ||
| 472 | + | /// A file long enough to bury everything after it starts folded, and the | |
| 473 | + | /// `collapsed` flag folds the rest. | |
| 474 | + | #[test] | |
| 475 | + | fn large_files_and_the_collapse_flag_fold_the_diff() { | |
| 476 | + | let big = fixture(LARGE_FILE_LINES + 1); | |
| 477 | + | let html = files(&DiffView::new(&big)).into_string(); | |
| 478 | + | assert!(html.contains("class=\"filediff is-big\""), "{html:.400}"); | |
| 479 | + | assert!(!html.contains("class=\"filediff is-big\" id=\"f-crates-df-web-src-views-review-rs\" open")); | |
| 480 | + | ||
| 481 | + | let small = fixture(10); | |
| 482 | + | assert!(files(&DiffView::new(&small)).into_string().contains(" open>")); | |
| 483 | + | let folded = files(&DiffView { collapsed: true, ..DiffView::new(&small) }).into_string(); | |
| 484 | + | assert!(!folded.contains(" open>"), "collapsed folds every file"); | |
| 485 | + | } | |
| 486 | + | ||
| 487 | + | /// The commit view's file header reaches the blob at that exact revision — | |
| 488 | + | /// the "and now show me the whole file" step, one click from any hunk. | |
| 489 | + | #[test] | |
| 490 | + | fn a_blob_base_links_each_file_to_its_content_at_the_revision() { | |
| 491 | + | let d = fixture(4); | |
| 492 | + | let html = | |
| 493 | + | files(&DiffView { blob_base: Some("/o/r/blob/abc"), ..DiffView::new(&d) }).into_string(); | |
| 494 | + | assert!( | |
| 495 | + | html.contains("href=\"/o/r/blob/abc/crates/df-web/src/views/review.rs\""), | |
| 496 | + | "{html:.600}" | |
| 497 | + | ); | |
| 498 | + | // Without one, no link — a change's revision may not be browsable. | |
| 499 | + | assert!(!files(&DiffView::new(&d)).into_string().contains("fd-view")); | |
| 500 | + | } | |
| 501 | + | ||
| 502 | + | /// A deleted file has no content at this revision, so linking to it would | |
| 503 | + | /// be a 404 with extra steps. | |
| 504 | + | #[test] | |
| 505 | + | fn a_deleted_file_gets_no_view_link() { | |
| 506 | + | let mut d = fixture(4); | |
| 507 | + | d.files[0].kind = ChangeKind::Deleted; | |
| 508 | + | let html = | |
| 509 | + | files(&DiffView { blob_base: Some("/o/r/blob/abc"), ..DiffView::new(&d) }).into_string(); | |
| 510 | + | assert!(!html.contains("fd-view"), "{html:.400}"); | |
| 511 | + | } | |
| 512 | + | ||
| 513 | + | #[test] | |
| 514 | + | fn hunk_headers_carry_both_side_ranges() { | |
| 515 | + | let d = fixture(4); | |
| 516 | + | let html = files(&DiffView::new(&d)).into_string(); | |
| 517 | + | assert!(html.contains("@@ −1,4 +1,4 @@"), "{html:.600}"); | |
| 518 | + | } | |
| 519 | + | } | |
Amigrations/0005_session_tokens.sql+23−0
| @@ −0,0 +1,23 @@ | |||
| 1 | + | -- Session cookies carry a random token, not the row id. | |
| 2 | + | -- | |
| 3 | + | -- The cookie used to be the `sessions.id` UUIDv7: a 48-bit millisecond | |
| 4 | + | -- timestamp plus a counter that is reseeded once per millisecond and then | |
| 5 | + | -- incremented. Sessions minted in the same millisecond therefore shared their | |
| 6 | + | -- leading bits, and other UUIDs from the same generator (comment ids, which are | |
| 7 | + | -- rendered into review pages) disclosed the counter state. A UUIDv7 is a good | |
| 8 | + | -- primary key and was never meant to be a bearer secret. | |
| 9 | + | -- | |
| 10 | + | -- The cookie now carries 32 CSPRNG bytes and this table stores only their | |
| 11 | + | -- SHA-256, so a leaked snapshot of `sessions` no longer contains anything that | |
| 12 | + | -- can be presented to the server. | |
| 13 | + | -- | |
| 14 | + | -- Existing rows cannot be migrated: their tokens never existed, and the id is | |
| 15 | + | -- deliberately no longer accepted. Everyone is signed out exactly once, which | |
| 16 | + | -- is the correct price for the change. | |
| 17 | + | DELETE FROM sessions; | |
| 18 | + | ||
| 19 | + | ALTER TABLE sessions ADD COLUMN token_hash text NOT NULL; | |
| 20 | + | ||
| 21 | + | -- Unique because it is the lookup key, and the index is what makes resolving a | |
| 22 | + | -- session one probe rather than a scan. | |
| 23 | + | CREATE UNIQUE INDEX sessions_token_hash_idx ON sessions (token_hash); | |