Skip to content

security: fix 6 vulnerabilities (XSS, CSRF, SSRF, path traversal, credentials, prompt injection)#195

Merged
zfoong merged 13 commits intoCraftOS-dev:V1.2.3from
eesb99:security/fix-critical-vulnerabilities
Apr 15, 2026
Merged

security: fix 6 vulnerabilities (XSS, CSRF, SSRF, path traversal, credentials, prompt injection)#195
zfoong merged 13 commits intoCraftOS-dev:V1.2.3from
eesb99:security/fix-critical-vulnerabilities

Conversation

@eesb99
Copy link
Copy Markdown
Contributor

@eesb99 eesb99 commented Apr 15, 2026

Summary

Security review of the codebase identified 12 vulnerabilities. This PR fixes the 6 that can be cleanly patched without architectural changes:

  • Reflected XSS in OAuth callback — HTML-escape the error parameter in oauth_server.py to prevent script injection via crafted callback URLs
  • OAuth CSRF (state not validated) — Extract and validate the state parameter in the OAuth callback to prevent cross-site request forgery
  • SSRF in http_request action — Block requests to private IP ranges (10.x, 172.16.x, 192.168.x, 127.x) and cloud metadata endpoints (169.254.169.254)
  • Path traversal in read_file/write_file — Resolve symlinks, normalize paths, and block access to sensitive directories (.credentials, .ssh, .gnupg, .aws)
  • Plaintext credential storage permissions — Set file permissions to 0600 (owner read/write only) on credential files and 0700 on the .credentials directory
  • Prompt sanitizer not enforcing — The sanitizer detected injection patterns but only logged them; now strips matched patterns with [FILTERED]

Remaining issues (not addressed here — require design decisions)

These should be tracked as separate issues:

Severity Issue Why not fixed here
CRITICAL Hardcoded OAuth client IDs/secrets in embedded_credentials.py (base64 is not encryption) Removing would break distributed app functionality; needs design decision on device code flow or dynamic registration
CRITICAL run_python.py uses exec() without sandboxing in same process Requires architectural change to subprocess/container isolation
CRITICAL run_shell.py allows unrestricted shell=True execution By-design for agent, but needs user confirmation flow
MEDIUM Auto-install of arbitrary pip packages on ModuleNotFoundError Needs allowlist design decision
MEDIUM Docker socket exposure for GUI container Deployment/architecture concern

Files changed

  • agent_core/core/credentials/oauth_server.py — XSS fix + CSRF state validation
  • app/data/action/http_request.py — SSRF protection
  • app/data/action/read_file.py — Path traversal protection
  • app/data/action/write_file.py — Path traversal protection
  • app/external_comms/credentials.py — File permission hardening
  • app/security/prompt_sanitizer.py — Enforce pattern stripping

Test plan

  • Verify OAuth login flow still works (Google, Slack, Notion)
  • Verify read_file can still read workspace files but blocks .ssh/ paths
  • Verify write_file can still create files in workspace but blocks .credentials/ paths
  • Verify http_request can reach external APIs but blocks 169.254.169.254
  • Verify credential files are created with 0600 permissions on Linux/macOS
  • Verify prompt sanitizer strips injection patterns from user messages

🤖 Generated with Claude Code

zfoong and others added 10 commits March 16, 2026 08:37
Fixed the service. Uninstall issue.
…dential storage, prompt injection)

- Fix reflected XSS in OAuth callback by HTML-escaping error parameter
- Add OAuth state parameter validation to prevent CSRF attacks
- Add SSRF protection to http_request action (block private IPs, cloud metadata)
- Add path traversal protection to read_file/write_file actions (block sensitive dirs)
- Set restrictive file permissions (0600) on stored credentials
- Make prompt sanitizer actually strip detected injection patterns instead of just logging

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@zfoong zfoong changed the base branch from main to V1.2.3 April 15, 2026 05:31
Copy link
Copy Markdown
Collaborator

@zfoong zfoong left a comment

Choose a reason for hiding this comment

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

SSRF check implementation issue

  • The SSRF validation appears to use the wrong module reference in the exception handling. It catches “socket.gaierror”, but the socket module is imported as “_socket”. This could raise a NameError.

Sanitizer logic may break legitimate usage

  • The change from logging to stripping content might produce incorrect outputs. Since pattern matching with words might be too broad.

Path traversal protection design

  • Same as above.

zfoong added 3 commits April 15, 2026 18:11
Undo changes for write_file action
Undo changes on read_file
undo changes on prompt_sanitizer
@zfoong zfoong merged commit a60e068 into CraftOS-dev:V1.2.3 Apr 15, 2026
eesb99 added a commit to eesb99/CraftBot that referenced this pull request Apr 15, 2026
- context/claude.md: Session 2 summary with unified review findings,
  implementation details, code design assessment, and advisor recommendations
- Updated PR references (CraftOS-dev#195 merged, CraftOS-dev#198 open)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.

4 participants