Diff
patch 3974786fc6ec23b670d260f4bd4c9e671263f122
Author: fritjof@alokat.org
Date: Wed Apr 22 06:24:50 UTC 2026
* Security + performance hardening reconciled from Claude + Codex reviews
Security:
- getRepoTree now rejects non-empty subpaths that fail isSafeSubPath,
closing a path-disclosure that exposed _darcs/ via the tree UI.
- Tree/blob breadcrumbs HTML-escape labels and percent-encode hrefs
so a directory name containing HTML or URL-structural characters
can no longer inject markup or break out of the URL.
- serveClone enforces a strict two-segment allowlist
(hashed_inventory, inventories/_, patches/_, pristine.hashed/_,
optional packs/basic.tar.gz + packs/patches.tar.gz) instead of
serving every file that passed isSafeSubPath.
- readRepoDescription and getRepoBlob use strict ByteString +
explicit UTF-8 validation under try; a NUL-byte sniff keeps binary
files from being escaped character by character.
- All request-time canonicalizePath calls go through a new
safeCanonicalize helper that degrades IO errors to 404 instead of
leaking framework 500s.
- parsePortPure rejects empty, whitespace, hex, and non-digit port
strings, replacing the partial Text.Read.read on startup.
Performance:
- getRepoPatch hash-filters before rendering diffs; a missing hash no
longer walks and renders the entire patch history.
- getRepoSummary replaces listRepos + getRepoPatches + getRepoTags
with a single readPatches pass. Count and tag positions come from
cheap PatchInfo; extractPatchListing only runs on rows actually
displayed.
- Native Text implementation of esc replaces the HtmlPure.esc String
bridge. Equivalence against the Coq-extracted spec is pinned by
QuickCheck with a biased generator over escaped characters.
- Tree/blob breadcrumb build is now O(depth) (threaded Text prefix)
instead of O(depth^2) (acc ++ [p]).
UX / mobile-first:
- renderPage emits semantic <header>/<main>/<footer> landmarks plus a
favicon <link>. Empty breadcrumb <nav> is skipped on index and 404.
- Index page has a visible <h1> site title.
- repoNavBar is a <nav aria-label="Repository sections"> with
aria-current="page" on the active tab.
- Full-log entries are <article class="log-entry">.
- Tree-icon cells carry aria-hidden="true".
Tests:
- New Properties.Config covers parsePortPure's accepted/rejected set.
- Properties.Clone pins the two-segment clone allowlist and rejects
nested paths (patches/a/b, inventories/a/b, pristine.hashed/a/b).
- Properties.Html pins the native esc equivalence vs HtmlPure.esc
with a biased generator, plus example-based tests for the
breadcrumb escape + percent-encode contracts (including the empty
blob-breadcrumb degenerate case).
Reviews under reviews/ record the pre- and post-change findings from
three Codex + three Claude agents (first pass) and four Codex + four
Claude agents (post-change), plus the reconciliation document that
the user-visible fixes were derived from.
hunk ./app/Main.hs 17
+import Data.List (isPrefixOf)
hunk ./app/Main.hs 36
-import Data.List (isPrefixOf)
hunk ./app/Main.hs 74
- (o, _, []) -> return (foldl (flip id) defaultOpts o)
+ -- Apply option updates in input order (left-to-right) so that a
+ -- later @-c@ overrides an earlier one, matching common CLI
+ -- expectation and the pre-refactor behaviour.
+ (updates, _, []) -> return (foldl (\opts f -> f opts) defaultOpts updates)
hunk ./app/Main.hs 99
- let port = read (cfgLookupDefault "port" "3000" cfgMap)
- bind = cfgLookupDefault "bind" "127.0.0.1" cfgMap
+ port <- parsePort (cfgLookupDefault "port" "3000" cfgMap)
+ let bind = cfgLookupDefault "bind" "127.0.0.1" cfgMap
hunk ./app/Main.hs 125
+-- | Parse the @port@ config value into a valid TCP port or abort with a
+-- clear diagnostic. Replaces partial 'Text.Read.read', which crashed
+-- the process with an opaque @error "no parse"@ on any unparseable
+-- value (trailing whitespace, comment, hex, empty string).
+-- The accepted/rejected set is pinned by QuickCheck properties on
+-- 'parsePortPure'.
+parsePort :: String -> IO Int
+parsePort s = case parsePortPure s of
+ Just p -> return p
+ Nothing -> do
+ hPutStrLn stderr ("Invalid 'port' in config: " ++ show s
+ ++ " (expected integer 1-65535)")
+ exitFailure
+
hunk ./app/Main.hs 250
- -- Repository summary
+ -- Repository summary. Uses a single-repo summary builder that reads
+ -- the repository exactly once per request; the previous code rescanned
+ -- every repository in cfgRepoDir and reloaded the target repo three
+ -- times just to render this page.
hunk ./app/Main.hs 258
- repos <- liftIO $ listRepos (cfgRepoDir cfg)
- let mRepoInfo = findRepo name repos
- case mRepoInfo of
- Nothing -> do
- status status404
- html $ TL.fromStrict $ render404 "Repository not found."
- Just ri -> do
- patches <- liftIO $ getRepoPatches repoPath
- tags <- liftIO $ getRepoTags repoPath
- req <- request
- let clone = cloneUrl req name
- html $ TL.fromStrict $ renderRepoSummary now name ri clone (take 10 patches) tags
-
- -- Read-only darcs clone access: serves files strictly from the repo's
- -- _darcs/ directory. Only regular files are served; sub-paths are
- -- validated by the formally verified isSafeSubPath (no .., no hidden
- -- names, no nested _darcs/.git), and the canonical target must stay
- -- inside the repo's _darcs/ directory.
+ (ri, patches, tags) <- liftIO $ getRepoSummary repoPath name
+ req <- request
+ let clone = cloneUrl req name
+ html $ TL.fromStrict $ renderRepoSummary now name ri clone (take 10 patches) tags
+
+ -- Read-only darcs clone access: serves a strict allowlist of objects
+ -- required by the HTTP clone protocol (hashed_inventory, inventories,
+ -- patches, pristine.hashed, optional packs). Anything else under
+ -- _darcs/ ��� including prefs/*, format, and custom scripts ��� is kept
+ -- private even when it would pass the hidden-segment check.
hunk ./app/Main.hs 358
+-- 'safeCanonicalize' traps permission errors, symlink loops, and invalid
+-- byte sequences so they degrade to 404 rather than leaking framework
+-- 500 exception messages to the client.
hunk ./app/Main.hs 372
- then do
- status status404
- html $ TL.fromStrict $ render404 "Not found."
+ then notFound404
hunk ./app/Main.hs 378
- canonical <- liftIO $ canonicalizePath candidate
- let jailDir = addTrailingSlash (cfgStaticDir cfg)
- if not (jailDir `isPrefixOf` canonical)
- then do
- status status404
- html $ TL.fromStrict $ render404 "Not found."
- else do
- exists <- liftIO $ doesFileExist canonical
- if not exists
- then do
- status status404
- html $ TL.fromStrict $ render404 "Not found."
+ mCanonical <- liftIO $ safeCanonicalize candidate
+ case mCanonical of
+ Nothing -> notFound404
+ Just canonical -> do
+ let jailDir = addTrailingSlash (cfgStaticDir cfg)
+ if not (jailDir `isPrefixOf` canonical)
+ then notFound404
hunk ./app/Main.hs 386
- setHeader "Content-Type" (mimeType (takeExtension canonical))
- setHeader "Cache-Control" "public, max-age=86400, immutable"
- file canonical
+ exists <- liftIO $ doesFileExist canonical
+ if not exists
+ then notFound404
+ else do
+ setHeader "Content-Type" (mimeType (takeExtension canonical))
+ setHeader "Cache-Control" "public, max-age=86400, immutable"
+ file canonical
hunk ./app/Main.hs 397
+-- Canonicalization failures (e.g. NUL byte in name, symlink loop) yield 404.
hunk ./app/Main.hs 413
- canonical <- liftIO $ canonicalizePath candidate
- let jailDir = addTrailingSlash (cfgRepoDir cfg)
- if jailDir `isPrefixOf` canonical
- then action canonical
- else do
- status status403
- html $ TL.fromStrict $ render404 "Access denied."
+ mCanonical <- liftIO $ safeCanonicalize candidate
+ case mCanonical of
+ Nothing -> do
+ status status404
+ html $ TL.fromStrict $ render404 "Repository not found."
+ Just canonical -> do
+ let jailDir = addTrailingSlash (cfgRepoDir cfg)
+ if jailDir `isPrefixOf` canonical
+ then action canonical
+ else do
+ status status403
+ html $ TL.fromStrict $ render404 "Access denied."
hunk ./app/Main.hs 431
-findRepo :: T.Text -> [RepoInfo] -> Maybe RepoInfo
-findRepo name = foldr (\r acc -> if riName r == name then Just r else acc) Nothing
-
hunk ./app/Main.hs 432
--- read-only darcs clones over HTTP. Rejects unsafe sub-paths and
--- anything that escapes the repo jail after canonicalization.
+-- read-only darcs clones over HTTP.
+-- Rejects anything outside the 'isCloneSubPath' allowlist so that only
+-- the objects the darcs client actually needs (hashed_inventory,
+-- inventories/*, patches/*, pristine.hashed/*, optional packs/*) are
+-- served; prefs, format, and any unrelated file stay private.
hunk ./app/Main.hs 439
- | not (isSafeSubPath subPath) = notFound404
+ | not (isCloneSubPath subPath) = notFound404
hunk ./app/Main.hs 443
- canonical <- liftIO $ canonicalizePath candidate
- let jailSlash = addTrailingSlash jail
- if not (jailSlash `isPrefixOf` canonical)
- then notFound404
- else do
- exists <- liftIO $ doesFileExist canonical
- if not exists
+ mCanonical <- liftIO $ safeCanonicalize candidate
+ case mCanonical of
+ Nothing -> notFound404
+ Just canonical -> do
+ let jailSlash = addTrailingSlash jail
+ if not (jailSlash `isPrefixOf` canonical)
hunk ./app/Main.hs 451
- setHeader "Content-Type" "application/octet-stream"
- setHeader "Cache-Control" "no-cache"
- file canonical
- where
- notFound404 = do
- status status404
- html $ TL.fromStrict $ render404 "Not found."
+ exists <- liftIO $ doesFileExist canonical
+ if not exists
+ then notFound404
+ else do
+ setHeader "Content-Type" "application/octet-stream"
+ setHeader "Cache-Control" "no-cache"
+ file canonical
+
+-- | Shared 404 responder for Scotty handlers.
+notFound404 :: ActionM ()
+notFound404 = do
+ status status404
+ html $ TL.fromStrict $ render404 "Not found."
hunk ./app/Main.hs 478
-
hunk ./darcsweb.cabal 64
+ , Properties.Config
adddir ./reviews
addfile ./reviews/_post-prompt-bugs.md
hunk ./reviews/_post-prompt-bugs.md 1
+You are a review agent. Review this code primarily for bugs, correctness issues, behavioral regressions, missing validation in tests and verification artifacts, and secure-by-default behavior.
+
+Scope:
+- Review the darcsweb Haskell project at /home/fritjof/repositories/playground/darcsweb against the current uncommitted darcs changes. The diff touches `app/Main.hs`, `src/DarcsWeb/Config.hs`, `src/DarcsWeb/Darcs.hs`, `src/DarcsWeb/Html.hs`, `test/Properties/Clone.hs`, and `test/Properties/Html.hs`. Use `darcs whatsnew` or read the changed files directly to see the new state. The full diff is also available at /tmp/darcs-diff.txt.
+- Prioritize changed code, but inspect nearby code when needed.
+- When useful, inspect recent darcs history (`darcs log`) for the touched code to understand intent and past fixes.
+- The codebase includes Coq/Rocq proofs under `verified/` and QuickCheck properties under `test/Properties/`. Treat them as part of the verification surface.
+
+Review focus:
+- Logic errors
+- Wrong assumptions
+- Null/None/optional handling
+- Boundary conditions and off-by-one mistakes
+- State handling, concurrency, and race conditions
+- Error handling and recovery
+- Input validation
+- Security-relevant correctness issues
+- Whether the implementation follows secure-by-default principles such as safe defaults, least privilege, fail-closed behavior, and explicit opt-in for risky behavior
+- Regressions against likely intended behavior
+- Whether relevant tests were added or updated
+- Whether tests actually cover changed behavior, edge cases, and failure paths
+- Whether verification artifacts were added or updated where required
+- Whether verification still matches the implementation and intended behavior
+- Gaps where the change should have been reflected in tests, assertions, contracts, proofs, or other verification assets
+
+Output rules:
+- Report findings only if they are concrete and defensible.
+- Prioritize higher-severity issues first.
+- For each finding, use this format:
+ 1. Severity
+ 2. File and line
+ 3. What is wrong
+ 4. Why it can fail in practice
+ 5. Minimal fix suggestion
+- Ignore style and minor maintainability comments unless they directly affect correctness.
+- Treat missing, weak, outdated, or inconsistent tests/verification as findings when they materially reduce confidence.
+- Treat insecure defaults or permissive fallback behavior as findings when they violate secure-by-default expectations.
+
+If no issues are found, say exactly:
+No concrete bug findings.
+
+Then list:
+- Residual risks
+- Missing test coverage
+- Any changed behavior that remains unverified
+
+Write the full review to /home/fritjof/repositories/playground/darcsweb/reviews/post-{AI}-bugs.md (replace {AI} with "codex" or "claude" depending on who you are).
addfile ./reviews/_post-prompt-efficiency.md
hunk ./reviews/_post-prompt-efficiency.md 1
+You are a review agent. Review this code primarily for efficiency and performance risks.
+
+Scope:
+- Review the darcsweb Haskell project at /home/fritjof/repositories/playground/darcsweb against the current uncommitted darcs changes. The diff touches `app/Main.hs`, `src/DarcsWeb/Config.hs`, `src/DarcsWeb/Darcs.hs`, `src/DarcsWeb/Html.hs`, `test/Properties/Clone.hs`, and `test/Properties/Html.hs`. Use `darcs whatsnew` or read the changed files directly. The full diff is at /tmp/darcs-diff.txt.
+- Prioritize changed code, but inspect nearby code when needed to understand runtime cost.
+- When useful, inspect recent darcs history for touched code to understand performance-sensitive context.
+- Focus on meaningful performance issues, not speculative micro-optimizations.
+
+Review focus:
+- Algorithmic complexity
+- Unnecessary allocations or copies
+- Repeated work inside loops
+- Expensive I/O, network, or filesystem usage
+- Database query count and query shape (N/A for this project)
+- Synchronization and locking overhead
+- Memory growth risks
+- Hot-path inefficiencies
+- Poor batching, caching, or buffering decisions
+- Scalability problems under realistic load
+
+Output rules:
+- Report only meaningful efficiency findings.
+- Prefer evidence-based reasoning. If something is inferred to be a hot path, state that clearly as an inference.
+- For each finding, use this format:
+ 1. Severity
+ 2. File and line
+ 3. The inefficiency
+ 4. When it becomes a real problem
+ 5. A concrete optimization direction
+- Do not recommend changes that make the code substantially harder to maintain unless the performance benefit is likely to matter.
+
+If no issues are found, say exactly:
+No significant efficiency findings.
+
+Then list:
+- Paths worth benchmarking
+- Load assumptions behind the review
+- Any scalability questions that could not be verified from code alone
+
+Write the full review to /home/fritjof/repositories/playground/darcsweb/reviews/post-{AI}-efficiency.md (replace {AI} with "codex" or "claude" depending on who you are).
addfile ./reviews/_post-prompt-quality.md
hunk ./reviews/_post-prompt-quality.md 1
+You are a review agent. Review this code primarily for code quality, maintainability, clarity, and design robustness.
+
+Scope:
+- Review the darcsweb Haskell project at /home/fritjof/repositories/playground/darcsweb against the current uncommitted darcs changes. The diff touches `app/Main.hs`, `src/DarcsWeb/Config.hs`, `src/DarcsWeb/Darcs.hs`, `src/DarcsWeb/Html.hs`, `test/Properties/Clone.hs`, and `test/Properties/Html.hs`. Use `darcs whatsnew` or read the changed files directly. The full diff is at /tmp/darcs-diff.txt.
+- Prioritize changed code, but inspect nearby code when needed.
+- When useful, inspect recent darcs history (`darcs log`) to understand why the code evolved this way.
+- Judge the code against existing project patterns rather than personal style preferences.
+
+Review focus:
+- Readability and clarity
+- Cohesion and separation of concerns
+- Unnecessary complexity
+- Duplication
+- Naming quality
+- API design and ergonomics
+- Maintainability risks
+- Fragile abstractions
+- Error-prone structure
+- Test quality and test maintainability
+- Violations of established project conventions or architecture
+
+Output rules:
+- Focus on substantive quality issues, not cosmetic nits.
+- Separate must-fix issues from optional improvements.
+- For each finding, use this format:
+ 1. Severity
+ 2. File and line
+ 3. The quality problem
+ 4. Why it matters long-term
+ 5. A practical improvement
+- Prefer simpler solutions over additional abstraction unless the abstraction clearly pays for itself.
+
+If no issues are found, say exactly:
+No major code quality findings.
+
+Then list:
+- Optional improvements
+- Areas that may become maintenance risks later
+- Any testing or structure gaps worth watching
+
+Write the full review to /home/fritjof/repositories/playground/darcsweb/reviews/post-{AI}-quality.md (replace {AI} with "codex" or "claude" depending on who you are).
addfile ./reviews/_post-prompt-webapp.md
hunk ./reviews/_post-prompt-webapp.md 1
+You are a review agent. Review this code only as a web application review, with primary focus on mobile-first design, consistent behavior, and frontend performance.
+
+Scope:
+- Review the darcsweb Haskell project at /home/fritjof/repositories/playground/darcsweb against the current uncommitted darcs changes. The diff touches `app/Main.hs`, `src/DarcsWeb/Config.hs`, `src/DarcsWeb/Darcs.hs`, `src/DarcsWeb/Html.hs`, `test/Properties/Clone.hs`, and `test/Properties/Html.hs`. Use `darcs whatsnew` or read the changed files directly. The full diff is at /tmp/darcs-diff.txt.
+- Focus on web application code and assets: UI components, pages, layouts, styling, client-side logic, routing, rendering, and web-facing API usage that affects user experience. In this project the HTML is produced server-side by `src/DarcsWeb/Html.hs` and route handlers in `app/Main.hs`. You may consult `static/style.css` for rendering behavior but do not critique the CSS itself unless a Haskell-side change requires a matching CSS change.
+- Prioritize changed code, but inspect nearby code when needed to understand behavior.
+- When useful, inspect recent darcs history for touched code to understand intent and earlier fixes.
+- Prefer concrete findings over design preference comments.
+
+Review focus:
+- Mobile-first layout behavior
+- Responsive design across small screens first, then larger breakpoints
+- Touch target sizes, spacing, typography, and readability on mobile
+- Consistency of behavior across pages, components, states, and breakpoints
+- Loading, empty, error, and interactive states
+- Cross-browser or device-specific fragility when it is reasonably inferable from the code
+- Unnecessary client-side work
+- Large bundles or avoidable dependencies
+- Expensive rendering, re-rendering, or hydration
+- Inefficient network usage, asset loading, image handling, and font loading
+- Layout shift, blocked rendering, and poor perceived performance
+- Backend interaction patterns that are likely to slow down the web UI
+
+Output rules:
+- Report only meaningful findings that affect mobile usability, behavioral consistency, or speed.
+- Do not focus on visual taste unless it harms usability or consistency.
+- Distinguish clearly between mobile-first design issues, consistency issues, and performance issues.
+- If something is inferred rather than measured, say that clearly.
+- For each finding, use this format:
+ 1. Severity
+ 2. File and line
+ 3. The issue
+ 4. Why it matters for users
+ 5. A practical fix
+- Prefer recommendations that improve speed without making the code substantially harder to maintain.
+
+If no issues are found, say exactly:
+No major web application findings.
+
+Then list:
+- Areas worth testing on real mobile devices
+- Performance paths worth measuring
+- Any consistency risks that could not be fully verified from code alone
+
+Write the full review to /home/fritjof/repositories/playground/darcsweb/reviews/post-{AI}-webapp.md (replace {AI} with "codex" or "claude" depending on who you are).
addfile ./reviews/_prompt-readability.md
hunk ./reviews/_prompt-readability.md 1
+# Persona: Readability & Clarity Specialist
+
+You are a highly skilled developer reviewing the darcsweb Haskell project with a single dominant concern: **readability**. Code must be clear to a competent Haskeller who is reading it for the first time, without needing to decode clever tricks.
+
+## What you value
+
+- Plain, boring Haskell. Standard idioms, descriptive names, explicit types at top level.
+- Small functions with a single job. Functions that fit on one screen.
+- No point-free golf where an explicit lambda or `where`-binding would be clearer.
+- No unnecessary operator chains or `<$> <*> pure $ x`-style stacking when a `do` block is clearer.
+- Modules organized by responsibility, with clear import lists (preferably explicit or qualified).
+- Names that communicate domain meaning, not types or abbreviations.
+- Comments that explain WHY only where the WHY is non-obvious. No comments that restate the code.
+- Error paths that are as readable as the happy path.
+
+## What you dislike (and will flag)
+
+- Language extensions used when vanilla Haskell suffices.
+- `fmap . fmap`, excessive `&`/`flip`/`on` usage when direct code is clearer.
+- Large `do` blocks that mix IO, pure transformations, and error handling without separation.
+- Partial functions (`head`, `fromJust`, `!!`, pattern match failure) ��� these hurt readability because the reader has to reason about totality.
+- Magic numbers, ad-hoc string tags, inconsistent naming (`cfg` in one place, `config` in another).
+- Over-abstraction: typeclass/type-family gymnastics where a sum type and a case would do.
+
+## Review scope
+
+Haskell sources only:
+- `src/DarcsWeb/*.hs`
+- `app/Main.hs`
+- `gen/*.hs` (these are machine-extracted from Coq ��� note style issues but mark them as "extracted, cannot edit directly" if they stem from extraction)
+- `test/Spec.hs` and `test/Properties/*.hs`
+
+Do NOT review: `verified/*.v`, Dockerfile, Caddyfile, `static/`, `_darcs/`, `dist-newstyle/`, `.stack-work/`.
+
+## Output format
+
+Write a markdown file with these sections:
+
+```
+# Readability Review ��� darcsweb
+
+## Summary
+(2���4 sentences: overall readability, biggest friction points for a new reader.)
+
+## Confusing constructs
+Point-free chains, clever operators, over-polymorphic helpers. file:line + suggested plain rewrite.
+
+## Naming
+Inconsistent or unclear identifiers. file:line + suggested name.
+
+## Structure
+Modules/functions that should be split, reordered, or regrouped. file:line.
+
+## Comments & docs
+Missing/misleading comments; top-of-file module headers; Haddock that would help. file:line.
+
+## Small wins
+Trivial edits (imports, whitespace, type signatures on top-level bindings) that make the code noticeably cleaner. Ordered by impact.
+
+## Extracted code note
+Call out any `gen/*.hs` readability issues but flag them as un-editable at source; suggest a corresponding Coq-side or wrapper-side improvement instead.
+```
+
+Every finding MUST cite `path/to/file.hs:LINE`.
addfile ./reviews/_prompt-security.md
hunk ./reviews/_prompt-security.md 1
+# Persona: Paranoid Security & Verification Specialist
+
+You are a paranoid, highly skilled developer reviewing the darcsweb Haskell project. Your priorities, in order:
+
+1. **Security**: treat this as a web application exposed to the public internet. Think OWASP Top 10 (injection, path traversal, XSS, CSRF, auth, deserialization, SSRF, etc.), Haskell-specific hazards (partial functions, impure `Text.Read.read`, `undefined`, lazy IO, exceptions escaping `IO`), and process-boundary issues (subprocess invocation of `darcs`, shell injection, argument quoting, environment leakage).
+2. **Secure-by-default**: flag any API or default that makes misuse the path of least resistance. Prefer types that make wrong states unrepresentable.
+3. **Verification over testing**: the project already has Coq proofs in `verified/` for the pure modules (`HtmlPure`, `PathPure`, `CspPure`). Check whether the Haskell implementations in `gen/` faithfully match the Coq model, and whether the property boundary (pure vs IO) is correctly drawn. If tests are warranted, prefer QuickCheck-style randomized properties over example-based tests.
+
+## Review scope
+
+Haskell sources only. Read and analyze:
+- `src/DarcsWeb/*.hs` ��� application logic (Clone, Config, Darcs, Html, Types)
+- `app/Main.hs` ��� entry point / web server wiring
+- `gen/*.hs` ��� extracted pure code (HtmlPure, PathPure, CspPure)
+- `test/Spec.hs` and `test/Properties/*.hs` ��� existing QuickCheck properties
+- `darcsweb.cabal` ��� dependency bounds and compiler flags
+
+Do NOT review: `verified/*.v`, Dockerfile, Caddyfile, `static/`, `_darcs/`, `dist-newstyle/`, `.stack-work/`.
+
+## Output format
+
+Write a markdown file with these sections:
+
+```
+# Security Review ��� darcsweb
+
+## Summary
+(2���4 sentences: overall posture, biggest risks.)
+
+## Critical findings
+For each: title, file:line, why it is dangerous, concrete exploit scenario, proposed fix.
+
+## High findings
+Same structure.
+
+## Medium / Low findings
+Same structure, terser.
+
+## Verification gaps
+Where does the Haskell code diverge from what the Coq proofs cover? Which IO-edges need additional QuickCheck properties?
+
+## Hardening recommendations
+Defaults, types, dependency bounds, compiler flags. Be concrete.
+```
+
+Be specific. Every finding MUST cite `path/to/file.hs:LINE` so the fix can be applied. If you are unsure whether a finding is exploitable, still list it and mark severity as "Medium ��� needs confirmation".
addfile ./reviews/_prompt-webperf.md
hunk ./reviews/_prompt-webperf.md 1
+# Persona: Mobile-First Web Performance & Design Specialist
+
+You are a highly skilled web developer reviewing the darcsweb Haskell project. Your priorities, in order:
+
+1. **Mobile-first**: the HTML output has to render well and be usable on a 360px-wide phone over a slow connection first, desktop second. Viewport, tap targets, overflow handling, readable font sizes, no horizontal scroll.
+2. **Speed**: TTFB, payload size, number of requests, cache headers, compression, unnecessary work on every request. Haskell-side: strict vs lazy ByteString, Text encoding, repeated file reads, subprocess spawns. HTTP-side: Caddyfile-level concerns are out of scope, but anything the Haskell app controls (Cache-Control, ETag, Content-Length, streaming) is in scope.
+3. **Clean, consistent design**: visual consistency of generated HTML, semantic markup (use `<nav>`, `<main>`, `<article>`, not div soup), accessibility (alt text, labels, heading hierarchy, color contrast), and a coherent component style.
+
+## Review scope
+
+Haskell sources only. Focus on code that produces HTML, sets headers, or reads/streams bytes:
+- `src/DarcsWeb/Html.hs` and `gen/HtmlPure.hs` ��� HTML generation
+- `src/DarcsWeb/*.hs` ��� any handlers that touch the response
+- `app/Main.hs` ��� routing, response headers, middleware
+- `gen/CspPure.hs`, `gen/PathPure.hs` ��� CSP headers and URL/path logic
+- `darcsweb.cabal` ��� dependency choices that affect bundle/perf (e.g. `text` vs `bytestring`)
+
+Do NOT review: `verified/*.v`, Dockerfile, Caddyfile, `_darcs/`, `dist-newstyle/`, `.stack-work/`. You may reference `static/style.css` if an HTML change would require a matching CSS change, but do not critique the CSS itself in depth.
+
+## Output format
+
+Write a markdown file with these sections:
+
+```
+# Mobile-First Web Performance Review ��� darcsweb
+
+## Summary
+(2���4 sentences: overall perf/UX posture on mobile, biggest wins.)
+
+## Mobile rendering issues
+Missing viewport meta, non-responsive layout, tap-target sizing, overflow, font-size, touch-friendliness. file:line each.
+
+## Performance issues
+Payload bloat, inefficient ByteString/Text handling, repeated IO, missing caching headers, unnecessary subprocess spawns. file:line each.
+
+## Design & semantics
+Non-semantic HTML, accessibility gaps, heading hierarchy, color/contrast (only if introduced by the Haskell code), inconsistent markup between pages. file:line each.
+
+## Quick wins
+3���7 concrete changes with the highest impact-to-effort ratio. Ordered. Each with an explicit diff sketch or exact code change.
+
+## Deeper restructuring (optional)
+Larger changes, only if they unlock a qualitative jump.
+```
+
+Every finding MUST cite `path/to/file.hs:LINE`.
addfile ./reviews/claude-readability.md
hunk ./reviews/claude-readability.md 1
+# Readability Review ��� darcsweb
+
+## Summary
+
+Overall the project reads well for a small Scotty-based web app: modules are
+split by responsibility, the domain types live in one place, and the security-
+sensitive helpers are isolated in `Clone.hs`/`Darcs.hs`. The biggest sources
+of friction for a first-time reader are (1) deeply nested `if ��� then ��� else`
+chains in `Main.hs` (`serveStatic`, `withRepo`, `serveClone`, `getRepoBlob`)
+where guards or early-return helpers would flatten the control flow
+dramatically, (2) long `renderXxx` functions in `Html.hs` that interleave
+string concatenation, branching and escaping in one big `T.concat` block,
+and (3) a sprinkling of partial functions (`head`, `init`, `last`, `read`,
+`!!`) that force the reader to reason about totality. The Coq-extracted
+modules in `gen/` are, as expected, painful to read but correctly marked
+un-editable at the Haskell level.
+
+## Confusing constructs
+
+### `app/Main.hs`
+
+- **`app/Main.hs:74`** ��� `foldl (flip id) defaultOpts o` is cute but opaque.
+ Current:
+ ```haskell
+ (o, _, []) -> return (foldl (flip id) defaultOpts o)
+ ```
+ Plain rewrite:
+ ```haskell
+ (updates, _, []) -> return (foldr ($) defaultOpts updates)
+ -- or, even more explicit:
+ -- return (foldl (\opts f -> f opts) defaultOpts updates)
+ ```
+ `foldr ($)` is the standard idiom for "apply each endomorphism in turn";
+ `flip id` hides the fact that the fold is just function application.
+
+- **`app/Main.hs:105-120`** ��� The two branches of `if optDaemon opts` duplicate
+ almost the entire startup (logging, building the config, calling
+ `runServer`). A reader has to diff the two branches visually to see that
+ only the logger and the `withSyslog` wrapper differ. Extract:
+ ```haskell
+ let startup logf wrap = wrap $ do
+ logf $ "Starting on " ++ bind ++ ":" ++ show port
+ logf $ "Serving repositories from: " ++ absRepoDir
+ runServer (mkConfig port bind absRepoDir title absStaticDir logf)
+ if optDaemon opts
+ then do
+ logStdout $ "Daemonizing (bind " ++ bind ++ ":" ++ show port ++ ")"
+ hFlush stdout
+ daemonize (withSyslog "darcsweb" [LogPID] Daemon (startup logSyslog id))
+ else startup logStdout id
+ ```
+
+- **`app/Main.hs:346-380`** ��� `serveStatic` is a 35-line cascade of five
+ nested `if ��� then ��� else` blocks, each with a duplicated 404 tail. Refactor
+ to a helper that returns `Either NotFound FilePath` and a single responder:
+ ```haskell
+ serveStatic :: DarcsWebConfig -> ActionM ()
+ serveStatic cfg = do
+ req <- request
+ mFile <- liftIO (resolveStatic cfg (pathInfo req))
+ case mFile of
+ Nothing -> respond404 "Not found."
+ Just path -> do
+ setHeader "Content-Type" (mimeType (takeExtension path))
+ setHeader "Cache-Control" "public, max-age=86400, immutable"
+ file path
+
+ respond404 :: Text -> ActionM ()
+ respond404 msg = do
+ status status404
+ html $ TL.fromStrict $ render404 msg
+ ```
+ `respond404` alone replaces six copy-pasted 2-line blocks across this
+ file (lines 242, 292, 306, 320, 333, 341, 358, 368, 375, 389, 396,
+ 405, 438). This is probably the single biggest readability win in the
+ project.
+
+- **`app/Main.hs:354-356`** ��� Three separate `any` predicates on the same
+ list, two of which overlap (`s == "."` and `T.isPrefixOf "."`).
+ Current:
+ ```haskell
+ if null subSegments
+ || any (\s -> s == ".." || s == "." || T.null s) subSegments
+ || any (T.isPrefixOf ".") subSegments
+ ```
+ Plain rewrite:
+ ```haskell
+ let isUnsafe s = T.null s || s == ".." || T.isPrefixOf "." s
+ if null subSegments || any isUnsafe subSegments
+ ```
+ (`T.isPrefixOf "."` already covers `s == "."`.)
+
+- **`app/Main.hs:385-406`** ��� `withRepo` mixes name validation, repo
+ existence check, canonicalization, and jail check in one big guard-and-
+ nested-if. The guard on line 387-388 packs three predicates with awkward
+ operator precedence. Rewrite the guard as a named helper:
+ ```haskell
+ isBadName :: Text -> Bool
+ isBadName n =
+ T.any (`elem` ("/\\" :: String)) n
+ || T.isInfixOf ".." n
+ || T.isPrefixOf "." n
+ ```
+ then `withRepo` reads `| isBadName name = respond404 "Invalid repository name."`.
+
+- **`app/Main.hs:413-414`** ��� `findRepo` reimplements `Data.List.find`:
+ ```haskell
+ findRepo name = foldr (\r acc -> if riName r == name then Just r else acc) Nothing
+ ```
+ Plain rewrite:
+ ```haskell
+ import Data.List (find)
+ findRepo name = find ((== name) . riName)
+ ```
+
+- **`app/Main.hs:449-454`** ��� Point-free section hides intent:
+ ```haskell
+ decodeHdr = (>>= eitherToMaybe . TE.decodeUtf8') . flip lookup hdrs
+ ```
+ Plain rewrite:
+ ```haskell
+ decodeHdr :: BS.ByteString -> Maybe Text
+ decodeHdr name = do
+ raw <- lookup name hdrs
+ either (const Nothing) Just (TE.decodeUtf8' raw)
+ ```
+ One fewer combinator, one extra type signature, much clearer causal chain.
+
+### `src/DarcsWeb/Clone.hs`
+
+- **`src/DarcsWeb/Clone.hs:117-119`** ��� The list comprehension returns a
+ list of `Text`s and then `T.concat`s, which re-walks the bytestring:
+ ```haskell
+ encodePathSegment t =
+ T.concat [ encodeByte b | b <- BS.unpack (TE.encodeUtf8 t) ]
+ ```
+ Plain and equivalent:
+ ```haskell
+ encodePathSegment t =
+ T.concat (map encodeByte (BS.unpack (TE.encodeUtf8 t)))
+ ```
+ A trivial change, but it removes the one list-comprehension from this
+ module (the rest is `map`/`filter`/`case`), eliminating a stylistic bump.
+
+- **`src/DarcsWeb/Clone.hs:122-124`** ��� The `Word8 -> Text` branch uses
+ `toEnum (fromIntegral b)` which is only safe because we already filtered
+ to unreserved ASCII. A reader has to verify that. A more obviously safe
+ form: `T.singleton (BC.head (BS.singleton b))` is worse; the clearer
+ fix is a comment or a named helper:
+ ```haskell
+ -- safe: isUnreserved b implies b is an ASCII byte
+ word8ToChar :: Word8 -> Char
+ word8ToChar = toEnum . fromIntegral
+ ```
+ and use `word8ToChar b` in both `isUnreserved` (which also does the same
+ conversion) and the `T.singleton` branch.
+
+- **`src/DarcsWeb/Clone.hs:125-129`** ��� `isUnreserved` is defined inside
+ `encodeByte`'s `where` and does the `toEnum (fromIntegral b)` dance
+ again. Extract once (see previous bullet).
+
+### `src/DarcsWeb/Darcs.hs`
+
+- **`src/DarcsWeb/Darcs.hs:69-96`** ��� `checkRepo` has a ladder of three
+ nested `if ��� then ��� else` and duplicates the `RepoInfo` record
+ construction between the two `Just`/`Nothing` cases (only two fields
+ differ). Rewrite as a single early-exit style:
+ ```haskell
+ checkRepo baseDir name = do
+ let fullPath = baseDir </> name
+ isDir <- doesDirectoryExist fullPath
+ isRepo <- if isDir then isDarcsRepo fullPath else pure False
+ if not isRepo
+ then pure []
+ else do
+ desc <- readRepoDescription fullPath
+ mInfo <- getBasicRepoInfo fullPath
+ let (lastDate, count) = maybe ("", 0) id mInfo
+ pure [ RepoInfo
+ { riName = T.pack name
+ , riPath = fullPath
+ , riDescription = desc
+ , riLastChange = lastDate
+ , riPatchCount = count
+ } ]
+ ```
+ (`fromMaybe ("", 0) mInfo` is even shorter.) The record literal appears
+ once, which means future fields can't drift between the branches.
+
+- **`src/DarcsWeb/Darcs.hs:176-181`** ��� `jailCheck` re-implements trailing-slash
+ logic that already lives in `PathPure.add_trailing_slash`:
+ ```haskell
+ let jailSlash = if null jail then "/"
+ else if last jail == '/' then jail else jail ++ "/"
+ return (jailSlash `isPrefixOf` canonical || canonical == init jailSlash)
+ ```
+ Uses **partial** `last` and **partial** `init`. Both are safe only
+ because of the `null` guard and the fact that the string ends in `/`,
+ which forces the reader to trace the condition twice. Plain rewrite:
+ ```haskell
+ jailCheck jail path = do
+ canonical <- canonicalizePath path
+ let jailSlash = PathPure.add_trailing_slash jail
+ jailNoSlash = dropSuffix "/" jailSlash
+ pure (jailSlash `isPrefixOf` canonical || canonical == jailNoSlash)
+ where
+ dropSuffix suf s
+ | suf `isSuffixOf` s = take (length s - length suf) s
+ | otherwise = s
+ ```
+ Now there are no partial functions and the "is it the jail itself, or
+ inside the jail?" logic is a one-liner predicate each.
+
+- **`src/DarcsWeb/Darcs.hs:141-144`, `252-272`** ��� Two `extractPatch*`
+ functions differ only in whether they render the diff. Collapse into
+ a parameterised helper:
+ ```haskell
+ data DiffMode = WithDiff | ListingOnly
+
+ extractPatch :: RepoPatch p => DiffMode -> PatchInfoAnd p wA wB -> PatchSummary
+ extractPatch mode piap =
+ let pinfo = info piap
+ (diffText, summaryText) = case hopefullyM piap of
+ Just p -> ( case mode of
+ WithDiff -> T.pack (renderString (showPatch ForDisplay p))
+ ListingOnly -> ""
+ , T.pack (renderString (summary p)) )
+ Nothing -> (if mode == WithDiff then "(patch content unavailable)" else "", "")
+ in patchInfoToSummary pinfo diffText summaryText
+ ```
+ Two callers, one function. If that feels over-engineered, at minimum
+ factor out the shared `case hopefullyM piap of Just _ -> ��� ; Nothing -> ���`
+ idiom into a helper so the two definitions visually pair up.
+
+### `src/DarcsWeb/Html.hs`
+
+- **`src/DarcsWeb/Html.hs:274`** ��� `maybe (psName ps) id (psTagName ps)` is
+ `fromMaybe (psName ps) (psTagName ps)`. Preferred:
+ ```haskell
+ import Data.Maybe (fromMaybe)
+ fromMaybe (psName ps) (psTagName ps)
+ ```
+
+- **`src/DarcsWeb/Html.hs:298-308`** ��� `buildCrumbs` is a recursive helper
+ that threads an accumulator. A reader has to follow two state updates
+ per recursion. A `scanl`/`zip`/`map` version reads as "build prefix paths,
+ then render each":
+ ```haskell
+ renderTreeBreadcrumb repoName subPath =
+ let parts = if T.null subPath then [] else T.splitOn "/" subPath
+ paths = drop 1 (T.scanl joinSeg "" parts)
+ crumbs = zipWith crumb paths parts
+ crumb href label =
+ " / <a href=\"/repo/" <> esc repoName <> "/tree/" <> esc href <> "\">"
+ <> esc label <> "</a>"
+ joinSeg acc p = if T.null acc then p else acc <> "/" <> p
+ in "<div class=\"tree-path\">"
+ <> "<a href=\"/repo/" <> esc repoName <> "/tree\">[root]</a>"
+ <> T.concat crumbs
+ <> "</div>\n"
+ ```
+ (Note: `T.scanl` works on chars, so use `Data.List.scanl` with the list.
+ The point stands: "paths, labels, zip, render" is easier to skim than
+ mutually recursive accumulator code.) The same helper is **duplicated**
+ in `renderBlobBreadcrumb` (line 367-381) with only the final `fileName`
+ line differing ��� extract once.
+
+- **`src/DarcsWeb/Html.hs:431-445`** ��� `relativeDate` is a ladder of six
+ `if ��� then ��� else if ���` branches. Haskell's `|` guards read much
+ better for this pattern:
+ ```haskell
+ relativeDate now dateStr =
+ case parseDarcsDate (T.unpack dateStr) of
+ Nothing -> dateStr
+ Just t ->
+ let secs = floor (diffUTCTime now t) :: Int
+ in pickBucket secs
+ where
+ pickBucket s
+ | s < 0 = formatAbsolute dateStr
+ | s < 60 = pluralize s "second"
+ | s < 3600 = pluralize (s `div` 60) "minute"
+ | s < 86400 = pluralize (s `div` 3600) "hour"
+ | s < 604800 = pluralize (s `div` 86400) "day"
+ | s < 2419200 = pluralize (s `div` 604800) "week"
+ | otherwise = formatAbsolute dateStr
+ ```
+ Bonus: name the magic numbers:
+ ```haskell
+ minute, hour, day, week, month :: Int
+ minute = 60; hour = 60 * minute; day = 24 * hour
+ week = 7 * day; month = 4 * week
+ ```
+
+- **`src/DarcsWeb/Html.hs:419-425`** ��� `highlightDiff`'s three-branch cascade
+ is fine, but each branch builds the same `<span class="diff-X">���</span>\n`
+ shape. A data-driven version reads more symmetrically:
+ ```haskell
+ highlightDiff = T.concat . map highlightLine . T.lines
+ where
+ highlightLine l = case classify l of
+ Just cls -> "<span class=\"" <> cls <> "\">" <> esc l <> "</span>\n"
+ Nothing -> esc l <> "\n"
+ classify l
+ | T.isPrefixOf "+" l = Just "diff-add"
+ | T.isPrefixOf "-" l = Just "diff-del"
+ | T.isPrefixOf "@@" l = Just "diff-hunk"
+ | otherwise = Nothing
+ ```
+
+- **`src/DarcsWeb/Html.hs:452-458`** ��� `formatAbsolute` uses string slicing
+ with `take`/`drop` offsets. It works but is tedious to audit. A splitAt
+ pipeline reads more naturally:
+ ```haskell
+ formatAbsolute d
+ | T.length d >= 14 =
+ let (y, r1) = T.splitAt 4 d
+ (mo, r2) = T.splitAt 2 r1
+ (dy, r3) = T.splitAt 2 r2
+ (h, r4) = T.splitAt 2 r3
+ (mi, r5) = T.splitAt 2 r4
+ (se, _) = T.splitAt 2 r5
+ in y <> "-" <> mo <> "-" <> dy <> " " <> h <> ":" <> mi <> ":" <> se
+ | otherwise = d
+ ```
+ (And stay in `Text`, avoiding the `T.unpack`/`T.pack` round-trip.)
+
+- **`src/DarcsWeb/Html.hs:367-381`** ��� `renderBlobBreadcrumb` uses partial
+ `init` and `last` on `T.splitOn "/" subPath`. If `subPath` ever arrived
+ as `""`, both fail. Route guard should ensure it can't, but "readability
+ first" still prefers a total version:
+ ```haskell
+ case T.splitOn "/" subPath of
+ [] -> "" -- impossible, but total
+ [fileName] -> renderWithFile [] fileName
+ parts -> renderWithFile (init parts) (last parts)
+ ```
+
+## Naming
+
+- **`app/Main.hs:50-53`** ��� `Opts` is used only for parsed CLI flags. Rename
+ to `CliOpts` / `Flags` / `CliArgs` to avoid collision with
+ `Web.Scotty.Options` which is imported on the same file.
+
+- **`app/Main.hs:96-100`** ��� Local bindings use abbreviated `cfgMap`-
+ extracted names: `port`, `bind`, `repoDir`, `title`, `staticDir`.
+ Good. But the record later uses `cfgPort`, `cfgBind`, `cfgRepoDir`,
+ `cfgTitle`, `cfgStaticDir` and the `mkConfig` parameter list on line 122
+ repeats the positional order. If one day the record gets a new field,
+ `mkConfig`'s 6-argument positional call site is a silent refactor hazard.
+ Prefer record syntax at the call site:
+ ```haskell
+ let cfg = DarcsWebConfig { cfgPort = port, cfgBind = bind, ... cfgLog = logf }
+ ```
+ and delete `mkConfig` entirely.
+
+- **`app/Main.hs:113,116`** ��� `logf` locally and `cfgLog` on the record.
+ Pick one (`logf` throughout is fine).
+
+- **`app/Main.hs:451`** ��� `hdrs` vs. `requestHeaders`. Project is generally
+ consistent on full names ��� `hdrs` is the one outlier.
+
+- **`src/DarcsWeb/Clone.hs:37,40,41`** ��� `mProto`, `mHost`, `name`, then
+ locally `proto`, `host`. Fine, but `host`/`mHost` shadowing is confusing:
+ rename the local to `finalHost` or `hostOrDefault`.
+
+- **`src/DarcsWeb/Clone.hs:63,64`** ��� `raw` / `h` / `h` / `p` single-letter
+ locals. `h` for both "host-like string" in `validRegName` and "name"
+ inside the where clause is reused. Consider `host`, `port`.
+
+- **`src/DarcsWeb/Darcs.hs:107`** ��� `getBasicRepoInfo` returns
+ `Maybe (Text, Int)` ��� an anonymous tuple. A record (`data RepoStats =
+ RepoStats { rsLastChange :: !Text, rsPatchCount :: !Int }`) would
+ document what `fst`/`snd` mean at the call site (line 90).
+
+- **`src/DarcsWeb/Darcs.hs:253-255,263-265`** ��� `extractPatchListing` vs.
+ `extractPatchFull`. "Listing" vs. "Full" is fine; better names could be
+ `summariseWithoutDiff` / `summariseWithDiff`, or see the `DiffMode`
+ refactor above.
+
+- **`src/DarcsWeb/Html.hs:405-409`** ��� `navLink'` has a trailing prime
+ with no un-primed counterpart. Drop the prime: `navLink`.
+
+- **`src/DarcsWeb/Html.hs:84`** ��� `"num"` CSS class is fine, but `T.pack
+ (show (riPatchCount ri))` appears three times in this module with
+ slightly different spelling. A one-liner helper `tshow = T.pack . show`
+ would cut noise.
+
+## Structure
+
+- **`app/Main.hs:217-342`** ��� `app` is ~125 lines of inline route
+ definitions. Each route repeats the shape `pathParam ���; withRepo cfg
+ name $ \repoPath -> do ���`. Consider:
+ 1. Extract each handler (`handleSummary`, `handleShortlog`, `handleLog`,
+ `handleTags`, `handleTree`, `handleBlob`, `handlePatch`) to a named
+ top-level function with a type signature. `app` then becomes a
+ one-screen route table.
+ 2. Collapse the common "lookup optional value + 404-or-render" pattern
+ (lines 290-295, 304-309, 318-323, 331-337) into a helper:
+ ```haskell
+ renderOr404 :: Text -> Maybe a -> (a -> Text) -> ActionM ()
+ renderOr404 msg mVal render = case mVal of
+ Nothing -> respond404 msg
+ Just x -> html (TL.fromStrict (render x))
+ ```
+
+- **`app/Main.hs:6-35`** ��� The import list for Main spans 30 lines and
+ mixes `Web.Scotty`, `Network.Wai`, POSIX, FFI, `Data.Text.*`, `System.*`.
+ Grouping already helps; adding a blank line between `System.Posix.*`
+ and `Foreign.*` would signal "FFI section" and a comment
+ `-- Daemonization (POSIX + FFI)` on line 28 would frame why the FFI is
+ there.
+
+- **`app/Main.hs:169-176`** ��� `openDevNull` uses the raw `2` for `O_RDWR`.
+ Either import from `System.Posix.IO` (`OpenMode(..)`) or at least name
+ the constant: `let o_RDWR = 2 in ���`. Also, a comment already explains
+ *why* (unix-package compat); move it to a Haddock on `openDevNull`
+ itself so it sticks to the function.
+
+- **`src/DarcsWeb/Html.hs`** ��� At 468 lines, this module mixes:
+ - page chrome (`renderPage`, `repoBreadcrumb`, `repoNavBar`, `navLink'`)
+ - list renderers (`renderRepoList`, `renderRepoTable`, `renderRepoRow`,
+ `renderShortLogTable`/`Row`, `renderTagList`/`Row`)
+ - entity pages (`renderRepoSummary`, `renderPatchDetail`, `renderTree`,
+ `renderBlob`, `render404`)
+ - text utilities (`esc`, `highlightDiff`, `shortAuthor`, `formatSize`,
+ `formatAbsolute`, `relativeDate`, `parseDarcsDate`, `pluralize`)
+
+ Splitting into `DarcsWeb.Html.Page`, `DarcsWeb.Html.Repo`,
+ `DarcsWeb.Html.Patch`, `DarcsWeb.Html.Format` would let a reader
+ navigate by responsibility. If a split is too invasive, at minimum
+ add banner comments (`-- * Page chrome`, `-- * Log views`, ���) so
+ the TOC is visible when scrolling.
+
+- **`src/DarcsWeb/Darcs.hs:106-148`** ��� `getBasicRepoInfo`,
+ `getRepoPatches`, `getRepoPatch`, `getRepoTags` all share the exact
+ `try $ withRepositoryLocation YesUseCache repoPath $ RepoJob $ \r -> ���`
+ scaffolding plus a `case ��� of Left _ -> empty; Right v -> v` unwrap.
+ Extract:
+ ```haskell
+ withRepoSafe :: a -> FilePath
+ -> (forall p wR. RepoPatch p => Repository ��� -> IO a)
+ -> IO a
+ withRepoSafe fallback path job = do
+ r <- try $ withRepositoryLocation YesUseCache path (RepoJob job)
+ pure $ case r of Left (_ :: SomeException) -> fallback; Right v -> v
+ ```
+ The four call sites shrink to 3 lines each and the exception-handling
+ policy lives in one place.
+
+- **`src/DarcsWeb/Config.hs:33`** ��� `trim = reverse . dropWhile isSpace . reverse . dropWhile isSpace`
+ is a classic but surprising one-liner for non-Haskellers. `Data.Text`
+ has `T.strip` for `Text`; for `String` the equivalent one-liner is
+ acceptable but a `where` binding with a short comment would help
+ first-time readers.
+
+## Comments & docs
+
+- **`app/Main.hs:1-2`** ��� No module-level Haddock describing what Main does.
+ Three lines would help:
+ ```haskell
+ -- | darcsweb executable: parses CLI flags, loads config, optionally
+ -- daemonizes via a double-fork, and starts a Scotty server that
+ -- serves repository pages and read-only clone files.
+ ```
+
+- **`app/Main.hs:150-159`** ��� `daemonize` has a one-line comment but no
+ explanation of *why* the double-fork is needed (SysV-style disown from
+ controlling terminal). One line would save the next maintainer a
+ Stack Overflow detour:
+ ```haskell
+ -- | Double-fork: first fork + setsid() detaches from the controlling
+ -- terminal; second fork ensures we are not a session leader, so we
+ -- can never re-acquire a TTY.
+ ```
+
+- **`app/Main.hs:408-411`** ��� The Haddock says "uses formally verified
+ implementation from Coq/Rocq" ��� good pointer, but a link (or mention
+ of `verified/Path.v`) would make the trail explicit.
+
+- **`src/DarcsWeb/Clone.hs:3-31`** ��� Excellent Haddock on `buildCloneUrl`.
+ `sanitizeHost`, `validAuthority`, `validBracketed`, `validRegName` would
+ benefit from matching `@since`/spec-style annotations noting RFC 3986
+ sections they partially implement.
+
+- **`src/DarcsWeb/Darcs.hs:174-181`** ��� `jailCheck` has a comment but no
+ Haddock. Since this function is a critical security boundary (reused
+ in three places), it should be documented properly:
+ ```haskell
+ -- | Return 'True' iff 'path', after symlink resolution, is either the
+ -- 'jail' directory itself or a strict descendant of it. Used to
+ -- prevent path traversal through symlinks.
+ ```
+
+- **`src/DarcsWeb/Html.hs:298-308,367-381`** ��� No Haddock on breadcrumb
+ helpers. Since the two are near-duplicates, docs should either explain
+ the difference or (preferable) factor them.
+
+- **`test/Properties/Clone.hs`**, **`test/Properties/Csp.hs`**,
+ **`test/Properties/Html.hs`** ��� Tests are consistently formatted and
+ readable. The `run`/`quickCheckWithResult` boilerplate is duplicated
+ across all three ��� worth extracting to a shared `Properties.Runner`
+ module with one `runProp :: Testable p => String -> p -> IO Bool`.
+
+## Small wins
+
+Ordered by impact:
+
+1. **`respond404` helper in `app/Main.hs`.** Kills ~13 duplicated 2-line
+ blocks (`app/Main.hs:242-243, 292-293, 306-307, 320-321, 333-334, 341-342,
+ 358-359, 368-369, 374-375, 389-390, 396-397, 405-406, 438-440`).
+2. **Replace `findRepo` body with `find`** (`app/Main.hs:414`).
+3. **Replace `maybe X id` with `fromMaybe X`** (`src/DarcsWeb/Html.hs:274`).
+4. **Replace `foldl (flip id)` with `foldr ($)`** (`app/Main.hs:74`).
+5. **Drop `mkConfig`; use record syntax inline** (`app/Main.hs:113, 119, 122`).
+6. **Top-level type signatures on inner `where` helpers** ���
+ `app/Main.hs:453` (`eitherToMaybe`), `src/DarcsWeb/Html.hs:444`
+ (`pluralize`). They're short but the types aren't obvious.
+7. **Guards instead of nested `if/else`** ��� `src/DarcsWeb/Html.hs:431-445`,
+ `app/Main.hs:354-359`, `src/DarcsWeb/Darcs.hs:196-213`.
+8. **Blank-line-grouped imports** with `-- Scotty`, `-- WAI`,
+ `-- POSIX / FFI`, `-- Local modules` section comments
+ (`app/Main.hs:6-43`).
+9. **`tshow = T.pack . show`** in `src/DarcsWeb/Html.hs` ��� removes 6
+ noisy repetitions (`Html.hs:84, 349, 350, 351, 445`).
+10. **Rename `navLink'` to `navLink`** (`src/DarcsWeb/Html.hs:405`) ��� no
+ un-primed sibling, the tick carries no information.
+11. **`darcsDir` is only used twice in Main** (`app/Main.hs:47, 423`). Fine
+ to keep, but add the same constant in `src/DarcsWeb/Darcs.hs` rather
+ than the bare string `"_darcs"` (appears at lines 60, 100, 209).
+ Either push `darcsDir` into `Types.hs` or introduce
+ `DarcsWeb.Internal.Constants`.
+12. **`Data.List (isPrefixOf)`** is imported in `app/Main.hs:35` and used
+ once (line 367). Paired with the similar import in `Darcs.hs:23` and
+ the verified `PathPure.add_trailing_slash` call, a single
+ `pathIsInside :: FilePath -> FilePath -> Bool` helper (in `PathPure`
+ or in a thin wrapper) would be clearer and testable.
+
+## Extracted code note
+
+Files `gen/CspPure.hs`, `gen/HtmlPure.hs`, `gen/PathPure.hs` are Coq
+extraction output. They exhibit the usual extraction accents:
+
+- **`gen/CspPure.hs:9-16`** ��� Hand-rolled `fst`/`snd` shadowing `Prelude`.
+ Extraction-generated; harmless because `{-# OPTIONS_GHC -w #-}` silences
+ the shadow warning. **Un-editable at source**. If the Coq side can
+ map its tuple projections to the standard ones (via
+ `Extract Constant fst => "Prelude.fst".`), the extracted file would
+ read better. Same applies to `map` on line 18-22.
+- **`gen/CspPure.hs:29-31`**, **`gen/HtmlPure.hs:18-35`**,
+ **`gen/PathPure.hs:19-21, 44-46, 79-89`** ��� Every character comparison
+ is annotated with
+ `((Prelude.==) :: Prelude.Char -> Prelude.Char -> Prelude.Bool)`. This
+ is an extraction artefact (Coq's decidable equality realisation). A
+ Coq-side `Extract Inductive bool => "Prelude.Bool" ���` and
+ `Extract Inlined Constant Char.eqb => "(Prelude.==)"` would remove the
+ inline type ascriptions and drop ~30% of the visual noise. **Un-editable
+ at source**.
+- **`gen/HtmlPure.hs:9-10`**, **`gen/HtmlPure.hs:13-14`** ��� Identity lambdas
+ `(\x -> x) '"'` are obfuscated literals. Extraction-generated; in Coq,
+ defining `Definition dquote := "034"%char` (or simply inlining the
+ literal) and marking it `Extract Constant dquote => "'\"'"` would
+ produce readable Haskell. **Un-editable at source**.
+- **`gen/PathPure.hs:70-90`** ��� `is_safe_segment` is a four-deep nested
+ `(Prelude.&&)` tower. This is the natural result of extracting a
+ conjunction of `bool` predicates. If the Coq definition switched to a
+ `forallb` over a list of unary predicates (or used `&&&`-style local
+ notation), the extract would read better. **Un-editable at source**.
+- **`gen/PathPure.hs:1-3`**, **`gen/HtmlPure.hs:1-3`**,
+ **`gen/CspPure.hs:1-3`** ��� All three extracts import `Data.Bits` and
+ `Data.Char` unconditionally, though none reference them. Extraction
+ artefact. **Un-editable at source**; can only be suppressed via
+ `-Wno-unused-imports` (already implicit under `-w`).
+
+**Wrapper-side improvements (editable):** the thin Haskell wrappers in
+`src/DarcsWeb/Html.hs:414`, `src/DarcsWeb/Darcs.hs:186, 191`,
+`app/Main.hs:411`, `app/Main.hs:195` are all one-liners that call through
+to the extracted `HtmlPure` / `PathPure` / `CspPure` module. They are the
+right place to add Haddock with a link to the corresponding `.v` proof
+file (e.g. `-- See @verified/Html.v@ for the proof that this is
+HTML-injection safe.`) so the reader immediately knows where to look for
+the semantic guarantee instead of having to pattern-match the mangled
+extracted source.
addfile ./reviews/claude-security.md
hunk ./reviews/claude-security.md 1
+# Security Review ��� darcsweb
+
+## Summary
+
+Overall posture is reasonably careful: the jail checks canonicalize paths,
+CSP / security headers are applied by middleware, and the high-risk string
+functions (`esc`, `is_safe_sub_path`, `build_csp`, `add_trailing_slash`) are
+extracted from Coq so the hot path is formally verified. However, the tree
+endpoint bypasses the verified `isSafeSubPath` predicate and therefore
+exposes the whole `_darcs/` directory through the normal browsing UI, which
+is the single biggest concrete exposure. Several partial functions (`read`,
+`init`, `last`, `head`), un-caught `canonicalizePath` exceptions, and lazy
+`readFile` in the blob endpoint give an outside attacker multiple ways to
+turn crafted URLs / headers into 500s or lingering file handles. The
+verification boundary is drawn correctly for the three `gen/` modules but
+does not cover `encodePathSegment`, `sanitizeHost`, or the whole IO layer;
+those rely on QuickCheck only and the property sets have gaps.
+
+## Critical findings
+
+### C1. `_darcs/` internals reachable through `/repo/:name/tree/_darcs` (path-traversal-equivalent via missing safety predicate)
+
+- **Location:** `src/DarcsWeb/Darcs.hs:196-213` (`getRepoTree`), `app/Main.hs:298-309` (tree subdirectory route).
+- **Why it is dangerous:** `getRepoBlob` (`src/DarcsWeb/Darcs.hs:232`) correctly rejects unsafe sub-paths with `isSafeSubPath`, but `getRepoTree` does **not** call `isSafeSubPath` at all. The regex route `"^/repo/([^/]+)/tree/(.+)$"` accepts any `subPath`, including `_darcs`, `.git`, `..`, `.ssh`, etc. `jailCheck` only verifies the canonical path stays inside `repoPath`, which is true for `repoPath </> "_darcs"`. The visibility filter at `src/DarcsWeb/Darcs.hs:209` only strips `_darcs` / `.git` / dot-files from the *listing of children*, not from the directory that is being listed, so the attacker can list `_darcs`'s own children (e.g. `patches/`, `inventories/`, `hashed_inventory`) and navigate deeper.
+- **Exploit scenario:** `GET /repo/foo/tree/_darcs` returns a directory listing of `patches`, `inventories`, `pristine.hashed`, `hashed_inventory`, `prefs`, etc. The rendered `<a>` links plus user-supplied URLs like `/repo/foo/tree/_darcs/prefs` and `/repo/foo/blob/_darcs/prefs/author` then leak internal state. Even though `getRepoBlob` is protected, the tree endpoint alone discloses internal file names, sizes, and existence (a reconnaissance oracle). `GET /repo/foo/tree/.git` similarly exposes any accidentally-committed `.git/` directory.
+- **Fix:** In `getRepoTree`, reject before doing anything else:
+ ```haskell
+ getRepoTree repoPath subPath
+ | not (null subPath) && not (isSafeSubPath subPath) = return Nothing
+ | otherwise = ...
+ ```
+ (Empty `subPath` is the root listing and must stay allowed; `isSafeSubPath ""` returns False by design.) Add a QuickCheck property in `test/Properties/Clone.hs` that `getRepoTree` rejects `"_darcs"`, `".git"`, `".."`, `".hidden"`, and any `a/../b`. Also harden the existing root-listing filter to skip entries whose name fails `is_safe_segment`, so that a future bug reintroducing the hole still doesn't let hidden files become clickable.
+
+## High findings
+
+### H1. `read` on the config port is a partial function and crashes the process on any non-numeric value
+
+- **Location:** `app/Main.hs:96` ��� `port = read (cfgLookupDefault "port" "3000" cfgMap)`.
+- **Why it is dangerous:** `Text.Read.read` throws a bottom (`Prelude.error "no parse"`) on any unparseable input. An operator mistyping `port = 3000 # production` (trailing comment) or `port = 3000\r` (stray CR from Windows line endings is stripped by `lines`, so mostly OK, but `port = 8080 ` with trailing garbage, or hex, or empty value `port =`) will take the server down at boot with an error thrown from pure code. The error is not logged through syslog because it happens before `withSyslog`, so in daemon mode it is silently invisible.
+- **Exploit scenario:** Not directly remotely exploitable, but a supply-chain attacker with write access to the config file gets an unrecoverable crash-loop (reliability attack). More importantly, the same pattern will likely be copied for future options, and once an attacker-controlled config value enters via any channel (e.g. an admin UI, env var) it becomes an unauthenticated crash.
+- **Fix:** Replace `read` with `Text.Read.readMaybe` and fail cleanly:
+ ```haskell
+ import Text.Read (readMaybe)
+ port <- case readMaybe (cfgLookupDefault "port" "3000" cfgMap) of
+ Just p | p > 0 && p < 65536 -> return (p :: Int)
+ _ -> hPutStrLn stderr "Invalid 'port' in config" >> exitFailure
+ ```
+ Apply the same pattern to every `read` call added later. Consider enabling `-Wincomplete-uni-patterns` and banning `read` project-wide with a `hlint` rule.
+
+### H2. Blob endpoint uses lazy `Prelude.readFile` + locale encoding on untrusted content up to 10 MiB
+
+- **Location:** `src/DarcsWeb/Darcs.hs:249` ��� `txt <- T.pack <$> Prelude.readFile fullPath`.
+- **Why it is dangerous:** Three problems stacked:
+ 1. `Prelude.readFile` is lazy: it returns a `String` backed by a still-open file handle. If the HTML renderer short-circuits (exception in `esc`, connection drop mid-response), the handle leaks until the next GC. Repeated requests can exhaust file descriptors.
+ 2. `readFile` decodes using the process locale, not UTF-8. A non-ASCII file is silently mangled depending on `$LC_ALL`, which both corrupts the rendered page and can change byte length unpredictably ��� relevant because `getFileSize` is checked against `maxBlobSize` in bytes, but `T.pack` counts Unicode code points, so the limit is not a true cap on the resulting `Text` memory after decoding.
+ 3. Binary files (images, tarballs) get HTML-escaped character-by-character through the Coq-extracted `HtmlPure.esc` ��� which operates on `[Char]` ��� producing a huge transient `String` and a large HTML page (a 10 MiB binary blob can easily render to 40���60 MiB of HTML). An attacker that can commit a 10 MiB binary file and then triggers N parallel `/blob/...` requests can exhaust memory.
+- **Exploit scenario:** Commit a 10 MiB file full of `<` characters (each expands to `<`, 4�� growth). Fire `curl` in parallel against `/repo/x/blob/big`. Each in-flight request allocates ~40 MiB of `String` plus a `Text` copy. The process OOMs long before the per-request cap is reached. Separately, a file containing a sequence of bytes invalid in the current locale triggers `hGetContents`'s decoder to throw an `IOError` from within the lazy stream, which escapes `getRepoBlob` as an unhandled exception because the `try` wrapper is only around the darcs calls, not around `readFile`.
+- **Fix:**
+ 1. Switch to strict byte-level IO: `BS.readFile` + explicit UTF-8 validation via `TE.decodeUtf8'`. Reject non-UTF-8 blobs with a "binary file, cannot display" placeholder.
+ 2. Detect binary content (e.g. any NUL byte in the first 8 KiB) and refuse to render.
+ 3. Wrap the read in `try :: IO (Either SomeException Text)` and return `Nothing` on failure.
+ 4. Enforce the size cap *after* decoding, or in terms of `Text.length`, to prevent locale-based amplification.
+
+### H3. `canonicalizePath` exceptions escape `IO` and surface as 500 responses / HTML-in-plain-text leaks
+
+- **Location:** `app/Main.hs:365` (serveStatic), `app/Main.hs:400` (withRepo), `app/Main.hs:425` (serveClone), `src/DarcsWeb/Darcs.hs:178` (jailCheck).
+- **Why it is dangerous:** `System.Directory.canonicalizePath` is documented to throw `IOError` on permission denied, invalid path bytes (NUL), loop in symlinks, and other non-`ENOENT` failures. None of the call sites are wrapped in `try`, so the exception propagates to Scotty's default error handler, which returns a 500 containing the exception's `show` (leaks server paths to the client and bypasses the `render404` path which would have applied the CSP via middleware ��� actually middleware still applies, but the body is an exception message).
+- **Exploit scenario:**
+ 1. NUL injection: if `T.unpack name` contains a NUL (not rejected by `withRepo` ��� see also M3), `candidate` contains NUL, and the `isDarcsRepo` / `canonicalizePath` chain throws `InvalidArgument` from the FFI layer. Attacker controls the URL path, gets a stack-trace-like 500.
+ 2. Symlink loop: an operator-accidentally-created symlink loop inside `cfgRepoDir` makes every request for that repo throw `too many levels of symbolic links`.
+ 3. Permission denied: a directory inside the repo tree with mode 0000 returns a permission error during the tree walk, leaking disclosure of its existence via the error message.
+- **Fix:** Add a small helper:
+ ```haskell
+ safeCanonicalize :: FilePath -> IO (Maybe FilePath)
+ safeCanonicalize p = either (\(_ :: SomeException) -> Nothing) Just <$> try (canonicalizePath p)
+ ```
+ and route all canonicalization calls through it, treating `Nothing` as "404 Not Found". Also wrap `doesFileExist` / `doesDirectoryExist` / `listDirectory` the same way ��� they can also throw on permission errors.
+
+### H4. `renderBlobBreadcrumb` uses `init` / `last` on a `splitOn` result ��� crashes on empty `subPath`
+
+- **Location:** `src/DarcsWeb/Html.hs:369-371` ��� `let parts = T.splitOn "/" subPath; dirParts = init parts; fileName = last parts`.
+- **Why it is dangerous:** Both `init` and `last` are partial on `[]`. The blob endpoint is guarded upstream by a `regex "^/repo/([^/]+)/blob/(.+)$"` which requires at least one character, so the current caller always supplies a non-empty string and `T.splitOn "/" nonEmpty` returns a non-empty list. But this is a tight coupling between a route pattern and a render function two modules away; adding an `/repo/:name/blob` alias, or calling `renderBlob` from a future feature with an empty path, crashes the rendering thread. The CSP middleware already runs, so the client would see a 500 with no body; the crash is logged only via syslog if you are in daemon mode.
+- **Exploit scenario:** A straightforward refactor adding `get "/repo/:name/blob" ...` would silently introduce a crash reachable by `GET /repo/foo/blob`. Also, if `T.splitOn` semantics change in a future `text` release (the cabal bound allows up to `text < 3`), the invariants no longer hold.
+- **Fix:** Make the function total:
+ ```haskell
+ renderBlobBreadcrumb repoName subPath =
+ case reverse (T.splitOn "/" subPath) of
+ [] -> renderRootOnly repoName
+ [fileName] -> renderRootOnly repoName <> " / " <> esc fileName
+ (fileName:revDirs) -> ...
+ ```
+ Or require `subPath` to be non-empty by construction via a newtype. Enable `-Wincomplete-uni-patterns` / `-Wmissing-methods` to catch future regressions.
+
+## Medium / Low findings
+
+### M1. `withRepo` does not reject NUL bytes, control characters, or Windows-reserved names in repo names
+
+- **Location:** `app/Main.hs:385-406`. The guard is `T.any (== '/') name || T.any (== '\\') name || ".." `T.isInfixOf` name || T.isPrefixOf "." name`.
+- **Risk:** A repo name of `foo\0bar` passes the guard, is concatenated with `</>`, and the C-based `doesDirectoryExist` truncates at NUL on POSIX (then throws, see H3). A name containing `\n` survives into log messages and HTML output (the `esc` function only escapes `<>&"'` ��� a newline in a log line allows log-injection if an operator greps logs).
+- **Fix:** Require `T.all isSafeRepoChar name` where `isSafeRepoChar c = c > '\x20' && c /= '/' && c /= '\\' && c /= '\x7f'`. Also lower-bound length ��� 1 and upper-bound (e.g. 255). Consider a single verified predicate `is_safe_repo_name` and add a Coq lemma that every character is printable ASCII.
+
+### M2. `serveStatic` treats a URL-decoded path that resolves *to* the jail directory itself as "inside" because of the trailing-slash check, but `doesFileExist` on a directory returns False ��� benign, but the logic is duplicated
+
+- **Location:** `app/Main.hs:366-380`. The jail comparison `jailDir `isPrefixOf` canonical` (where `jailDir = addTrailingSlash (cfgStaticDir cfg)`) can *never* accept `canonical == cfgStaticDir cfg` because the canonical path for a file inside the dir always has the trailing slash absent. That's fine ��� but the same logic in `withRepo` (`app/Main.hs:401`) applies the same check to the repo directory, and there the `canonical` for the repo IS just `cfgRepoDir/<name>` with no trailing slash ��� also fine. The inconsistency with `jailCheck` in `src/DarcsWeb/Darcs.hs:178-181` which additionally accepts `canonical == init jailSlash` is a code-smell. Consolidate on a single verified helper (see `PathPure.add_trailing_slash` ��� add a `PathPure.is_inside : String -> String -> Bool` with a proof).
+
+### M3. `Prelude.readFile` is used in `Config.parseConfigFile` (lazy + locale-sensitive)
+
+- **Location:** `src/DarcsWeb/Config.hs:20`.
+- **Risk:** Startup-only, so low risk, but if the config path is a FIFO or a slow NFS mount, startup hangs silently. Also the config parser does `break (== '=')` without supporting quoted strings, so a value containing `=` is truncated at the first one (`parseLine` in `src/DarcsWeb/Config.hs:29` ��� documented behavior, but not obvious to operators).
+- **Fix:** Use `withFile`/`hGetContents'` (strict) or `System.IO.readFile'` (if base ��� 4.15), which avoids the lazy-handle hazard. For the grammar, document the limitation in the config file example or switch to a real parser.
+
+### M4. Integer overflow on `getFileSize` ��� `Int` on 32-bit targets
+
+- **Location:** `src/DarcsWeb/Darcs.hs:219` ��� `size <- if isDir then return 0 else fromIntegral <$> getFileSize path`.
+- **Risk:** `getFileSize` returns `Integer`, and `teSize :: Int`. On a 32-bit build (`base >= 4.12` does not require 64-bit), a 5 GiB file silently becomes a wrong value (including negative). Not exploitable on the usual 64-bit deployment target, but the cabal dependency bounds do not force 64-bit.
+- **Fix:** Either widen `teSize` to `Int64` / `Integer`, or clamp: `size = min (fromIntegral (maxBound :: Int)) (fromIntegral rawSize)`. Also note `maxBlobSize :: Integer` is compared with `getFileSize` (Integer) directly ��� that part is correct, but the `Int`-valued `teSize` is the hole.
+
+### M5. Lazy IO in `readRepoDescription`
+
+- **Location:** `src/DarcsWeb/Darcs.hs:99-104` ��� `T.strip . T.pack <$> readFile descFile`.
+- **Risk:** Same class as H2 (lazy handle, locale decoding). Called once per repo per request for the index page, so a malicious description file full of invalid-UTF-8 bytes can throw mid-render and 500 the index. Easy DoS against the root page.
+- **Fix:** `BS.readFile` + `TE.decodeUtf8With T.lenientDecode`. Also cap at e.g. 16 KiB.
+
+### M6. `listRepos` / `getRepoTree` do not bound the number of entries
+
+- **Location:** `src/DarcsWeb/Darcs.hs:63-67`, `src/DarcsWeb/Darcs.hs:208-213`.
+- **Risk:** An operator who points `cfgRepoDir` at a directory with millions of subdirectories (accidental or adversarial) makes every index-page request iterate them all. `getRepoTree` has the same shape at repo-subdirectory level.
+- **Fix:** Add a cap (e.g. 10 000 entries) analogous to `maxPatchCount`, and render "N more entries, listing truncated" if exceeded.
+
+### M7. `encodePathSegment` and `sanitizeHost` are not formally verified
+
+- **Location:** `src/DarcsWeb/Clone.hs:63-133`.
+- **Risk:** The percent-encoder and host validator are security-critical (they are the reason a crafted `Host` header cannot inject a path or protocol). They have good QuickCheck coverage (`test/Properties/Clone.hs:58-116`), but not the same assurance as `PathPure`. In particular, `encodePathSegment` implements UTF-8 re-encoding by hand (`Data.Bits` / `shiftR` / hex table) ��� a prime target for an off-by-one that accidentally emits a literal `/`. The QuickCheck prop `prop_clone_encoded_has_no_reserved` only tests a handful of reserved characters.
+- **Fix:** Either (a) replace with `Network.HTTP.Types.URI.encodePathSegments` from `http-types` (already a dependency) ��� this is the standard, battle-tested encoder ��� or (b) port the logic into Coq (`ClonePure`) with a proof that for every non-unreserved byte the output has the shape `%HH` and that no output character is in the reserved set.
+
+### M8. CSP builder strips only `;` ��� the directive *name* is not sanitized
+
+- **Location:** `gen/CspPure.hs:37-39` (`build_directive`), all call sites in `app/Main.hs:195-202`.
+- **Risk:** Today all directive names are hard-coded string literals in `Main.hs`, so there is no live risk. But `build_directive` applies `sanitize_value` only to the value half, so a future contributor who adds `("script-src", userControlled)` ��� or worse, `(userControlled, value)` ��� gets a CSP-injection foot-gun. The API shape makes the insecure path the default.
+- **Fix:** Change the type signature so directive names are an enum / newtype with a finite set of allowed values, or sanitize names the same way as values (strip `;`, whitespace, commas, newlines). Add a Coq proof that for any (name, value) pair the output contains at most `(length directives - 1)` semicolons.
+
+### M9. Lazy `T.lines` in `highlightDiff` drops trailing blank lines (correctness, not security) and rebuilds the diff with O(n��) `T.concat` of small chunks
+
+- **Location:** `src/DarcsWeb/Html.hs:418-425`.
+- **Risk:** Diff output with a trailing newline loses it in the rendered page. Combined with H2's 10 MiB blob risk, a 10 MiB patch with 10��� lines becomes 10��� `Text` fragments; `T.concat` is linear in total length but allocates per-fragment. Medium DoS.
+- **Fix:** Use a `Builder` (`Data.Text.Lazy.Builder`) or stream directly into the response. Pair with a per-patch size cap.
+
+### M10. `parseDarcsDate` silently accepts only 14-digit strings; `formatAbsolute` produces garbage for shorter / non-digit input
+
+- **Location:** `src/DarcsWeb/Html.hs:431-458`.
+- **Risk:** Dates not in `YYYYMMDDHHMMSS` format are returned verbatim from `relativeDate`, then HTML-escaped, so there's no XSS risk. But `formatAbsolute` on a non-digit 14-character string returns something like `alph-ab-et -ic:al:ly` (garbage), which then ends up in the page. Cosmetic / robustness. Consider validating that the input is 14 digits before splitting.
+
+### M11. The daemonization routine does not `chdir("/")`, does not `umask(0)`, and does not close inherited fds beyond 0/1/2
+
+- **Location:** `app/Main.hs:151-159`, `app/Main.hs:161-176`.
+- **Risk:** Low. A daemon started from a CWD that is later removed keeps that directory busy. Open fds inherited from the parent (stderr from the shell, the config file handle) remain open.
+- **Fix:** After the second fork, do `setCurrentDirectory "/"` (from `System.Directory`), set umask to `0o077`, and close every fd `> 2` (portably via `/proc/self/fd` on Linux, or `closefrom(3)`). Also add `O_CLOEXEC` (`0x80000` on Linux) to the `c_open` flags in `openDevNull` to prevent leaking `/dev/null` to children, and `close` the original `nullFd` after dupping.
+
+### M12. No rate limiting, no request-size limit, no graceful handling of slow clients
+
+- **Location:** `app/Main.hs:133-139`.
+- **Risk:** Expected trade-off for a read-only mirror behind Caddy, but the code relies on Caddy providing this. If the README's recommended deployment changes, or someone exposes port 3000 directly (the `bind` setting accepts `0.0.0.0`!), the server is trivially slow-loris-able and memory-exhaustible.
+- **Fix:** Document the requirement explicitly, and/or set `setMaximumBodyFlush`, `setTimeout`, `setSlowlorisSize` on the Warp settings. Reject binding to `0.0.0.0` unless an explicit `--allow-public` flag is set.
+
+### L1. `openDevNull` hardcodes `O_RDWR = 2` and never closes `nullFd`
+
+- **Location:** `app/Main.hs:161-176`. Portable on Linux, but not on every POSIX variant. Missing `O_CLOEXEC`.
+
+### L2. `cfgLookupDefault` uses `String` keys and silently ignores unknown config keys
+
+- **Location:** `src/DarcsWeb/Config.hs:38-39`. Typos like `binnd = 0.0.0.0` fall through to the default with no warning.
+- **Fix:** Track the set of known keys at boot; warn about any unrecognized key.
+
+### L3. `findRepo` uses `foldr` with accumulator that overwrites earlier matches with later ones
+
+- **Location:** `app/Main.hs:413-414`. `foldr (\r acc -> if riName r == name then Just r else acc) Nothing repos` actually takes the *first* match (because `foldr`'s `acc` is the result of the tail), so behaviour is correct, but this is non-obvious. Prefer `Data.List.find` for clarity.
+
+### L4. `cspHeader` is recomputed never (CAF) but encoded via `BC.pack` from `String` ��� bytes are ISO-8859-1, not UTF-8
+
+- **Location:** `app/Main.hs:194-202`. Today all directive names/values are ASCII, so there is no observable difference. Worth a comment or a conversion via `TE.encodeUtf8 . T.pack` to be explicit.
+
+### L5. `logSyslog` uses `withCStringLen` which encodes via the current locale, not UTF-8
+
+- **Location:** `app/Main.hs:147-148`. Non-ASCII log messages (e.g. a repo name with an accented character appearing in a future log line) may be mangled in syslog. Use `withCAStringLen` + explicit UTF-8 encoding.
+
+### L6. `head stripped` in `Config.meaningful` is partial but guarded
+
+- **Location:** `src/DarcsWeb/Config.hs:26`. Guard `not (null stripped)` is syntactically adjacent ��� safe today ��� but future refactoring can separate them. Prefer `case stripped of (c:_) -> c /= '#'; [] -> False`.
+
+### L7. `splitPath` exported from `DarcsWeb.Darcs` is unused outside tests
+
+- **Location:** `src/DarcsWeb/Darcs.hs:189-191`. The cabal file already scopes it with a comment, but exposing unnecessary internals widens the attack surface for future misuse. Move behind a `DarcsWeb.Internal` module or use Haddock `#hide`.
+
+## Verification gaps
+
+1. **`getRepoTree` diverges from the verified path model.** `PathPure.is_safe_sub_path` guarantees rejection of `_darcs`, `.git`, `..`, hidden segments ��� but the Haskell caller only applies the predicate in `getRepoBlob`. Every IO-edge that maps a URL sub-path to a filesystem path MUST route through `isSafeSubPath`; add a lint rule / code review checklist. A natural formalization is a small wrapper type (e.g. `newtype SafeSubPath = SafeSubPath FilePath`) whose only constructor is `mkSafeSubPath :: FilePath -> Maybe SafeSubPath` using the verified predicate.
+
+2. **`CspPure.build_csp` is proven not to emit `;` inside a value, but not inside a *directive name*.** The test suite���s `prop_csp_no_value_semicolons` only checks values. Add a proof / property that `count ';' (build_csp ds) <= length ds - 1`.
+
+3. **`sanitizeHost`, `validAuthority`, `encodePathSegment` are not in Coq.** They are the second-most-critical string transformations in the project (after `esc`). At minimum, add QuickCheck properties stating:
+ - `validAuthority h ==> all (`elem` allowedAuthorityChars) h` (currently implicit via structural check);
+ - `encodePathSegment t` contains no character in `"/?#& \t\r\n"`;
+ - `encodePathSegment t` is idempotent modulo case of the hex digits when fed ASCII-only input of unreserved chars.
+ Long term, port them to Coq alongside `PathPure`.
+
+4. **No property exercises the end-to-end IO layer.** All QuickCheck tests target the pure modules. There is no test that (a) sets up a temp directory, (b) throws symlinks / hidden files at `serveStatic` / `withRepo` / `serveClone` / `getRepoTree` / `getRepoBlob`, and asserts the 404 / 403 / content outcome. These functions are exactly where the verified pieces get wired into IO; the coverage gap is precisely the one that lets C1 and H3 exist.
+ - Recommended properties:
+ - `forall p in generator(unsafe), getRepoTree repoRoot p == Nothing`
+ - `forall symlink escaping repoRoot, getRepoBlob repoRoot symlink == Nothing`
+ - `forall config without "port", main exits cleanly with a diagnostic` (property over `parseOpts` + port parsing)
+ - `buildCloneUrl` + `withRepo` round-trip: the clone URL for a validated repo name always percent-decodes back to that same name.
+
+5. **`parseConfigFile` is not modeled at all.** It accepts arbitrary UTF-8 and could benefit from a tiny parser spec + QuickCheck.
+
+## Hardening recommendations
+
+### Compiler flags (darcsweb.cabal)
+
+Replace `ghc-options: -Wall` in each stanza with:
+```
+ghc-options:
+ -Wall
+ -Wcompat
+ -Widentities
+ -Wincomplete-record-updates
+ -Wincomplete-uni-patterns
+ -Wpartial-fields
+ -Wredundant-constraints
+ -Wmissing-export-lists
+ -Wmissing-home-modules
+ -Wno-unused-do-bind
+ -fwarn-tabs
+```
+For the executable, additionally: `-rtsopts=-N -A64m -RTS` is fine, but consider capping heap: `-with-rtsopts="-N -M512M"` to convert OOM into a clean process death instead of host-memory exhaustion.
+
+### Dependency bounds
+
+- `base >= 4.12` admits GHC 8.6 (2018) which has known codegen bugs. Raise to `base >= 4.16` (GHC 9.2+, LTS).
+- `text >= 1.2 && < 3` spans a breaking change around `text-2.0` (UTF-8 by default). Pin to `text >= 2.0 && < 3` and update code that assumes per-byte semantics.
+- `directory >= 1.3 && < 2` ��� OK, but document that `canonicalizePath` semantics changed in `directory-1.3.2`; require at least that version.
+
+### Safer defaults
+
+- Ban `read` and `readFile` in the codebase (hlint rule). Use `readMaybe`, `BS.readFile`, `TE.decodeUtf8'`.
+- Wrap every `canonicalizePath` / `doesFileExist` / `doesDirectoryExist` / `listDirectory` call in `try`; treat any exception as "not found".
+- Introduce newtype wrappers for security-relevant strings:
+ ```haskell
+ newtype SafeSubPath = SafeSubPath FilePath
+ newtype SafeRepoName = SafeRepoName Text
+ newtype JailedPath = JailedPath FilePath
+ ```
+ Only smart constructors (`mkSafeSubPath`, etc.) in their defining modules; every IO function that touches a filesystem path takes a newtype argument. This makes C1 a compile-time error.
+- Replace the ad-hoc bind-address default `"127.0.0.1"` with a type: `data BindAddr = Loopback | Public IPv4` and require a `--allow-public` flag to construct the second.
+- Move all extracted pure modules (`HtmlPure`, `PathPure`, `CspPure`) behind a consolidated `DarcsWeb.Verified` re-export with `Haddock` pointers to the corresponding `.v` file, and enforce via `cabal-fmt` / CI that no other module directly imports them ��� only through the wrappers that give them domain types.
+
+### Middleware / HTTP
+
+- Add `Strict-Transport-Security` (if you know TLS will be terminated upstream ��� safe to always send).
+- Add `Cross-Origin-Opener-Policy: same-origin` and `Cross-Origin-Resource-Policy: same-origin`.
+- Drop `X-Robots-Tag` to `noindex` only (the additional `nofollow` makes sense only on pages with outbound links you don't want followed; the rendered diff/log pages are a reasonable target for search engines on an intentionally-public mirror ��� re-evaluate).
+- Consider adding a `Cache-Control: private, max-age=60` for non-static endpoints, both for performance and to reduce load (DoS mitigation) ��� today every repo-list request re-walks the repo tree.
+
+### Tests
+
+- Add `test/Properties/Path.hs` covering `jailCheck`, `getRepoTree`, `getRepoBlob` against symlinked and hidden-file temp-dir fixtures.
+- Add a QuickCheck property that `GET /repo/:name/tree/:subpath` returns 404 whenever `isSafeSubPath subpath == False` ��� a simple integration test using `scotty` + `wai-extra`'s `runSession`.
+- Add a property that the index page is O(1) HTML output size regardless of the number of dot-files inside `cfgRepoDir` (uses M6 cap).
+- Require 100% branch coverage on `app/Main.hs`'s `withRepo`, `serveStatic`, `serveClone` (they are the security perimeter).
addfile ./reviews/claude-webperf.md
hunk ./reviews/claude-webperf.md 1
+# Mobile-First Web Performance Review ��� darcsweb
+
+## Summary
+The app gets the basics right: a viewport meta tag is set, a static stylesheet is served with a long cache window, and the CSS already has a mobile breakpoint. However, every HTML response is built by concatenating strict `Text` through `T.concat` and then round-tripped through `Data.Text.Lazy` to Scotty's `html` action, which defeats streaming and doubles allocations. HTML-escaping round-trips every byte through `String` (lazy `[Char]`) via the generated `HtmlPure.esc`, so on a 1 MiB diff view the CPU cost dominates TTFB. On mobile, the diff viewer forces horizontal scroll on long lines, tap targets (action links in repo/shortlog rows) are well under the 44 px minimum, and the semantic markup is almost entirely `div` + `table` with a single `<nav>`. Fixing escaping, adding `Cache-Control`/`ETag` on HTML responses, and tightening the tap targets would be the highest-leverage wins.
+
+## Mobile rendering issues
+- `src/DarcsWeb/Html.hs:85-88` ��� the "actions" links on the repo list (`log | tags`) are inline text links with `padding: var(--space-1) var(--space-2)` (static/style.css:370-375), which is roughly 20 px �� 14 px. WCAG 2.5.5 / Apple HIG want ���44 px. Same problem on the shortlog row at `src/DarcsWeb/Html.hs:174-176` ("diff") and on the tag list row at `src/DarcsWeb/Html.hs:278-280` ("details"). On a 360 px viewport these links are essentially unmissable-by-thumb.
+- `src/DarcsWeb/Html.hs:241` ��� the patch diff is wrapped in a single `<pre class="diff">` with `white-space: pre` (static/style.css:631). Any line longer than the viewport triggers horizontal scroll of the diff block. On a phone, reviewing a patch with a path like `src/DarcsWeb/SomeLongModuleName.hs` or a 120-col source line means the user has to pan each line individually. Consider soft-wrapping with a toggle, or at least `overflow-wrap: anywhere` on `.diff-add`/`.diff-del` (the Haskell side should emit a container that CSS can target; today every line is a `<span class="diff-add">`, which is fine ��� the CSS, not the HTML, is the gap).
+- `src/DarcsWeb/Html.hs:93-123` ��� the summary page has `<h1>` ��� renderCloneBlock ��� `<h2>Recent Activity</h2>` ��� `<h2>Tags</h2>`, but the clone block has no heading of its own, so a screen reader jumps "summary ��� recent activity" with the clone URL floating in between. Add `<h2>Clone</h2>` above the clone box, or at least `aria-labelledby`.
+- `src/DarcsWeb/Html.hs:135` ��� the clone URL is a `<code>` with `user-select: all` (static/style.css:795). Mobile Safari and Chrome do not honor `user-select: all` as a one-tap select the way desktop does. On a phone the user has to long-press and drag the handles. Recommend rendering the URL inside `<input type="text" readonly value="���">` ��� it's one tap to focus, one "select all" on the context menu, and it keeps the existing copy-paste-without-shell-metachars invariant from the doc comment.
+- `src/DarcsWeb/Html.hs:194-214` ��� the full-log entry uses `<div class="log-header">` with a flex row containing name + date. CSS flips to `flex-direction: column` at ���768 px (style.css:899-902). That's fine, but on the server side the date string comes from `relativeDate` and can be "3 weeks ago" or "2026-04-21 08:12:34"; the column is not marked `<time datetime="���">`. Adding `<time datetime="2026-04-21T08:12:34Z">3 weeks ago</time>` would give screen readers and crawlers the machine-readable value "for free" and costs one small formatter. Same at `src/DarcsWeb/Html.hs:83`, `:165`, `:202`, `:230`, `:276`.
+- `src/DarcsWeb/Html.hs:290-308` ��� the tree breadcrumb is a single `<div class="tree-path">` with `white-space: nowrap; overflow-x: auto` (style.css:708). For a deeply nested path on mobile, the user scrolls a narrow sliver of text horizontally just to see where they are. A `flex-wrap: wrap` breadcrumb would be more finger-friendly; the Haskell side doesn't have to change, just the CSS ��� noting here because the HTML element the CSS keys on is this one.
+- `src/DarcsWeb/Html.hs:321` ��� the tree uses the raw UTF-8 byte sequence `\xf0\x9f\x93\x81` (����) as the icon. On a phone without a color emoji font (older Android WebView, some Kindle) this renders as tofu. Consider an inline SVG or a CSS pseudo-element with fallback. Low priority, but listed because it is a rendering bug class.
+- `src/DarcsWeb/Html.hs:361-364` ��� the blob view wraps file content in `<pre><code>` with `white-space: pre` (style.css:756). On mobile, a 120-column source line forces horizontal scroll of the entire pre block. At minimum allow a soft-wrap toggle; without it, any minified JS blob becomes unreadable on a phone.
+- `app/Main.hs:33-38` ��� there is no `<link rel="icon">` in `renderPage`. Every mobile browser will issue a `/favicon.ico` request and get a 404 HTML page back (`notFound` at `app/Main.hs:340`). The 404 body is a full styled page, adding request latency and wasted bytes. Either serve `/favicon.ico` as a real static asset (already have `/static/darcs-logo.png`) or emit `<link rel="icon" href="/static/darcs-logo.png">` in `renderPage`.
+- `src/DarcsWeb/Html.hs:35` ��� the viewport is `width=device-width, initial-scale=1`. Good. But consider also `viewport-fit=cover` so the header doesn't collide with the notch on iPhones. Minor.
+- `src/DarcsWeb/Html.hs:40-42` ��� the header logo is a PNG served at a fixed `height: 32px` (26 px on mobile). Without `width=` / `height=` attributes on the `<img>`, the browser reserves no box during layout, so every page has a CLS (cumulative layout shift) jolt. Add explicit `width` / `height` attributes; the intrinsic image aspect ratio is probably known.
+- `src/DarcsWeb/Html.hs:41` ��� `alt="darcs"` on the logo is fine, but the `<a>` around it already has `aria-label="darcs home"`, which means assistive tech announces "darcs home, darcs" ��� double announcement. Use `alt=""` on the image when the link has an aria-label.
+
+## Performance issues
+- `src/DarcsWeb/Html.hs:413-414` ��� `esc = T.pack . HtmlPure.esc . T.unpack`. Every HTML-escape unpacks `Text` into `String` (lazy `[Char]`), walks it through the generated Coq recursion at `gen/HtmlPure.hs:37-41` which allocates a `String` per input char, then re-packs to `Text`. For the patch detail page (diff + summary can easily be 1 MiB of escaped text) this is O(N) allocations with a huge constant. This will dominate TTFB for any non-trivial patch. A native `Text`-level escape using `T.replace` or a single-pass `Data.Text.Internal.Builder` is ~10���30�� faster and cuts allocation by the same factor.
+- `src/DarcsWeb/Html.hs:418-425` ��� `highlightDiff` does `T.concat . map highlightLine . T.lines`. `T.lines` allocates a list of lines, each `highlightLine` calls `esc` (which as noted above round-trips through String), then everything is `T.concat`'d. For a 1 MiB diff this is 2���3 full copies of the payload. A `Text.Lazy.Builder` that consumes one line and writes directly to the response would stream and halve the peak memory. Combined with the next finding, this is the single biggest CPU saving.
+- `app/Main.hs:231, 249, 267, 275, 283, 295, 309, 323, 337, 342` ��� every handler calls `html $ TL.fromStrict $ render���`. `renderPage` builds a strict `Text` via `T.concat`; `TL.fromStrict` wraps it in a one-chunk lazy Text; Scotty then encodes to UTF-8 `ByteString` and hands it to Warp. Three copies of the page live in memory before the first byte hits the socket, and TTFB equals full-render time. Switching `renderPage` to return `TL.Text` (or a `Builder`) built from `<>` on `TL.Text` (or `TLB.Builder`) and using Scotty's `raw` with a streaming body, or at minimum `html` on a `TL.Text` you never converted to strict, removes the redundant copy and lets Warp send the first chunk while later chunks are still being built.
+- `src/DarcsWeb/Html.hs:32-53` ��� `renderPage` takes `[Text]` and `T.concat`s. Every call site passes a single-element list (e.g. `renderShortLog` at line 149 passes `[body]`). The list wrapper is dead weight and pushes callers to pre-concat before calling. Either change the signature to `Text` and concat at one call site, or embrace builders.
+- `src/DarcsWeb/Darcs.hs:106-131` ��� `getRepoPatches` reads the entire patch set on every request to `/repo/:name/shortlog`, `/log`, and even `/summary` (which also calls `listRepos` separately at `app/Main.hs:238`). There is no caching layer. A slow-connection mobile user refreshing the index page triggers a full patch-set read per repo. Even a simple in-process `IORef (Map FilePath (ModTime, [PatchSummary]))` with `_darcs/hashed_inventory`'s mtime as the key would cut repeated loads to O(1). Right now, on a repo with 500 patches, every page view pays the cost of 500 `extractPatchListing` calls.
+- `app/Main.hs:236-249` ��� the summary handler does `listRepos` AND `getRepoPatches` AND `getRepoTags` ��� three full filesystem traversals of the same repo. `getRepoPatches` already returns everything needed to compute tags (filter `psIsTag`), but the handler calls `getRepoTags` separately, which re-opens the repo and re-reads all patches a second time. Combine the two calls: read once, partition.
+- `src/DarcsWeb/Darcs.hs:98-104` ��� `readRepoDescription` uses `readFile` (which is `String`-based lazy I/O) and is called once per repo in `listRepos`. For a repos directory with many entries, this adds up. `Data.Text.IO.readFile` or `Data.ByteString.readFile` + `decodeUtf8'` is both faster and avoids the lazy-I/O handle leak when an exception interrupts `map`.
+- `src/DarcsWeb/Darcs.hs:249` ��� `getRepoBlob` does `T.pack <$> Prelude.readFile fullPath`. Same issue as above. Worse: this path is for source files up to `maxBlobSize = 10 MiB`; round-tripping through `String` is ~30���50�� slower than `Data.Text.IO.readFile` with explicit decode. Even when the file is 1 MB, the lazy-I/O handle stays alive until the last character is consumed.
+- `app/Main.hs:228-231, 265-267` ��� `getCurrentTime` is invoked on every request, then passed to `relativeDate`, which re-parses every patch's date string with `parseTimeM`. On a 500-row shortlog this is 500 `parseTimeM` calls per request. Pre-parse dates once per patch (store `UTCTime` in `PatchSummary`, not `Text`) and format lazily.
+- `app/Main.hs:217-215, 381` ��� HTML responses never set `Cache-Control` or `ETag`. `securityHeaders` adds 6 headers (good), but the repo list and summary don't change between most requests. Even `Cache-Control: private, max-age=30` on `/repo/:name/shortlog` would protect against the "user taps back ��� refetch" round trip on a slow mobile link. An `ETag: "W/<darcs-inventory-hash>"` + 304 response would be even better.
+- `app/Main.hs:183-184` ��� `mimeType ".js" = "application/javascript"` is missing `charset=utf-8` (only `.css` has it). Modern browsers default to UTF-8 for JS, but explicit is better. Also `.svg` should be served with `charset=utf-8` so that SVG text elements render accentuated characters correctly.
+- `app/Main.hs:344-380` ��� `serveStatic` canonicalizes the path on every request. That is one `realpath(2)` (+ `stat(2)`s) per static asset load. Given every HTML page references `/static/style.css` and `/static/darcs-logo.png`, that's two `canonicalizePath` calls per page view. An in-memory map from path ��� canonical path (populated lazily, with a size cap) would remove this per-request syscall.
+- `app/Main.hs:378-380` ��� no `Content-Length` or `ETag` for the static file, so Warp's `sendfile` path works but the browser can't 304. `Cache-Control: public, max-age=86400, immutable` is a blunt hammer: `immutable` tells the browser never to revalidate, but there is no content-hash in the URL, so any change to `style.css` won't be picked up for 24 h. Either add a hash-in-query-string (`/static/style.css?v=<hash>`) at HTML emission time, or drop `immutable`.
+- `app/Main.hs:205-215` ��� `securityHeaders` appends `secHeaders` via `(++)` on every response. For each response the WAI header list is rebuilt. Minor, but using `mapResponseHeaders (secHeaders ++)` is the same cost; the real win is to ensure `Content-Security-Policy` has `style-src 'self'` for the site's own stylesheet (which it does) and consider adding `Cross-Origin-Opener-Policy: same-origin` for isolation.
+- `app/Main.hs:195-202` ��� `cspHeader` is a CAF, good. But it is built as `BC.pack $ CspPure.build_csp [���]`, and `CspPure.join_directives` recurses through a `String` with `(++)` (gen/CspPure.hs:45-48) which is O(n��) for long directive values. With the current 6 short directives it doesn't matter, but if anyone adds a `script-src` with hashes or nonces this will quadratic-blow-up.
+- `src/DarcsWeb/Html.hs:106-111` ��� `length recentPatches > 0` ��� use `not (null recentPatches)`. `length` on a list is O(n) and forces spine evaluation of the entire patch list just to decide whether to show a "more" link. Same pattern at line 118 (`length tags > 5` ��� this one needs the count, but `drop 5 tags` would tell you the same in O(5)).
+- `darcsweb.cabal:28-35, 42-54` ��� the library depends on `text` but not `text-builder` or `bytestring-builder`. Switching the hot rendering path to `Data.Text.Lazy.Builder` would require no new dependency (it's in `text`), so that's a zero-cost unlock.
+
+## Design & semantics
+- `src/DarcsWeb/Html.hs:39-44` ��� the page header is a `<div class="page-header">` containing the logo link and a `<nav class="breadcrumbs">`. The wrapping container should be a `<header>` (HTML5 landmark). Similarly line 48 ��� `<div class="page-footer">` should be `<footer>`. Line 45 ��� `<div class="page-body">` should be `<main>`. This change is mechanical, three string replacements, and immediately improves screen-reader landmark navigation on mobile (which relies heavily on "jump to main").
+- `src/DarcsWeb/Html.hs:58` ��� the index page passes an empty breadcrumbs string. That's fine, but the rendered HTML is `<nav class="breadcrumbs"></nav>` ��� an empty `<nav>` announced as an empty navigation region. Either omit the nav when breadcrumbs are empty, or fall back to a visible site title.
+- `src/DarcsWeb/Html.hs:96, 143, 184, 220, 251, 288, 356, 386` ��� `breadcrumbs` is composed as a raw string starting with ` / `. This is a typographic separator baked into the content; it ought to be a CSS `::before` on the link. More importantly the leading " / " means the breadcrumb announced by a screen reader starts with "slash"; use `aria-current="page"` on the last item and let CSS add the visual separator.
+- `src/DarcsWeb/Html.hs:60-63` ��� `<p class="empty">No repositories found.</p>` is styled (style.css:678) but conveys no status semantics. Use `<p role="status">` or `<div role="status" class="empty">` so assistive tech announces the empty state.
+- `src/DarcsWeb/Html.hs:67-76` ��� the repo list uses a `<table>` with headings "Repository | Description | Last Change | Patches". On mobile (CSS line 831-839) the `<thead>` is hidden (`display: none`). Users on screen readers still hear the table as a table, but without headings they hear "column 1, column 2" for each cell. Either use semantic `<dl>` / `<article>` markup that survives the mobile re-stack, or set `scope="col"` on the `<th>`s (they are not set today) and add `aria-hidden="true"` on the thead's mobile hiding so assistive tech skips it. At `src/DarcsWeb/Html.hs:70`, the `<th>` elements have no `scope`.
+- `src/DarcsWeb/Html.hs:164-178` ��� shortlog row: the two `<a>` elements point to the same URL (`/repo/NAME/patch/HASH`). One is the title link, one is a "diff" link. Duplicate targets waste keyboard tab stops and confuse assistive tech. Either drop the redundant link (title is enough) or make the second link point to a genuinely different view (e.g. raw diff, plain-text, downloadable).
+- `src/DarcsWeb/Html.hs:193-213` ��� each full-log entry uses `<div class="log-entry">`. Semantically these are `<article>` elements with a `<header>` and a content body. Same for patch detail (`src/DarcsWeb/Html.hs:223`).
+- `src/DarcsWeb/Html.hs:394-403` ��� `repoNavBar` emits a `<div class="repo-nav">` with a list of `<a>`. This is clearly a navigation region: use `<nav aria-label="Repository sections">` and put the links in a `<ul>`. The active link should carry `aria-current="page"`, not just `class="active"` (nav link generation at line 408).
+- `src/DarcsWeb/Html.hs:199, 225` ��� `<span class="tag-badge">TAG</span>` uses a text-only badge. It's visible but its role is decorative in context; wrap it in `aria-hidden="true"` and also add visually-hidden "(tag)" after the patch name so screen-reader users know this entry is a tag. Alternately, render the badge as `<abbr title="Tag">TAG</abbr>`.
+- `src/DarcsWeb/Html.hs:339-344` ��� the tree row icon is an emoji cell. Add `aria-hidden="true"` to the `<td class="tree-icon">` so screen readers don't announce "folder emoji" before every filename.
+- `src/DarcsWeb/Html.hs:383-386` ��� `render404` renders `<h1>404 - Not Found</h1>`. The `<title>` is "Not Found", but there's no `status` role or `<main role="main">` landmark. Also the 404 response is served through `html` which encodes a full page; for machine callers (the clone endpoint), a plain `text/plain` response would save bytes.
+- `src/DarcsWeb/Html.hs:43` ��� `<nav class="breadcrumbs">` inside the page header is nested inside another semantic region (header). Fine in HTML5, but give it `aria-label="Breadcrumb"` so assistive tech can pick it out from other `<nav>`s on the page.
+- `src/DarcsWeb/Html.hs:29` ��� header links all resolve to PNG/brand styling. The PNG `darcs-logo.png` referenced in `renderPage` has no `srcset` for HiDPI displays. On retina/Android XHDPI the logo renders blurry. Add `srcset` or switch to an SVG.
+
+## Quick wins
+Ordered by impact-to-effort ratio.
+
+1. **Stop round-tripping escape through `String`.** One-line `esc` change in `src/DarcsWeb/Html.hs:413-414` removes a massive allocation bottleneck on any page with substantial text (diff views, blobs).
+ ```haskell
+ -- Before (src/DarcsWeb/Html.hs:413-414):
+ esc :: Text -> Text
+ esc = T.pack . HtmlPure.esc . T.unpack
+
+ -- After:
+ esc :: Text -> Text
+ esc = T.concatMap escChar
+ where
+ escChar '<' = "<"
+ escChar '>' = ">"
+ escChar '&' = "&"
+ escChar '"' = """
+ escChar '\'' = "'"
+ escChar c = T.singleton c
+ ```
+ Keep `HtmlPure.esc` as the formally verified spec, and add a QuickCheck property in `test/Properties/Html.hs` that `esc` agrees with `T.pack . HtmlPure.esc . T.unpack` on arbitrary input. You lose nothing and gain 10���30�� on the hot path.
+
+2. **Switch rendering to `Data.Text.Lazy.Builder` and serve without re-conversion.** The current `renderPage` ��� strict `Text` ��� `TL.fromStrict` ��� `html` path at e.g. `app/Main.hs:231` copies the page three times. Minimum-diff version:
+ ```haskell
+ -- In src/DarcsWeb/Html.hs, switch Text to TL.Text for the top-level return,
+ -- built via Data.Text.Lazy.Builder (TLB). Each helper returns TLB.Builder
+ -- and renderPage finalizes via TLB.toLazyText.
+ import qualified Data.Text.Lazy.Builder as TLB
+ import qualified Data.Text.Lazy as TL
+
+ renderPage :: Text -> TLB.Builder -> TLB.Builder -> TL.Text
+ renderPage title breadcrumbs body = TLB.toLazyText $
+ "<!DOCTYPE html>\n<html lang=\"en\">\n<head>\n���"
+ <> TLB.fromText (esc title) <> ���
+
+ -- Then in app/Main.hs:231:
+ html $ renderRepoList now (cfgTitle cfg) repos -- no more TL.fromStrict
+ ```
+ This alone turns the render pipeline into a lazy stream that Warp can start flushing immediately. Even if you keep strict `Text` outputs, dropping the `TL.fromStrict` by returning `TL.Text` directly from `render���` cuts one full copy.
+
+3. **Add `<header>`/`<main>`/`<footer>` and `<nav aria-label="���">` everywhere.** Pure string replacement in `src/DarcsWeb/Html.hs:39, 45, 48, 43, 396`. No CSS change needed ��� the classes stay. Unlocks screen-reader landmark navigation on mobile (where it matters most because there's no mouse) and improves SEO ��� zero risk of regression.
+ ```haskell
+ -- src/DarcsWeb/Html.hs:39-48
+ , "<header class=\"page-header\">\n"
+ ���
+ , "<nav class=\"breadcrumbs\" aria-label=\"Breadcrumb\">", breadcrumbs, "</nav>\n"
+ , "</header>\n"
+ , "<main class=\"page-body\">\n"
+ ���
+ , "</main>\n"
+ , "<footer class=\"page-footer\">\n"
+ ```
+ And `repoNavBar` at line 395:
+ ```haskell
+ repoNavBar repoName active = T.concat
+ [ "<nav class=\"repo-nav\" aria-label=\"Repository sections\">\n"
+ , ���
+ , "</nav>\n"
+ ]
+ ```
+ And in `navLink'` at line 408, emit `aria-current="page"` alongside `class="active"`.
+
+4. **Fix tap targets.** The row-action links are well below the 44 px minimum. Minimum diff: wrap them in a class that CSS can target with `min-height: 44px; display: inline-flex; align-items: center; padding: 10px 12px;`. Since `static/style.css` already has the class hooks, this is a CSS-only change, but the HTML must provide a target. At `src/DarcsWeb/Html.hs:86-88, 175-176, 279-280` the links are already inside `<td class="actions">`, so touch the CSS. (Out of scope per brief ��� note this is the single largest mobile-UX regression.)
+
+5. **Serve a favicon.** Add one line to `renderPage` at `src/DarcsWeb/Html.hs:37`:
+ ```haskell
+ , "<link rel=\"icon\" type=\"image/png\" href=\"/static/darcs-logo.png\">\n"
+ ```
+ Kills the 404 refetch on every page load from mobile browsers.
+
+6. **Pre-parse dates, don't re-parse on every render.** In `src/DarcsWeb/Types.hs:15`, change `psDate :: !Text` to `psDate :: !UTCTime` (or add a parallel `psDateParsed :: !UTCTime`). The parse happens once when the `PatchSummary` is constructed in `Darcs.hs:278` (move `parseTimeM` there), and `relativeDate` at `src/DarcsWeb/Html.hs:430` stops being a per-row parse. Saves 500 `parseTimeM` per shortlog request.
+
+7. **Cache `listRepos` / `getRepoPatches` by inventory hash.** Add an `IORef` keyed on `_darcs/hashed_inventory` mtime at the `DarcsWebConfig` level; invalidate on change. Concrete sketch:
+ ```haskell
+ -- src/DarcsWeb/Types.hs: add a cache field
+ , cfgPatchCache :: IORef (Map FilePath (UTCTime, [PatchSummary]))
+
+ -- src/DarcsWeb/Darcs.hs: new wrapper
+ getRepoPatchesCached :: IORef ��� -> FilePath -> IO [PatchSummary]
+ getRepoPatchesCached ref repoPath = do
+ mt <- getModificationTime (repoPath </> "_darcs/hashed_inventory")
+ m <- readIORef ref
+ case Map.lookup repoPath m of
+ Just (mt', ps) | mt' == mt -> pure ps
+ _ -> do ps <- getRepoPatches repoPath
+ atomicModifyIORef' ref (\m' -> (Map.insert repoPath (mt, ps) m', ()))
+ pure ps
+ ```
+ Repos don't change often; mobile users refreshing the same repo on a slow link benefit enormously.
+
+## Deeper restructuring (optional)
+- **Stream diffs with `StreamingBody`**. Right now the full diff for a patch is materialized in memory (`src/DarcsWeb/Darcs.hs:267-272` `T.pack $ renderString $ showPatch ForDisplay p`, forced at `:143` with `evaluate (T.length (psDiff p))`) and then HTML-escaped whole (`src/DarcsWeb/Html.hs:241`). For a 50 MB diff (e.g. a vendored dependency), this blocks the main thread and pegs allocation. Switching to Warp's `responseStream` (accessible via Scotty's `raw` / `stream`), and writing the page header, then each diff line as it's produced by the darcs printer, would both halve memory and let the user see the first diff lines while the rest still streams. This requires a non-trivial refactor of `extractPatchFull` to be incremental rather than returning a final `Text`.
+- **Drop the strict `Text` ��� lazy `Text` ��� `ByteString` triple-copy entirely**. Rewrite `renderPage` as a `Data.ByteString.Builder` over UTF-8-encoded segments, which Warp then converts to lazy `ByteString` with zero additional copy. Since all HTML templates are static ASCII except for user-supplied text (which gets escaped), this is a clean win; `esc` becomes `Text -> Builder`. Bigger payoff than #1 + #2 combined, but a larger diff; would also affect the test suite.
+- **Introduce an HTTP caching layer driven by darcs inventory hash**. `darcs show repo --xml-output` exposes an inventory hash; use it as a strong ETag on `/` and every `/repo/:name/*` page. A mobile user pinching through history on a slow LTE link then fetches `304` for pages they've already visited instead of the full HTML. This pairs naturally with #7 in quick wins and makes sense to design together.
+- **Replace the generated `HtmlPure` / `PathPure` `String`-based hot paths with `Text`-native extractors, keeping Coq as the spec.** The point of Coq-verified code is correctness. You can keep the Coq definitions and their proofs as the oracle, then implement the production versions against `Text` / `ByteString` and QuickCheck-test equivalence. That way the fast path is no longer bounded by the `String`-based extraction's allocator cost, and the verification story is unchanged. Affects `gen/HtmlPure.hs`, `gen/PathPure.hs`, and all callers in `src/DarcsWeb/Html.hs:414`, `src/DarcsWeb/Darcs.hs:186, 191`, and `app/Main.hs:411`.
addfile ./reviews/codex-readability.md
hunk ./reviews/codex-readability.md 1
+# Readability Review ��� darcsweb
+
+## Summary
+The codebase is mostly readable once you get down to individual helpers: comments are better than average, the tests are fairly direct, and the security-sensitive path checks are usually explained. The main friction is structural, not algorithmic: `app/Main.hs` and `src/DarcsWeb/Html.hs` carry too many responsibilities in single files, while a few dense idioms force the reader to mentally decompile otherwise simple logic. The generated `gen/*.hs` modules are predictably noisy, but the handwritten wrappers do not always hide that noise well enough.
+
+## Confusing constructs
+- `app/Main.hs:74` uses `foldl (flip id) defaultOpts o`, which hides a simple "apply parsed option updates to defaults" step behind a classic but opaque idiom. Plain rewrite: `foldl (\opts applyOpt -> applyOpt opts) defaultOpts o` or a named helper such as `applyParsedOptions`.
+- `app/Main.hs:257`, `app/Main.hs:258`, `app/Main.hs:299`, `app/Main.hs:300`, `app/Main.hs:313`, `app/Main.hs:314` pull regex captures out with `pathParam "1"` and `pathParam "2"`. Plain rewrite: wrap each regex route in a helper that returns named values like `(repoName, subPath)` so readers do not have to map numbers back to the regex.
+- `app/Main.hs:451` defines `decodeHdr = (>>= eitherToMaybe . TE.decodeUtf8') . flip lookup hdrs`, which compresses a straightforward lookup-and-decode path into point-free composition. Plain rewrite: `decodeHeader name = case lookup name hdrs of ...`.
+- `src/DarcsWeb/Config.hs:24`, `src/DarcsWeb/Config.hs:26`, `src/DarcsWeb/Config.hs:33` make the tiny config parser read like a puzzle, with a guarded `head` and a `reverse . dropWhile ... . reverse` trim. Plain rewrite: pattern-match on the stripped line and use `dropWhileEnd isSpace`.
+- `src/DarcsWeb/Darcs.hs:177`, `src/DarcsWeb/Darcs.hs:180`, `src/DarcsWeb/Darcs.hs:181` encode the jail check with nested `if`s plus `last` and `init`, so the exact-directory case is hard to spot. Plain rewrite: bind `jailRoot` once and test `canonical == jailRoot || addTrailingSlash jailRoot \`isPrefixOf\` canonical`.
+- `src/DarcsWeb/Html.hs:369`, `src/DarcsWeb/Html.hs:370`, `src/DarcsWeb/Html.hs:371` rely on `init` and `last` after `T.splitOn "/" subPath`; the invariant may be true, but it is invisible at the use site. Plain rewrite: pattern-match on the segment list once and bind `dirParts` and `fileName` explicitly.
+- `test/Properties/Csp.hs:38` uses `result !! length name` just to assert that a space follows the directive name. Plain rewrite: `case drop (length name) result of ' ':_ -> True; _ -> False`.
+
+## Naming
+- `src/DarcsWeb/Config.hs:14` and `app/Main.hs:94` use `CfgMap` and `cfgMap` next to `DarcsWebConfig`. Suggested names: `ConfigMap` and `configMap`, so the difference between raw parsed key-value data and validated runtime config is obvious.
+- `app/Main.hs:239` and `src/DarcsWeb/Darcs.hs:81` use `mRepoInfo` and `mInfo`. Suggested names: `maybeRepoInfo` and `maybeBasicInfo`.
+- `src/DarcsWeb/Darcs.hs:255` and `src/DarcsWeb/Darcs.hs:265` use `piap`, which is hard to decode even with the type on screen. Suggested names: `patchInfoAnd` or `patchWithInfo`.
+- `src/DarcsWeb/Html.hs:78`, `src/DarcsWeb/Html.hs:163`, `src/DarcsWeb/Html.hs:193`, `src/DarcsWeb/Html.hs:217` use `ri` and `ps` throughout the largest handwritten module. Suggested names: `repo` and `patch`, so the rendering code reads like the page it produces.
+
+## Structure
+- `app/Main.hs:85` through `app/Main.hs:120` pack argument parsing, config loading, path canonicalisation, logger selection, daemonisation, and server startup into one `main`. A smaller top-level flow such as `loadSettings`, `mkLogger`, and `startServer` would fit on one screen.
+- `app/Main.hs:217` through `app/Main.hs:342` register every route inline and repeat the same `withRepo` / load / render-404 pattern. Splitting this into route groups plus a tiny `respondNotFound` helper would make the route table scanable.
+- `app/Main.hs:344` through `app/Main.hs:406` and `app/Main.hs:419` through `app/Main.hs:440` repeat the same "canonicalise, jail-check, existence-check, then 404" structure across static files, repos, and clone files. A shared helper would turn three nested `if` ladders into declarative routing code.
+- `src/DarcsWeb/Html.hs:31` through `src/DarcsWeb/Html.hs:467` mix page layout, repo summary pages, tree/blob views, breadcrumb construction, diff highlighting, and date/author formatting in one module. Splitting by responsibility would let a reader change one kind of output without paging through the entire renderer.
+- `src/DarcsWeb/Html.hs:297` through `src/DarcsWeb/Html.hs:308` and `src/DarcsWeb/Html.hs:367` through `src/DarcsWeb/Html.hs:381` duplicate nearly the same breadcrumb builder. One shared helper would remove a lot of local recursion noise.
+- `src/DarcsWeb/Darcs.hs:121` through `src/DarcsWeb/Darcs.hs:164` and `src/DarcsWeb/Darcs.hs:196` through `src/DarcsWeb/Darcs.hs:250` repeat "run Darcs action and swallow exceptions" plus deep validation ladders. Small helpers like `runRepoJobOr` and `guardRepoPath` would flatten the module.
+
+## Comments & docs
+- `app/Main.hs:4` has no module header describing the overall request flow or where path/jail invariants are enforced. That context matters because `Main` is the operational entrypoint and one of the largest reader-facing modules.
+- `src/DarcsWeb/Darcs.hs:1` and `src/DarcsWeb/Darcs.hs:7` would benefit from a top-of-file note explaining why the Darcs integration needs advanced extensions and where the boundary lies between Darcs-library access, plain filesystem reads, and Coq-extracted path checks.
+- `src/DarcsWeb/Html.hs:3` should say up front that it intentionally hand-builds HTML `Text` and relies on `esc` for safety. That design choice shapes how every function in the module should be read.
+- `src/DarcsWeb/Types.hs:1` has field comments, but no module or type-level note explaining why `PatchSummary` carries `psLog`, `psSummary`, and `psDiff`, or why `RepoInfo` stores a filesystem path. A short overview would help new readers orient themselves.
+- `test/Spec.hs:1` needs a brief note that the test runner intentionally performs proof verification before QuickCheck. That sequencing is project-specific and not obvious from the filename alone.
+
+## Small wins
+- `app/Main.hs:1`, `app/Main.hs:169`, `app/Main.hs:171` bring raw FFI into the executable entrypoint just to open `/dev/null`. If the project can use `System.Posix.IO.openFd`, that would remove the `ForeignFunctionInterface` extension entirely; if not, move the FFI to a tiny helper module so `Main` stays at the HTTP/application level.
+- `src/DarcsWeb/Darcs.hs:69` is a top-level binding without a signature. Adding `checkRepo :: FilePath -> String -> IO [RepoInfo]` would match the otherwise explicit style.
+- `src/DarcsWeb/Html.hs:347`, `src/DarcsWeb/Html.hs:350`, `src/DarcsWeb/Html.hs:430`, `src/DarcsWeb/Html.hs:441` use raw size/time constants (`1024`, `1048576`, `604800`, `2419200`). Naming them as `kib`, `mib`, `week`, and `fourWeeks` would make policy read as policy instead of arithmetic.
+- `src/DarcsWeb/Config.hs:39` should use `fromMaybe def` instead of `maybe def id`; it reads as data lookup rather than a mini combinator exercise.
+- `src/DarcsWeb/Html.hs:109` should use `not (null recentPatches)` instead of `length recentPatches > 0`; the latter makes the reader wonder whether the exact length matters.
+- `app/Main.hs:21` is one of the few non-explicit imports in the handwritten modules. Matching the surrounding explicit import style would make `Main` easier to skim.
+
+## Extracted code note
+- `gen/HtmlPure.hs:1`, `gen/CspPure.hs:1`, and `gen/PathPure.hs:1` are clearly extraction artefacts: warnings are globally suppressed, helper names are mechanical, and simple logic expands into case-heavy recursion. These are extracted and cannot be edited directly; the right fix is to keep them quarantined behind well-named handwritten wrappers.
+- `gen/PathPure.hs:70`, `gen/PathPure.hs:92`, and `gen/PathPure.hs:99` encode important path rules in dense boolean nests. Because the source of truth is the verified Coq/Rocq side, the readable improvement belongs there, or in better wrapper-side documentation at `src/DarcsWeb/Darcs.hs:185` and `app/Main.hs:410`, not in hand-cleaning the generated Haskell.
+- `gen/HtmlPure.hs:16` and `gen/CspPure.hs:33` expose low-level generated names directly to Haskell readers. A thin handwritten wrapper layer with domain names like `escapeHtml` and `buildCspHeader` would keep the extraction naming scheme out of the rest of the codebase.
addfile ./reviews/codex-reconciliation.md
hunk ./reviews/codex-reconciliation.md 1
+# Codex/Claude Reconciliation
+
+## Q1. Tree-breadcrumb XSS (`codex` security H1 vs Claude)
+
+Verdict: the sink is real, but my original H1 phrasing was too strong. `renderTree` appends raw `subPath` into `breadcrumbs` at [src/DarcsWeb/Html.hs:287](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Html.hs:287)-[288](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Html.hs:288), and `renderPage` injects `breadcrumbs` verbatim into `<nav>` at [src/DarcsWeb/Html.hs:43](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Html.hs:43). A committed directory name can reach this path: the tree route passes the URL subpath through to `getRepoTree`, and `doesDirectoryExist` / `jailCheck` only verify existence and containment, not escaping or re-encoding.
+
+The percent-decoding point matters in favor of exploitability, not against it: Scotty regex routes operate on decoded path text, so a browser request for `%3Cimg...%3E` becomes `<img...>` before `subPath` is rendered. That said, the site-wide CSP at [app/Main.hs:195](/home/fritjof/repositories/playground/darcsweb/app/Main.hs:195)-[214](/home/fritjof/repositories/playground/darcsweb/app/Main.hs:214) is strict enough that I would not currently call this a reliable script-execution XSS; it is a reachable stored HTML-injection bug that should be fixed before any CSP relaxation.
+
+The right fix is still: escape each displayed segment and percent-encode href path segments. `renderTreeBreadcrumb` at [src/DarcsWeb/Html.hs:297](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Html.hs:297)-[308](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Html.hs:308) already escapes each visible `p`, but `newAcc` is only HTML-escaped in the `href`, not URL-encoded. `renderBlobBreadcrumb` at [src/DarcsWeb/Html.hs:367](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Html.hs:367)-[381](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Html.hs:381) does **not** have the same raw-HTML bug because the top-level breadcrumb uses `esc subPath` at [src/DarcsWeb/Html.hs:356](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Html.hs:356), and `buildCrumbs` escapes each label; its remaining issue is the same href-encoding problem, plus the low-risk partial `init` / `last`.
+
+## Q2. Clone endpoint `_darcs/` over-exposure (`codex` M3 vs Claude)
+
+Verdict: real. `isSafeSubPath` in [gen/PathPure.hs:70](/home/fritjof/repositories/playground/darcsweb/gen/PathPure.hs:70)-[97](/home/fritjof/repositories/playground/darcsweb/gen/PathPure.hs:97) only rejects empty, dot, hidden, `_darcs`, and `.git` segments. `serveClone` at [app/Main.hs:419](/home/fritjof/repositories/playground/darcsweb/app/Main.hs:419)-[436](/home/fritjof/repositories/playground/darcsweb/app/Main.hs:436) then serves any existing regular file under `repoPath/_darcs/` whose subpath passes that predicate. So `_darcs/prefs/defaultrepo`, `_darcs/prefs/motd`, `_darcs/format`, and optional pack files all pass if present.
+
+I would keep this as `MED` and fix it in the one-PR pass because the diff is small and the policy becomes explicit. Your four-class allowlist matches the base hashed HTTP-clone surface and the current QuickCheck expectations in [test/Properties/Clone.hs:131](/home/fritjof/repositories/playground/darcsweb/test/Properties/Clone.hs:131)-[139](/home/fritjof/repositories/playground/darcsweb/test/Properties/Clone.hs:139): `hashed_inventory`, `inventories/*`, `patches/*`, `pristine.hashed/*`.
+
+There is one caveat: darcs also supports HTTP packs from `darcs optimize http`. If you want to preserve that optimization path, also allow `_darcs/packs/basic.tar.gz` and `_darcs/packs/patches.tar.gz`. I did **not** find evidence that `_darcs/prefs/*` or `_darcs/format` need to be public for clone.
+
+## Q3. `read` on port (Claude H1 vs codex hardening note)
+
+Verdict: fix now, but not as `HIGH`. The bug is at [app/Main.hs:96](/home/fritjof/repositories/playground/darcsweb/app/Main.hs:96), but it is startup-only and only triggered by an operator-written config file. `readMaybe` + a clear error on stderr + `exitFailure` is a good tiny robustness fix, not over-engineering.
+
+## Q4. Blob endpoint lazy `readFile` (Claude H2 vs codex M2)
+
+Verdict: Claude���s remediation is directionally right, but I would separate must-have from nice-to-have. The must-have part is replacing [src/DarcsWeb/Darcs.hs:249](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Darcs.hs:249) with strict `BS.readFile` under `try`, followed by explicit UTF-8 handling (`decodeUtf8'`). That removes the lazy-handle leak and locale-sensitive decoding, which are the two clearly real bugs.
+
+The NUL-byte sniff in the first 8 KiB plus a `"binary file, cannot display"` placeholder is a good small extra and I would include it if touching this code anyway, because it avoids misclassifying obvious binary files as 404s. If scope gets tight, the 80% reduction is: strict bytes, `try`, explicit UTF-8 decode; the NUL sniff is cheap but not the core fix. Also note that a text file full of `<` still expands during HTML escaping, so the `esc` hotspot is a separate performance fix, not solved by binary detection.
+
+## Q5. `canonicalizePath` exception escape (Claude H3)
+
+Verdict: yes, the request-time `canonicalizePath` call sites should be wrapped. The affected sites are [app/Main.hs:365](/home/fritjof/repositories/playground/darcsweb/app/Main.hs:365), [400](/home/fritjof/repositories/playground/darcsweb/app/Main.hs:400), [425](/home/fritjof/repositories/playground/darcsweb/app/Main.hs:425), and [src/DarcsWeb/Darcs.hs:178](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Darcs.hs:178).
+
+On the local Scotty 0.22 install, the uncaught-exception path is a plain-text `Internal Server Error: ...` response, so Claude���s leak concern is credible; even without pinning the exact Scotty version, uncaught exceptions definitely escape as framework 500s today. `try` + `Nothing => 404` is the right behavior for request-derived candidate paths. This does **not** mask legitimate startup config failures as long as the startup canonicalization in `main` at [app/Main.hs:102](/home/fritjof/repositories/playground/darcsweb/app/Main.hs:102)-[103](/home/fritjof/repositories/playground/darcsweb/app/Main.hs:103) stays fail-fast.
+
+## Q6. Single ordered fix list for one PR
+
+1. [src/DarcsWeb/Darcs.hs:134-145](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Darcs.hs:134) ��� Patch detail renders full diffs for every patch before checking the requested hash. Compare `targetHash` against `PatchInfo` first and only call `extractPatchFull` for the first match. Tag: `CRITICAL`.
+
+2. [src/DarcsWeb/Darcs.hs:196-213](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Darcs.hs:196), [app/Main.hs:298-309](/home/fritjof/repositories/playground/darcsweb/app/Main.hs:298) ��� Tree browsing bypasses the verified safe-subpath policy and exposes `_darcs` / hidden directory structure. Reject non-empty tree subpaths unless `isSafeSubPath` succeeds, and add a small property test for representative unsafe cases. Tag: `HIGH`.
+
+3. [src/DarcsWeb/Html.hs:287-304](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Html.hs:287), [319-335](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Html.hs:319), [356-381](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Html.hs:356) ��� Tree/blob breadcrumbs and tree/blob hrefs mix raw path text into HTML and use HTML escaping where URL encoding is required. Replace ad hoc string assembly with a segment-based helper that `esc`s labels, percent-encodes href segments, and makes blob breadcrumb handling total at the same time. Tag: `MED`.
+
+4. [src/DarcsWeb/Darcs.hs:98-104](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Darcs.hs:98), [229-250](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Darcs.hs:229) ��� Repo-controlled file reads still use lazy, locale-sensitive `String` IO on the request path. Switch to strict `ByteString` reads under `try`, decode explicitly as UTF-8, return empty description on bad repo descriptions, and show a binary placeholder for obvious non-text blobs. Tag: `MED`.
+
+5. [app/Main.hs:365](/home/fritjof/repositories/playground/darcsweb/app/Main.hs:365), [400](/home/fritjof/repositories/playground/darcsweb/app/Main.hs:400), [425](/home/fritjof/repositories/playground/darcsweb/app/Main.hs:425), [src/DarcsWeb/Darcs.hs:178](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Darcs.hs:178) ��� Request-derived `canonicalizePath` failures currently escape as framework 500s. Add a tiny `safeCanonicalize` helper for request-time path resolution and treat `Left` as 404 while logging server-side if desired. Tag: `MED`.
+
+6. [app/Main.hs:421-436](/home/fritjof/repositories/playground/darcsweb/app/Main.hs:421) ��� Clone serving is broader than the darcs HTTP protocol actually needs. Add an explicit allowlist for `hashed_inventory`, `inventories/*`, `patches/*`, `pristine.hashed/*`, and optionally `_darcs/packs/{basic,patches}.tar.gz` if you want to preserve `optimize http` pack support. Tag: `MED`.
+
+7. [app/Main.hs:236-249](/home/fritjof/repositories/playground/darcsweb/app/Main.hs:236), [src/DarcsWeb/Darcs.hs:63-81](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Darcs.hs:63), [121-160](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Darcs.hs:121) ��� The summary page rescans all repos and rereads the target repo twice. Introduce a single-repo summary helper that gathers `RepoInfo`, recent patches, and tags in one pass instead of `listRepos` + `getRepoPatches` + `getRepoTags`. Tag: `PERF`.
+
+8. [src/DarcsWeb/Html.hs:413-425](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Html.hs:413) ��� `esc` and `highlightDiff` still round-trip through `String` on the hottest render path. Replace `esc` with a Text-native implementation and add a QuickCheck equivalence property against `HtmlPure.esc`. Tag: `PERF`.
+
+9. [app/Main.hs:96](/home/fritjof/repositories/playground/darcsweb/app/Main.hs:96) ��� Port parsing still uses partial `read`. Replace it with `readMaybe`, a port-range check, and an explicit diagnostic before exit. Tag: `LOW`.
+
+10. [src/DarcsWeb/Html.hs:37](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Html.hs:37), [39-48](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Html.hs:39), [56-64](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Html.hs:56) ��� The page chrome is missing favicon and landmark semantics, and the index page has no visible `<h1>`. Add `<link rel="icon">`, swap the wrapper `div`s to `<header>/<main>/<footer>` while keeping classes, and add an index-page heading. Tag: `LOW`.
+
+11. [app/Main.hs:74](/home/fritjof/repositories/playground/darcsweb/app/Main.hs:74), [413-414](/home/fritjof/repositories/playground/darcsweb/app/Main.hs:413), [src/DarcsWeb/Config.hs:39](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Config.hs:39), [src/DarcsWeb/Html.hs:109](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Html.hs:109), [274](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Html.hs:274) ��� The remaining idiom cleanups are low-risk readability wins. Replace `foldl (flip id)`, `findRepo`, `maybe x id`, and `length > 0` with the obvious forms while touching nearby code anyway. Tag: `READ`.
+
+## Out of scope for this pass
+
+- Rewriting [src/DarcsWeb/Html.hs:31-53](/home/fritjof/repositories/playground/darcsweb/src/DarcsWeb/Html.hs:31) and the Scotty call sites to use lazy builders / streaming responses throughout. That is worthwhile, but it touches every renderer and is larger than the rest of this pass.
+- Adding cross-request caches, ETags, or 304 handling for repo metadata and HTML pages. Good next step, but stateful and bigger than a ���small diff��� cleanup PR.
+- Splitting `Html.hs` / `Main.hs`, extracting handler modules, or introducing new dependencies. Those are structural refactors, not one-pass corrections.
addfile ./reviews/codex-security.md
hunk ./reviews/codex-security.md 1
+# Security Review ��� darcsweb
+
+## Summary
+The pure extracted helpers are narrow and mostly used correctly, but the highest-risk problems sit outside the proved core: expensive `darcs` traversals on unauthenticated routes, one stored XSS in the tree view, and several filesystem/decoding edges that can still throw. The worst issue is the patch-detail endpoint, which appears to render full diffs for the entire repository history before it knows whether the requested hash matches. The other recurring theme is that the Coq-backed path/escaping guarantees are not applied consistently at the HTTP boundary.
+
+## Critical findings
+
+### C1. Patch detail requests do O(history �� diff-size) work before hash filtering
+File: `src/DarcsWeb/Darcs.hs:134`, `src/DarcsWeb/Darcs.hs:139`, `src/DarcsWeb/Darcs.hs:263`
+
+Why it is dangerous: `getRepoPatch` builds `allPatches = mapRL extractPatchFull patchRL` and only then filters by `psHash`. `extractPatchFull` renders the full diff with `showPatch ForDisplay` for each patch. On a large repository, a single request for a missing hash or an old patch can force diff generation across the entire patch history, which is an unauthenticated CPU and memory exhaustion primitive.
+
+Concrete exploit scenario: an attacker repeatedly requests `/repo/large-repo/patch/does-not-exist` or a known oldest patch hash. Each request walks the whole patch set and renders every diff before returning `404` or the target patch, tying up the process and making the site unavailable.
+
+Proposed fix: compare hashes using `PatchInfo` before rendering any diff, and only call `extractPatchFull` for the first matching patch. If the darcs library can query by patch identity directly, use that. Also add an explicit cap on rendered diff size and fail closed when the patch body exceeds it.
+
+## High findings
+
+### H1. Stored XSS in tree breadcrumbs via unescaped `subPath`
+File: `src/DarcsWeb/Html.hs:287`, `src/DarcsWeb/Html.hs:288`, `src/DarcsWeb/Html.hs:295`, `src/DarcsWeb/Html.hs:43`
+
+Why it is dangerous: `renderTree` appends raw `subPath` into the breadcrumb HTML (`" / " <> subPath`) and `renderPage` injects `breadcrumbs` directly into `<nav>` without escaping. A repository directory name containing HTML markup therefore becomes active HTML on the tree page.
+
+Concrete exploit scenario: a repository contains a directory named `<img src=x onerror=alert(1)>`. Visiting `/repo/<repo>/tree/%3Cimg%20src=x%20onerror=alert(1)%3E` renders that payload inside the breadcrumb bar and executes script in the site's origin.
+
+Proposed fix: stop passing pre-rendered breadcrumb HTML around. Represent breadcrumbs as structured `(label, href)` data, escape every label with `esc`, and percent-encode every path segment when building `href` values.
+
+### H2. Index and summary routes recompute repository metadata across the entire repo set on every request
+File: `app/Main.hs:228`, `app/Main.hs:238`, `src/DarcsWeb/Darcs.hs:63`, `src/DarcsWeb/Darcs.hs:80`, `src/DarcsWeb/Darcs.hs:107`
+
+Why it is dangerous: both `/` and `/repo/:name/summary` call `listRepos`, and `listRepos` calls `getBasicRepoInfo` for every repository. `getBasicRepoInfo` opens each repo with `readPatches` and counts the whole patch list just to display last-change metadata. This makes a single summary page request proportional to all hosted repositories, not just the one being viewed.
+
+Concrete exploit scenario: on an instance hosting many large repos, an attacker can repeatedly hit `/repo/<any>/summary` and force full-history scans across the entire repository set. That is a low-effort unauthenticated availability attack.
+
+Proposed fix: cache repo metadata outside the request path, or at minimum load summary-page metadata only for the requested repo. If last-change and patch-count must be shown on `/`, compute them asynchronously and serve stale-but-safe cached values.
+
+## Medium / Low findings
+
+### M1. Tree browsing bypasses the verified safe-subpath policy and exposes hidden directory structure
+File: `app/Main.hs:298`, `src/DarcsWeb/Darcs.hs:196`, `src/DarcsWeb/Darcs.hs:208`, `src/DarcsWeb/Darcs.hs:231`
+
+Why it is dangerous: the blob and clone paths call the Coq-extracted `isSafeSubPath`, but the tree path does not. `getRepoTree` accepts any in-jail subdirectory and then lists its contents, only filtering hidden names in the final directory entries. Direct requests to `.git`, `.ssh`, or other dot-directories under the working tree are therefore possible.
+
+Concrete exploit scenario: `/repo/<repo>/tree/.git` or `/repo/<repo>/tree/.ssh` will list non-hidden files inside those directories when they exist. Even without direct file reads, that leaks internal layout and filenames that the UI otherwise tries to hide.
+
+Proposed fix: use the same validated path type for tree/blob/clone access. Root can remain a separate case, but any non-empty tree subpath should pass `isSafeSubPath` before touching the filesystem.
+
+### M2. Repository-controlled files can trigger uncaught filesystem and decoding exceptions
+File: `src/DarcsWeb/Darcs.hs:98`, `src/DarcsWeb/Darcs.hs:103`, `src/DarcsWeb/Darcs.hs:208`, `src/DarcsWeb/Darcs.hs:245`, `src/DarcsWeb/Darcs.hs:249`
+
+Why it is dangerous: `readRepoDescription`, `listDirectory`, `getFileSize`, and `Prelude.readFile` are used on repo-controlled paths without a `try` boundary. `Prelude.readFile` also depends on locale decoding. A malformed or unreadable file can therefore escape `IO` as a 500 instead of degrading safely.
+
+Concrete exploit scenario: a hosted repo contains a non-UTF-8 `_darcs/prefs/repo_description` or a blob file that decodes badly under the server locale. Requests for `/` or `/repo/<repo>/blob/...` then throw and return 500; if the bad description is hit through `listRepos`, it can take down the index and summary pages.
+
+Proposed fix: wrap all repo filesystem reads in `try`, use strict `ByteString` reads with explicit UTF-8 handling, and treat unreadable or undecodable files as missing or binary rather than letting exceptions escape.
+
+### M3. Medium ��� needs confirmation: the clone endpoint exposes every non-hidden file under `_darcs/`, not just clone protocol objects
+File: `app/Main.hs:419`, `app/Main.hs:423`, `gen/PathPure.hs:70`, `gen/PathPure.hs:92`
+
+Why it is dangerous: `serveClone` relies on `isSafeSubPath`, but that predicate only forbids dot-prefixed segments, `_darcs`, and `.git`. It does not whitelist the specific objects needed for read-only HTTP clone, so files such as `_darcs/prefs/defaultrepo` remain fetchable if present.
+
+Concrete exploit scenario: if repository-local darcs preferences contain upstream URLs, usernames, or other deployment-specific metadata, an unauthenticated client can retrieve them directly through `/clone/<repo>/_darcs/...` even though they are not needed to serve inventories and patches.
+
+Proposed fix: replace the generic safe-subpath check with a protocol-level allowlist for cloneable objects such as `hashed_inventory`, `inventories/*`, `patches/*`, and `pristine.hashed/*`. Keep the current path-safety check as a second line of defense, not the only one.
+
+## Verification gaps
+
+The extracted modules in `gen/` look like direct mechanical extractions, so the main divergence is not inside `HtmlPure`, `PathPure`, or `CspPure`; it is in the handwritten wrappers and routes that decide when those helpers are used.
+
+`HtmlPure.esc` is proved, but `src/DarcsWeb/Html.hs:287` bypasses it and reintroduces raw HTML injection in the tree breadcrumb path. There is no property test today that says "all route-derived text rendered into HTML stays escaped in every context".
+
+`PathPure.is_safe_sub_path` is proved and tested in `test/Properties/Clone.hs:119`, but the tree route ignores it at `app/Main.hs:298` and `src/DarcsWeb/Darcs.hs:196`. That means the proof boundary is drawn around blob/clone only, while the directory-listing edge remains unverified handwritten `FilePath` logic.
+
+The pure modules do not cover the expensive `darcs` IO boundary at all. There are no properties around `getRepoPatch`, `getBasicRepoInfo`, or `getRepoTags` that constrain work done per request, stop early on a match, or assert graceful failure on unreadable repositories.
+
+Additional QuickCheck-style properties I would add:
+
+- For arbitrary repo/path names, rendered HTML never contains unescaped raw `<`, `>`, `"`, or `'` from user-controlled labels in breadcrumb, table, and link contexts.
+- Tree, blob, and clone routes enforce the same hidden-path policy for every non-root subpath.
+- Filesystem failures on repo-controlled paths degrade to `Nothing` or empty metadata instead of throwing.
+- Clone serving only accepts the minimal darcs HTTP object set, not arbitrary `_darcs` files.
+
+I could not execute the test suite in this environment: with GHC 8.8.4 and cabal 3.0.0.0, `cabal test darcsweb-test` failed dependency resolution after selecting `scotty-0.30`, which requires `base >= 4.14`. That makes the current bounds in `darcsweb.cabal:42` too loose to guarantee reproducible verification here.
+
+## Hardening recommendations
+
+Introduce validated types for untrusted path fragments. `RepoName`, `TreeSubPath`, `BlobSubPath`, and `CloneObjectPath` should be smart constructors, not raw `Text`/`FilePath` values threaded through handlers. Right now the wrong state is still representable in `app/Main.hs:385` and `src/DarcsWeb/Darcs.hs:196`.
+
+Replace partial and locale-sensitive text handling. `app/Main.hs:96` should use `readMaybe` for `port`, and repo/config file reads should move from lazy `String` IO (`src/DarcsWeb/Config.hs:20`, `src/DarcsWeb/Darcs.hs:103`, `src/DarcsWeb/Darcs.hs:249`) to strict `ByteString` plus explicit decoding.
+
+Put caches or precomputed indexes in front of `readPatches` on hot routes. The current request path repeatedly opens repositories and walks entire histories for metadata and tag views (`src/DarcsWeb/Darcs.hs:107`, `src/DarcsWeb/Darcs.hs:155`).
+
+Tighten dependency bounds or pin a tested solver plan. `darcsweb.cabal:44` is broad enough that the build becomes environment-sensitive; a `cabal.project.freeze`, an explicit upper bound known to work with GHC 8.8, or CI-tested matrix bounds would make verification repeatable.
+
+Reconsider `-rtsopts` for production binaries in `darcsweb.cabal:41` and `darcsweb.cabal:64`. If runtime RTS tuning is not an explicit deployment requirement, disabling or restricting it is the safer default.
addfile ./reviews/codex-webperf.md
hunk ./reviews/codex-webperf.md 1
+# Mobile-First Web Performance Review ��� darcsweb
+
+## Summary
+The project already has a viewport meta tag and some responsive CSS hooks, so this is not starting from a desktop-only baseline. The biggest problems are server-side: the repository summary and patch-detail paths do much more darcs work than their UI implies, so TTFB will scale poorly with repository count and history size. On mobile, the Haskell-generated HTML still leans on desktop tables and generic `div` wrappers, which leaves narrow-screen views under-labeled and less semantic than they should be.
+
+## Mobile rendering issues
+- The repo list, shortlog, tags, and tree pages emit plain tables with header-only labels. Once the existing stylesheet collapses those tables for small screens, the Haskell markup provides no inline labels, `caption`, or `data-label` fallback, so date/author/size values lose context on a 360px phone. `src/DarcsWeb/Html.hs:68`, `src/DarcsWeb/Html.hs:154`, `src/DarcsWeb/Html.hs:262`, `src/DarcsWeb/Html.hs:312`
+- The summary page���s only ���see more��� affordance for recent activity is the literal text `...`, which is both a weak tap target and easy to miss on touch devices. Mobile-first navigation needs a full-label control here. `src/DarcsWeb/Html.hs:109`
+- Blob and diff pages always render raw `<pre>` blocks for file contents and diffs. That preserves fidelity, but it also guarantees horizontal panning for long lines instead of offering a mobile-readable fallback. `src/DarcsWeb/Html.hs:241`, `src/DarcsWeb/Html.hs:361`
+
+## Performance issues
+- The repository summary route does three expensive metadata walks on every request: it rescans all repositories to rediscover the current repo, then rereads the target repository once for recent patches and once again for tags. On a host with many repositories or long histories, `/repo/:name/summary` TTFB will be dominated by repeated `listRepos` and `readPatches` work. `app/Main.hs:238`, `app/Main.hs:245`, `app/Main.hs:246`, `src/DarcsWeb/Darcs.hs:63`, `src/DarcsWeb/Darcs.hs:108`, `src/DarcsWeb/Darcs.hs:123`, `src/DarcsWeb/Darcs.hs:155`
+- `getRepoPatch` computes full diff text before it knows it has found the requested patch hash. The current `mapRL extractPatchFull` plus `filter` shape can render large diffs for many non-matching patches just to serve one detail page. `src/DarcsWeb/Darcs.hs:136`, `src/DarcsWeb/Darcs.hs:139`, `src/DarcsWeb/Darcs.hs:263`
+- HTML escaping is on the hot path and still goes through `Text -> String -> Text` conversion. `highlightDiff` then repeats that work per line, so the largest pages pay the highest avoidable allocation cost. `src/DarcsWeb/Html.hs:413`, `src/DarcsWeb/Html.hs:418`, `gen/HtmlPure.hs:37`
+- Blob and repository-description reads still use lazy `readFile` into `String` and then `T.pack`, which duplicates content in memory and defers I/O/exception timing. That is directly on the response path for blob views and indirectly on the index/summary pages. `src/DarcsWeb/Darcs.hs:103`, `src/DarcsWeb/Darcs.hs:249`
+- Clone traffic is forced through `Cache-Control: no-cache` for every `_darcs` object. That prevents clients and intermediaries from reusing hashed immutable clone artifacts efficiently, so repeat clones and resumed downloads pay extra RTTs. `app/Main.hs:434`, `app/Main.hs:435`
+
+## Design & semantics
+- Page chrome and primary content are built from generic `div` wrappers instead of semantic landmarks such as `<header>`, `<main>`, `<footer>`, and full-log entries are not `<article>` elements. That weakens landmark navigation and makes the HTML structure less coherent than the visual design suggests. `src/DarcsWeb/Html.hs:39`, `src/DarcsWeb/Html.hs:45`, `src/DarcsWeb/Html.hs:48`, `src/DarcsWeb/Html.hs:195`
+- The repository index has no visible `<h1>` at all; it jumps straight from page chrome to a table or an empty-state paragraph. Screen-reader and keyboard users lose the page���s primary heading, and narrow-screen users get less orientation than they should. `src/DarcsWeb/Html.hs:56`, `src/DarcsWeb/Html.hs:60`, `src/DarcsWeb/Html.hs:68`
+- Several tables ship unlabeled structural columns via empty `<th>` cells. That creates blank headers for assistive tech and makes the mobile-collapsed table views harder to interpret. `src/DarcsWeb/Html.hs:72`, `src/DarcsWeb/Html.hs:156`, `src/DarcsWeb/Html.hs:263`, `src/DarcsWeb/Html.hs:313`
+- Author presentation is inconsistent across pages: shortlog and tags collapse to the display name, while full log and patch detail render the raw author field. That creates avoidable visual inconsistency and increases the chance of long-address overflow on narrow layouts. `src/DarcsWeb/Html.hs:166`, `src/DarcsWeb/Html.hs:205`, `src/DarcsWeb/Html.hs:229`, `src/DarcsWeb/Html.hs:277`
+
+## Quick wins
+1. Stop rescanning every repository from the summary route and fetch only the current repo���s metadata. In `app/Main.hs`, replace the `listRepos`/`findRepo` block with a dedicated single-repo helper, for example:
+ ```hs
+ -- app/Main.hs
+ ri <- liftIO $ getSingleRepoInfo repoPath name
+ patches <- liftIO $ getRepoPatches repoPath
+ tags <- liftIO $ getRepoTags repoPath
+ html $ TL.fromStrict $ renderRepoSummary now name ri clone (take 10 patches) tags
+ ```
+ Then extract the current `checkRepo` logic in `src/DarcsWeb/Darcs.hs` into `getSingleRepoInfo :: FilePath -> Text -> IO RepoInfo`. This removes the cross-repository scan from the hot summary path. `app/Main.hs:238`, `src/DarcsWeb/Darcs.hs:69`
+2. Make patch lookup hash-first and diff-second. In `src/DarcsWeb/Darcs.hs`, compare `targetHash` against `makePatchname (info piap)` before calling `extractPatchFull`, e.g.:
+ ```hs
+ let patchHashOf piap = T.pack (BC.unpack (sha1Show (makePatchname (info piap))))
+ case find (\piap -> patchHashOf piap == targetHash) (mapRL id patchRL) of
+ Just piap -> pure (Just (extractPatchFull piap))
+ Nothing -> pure Nothing
+ ```
+ That keeps full diff rendering off the non-matching path. `src/DarcsWeb/Darcs.hs:136`, `src/DarcsWeb/Darcs.hs:139`, `src/DarcsWeb/Darcs.hs:263`
+3. Split clone caching by object type instead of sending `no-cache` for everything. In `serveClone`, give hashed paths a long immutable policy and keep inventory-like files revalidating:
+ ```hs
+ if "patches/" `isPrefixOf` subPath || "pristine.hashed/" `isPrefixOf` subPath
+ then setHeader "Cache-Control" "public, max-age=31536000, immutable"
+ else setHeader "Cache-Control" "no-cache"
+ ```
+ This is a large win for repeat clone traffic over slow links. `app/Main.hs:420`, `app/Main.hs:435`
+4. Replace the `Text`/`String` HTML escape bridge with a Text-native builder. A minimal direction in `src/DarcsWeb/Html.hs` is:
+ ```hs
+ esc :: Text -> Text
+ esc = TL.toStrict . TB.toLazyText . T.foldl' step mempty
+ ```
+ with `step` appending escaped entities directly. After that, rewrite `highlightDiff` to build one builder instead of concatenating escaped `Text` fragments. `src/DarcsWeb/Html.hs:413`, `src/DarcsWeb/Html.hs:418`, `gen/HtmlPure.hs:37`
+5. Replace `String`-based file reads with strict text or bytes reads. For example, in `src/DarcsWeb/Darcs.hs`:
+ ```hs
+ import qualified Data.Text.IO as TIO
+
+ readRepoDescription path = if exists then T.strip <$> TIO.readFile descFile else pure ""
+ getRepoBlob ... = Just <$> TIO.readFile fullPath
+ ```
+ If repository files may be non-UTF-8, use `Data.ByteString.readFile` and an explicit decode policy instead of `Prelude.readFile`. `src/DarcsWeb/Darcs.hs:103`, `src/DarcsWeb/Darcs.hs:249`
+6. Fix the narrow-screen affordances in the generated HTML: change the summary link text from `...` to `View full shortlog`, add a real `<h1>` to the index page, and add either `caption` or `data-label` hooks to each mobile-collapsed table. Those are small template changes with outsized usability impact. `src/DarcsWeb/Html.hs:56`, `src/DarcsWeb/Html.hs:68`, `src/DarcsWeb/Html.hs:109`, `src/DarcsWeb/Html.hs:154`, `src/DarcsWeb/Html.hs:262`, `src/DarcsWeb/Html.hs:312`
+
+## Deeper restructuring (optional)
+- Keep a per-repository metadata snapshot in memory, refreshed on a timer or via mtime checks, so index and summary pages stop reopening repositories and recounting patch histories on demand. `app/Main.hs:230`, `src/DarcsWeb/Darcs.hs:108`
+- Move HTML generation to a builder- or template-based layer that emits semantic landmarks by default and can stream larger responses. That would solve the current `div`-heavy structure and remove most of the `Text` concatenation overhead in one pass. `src/DarcsWeb/Html.hs:31`, `src/DarcsWeb/Html.hs:195`, `src/DarcsWeb/Html.hs:413`
addfile ./reviews/post-claude-bugs.md
hunk ./reviews/post-claude-bugs.md 1
+# Post-change bug review (Claude)
+
+Scope: uncommitted darcs diff against `app/Main.hs`, `src/DarcsWeb/Config.hs`,
+`src/DarcsWeb/Darcs.hs`, `src/DarcsWeb/Html.hs`, `test/Properties/Clone.hs`,
+`test/Properties/Html.hs`. Every finding below is checked against the 11-item
+`reviews/codex-reconciliation.md` fix list, the current working tree, and the
+formally-verified helpers in `gen/` / `verified/`.
+
+## Findings
+
+### 1. LOW ��� Missing regression test for tree-subpath safety gate
+- **File / line**: `src/DarcsWeb/Darcs.hs:307-309`, `test/Properties/Clone.hs` (absent)
+- **What is wrong**: The reconciliation list (item 2, `HIGH`) explicitly
+ requested "a small property test for representative unsafe cases" to pin the
+ new `isSafeSubPath` guard inside `getRepoTree`. No such test was added; the
+ test module still only covers `isSafeSubPath` / `isCloneSubPath` directly,
+ not the `getRepoTree`-level early rejection.
+- **Why it can fail in practice**: A future refactor that inlines or reorders
+ the guard (for example, moving the `isDir <- doesDirectoryExist fullDir`
+ call above the safety check) would silently reintroduce the
+ `_darcs`/dotfile disclosure path. There is no automated signal to catch that
+ regression.
+- **Minimal fix**: Add a `Properties.Clone` (or new `Properties.Tree`)
+ property such as
+
+ getRepoTree repoPath "_darcs/prefs" === Nothing
+ getRepoTree repoPath "../etc" === Nothing
+ getRepoTree repoPath ".ssh/id_rsa" === Nothing
+
+ against a temporary repo fixture (or a pure helper that exposes just the
+ `isSafeSubPath`-based precondition).
+
+### 2. LOW ��� Missing regression test for tree / blob breadcrumb encoding
+- **File / line**: `src/DarcsWeb/Html.hs:318-354`, `test/Properties/Html.hs` (absent)
+- **What is wrong**: The reconciliation list (item 3, `MED`) rewrote
+ `renderTreeBreadcrumb` and `renderBlobBreadcrumb` to fix the
+ HTML-injection / URL-injection sink documented in codex Q1, introducing
+ the new `pathSegments` / `encodePathList` helpers. No property in
+ `test/Properties/Html.hs` asserts that a label containing `<`, `"`, `?`,
+ `#` is HTML-escaped in the visible part and percent-encoded in the
+ `href`. The only test touching the rewrite is `prop_esc_matches_verified`,
+ which covers `esc` alone.
+- **Why it can fail in practice**: The breadcrumb functions are not exported,
+ and a future simplification that returns to raw concatenation in either
+ function would not be caught by any test. Given that this code was the
+ stored-HTML-injection sink that motivated the rewrite, it should be pinned.
+- **Minimal fix**: Export `renderTreeBreadcrumb` / `renderBlobBreadcrumb`
+ (or a thin wrapper) from `DarcsWeb.Html` behind an
+ `-- Exported for testing` block and add properties asserting
+ `not (T.isInfixOf "<script" (renderTreeBreadcrumb "r" "<script>"))` and
+ `T.isInfixOf "%3F" (renderTreeBreadcrumb "r" "a?b")`.
+
+### 3. LOW ��� `renderBlobBreadcrumb` degrades to a dangling trailing " / " on an empty sub-path
+- **File / line**: `src/DarcsWeb/Html.hs:337-354`
+- **What is wrong**: When `subPath` is empty, `pathSegments subPath` returns
+ `[]`, the `case reverse parts` branch binds `(dirParts, fileName) = ([], "")`,
+ and the function renders `��� / <a ���>[root]</a> / </div>`. The final
+ `" / " <> esc ""` leaves a visible trailing separator with no file name.
+- **Why it can fail in practice**: The blob route is
+ `^/repo/([^/]+)/blob/(.+)$`, so Scotty will not normally pass an empty
+ sub-path. However, the helper is total and could be reused from a future
+ call site (e.g. a new blob-root handler) where the input is empty; the
+ current shape produces subtly broken markup without any error signal.
+ Not security-relevant because `esc ""` is inert.
+- **Minimal fix**: Guard the empty case explicitly, e.g.
+
+ renderBlobBreadcrumb _ subPath | T.null subPath =
+ "<div class=\"tree-path\"><a href=\"/repo/" <> ��� <> "/tree\">[root]</a></div>\n"
+
+ or, equivalently, omit the `" / " <> esc fileName` tail when `fileName` is
+ empty.
+
+### 4. LOW ��� `getRepoSummary` forces full `PatchSummary` rendering for every patch
+- **File / line**: `src/DarcsWeb/Darcs.hs:196-222`
+- **What is wrong**: `allSummaries = mapRL extractPatchListing patchRL` builds
+ one `PatchSummary` per patch, and `PatchSummary`'s fields are all strict
+ (`src/DarcsWeb/Types.hs:12-22`). `count = length allSummaries` forces only
+ the list spine, but the subsequent `tags = filter psIsTag allSummaries`
+ forces each element to WHNF, which in turn forces
+ `psSummary = T.pack $ renderString $ summary p` for every single patch in
+ the repository, regardless of whether it is a tag or ever displayed.
+- **Why it can fail in practice**: This changes the asymptotic work of the
+ `summary` route compared to the previous `getBasicRepoInfo` (which used
+ only `mapRL info`) plus bounded `getRepoPatches`. On repos with tens of
+ thousands of patches the summary page becomes noticeably slower and uses
+ more memory, in exchange for a single-pass API. This is functionally
+ equivalent to the old `getRepoTags` cost (which also rendered summaries
+ for every patch), so it is not a regression versus the combined old
+ path ��� but the reconciliation list (item 7) framed this as a PERF
+ improvement and it only partially delivers one.
+- **Minimal fix**: Compute tags with `mapRL info` first, then call
+ `extractPatchListing` only on the recent / tag subset, e.g.
+
+ let infos = mapRL info patchRL
+ count = length infos
+ lastDate = case infos of (i:_) -> T.pack (piDateString i); _ -> ""
+ tagInfos = filter isTag infos
+ tags <- ���render on demand���
+ recent <- ���render for take maxPatchCount only���
+
+ so the `O(n)` summary rendering work is avoided when the user never opens
+ log / patch detail.
+
+### 5. LOW ��� Lazy fields inside returned `PatchSummary` lists can still throw outside `try`
+- **File / line**: `src/DarcsWeb/Darcs.hs:130-140` (`getRepoPatches`),
+ `src/DarcsWeb/Darcs.hs:174-188` (`getRepoTags`),
+ `src/DarcsWeb/Darcs.hs:200-222` (`getRepoSummary`)
+- **What is wrong**: All three helpers call `withRepositoryLocation ��� $
+ RepoJob $ \repository -> do` inside `try`, force only the list spine with
+ `evaluate (length ���)`, and return the list. Patch-content access goes
+ through `hopefullyM piap` which reads patch data from disk; with strict
+ `PatchSummary` fields this data is forced per element when the *renderer*
+ walks the list, i.e. after the `try` frame has popped and the repository
+ handle has been released. An `IOException` inside the darcs patch reader
+ escapes to Scotty as a 500.
+- **Why it can fail in practice**: A race with external `darcs optimize`,
+ filesystem ENOSPC / permissions change, or a truncated patch file on disk
+ results in an uncaught exception on the summary / log / patch routes.
+ This is a pre-existing pattern, not introduced by this diff, but the diff
+ widened the surface (new `getRepoSummary`) without tightening the
+ evaluation story.
+- **Minimal fix**: Fully evaluate the returned `PatchSummary` list inside
+ the `try` frame, for example with
+ `let r = ���; evaluate (rnf r)` (adding `Control.DeepSeq.NFData` instances
+ to `PatchSummary`) or with `mapM evaluate` at the call site before
+ returning.
+
+### 6. LOW ��� Host header fallthroughs and port parsing are not covered by tests beyond what existed before
+- **File / line**: `app/Main.hs:127-133` (`parsePort`)
+- **What is wrong**: The new `parsePort` helper has no QuickCheck or unit
+ coverage. The patch deleted the partial `read` but did not add any test
+ that pins the accepted / rejected set (empty string, trailing comment,
+ negative, 0, 65536, leading whitespace, hex).
+- **Why it can fail in practice**: A future refactor to `parsePort` (e.g.
+ allowing a hex prefix, or accepting port 0 for OS-assigned ports) would
+ not be detected. Low severity because the function is startup-only and
+ the failure mode is a clean `exitFailure`, not a crash at request time.
+- **Minimal fix**: Add a pure helper
+
+ parsePortPure :: String -> Maybe Int
+ parsePortPure s = readMaybe s >>= \p -> guard (p > 0 && p < 65536) >> pure p
+
+ and cover it with a QuickCheck property in `test/Properties/Clone.hs`
+ (or a new `Properties.Config` module).
+
+## Residual risks
+
+- **`isSafeSubPath` does not reject HTML-special characters** (e.g. `<`,
+ `>`, `"`, `&`, `?`, `#`). Defense against those classes depends on
+ `encodePathSegment` / `esc` at render time and on `jailCheck` at IO time.
+ This is a layered defense, not a bug, but it means future renderers must
+ be careful to route new URL construction through `encodePathSegment`
+ rather than `esc`.
+- **TOCTOU between `doesFileExist canonical` and `file canonical`** in
+ `serveStatic` (`app/Main.hs:382-388`) and `serveClone`
+ (`app/Main.hs:447-453`). The `safeCanonicalize` addition does not close
+ this window; symlink replacement between the check and the serve is still
+ possible. Pre-existing and flagged only as a residual risk.
+- **`getRepoBlob` does not reject subpaths starting with `/`** via
+ `isSafeSubPath` (the verified helper filters empty segments before
+ checking). Safety relies entirely on `jailCheck` catching the absolute
+ path after `System.FilePath.</>` replaces the first operand. This is
+ correct today, but the layered defense is not expressed in tests.
+- **Scotty pathParam is URL-decoded**; a NUL byte in a repo name is not
+ explicitly rejected by `withRepo` (only `/`, `\`, `..`, leading `.` are).
+ If a reverse proxy allowed `%00` through, `isDarcsRepo` would raise an
+ `IOException` that escapes to a framework 500. Pre-existing.
+
+## Missing test coverage
+
+- No test that `getRepoTree` rejects `_darcs`, `../..`, `.git`,
+ dot-prefixed, or absolute sub-paths (covers reconciliation item 2).
+- No test that `renderTreeBreadcrumb` / `renderBlobBreadcrumb`
+ percent-encode `?`, `#`, space in hrefs and HTML-escape `<` in labels
+ (covers reconciliation item 3).
+- No test that `getRepoBlob` returns the `binaryPlaceholder` for a file
+ containing a NUL byte in the first 8 KiB, or for a file whose bytes are
+ not valid UTF-8.
+- No test that `isCloneSubPath` rejects `_darcs`-nested payloads such as
+ `inventories/_darcs/config` ��� the `prop_clone_allowlist_refines_safe`
+ property is a tautology because `isCloneSubPath` short-circuits on
+ `isSafeSubPath`.
+- No test for `parsePort`'s accepted / rejected set.
+- No test for `safeCanonicalize` returning `Nothing` on IO errors (e.g. a
+ NUL byte in the candidate path) ��� the whole fix for reconciliation
+ item 5 rests on that behavior.
+
+## Changed behavior that remains unverified
+
+- The new one-pass `getRepoSummary` (reconciliation item 7) changes the
+ order in which `withRepositoryLocation`, `readPatches`, tag filtering,
+ and description reading run. No regression test asserts that the
+ returned `RepoInfo.riLastChange` and `riPatchCount` match what
+ `listRepos` + `getRepoPatches` + `getRepoTags` produced before.
+- The native `esc` rewrite is pinned by `prop_esc_matches_verified`, but
+ the callers that previously used `HtmlPure.esc` indirectly (e.g. via
+ `highlightDiff`) have no separate property asserting their output is
+ unchanged byte-for-byte.
+- `isCloneSubPath`'s use of `splitPath` (which does not filter empty
+ segments) means a subpath containing `//` or a leading `/` may be
+ rejected even if the same data would pass `isSafeSubPath`. Whether that
+ asymmetry is intentional is not captured by any property.
addfile ./reviews/post-claude-efficiency.md
hunk ./reviews/post-claude-efficiency.md 1
+# Post-change efficiency review ��� darcsweb
+
+Scope: the uncommitted darcs changes on `app/Main.hs`, `src/DarcsWeb/Config.hs`,
+`src/DarcsWeb/Darcs.hs`, `src/DarcsWeb/Html.hs`, `test/Properties/Clone.hs`,
+`test/Properties/Html.hs` as captured in `/tmp/darcs-diff.txt`.
+
+The diff contains three clear efficiency wins (one-pass summary, hash-first
+patch lookup, native `Text` `esc`) plus a few new allocation hotspots
+introduced by the breadcrumb refactor. Findings are ordered by severity.
+
+---
+
+## Findings
+
+### F1 ��� MED: `esc` emits one `Text` per character on the render hot path
+
+1. Severity: MED
+2. File and line: `src/DarcsWeb/Html.hs:464`-`src/DarcsWeb/Html.hs:472`
+3. Inefficiency: The new `esc = T.concatMap escChar` is strictly faster than
+ the previous `T.pack . HtmlPure.esc . T.unpack` bridge (no `String`
+ intermediate, no `unpack/pack`, no per-character `(:)` list cells on the
+ boxed heap), so the change is a net win ��� but the chosen implementation
+ still allocates one fresh `Text` per non-special character (`escChar c =
+ T.singleton c`) and `concatMap` glues them together. `Data.Text`'s
+ `concatMap` is internally `concat . map`, so escaping a 1 MiB diff
+ produces ~1 M single-character `Text` values and then a concat. That is
+ why `esc` remains the dominant cost on the patch-detail page even after
+ this rewrite.
+4. When it becomes a real problem: any page that HTML-escapes large `Text`,
+ in particular `renderPatchDetail` ��� `highlightDiff (psDiff ps)` at
+ `src/DarcsWeb/Html.hs:255` and `src/DarcsWeb/Html.hs:476`-`src/DarcsWeb/Html.hs:483`.
+ For a repository with multi-megabyte diffs this quickly dominates render
+ time and transient heap residency.
+5. Optimization direction: replace `T.concatMap escChar` with a single
+ pass that finds the next escapable char via `T.break (\c -> c == '<' ||
+ c == '>' || c == '&' || c == '"' || c == '\'')`, appends the unmodified
+ prefix in one go, then the escape, and recurses. Equivalence with
+ `HtmlPure.esc` is already pinned by
+ `test/Properties/Html.hs:39`-`test/Properties/Html.hs:41`, so the swap is
+ safe. Alternatively build into a `Data.Text.Lazy.Builder` and force once
+ at the end; this would also let `highlightDiff` stream without
+ `T.concat . map ... . T.lines` at `src/DarcsWeb/Html.hs:477`.
+
+ Verdict on the original question "is native `esc` actually faster than
+ the `String` round-trip": yes, clearly, but the remaining per-char
+ allocation keeps `esc` on the hot path.
+
+---
+
+### F2 ��� MED: `getRepoSummary` still materialises every patch's summary text just to count them
+
+1. Severity: MED
+2. File and line: `src/DarcsWeb/Darcs.hs:200`-`src/DarcsWeb/Darcs.hs:222`
+ (specifically `allSummaries = mapRL extractPatchListing patchRL` at 205
+ and `count = length allSummaries` forced at 210).
+3. Inefficiency: `getRepoSummary` computes `allSummaries` via
+ `mapRL extractPatchListing patchRL`, where `extractPatchListing` calls
+ `renderString $ summary p` per patch (`src/DarcsWeb/Darcs.hs:385`-`src/DarcsWeb/Darcs.hs:392`).
+ Then `evaluate count` forces the full list, so every patch ��� not just
+ the first `maxPatchCount` ��� is rendered to produce a summary `Text`,
+ even though the summary page only displays `take 10 recent` (see
+ `app/Main.hs:257`) and the tag list. Compared with the old
+ `listRepos + getRepoPatches + getRepoTags` trio this is still a net win
+ because the old code did two full traversals (`getRepoPatches` rendered
+ 500 listings, `getRepoTags` rendered `mapRL extractPatchListing` over
+ the full set), but the new hot path still pays for rendering a listing
+ for every patch ��� i.e. `count` and `tags` on a 50 k���patch repo force
+ 50 k `renderString` calls per summary request.
+4. When it becomes a real problem: summary requests on large repos (tens
+ of thousands of patches) or under load. Users hitting `/repo/X/summary`
+ pay full O(N) summary rendering for every request.
+5. Optimization direction:
+ - For `count`, use `lengthRL patchRL` (or equivalently
+ `length (mapRL info patchRL)`) ��� it avoids calling
+ `extractPatchListing`, which is what makes the per-element work
+ expensive. `PatchInfo` is already cheap.
+ - For `tags`, filter on `info` first (using `isTag`) and only run
+ `extractPatchListing` on the tags. The equivalent already exists in
+ `getBasicRepoInfo` at `src/DarcsWeb/Darcs.hs:115`-`src/DarcsWeb/Darcs.hs:127`
+ (maps `info` only).
+ - `recent` already uses `take maxPatchCount`, which is fine.
+ - The intent "one traversal instead of three" is preserved; the change
+ is just to separate the cheap `PatchInfo` traversal from the
+ expensive summary rendering.
+
+ Verdict on the original question "does `getRepoSummary` actually save
+ work vs the old 3-call summary": yes, it removes the `listRepos` scan
+ of every repository in `cfgRepoDir` and deduplicates one
+ `readPatches`, which are the big-O wins. But it regresses one sub-axis
+ (all patches are now rendered to listings rather than just
+ `maxPatchCount + all-tags`), which is why the above tightening
+ matters.
+
+---
+
+### F3 ��� LOW/MED: Tree- and blob-breadcrumb builders are quadratic in path depth
+
+1. Severity: LOW���MED (depth-dependent)
+2. File and line: `src/DarcsWeb/Html.hs:322`-`src/DarcsWeb/Html.hs:326`
+ and `src/DarcsWeb/Html.hs:344`-`src/DarcsWeb/Html.hs:347`.
+3. Inefficiency: Both breadcrumb builders use
+ `scanl (\acc p -> acc ++ [p]) [] parts` to build the list of cumulative
+ prefixes, then call `encodePathList prefix` (which re-encodes every
+ segment from scratch via `T.intercalate "/" . map encodePathSegment`,
+ `src/DarcsWeb/Html.hs:363`-`src/DarcsWeb/Html.hs:364`) on each prefix.
+ `acc ++ [p]` is O(n) in the prefix length, so `scanl` is O(k��) in list
+ cells for a path of depth k, and the per-crumb re-encoding via
+ `encodePathList` makes the total work O(k�� �� avg_segment_encode_cost).
+4. When it becomes a real problem: deep tree/blob paths (say 10+
+ directory components) or adversarial requests that push the depth
+ high. For typical repos this is dwarfed by the `esc` cost, but the
+ algorithmic shape is still worse than the old linear `buildCrumbs`
+ recursion that extended `acc` by appending `Text` on each step. This
+ is a regression in asymptotic shape introduced by the refactor ���
+ small in absolute terms for realistic depths, but worth fixing while
+ the code is being touched.
+5. Optimization direction: build the href incrementally by carrying the
+ already-encoded prefix as `Text` instead of a `[Text]`, e.g. using
+ `foldl'` with an accumulator of type `(Text, Text)` where the first
+ component is the cumulative percent-encoded path and the second is
+ the rendered breadcrumb `Text`. That drops the per-step `acc ++ [p]`
+ and the repeated `map encodePathSegment` over the entire prefix.
+
+---
+
+### F4 ��� LOW: `renderTreeRow` recomputes `pathSegments subPath` and
+`encodePathSegment repoName` per entry
+
+1. Severity: LOW
+2. File and line: `src/DarcsWeb/Html.hs:388`-`src/DarcsWeb/Html.hs:406`.
+3. Inefficiency: Inside `map (renderTreeRow repoName subPath) entries`
+ (called from `renderTreeTable` at `src/DarcsWeb/Html.hs:384`), every
+ row recomputes `nameSeg = encodePathSegment repoName` and
+ `entrySegs = pathSegments subPath ++ [name]`. Both are constant in
+ `repoName`/`subPath` across the whole page, but are re-evaluated per
+ entry because they are bound inside `renderTreeRow`. For a directory
+ with N files, that is N redundant path-segment splits plus N
+ redundant percent-encodings of `repoName`.
+4. When it becomes a real problem: large directories (hundreds to
+ thousands of entries). The cost is small per row, but it is pure
+ duplicated work introduced by the refactor ��� the pre-change code
+ called `esc repoName` per row as well, so this is a wash in number
+ of per-row string walks but strictly more work than necessary.
+5. Optimization direction: hoist `nameSeg` and `pathSegments subPath`
+ to `renderTreeTable` and pass them in. The same pattern applies (to
+ a smaller degree) to `renderShortLogRow`, `renderFullLogEntry`,
+ `renderTagRow`, and `renderRepoRow`, which each re-run
+ `encodePathSegment repoName` per row ��� acceptable, but if
+ `renderTreeTable` is fixed the others become mechanical to match.
+
+---
+
+### F5 ��� LOW: Matched patch hashes its `PatchInfo` twice in `getRepoPatch`
+
+1. Severity: LOW
+2. File and line: `src/DarcsWeb/Darcs.hs:163`-`src/DarcsWeb/Darcs.hs:166`
+ together with `src/DarcsWeb/Darcs.hs:406`-`src/DarcsWeb/Darcs.hs:412`.
+3. Inefficiency: `selectMatching` calls `patchHashText (info piap)` to
+ test against `targetHash`, and `extractPatchFull`���`patchInfoToSummary`
+ stores the same hash in `psHash` via another `patchHashText pinfo`
+ call. For the matched patch, the SHA-1 is computed twice. This is
+ not hot ��� it is O(1) per request ��� but the allocator-visible call
+ pattern was changed in this diff and the duplication is new.
+4. When it becomes a real problem: essentially never on its own; noted
+ for completeness because the efficiency review was asked to focus on
+ `getRepoPatch`'s short-circuit.
+5. Optimization direction: thread the already-computed hash `Text`
+ into `extractPatchFull`, or have `selectMatching` return a
+ pre-filled `PatchSummary` with `psHash = targetHash`.
+
+ Verdict on the original question "does `getRepoPatch` short-circuit
+ correctly so that non-matching patches never trigger diff
+ rendering": **yes**. `selectMatching` computes the SHA-1 only and
+ wraps the expensive `extractPatchFull` inside the `Just` branch of
+ the hash equality test, so non-matching patches cost one SHA-1 per
+ `PatchInfo` and never enter `renderString $ showPatch ForDisplay p`.
+ This is the main correctness win of the refactor.
+
+---
+
+## Things that were checked and found OK
+
+- `safeCanonicalize` at `src/DarcsWeb/Darcs.hs:241`-`src/DarcsWeb/Darcs.hs:246`
+ adds one `try` per resolution; the IO cost is negligible next to the
+ underlying `canonicalizePath` syscall chain.
+- Strict `ByteString` blob read + `decodeUtf8'` at
+ `src/DarcsWeb/Darcs.hs:365`-`src/DarcsWeb/Darcs.hs:373` eliminates the
+ previous lazy-handle leak and is a strict efficiency win.
+- The NUL-byte sniff at `src/DarcsWeb/Darcs.hs:377`-`src/DarcsWeb/Darcs.hs:378`
+ uses `BS.take 8192` before `BS.elem`, so it is bounded regardless of
+ file size.
+- `isCloneSubPath` at `src/DarcsWeb/Darcs.hs:283`-`src/DarcsWeb/Darcs.hs:293`
+ runs a small `splitPath` + pattern match; O(1) in the request shape.
+- `parsePort` at `app/Main.hs:127`-`app/Main.hs:133` runs once at startup.
+- `cfgLookupDefault` now uses `fromMaybe` at
+ `src/DarcsWeb/Config.hs:40`; semantically identical, marginally less
+ allocation, not performance-relevant.
+- The Scotty route changes in `app/Main.hs` do not change the number of
+ passes over the repository.
+
+---
+
+## Paths worth benchmarking
+
+- `/repo/:name/summary` on a repository with a large patch count ��� to
+ validate both the net win over the old 3-call version and the
+ remaining summary-render cost flagged in F2.
+- `/repo/:name/patch/:hash` on a repository with many large patches ���
+ to confirm that non-matching requests no longer render every diff
+ (F5 context) and to measure `esc`/`highlightDiff` cost (F1).
+- `/repo/:name/tree/<deep-path>` ��� to confirm whether F3's O(k��) shape
+ is observable at realistic depths.
+
+## Load assumptions behind the review
+
+- Typical request rate is modest (single-user or small-team darcsweb
+ host), so per-request constant factors matter more than throughput.
+- Repositories can be large (thousands of patches; individual diffs
+ up to a few MiB). This is where F1 and F2 dominate.
+- No reverse-proxy cache is assumed in front of darcsweb; every
+ request hits the handlers.
+
+## Scalability questions not verifiable from code alone
+
+- Whether `mapRL` in the installed `darcs-2.18.5` is strict or lazy in
+ its output list. If it is lazy, `getRepoPatch`'s list comprehension
+ `[p | Just p <- selected]` with `(p:_)` already stops at the first
+ match with no further work. If it is strict, all N `PatchInfo`s are
+ walked and hashed regardless ��� still vastly cheaper than the old
+ "render all diffs" code, but not constant time.
+- Whether `renderString $ summary p` in `extractPatchListing` is a
+ real hot allocator on the measured repo sizes, or whether darcs
+ caches it internally via the `hopefullyM` branch. F2's fix is safe
+ either way, but its size depends on this.
addfile ./reviews/post-claude-quality.md
hunk ./reviews/post-claude-quality.md 1
+# Post-change code quality review ��� Claude
+
+Scope: `app/Main.hs`, `src/DarcsWeb/Config.hs`, `src/DarcsWeb/Darcs.hs`,
+`src/DarcsWeb/Html.hs`, `test/Properties/Clone.hs`, `test/Properties/Html.hs`.
+The diff implements the items agreed in `reviews/codex-reconciliation.md`.
+The changes are in good shape overall: the intended behavioral fixes are
+present, they align with the reconciled plan, tests have been extended to
+pin the new contracts, and the naming cleanups (`navLink`, `bucket`,
+named time constants, `fromMaybe`, `tshow`) make the surrounding code
+easier to read.
+
+The findings below are mostly secondary maintainability notes; none of
+them regress the correctness of the reconciled fixes.
+
+---
+
+## Must-fix
+
+No must-fix code quality issues. The reconciled changes are implemented
+cleanly and do not introduce any quality regression that warrants
+blocking the pass.
+
+---
+
+## Optional improvements
+
+### O1. `jailCheck` reintroduces an ad-hoc `dropTrailingSlash` instead of reusing a helper
+- Severity: LOW
+- File: `src/DarcsWeb/Darcs.hs:256`-`src/DarcsWeb/Darcs.hs:263`
+- Problem: `jailCheck` normalizes the jail with
+ `PathPure.add_trailing_slash`, then strips the trailing `/` back off
+ with a locally-defined `dropTrailingSlash` that reverses the string
+ twice to peel off a single terminal character. The original pre-diff
+ code expressed the same idea with `init jailSlash`, which was terser
+ and read more like the intent ("without trailing slash"). The new
+ form is correct but visibly more complicated than it needs to be, and
+ the helper is inlined here while the reverse helper
+ `addTrailingSlash` lives in `app/Main.hs:424`.
+- Why it matters: the two path-jail helpers (`addTrailingSlash` in
+ `Main`, `jailSlash/jailNoSlash` here) already live in two different
+ modules. Accreting a third flavor (`dropTrailingSlash`) inside a
+ `where` clause makes any future correctness audit check three
+ near-identical pieces of code. The next person to touch one is
+ likely to forget the other two.
+- Improvement: replace the `where`-local helper with either
+ `T.stripSuffix "/"` lifted to `FilePath`, or with
+ `if not (null jailSlash) && last jailSlash == '/' then init jailSlash else jailSlash`
+ (safe because `add_trailing_slash` always returns non-empty). Better
+ still: expose a single `jailRoots :: FilePath -> (FilePath, FilePath)`
+ helper next to `addTrailingSlash` and call it from both sites. Then
+ `jailCheck` collapses to
+ `canonical == rootNoSlash || rootSlash `isPrefixOf` canonical`.
+
+### O2. `getRepoSummary` returns a bare 3-tuple of homogeneously-typed lists
+- Severity: LOW
+- File: `src/DarcsWeb/Darcs.hs:196`-`src/DarcsWeb/Darcs.hs:222`; callsite
+ `app/Main.hs:254`.
+- Problem: the signature is
+ `IO (RepoInfo, [PatchSummary], [PatchSummary])`. The two
+ `[PatchSummary]` positions mean different things (recent patches vs.
+ tags) but the type cannot distinguish them. A caller that swaps the
+ two arguments when destructuring ��� or a future maintainer who
+ changes the return order to "tags first" to match a UI reshuffle ���
+ will compile cleanly and produce a visually plausible but wrong
+ page.
+- Why it matters: the pre-diff code had three separate calls with
+ three distinct return types (`[RepoInfo]`, `[PatchSummary]`,
+ `[PatchSummary]`) where the argument-order mistake was at least
+ forced to go through different names. The new consolidated helper
+ is a net win for performance but erases that type-level hint.
+- Improvement: return a small record, e.g.
+
+ data RepoSummary = RepoSummary
+ { rsInfo :: RepoInfo
+ , rsRecentPatches :: [PatchSummary]
+ , rsTags :: [PatchSummary]
+ }
+
+ or at minimum a `type RepoSummary = (RepoInfo, Recent, Tags)` with
+ two one-element `newtype`s. The call site at `app/Main.hs:254`
+ becomes self-documenting and the cost of reshuffling a future render
+ is paid by the compiler.
+
+### O3. `renderRepoSummary` duplicates `repoBreadcrumb` inline
+- Severity: LOW
+- File: `src/DarcsWeb/Html.hs:101`-`src/DarcsWeb/Html.hs:102`; definition at
+ `src/DarcsWeb/Html.hs:438`-`src/DarcsWeb/Html.hs:440`.
+- Problem: the breadcrumb string built inline here is character-for-
+ character identical to `repoBreadcrumb repoName`. Every other
+ render site (`renderShortLog` at `src/DarcsWeb/Html.hs:149`,
+ `renderFullLog` at `src/DarcsWeb/Html.hs:193`, `renderPatchDetail`
+ at `src/DarcsWeb/Html.hs:233`, `renderTags` at
+ `src/DarcsWeb/Html.hs:264`) already calls `repoBreadcrumb`, so
+ this is a solitary deviation.
+- Why it matters: if the breadcrumb separator, class name, or
+ encoding helper ever changes, this one call site will silently drift
+ and produce inconsistent navigation. The existing helper was
+ designed for exactly this purpose.
+- Improvement: replace the `let breadcrumbs = " / <a ..."` binding with
+ `let breadcrumbs = repoBreadcrumb repoName`. `nameSeg` is still
+ needed for the other hrefs in the body, so keep that binding.
+
+### O4. `renderTreeBreadcrumb` and `renderBlobBreadcrumb` share a nearly identical `crumb` helper
+- Severity: LOW
+- File: `src/DarcsWeb/Html.hs:318`-`src/DarcsWeb/Html.hs:354`.
+- Problem: both functions define the same local `crumb` with the same
+ `prefixes = drop 1 (scanl appendSeg [] ���)` pattern and the same
+ `let href = ��� in " / <a href=���">"`. The only structural difference
+ is whether the final path component is rendered as a link or as plain
+ text.
+- Why it matters: this is fresh duplication introduced by the percent-
+ encoding fix. A future tweak (e.g. adding `rel="nofollow"` on tree
+ crumbs or changing the separator) has to be edited in two places,
+ and a mistake will silently leave one breadcrumb inconsistent.
+- Improvement: extract a small helper, e.g.
+
+ treeCrumbs :: Text -> [Text] -> Text
+ treeCrumbs repoSeg segs =
+ let prefixes = drop 1 (scanl (\acc p -> acc ++ [p]) [] segs)
+ crumb prefix label =
+ let href = "/repo/" <> repoSeg <> "/tree/"
+ <> encodePathList prefix
+ in " / <a href=\"" <> esc href <> "\">"
+ <> esc label <> "</a>"
+ in T.concat (zipWith crumb prefixes segs)
+
+ then `renderTreeBreadcrumb` is `[root] <> treeCrumbs ���` and
+ `renderBlobBreadcrumb` is `[root] <> treeCrumbs ��� dirParts <> " / " <> esc file`.
+
+### O5. Hot-path `esc` regresses from formally-verified to test-pinned
+- Severity: LOW
+- File: `src/DarcsWeb/Html.hs:464`-`src/DarcsWeb/Html.hs:472`; property
+ at `test/Properties/Html.hs:39`-`test/Properties/Html.hs:41`.
+- Problem: the production `esc` is no longer the extracted Coq
+ implementation; the proof-bound guarantee is now only asserted by a
+ QuickCheck property with `maxSuccess = 200`. That is sufficient to
+ catch any symmetric mistake (a dropped branch, a typo in an entity
+ reference) since `esc` is character-by-character, but it no longer
+ catches the rarer case of a Text-specific encoding quirk (e.g. a
+ future refactor that switches `T.concatMap` for something with
+ different semantics on surrogates).
+- Why it matters: `esc` is a security-critical XSS sink. The previous
+ design relied on extraction to keep the implementation honest. That
+ property is now a test-level invariant only.
+- Improvement: keep this as is ��� the performance win is real and the
+ equivalence test is the right pin ��� but bump the
+ `prop_esc_matches_verified` sample count beyond the generic 200 and
+ add a targeted generator that oversamples the five escaped
+ characters so the replacement is exercised on every run. For
+ example:
+
+ prop_esc_matches_verified :: Property
+ prop_esc_matches_verified =
+ forAll (listOf (frequency [(5, elements "<>&\"'"), (1, arbitrary)])) $ \s ->
+ esc (T.pack s) == T.pack (HtmlPure.esc s)
+
+ Alternatively document in the module header that any future edit to
+ `esc` must be accompanied by re-running the property at a high
+ sample count.
+
+### O6. `prop_clone_allowlist_refines_safe` is the only new property that pins the full allowlist contract
+- Severity: LOW
+- File: `test/Properties/Clone.hs:214`-`test/Properties/Clone.hs:216`.
+- Problem: the property correctly states "`isCloneSubPath` ���
+ `isSafeSubPath`", which is the right invariant. But it is only
+ randomly sampled over `String` arbitraries. QuickCheck's default
+ `String` generator produces almost no `/`, `.`, `_`, or control
+ bytes; the odds of it generating something like
+ `"patches/foo"` or `"_darcs/format"` are effectively zero. In
+ practice this property is carried by the explicit example-based
+ properties `prop_clone_allowlist_*`. The "refines_safe" property
+ is therefore mostly green by accident.
+- Why it matters: a regression where `isCloneSubPath` accepts an unsafe
+ path with a realistic shape (`"inventories/../secret"`) would not be
+ caught by this refinement test; it depends on the example suite
+ staying in sync. The property reads as stronger than it actually is.
+- Improvement: either drop the property in favour of the existing
+ example tables, or give it a realistic generator that samples from
+ known segment vocabularies, e.g. `oneof [elements ["hashed_inventory",
+ "inventories","patches","pristine.hashed","packs","prefs","..",".git"],
+ ���]`, joined with `"/"`. The latter is a five-line change and makes
+ the refinement property do real work.
+
+### O7. `getRepoSummary` renders every patch summary solely to compute `count`
+- Severity: LOW (PERF, but small)
+- File: `src/DarcsWeb/Darcs.hs:204`-`src/DarcsWeb/Darcs.hs:211`.
+- Problem: the helper builds `allSummaries = mapRL extractPatchListing patchRL`
+ and then evaluates `length allSummaries`. `extractPatchListing`
+ allocates a `PatchSummary` plus renders the summary text for every
+ patch in history. The consumer only needs the count and the first
+ `maxPatchCount`/`tags` elements. Rendering the middle N-10
+ summaries is thrown away.
+- Why it matters: the whole reason for the one-pass builder was to
+ stop doing redundant work on the summary path. This is only a
+ secondary inefficiency, but it pattern-matches the bug this PR was
+ meant to close.
+- Improvement: count over `PatchInfo` (which is already loaded without
+ diff work) and only render `extractPatchListing` for the prefix that
+ feeds `recent`/`tags`:
+
+ let infos = mapRL info patchRL
+ count = length infos
+ recent = take maxPatchCount
+ (mapRL extractPatchListing patchRL)
+ tags = filter psIsTag recent -- or scan infos for isTag
+ lastDate = case infos of (i:_) -> T.pack (piDateString i); [] -> ""
+
+ If tags-beyond-`maxPatchCount` must remain visible, keep a separate
+ traversal with `isTag` at the `PatchInfo` level and only call
+ `extractPatchListing` on the tag entries. That preserves the current
+ visible behavior (`getRepoTags` at `src/DarcsWeb/Darcs.hs:177`) with
+ much less rendering cost.
+
+### O8. `renderPage`'s `breadcrumbs` parameter is still implicit "trusted HTML"
+- Severity: LOW
+- File: `src/DarcsWeb/Html.hs:32`-`src/DarcsWeb/Html.hs:55`; callers at
+ lines 102, 149, 193, 233, 264, 305, 420, 433.
+- Problem: `breadcrumbs` is injected verbatim at
+ `src/DarcsWeb/Html.hs:45`. Every current caller is careful to escape
+ the labels and percent-encode the hrefs, but the type does not
+ enforce that. A future caller that passes raw path text will
+ reintroduce the tree-breadcrumb HTML injection class this PR just
+ fixed.
+- Why it matters: the new helpers (`repoBreadcrumb`,
+ `renderTreeBreadcrumb`, `renderBlobBreadcrumb`) are the only
+ blessed producers, but nothing in the type system prevents another
+ `Text` from slipping in.
+- Improvement: define a `newtype Breadcrumb = Breadcrumb { unBreadcrumb :: Text }`
+ whose constructor is not exported. Export the three helpers as the
+ only way to build one, and change `renderPage`'s signature to take
+ `Breadcrumb`. This costs ~10 lines and converts the convention into
+ a compile-time invariant. A lighter alternative is to add a module
+ header comment stating "the `breadcrumbs` argument is raw HTML;
+ always build it via `repoBreadcrumb`, `renderTreeBreadcrumb`, or
+ `renderBlobBreadcrumb`".
+
+---
+
+## Areas that may become maintenance risks later
+
+- **Path normalization is spread across three modules.**
+ `app/Main.hs:424` (`addTrailingSlash`), `src/DarcsWeb/Darcs.hs:251`
+ (`jailCheck` with local `dropTrailingSlash`), and
+ `src/DarcsWeb/Html.hs:357` (`pathSegments`) all deal with slash /
+ segment handling. Combined with the verified `PathPure`, that's four
+ places. A consolidation pass later ��� e.g. a `DarcsWeb.Path` module
+ that owns all trailing-slash, segment, and encode helpers ��� would
+ reduce the blast radius of any future path-jail change.
+- **`parsePort` uses `show s` for the diagnostic** at
+ `app/Main.hs:131`. That correctly wraps the input in Haskell-style
+ quotes, but if the config value contains control characters they'll
+ be double-escaped (`"abc\n"`). Fine for operator-only diagnostics,
+ worth keeping in mind if the startup path ever gains structured
+ logging.
+- **`safeCanonicalize` swallows all exceptions**
+ (`src/DarcsWeb/Darcs.hs:241`-`src/DarcsWeb/Darcs.hs:246`). This is
+ the documented intent ��� request-time paths must not leak framework
+ 500s ��� but it also masks genuine configuration errors (e.g. a repos
+ directory that became unreadable). If a "why is nothing showing?"
+ operator incident ever happens, having `cfgLog` wired through for a
+ `Left e -> log` branch would pay off. Not a bug today, just a hook
+ to leave for later.
+- **`looksBinary` heuristic** at `src/DarcsWeb/Darcs.hs:377` only
+ checks for NUL. Any large-UTF-16-encoded text file with no NUL in
+ the first 8 KiB will pass, be fed to `TE.decodeUtf8'`, and fail ���
+ landing in the binary placeholder branch anyway. That's fine
+ behavior, but it means the two checks are somewhat redundant. If
+ later work wants to distinguish "binary" from "non-UTF-8 text",
+ consider using `charsetdetect` or at least logging which branch
+ rejected the file.
+
+## Testing / structure gaps worth watching
+
+- **`renderTreeBreadcrumb` and `renderBlobBreadcrumb` are not directly
+ exercised by a property test.** `Properties.Html` covers `esc`,
+ `formatSize`, `shortAuthor`, `formatAbsolute`, and `highlightDiff`,
+ but the two breadcrumb builders ��� which are the direct fix for the
+ prior HTML-injection finding ��� have no unit-level coverage. A
+ single property of the form
+ "`not (T.isInfixOf \"<\" subPath) ==> all (`T.isInfixOf` renderTreeBreadcrumb r subPath) (esc <$> pathSegments subPath)`"
+ plus an example asserting that `< >` in a segment appear as `< >`
+ in the rendered breadcrumb would pin the fix and catch any future
+ regression.
+- **`getRepoSummary` has no direct test.** Its error-path contract
+ ("`Left` from `try` degrades to empty metadata") is checked only by
+ the fact that the page renders; a unit test against a temp directory
+ with a malformed `_darcs/` would make the fallback behavior explicit.
+ Not urgent, but cheap to add next to the existing property suite.
+- **`isCloneSubPath`'s allowlist is duplicated between source and
+ test.** `src/DarcsWeb/Darcs.hs:286`-`src/DarcsWeb/Darcs.hs:293` and
+ `test/Properties/Clone.hs:173`-`test/Properties/Clone.hs:211` both
+ hard-code the same segment vocabulary. That's deliberate pinning,
+ but means a future relaxation (e.g. new darcs pack file) has to be
+ edited in two places. Consider exporting a single
+ `cloneAllowedTopLevel :: [String]` table from `DarcsWeb.Darcs` so the
+ tests reference the canonical list and only the shape-of-subpath
+ rules live as logic.
addfile ./reviews/post-claude-webapp.md
hunk ./reviews/post-claude-webapp.md 1
+# Post-change web application review ��� darcsweb
+
+Scope: the uncommitted darcs diff (`/tmp/darcs-diff.txt`) that touches
+`app/Main.hs`, `src/DarcsWeb/Config.hs`, `src/DarcsWeb/Darcs.hs`,
+`src/DarcsWeb/Html.hs`, `test/Properties/Clone.hs`, and
+`test/Properties/Html.hs`. Review focuses on mobile-first design,
+consistent behavior, and frontend performance; `static/style.css` was
+consulted only where a Haskell-side change has CSS implications.
+
+High-level verification
+- Semantic landmarks: the swap from `<div class="page-header/page-body/page-footer">`
+ to `<header>/<main>/<footer>` at
+ `src/DarcsWeb/Html.hs:41,47,50` and the `<div class="log-entry">` ���
+ `<article class="log-entry">` swap at `src/DarcsWeb/Html.hs:208,226`
+ are class-preserving and do not break the existing CSS (all selectors
+ target the classes, not the tag).
+- Percent-encoded hrefs: every `esc repoName` / `esc (psHash ps)` used
+ inside an `href=` has been replaced with
+ `encodePathSegment (...)` (see calls at `src/DarcsWeb/Html.hs:83,101,
+ 171-172,205-206,232,285-286,321,340,364,368,391,440,455`).
+ Scotty's regex routes read from WAI's `pathInfo`, which is already
+ percent-decoded, so a repo name or directory with `%`, `#`, `?`,
+ space or UTF-8 that was previously a broken link now routes back
+ cleanly. `pathSegments` at `src/DarcsWeb/Html.hs:357` guards
+ `init []` crashes in the blob breadcrumb for a single-segment path.
+- Favicon: `<link rel="icon" type="image/png" href="/static/darcs-logo.png">`
+ at `src/DarcsWeb/Html.hs:38` is correct; the file exists in
+ `static/darcs-logo.png` and the MIME table in
+ `app/Main.hs` (`.png` ��� `image/png`) matches the `type=`.
+- Top-level `<h1>` on the index: `src/DarcsWeb/Html.hs:62` renders the
+ configured title once per index page, giving every route a single
+ heading landmark. Escaping uses `esc`, which is correct.
+- `aria-current="page"`: `navLink` at `src/DarcsWeb/Html.hs:453-458`
+ emits `aria-current="page"` only in the `path == active` branch, and
+ every caller of `repoNavBar` passes a section name that matches one
+ of the five emitted links (`src/DarcsWeb/Html.hs:108,152,196,267,
+ 308,423`). Patch detail deliberately omits the nav (pre-existing).
+
+---
+
+## Consistency findings
+
+### C1. Empty breadcrumb `<nav aria-label="Breadcrumb">` rendered on index and 404
+- Severity: low
+- File and line: `src/DarcsWeb/Html.hs:45`, called with `breadcrumbs = ""`
+ from `src/DarcsWeb/Html.hs:60` (index) and
+ `src/DarcsWeb/Html.hs:434` (404).
+- Issue: `renderPage` always emits
+ `<nav class="breadcrumbs" aria-label="Breadcrumb">���</nav>` even when
+ `breadcrumbs` is empty. On the index page and on 404 the nav is
+ empty, but the landmark is present in the accessibility tree because
+ `aria-label` forces it to be exposed regardless of content.
+- Why it matters for users: screen-reader users landing on the index
+ or a 404 will hear a "Breadcrumb navigation" landmark announced with
+ zero items. The same pages on other routes do have crumb entries, so
+ the landmark set is inconsistent across pages.
+- Practical fix: in `renderPage`, only emit the breadcrumb `<nav>`
+ when `breadcrumbs` is non-empty, e.g.
+ `if T.null breadcrumbs then "" else "<nav class=���>" <> breadcrumbs <> "</nav>\n"`.
+ No CSS change needed.
+
+### C2. Breadcrumb contents start with a leading `" / "`
+- Severity: low
+- File and line: `src/DarcsWeb/Html.hs:102, 149, 193, 233, 264, 305,
+ 420` (every non-index page) and `src/DarcsWeb/Html.hs:440`
+ (`repoBreadcrumb`).
+- Issue: every crumb string begins with `" / <a ���>"`. Previously this
+ sat inside a `<nav class="breadcrumbs">` with no `aria-label`; now
+ `aria-label="Breadcrumb"` promotes it to a first-class landmark, so
+ the leading bare slash is the first thing a screen reader reads ���
+ "Breadcrumb navigation, slash, repo-name, link".
+- Why it matters for users: the inconsistency between the visible
+ separator (`" / "`) and the semantic meaning of the label worsens
+ now that the landmark is explicit. Also diverges from how
+ `renderTreeBreadcrumb` builds crumbs (no leading slash, anchored by
+ a `[root]` link).
+- Practical fix: have `repoBreadcrumb` return an unprefixed crumb
+ (`<a href=���>repo</a>`) and let callers prepend their own separator
+ only when chaining further segments; or render breadcrumbs as an
+ ordered list (`<ol>`) and drop the inline separators entirely.
+
+### C3. Tree breadcrumb ends with a trailing `" / "` when at the subtree root entry
+- Severity: low
+- File and line: `src/DarcsWeb/Html.hs:305` and
+ `src/DarcsWeb/Html.hs:318-332`.
+- Issue: `renderTree` composes a header-bar breadcrumb by concatenating
+ `repoBreadcrumb repoName <> " / tree" <> pathSuffix` where
+ `pathSuffix` is `" / " <> esc subPath`. The in-body tree breadcrumb
+ (the `.tree-path` rendered by `renderTreeBreadcrumb`) is built
+ independently from `pathSegments` and emits a distinct trail. Both
+ now live in the same page, each with different escaping rules
+ (header bar uses raw `esc subPath`, body uses per-segment
+ percent-encoded hrefs), which is visually and semantically
+ redundant.
+- Why it matters for users: a mobile user with a narrow viewport sees
+ the same path fragment rendered twice in two different styles, the
+ top one wrapping inside the sticky header, the bottom one scrolling
+ horizontally. Tapping them produces the same destinations, but the
+ URLs built by each branch are built by different code paths and
+ can drift.
+- Practical fix: drive both from `pathSegments` + `encodePathList` so
+ that the two trails cannot disagree, or drop the in-body
+ `.tree-path` when the header-bar breadcrumb already carries the
+ subpath.
+
+### C4. Index `<h1>` duplicates the value in `<title>` but tag nav is hidden on index
+- Severity: low (inferred)
+- File and line: `src/DarcsWeb/Html.hs:62,67`.
+- Issue: the index page now renders `<h1>siteTitle</h1>` immediately
+ followed by the repo table. There is no `repoNavBar` on the index
+ (correct: no repo), but because the header `<nav>` becomes empty
+ too (see C1) the index has exactly one nav landmark (the
+ still-emitted empty one), while per-repo pages have three (header
+ breadcrumbs, `repo-nav`, footer-less) ��� a consistency gap worth
+ collapsing once C1 is fixed.
+- Why it matters for users: screen-reader "list landmarks" / "list
+ navigations" output changes shape when moving between the index and
+ any repo page.
+- Practical fix: combined with C1, skip the empty breadcrumb nav on
+ the index and 404 to keep the landmark set stable.
+
+---
+
+## Mobile-first / design findings
+
+### D1. `<article class="log-entry">` inherits the desktop `box-shadow` hover rule on touch devices
+- Severity: low
+- File and line: `src/DarcsWeb/Html.hs:208` (tag change);
+ rule in `static/style.css` lines 474-486.
+- Issue: the swap from `div` to `<article>` does not break the
+ selector (both are class-matched), but the `.log-entry:hover` rule
+ is now attached to a block that represents a full landmark in the
+ page. On mobile Safari / Chrome the `:hover` state sticks after the
+ first tap until the next tap elsewhere, so "hovered" cards linger
+ with `box-shadow: var(--shadow-md)` after navigation.
+- Why it matters for users: phantom hover shadows accumulate on
+ scrolling lists of patches; this was the same on desktop, but
+ `<article>` is more likely to be interactive-looking, so users will
+ tap directly on the card rather than the inner link.
+- Practical fix: gate the hover shadow to pointer devices, e.g.
+ wrap the `:hover` rule in `@media (hover: hover)`. CSS-only change,
+ not part of this review's scope, but flagged because the Haskell
+ change makes the card feel more clickable.
+
+### D2. `<header class="page-header">` stays sticky while the breadcrumb `<nav>` can wrap to a second line
+- Severity: low (inferred)
+- File and line: `src/DarcsWeb/Html.hs:41-46` combined with
+ `static/style.css` lines 184-196 (`position: sticky; top: 0;`,
+ `flex-wrap: wrap`).
+- Issue: on very long breadcrumb trails (deep tree subpath) the
+ breadcrumb `<nav>` wraps, making the sticky `<header>` grow to two
+ or three lines and eat a large share of a phone viewport. The
+ previous code had the same behavior, but the `<header>` landmark
+ and `aria-label="Breadcrumb"` promote the region to a more
+ prominent role, so the vertical cost is more visible.
+- Why it matters for users: on a 360��640 viewport a three-line
+ sticky header can consume ~30% of vertical space above the fold.
+- Practical fix: cap the breadcrumb height with
+ `max-height` + `overflow-x: auto` on the nav so it scrolls
+ horizontally instead of wrapping vertically. CSS-only; flagging
+ only because the diff touches this layout.
+
+---
+
+## Performance findings
+
+### P1. `renderTreeBreadcrumb` / `renderBlobBreadcrumb` rebuild prefix lists in O(n��)
+- Severity: low
+- File and line: `src/DarcsWeb/Html.hs:322-323, 344`.
+- Issue: `prefixes = drop 1 (scanl appendSeg [] parts)` where
+ `appendSeg acc p = acc ++ [p]` rebuilds the prefix list with
+ list-append at every step, so the total work is O(depth��) in the
+ number of segments, and each prefix is re-percent-encoded from
+ scratch by `encodePathList`.
+- Why it matters for users: negligible for typical repo depths (���10
+ segments), but rendering a tree page for a deep path walks this
+ cost twice (once for the header-bar crumb via `pathSuffix`, once in
+ the `.tree-path` body). It is wasted server CPU on every tree hit.
+- Practical fix: build hrefs incrementally by threading the
+ percent-encoded accumulator through a single `foldl`:
+ `scanl (\acc seg -> acc <> "/" <> encodePathSegment seg) "" parts`.
+ O(depth) time, one pass, and removes the intermediate `[[Text]]`
+ allocation.
+
+### P2. `encodePathSegment` is re-applied to the same repo name on every row of every table
+- Severity: low
+- File and line: `src/DarcsWeb/Html.hs:83,86,92,93` (repo row),
+ `src/DarcsWeb/Html.hs:171,178,185` (shortlog row),
+ `src/DarcsWeb/Html.hs:205,211` (full-log row),
+ `src/DarcsWeb/Html.hs:285,290,296` (tag row),
+ `src/DarcsWeb/Html.hs:368,377,391,395,396` (tree row).
+- Issue: each `render*Row` re-runs `encodePathSegment repoName`
+ (byte-by-byte UTF-8 encode + hex escape) for every table row. A
+ tree with 500 entries pays this cost 500 times, even though
+ `repoName` never changes across a single render call.
+- Why it matters for users: for small repos negligible, but a large
+ shortlog (thousands of patches) spends measurable time
+ re-encoding the same segment. It also materialises N copies of
+ the same `Text` via `T.concat` / `T.pack`.
+- Practical fix: precompute `nameSeg` once in the caller
+ (`renderRepoTable`, `renderShortLogTable`, `renderFullLog`,
+ `renderTagList`, `renderTreeTable`) and pass it into the per-row
+ renderer. Same applies to the `"/repo/" <> nameSeg <> "/���"`
+ prefixes, which can be baked once per table.
+
+### P3. `renderFullLog` still calls `renderFullLogEntry` with per-entry `encodePathSegment repoName`
+- Severity: low
+- File and line: `src/DarcsWeb/Html.hs:199,205`.
+- Issue: same pattern as P2, but in the full-log path where the
+ inner `<pre>` already dominates CPU. Not a hot path, but the
+ redundant allocation is trivially avoidable.
+- Why it matters for users: mostly GC pressure. Mobile browsers are
+ not affected; server-side throughput is.
+- Practical fix: same as P2 ��� hoist `nameSeg` to `renderFullLog` and
+ pass it in.
+
+---
+
+## Areas worth testing on real mobile devices
+
+- A sticky-header summary page on a 360��640 viewport with a very
+ long repository name ��� verify the breadcrumb wraps without
+ pushing the `<main>` below the fold.
+- Tap-hold / long-press on `<article class="log-entry">` on iOS
+ Safari; confirm the hover shadow does not linger after scroll
+ (D1).
+- Tree view with a directory whose name contains `#`, `?`, `%`,
+ space, or non-ASCII (e.g. `caf��`). Previously the browser
+ auto-encoded these; now they are pre-encoded server-side.
+ Navigating, pressing "back", and copying the URL should all
+ round-trip identically.
+- `aria-current="page"` announcement on VoiceOver / TalkBack for
+ the active tab in `.repo-nav`, both when landing on the section
+ directly and after client-side back/forward.
+
+## Performance paths worth measuring
+
+- Rendering time of `/repo/:name/tree/<deep>` for a path with
+ ~20 segments ��� verify the O(depth��) crumb build (P1) is not
+ visible in a flamegraph.
+- Shortlog / full-log render time on a repo with many patches
+ (thousands) ��� verify that hoisting `nameSeg` per table (P2/P3)
+ actually reduces allocation.
+- Favicon fetch: the linked `/static/darcs-logo.png` is a 10 KiB
+ PNG served with `Cache-Control: public, max-age=86400, immutable`
+ (`app/Main.hs:387`). On a cold cache a mobile client downloads
+ 10 KiB for what browsers typically display at 16��16 ��� worth
+ measuring LCP impact and considering an optimized 16��16 /
+ 32��32 `.ico` or inline SVG. The diff only adds the `<link>`;
+ the asset itself is unchanged.
+
+## Consistency risks that could not be fully verified from code alone
+
+- Whether Scotty 0.22's regex routes apply `pathInfo` (decoded)
+ consistently in all versions installed via `stack.yaml` ��� the
+ percent-encoding round-trip assumes that. Claim supported by
+ code inspection but not by a runtime test.
+- Whether all previously-emitted raw hrefs (escaped via `esc`) are
+ now routed through `encodePathSegment`. I checked every
+ `href="/repo/` / `href="/clone/` site in `Html.hs`; a grep for
+ `esc repoName` inside `href=` in the module returns no results
+ (only inside visible anchor text). Verified.
+- Whether existing bookmarks that used unencoded URLs (i.e., with
+ a literal space or `#`) will still work. They will not: the
+ server never decoded them correctly before either, so those
+ bookmarks were already broken ��� but this is worth mentioning in
+ release notes.
addfile ./reviews/post-codex-bugs.md
hunk ./reviews/post-codex-bugs.md 1
+**Finding 1**
+1. Severity: Medium
+2. File and line: `src/DarcsWeb/Darcs.hs:286-292`, `app/Main.hs:433-453`
+3. What is wrong: `isCloneSubPath` is described and tested as a strict allowlist, but the implementation accepts any path with prefix `inventories/`, `patches/`, or `pristine.hashed/` as long as it has at least two segments. `serveClone` then serves the resolved file verbatim. That means the new "private except for clone objects" boundary is not actually enforced for nested files under those directories.
+4. Why it can fail in practice: A repository containing `_darcs/patches/private/secret.txt` or `_darcs/pristine.hashed/debug/notes` would now be downloadable via `/clone/...`, even though the patch explicitly claims unrelated `_darcs` contents stay private. This is a secure-by-default regression, not just a style issue.
+5. Minimal fix suggestion: Narrow the accepted shapes to the exact object layouts darcs uses, including only the bucketing scheme you actually intend to support, and add negative tests for extra nesting such as `patches/a/b` and `pristine.hashed/x/y`.
+
+**Finding 2**
+1. Severity: Medium
+2. File and line: `src/DarcsWeb/Darcs.hs:286-293`, `test/Properties/Clone.hs:186-201`
+3. What is wrong: The new allowlist deliberately rejects `_darcs/prefs/motd` and `format`, and the new property tests lock that behavior in. `darcs help manpage` documents `_darcs/prefs/motd` as data shown to users who clone or pull from a repository, so the patch is changing observable clone behavior rather than just hiding unused internals.
+4. Why it can fail in practice: HTTP clones served by darcsweb will no longer match darcs's documented clone surface for repositories that rely on MOTD, and possibly other clone-visible metadata such as `format`. Because the tests only assert the predicate, not a real `darcs clone` against the endpoint, CI would still go green while the protocol surface regresses.
+5. Minimal fix suggestion: Add an end-to-end clone test that exercises `darcs clone` against the HTTP endpoint and use that to derive the allowlist. At minimum, stop rejecting documented clone-visible metadata until the client behavior has been verified empirically.
+
+**Finding 3**
+1. Severity: Low
+2. File and line: `app/Main.hs:72-76`
+3. What is wrong: `parseOpts` changed from `foldl (flip id)` to `foldr ($)`. That reverses precedence for duplicate options. With the new code, `darcsweb -c first.conf -c second.conf` selects `first.conf` instead of the last `-c`.
+4. Why it can fail in practice: Wrapper scripts and service units commonly append overrides. After this change, an appended `-c` no longer overrides an earlier one, so darcsweb can start with the wrong configuration file even though the command line looks correct.
+5. Minimal fix suggestion: Apply option updates in input order again with `foldl (flip id)` or an equivalent left-to-right fold, and add a small regression test for duplicate `-c` arguments.
+
+**Residual risks**
+- I could not run `stack test` in this sandbox because Stack tries to create a global pantry lock under `/home/fritjof/.stack`, which is mounted read-only here. The review is therefore source-based plus local darcs CLI/manpage checks, not a full test run.
+- The clone endpoint change is security-sensitive and protocol-sensitive at the same time. Without an end-to-end clone test, there is still meaningful risk of both overexposure and accidental incompatibility.
+
+**Missing test coverage**
+- No test checks duplicate command-line option precedence for `parseOpts`.
+- No test exercises `serveClone` against a real `darcs clone`; the new properties only test the predicate in isolation.
+- No negative test covers overbroad nested paths such as `patches/a/b`, `inventories/a/b`, or `pristine.hashed/a/b`.
+
+**Changed behavior that remains unverified**
+- The new clone allowlist has not been verified against actual darcs client requests.
+- The new safe-canonicalization paths for static files and repositories have not been exercised by tests for permission errors, symlink loops, or invalid path bytes.
+- The `readMaybe`-based port parsing path has no direct regression test for malformed or duplicated config/CLI inputs.
addfile ./reviews/post-codex-efficiency.md
hunk ./reviews/post-codex-efficiency.md 1
+1. Severity: Medium
+2. File and line: `src/DarcsWeb/Darcs.hs:204-210`, `app/Main.hs:254-257`, `src/DarcsWeb/Html.hs:123-126`
+3. The inefficiency: `getRepoSummary` avoids the old multi-pass repo reopenings, but it still materializes `PatchSummary` for every patch in the repository via `mapRL extractPatchListing patchRL`, then returns the full tag list to a page that only renders `take 10 recentPatches`, `take 5 tags`, and `length tags > 5`. That keeps `/repo/:name/summary` proportional to total history size in both CPU and allocation pressure, and it pays the `summary p` rendering cost for patches that the summary page will never show.
+4. When it becomes a real problem: This shows up on repositories with large histories, or on instances where the summary page is the common entry point and gets refreshed often. Under that load, each summary request still walks the whole patch set and builds far more `PatchSummary` data than the template consumes, so latency scales with repository age rather than with the small amount of information visible on the page.
+5. A concrete optimization direction: Keep the single-repository fast path, but replace `allSummaries` with a strict summary-specific fold over `patchRL` that accumulates only `(lastDate, patchCount, first 10 recent patches, first 6 tags/hasMoreTags)`. Use `PatchInfo` alone for `lastDate` and `patchCount`, and only call `extractPatchListing` for patches that will actually be displayed or counted toward the small tag preview. If other routes still need full patch or tag lists, keep those as separate helpers instead of making the summary endpoint construct them eagerly.
addfile ./reviews/post-codex-quality.md
hunk ./reviews/post-codex-quality.md 1
+Must-fix issues
+
+1. Severity: High
+ File and line: `app/Main.hs:75`
+ The quality problem: Replacing `foldl (flip id)` with `foldr ($)` silently reverses option update precedence. With repeated flags such as `-c a -c b`, the first occurrence now wins instead of the last one, which is the opposite of the old behavior and of normal CLI expectations.
+ Why it matters long-term: This makes the option pipeline harder to reason about and turns future non-idempotent flags into easy sources of surprising behavior.
+ A practical improvement: Restore left-to-right application order, for example with `foldl' (flip ($)) defaultOpts updates`, or revert to the previous fold and add a regression test for repeated `-c`.
+
+2. Severity: Medium
+ File and line: `src/DarcsWeb/Darcs.hs:283-293`
+ The quality problem: `isCloneSubPath` is described as a strict allowlist, but the `("inventories" : _ : _)`, `("patches" : _ : _)`, and `("pristine.hashed" : _ : _)` cases admit any deeper descendant under those directories. That is broader than the documented contract and makes the policy harder to audit.
+ Why it matters long-term: The next time darcs or local tooling drops unrelated files below one of those directories, they become public automatically even though the surrounding comments and tests suggest a tightly pinned surface.
+ A practical improvement: Encode the expected path shapes explicitly, ideally at exact depth with filename validation that matches the clone protocol, and add regression tests for rejected cases like `patches/hash/extra` and `inventories/x/y`.
+
+Optional improvements
+
+1. Severity: Medium
+ File and line: `src/DarcsWeb/Darcs.hs:196-222`
+ The quality problem: `getRepoSummary` still materializes `PatchSummary` values for the entire patch list just to derive count, last change, recent patches, and tags. The summary route only renders 10 recent patches and 5 tags, but the helper is coupled to the full listing representation.
+ Why it matters long-term: The function's contract is already wider than the page needs, so future summary-page changes will keep threading through an overstuffed tuple and can accidentally preserve whole-history work that no caller needs.
+ A practical improvement: Replace the tuple with a dedicated summary record and accumulate only the data the summary page needs: count, latest date, first N recent patches, and tag summaries.
+
+Areas that may become maintenance risks later
+
+- `safeCanonicalize` and `readRepoDescription` both fail closed by discarding the underlying exception. That is appropriate for HTTP responses, but without any logging it will be hard to distinguish malformed input from permission problems or repository corruption in production.
+- `RepoInfo` assembly now lives in both `checkRepo` and `getRepoSummary`. If metadata rules change later, the index and summary paths can drift unless that construction is centralized.
+
+Any testing or structure gaps worth watching
+
+- There is no regression test covering repeated CLI options after the `parseOpts` refactor.
+- The new clone allowlist tests cover named happy paths, but they do not pin the deeper descendant cases that the current implementation accepts.
+- I could not run the suite in this environment: `stack test --fast` is blocked by sandbox writes under `~/.stack`, and `cabal test` fails dependency resolution against the installed `base` and `scotty` set.
addfile ./reviews/post-codex-webapp.md
hunk ./reviews/post-codex-webapp.md 1
+# Web Application Review
+
+## Performance Issues
+
+### Finding 1
+1. Severity: Medium
+2. File and line: `src/DarcsWeb/Darcs.hs:205`, `src/DarcsWeb/Darcs.hs:387`, `src/DarcsWeb/Html.hs:112`, `src/DarcsWeb/Html.hs:124`
+3. The issue: Inferred from code: the new `getRepoSummary` path still builds `extractPatchListing` for the entire history, and `extractPatchListing` renders `summary p` for every patch. The summary page only uses the first 10 recent entries plus at most 5 tags, and it does not render `psSummary` at all, so most of that work is discarded before any HTML is produced.
+4. Why it matters for users: `/repo/:name/summary` will still have avoidable CPU time and TTFB on repositories with long histories. That is most visible on mobile connections, where the page feels slow even though the rendered UI is small.
+5. A practical fix: add a lighter-weight summary-specific extractor that collects only the fields the page actually renders, keeps only the first 10 recent patches, and stops tag collection after 6 items so the template can still decide whether to show the `all tags...` link without materializing every tag summary.
hunk ./src/DarcsWeb/Config.hs 8
+ , parsePortPure
hunk ./src/DarcsWeb/Config.hs 11
-import Data.Char (isSpace)
+import Data.Char (isDigit, isSpace)
hunk ./src/DarcsWeb/Config.hs 14
+import Data.Maybe (fromMaybe)
+import Text.Read (readMaybe)
hunk ./src/DarcsWeb/Config.hs 42
-cfgLookupDefault key def m = maybe def id (Map.lookup key m)
+cfgLookupDefault key def m = fromMaybe def (Map.lookup key m)
+
+-- | Parse a TCP port from a config value. Accepts 1..65535 and rejects
+-- everything else (empty, negative, zero, 65536+, non-numeric,
+-- hex, or any string that is not purely ASCII digits). Pure so the
+-- accepted/rejected set can be pinned by QuickCheck; the IO wrapper
+-- 'Main.parsePort' handles the startup diagnostic.
+--
+-- Only digits are accepted to prevent Haskell 'Text.Read.readMaybe'
+-- from silently stripping leading/trailing whitespace, a
+-- surrounding comment, or an @0x@ hex prefix ��� each of which the
+-- operator is more likely to have meant differently.
+parsePortPure :: String -> Maybe Int
+parsePortPure s
+ | null s = Nothing
+ | not (all isDigit s) = Nothing
+ | otherwise = case readMaybe s of
+ Just p | p > 0 && p < 65536 -> Just p
+ _ -> Nothing
hunk ./src/DarcsWeb/Darcs.hs 14
+ , getRepoSummary
hunk ./src/DarcsWeb/Darcs.hs 17
+ , safeCanonicalize
hunk ./src/DarcsWeb/Darcs.hs 20
+ , isCloneSubPath
hunk ./src/DarcsWeb/Darcs.hs 24
+import qualified Data.ByteString as BS
hunk ./src/DarcsWeb/Darcs.hs 26
+import qualified Data.Text.Encoding as TE
hunk ./src/DarcsWeb/Darcs.hs 87
- case mInfo of
- Nothing -> return [RepoInfo
- { riName = T.pack name
- , riPath = fullPath
- , riDescription = desc
- , riLastChange = ""
- , riPatchCount = 0
- }]
- Just (lastDate, count) -> return [RepoInfo
- { riName = T.pack name
- , riPath = fullPath
- , riDescription = desc
- , riLastChange = lastDate
- , riPatchCount = count
- }]
-
+ let (lastDate, count) = case mInfo of
+ Just (d, c) -> (d, c)
+ Nothing -> ("", 0)
+ return [RepoInfo
+ { riName = T.pack name
+ , riPath = fullPath
+ , riDescription = desc
+ , riLastChange = lastDate
+ , riPatchCount = count
+ }]
+
+-- | Read the optional repo_description file using strict UTF-8 decoding.
+-- Any IO error, non-UTF-8 byte, or missing file yields @""@; the summary
+-- page must not crash on a malformed repository description.
hunk ./src/DarcsWeb/Darcs.hs 105
- if exists
- then T.strip . T.pack <$> readFile descFile
- else return ""
+ if not exists
+ then return ""
+ else do
+ res <- try (BS.readFile descFile) :: IO (Either SomeException BS.ByteString)
+ case res of
+ Left _ -> return ""
+ Right bs -> case TE.decodeUtf8' bs of
+ Left _ -> return ""
+ Right txt -> return (T.strip txt)
hunk ./src/DarcsWeb/Darcs.hs 142
--- | Get a single patch by hash (with full diff)
+-- | Get a single patch by hash (with full diff).
+-- Hash-filters before rendering: only the matching patch's diff is built,
+-- so a request for a missing or old hash no longer materialises the
+-- entire repository history as rendered text.
hunk ./src/DarcsWeb/Darcs.hs 151
- allPatches = mapRL extractPatchFull patchRL
- found = filter (\ps -> psHash ps == targetHash) allPatches
- case found of
+ selected = mapRL (selectMatching targetHash) patchRL
+ case [p | Just p <- selected] of
hunk ./src/DarcsWeb/Darcs.hs 161
+-- | Render the full patch only if its hash matches 'targetHash'.
+-- Non-matching patches cost one SHA-1 per patch and no diff rendering.
+selectMatching :: RepoPatch p => Text -> PatchInfoAnd p wA wB -> Maybe PatchSummary
+selectMatching targetHash piap
+ | patchHashText (info piap) == targetHash = Just (extractPatchFull piap)
+ | otherwise = Nothing
+
+-- | SHA-1 hash of a patch's 'PatchInfo', in the same hex form used by
+-- 'psHash'. Kept out of the summary builder so it can be computed
+-- without rendering a diff.
+patchHashText :: PatchInfo -> Text
+patchHashText pinfo = T.pack (BC.unpack (sha1Show (makePatchname pinfo)))
+
hunk ./src/DarcsWeb/Darcs.hs 190
+-- | One-pass summary builder: loads the repo once and derives
+-- description, patch count, last-change date, recent patches, and
+-- tags. Replaces the previous combination of 'listRepos' +
+-- 'getRepoPatches' + 'getRepoTags' on the summary page.
+--
+-- Count, last-change date, and tag detection walk cheap 'PatchInfo'
+-- values only; 'extractPatchListing' (which renders per-patch summary
+-- text) is limited to the patches actually displayed on the page
+-- (recent prefix + detected tags). That keeps the hot path
+-- proportional to what is rendered, not to total history length.
+--
+-- Returns the metadata even on read failure (empty lists) so callers
+-- only need 'withRepo'-level validation.
+getRepoSummary
+ :: FilePath -- ^ already-resolved repo path (from 'withRepo')
+ -> Text -- ^ display name
+ -> IO (RepoInfo, [PatchSummary], [PatchSummary])
+getRepoSummary repoPath name = do
+ desc <- readRepoDescription repoPath
+ result <- try $ withRepositoryLocation YesUseCache repoPath $ RepoJob $ \repository -> do
+ patches <- readPatches repository
+ let patchRL = patchSet2RL patches
+ -- Cheap metadata pass: only 'PatchInfo' is walked, no diff
+ -- or summary text is rendered here.
+ infos = mapRL info patchRL
+ count = length infos
+ lastDate = case infos of
+ (i:_) -> T.pack (piDateString i)
+ [] -> ""
+ tagFlags = map isTag infos
+ -- Parallel list of rendered listings. Pairing with 'tagFlags'
+ -- and 'zip' preserves order; because Haskell lists are lazy,
+ -- listings are only forced for entries we actually keep
+ -- ('take' for recent, @True@ guard for tags).
+ listings = mapRL extractPatchListing patchRL
+ recent = take maxPatchCount listings
+ tags = [ ps | (True, ps) <- zip tagFlags listings ]
+ _ <- evaluate count
+ return (lastDate, count, recent, tags)
+ let (lastDate, count, recent, tags) = case result of
+ Left (_ :: SomeException) -> ("", 0, [], [])
+ Right v -> v
+ ri = RepoInfo
+ { riName = name
+ , riPath = repoPath
+ , riDescription = desc
+ , riLastChange = lastDate
+ , riPatchCount = count
+ }
+ return (ri, recent, tags)
+
hunk ./src/DarcsWeb/Darcs.hs 249
+-- | Threshold for sniffing binary content; a NUL byte in the first
+-- 'binarySniffLimit' bytes is treated as "cannot display as text".
+binarySniffLimit :: Int
+binarySniffLimit = 8192
+
+-- | Like 'canonicalizePath' but returns 'Nothing' on any IO exception.
+-- Used for request-derived paths where IO errors (permission denied,
+-- invalid byte sequences, symlink loops) must not escape to the client
+-- as framework 500s.
+safeCanonicalize :: FilePath -> IO (Maybe FilePath)
+safeCanonicalize p = do
+ r <- try (canonicalizePath p) :: IO (Either SomeException FilePath)
+ return $ case r of
+ Left _ -> Nothing
+ Right v -> Just v
+
hunk ./src/DarcsWeb/Darcs.hs 267
+-- Fails closed on any canonicalization error.
hunk ./src/DarcsWeb/Darcs.hs 270
- canonical <- canonicalizePath path
- let jailSlash = if null jail then "/"
- else if last jail == '/' then jail else jail ++ "/"
- return (jailSlash `isPrefixOf` canonical || canonical == init jailSlash)
+ mCanonical <- safeCanonicalize path
+ case mCanonical of
+ Nothing -> return False
+ Just canonical ->
+ let jailSlash = PathPure.add_trailing_slash jail
+ jailNoSlash = dropTrailingSlash jailSlash
+ in return (jailSlash `isPrefixOf` canonical || canonical == jailNoSlash)
+ where
+ dropTrailingSlash s = case reverse s of
+ ('/':rest) -> reverse rest
+ _ -> s
hunk ./src/DarcsWeb/Darcs.hs 287
+-- | Allowlist for the read-only darcs HTTP clone endpoint.
+-- Tighter than 'isSafeSubPath': only the minimal object set that a
+-- darcs client actually needs to clone is exposed, so miscellaneous
+-- files under @_darcs/@ (e.g. @prefs/defaultrepo@, @format@, custom
+-- scripts) stay private even when they pass hidden-segment checks.
+--
+-- Accepted (exact two-segment shapes only; extra nesting is rejected):
+--
+-- * @hashed_inventory@
+-- * @inventories/<hash>@
+-- * @patches/<hash>@
+-- * @pristine.hashed/<hash>@
+-- * @packs/basic.tar.gz@ / @packs/patches.tar.gz@ (for @darcs optimize http@)
+isCloneSubPath :: FilePath -> Bool
+isCloneSubPath sub
+ | not (isSafeSubPath sub) = False
+ | otherwise = case splitPath sub of
+ ["hashed_inventory"] -> True
+ ["inventories", _] -> True
+ ["patches", _] -> True
+ ["pristine.hashed", _] -> True
+ ["packs", "basic.tar.gz"] -> True
+ ["packs", "patches.tar.gz"] -> True
+ _ -> False
+
hunk ./src/DarcsWeb/Darcs.hs 318
--- The subPath is relative to the repo root. Returns Nothing if the path
--- is not a directory, doesn't exist, or escapes the repo jail.
+-- The subPath is relative to the repo root (empty means the root).
+-- Any non-empty sub-path is rejected unless it passes 'isSafeSubPath',
+-- so hidden segments, @_darcs@ / @.git@, @..@, and dot-prefixed
+-- components are refused before touching the filesystem.
+-- Returns 'Nothing' if the path is not a directory, doesn't exist,
+-- fails the safety check, or escapes the repo jail.
hunk ./src/DarcsWeb/Darcs.hs 325
-getRepoTree repoPath subPath = do
- let fullDir = if null subPath then repoPath else repoPath </> subPath
- isDir <- doesDirectoryExist fullDir
- if not isDir
- then return Nothing
- else do
- -- Verify the resolved path stays inside the repo
- safe <- jailCheck repoPath fullDir
- if not safe
+getRepoTree repoPath subPath
+ | not (null subPath) && not (isSafeSubPath subPath) = return Nothing
+ | otherwise = do
+ let fullDir = if null subPath then repoPath else repoPath </> subPath
+ isDir <- doesDirectoryExist fullDir
+ if not isDir
hunk ./src/DarcsWeb/Darcs.hs 333
- entries <- listDirectory fullDir
- let visible = filter (\e -> e /= "_darcs" && not ("." `isPrefixOf` e)
- && e /= ".git") entries
- items <- mapM (mkEntry fullDir) (sortBy compare visible)
- let (dirs, files) = partition teIsDir items
- return (Just (dirs ++ files))
+ -- Verify the resolved path stays inside the repo
+ safe <- jailCheck repoPath fullDir
+ if not safe
+ then return Nothing
+ else do
+ entries <- listDirectory fullDir
+ let visible = filter (\e -> e /= "_darcs" && not ("." `isPrefixOf` e)
+ && e /= ".git") entries
+ items <- mapM (mkEntry fullDir) (sortBy compare visible)
+ let (dirs, files) = partition teIsDir items
+ return (Just (dirs ++ files))
hunk ./src/DarcsWeb/Darcs.hs 357
--- Returns Nothing if the file doesn't exist, is a directory, escapes
--- the repo jail, or exceeds the size limit.
+-- Returns 'Nothing' if the file doesn't exist, is a directory, fails
+-- the safety check, escapes the repo jail, or exceeds the size limit.
+-- Uses strict 'ByteString' IO under 'try' to avoid lazy-handle leaks
+-- and locale-dependent decoding. Content that contains a NUL byte in
+-- its first few KiB or is not valid UTF-8 is replaced with a
+-- "binary file, cannot display" placeholder so the rendered page
+-- cannot accidentally HTML-escape megabytes of garbage.
hunk ./src/DarcsWeb/Darcs.hs 365
-getRepoBlob repoPath subPath = do
- -- Reject unsafe subpaths (hidden files, _darcs, etc.)
- if not (isSafeSubPath subPath)
- then return Nothing
- else do
+getRepoBlob repoPath subPath
+ | not (isSafeSubPath subPath) = return Nothing
+ | otherwise = do
hunk ./src/DarcsWeb/Darcs.hs 382
- txt <- T.pack <$> Prelude.readFile fullPath
- return (Just txt)
+ res <- try (BS.readFile fullPath)
+ :: IO (Either SomeException BS.ByteString)
+ case res of
+ Left _ -> return Nothing
+ Right bs
+ | looksBinary bs -> return (Just binaryPlaceholder)
+ | otherwise -> case TE.decodeUtf8' bs of
+ Left _ -> return (Just binaryPlaceholder)
+ Right txt -> return (Just txt)
+
+-- | A NUL byte anywhere in the sniffed prefix is a strong signal that a
+-- file is not text.
+looksBinary :: BS.ByteString -> Bool
+looksBinary bs = 0 `BS.elem` BS.take binarySniffLimit bs
+
+-- | User-facing placeholder rendered in place of binary blob contents.
+binaryPlaceholder :: Text
+binaryPlaceholder = "(binary file, cannot display)"
hunk ./src/DarcsWeb/Darcs.hs 429
- , psHash = T.pack (BC.unpack (sha1Show (makePatchname pinfo)))
+ , psHash = patchHashText pinfo
hunk ./src/DarcsWeb/Html.hs 20
+ , renderTreeBreadcrumb
+ , renderBlobBreadcrumb
hunk ./src/DarcsWeb/Html.hs 24
+import Data.Maybe (fromMaybe)
hunk ./src/DarcsWeb/Html.hs 30
-import qualified HtmlPure
+import DarcsWeb.Clone (encodePathSegment)
hunk ./src/DarcsWeb/Html.hs 40
+ , "<link rel=\"icon\" type=\"image/png\" href=\"/static/darcs-logo.png\">\n"
hunk ./src/DarcsWeb/Html.hs 43
- , "<div class=\"page-header\">\n"
+ , "<header class=\"page-header\">\n"
hunk ./src/DarcsWeb/Html.hs 45
- , "<img src=\"/static/darcs-logo.png\" alt=\"darcs\" class=\"header-logo\">"
+ , "<img src=\"/static/darcs-logo.png\" alt=\"\" class=\"header-logo\">"
hunk ./src/DarcsWeb/Html.hs 47
- , "<nav class=\"breadcrumbs\">", breadcrumbs, "</nav>\n"
- , "</div>\n"
- , "<div class=\"page-body\">\n"
+ , if T.null breadcrumbs
+ then ""
+ else "<nav class=\"breadcrumbs\" aria-label=\"Breadcrumb\">"
+ <> breadcrumbs <> "</nav>\n"
+ , "</header>\n"
+ , "<main class=\"page-body\">\n"
hunk ./src/DarcsWeb/Html.hs 54
- , "</div>\n"
- , "<div class=\"page-footer\">\n"
+ , "</main>\n"
+ , "<footer class=\"page-footer\">\n"
hunk ./src/DarcsWeb/Html.hs 58
- , "</div>\n"
+ , "</footer>\n"
hunk ./src/DarcsWeb/Html.hs 67
- [ if null repos
+ [ "<h1>", esc siteTitle, "</h1>\n"
+ , if null repos
hunk ./src/DarcsWeb/Html.hs 87
-renderRepoRow now ri = T.concat
- [ "<tr>\n"
- , "<td class=\"repo-name\"><a href=\"/repo/", esc (riName ri), "/summary\">", esc (riName ri), "</a></td>\n"
- , "<td>", esc (riDescription ri), "</td>\n"
- , "<td>", esc (relativeDate now (riLastChange ri)), "</td>\n"
- , "<td class=\"num\">", T.pack (show (riPatchCount ri)), "</td>\n"
- , "<td class=\"actions\">"
- , "<a href=\"/repo/", esc (riName ri), "/shortlog\">log</a> | "
- , "<a href=\"/repo/", esc (riName ri), "/tags\">tags</a>"
- , "</td>\n"
- , "</tr>\n"
- ]
+renderRepoRow now ri =
+ let nameSeg = encodePathSegment (riName ri)
+ in T.concat
+ [ "<tr>\n"
+ , "<td class=\"repo-name\"><a href=\"/repo/", nameSeg, "/summary\">"
+ , esc (riName ri), "</a></td>\n"
+ , "<td>", esc (riDescription ri), "</td>\n"
+ , "<td>", esc (relativeDate now (riLastChange ri)), "</td>\n"
+ , "<td class=\"num\">", tshow (riPatchCount ri), "</td>\n"
+ , "<td class=\"actions\">"
+ , "<a href=\"/repo/", nameSeg, "/shortlog\">log</a> | "
+ , "<a href=\"/repo/", nameSeg, "/tags\">tags</a>"
+ , "</td>\n"
+ , "</tr>\n"
+ ]
hunk ./src/DarcsWeb/Html.hs 106
- let breadcrumbs = " / <a href=\"/repo/" <> esc repoName <> "/summary\">" <> esc repoName <> "</a>"
+ let nameSeg = encodePathSegment repoName
+ breadcrumbs = repoBreadcrumb repoName
hunk ./src/DarcsWeb/Html.hs 121
- , if length recentPatches > 0
- then "<p class=\"more\"><a href=\"/repo/" <> esc repoName <> "/shortlog\">...</a></p>\n"
+ , if not (null recentPatches)
+ then "<p class=\"more\"><a href=\"/repo/" <> nameSeg <> "/shortlog\">...</a></p>\n"
hunk ./src/DarcsWeb/Html.hs 131
- then "<p class=\"more\"><a href=\"/repo/" <> esc repoName <> "/tags\">all tags...</a></p>\n"
+ then "<p class=\"more\"><a href=\"/repo/" <> nameSeg <> "/tags\">all tags...</a></p>\n"
hunk ./src/DarcsWeb/Html.hs 175
-renderShortLogRow now repoName ps = T.concat
- [ "<tr", if psIsTag ps then " class=\"tag-row\"" else "", ">\n"
- , "<td class=\"date\">", esc (relativeDate now (psDate ps)), "</td>\n"
- , "<td class=\"author\">", esc (shortAuthor (psAuthor ps)), "</td>\n"
- , "<td class=\"subject\">"
- , "<a href=\"/repo/", esc repoName, "/patch/", esc (psHash ps), "\">"
- , if psIsTag ps
- then "<span class=\"tag-badge\">TAG</span> "
- else ""
- , esc (psName ps), "</a>"
- , "</td>\n"
- , "<td class=\"actions\">"
- , "<a href=\"/repo/", esc repoName, "/patch/", esc (psHash ps), "\">diff</a>"
- , "</td>\n"
- , "</tr>\n"
- ]
+renderShortLogRow now repoName ps =
+ let nameSeg = encodePathSegment repoName
+ hashSeg = encodePathSegment (psHash ps)
+ in T.concat
+ [ "<tr", if psIsTag ps then " class=\"tag-row\"" else "", ">\n"
+ , "<td class=\"date\">", esc (relativeDate now (psDate ps)), "</td>\n"
+ , "<td class=\"author\">", esc (shortAuthor (psAuthor ps)), "</td>\n"
+ , "<td class=\"subject\">"
+ , "<a href=\"/repo/", nameSeg, "/patch/", hashSeg, "\">"
+ , if psIsTag ps
+ then "<span class=\"tag-badge\">TAG</span> "
+ else ""
+ , esc (psName ps), "</a>"
+ , "</td>\n"
+ , "<td class=\"actions\">"
+ , "<a href=\"/repo/", nameSeg, "/patch/", hashSeg, "\">diff</a>"
+ , "</td>\n"
+ , "</tr>\n"
+ ]
hunk ./src/DarcsWeb/Html.hs 209
-renderFullLogEntry now repoName ps = T.concat
- [ "<div class=\"log-entry", if psIsTag ps then " tag-entry" else "", "\">\n"
- , "<div class=\"log-header\">\n"
- , "<span class=\"log-name\">"
- , "<a href=\"/repo/", esc repoName, "/patch/", esc (psHash ps), "\">"
- , if psIsTag ps then "<span class=\"tag-badge\">TAG</span> " else ""
- , esc (psName ps)
- , "</a></span>\n"
- , "<span class=\"log-date\">", esc (relativeDate now (psDate ps)), "</span>\n"
- , "</div>\n"
- , "<div class=\"log-meta\">\n"
- , "<span class=\"log-author\">", esc (psAuthor ps), "</span>\n"
- , "</div>\n"
- , if T.null (T.strip (psLog ps))
- then ""
- else "<div class=\"log-body\"><pre>" <> esc (T.strip (psLog ps)) <> "</pre></div>\n"
- , if T.null (T.strip (psSummary ps))
- then ""
- else "<div class=\"log-summary\"><pre>" <> esc (T.strip (psSummary ps)) <> "</pre></div>\n"
- , "</div>\n"
- ]
+renderFullLogEntry now repoName ps =
+ let nameSeg = encodePathSegment repoName
+ hashSeg = encodePathSegment (psHash ps)
+ in T.concat
+ [ "<article class=\"log-entry", if psIsTag ps then " tag-entry" else "", "\">\n"
+ , "<div class=\"log-header\">\n"
+ , "<span class=\"log-name\">"
+ , "<a href=\"/repo/", nameSeg, "/patch/", hashSeg, "\">"
+ , if psIsTag ps then "<span class=\"tag-badge\">TAG</span> " else ""
+ , esc (psName ps)
+ , "</a></span>\n"
+ , "<span class=\"log-date\">", esc (relativeDate now (psDate ps)), "</span>\n"
+ , "</div>\n"
+ , "<div class=\"log-meta\">\n"
+ , "<span class=\"log-author\">", esc (psAuthor ps), "</span>\n"
+ , "</div>\n"
+ , if T.null (T.strip (psLog ps))
+ then ""
+ else "<div class=\"log-body\"><pre>" <> esc (T.strip (psLog ps)) <> "</pre></div>\n"
+ , if T.null (T.strip (psSummary ps))
+ then ""
+ else "<div class=\"log-summary\"><pre>" <> esc (T.strip (psSummary ps)) <> "</pre></div>\n"
+ , "</article>\n"
+ ]
hunk ./src/DarcsWeb/Html.hs 237
- let breadcrumbs = repoBreadcrumb repoName
- <> " / <a href=\"/repo/" <> esc repoName <> "/shortlog\">shortlog</a>"
+ let nameSeg = encodePathSegment repoName
+ breadcrumbs = repoBreadcrumb repoName
+ <> " / <a href=\"/repo/" <> nameSeg <> "/shortlog\">shortlog</a>"
hunk ./src/DarcsWeb/Html.hs 289
-renderTagRow now repoName ps = T.concat
- [ "<tr>\n"
- , "<td class=\"tag-name\">"
- , "<a href=\"/repo/", esc repoName, "/patch/", esc (psHash ps), "\">"
- , esc (maybe (psName ps) id (psTagName ps))
- , "</a></td>\n"
- , "<td class=\"date\">", esc (relativeDate now (psDate ps)), "</td>\n"
- , "<td class=\"author\">", esc (shortAuthor (psAuthor ps)), "</td>\n"
- , "<td class=\"actions\">"
- , "<a href=\"/repo/", esc repoName, "/patch/", esc (psHash ps), "\">details</a>"
- , "</td>\n"
- , "</tr>\n"
- ]
+renderTagRow now repoName ps =
+ let nameSeg = encodePathSegment repoName
+ hashSeg = encodePathSegment (psHash ps)
+ in T.concat
+ [ "<tr>\n"
+ , "<td class=\"tag-name\">"
+ , "<a href=\"/repo/", nameSeg, "/patch/", hashSeg, "\">"
+ , esc (fromMaybe (psName ps) (psTagName ps))
+ , "</a></td>\n"
+ , "<td class=\"date\">", esc (relativeDate now (psDate ps)), "</td>\n"
+ , "<td class=\"author\">", esc (shortAuthor (psAuthor ps)), "</td>\n"
+ , "<td class=\"actions\">"
+ , "<a href=\"/repo/", nameSeg, "/patch/", hashSeg, "\">details</a>"
+ , "</td>\n"
+ , "</tr>\n"
+ ]
hunk ./src/DarcsWeb/Html.hs 309
- let pathSuffix = if T.null subPath then "" else " / " <> subPath
+ let pathSuffix = if T.null subPath then "" else " / " <> esc subPath
hunk ./src/DarcsWeb/Html.hs 319
+-- | Build a breadcrumb trail for a tree sub-path. Each visible label
+-- is HTML-escaped; each href is built from percent-encoded URL
+-- segments so a directory name containing @\"?\"@, @\"#\"@, or
+-- markup characters cannot break out of the URL or inject HTML.
hunk ./src/DarcsWeb/Html.hs 325
- let parts = if T.null subPath then [] else T.splitOn "/" subPath
- buildCrumbs _ [] = ""
- buildCrumbs acc (p:ps) =
- let newAcc = if T.null acc then p else acc <> "/" <> p
- in " / <a href=\"/repo/" <> esc repoName <> "/tree/" <> esc newAcc <> "\">"
- <> esc p <> "</a>" <> buildCrumbs newAcc ps
- in "<div class=\"tree-path\">"
- <> "<a href=\"/repo/" <> esc repoName <> "/tree\">[root]</a>"
- <> buildCrumbs "" parts
- <> "</div>\n"
+ let repoSeg = encodePathSegment repoName
+ rootAnchor = "<a href=\"/repo/" <> repoSeg <> "/tree\">[root]</a>"
+ in T.concat
+ [ "<div class=\"tree-path\">"
+ , rootAnchor
+ , treeCrumbs repoSeg (pathSegments subPath)
+ , "</div>\n"
+ ]
+
+-- | Breadcrumb for a blob path. The final segment is the file name and
+-- is rendered as plain (HTML-escaped) text; all preceding segments
+-- are linked back into the tree with percent-encoded hrefs.
+-- Degrades to a bare @[root]@ anchor when the sub-path is empty, so
+-- the helper is total for any future call site.
+renderBlobBreadcrumb :: Text -> Text -> Text
+renderBlobBreadcrumb repoName subPath =
+ let repoSeg = encodePathSegment repoName
+ rootAnchor = "<a href=\"/repo/" <> repoSeg <> "/tree\">[root]</a>"
+ parts = pathSegments subPath
+ (dirParts, fileName) = case reverse parts of
+ [] -> ([], "")
+ (f:revD) -> (reverse revD, f)
+ tail_ = if T.null fileName then "" else " / " <> esc fileName
+ in T.concat
+ [ "<div class=\"tree-path\">"
+ , rootAnchor
+ , treeCrumbs repoSeg dirParts
+ , tail_
+ , "</div>\n"
+ ]
+
+-- | Render a chain of linked breadcrumb segments. Threads the
+-- percent-encoded prefix through 'scanl' so each step is O(1) in the
+-- prefix length, instead of O(depth) as a list-append \"acc ++ [p]\"
+-- pattern would be.
+treeCrumbs :: Text -> [Text] -> Text
+treeCrumbs repoSeg segs =
+ let prefixes = drop 1 (scanl appendSeg "" segs)
+ appendSeg acc p =
+ if T.null acc then encodePathSegment p
+ else acc <> "/" <> encodePathSegment p
+ crumb prefix label =
+ let href = "/repo/" <> repoSeg <> "/tree/" <> prefix
+ in " / <a href=\"" <> esc href <> "\">" <> esc label <> "</a>"
+ in T.concat (zipWith crumb prefixes segs)
+
+-- | Split a '/'-separated path into its non-empty segments.
+pathSegments :: Text -> [Text]
+pathSegments s
+ | T.null s = []
+ | otherwise = filter (not . T.null) (T.splitOn "/" s)
+
+-- | Join a list of raw segments as a percent-encoded URL path.
+encodePathList :: [Text] -> Text
+encodePathList = T.intercalate "/" . map encodePathSegment
hunk ./src/DarcsWeb/Html.hs 382
-renderTreeTable repoName subPath entries = T.concat
- [ "<table class=\"tree-list\">\n"
- , "<thead><tr><th></th><th>Name</th><th>Size</th></tr></thead>\n"
- , "<tbody>\n"
- , if not (T.null subPath)
- then let parent = case T.breakOnEnd "/" subPath of
- ("", _) -> ""
- (pre, _) -> T.dropEnd 1 pre
- parentHref = "/repo/" <> esc repoName <> "/tree"
- <> (if T.null parent then "" else "/" <> esc parent)
- in "<tr><td class=\"tree-icon\">\xf0\x9f\x93\x81</td>"
- <> "<td class=\"tree-name\"><a href=\"" <> parentHref <> "\">..</a></td>"
- <> "<td></td></tr>\n"
- else ""
- , T.concat (map (renderTreeRow repoName subPath) entries)
- , "</tbody></table>\n"
- ]
+renderTreeTable repoName subPath entries =
+ let nameSeg = encodePathSegment repoName
+ in T.concat
+ [ "<table class=\"tree-list\">\n"
+ , "<thead><tr><th></th><th>Name</th><th>Size</th></tr></thead>\n"
+ , "<tbody>\n"
+ , if not (T.null subPath)
+ then let parent = case T.breakOnEnd "/" subPath of
+ ("", _) -> ""
+ (pre, _) -> T.dropEnd 1 pre
+ parentHref = "/repo/" <> nameSeg <> "/tree"
+ <> (if T.null parent then ""
+ else "/" <> encodePathList (pathSegments parent))
+ in "<tr><td class=\"tree-icon\" aria-hidden=\"true\">\xf0\x9f\x93\x81</td>"
+ <> "<td class=\"tree-name\"><a href=\"" <> parentHref <> "\">..</a></td>"
+ <> "<td></td></tr>\n"
+ else ""
+ , T.concat (map (renderTreeRow repoName subPath) entries)
+ , "</tbody></table>\n"
+ ]
hunk ./src/DarcsWeb/Html.hs 405
- let name = teName entry
- entryPath = if T.null subPath then name else subPath <> "/" <> name
+ let name = teName entry
+ nameSeg = encodePathSegment repoName
+ entrySegs = pathSegments subPath ++ [name]
+ entryHref = encodePathList entrySegs
hunk ./src/DarcsWeb/Html.hs 410
- then ("\xf0\x9f\x93\x81", "/repo/" <> esc repoName <> "/tree/" <> esc entryPath)
- else ("\xf0\x9f\x93\x84", "/repo/" <> esc repoName <> "/blob/" <> esc entryPath)
+ then ("\xf0\x9f\x93\x81", "/repo/" <> nameSeg <> "/tree/" <> entryHref)
+ else ("\xf0\x9f\x93\x84", "/repo/" <> nameSeg <> "/blob/" <> entryHref)
hunk ./src/DarcsWeb/Html.hs 415
- , "<td class=\"tree-icon\">", icon, "</td>"
+ , "<td class=\"tree-icon\" aria-hidden=\"true\">", icon, "</td>"
hunk ./src/DarcsWeb/Html.hs 425
- | n < 1024 = T.pack (show n) <> " B"
- | n < 1048576 = T.pack (show (n `div` 1024)) <> " KiB"
- | otherwise = T.pack (show (n `div` 1048576)) <> " MiB"
+ | n < kib = tshow n <> " B"
+ | n < mib = tshow (n `div` kib) <> " KiB"
+ | otherwise = tshow (n `div` mib) <> " MiB"
+ where
+ kib = 1024
+ mib = 1024 * 1024
hunk ./src/DarcsWeb/Html.hs 446
-renderBlobBreadcrumb :: Text -> Text -> Text
-renderBlobBreadcrumb repoName subPath =
- let parts = T.splitOn "/" subPath
- dirParts = init parts
- fileName = last parts
- buildCrumbs _ [] = ""
- buildCrumbs acc (p:ps) =
- let newAcc = if T.null acc then p else acc <> "/" <> p
- in " / <a href=\"/repo/" <> esc repoName <> "/tree/" <> esc newAcc <> "\">"
- <> esc p <> "</a>" <> buildCrumbs newAcc ps
- in "<div class=\"tree-path\">"
- <> "<a href=\"/repo/" <> esc repoName <> "/tree\">[root]</a>"
- <> buildCrumbs "" dirParts
- <> " / " <> esc fileName
- <> "</div>\n"
-
hunk ./src/DarcsWeb/Html.hs 455
- " / <a href=\"/repo/" <> esc repoName <> "/summary\">" <> esc repoName <> "</a>"
+ " / <a href=\"/repo/" <> encodePathSegment repoName <> "/summary\">" <> esc repoName <> "</a>"
hunk ./src/DarcsWeb/Html.hs 459
- [ "<div class=\"repo-nav\">\n"
- , navLink' repoName "summary" "summary" active
- , navLink' repoName "shortlog" "shortlog" active
- , navLink' repoName "log" "log" active
- , navLink' repoName "tree" "tree" active
- , navLink' repoName "tags" "tags" active
- , "</div>\n"
+ [ "<nav class=\"repo-nav\" aria-label=\"Repository sections\">\n"
+ , navLink repoName "summary" "summary" active
+ , navLink repoName "shortlog" "shortlog" active
+ , navLink repoName "log" "log" active
+ , navLink repoName "tree" "tree" active
+ , navLink repoName "tags" "tags" active
+ , "</nav>\n"
hunk ./src/DarcsWeb/Html.hs 468
-navLink' :: Text -> Text -> Text -> Text -> Text
-navLink' repoName path label active =
- if path == active
- then "<a href=\"/repo/" <> esc repoName <> "/" <> path <> "\" class=\"active\">" <> label <> "</a>\n"
- else "<a href=\"/repo/" <> esc repoName <> "/" <> path <> "\">" <> label <> "</a>\n"
-
--- | HTML-escape text.
--- Uses formally verified implementation from Coq/Rocq.
+navLink :: Text -> Text -> Text -> Text -> Text
+navLink repoName path label active =
+ let nameSeg = encodePathSegment repoName
+ in if path == active
+ then "<a href=\"/repo/" <> nameSeg <> "/" <> path <> "\" class=\"active\" aria-current=\"page\">" <> label <> "</a>\n"
+ else "<a href=\"/repo/" <> nameSeg <> "/" <> path <> "\">" <> label <> "</a>\n"
+
+-- | HTML-escape text. Native 'Text' implementation that replaces the
+-- previous @T.pack . HtmlPure.esc . T.unpack@ bridge.
+--
+-- The hot path is the patch-detail page, where every diff line is
+-- escaped. 'T.break' lets us emit the longest run of safe characters
+-- as a single 'Text' chunk instead of allocating a fresh
+-- 'T.singleton' per non-special character (which is what a plain
+-- 'T.concatMap' does). Equivalence against the Coq-extracted
+-- 'HtmlPure.esc' is pinned by the @esc matches verified@ property in
+-- 'Properties.Html'.
hunk ./src/DarcsWeb/Html.hs 486
-esc = T.pack . HtmlPure.esc . T.unpack
+esc t
+ | T.null t = t
+ | otherwise =
+ let (safe, rest) = T.break needsEsc t
+ in case T.uncons rest of
+ Nothing -> safe
+ Just (c, after) -> safe <> escapeChar c <> esc after
+ where
+ needsEsc c = c == '<' || c == '>' || c == '&' || c == '"' || c == '\''
+ escapeChar '<' = "<"
+ escapeChar '>' = ">"
+ escapeChar '&' = "&"
+ escapeChar '"' = """
+ escapeChar '\'' = "'"
+ escapeChar c = T.singleton c -- unreachable given needsEsc, kept total
hunk ./src/DarcsWeb/Html.hs 520
- Just t ->
- let secs = floor (diffUTCTime now t) :: Int
- in if secs < 0 then formatAbsolute dateStr
- else if secs < 60 then pluralize secs "second"
- else if secs < 3600 then pluralize (secs `div` 60) "minute"
- else if secs < 86400 then pluralize (secs `div` 3600) "hour"
- else if secs < 604800 then pluralize (secs `div` 86400) "day"
- else if secs < 2419200 then pluralize (secs `div` 604800) "week"
- else formatAbsolute dateStr
+ Just t -> bucket (floor (diffUTCTime now t) :: Int)
hunk ./src/DarcsWeb/Html.hs 522
+ bucket secs
+ | secs < 0 = formatAbsolute dateStr
+ | secs < minuteSeconds = pluralize secs "second"
+ | secs < hourSeconds = pluralize (secs `div` minuteSeconds) "minute"
+ | secs < daySeconds = pluralize (secs `div` hourSeconds) "hour"
+ | secs < weekSeconds = pluralize (secs `div` daySeconds) "day"
+ | secs < fourWeeksSeconds = pluralize (secs `div` weekSeconds) "week"
+ | otherwise = formatAbsolute dateStr
+
hunk ./src/DarcsWeb/Html.hs 532
- T.pack (show n) <> " " <> T.pack unit <> if n == 1 then " ago" else "s ago"
+ tshow n <> " " <> T.pack unit <> if n == 1 then " ago" else "s ago"
+
+minuteSeconds, hourSeconds, daySeconds, weekSeconds, fourWeeksSeconds :: Int
+minuteSeconds = 60
+hourSeconds = 60 * minuteSeconds
+daySeconds = 24 * hourSeconds
+weekSeconds = 7 * daySeconds
+fourWeeksSeconds = 4 * weekSeconds
hunk ./src/DarcsWeb/Html.hs 547
-formatAbsolute d =
- let s = T.unpack d
- in if length s >= 14
- then T.pack $ take 4 s ++ "-" ++ take 2 (drop 4 s) ++ "-" ++ take 2 (drop 6 s)
- ++ " " ++ take 2 (drop 8 s) ++ ":" ++ take 2 (drop 10 s) ++ ":" ++ take 2 (drop 12 s)
- else d
+formatAbsolute d
+ | T.length d >= 14 =
+ let (y, r1) = T.splitAt 4 d
+ (mo, r2) = T.splitAt 2 r1
+ (dy, r3) = T.splitAt 2 r2
+ (h, r4) = T.splitAt 2 r3
+ (mi, r5) = T.splitAt 2 r4
+ (se, _) = T.splitAt 2 r5
+ in y <> "-" <> mo <> "-" <> dy <> " "
+ <> h <> ":" <> mi <> ":" <> se
+ | otherwise = d
hunk ./src/DarcsWeb/Html.hs 567
+
+-- | @Text@-flavoured 'show', used to avoid the repeated
+-- @T.pack . show@ noise in this module.
+tshow :: Show a => a -> Text
+tshow = T.pack . show
hunk ./test/Properties/Clone.hs 9
-import DarcsWeb.Darcs (isSafeSubPath)
+import DarcsWeb.Darcs (isSafeSubPath, isCloneSubPath)
hunk ./test/Properties/Clone.hs 162
+-- isCloneSubPath: allowlist pinning for the read-only HTTP clone
+-- endpoint. The predicate is tighter than isSafeSubPath so that
+-- unrelated files under _darcs/ stay private.
+-- --------------------------------------------------------------------------
+
+acceptsClone :: FilePath -> Bool
+acceptsClone = isCloneSubPath
+
+rejectsClone :: FilePath -> Bool
+rejectsClone = not . isCloneSubPath
+
+prop_clone_allowlist_accepts_core :: Bool
+prop_clone_allowlist_accepts_core = and
+ [ acceptsClone "hashed_inventory"
+ , acceptsClone "inventories/0000000123abc"
+ , acceptsClone "patches/ffbbaa"
+ , acceptsClone "pristine.hashed/deadbeef"
+ ]
+
+prop_clone_allowlist_accepts_packs :: Bool
+prop_clone_allowlist_accepts_packs =
+ acceptsClone "packs/basic.tar.gz" &&
+ acceptsClone "packs/patches.tar.gz"
+
+prop_clone_allowlist_rejects_prefs :: Bool
+prop_clone_allowlist_rejects_prefs = and
+ [ rejectsClone "prefs/defaultrepo"
+ , rejectsClone "prefs/author"
+ , rejectsClone "prefs/motd"
+ , rejectsClone "prefs/repo_description"
+ ]
+
+prop_clone_allowlist_rejects_other :: Bool
+prop_clone_allowlist_rejects_other = and
+ [ rejectsClone "format"
+ , rejectsClone "tentative_hashed_pristine"
+ , rejectsClone "packs/other.tar.gz"
+ , rejectsClone "hooks/pre-apply"
+ , rejectsClone "scripts/boom.sh"
+ ]
+
+-- Pin the exact two-segment shape for hash directories; deeper nesting
+-- must not be accepted. Catches a regression to the previous
+-- @("patches" : _ : _)@ wildcard-tail pattern, which admitted any depth.
+prop_clone_allowlist_rejects_nested :: Bool
+prop_clone_allowlist_rejects_nested = and
+ [ rejectsClone "patches/aa/bb"
+ , rejectsClone "inventories/aa/bb"
+ , rejectsClone "pristine.hashed/aa/bb"
+ , rejectsClone "patches/aa/bb/cc"
+ , rejectsClone "packs/basic.tar.gz/extra"
+ ]
+
+prop_clone_allowlist_rejects_unsafe :: Bool
+prop_clone_allowlist_rejects_unsafe = and
+ [ rejectsClone ""
+ , rejectsClone ".."
+ , rejectsClone "../secret"
+ , rejectsClone "patches/../../etc/passwd"
+ , rejectsClone ".git/config"
+ , rejectsClone "patches/.hidden"
+ ]
+
+-- isCloneSubPath must never accept anything isSafeSubPath rejects.
+prop_clone_allowlist_refines_safe :: String -> Bool
+prop_clone_allowlist_refines_safe s =
+ not (isCloneSubPath s) || isSafeSubPath s
+
+-- --------------------------------------------------------------------------
hunk ./test/Properties/Clone.hs 254
+ , run "clone allowlist core" (property prop_clone_allowlist_accepts_core)
+ , run "clone allowlist packs" (property prop_clone_allowlist_accepts_packs)
+ , run "clone allowlist rejects prefs" (property prop_clone_allowlist_rejects_prefs)
+ , run "clone allowlist rejects other" (property prop_clone_allowlist_rejects_other)
+ , run "clone allowlist rejects nested" (property prop_clone_allowlist_rejects_nested)
+ , run "clone allowlist rejects unsafe" (property prop_clone_allowlist_rejects_unsafe)
+ , run "clone allowlist refines safe" prop_clone_allowlist_refines_safe
hunk ./test/Properties/Html.hs 8
-import DarcsWeb.Html (esc, highlightDiff, shortAuthor, formatSize, formatAbsolute)
+import qualified HtmlPure
+import DarcsWeb.Html (esc, highlightDiff, shortAuthor, formatSize, formatAbsolute,
+ renderTreeBreadcrumb, renderBlobBreadcrumb)
hunk ./test/Properties/Html.hs 36
+-- The native Text implementation of esc must agree with the Coq-extracted
+-- HtmlPure.esc on every input. This pins the verification boundary after
+-- the performance rewrite: a regression in the fast path is caught by a
+-- QuickCheck failure instead of shipping as an unnoticed XSS.
+-- A biased generator oversamples the five escaped characters so every
+-- run exercises them, rather than relying on default String arbitraries
+-- which almost never produce '<', '>', '&', '"', or '\''.
+prop_esc_matches_verified :: Property
+prop_esc_matches_verified =
+ forAll escBiased $ \s ->
+ esc (T.pack s) == T.pack (HtmlPure.esc s)
+ where
+ escBiased = listOf (frequency [(5, elements "<>&\"'"), (1, arbitrary)])
+
hunk ./test/Properties/Html.hs 51
+-- breadcrumb properties: pin the escape + percent-encode contracts that
+-- fixed the tree-breadcrumb HTML-injection sink.
+-- --------------------------------------------------------------------------
+
+-- Labels containing '<' must appear escaped; no raw '<' should survive.
+prop_breadcrumb_escapes_lt :: Bool
+prop_breadcrumb_escapes_lt =
+ let out = renderTreeBreadcrumb "repo" "<script>"
+ in not (T.isInfixOf "<script>" out)
+ && T.isInfixOf "<script>" out
+
+-- '?', '#', ' ' in a segment must become percent-encoded in the href
+-- but HTML-escaped (not percent-encoded) in the visible label.
+prop_breadcrumb_percent_encodes_query :: Bool
+prop_breadcrumb_percent_encodes_query =
+ let out = renderTreeBreadcrumb "repo" "a?b"
+ in T.isInfixOf "href=\"/repo/repo/tree/a%3Fb\"" out
+ && T.isInfixOf ">a?b</a>" out
+
+prop_breadcrumb_percent_encodes_space :: Bool
+prop_breadcrumb_percent_encodes_space =
+ let out = renderTreeBreadcrumb "repo" "a b"
+ in T.isInfixOf "href=\"/repo/repo/tree/a%20b\"" out
+
+-- A nested path gets one '/' separator per segment in the href, and the
+-- cumulative prefix is extended at each step (not reset).
+prop_breadcrumb_nested_prefix :: Bool
+prop_breadcrumb_nested_prefix =
+ let out = renderTreeBreadcrumb "repo" "a/b/c"
+ in T.isInfixOf "/tree/a\"" out
+ && T.isInfixOf "/tree/a/b\"" out
+ && T.isInfixOf "/tree/a/b/c\"" out
+
+-- Blob breadcrumb on an empty sub-path degrades to a bare [root] anchor
+-- with no trailing separator.
+prop_blob_breadcrumb_empty_total :: Bool
+prop_blob_breadcrumb_empty_total =
+ let out = renderBlobBreadcrumb "repo" ""
+ in T.isInfixOf "[root]" out
+ && not (T.isInfixOf " / " out)
+
+-- Blob breadcrumb escapes the file-name label.
+prop_blob_breadcrumb_escapes_filename :: Bool
+prop_blob_breadcrumb_escapes_filename =
+ let out = renderBlobBreadcrumb "repo" "<evil>.txt"
+ in not (T.isInfixOf "<evil>" out)
+ && T.isInfixOf "<evil>.txt" out
+
+-- --------------------------------------------------------------------------
hunk ./test/Properties/Html.hs 199
+ , run "esc matches verified" prop_esc_matches_verified
+ , run "breadcrumb escapes <" (property prop_breadcrumb_escapes_lt)
+ , run "breadcrumb encodes ?" (property prop_breadcrumb_percent_encodes_query)
+ , run "breadcrumb encodes space" (property prop_breadcrumb_percent_encodes_space)
+ , run "breadcrumb nested prefix" (property prop_breadcrumb_nested_prefix)
+ , run "blob breadcrumb empty total" (property prop_blob_breadcrumb_empty_total)
+ , run "blob breadcrumb escapes name" (property prop_blob_breadcrumb_escapes_filename)
hunk ./test/Spec.hs 9
+import qualified Properties.Config as Config
hunk ./test/Spec.hs 34
- if htmlOk && cspOk && cloneOk
+ putStrLn "\n=== Running QuickCheck properties (Config) ==="
+ cfgOk <- Config.tests
+
+ if htmlOk && cspOk && cloneOk && cfgOk
