eng-pad-server/AUDIT.md

# AUDIT.md — eng-pad-server Security Audit

## Audit Date: 2026-03-24

### Summary

Comprehensive security and engineering review of the eng-pad-server
codebase. 17 issues identified across critical, high, and medium
severity levels. All critical and high issues resolved.

---

## Findings

### A-001: SQL Injection in Database Backup
- **Severity**: Critical
- **Status**: ~~Resolved~~
- **Location**: `cmd/eng-pad-server/snapshot.go`
- **Description**: Backup path interpolated into SQL via `fmt.Sprintf`
  without escaping. Allows arbitrary SQL execution if path contains
  single quotes.
- **Resolution**: Escape single quotes in the backup path before
  interpolation.

### A-002: Authentication Timing Attack
- **Severity**: High
- **Status**: ~~Resolved~~
- **Location**: `internal/auth/users.go`
- **Description**: Early return when user not found skips Argon2id
  computation, allowing username enumeration via response timing.
- **Resolution**: Always perform Argon2id verification against a dummy
  hash when user is not found, consuming constant time.

### A-003: Missing Notebook Ownership Checks
- **Severity**: High
- **Status**: ~~Resolved~~
- **Location**: `internal/server/notebooks_handler.go`
- **Description**: Authenticated endpoints load notebooks by ID without
  verifying the notebook belongs to the requesting user. An
  authenticated user could access any notebook by guessing IDs.
- **Resolution**: Added `user_id` check to all notebook queries in
  authenticated endpoints. Share link endpoints use the validated
  notebook ID from the share link.

### A-004: Unbounded Point Data Decoding
- **Severity**: High
- **Status**: ~~Resolved~~
- **Location**: `internal/render/svg.go`
- **Description**: `decodePoints()` panics on data not aligned to 4
  bytes, has no size limit (OOM risk), and doesn't filter NaN/Inf.
- **Resolution**: Added alignment check, 4MB size limit, NaN/Inf
  filtering. Function now returns error.

### A-005: Error Messages Leak Information
- **Severity**: High
- **Status**: ~~Resolved~~
- **Location**: Multiple REST handlers
- **Description**: `err.Error()` concatenated directly into HTTP JSON
  responses, leaking database structure and internal state.
- **Resolution**: All error responses use generic messages. Detailed
  errors logged server-side only.

### A-006: No Authorization on Share Link Revocation
- **Severity**: High
- **Status**: ~~Resolved~~
- **Location**: `internal/grpcserver/share.go`
- **Description**: Any authenticated user could revoke any share link
  by token, regardless of notebook ownership.
- **Resolution**: Added JOIN with notebooks table to verify the calling
  user owns the notebook before allowing revocation.

### A-007: Silent Row Scan Errors
- **Severity**: High
- **Status**: ~~Resolved~~
- **Location**: Multiple handlers
- **Description**: `continue` on row scan errors silently returns
  incomplete data without indication.
- **Resolution**: Scan errors now return 500 Internal Server Error in
  REST handlers.

### A-008: Web Server Missing TLS
- **Severity**: Medium
- **Status**: ~~Resolved~~
- **Location**: `internal/webserver/server.go`
- **Description**: Web UI served over plain HTTP. Session cookies marked
  `Secure: true` are ineffective without TLS.
- **Resolution**: Added TLS cert/key fields to web server config. Uses
  HTTPS when configured, falls back to HTTP for development.

### A-009: Missing Input Validation in Sync RPC
- **Severity**: Medium
- **Status**: ~~Resolved~~
- **Location**: `internal/grpcserver/sync.go`
- **Description**: No validation of page_size, stroke count, or point
  data alignment in SyncNotebook RPC.
- **Resolution**: Added validation: page_size must be REGULAR or LARGE,
  total strokes limited to 100,000, point_data must be 4-byte aligned.

### A-010: No Graceful Shutdown
- **Severity**: Medium
- **Status**: ~~Resolved~~
- **Location**: `cmd/eng-pad-server/server.go`
- **Description**: Signal handler terminates immediately without
  draining in-flight requests.
- **Resolution**: Graceful shutdown with 30-second timeout: gRPC
  GracefulStop, HTTP Shutdown, database Close.

### A-011: Missing CSRF Protection on Web Forms
- **Severity**: Medium
- **Status**: Accepted
- **Rationale**: Web UI is currently read-only (viewing synced
  notebooks). The only mutating form is login, which is not a CSRF
  target (attacker gains nothing by logging victim into their own
  account). Will add CSRF tokens when/if web UI gains write features.

### A-012: WebAuthn Sign Count Not Verified
- **Severity**: Medium
- **Status**: Accepted
- **Rationale**: Sign count regression detection is defense-in-depth
  against cloned authenticators. Risk is low for a personal service.
  Will add verification when WebAuthn is fully wired into the web UI.

### A-013: gRPC Per-Request Password Auth
- **Severity**: Medium (audit assessment) / Accepted (our assessment)
- **Status**: Accepted
- **Rationale**: By design. Password travels over TLS 1.3 (encrypted),
  stored in Android Keystore (hardware-backed). Sync is manual and
  infrequent. Token-based auth adds complexity without meaningful
  security gain for this use case.

### A-014: No Structured Logging
- **Severity**: Medium
- **Status**: Open
- **Description**: Only `fmt.Printf` to stdout. No log levels, no
  structured output, no request tracking.
- **Plan**: Add `log/slog` based logging in a future phase.

### A-015: Incomplete Config Validation
- **Severity**: Medium
- **Status**: ~~Resolved~~
- **Description**: TLS files not checked for existence at startup.
  Token TTL, WebAuthn config not validated.
- **Resolution**: Split validation into `validateFields()` and
  `validateFiles()`. Added checks for: TLS file existence, token TTL
  parseability and positivity, Argon2 params > 0, valid log level,
  non-empty listen addresses. Full test coverage in config_test.go.

### A-016: Inconsistent Error Types
- **Severity**: Low
- **Status**: Open
- **Description**: String errors instead of sentinel errors make
  error handling difficult for callers.

### A-017: No Race Condition Testing
- **Severity**: Low
- **Status**: ~~Resolved~~
- **Description**: Test suite does not use `-race` flag.
- **Resolution**: Added `make test-race` target that runs `go test -race ./...`.

---

## Priority Summary

| ID | Severity | Status |
|----|----------|--------|
| A-001 | Critical | ~~Resolved~~ |
| A-002 | High | ~~Resolved~~ |
| A-003 | High | ~~Resolved~~ |
| A-004 | High | ~~Resolved~~ |
| A-005 | High | ~~Resolved~~ |
| A-006 | High | ~~Resolved~~ |
| A-007 | High | ~~Resolved~~ |
| A-008 | Medium | ~~Resolved~~ |
| A-009 | Medium | ~~Resolved~~ |
| A-010 | Medium | ~~Resolved~~ |
| A-011 | Medium | Accepted |
| A-012 | Medium | Accepted |
| A-013 | Medium | Accepted |
| A-014 | Medium | Open |
| A-015 | Medium | ~~Resolved~~ |
| A-016 | Low | Open |
| A-017 | Low | ~~Resolved~~ |

## Design Note: Single-User Model

This server is designed for a single authenticated user. The users
table and multi-user schema exist but are used for one account only.
If multi-user support is ever needed, authentication will migrate to
MCIAS (Metacircular Identity and Access Service) — the standalone
auth implementation here will not be extended for multiple users.