-
Notifications
You must be signed in to change notification settings - Fork 0
fix: require HTTPS for remote file reads #83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ | |
| from dataclasses import dataclass, field | ||
| from pathlib import Path | ||
| from typing import TYPE_CHECKING, Any, TypeAlias, cast | ||
| from urllib.parse import urlsplit | ||
|
|
||
| import validators | ||
|
|
||
|
|
@@ -411,22 +412,21 @@ def is_url(path: str) -> bool: | |
| """Check if a string is a valid and safe URL. | ||
|
|
||
| Uses the validators library for robust URL validation, | ||
| restricted to HTTP/HTTPS schemes only. | ||
| restricted to HTTPS URLs only so remote file reads are encrypted in transit. | ||
|
|
||
| Args: | ||
| path (str): The string to check. | ||
|
|
||
| Returns: | ||
| bool: True if the string is a valid HTTP/HTTPS URL. | ||
| bool: True if the string is a valid HTTPS URL. | ||
| """ | ||
| if not path: | ||
| return False | ||
| # validators.url returns True for valid URLs, ValidationError otherwise | ||
| result = validators.url(path) | ||
| if result is not True: | ||
| return False | ||
| # Additional check: only allow http/https schemes | ||
| return path.startswith(("http://", "https://")) | ||
| return urlsplit(path).scheme == "https" | ||
|
|
||
|
|
||
| def read_file( | ||
|
|
@@ -455,11 +455,14 @@ def read_file( | |
|
|
||
| Raises: | ||
| urllib.error.URLError: If the URL cannot be accessed. | ||
| ValueError: If the URL scheme is not allowed (only http/https permitted). | ||
| ValueError: If the URL scheme is not allowed (only HTTPS is permitted). | ||
| """ | ||
| path_str = str(file_path) | ||
|
|
||
| # Handle URLs (is_url already validates HTTP/HTTPS only) | ||
| if "://" in path_str and not is_url(path_str): | ||
| raise ValueError("Remote file URLs must use HTTPS") | ||
|
Comment on lines
+462
to
+463
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On Windows, callers can validly pass a string such as Useful? React with 👍 / 👎. |
||
|
|
||
| # Handle URLs (is_url already validates HTTPS only). | ||
| if is_url(path_str): | ||
| headers = headers or {} | ||
| request = urllib.request.Request(path_str, headers=dict(headers)) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an accepted HTTPS endpoint responds with a redirect whose
Locationuseshttp://, the defaulturllib.request.urlopenredirect handler creates and follows that request without re-running this guard; it also carries the supplied custom headers into the redirected request. Consequently, both the response and potentially sensitive headers can still travel over clear text despite the newly documented HTTPS-only boundary. Use an opener/redirect handler that rejects every non-HTTPS redirect target.AGENTS.md reference: AGENTS.md:L77-L77
Useful? React with 👍 / 👎.