Skip to content

fix(security): comprehensive security hardening, IDOR fixes, and query optimization#33

Merged
0xmanhnv merged 4 commits into
mainfrom
develop
Apr 7, 2026
Merged

fix(security): comprehensive security hardening, IDOR fixes, and query optimization#33
0xmanhnv merged 4 commits into
mainfrom
develop

Conversation

@0xmanhnv
Copy link
Copy Markdown
Collaborator

@0xmanhnv 0xmanhnv commented Apr 7, 2026

Summary

  • Fix 7 critical security vulnerabilities (IDOR, OAuth redirect, SSRF, command injection, header
    injection, token race condition)
  • Add tenant-scoped GetByTenantAndID to 15 repositories
  • Optimize BulkUpdateAssetStatus and BulkUpdateFindingsStatus from N+1 to 2 queries
  • Replace in-memory stats with SQL aggregation
  • Add 70+ security tests
  • Add performance indexes (migration 000099)
  • Expose asset properties in API response

Test plan

  • go build ./cmd/server/ passes
  • go test ./tests/unit/... passes (12.5s)
  • go vet ./... clean
  • Security tests: SSRF, ExtraArgs, OAuth redirect, header validation all pass

…ization

Security fixes:
- Fix IDOR: add GetByTenantAndID to 15 tenant-scoped repositories
- Fix OAuth open redirect: validate redirect_uri against allowed origins
- Fix command injection: validate ExtraArgs in scan executors (vulnscan + recon)
- Fix SSRF: validate webhook/integration/template source URLs
- Fix X-Forwarded header injection: validate Proto and Host values
- Fix password reset token race condition: clear token before password update
- Fix encryption key crash: handle missing APP_ENCRYPTION_KEY gracefully

Performance:
- Fix GetStats: use SQL aggregation instead of loading 1000 assets in memory
- Fix HasFindings filter: use EXISTS subquery (finding_count is computed, not a column)
- Fix BulkUpdateAssetStatus: use atomic single-query UPDATE instead of N+1 loop
- Add compound index (tenant_id, asset_type, criticality, status)
- Add FindingSeverityCounts to asset API response

Testing:
- Add 70+ security tests (SSRF, ExtraArgs, OAuth redirect, header validation)
- Update 13 mock repos for new GetByTenantAndID interface
- Fix auth test for 2-step password reset flow

Database:
- Fix migration numbering conflict (000096 duplicate)
- Add migration 000099 for performance indexes
Replace per-finding loop (N SELECT + N UPDATE queries) with:
- Batch fetch via GetByIDs (1 SELECT with ANY clause)
- In-memory workflow transition validation
- Batch update via UpdateStatusBatch (1 UPDATE)

Total: 2 queries instead of 2N queries.

Also:
- Add FindingRepository.GetByIDs interface + postgres implementation
- Add GetByIDs stubs to 6 test mock files
- Fix staticcheck: remove unused stubContext, fix nil context
Add properties field to AssetResponse so CTIS properties (OS, version,
enabled status, DNS hostname, etc.) collected by the asset-inventory
agent are returned in the API response alongside metadata.
@0xmanhnv 0xmanhnv merged commit 9b3af8c into main Apr 7, 2026
21 of 22 checks passed
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