Skip to content

fix(security): close remaining security audit items 3 & 4 (CWE-918, CWE-209)#552

Open
LuisMIguelFurlanettoSousa wants to merge 8 commits intoCrosstalk-Solutions:mainfrom
LuisMIguelFurlanettoSousa:fix/security-hardening-211
Open

fix(security): close remaining security audit items 3 & 4 (CWE-918, CWE-209)#552
LuisMIguelFurlanettoSousa wants to merge 8 commits intoCrosstalk-Solutions:mainfrom
LuisMIguelFurlanettoSousa:fix/security-hardening-211

Conversation

@LuisMIguelFurlanettoSousa
Copy link
Copy Markdown
Contributor

Summary

Closes remaining medium-priority items from the pre-launch security audit (issue #211):

  • Item 3 — Content Update URL Injection (CWE-918): Added assertNotPrivateUrl() SSRF validation to map_service.ts download flows. Previously, URLs from the cached manifest could bypass validation. Now loopback/link-local addresses are blocked at the download boundary, while RFC1918 (LAN) addresses remain allowed for LAN appliance use.

  • Item 4 — Verbose Error Messages (CWE-209): Sanitized 21+ locations across 8 files where raw error.message leaked to HTTP responses or WebSocket broadcasts. Internal details (paths, stack traces, Docker errors, DB connection strings) are now logged server-side only via structured logger.error(). Clients receive generic, descriptive error messages.

Files Changed

File Change
map_service.ts SSRF validation in downloadCollection() + preflightRemoteDownload()
rag_controller.ts Remove details: error.message from response
benchmark_controller.ts Sanitize 2 error responses
system_controller.ts Sanitize 1 error response
chats_controller.ts Sanitize 6 error responses
docker_service.ts Sanitize 6 error responses + broadcasts
system_update_service.ts Sanitize 1 error response
collection_update_service.ts Sanitize 2 error responses

Related Issues

Addresses items 3 and 4 from #211

Test Plan

  • Verify map download rejects loopback/link-local URLs from manifest
  • Verify all error API responses no longer contain internal paths or stack traces
  • Verify server logs still contain full error details for debugging
  • Smoke test: trigger errors (e.g., stop MySQL) and confirm responses are generic

@chriscrosstalk
Copy link
Copy Markdown
Collaborator

Nice work — this directly addresses items 3 and 4 from our security audit (#211). The SSRF validation approach matches what we did in PR #210 (keeping RFC1918 allowed for LAN use), and sanitizing error messages is a solid hardening step. Leaving for Jake to do the final review and merge.

@chriscrosstalk chriscrosstalk added enhancement New feature or request Roadmap Item Will be included in a future release labels Mar 31, 2026
@chriscrosstalk
Copy link
Copy Markdown
Collaborator

UPDATE: Roadmap item planned for a future version

Copy link
Copy Markdown
Collaborator

@chriscrosstalk chriscrosstalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for closing out the remaining audit items. The approach is exactly right:

  • Item 3 (SSRF): Using the existing assertNotPrivateUrl() validator is the clean way to extend coverage to the map download paths. Scope is correct — loopback/link-local blocked, RFC1918 preserved for LAN mirror use cases.
  • Item 4 (verbose errors): Structured logger.error({ err: error }, ...) with sanitized client-facing strings is the right pattern for pino. Coverage across controllers and services looks thorough.

Two things before this can merge:

  1. Base branch should be dev, not main. Please rebase against dev.
  2. Status is currently CONFLICTING — the rebase should resolve this.

I'm also going to swap the label from Roadmap Item to Next Release — security hardening shouldn't live in the roadmap bucket, and closing out the audit items is the kind of work that should land in the next cycle rather than drift.

Approving in principle pending the rebase. Once that's sorted, this is ready to ship. Thanks for sticking with the contribution pattern — you've submitted several PRs recently and the test/security discipline is appreciated.

@chriscrosstalk chriscrosstalk added Next Release This PR is staged for our next release. and removed Roadmap Item Will be included in a future release labels Apr 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request Next Release This PR is staged for our next release.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants