fix: Add redirect support and security safeguards to curl command #1
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds HTTP redirect support to the HTTP.sol library's curl command with security safeguards to prevent protocol downgrade attacks and infinite redirect loops. It also includes a foundry.lock file to track dependency versions.
Changes:
- Modified curl command to follow redirects with security constraints (redirect limit and protocol restrictions)
- Added foundry.lock file with dependency revisions for forge-std and solidity-stringutils
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/HTTP.sol | Updated curl command to add -L flag for following redirects, with --max-redirs 3 limit and --proto-redir =https protocol restriction |
| foundry.lock | Added Foundry lockfile to track library dependency versions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| function request(Request storage req) internal returns (Response memory res) { | ||
| string memory scriptStart = 'response=$(curl -s -w "\\n%{http_code}" '; | ||
| string memory scriptStart = 'response=$(curl -sL --max-redirs 3 --proto-redir =https -w "\\n%{http_code}" '; |
There was a problem hiding this comment.
The --proto-redir flag has incorrect syntax. There should be no space between the equals sign and the protocol value. The correct syntax is --proto-redir=https (without the space). With the current syntax, curl will likely fail or ignore this security safeguard, potentially allowing protocol downgrades from HTTPS to HTTP.
|
|
||
| function request(Request storage req) internal returns (Response memory res) { | ||
| string memory scriptStart = 'response=$(curl -s -w "\\n%{http_code}" '; | ||
| string memory scriptStart = 'response=$(curl -sL --max-redirs 3 --proto-redir =https -w "\\n%{http_code}" '; |
There was a problem hiding this comment.
The new redirect functionality lacks test coverage. Since the test suite in test/HTTP.t.sol contains comprehensive tests for other HTTP features, consider adding a test case that verifies redirect behavior works correctly. This could test that a URL returning a 3xx redirect status is properly followed, and that the final response is returned rather than the redirect response.
|
Would close this in favour of #2 |
|
hey @Khrafts thank you for your contribution in any case I'll be merging it and publishing a new release |
2 participants
Summary
-Lflag to follow HTTP redirects--max-redirs 3to prevent infinite redirect loops--proto-redir =httpsto prevent protocol downgrade attacksProblem
The curl command in
HTTP.soldoes not follow redirects. When APIs return 3xx redirect responses (e.g., 308 Permanent Redirect), the library returns the redirect HTML page instead of following it to the actual endpoint.This causes issues with APIs like the Safe Transaction Service, which may return redirects for URL normalization (e.g., trailing slash handling).
Security Considerations
While adding
-Lis necessary for functionality, unrestricted redirects can pose security risks:This PR mitigates these risks by:
--max-redirs 3)--proto-redir =https)