Skip to content

🛡️ Sentinel: [HIGH] Fix path traversal and access to hidden files in static serving#105

Open
socialawy-dev wants to merge 1 commit into
mainfrom
sentinel-harden-static-files-12627956956533883851
Open

🛡️ Sentinel: [HIGH] Fix path traversal and access to hidden files in static serving#105
socialawy-dev wants to merge 1 commit into
mainfrom
sentinel-harden-static-files-12627956956533883851

Conversation

@socialawy-dev
Copy link
Copy Markdown
Collaborator

🚨 Severity: HIGH
💡 Vulnerability: SafeStaticFiles implemented a hardcoded blocklist (.env, .git) instead of blocking all hidden files. Additionally, validate_path_within was missing TypeError and AttributeError exception catching, potentially causing the app to crash or bypass checks if a malformed input like None was processed.
🎯 Impact: An attacker could potentially access sensitive hidden files (like .htpasswd or .DS_Store) that weren't in the specific blocklist. Path validation failures due to unexpected data types could lead to unexpected behavior or bypasses.
🔧 Fix: Used any(part.startswith('.') for part in p.parts) in SafeStaticFiles to broadly block any hidden paths. Caught additional exceptions (TypeError, AttributeError) in validate_path_within to ensure a consistent fail-closed behavior for invalid inputs. Added a None check to fail fast for empty paths.
✅ Verification: Ensure the updated test suite passes and verify manually that attempting to access a path starting with . throws a 403 error.


PR created automatically by Jules for task 12627956956533883851 started by @socialawy

Added check to fail safely if a `None` path is passed.
Hardened `SafeStaticFiles` to block access to *any* hidden file or directory.
Enhanced `validate_path_within` to catch `TypeError` and `AttributeError` for safer failing.

Co-authored-by: socialawy <24765060+socialawy@users.noreply.github.com>
@google-labs-jules
Copy link
Copy Markdown
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request enhances path validation and security within the static file serving middleware. It introduces checks for None paths, blocks access to all hidden files/directories comprehensively, and catches additional type-level exceptions (TypeError, AttributeError) to ensure secure fail-closed behavior. The review feedback suggests wrapping the path instantiation in a try...except ValueError block to handle potential embedded null bytes gracefully, preventing unhandled 500 errors.

Comment on lines 30 to 34
p = Path(str(path).lower())
if "00_config" in p.parts or p.name.startswith(".env") or ".git" in p.parts:
if "00_config" in p.parts or any(part.startswith(".") for part in p.parts):
raise HTTPException(
status_code=403, detail="Access denied to sensitive resource"
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

security-medium medium

If the path parameter contains null bytes (e.g., foo\x00bar), instantiating Path(str(path).lower()) will raise a ValueError: embedded null byte. Since this exception is not caught within get_response, it will result in an unhandled 500 Internal Server Error (or application crash) rather than a clean client-side error.

To ensure consistent fail-closed behavior and prevent potential Denial of Service (DoS) or crash vectors, wrap the path initialization and checks in a try...except ValueError block and raise a 400 Bad Request or 403 Forbidden exception.

Suggested change
p = Path(str(path).lower())
if "00_config" in p.parts or p.name.startswith(".env") or ".git" in p.parts:
if "00_config" in p.parts or any(part.startswith(".") for part in p.parts):
raise HTTPException(
status_code=403, detail="Access denied to sensitive resource"
)
try:
p = Path(str(path).lower())
if "00_config" in p.parts or any(part.startswith(".") for part in p.parts):
raise HTTPException(
status_code=403, detail="Access denied to sensitive resource"
)
except ValueError:
raise HTTPException(
status_code=400, detail="Invalid path format"
)

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