🔒 [security fix] Fix unchecked URL encoding fallback in HTTPClient

[segin]

🎯 What: Fixed a security vulnerability in HTTPClient::urlEncode that returned unencoded input on failure.
⚠️ Risk: Potential URL injection or broken URL structure if input containing characters like &, =, or ? was returned unencoded.
🛡️ Solution: Implemented a robust manual RFC 3986-compliant percent-encoding fallback that is used if libcurl is uninitialized or if curl_easy_escape fails. This ensures that the output of urlEncode is always safely encoded.

---

PR created automatically by Jules for task 15096492925585671045 started by @segin

[google-labs-jules]

👋 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.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[github-actions]

Claude finished @segin's task —— View job

---

Security Fix Code Review

I'll review this security fix for the HTTPClient URL encoding vulnerability. Here's my review plan:

Todo List:

• Read and analyze the main HTTPClient.cpp changes
• Review GitHub workflow file modifications
• Assess security implementation completeness
• Check for potential bugs or edge cases
• Evaluate performance considerations
• Review test coverage needs
• Provide comprehensive feedback

---

🔒 Security Analysis

Vulnerability Assessment: EXCELLENT FIX ✅

The original HTTPClient::urlEncode() had a critical security flaw where it would return unencoded input on any failure condition. This could lead to:

• URL injection attacks when unencoded &, =, ? characters break URL structure
• Parameter pollution when special chars are passed through unescaped
• Path traversal if ../ sequences aren't encoded

Security Implementation Review

✅ Strong Points:

1. RFC 3986 Compliance: The fallback encoder correctly implements unreserved characters (-._~ + alphanumeric)
2. Complete Coverage: All failure paths now use safe encoding instead of returning raw input
3. Proper Hex Encoding: Uses uppercase hex format (%XX) as per standard
4. Defense in Depth: Multiple fallback layers (libcurl → manual encoding)

⚠️ Minor Considerations:

1. Character Set Handling: The unsigned char cast at src/io/http/HTTPClient.cpp:469 is correct and prevents sign extension issues with high-bit characters
2. Buffer Sizing: result.reserve(input.length() * 3) is appropriate for worst-case scenario (all chars encoded)

---

🐛 Code Quality Review

Implementation: VERY GOOD ✅

Strengths:

• Clean separation of concerns (libcurl first, fallback second)
• Proper RAII with curl handle cleanup
• Informative debug logging when fallback is used
• Efficient string building with pre-allocation

Minor Improvement Opportunities:

1. Input Validation: Consider adding input size limits to prevent excessive memory usage

   if (input.length() > MAX_URL_COMPONENT_SIZE) {
       throw std::invalid_argument("URL component too large");
   }

2. Const Correctness: The implementation is already const-correct ✅

---

⚡ Performance Analysis

Performance: GOOD ✅

Efficient Aspects:

• Single-pass encoding algorithm
• Pre-allocated result buffer prevents reallocations
• Short-circuit on empty input
• Minimal function call overhead in fallback path

Impact Assessment:

• Space: 3x memory in worst case (all chars need encoding) - acceptable
• Time: O(n) complexity - optimal for character-by-character encoding
• Allocation: One string allocation with proper sizing - efficient

---

🧪 Test Coverage Assessment

Coverage: NEEDS IMPROVEMENT ⚠️

Missing Test Scenarios:

1. Security Edge Cases:

   • Mixed dangerous chars: "user=admin&role=root"
   • Unicode/high-bit characters: "café"
   • URL injection attempts: "normal?evil=true&admin=1"

2. Fallback Path Testing:

   • Force libcurl unavailable condition
   • Verify curl_easy_escape failure handling
   • Boundary conditions (empty string, very long strings)

3. Integration Testing:

   • End-to-end URL construction with encoded components
   • Verify server receives properly encoded data

Recommended Test Addition:

// Add to test suite
void testUrlEncoding() {
    // Security cases
    ASSERT_EQ(HTTPClient::urlEncode("user=admin&role=root"), "user%3Dadmin%26role%3Droot");
    ASSERT_EQ(HTTPClient::urlEncode("path/to/file"), "path%2Fto%2Ffile");
    
    // Edge cases
    ASSERT_EQ(HTTPClient::urlEncode(""), "");
    ASSERT_EQ(HTTPClient::urlEncode("safe123-._~"), "safe123-._~");
}

---

🛠️ Workflow Changes Review

CI/CD Updates: APPROPRIATE ✅

The workflow changes (updating deprecated GitHub Actions, fixing permissions) are:

• Security-positive: Updated actions reduce supply chain risks
• Maintenance-friendly: Removes deprecated warnings
• Properly scoped: Only necessary permission grants

---

📋 Summary & Recommendations

Overall Assessment: STRONG SECURITY FIX ⭐⭐⭐⭐⭐

This PR successfully eliminates a significant security vulnerability. The implementation is robust, follows security best practices, and maintains good performance characteristics.

Immediate Actions: ✅ APPROVE & MERGE
Follow-up Work: Add comprehensive test coverage for the security scenarios mentioned above

Security Impact: HIGH POSITIVE - Prevents URL injection and related attacks
Code Quality: HIGH - Clean, maintainable implementation
Performance: NEUTRAL - No significant performance impact

---

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.