adam/proposal-system
1
0
Fork 0
mirror of https://github.com/Sea-Haven-Industries/proposal-system.git synced 2026-09-30 08:53:15 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions
proposal-system/AUDIT-REPORT.md
Adam Moussa 4f1271eb50 audit: fix all Critical and High security/reliability issues across monorepo 
6-domain audit (API, web, mobile, lambdas, infra, QA) with fixes:

API security: scope internal API key middleware to allowed paths only,
return 401 on invalid key instead of falling through, remove unvalidated
JWT code path, sanitize error messages, add UpdateProposal validator,
remove status field from UpdateProposalRequest to prevent over-posting,
log swallowed exceptions in ProposalService.

Infrastructure: enforce SSL on all S3 buckets, encrypt SQS queues,
enable optional MFA on Cognito, add API Gateway access logging.

Lambdas: fix _retry_request undefined variable across all 4 Lambdas,
re-raise exceptions in pdf-extract/pdf-generate instead of swallowing,
add idempotency guard to suggestions Lambda.

Web: add ErrorBoundary, add auth loading state to ProtectedRoute,
add mutation error toasts in AdminWorkspace, fix dead Cognito link.

Mobile: add mutex to offline queue processing, distinguish permanent
vs retryable failures, register all screens for both roles, log sync
errors.

Swagger/OpenAPI: add Swashbuckle with JWT bearer security definition,
add ProducesResponseType attributes to key endpoints.

Includes AUDIT-REPORT.md with complete findings and CLAUDE.md project
instructions.
2026-05-27 18:18:44 -04:00

248 lines
11 KiB
Markdown
Raw Blame History

# Proposal System — Production Readiness Audit Report
**Date:** 2026-05-27
**Auditor:** Claude Code (6 parallel specialist agents)
**Scope:** Full monorepo — API, Web, Mobile, Lambdas, Infrastructure/CI/CD, QA/Testing
---
## Executive Summary
The Proposal System has a solid architectural foundation with clean separation of concerns, proper Cognito JWT auth at the API Gateway layer, encrypted RDS, and a working end-to-end flow. However, the audit uncovered **5 Critical**, **36 High**, **75+ Medium**, and **60+ Low** severity findings across all layers.
**The most urgent issues:**
1. **Internal API key middleware applies globally** — any endpoint can be accessed with the internal key, bypassing JWT auth entirely (API Critical)
2. **JWT validation skipped when Authority not configured** — tokens are parsed but signatures are never verified (API Critical)
3. **Lambda Function URL has AUTH_NONE** — publicly accessible, bypassing API Gateway authorization (Infra/Lambda Critical)
4. **JWT stored in localStorage** — vulnerable to XSS token theft (Web Critical)
5. **Zero test coverage across entire monorepo** — no test files, no test frameworks, CI passes with zero tests (QA Critical)
6. **DevMode has no environment guard** — if `Auth:DevMode=true` leaks to production, anyone can mint tokens for any role (API High)
---
## Findings by Domain
### 1. API Security (2 Critical, 8 High, 14 Medium, 13 Low)
#### Critical
| ID | Finding | File | Fix |
|----|---------|------|-----|
| API-C1 | Internal API key middleware applies to ALL routes, not scoped to internal paths. Any request with the key bypasses JWT and gets admin role. | `Middleware/InternalApiKeyMiddleware.cs:22-48` | Scope to internal-only paths via path check |
| API-C2 | Auth callback skips JWT signature validation when `Auth:Authority` is empty — tokens are parsed but never verified. `appsettings.json` defaults to `""`. | `Controllers/AuthController.cs:70-73` | Throw on empty authority in non-dev mode; remove unvalidated path |
#### High
| ID | Finding | File |
|----|---------|------|
| API-H1 | Invalid API key does not short-circuit — request continues through normal auth pipeline | `InternalApiKeyMiddleware.cs:42-47` |
| API-H2 | Auth callback `redirectUri` not validated server-side | `AuthController.cs:31-124` |
| API-H3 | `UpdateProposalRequest` exposes `Status` field — over-posting risk | `ProposalDtos.cs:22`, `ProposalService.cs:178-180` |
| API-H4 | No validator for `UpdateProposalRequest` — unlimited string lengths | `Validators/` (missing file) |
| API-H5 | `InvalidOperationException` messages leaked to clients | `GlobalExceptionHandler.cs:60-67` |
| API-H6 | No structured logging in any service or controller | All services |
| API-H7 | No Swagger/OpenAPI configuration at all | `Program.cs`, `.csproj` |
| API-H8 | DevMode controlled solely by config — no `IsDevelopment()` guard | `Program.cs:20` |
#### Medium
| ID | Finding |
|----|---------|
| API-M1 | Internal API key always grants `admins` role, never `sysadmins` |
| API-M2 | Silent auth failure when neither Cognito nor DevMode configured |
| API-M3 | Dispatchers can read any proposal's line items (no ownership check) |
| API-M4 | Dispatchers can access PDF endpoints for any proposal |
| API-M5 | Missing validators for VendorProposal, GeneratedPdf, SimilarReference DTOs |
| API-M6 | No file size validation on presigned upload URLs |
| API-M7 | No `.AsNoTracking()` on read-only queries |
| API-M8 | BulkUpdate uses delete-all/insert-all without explicit transaction |
| API-M9 | Dev PDF generation leaks stderr to client |
| API-M10 | Auth callback reveals config state in error responses |
| API-M11 | Silent exception swallowing on audit logging (`catch { }`) |
| API-M12 | Audit trail does not capture before/after values |
| API-M13 | User role change audit does not log previous role |
| API-M14 | Dev signing key hardcoded in committed config |
---
### 2. Web Frontend (1 Critical, 6 High, 13 Medium, 8 Low)
#### Critical
| ID | Finding | File |
|----|---------|------|
| WEB-C1 | JWT token stored in localStorage — XSS token theft risk | `authSlice.ts:51,58`, `client.ts:14` |
#### High
| ID | Finding | File |
|----|---------|------|
| WEB-H1 | No token refresh mechanism — expired tokens cause abrupt redirect | `authSlice.ts:21-30` |
| WEB-H2 | ProtectedRoute doesn't check loading state — flash-redirect on hydration | `ProtectedRoute.tsx:4-12` |
| WEB-H3 | Dispatcher can view any proposal via direct URL | `App.tsx:40` |
| WEB-H4 | "View Access Roles" button does nothing | `App.tsx:57-59` |
| WEB-H5 | saveMutation has no onError; partial failure leaves inconsistent state | `AdminWorkspace.tsx:114-138` |
| WEB-H6 | approveMutation chains save→approve with no error recovery | `AdminWorkspace.tsx:140-154` |
| WEB-H7 | No React error boundary — blank white screen on crash | `main.tsx` |
#### Medium
| ID | Finding |
|----|---------|
| WEB-M1 | Dev login shown when client ID absent — verify API gate |
| WEB-M2 | 401 interceptor clears token but not Redux state |
| WEB-M3 | Proposal form accepts 1-char scope (no minimum) |
| WEB-M4 | ServiceCategory `Other` not in shared contract |
| WEB-M5 | `CreateProposalRequest` type diverges from shared contract |
| WEB-M6 | No file size/type validation on vendor PDF upload |
| WEB-M7 | AdminWorkspace shows no error state for failed fetch |
| WEB-M8 | Dashboard stats show zeros on fetch error |
| WEB-M9 | Missing loading state for line items |
| WEB-M10 | Proposal state transitions not guarded on client |
| WEB-M11 | `returnToReview` API method wired but never called from UI |
| WEB-M12 | Table rows not keyboard accessible |
| WEB-M13 | ToastContainer rendered outside RouterProvider |
---
### 3. Mobile (0 Critical, 4 High, 12 Medium, 11 Low)
#### High
| ID | Finding | File |
|----|---------|------|
| MOB-H1 | Offline queue race condition — no mutex, duplicate proposals | `useOfflineDraft.ts:63-94` |
| MOB-H2 | Conditional screen registration — push/deep links may crash | `RootNavigator.tsx:33-78` |
| MOB-H3 | Offline queue sync errors silently swallowed | `App.tsx:80` |
| MOB-H4 | Bulk line item update has no optimistic concurrency | `LineItemEditScreen.tsx:55-101` |
#### Medium
| ID | Finding |
|----|---------|
| MOB-M1-M5 | Token refresh gaps, queue processing blocks on first failure, no queue UI, processes on every network event |
| MOB-M6-M8 | Shared contract mismatches (poNumber, id field) |
| MOB-M9-M12 | Navigation UX, loading states, unhandled promise rejections, atob encoding |
---
### 4. Lambda Pipeline (1 Critical, 5 High, 14 Medium, 8 Low)
#### Critical
| ID | Finding | File |
|----|---------|------|
| LAM-C1 | Function URL `authType: NONE` — publicly accessible | `compute-stack.ts:241-243` |
#### High
| ID | Finding | File |
|----|---------|------|
| LAM-H1 | pdf-generate: `register_pdf` failure doesn't raise — PDF in S3 but not in DB | `pdf-generate/app.py:96-98` |
| LAM-H2 | pdf-extract: exception swallowed, SQS considers it success, no retry | `pdf-extract/app.py:79-83` |
| LAM-H3 | pdf-extract: missing `s3Key` silently skips without batch failure | `pdf-extract/app.py:54-55` |
| LAM-H4 | suggestions: duplicate SQS delivery overwrites admin-edited line items | `suggestions/app.py:232-289` |
| LAM-H5 | `_retry_request` can return undefined `resp` | All 4 Lambda files |
#### Medium
| ID | Finding |
|----|---------|
| LAM-M1-M4 | Event validation, prompt injection risk, PDF size limits, Bedrock timeout |
| LAM-M5-M8 | Missing stack traces, numeric validation, tight Lambda timeout, S3 key sanitization |
| LAM-M9-M14 | Stale API key cache, empty env var defaults, KB sync flooding, CDK bundling gaps |
---
### 5. Infrastructure & CI/CD (0 Critical, 5 High, 9 Medium, 10 Low)
#### High
| ID | Finding | File |
|----|---------|------|
| INF-H1 | Function URL `authType: NONE` | `compute-stack.ts:241-243` |
| INF-H2 | SQS queues have no encryption at rest | `foundation-stack.ts:162-174` |
| INF-H3 | OpenSearch Serverless allows public network access | `compute-stack.ts:61-70` |
| INF-H4 | No MFA configured on Cognito user pool | `foundation-stack.ts:177-194` |
| INF-H5 | No access logging on HTTP API Gateway | `compute-stack.ts:246-263` |
#### Medium
| ID | Finding |
|----|---------|
| INF-M1-M2 | Bedrock wildcard model ARN, AOSS `aoss:*` permissions |
| INF-M3-M4 | No Cognito advanced security, Google OAuth not in CDK |
| INF-M5-M7 | No S3 enforceSSL, no custom domain on CF, no WAF |
| INF-M8-M9 | Workflows pinned to @main, --require-approval never locally |
---
### 6. QA & Testing (6 Critical, 16 High)
**ZERO application-level test coverage across the entire monorepo.**
#### Critical Gaps
| ID | What's Untested | Risk |
|----|-----------------|------|
| QA-C1 | No test project in .NET solution | CI `dotnet test` is a no-op |
| QA-C2 | Proposal state machine | Invalid transitions undetectable |
| QA-C3 | Authorization enforcement | Privilege escalation undetectable |
| QA-C4 | InternalApiKeyMiddleware | Auth bypass regression risk |
| QA-C5 | ProtectedRoute and RoleGuard | Client-side auth unverified |
| QA-C6 | Mobile offline draft and queue | Data loss risk |
#### High Gaps
Validators, ProposalNumberGenerator, AuthController, LineItemService state guards, all frontend components, API client interceptors, all Lambda handlers, SQS event parsing, PDF parsers, suggestions logic, CI pipeline executes zero tests.
---
## Prioritized Remediation Plan
### Phase 1 — Critical Security Fixes
1. Scope internal API key middleware to internal-only routes
2. Guard JWT validation — reject when Authority not configured in non-dev
3. Add DevMode environment guard (`IsDevelopment()` required)
4. Make invalid API key reject request immediately (401)
5. Add React error boundary to web app
6. Fix ProtectedRoute loading state
### Phase 2 — High Security & Reliability Fixes
7. Remove `Status` from `UpdateProposalRequest`
8. Add `UpdateProposalValidator`
9. Sanitize error messages in GlobalExceptionHandler
10. Fix Lambda error propagation (pdf-extract, pdf-generate)
11. Add suggestions idempotency check
12. Fix `_retry_request` undefined variable
13. Fix AdminWorkspace mutation error handling
14. Fix dead "View Access Roles" button
15. Add mobile offline queue mutex and error handling
### Phase 3 — Swagger/OpenAPI
16. Add Swashbuckle and configure
17. Add JWT security definition
18. Add `[ProducesResponseType]` to all controllers
19. Gate Swagger UI to dev/staging
### Phase 4 — Infrastructure Hardening (CDK)
20. SQS encryption, API Gateway logging, Cognito MFA, S3 enforceSSL
### Phase 5 — Test Infrastructure (future)
25. .NET test project, vitest for web, pytest for Lambdas, CI integration
---
## Positive Findings
- RDS: private subnets, not publicly accessible, encrypted, deletion protection, 7-day backups
- Cognito: self-signup disabled (admin-created accounts only)
- CORS: properly scoped to production origin
- Secrets: production connection string uses Secrets Manager
- S3: all buckets have `BlockPublicAccess.BLOCK_ALL`
- CloudFront: OAC, HTTPS redirect, security headers, TLS 1.2 minimum
- GitHub Actions: OIDC (no long-lived credentials), minimal permissions
- Monitoring: alarms for DLQ depth, RDS metrics, Lambda errors, API 5xx
- Mobile: tokens in iOS Keychain, no secrets in Fastlane config
- SQS: visibility timeout properly sized for Lambda consumers
Reference in a new issue View git blame Copy permalink