177 lines
8.8 KiB
Markdown
177 lines
8.8 KiB
Markdown
# 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
|