fix(network): drop credentials from HTTP hops after an HTTPS redirect - #4
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 55 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (13)
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 |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| defer { origin.stop() } | ||
| let previous = EngineTLS.serverTrustEvaluator | ||
| defer { EngineTLS.serverTrustEvaluator = previous } | ||
| EngineTLS.serverTrustEvaluator = { $0.host == "127.0.0.1" } |
There was a problem hiding this comment.
These new tests add more writes to the process-global EngineTLS.serverTrustEvaluator, while isolation from EngineTLSTests exists only in the CI command partition. A normal swift test can still run the two separately serialized suites concurrently, so one suite may temporarily observe the other suite's evaluator and produce flaky or invalid local results. Please enforce cross-suite serialization in the test code or make the ordinary documented test command use the same process partition. The same concern applies to the new evaluator assignments around lines 239, 259, and 278.
# Conflicts: # CHANGELOG.md
The relay stripped credential headers from static headers on an HTTPS-to-HTTP redirect, but sent whatever HTTPRequestAuthorization returned for the HTTP hop. A provider that authorized every URL therefore sent its bearer in cleartext. Once a redirect chain has reached HTTPS, HLSOriginRelay.fetch now removes RedirectHeaderPolicy's credential headers from every later HTTP hop, including provider output and 401 retries. The hop still runs, so anonymous redirects keep working, and an origin the host configured as HTTP still receives credentials. Tests: - Redirect-scope tests move to their own suite and assert the safe outcome. They record which URLs the provider was asked about, so a refusal test proves the provider refused. An HTTP-origin case covers configured HTTP deployments. - EngineTLS.resolve takes an evaluator parameter, so EngineTLSTests no longer writes the process-global evaluator. Live suites that do nest under one serialized LiveTrustEvaluatorTests parent, which removes the race when `swift test` runs everything in one process. - PythonOrigin.Launched.stop() replaces five copies of the launch/stop code. - The request log reads as empty when no request arrived. Docs drop claims about a specific downstream host, describe the engine's downgrade filter, and give the actual reason for the CI test partition. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
Quick104
changed the title
2 participants
Problem
The CodeRabbit downgrade warning on PR #2 is real. On an HTTPS-to-HTTP redirect,
HLSOriginRelay.fetchstripped credential headers from the static headers, but sent whateverHTTPRequestAuthorizationreturned for the HTTP hop without filtering it. Any host whose provider checks only the hostname, or authorizes every URL, sent its bearer in cleartext. The first version of this PR documented that behavior and added a test that expected the leak.Solution
Once a redirect chain has reached HTTPS,
fetchdropsRedirectHeaderPolicy's credential headers (Authorization,Proxy-Authorization,Cookie, Emby/Jellyfin tokens) from every later HTTP hop. That covers static headers, provider output and the 401 retry. The HTTP hop still runs and keeps non-credential headers, so anonymous redirects keep working. A chain that starts on HTTP (a host-configured HTTP server) still gets the provider's headers. Provider scope checks are still required; this filter is a backstop.Tests
The redirect tests move out of the TLS handshake suite into
RedirectAuthorizationScopeTests:[:]for HTTPWith the engine change reverted, the first case fails on
requests.map(\.authorization) == [Self.bearer, nil].Global trust evaluator race
EngineTLS.resolvenow takes anevaluatorparameter (defaulting to the global), soEngineTLSTestsno longer writesEngineTLS.serverTrustEvaluator. The live suites that do write it,EngineTLSHandshakeTestsandRedirectAuthorizationScopeTests, nest under one.serializedparent,LiveTrustEvaluatorTests. Plainswift testno longer races them. The CI partition filter now names the parent, and CONTRIBUTING explains the real reason: the redirect tests have authorization deadlines, and the handshake tests share the global with them.Other review fixes
PythonOrigin.Launched.stop()replaces five copies of the launch,workDirand stop code. AwithEvaluatorhelper replaces the per-test save, set and restore of the evaluator.The branch merges current
main(one CHANGELOG conflict, both entries kept).Validation
Mac Studio, macOS arm64, Xcode 27.0, Swift 6.4, at
b7713f61:swift test --skip-build --skip "$AUTHORIZATION_TEST_SUITES": 3,387 Swift Testing tests passed. XCTest: 636 tests, 1 skip, 0 failures.swift test --skip-build --filter "$AUTHORIZATION_TEST_SUITES": 56 tests passed, including all 5 redirect-scope and 8 handshake tests.EngineTLSTests|LiveTrustEvaluatorTeststogether in one process, 5 runs: 18 tests passed each time.EngineTLSTests|HLSOriginRelayTests|Issue551|Issue119: 43 tests passed.Scripts/check-doc-links.pyandgit diff --checkpassed.Risk
A host that intentionally sends credentials to an HTTP redirect target after starting on HTTPS will now send that request without them. Same-scheme behavior is unchanged.
AI disclosure
The first commit used OpenAI
gpt-6-astravia the OpenAI Codex CLI (codex-tui0.155.1). The review fixes, engine change and PR update used Anthropicclaude-opus-5-5[1m]in Claude Code, run from T3 Code. No other AI tooling was used.🤖 Generated with Claude Code
Note
Strip credential headers from HTTP hops after an HTTPS redirect in
HLSOriginRelayRedirectHeaderPolicy.withoutCredentialsand adds an optional evaluator parameter toEngineTLS.resolveso callers can supply a trust evaluator directly instead of the process-global one.RedirectAuthorizationScopeTestswith a paired HTTPS/HTTP test-origin fixture, and documents the authorization scope rules in HTTPRequestAuthorization.swift and api.md.Macroscope summarized b7713f6.