Skip to content

fix(infra): parseInt safety guards, remove duplicate code, JSON parse error handling, security hardening#5

Merged
bsatrom merged 4 commits intomainfrom
fix/infra-defensive-coding
Apr 17, 2026
Merged

fix(infra): parseInt safety guards, remove duplicate code, JSON parse error handling, security hardening#5
bsatrom merged 4 commits intomainfrom
fix/infra-defensive-coding

Conversation

@bsatrom
Copy link
Copy Markdown
Member

@bsatrom bsatrom commented Apr 15, 2026

Summary

Security hardening (previous commits):

  • Replace manual string interpolation in rag-retrieval.ts and feedback.ts with RDS Data API parameterized queries — eliminates SQL injection vector
  • Add cognito:groups Admin check to GET /analytics/feedback — closes authorization gap on endpoint that returns all users' query history
  • Remove Aurora fallback in chat-query.ts that silently returned ALL device serial numbers when none provided — now returns 403

Defensive coding (new commits):

  • Add parseIntParam helper to shared/utils.ts with NaN guard and max-value cap — replace unguarded parseInt() calls across 5 handler files (api-devices, api-alerts, api-telemetry, api-activity, api-journeys); bad input now returns default instead of propagating NaN into DynamoDB
  • Remove duplicate getAllDeviceUidsForSerial from api-alerts and import the shared version from device-lookup.ts
  • Wrap JSON.parse(event.body) in try/catch in mutation handlers in api-devices (mergeDevices, updateDeviceBySerial) and api-commands (sendCommand) — malformed JSON now returns 400 instead of 500
  • Gate span lifecycle console.log calls in tracing.ts behind DEBUG_TRACING=true env var — reduces CloudWatch log volume in production
  • Remove aws-marketplace:Subscribe / ViewSubscriptions from Lambda IAM role — Lambda should never subscribe to paid Marketplace products
  • Replace Math.random() alert ID generation with crypto.randomUUID() — eliminates collision window at high throughput
  • Add ACKNOWLEDGED shared constant for DynamoDB GSI partition key string values — prevents accidental boolean writes that break queries

Test plan

  • GET /devices?limit=abc returns results with the default limit, not a 500
  • GET /alerts?limit=999999 returns results capped at 500
  • POST /devices/merge with invalid JSON body returns 400 with descriptive error
  • POST /devices/{sn}/commands with invalid JSON body returns 400
  • Alert queries return correct results (device lookup still works via shared function)
  • Tracing spans still appear in Phoenix UI
  • RAG retrieval still returns relevant documents for chat queries
  • GET /analytics/feedback returns 403 for non-admin users
  • Chat query without deviceSerialNumbers returns 403

🤖 Generated with Claude Code

bsatrom and others added 2 commits April 14, 2026 17:15
…eedback endpoint, restrict device auth

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…for alert IDs, add ACKNOWLEDGED constant

- Remove aws-marketplace:Subscribe and aws-marketplace:ViewSubscriptions from chat query Lambda IAM role; a Lambda should never be able to subscribe to paid Marketplace products
- Scope bedrock:InvokeModel to specific Claude 3.5 Sonnet ARN prefixes instead of wildcard *
- Replace Math.random() alert ID generation with crypto.randomUUID() in api-ingest — eliminates 5-6 char collision window at high throughput, uses built-in Node 20 crypto module
- Add ACKNOWLEDGED constant (TRUE/FALSE string literals) to shared/constants.ts — prevents accidental boolean writes that break the DynamoDB GSI partition key
- Import and use ACKNOWLEDGED constant in api-alerts and api-ingest at all write and query sites
- Add marshallOptions: { removeUndefinedValues: true } to DynamoDB document clients in api-alerts and api-devices — matches the pattern already used in api-ingest, prevents latent runtime errors on optional fields

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@bsatrom bsatrom changed the title fix(infra): SQL injection, missing admin check on feedback, unrestricted device query fix(infra): parseInt safety guards, remove duplicate code, JSON parse error handling, security hardening Apr 15, 2026
bsatrom and others added 2 commits April 16, 2026 18:38
Merge origin/main to pick up the test suite from #18, then update the
3 GET /analytics/feedback tests to include cognito:groups=Admin in the
authorizer JWT claims, matching the admin-group check this PR adds to
the handler. Also adds a new test covering the 403 denial path.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@bsatrom bsatrom merged commit 2aac294 into main Apr 17, 2026
2 checks passed
@bsatrom bsatrom deleted the fix/infra-defensive-coding branch April 17, 2026 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant