fix: require HTTPS for remote file reads - #83
Conversation
|
Warning Review limit reachedNext included review available in 5 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
The security hardening changes to enforce HTTPS-only for remote file reads are well-implemented. The PR correctly restricts the is_url() function to only accept HTTPS URLs, adds early validation to reject non-HTTPS schemes before making network requests, updates documentation to reflect the new requirement, and includes comprehensive test coverage for the changes. The implementation follows a defense-in-depth approach by validating URLs at multiple layers. No blocking issues identified.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 520440986b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if "://" in path_str and not is_url(path_str): | ||
| raise ValueError("Remote file URLs must use HTTPS") |
There was a problem hiding this comment.
When an accepted HTTPS endpoint responds with a redirect whose Location uses http://, the default urllib.request.urlopen redirect 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 👍 / 👎.
| if "://" in path_str and not is_url(path_str): | ||
| raise ValueError("Remote file URLs must use HTTPS") |
There was a problem hiding this comment.
On Windows, callers can validly pass a string such as C://Users/me/config.json, which Path normally resolves as an absolute local path. The new substring check instead treats any :// as evidence of a remote URL; because is_url rejects the c scheme, read_file now raises ValueError before local-path resolution. Detect an actual URI scheme without misclassifying drive-letter paths.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
tox -e lint,typecheck,py311,py312,py313,py314,examples,docs,build,audit(818 passed per interpreter; 95% coverage)pnpm --dir docs validateactionlint .github/workflows/*.yml