Phase 2 #4

Merged

navicore merged 3 commits from phase-2 into main

2026-04-05 22:23:31 +00:00

navicore commented

2026-04-05 22:23:15 +00:00

(Migrated from github.com)

No description provided.

claude[bot] commented

2026-04-05 22:27:35 +00:00

(Migrated from github.com)

Phase 2 Review

Good, solid work — rate limiting, audit logging, branding, session management, and RFC 7009 revocation are all implemented cleanly. The 53-test suite is a major improvement over Phase 1. A few things worth looking at:

Security

custom_css is HTML-escaped by Askama (templates/login.html)

{% when Some with (css) %}
<style>{{ css }}</style>

Askama escapes {{ }} variables as HTML by default, so characters like <, >, & in the CSS value will be rendered as <, >, etc. This means CSS containing those characters (e.g. content: "<", attribute selectors) will be silently broken. Since this is operator-controlled config, the fix is to render it raw with {{ css | safe }} and accept that only the operator can set it. The current behaviour is confusing — it looks like it works but quietly corrupts valid CSS.

validate_aud = false in src/crypto/token.rs

validation.validate_aud = false;

This was added without a comment. For a JWT library that validates claims, silently disabling audience validation is a non-obvious security trade-off. Even if it's intentional (public clients, no fixed audience), a comment explaining the reasoning would help the next reader avoid re-adding it by mistake.

Rate limiter map grows unbounded (src/server/mod.rs)

login_attempts: HashMap<IpAddr, Vec<Instant>> accumulates one entry per unique source IP and is never cleaned up. retain prunes old timestamps for the IP being checked, but entries for IPs that never check again stay forever. On a reachable server this could slowly consume memory. A simple fix: after retain, remove the entry if it becomes empty:

entry.retain(|t| *t > cutoff);
if entry.is_empty() {
    attempts.remove(&ip);
}

revoke.rs — no client authentication

RFC 7009 §2.1 says requests SHOULD include client credentials. The current implementation accepts revocations from anyone with knowledge of a valid refresh token. For public-client-only deployments this is acceptable, but it should be called out as a documented gap (it isn't listed in ROADMAP.md's Known Gaps).

Code Quality

branding_title used as submit button text (templates/login.html)

<button type="submit">{{ branding_title }}</button>

Before this change the button said "Sign In". Now if title is set to "Acme Corp", the submit button reads "Acme Corp". The title was designed for the <h1> heading — the button should probably stay "Sign In" or get its own button_label config field.

hex_encode allocates per byte (src/crypto/mod.rs)

pub fn hex_encode(bytes: &[u8]) -> String {
    bytes.iter().map(|b| format!("{b:02x}")).collect()
}

format! allocates a String per byte. At SHA-256 output size (32 bytes) this is 32 small heap allocations. It works fine at this scale, but the idiomatic Rust approach avoids the allocations:

use std::fmt::Write;
pub fn hex_encode(bytes: &[u8]) -> String {
    let mut s = String::with_capacity(bytes.len() * 2);
    for b in bytes { let _ = write!(s, "{b:02x}"); }
    s
}

Duplicate realm+user lookup in src/cli/session.rs

The List and Revoke arms both do identical realm-then-user lookups. Not a bug, but if a third session subcommand is added it becomes a maintenance burden. A small helper function would help.

Test Coverage

These new modules have no tests:

src/server/static_files.rs — path traversal protection is security-critical; worth at least a test that attempts ../ traversal and gets 400/404
src/server/revoke.rs — the core RFC 7009 logic (unknown token → 200, valid token → 200 + revoked, access_token hint → no-op) is untested
src/cli/session.rs handler — the list/revoke/cleanup dispatch is untested

The existing test for static_files is implicit (relies on canonicalize returning NOT_FOUND for nonexistent paths). An explicit traversal test would be more trustworthy.

Minor Notes

audit_log_enabled defaults to true, writing audit.log in the current working directory on first run without config. This is surprising and could fail silently if the CWD is not writable (it does log a warning). Consider defaulting to false and letting operators opt in.
The #[cfg(test)] pub fn open_in_memory() helper in src/db/mod.rs is a clean pattern — good call exposing it there rather than duplicating setup in each test module.
CI now runs on macOS only on push to main (not on PRs). If a PR breaks the macOS build you won't find out until after merge. Worth considering running macOS CI on PRs too, accepting the longer feedback loop.

Overall this is well-structured code with good separation of concerns, idiomatic Rust error handling, and solid coverage of the core cryptographic and DB paths. The issues above are mostly minor polish items — the main ones worth acting on are the custom_css escaping bug, the rate limiter memory growth, and the missing traversal test for static_files.

## Phase 2 Review Good, solid work — rate limiting, audit logging, branding, session management, and RFC 7009 revocation are all implemented cleanly. The 53-test suite is a major improvement over Phase 1. A few things worth looking at: --- ### Security **`custom_css` is HTML-escaped by Askama (`templates/login.html`)** ```html {% when Some with (css) %} <style>{{ css }}</style> ``` Askama escapes `{{ }}` variables as HTML by default, so characters like `<`, `>`, `&` in the CSS value will be rendered as `<`, `>`, etc. This means CSS containing those characters (e.g. `content: "<"`, attribute selectors) will be silently broken. Since this is operator-controlled config, the fix is to render it raw with `{{ css | safe }}` and accept that only the operator can set it. The current behaviour is confusing — it looks like it works but quietly corrupts valid CSS. **`validate_aud = false` in `src/crypto/token.rs`** ```rust validation.validate_aud = false; ``` This was added without a comment. For a JWT library that validates claims, silently disabling audience validation is a non-obvious security trade-off. Even if it's intentional (public clients, no fixed audience), a comment explaining the reasoning would help the next reader avoid re-adding it by mistake. **Rate limiter map grows unbounded (`src/server/mod.rs`)** `login_attempts: HashMap<IpAddr, Vec<Instant>>` accumulates one entry per unique source IP and is never cleaned up. `retain` prunes old timestamps for the IP being *checked*, but entries for IPs that never check again stay forever. On a reachable server this could slowly consume memory. A simple fix: after `retain`, remove the entry if it becomes empty: ```rust entry.retain(|t| *t > cutoff); if entry.is_empty() { attempts.remove(&ip); } ``` **`revoke.rs` — no client authentication** RFC 7009 §2.1 says requests SHOULD include client credentials. The current implementation accepts revocations from anyone with knowledge of a valid refresh token. For public-client-only deployments this is acceptable, but it should be called out as a documented gap (it isn't listed in ROADMAP.md's Known Gaps). --- ### Code Quality **`branding_title` used as submit button text (`templates/login.html`)** ```html <button type="submit">{{ branding_title }}</button> ``` Before this change the button said "Sign In". Now if `title` is set to "Acme Corp", the submit button reads "Acme Corp". The title was designed for the `<h1>` heading — the button should probably stay "Sign In" or get its own `button_label` config field. **`hex_encode` allocates per byte (`src/crypto/mod.rs`)** ```rust pub fn hex_encode(bytes: &[u8]) -> String { bytes.iter().map(|b| format!("{b:02x}")).collect() } ``` `format!` allocates a `String` per byte. At SHA-256 output size (32 bytes) this is 32 small heap allocations. It works fine at this scale, but the idiomatic Rust approach avoids the allocations: ```rust use std::fmt::Write; pub fn hex_encode(bytes: &[u8]) -> String { let mut s = String::with_capacity(bytes.len() * 2); for b in bytes { let _ = write!(s, "{b:02x}"); } s } ``` **Duplicate realm+user lookup in `src/cli/session.rs`** The `List` and `Revoke` arms both do identical realm-then-user lookups. Not a bug, but if a third session subcommand is added it becomes a maintenance burden. A small helper function would help. --- ### Test Coverage These new modules have no tests: - `src/server/static_files.rs` — path traversal protection is security-critical; worth at least a test that attempts `../` traversal and gets 400/404 - `src/server/revoke.rs` — the core RFC 7009 logic (unknown token → 200, valid token → 200 + revoked, access_token hint → no-op) is untested - `src/cli/session.rs` handler — the list/revoke/cleanup dispatch is untested The existing test for `static_files` is implicit (relies on canonicalize returning NOT_FOUND for nonexistent paths). An explicit traversal test would be more trustworthy. --- ### Minor Notes - `audit_log_enabled` defaults to `true`, writing `audit.log` in the current working directory on first run without config. This is surprising and could fail silently if the CWD is not writable (it does log a warning). Consider defaulting to `false` and letting operators opt in. - The `#[cfg(test)] pub fn open_in_memory()` helper in `src/db/mod.rs` is a clean pattern — good call exposing it there rather than duplicating setup in each test module. - CI now runs on macOS only on push to main (not on PRs). If a PR breaks the macOS build you won't find out until after merge. Worth considering running macOS CI on PRs too, accepting the longer feedback loop. --- Overall this is well-structured code with good separation of concerns, idiomatic Rust error handling, and solid coverage of the core cryptographic and DB paths. The issues above are mostly minor polish items — the main ones worth acting on are the `custom_css` escaping bug, the rate limiter memory growth, and the missing traversal test for `static_files`.