Conversation
Changes Made
Runtime (crates/runtime/):
- Created http_client.rs with 4 HTTP builtins:
- http.get - GET request (url -- response-map)
- http.post - POST request (url body content-type -- response-map)
- http.put - PUT request (url body content-type -- response-map)
- http.delete - DELETE request (url -- response-map)
- Added ureq dependency for HTTP/HTTPS with TLS support
- Response maps contain: status (Int), body (String), ok (Bool), error (String optional)
Compiler (crates/compiler/):
- runtime.rs: Added runtime declarations and builtin mappings
- ast.rs: Registered HTTP builtins
- builtins.rs: Added type signatures and documentation
Tests & Examples:
- tests/integration/src/test-http.seq - Integration tests for HTTP operations
- examples/http-client.seq - Example demonstrating HTTP usage
Documentation:
- Updated BATTERIES_INCLUDED.md with HTTP client details
API Example
# GET request
"https://httpbin.org/get" http.get
dup "ok" map.get drop
if
"body" map.get drop io.write-line
else
"error" map.get drop "Error: " swap string.concat io.write-line
then
# POST JSON
"https://api.example.com/users" "{\"name\":\"Alice\"}" "application/json" http.post
Pull Request Review: HTTP Client Implementation (#255)Overall AssessmentStatus: ✅ APPROVED with minor suggestions This is a well-implemented feature that adds HTTP client functionality to Seq. The implementation is clean, secure, and follows the existing codebase patterns. Great work! 🎯 Strengths1. Excellent Code Quality
2. Strong Security Practices
3. Comprehensive Testing
4. Good API Design
5. Documentation
🔍 Issues FoundCritical Issues: None ✅High Priority Issues: None ✅Medium Priority Issues1. Dependency Version MismatchLocation: ureq = { version = "2.10", features = ["tls"] }Issue: The PR adds Recommendation: Update to match actual version: ureq = { version = "2.12", features = ["tls"] }2. Code Duplication in HTTP MethodsLocation: Issue: The four HTTP methods (
Recommendation: Consider extracting common logic: fn execute_request<F>(request_fn: F) -> Value
where
F: FnOnce(ureq::Agent) -> Result<ureq::Response, ureq::Error>
{
let agent = ureq::AgentBuilder::new()
.timeout(Duration::from_secs(DEFAULT_TIMEOUT_SECS))
.build();
handle_response(request_fn(agent))
}This is not critical but would improve maintainability. Low Priority Issues3. Missing Response Headers AccessLocation: API design Observation: The current API only returns status code and body, but many HTTP use cases need headers (e.g., Suggestion: Consider a future enhancement to include headers in the response map: Not blocking for this PR, but worth considering for future iterations. 4. No Custom Header SupportLocation: Observation: POST/PUT only allow setting Suggestion: Future enhancement could accept a headers map: Again, not blocking - current implementation is fine for v1. 📊 Test CoverageStrengths:
Suggestions:
🔒 Security Assessment✅ No security vulnerabilities found Good practices observed:
Note: SSRF (Server-Side Request Forgery) is possible since users can specify any URL, but this is expected behavior for an HTTP client. Applications using this should validate URLs if needed. ⚡ Performance ConsiderationsStrengths:
Observations:
Verdict: Performance is appropriate for the use case. No changes needed. 📝 Documentation QualityExcellent documentation throughout:
Small suggestion: Add a note in the documentation about thread safety if Seq supports concurrent HTTP requests. ✨ Recommendations SummaryMust Fix (blocking)None - this PR is ready to merge! Should Fix (non-blocking)
Nice to Have (future work)
🎉 ConclusionThis is a high-quality implementation that adds valuable functionality to Seq. The code is secure, well-tested, and follows best practices. The minor issues noted above are primarily about maintainability and future enhancements, not correctness. Recommendation: ✅ Approve and merge after addressing the version mismatch in Great work on this feature! 🚀 Review generated with ❤️ by Claude Code |
…s now compile and pass.
Fixes made:
1. examples/http-client.seq - Added dup before "error" map.has? to preserve the map
2. tests/integration/src/test-http.seq - Multiple fixes:
- Removed helper function with type annotation issue
- Changed >=, <=, = to i.>=, i.<=, i.= (Seq integer comparison operators)
- Fixed stack management throughout
Test results:
- 8/8 HTTP integration tests pass
- All workspace tests pass
- Example compiles successfully
Pull Request Review: HTTP Client ImplementationSummaryThis PR adds HTTP client functionality to patch-seq with GET, POST, PUT, and DELETE operations using the Code Quality: ✅ GoodStrengths
Code Quality Observations
Potential Issues:
|
1. Code Duplication Fixed (http_client.rs)
- Extracted build_agent() - creates configured HTTP agent with timeout
- Extracted handle_response() - handles Result<Response, Error> conversion to Value
- Reduced ~170 lines of duplicated code to ~75 lines
- Each perform function is now 1-6 lines instead of ~40 lines
2. Version Mismatch Fixed (Cargo.toml)
- Updated ureq from "2.10" to "2.12" to match Cargo.lock
Before:
fn perform_get(url: &str) -> Value {
let agent = ureq::AgentBuilder::new()... // 40+ lines of duplicated logic
}
fn perform_post(...) -> Value { /* same 40 lines */ }
fn perform_put(...) -> Value { /* same 40 lines */ }
fn perform_delete(...) -> Value { /* same 40 lines */ }
After:
fn perform_get(url: &str) -> Value {
handle_response(build_agent().get(url).call())
}
fn perform_post(url: &str, body: &str, content_type: &str) -> Value {
handle_response(build_agent().post(url).set("Content-Type", content_type).send_string(body))
}
// etc.
All tests pass.
Pull Request Review: HTTP Client ImplementationThank you for this comprehensive implementation! This is a well-structured PR that adds HTTP client capabilities to patch-seq. ✅ StrengthsCode Quality
Implementation Details
Testing
🔍 Issues & RecommendationsCRITICAL: Security - SSRF VulnerabilityLocation: Issue: No URL validation allows Server-Side Request Forgery (SSRF) attacks:
Recommendation: Add URL validation or document risks clearly HIGH: Missing Header SupportLocation: Issue: Cannot set custom headers (Authorization, User-Agent, X-API-Key) Recommendation: Add header parameter or builder pattern MEDIUM: Performance - Agent ReuseLocation: Issue: New Recommendation: Use static agent with MEDIUM: Error Information LossLocation: Issue: Generic error messages for HTTP errors (4xx, 5xx) Recommendation: Include body excerpt in error message for better debugging LOW: Test Coverage GapsMissing tests:
LOW: Magic NumbersLocation: Consider documenting why 30s timeout and 10MB limit were chosen. Consider making configurable. MINOR: DocumentationLocation: Add security considerations section covering:
📊 Code Metrics
🏁 VerdictRecommendation: Approve with Required Changes Solid implementation with clean code and good documentation. However, SSRF vulnerability is a blocker. Required Before Merge:
Recommended for Follow-up:
💡 Future Enhancements
Great work! Once SSRF is addressed, this will be a valuable addition to patch-seq. Files Reviewed: http_client.rs, ast.rs, builtins.rs, runtime.rs, test-http.seq, http-client.seq, BATTERIES_INCLUDED.md, Cargo files |
…eview: 1. SSRF Security Documentation (CRITICAL) - Added comprehensive security section to http_client.rs module docs - Added security considerations to BATTERIES_INCLUDED.md - Documents risks: internal service access, network scanning, cloud metadata - Documents mitigations: URL validation, allowlists, IP blocking, egress filtering 2. Connection Pooling (MEDIUM) - Added static HTTP_AGENT: LazyLock<ureq::Agent> for shared agent - All HTTP methods now use the pooled agent - Benefits: connection reuse, better performance for multiple requests Changes summary: - http_client.rs: Added security docs, LazyLock agent, removed per-request agent creation - BATTERIES_INCLUDED.md: Added security considerations section - Cargo.toml: Updated ureq version to 2.12 The review also mentioned these as future enhancements (not blocking): - Custom header support (needed for auth) - Improved error messages with body excerpts - Additional tests (large bodies, timeouts, redirects)
PR Review: HTTP Client Implementation (#255)SummaryThis PR implements a comprehensive HTTP client for the Seq language, adding GET, POST, PUT, and DELETE operations with TLS support via the ✅ Strengths1. Code Quality & Architecture
2. Security Awareness
3. Testing
4. Documentation
🔍 Issues & Concerns1. Security: SSRF Not Mitigated (HIGH PRIORITY)Location: While the documentation warns about SSRF, the implementation does nothing to prevent it. This is a significant security risk if Seq is used in any server-side context. Issue: User-controlled URLs can access:
Recommendation: Add URL validation function that blocks internal addresses. Consider making this opt-out via feature flag for development. |
2. Error Handling: Body Size Check After Reading (MEDIUM)Location: The body is fully read into memory before checking size. A 100 MB response will consume memory before being rejected. Fix: Use ureq built-in limits: static HTTP_AGENT: LazyLock<ureq::Agent> = LazyLock::new(|| {
ureq::AgentBuilder::new()
.timeout(Duration::from_secs(DEFAULT_TIMEOUT_SECS))
.max_response_body_size(MAX_BODY_SIZE)
.build()
});3. Missing: Custom Headers (MEDIUM)No way to pass custom headers (especially 4. Testing: Missing Edge CasesThe test coverage is good but could include:
🎯 Performance ConsiderationsPositive:
Concerns:
|
🧪 Test Coverage AssessmentCoverage: Good (8 integration tests + 3 unit tests) Strengths:
Gaps: timeout behavior, redirect handling, max body size enforcement, invalid URLs 📝 Documentation QualityExcellent. The documentation is thorough and includes API examples, response format, security warnings, and resource limits. Suggestion: Add a "Limitations" section mentioning:
🚀 RecommendationsMust Fix (Before Merge):
Should Fix (Before Merge):
Nice to Have (Future PR):
✅ Final AssessmentCode Quality: 8/10 - Clean, well-refactored, good Rust practices Overall: Solid implementation with good iterative improvement. Main concerns are SSRF and body size check. Recommendation: Request changes for issues #1 and #2, then approve. 🚦 👍 Great Work!The incremental improvements through multiple commits show excellent response to feedback. The refactoring from 170 lines of duplication to clean helper functions demonstrates good engineering discipline. Once the security issues are addressed, this will be a valuable feature! |
…implemented:
SSRF Protection Implementation:
1. URL Validation (validate_url_for_ssrf)
- Parses URL and validates scheme (only http/https allowed)
- Blocks localhost and *.localhost hostnames
- Resolves hostname to IP and checks against blocked ranges
2. Blocked IP Ranges:
- Loopback: 127.0.0.0/8
- Private: 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16
- Link-local: 169.254.0.0/16 (cloud metadata endpoints)
- IPv6: loopback, link-local (fe80::/10), unique local (fc00::/7)
- IPv4-mapped IPv6 addresses are also checked
3. Blocked Schemes:
- file://, ftp://, gopher://, etc.
4. Tests Added: 8 new unit tests covering:
- Localhost blocking
- Loopback IP blocking
- Private range blocking
- Link-local blocking
- Invalid scheme blocking
- Public URL allowance
- IPv4/IPv6 dangerous address detection
Other improvements in this session:
- Connection pooling via LazyLock<ureq::Agent>
- Code deduplication with handle_response()
- Updated ureq version to 2.12
- Added url crate for URL parsing
Pull Request Review: HTTP Client Implementation (#255)SummaryThis PR implements a comprehensive HTTP client for Seq with GET, POST, PUT, DELETE operations, built-in SSRF protection, and TLS support via ✅ Strengths1. Security - SSRF ProtectionExcellent work on SSRF mitigation. The implementation blocks:
The validation happens before requests are made, which is the correct approach. Code reference: 2. Resource Limits
3. Comprehensive Testing
4. API DesignThe response map structure is clean and idiomatic: 5. Documentation
🔍 Issues & Concerns1. CRITICAL: DNS Resolution Timing IssueLocation: match addr_str.to_socket_addrs() {
Ok(addrs) => {
for addr in addrs {
if is_dangerous_ip(addr.ip()) {
return Err(format!(...));
}
}
}
Err(_) => {
// DNS resolution failed - allow the request to proceed
// (ureq will handle the DNS error appropriately)
}
}Problem: This creates a Time-of-Check-Time-of-Use (TOCTOU) race condition. The DNS lookup happens during validation, but
Attack Scenario:
Recommendation: This is difficult to fix perfectly with
2. MODERATE: IPv6 IPv4-Mapped Address Edge CaseLocation: // IPv4-mapped IPv6 addresses: check the embedded IPv4
if let Some(ipv4) = ip.to_ipv4_mapped() {
return is_dangerous_ipv4(ipv4);
}Issue: This only handles the mapped format (
Recommendation: // Add to is_dangerous_ipv6
// IPv4-compatible (deprecated): ::w.x.y.z
if segments[0..6] == [0, 0, 0, 0, 0, 0] {
let ipv4 = Ipv4Addr::new(
(segments[6] >> 8) as u8, (segments[6] & 0xff) as u8,
(segments[7] >> 8) as u8, (segments[7] & 0xff) as u8
);
return is_dangerous_ipv4(ipv4);
}3. MINOR: Error Information LeakLocation: return Err(format!(
"Blocked: {} resolves to private/internal IP {}",
host, addr.ip()
));Issue: Revealing the resolved IP address in error messages could leak information about internal network structure to attackers. Recommendation: Simplify to: return Err(format!("Blocked: {} resolves to private/internal IP", host));4. MINOR: Missing URL Decoding in Host CheckLocation: URLs can contain percent-encoded characters. An attacker might try:
Status: The Recommendation: Add a test case: #[test]
fn test_ssrf_blocks_encoded_localhost() {
assert!(validate_url_for_ssrf("http://local%68ost/").is_err());
assert!(validate_url_for_ssrf("http://127.0.0.%31/").is_err());
}5. MINOR: Response Body Size Check TimingLocation: match response.into_string() {
Ok(body) => {
if body.len() > MAX_BODY_SIZE {
error_response(format!(...))
}
}
}Issue: The entire response body is read into memory before checking the size limit. For a malicious server sending 100MB, this will consume 100MB before rejecting it. Recommendation: Use let content_length = response.header("content-length")
.and_then(|v| v.parse::<usize>().ok());
if let Some(len) = content_length {
if len > MAX_BODY_SIZE {
return error_response(format!("Response too large..."));
}
}
// Then read with limit6. MINOR: Consistent Error Handling in TestsLocation: Issue: This test uses Recommendation: Use a public IP with closed port, or add a test exemption, or use a different approach. 🎯 Code QualityPositive
Minor Suggestions
📊 Performance Considerations✅ Good:
🔒 Security Rating
✅ Test Coverage
📝 Recommendations SummaryMust Fix (Before Merge)
Should Fix (Before Merge)
Consider for Follow-up PR
🎉 ConclusionThis is a high-quality implementation that adds valuable functionality to Seq. The code is well-structured, secure by default, and properly tested. The SSRF protection is notably thorough, which is rare to see in HTTP client implementations. The TOCTOU DNS issue is the main concern, but it's a known limitation of validating URLs before passing them to a third-party HTTP client. With proper documentation, this is acceptable for v1. Recommendation: ✅ Approve with minor changes (fix the test, add TOCTOU documentation) Great work on this implementation! The security-first approach is exactly what Seq needs in its standard library. |
1 participant
Changes Made
#247
Runtime (crates/runtime/):
Compiler (crates/compiler/):
Tests & Examples:
Documentation:
API Example
GET request
"https://httpbin.org/get" http.get
dup "ok" map.get drop
if
"body" map.get drop io.write-line else "error" map.get drop "Error: " swap string.concat io.write-line then
POST JSON
"https://api.example.com/users" "{"name":"Alice"}" "application/json" http.post