client/specs/reviews/002-adversarial-review-phase3.md

Adversarial Review 002 - Post Spec 004 (Projects & Usage)

Date: 2026-03-07 Scope: Full codebase review after specs 001-004 Tests: 53 total (17 core + 36 server), clippy clean Verified: Against real forest-server on localhost:4040

---

1. Architecture: Repeated gRPC Calls Per Page Load

Severity: High

Every authenticated platform page (projects_list, project_detail, usage) calls list_my_organisations to verify membership. This means:

• /orgs/testorg/projects -> 1 call to list orgs + 1 call to list projects = 2 gRPC calls
• /orgs/testorg/projects/my-api -> 1 call to list orgs + 1 call to list artifacts = 2 gRPC calls
• Dashboard -> 1 call to list orgs (redirect) then the target page makes its own calls

This is the same pattern we fixed for get_user() in spec 003 (caching user in session). The org list should be cached in the session too, or at minimum passed through from the Session extractor.

Recommendation: Cache the user's org memberships in SessionData / CachedUser. Refresh on session refresh or after a configurable TTL. This eliminates the most expensive repeated call.

---

2. Architecture: Two Traits, One Struct, Inconsistent Error Handling

Severity: Medium

GrpcForestClient now implements both ForestAuth and ForestPlatform. The authed_request helper is duplicated:

• GrpcForestClient::authed_request() returns AuthError
• platform_authed_request() is a free function returning PlatformError

Same logic, two copies, two error types. AppState holds Arc<dyn ForestAuth> + Arc<dyn ForestPlatform> which in production point to the same struct. This is fine for testability but means the constructors are getting wide (4 args now).

Recommendation: Consider a single ForestClient trait that combines both, or unify the auth helper into a generic form. Not urgent but will become pain as more services are added.

---

3. Security: Org Name in URL Path is User-Controlled

Severity: Medium

Routes use {org} from the URL path and pass it directly to gRPC calls and template rendering:

• format!("{org} - Projects - Forage") in HTML title
• format!("Projects in {org}") in meta description

MiniJinja auto-escapes by default in HTML context, so XSS via <script> in org name is mitigated. However:

• The title tag is outside normal HTML body escaping in some edge cases
• The description meta tag uses attribute context escaping

Recommendation: Validate or sanitize {org} and {project} path params at the route level. The org membership check already prevents arbitrary names from rendering (403 if not a member), but defense in depth matters.

---

4. Session: last_seen_at Updated on Every Request

Severity: Low

The Session extractor calls state.sessions.update() on every single request to update last_seen_at. For the PostgreSQL store, this means a write query per page load. For the in-memory store, it's a write lock on the HashMap.

Recommendation: Only update last_seen_at if the previous value is older than some threshold (e.g., 5 minutes). This is a simple check that eliminates 95%+ of session writes.

---

5. Testing: No Test for the ForestPlatform gRPC Implementation

Severity: Medium

The GrpcForestClient ForestPlatform impl (lines 294-393 of forest_client.rs) has zero test coverage. It's only tested indirectly via integration tests that use MockPlatformClient. The mapping from proto types to domain types (Organisation, Artifact) is untested.

Specifically:

• The zip(resp.roles) could silently truncate if lengths don't match
• The unwrap_or_default() on a.context hides missing data
• The empty-string-to-None conversion for description is a subtle behavior

Recommendation: Add unit tests for the proto-to-domain conversion functions. Extract them into named functions (like convert_user and convert_token for auth) to make them testable.

---

6. Testing: Dashboard Test Changed Behavior Without Full Coverage

Severity: Medium

dashboard_with_session_returns_200 was renamed to dashboard_with_session_redirects_to_org and now only checks for StatusCode::SEE_OTHER. The old test verified the dashboard rendered with testuser content. The new behavior (redirect) is tested, but the onboarding page content is only tested in dashboard_no_orgs_shows_onboarding which checks for "forest orgs create".

Nobody tests:

• What happens if list_my_organisations returns an error (not empty, an actual error)
• The dashboard template rendering is correct (title, user info)

Recommendation: Add test for platform unavailable during dashboard load.

---

7. VSDD Process: Spec 004 Skipped the Red Gate

Severity: Medium (Process)

The VSDD spec says "All tests must fail before implementation begins." In spec 004, we wrote templates, routes, AND tests in the same step. Tests never had a Red phase - they were green on first run. This is pragmatic but violates VSDD.

The earlier specs (001-003) had proper Red->Green cycles. Spec 004 was implemented as "write everything at once."

Recommendation: For future specs, write the test assertions first with stub routes that return 501/500, verify they fail, then implement. Even if the cycle is fast, the discipline catches assumption errors.

---

8. Template: No Authenticated Navigation

Severity: Medium (UX)

The spec called for "Authenticated navigation with org switcher" but it wasn't implemented. All pages (projects, usage, onboarding) use the same marketing base.html.jinja which shows "Pricing / Components / Sign in" in the nav, even when the user is authenticated and browsing their org's projects.

This means:

• No way to switch orgs from the nav
• No visual indication you're logged in (except the page content)
• No link back to projects/usage from the nav on authenticated pages

Recommendation: Either pass user and orgs to the base template and conditionally render an app nav, or create a separate app_base.html.jinja that authenticated pages extend.

---

9. Error UX: Raw Status Codes as Responses

Severity: Medium

403 and 500 errors return bare Axum status codes with no HTML body:

• Non-member accessing /orgs/someorg/projects -> blank 403 page
• Template error -> blank 500 page

Recommendation: Add simple error templates (403.html.jinja, 500.html.jinja) and render them instead of bare status codes. Even a one-line "You don't have access to this organisation" is better than a browser default error page.

---

10. Code: expires_in_seconds is Suspiciously Large

Severity: Low (Upstream)

During integration testing, forest-server returned expiresInSeconds: 1775498883 which is ~56 years. This is likely a bug in forest-server (perhaps it's returning an absolute timestamp instead of a duration). Our code treats it as a duration: now + Duration::seconds(tokens.expires_in_seconds). If forest-server is actually returning a Unix timestamp, we'd set expiry to year 2082.

The session refresh logic would never trigger, which means tokens are effectively permanent. The BFF session protects the browser from this (sessions expire by last_seen_at reaper), but the underlying token is never refreshed.

Recommendation: Verify with forest-server what expires_in_seconds actually means. If it's a bug, cap it to a sane maximum (e.g., 24h) client-side.

---

11. Missing: CSRF Protection on State-Mutating Endpoints

Severity: Medium (Security)

POST /logout, POST /login, POST /signup, POST /settings/tokens, POST /settings/tokens/{id}/delete all accept form submissions with no CSRF token. The SameSite=Lax cookie provides baseline protection against cross-origin POST from foreign sites, but:

• SameSite=Lax allows top-level navigations (e.g., form auto-submit from a link)
• A CSRF token is the standard defense-in-depth

Recommendation: Add CSRF tokens to all forms. MiniJinja can render a hidden <input> and the server validates it against a session-bound value.

---

Prioritized Actions

Must Do (before next feature)

1. Error pages: Add 403/500 error templates (bare status codes are bad UX)
2. Authenticated nav: Implement app navigation for logged-in users
3. Platform-unavailable test: Add test for dashboard when list_my_organisations errors

Should Do (this iteration)

4. Cache org memberships in session: Eliminate repeated list_my_organisations gRPC call
5. Throttle session writes: Only update last_seen_at if stale (>5min)
6. Extract proto conversion functions: Make them testable, add unit tests
7. CSRF tokens: Add to all POST forms

Do When Relevant

8. Unify auth helper: Deduplicate authed_request / platform_authed_request
9. Validate path params: Sanitize {org} and {project} at route level
10. Investigate expires_in_seconds: Confirm forest-server semantics, cap if needed
11. VSDD discipline: Enforce Red Gate for future specs