security: fix SSRF NameError, timing-safe OAuth, atomic credential perms#198
Closed
eesb99 wants to merge 4 commits intoCraftOS-dev:mainfrom
Closed
security: fix SSRF NameError, timing-safe OAuth, atomic credential perms#198eesb99 wants to merge 4 commits intoCraftOS-dev:mainfrom
eesb99 wants to merge 4 commits intoCraftOS-dev:mainfrom
Conversation
…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>
…ial perms - Fix socket.gaierror -> _socket.gaierror reference bug that silently disabled SSRF protection when DNS resolution failed - Use hmac.compare_digest() for OAuth state comparison to prevent timing side-channel attacks - Use os.open() with 0o600 mode for atomic credential file permissions instead of write-then-chmod race Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rcement SSRF (http_request.py): - Extract _is_url_ssrf_safe() helper for reuse across redirect hops - Disable allow_redirects in requests lib, manually follow redirects with SSRF validation on each hop (closes redirect bypass) - Remove fail-open except Exception: pass -- DNS failures now block the request instead of silently allowing it - Unresolvable hostnames return an error (fail closed) OAuth CSRF (oauth_server.py): - Explicit 3-way state handling: no state expected (warn + allow), state expected but missing from callback (reject), mismatch (reject) - Previously silently skipped validation when state was absent Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- 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>
Collaborator
|
Some changes are repeated and are implemented in V1.2.3. Can you make another PR pointing at the V1.2.3 version branch? |
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Commit 1: Fix bugs in existing security code
socket.gaierror->_socket.gaierrorreference bug that silently disabled SSRF protection when DNS resolution failedhmac.compare_digest()for OAuth state comparison to prevent timing side-channel attacksos.open()with0o600mode for atomic credential file permissions instead of write-then-chmod raceCommit 2: Close architectural bypass vectors
allow_redirectsin requests lib, manually follow redirects with SSRF validation on each hopexcept Exception: pass-- DNS failures and unresolvable hostnames now block the request (fail closed)Remaining architectural items (separate PR)
http_request.pyread_file.py,write_file.py/etc/passwd,/proc/self/,.env. Consider allowlist approach.prompt_sanitizer.pyre.subcan concatenate text into new payloads. Detect-and-reject is safer.prompt_sanitizer.pyFollow-up from #195
Test plan
py_compilesyntax check🤖 Generated with Claude Code