153 lines
7.9 KiB
Markdown
153 lines
7.9 KiB
Markdown
# Implementation Plan: Code Audit Fixes
|
||
|
||
## Overview
|
||
|
||
Surgical fixes for 13 Critical + High audit issues across the ArchNest backend (Fastify 5 + TypeScript) and frontend (React 19 + TypeScript), plus a CloudFormation test deploy template. Each task targets a specific file with a minimal, correct patch. No new dependencies introduced.
|
||
|
||
## Tasks
|
||
|
||
- [ ] 1. Backend security hardening — input validation and auth
|
||
- [ ] 1.1 Add path traversal prevention to `backend/src/routes/files.ts`
|
||
- Add `validatePath()` function using `posix.normalize()` to reject absolute paths and `..` traversal
|
||
- Apply validation at the top of every handler that accepts a user-supplied path (list, content read, content write, mkdir, rename, delete, chmod, download, upload)
|
||
- Return HTTP 400 with descriptive error on rejection
|
||
- _Requirements: 6.1, 6.2, 6.3, 6.4_
|
||
|
||
- [ ] 1.2 Fix agent token timing-safe comparison in `backend/src/routes/agents.ts`
|
||
- Replace early-return on length mismatch with padded constant-time comparison
|
||
- Pad both buffers to `Math.max(a.length, b.length)` before `timingSafeEqual`
|
||
- Ensure `ok: true` only when both length AND content match
|
||
- Same 401 response for all rejection cases
|
||
- _Requirements: 7.1, 7.2, 7.3_
|
||
|
||
- [ ] 1.3 Add session log integrationId validation in `backend/src/routes/terminal.ts`
|
||
- Update `sessionLogPath()` to return `null` when integrationId is not a positive integer
|
||
- Callers skip logging on `null` return
|
||
- _Requirements: 13.1, 13.2_
|
||
|
||
- [ ] 1.4 Add body size limit to data import in `backend/src/routes/data.ts`
|
||
- Add `bodyLimit: 10 * 1024 * 1024` to the POST `/api/data/import` route registration
|
||
- Fastify returns 413 automatically when exceeded
|
||
- _Requirements: 8.1, 8.2_
|
||
|
||
- [ ] 1.5 Set CORS origin to fail-closed default in `backend/src/server.ts`
|
||
- Change fallback from `true` to `false` when `ARCHNEST_CORS_ORIGIN` is not set
|
||
- Log a warning at startup when env var is missing
|
||
- _Requirements: 10.1, 10.2, 10.3_
|
||
|
||
- [ ]* 1.6 Write property tests for path traversal, agent token, and integrationId validation
|
||
- **Property 3: Path Traversal Prevention** — verify `validatePath` rejects all `..` escape patterns and accepts valid relative paths
|
||
- **Property 4: Agent Token Comparison Correctness** — verify `ok: true` iff `presented === expected`, never throws
|
||
- **Property 8: Integration ID Numeric Validation** — verify `sessionLogPath` returns null for non-positive-integer values
|
||
- **Validates: Requirements 6.1, 6.2, 7.1, 7.2, 7.3, 13.1, 13.2**
|
||
|
||
- [ ] 2. Checkpoint — Backend security
|
||
- Ensure all tests pass, ask the user if questions arise.
|
||
|
||
- [ ] 3. WebSocket authentication refactor
|
||
- [ ] 3.1 Implement first-message auth gate in `backend/src/routes/terminal.ts`
|
||
- Add `authenticated` flag per connection
|
||
- Require `{ type: 'auth', token }` as first message; verify JWT
|
||
- Reject all other message types before auth with error frame + close
|
||
- Remove `req.query.token` usage from connect/list_tmux handlers
|
||
- _Requirements: 5.3, 5.4_
|
||
|
||
- [ ] 3.2 Implement first-message auth gate in `backend/src/routes/docker.ts`
|
||
- Same pattern as terminal: auth-first protocol for Docker exec WebSocket
|
||
- Verify existing JSON parse error handling remains intact (already correct per design)
|
||
- _Requirements: 5.3, 5.4, 12.1, 12.2_
|
||
|
||
- [ ] 3.3 Update frontend WebSocket connections in `src/lib/TerminalSessionContext.tsx`
|
||
- Remove `?token=` from WebSocket URL query string
|
||
- Send `{ type: 'auth', token }` as first message on `ws.onopen`
|
||
- Apply same change to `fetchTmuxSessions()` WebSocket
|
||
- _Requirements: 5.1, 5.2_
|
||
|
||
- [ ] 3.4 Fix WebSocket session leak in `src/lib/TerminalSessionContext.tsx`
|
||
- Guard `ws.close()` with readyState check (OPEN or CONNECTING)
|
||
- Capture `thisWs` reference; in `onclose`, bail if `s.ws !== thisWs`
|
||
- _Requirements: 1.1, 1.2, 1.3_
|
||
|
||
- [ ]* 3.5 Write property test for WebSocket auth gate
|
||
- **Property 5: WebSocket Auth Gate Rejects Unauthenticated Messages**
|
||
- Verify no message type other than `auth` is processed before authentication
|
||
- **Validates: Requirements 5.3, 5.4**
|
||
|
||
- [ ] 4. Checkpoint — WebSocket auth
|
||
- Ensure all tests pass, ask the user if questions arise.
|
||
|
||
- [ ] 5. Backend — tmux validation and SSH cleanup verification
|
||
- [ ] 5.1 Harden tmux session name usage in `backend/src/routes/terminal.ts`
|
||
- Ensure validated name is single-quoted in shell command construction
|
||
- Defense-in-depth: strip any `'` (impossible given regex, but belt+suspenders)
|
||
- _Requirements: 2.1, 2.2, 2.3_
|
||
|
||
- [ ] 5.2 Verify container ref validation in `backend/src/ssh/docker.ts`
|
||
- Confirm `CONTAINER_REF_RE` regex is `^[A-Za-z0-9][A-Za-z0-9_.-]{0,127}$`
|
||
- Confirm `shQuote()` is applied to all container ref interpolations
|
||
- Confirm SSH cleanup in `withSshClient` finally block is present
|
||
- If already correct (per design), add a code comment noting audit verification
|
||
- _Requirements: 3.1, 3.2, 3.3, 9.1, 9.2_
|
||
|
||
- [ ]* 5.3 Write property tests for tmux name validation and container ref validation
|
||
- **Property 1: tmux Session Name Validation Prevents Injection** — verify only `[A-Za-z0-9_-]{1,64}` passes
|
||
- **Property 2: Container Reference Validation and Safe Escaping** — verify regex and `shQuote` safety
|
||
- **Property 6: SSH Connection Cleanup Guarantee** — verify `withSshClient` always calls `.end()`
|
||
- **Validates: Requirements 2.1, 2.3, 3.1, 3.3, 9.1, 9.2**
|
||
|
||
- [ ] 6. Frontend stability fixes
|
||
- [ ] 6.1 Add `.catch()` to Sidebar promise in `src/components/Sidebar.tsx`
|
||
- Append `.catch(() => {})` to the `api.listIntegrations()` call
|
||
- Ensure integrations state remains null on failure (shows "Checking…")
|
||
- _Requirements: 4.1, 4.2_
|
||
|
||
- [ ] 6.2 Create Error Boundary component at `src/components/ErrorBoundary.tsx`
|
||
- React class component with `getDerivedStateFromError` + `componentDidCatch`
|
||
- Fallback UI: centered message + gold "Reload Page" button on dark background
|
||
- Log error to console
|
||
- _Requirements: 11.2, 11.3, 11.4_
|
||
|
||
- [ ] 6.3 Wrap Dashboard in ErrorBoundary in `src/App.tsx`
|
||
- Import and wrap the top-level Dashboard component tree
|
||
- _Requirements: 11.1_
|
||
|
||
- [ ]* 6.4 Write unit tests for ErrorBoundary and Sidebar error handling
|
||
- Verify ErrorBoundary renders fallback on child throw
|
||
- Verify Sidebar swallows rejected promise without crashing
|
||
- **Validates: Requirements 4.1, 4.2, 11.1, 11.2, 11.3, 11.4**
|
||
|
||
- [ ] 7. CloudFormation test deploy template
|
||
- [ ] 7.1 Create `infra/test-deploy.yml` CloudFormation template
|
||
- Parameters: KeyPairName (KeyPair type), NotificationEmail (String)
|
||
- Resources: SecurityGroup (SSH + HTTP/HTTPS), EC2 Instance (t4g.small, AL2023 ARM64, Docker + Compose via UserData), Budget alarm ($30/month, 80% threshold)
|
||
- Outputs: PublicIP, InstanceId
|
||
- _Requirements: 14.1, 14.2, 14.3, 14.4, 14.5, 14.6_
|
||
|
||
- [ ]* 7.2 Write smoke test validating CloudFormation template structure
|
||
- Verify required resource types, parameters, and outputs exist
|
||
- Validate YAML parses correctly
|
||
- **Validates: Requirements 14.1–14.6**
|
||
|
||
- [ ] 8. Final checkpoint
|
||
- Ensure all tests pass, ask the user if questions arise.
|
||
|
||
## Notes
|
||
|
||
- Tasks marked with `*` are optional and can be skipped for faster MVP
|
||
- Each task references specific requirements for traceability
|
||
- Components 3 (container ref), 9 (SSH cleanup), and 12 (JSON parse) are verified-already-correct per design — task 5.2 confirms with a code comment
|
||
- The project uses TypeScript throughout (Fastify 5 backend, React 19 frontend)
|
||
- No new dependencies are introduced; all fixes use Node.js built-ins and existing patterns
|
||
|
||
## Task Dependency Graph
|
||
|
||
```json
|
||
{
|
||
"waves": [
|
||
{ "id": 0, "tasks": ["1.1", "1.2", "1.3", "1.4", "1.5", "6.1", "6.2", "7.1"] },
|
||
{ "id": 1, "tasks": ["1.6", "6.3", "6.4", "7.2"] },
|
||
{ "id": 2, "tasks": ["3.1", "3.2", "5.1", "5.2"] },
|
||
{ "id": 3, "tasks": ["3.3", "3.4", "3.5", "5.3"] }
|
||
]
|
||
}
|
||
```
|