[security] fix(providers): reject insecure credentialed endpoint overrides - #1236
[security] fix(providers): reject insecure credentialed endpoint overrides#1236Hinotoi-agent wants to merge 4 commits into
Conversation
|
Codex review: found issues before merge. Reviewed June 2, 2026, 3:27 AM ET / 07:27 UTC. Summary Reproducibility: yes. at source level: current main accepts explicit non-HTTPS override schemes in the MiniMax and Alibaba settings readers, and the PR body includes dummy-harness output showing after-fix rejection before credentialed work. I did not run local tests because this review must keep the checkout read-only and avoid live credential/keychain paths. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Land the scoped endpoint hardening after maintainer sign-off on the fail-closed HTTP policy, with the diagnostic keyword ordering narrowed before merge. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main accepts explicit non-HTTPS override schemes in the MiniMax and Alibaba settings readers, and the PR body includes dummy-harness output showing after-fix rejection before credentialed work. I did not run local tests because this review must keep the checkout read-only and avoid live credential/keychain paths. Is this the best way to solve the issue? Mostly yes: failing closed before credential discovery is the narrow security fix, and the PR preserves HTTPS and bare host:port compatibility. The remaining concerns are maintainer approval of the intentional plaintext override break and narrowing the diagnostic heuristic. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against dc4e4835bc6e. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f29828a6a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
8f29828 to
e520e71
Compare
|
Addressed the fail-closed feedback in the latest push ( What changed:
Local validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e520e71813
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
e520e71 to
da442a7
Compare
da442a7 to
1ea8f96
Compare
|
Pushed an update at What changed:
Local validation run on the current head:
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review Please re-review the latest PR head. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Pushed follow-up commits through What changed:
Local validation run on the current head:
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Pushed What changed:
Local validation on the pushed head:
No real provider credentials were used; the temporary smoke harness and binary were removed after the proof run. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
d7564fb to
55d63ed
Compare
|
Pushed What changed:
Validation on the pushed head:
Smoke proof, with credentials redacted and no real provider credentials used: Focused repository The PR body is updated with this current proof. The explicit non-HTTPS override policy remains fail-closed for credentialed provider paths; maintainers should still sign off on that compatibility/security tradeoff. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
55d63ed to
5c0749e
Compare
|
Pushed What changed in this push:
Validation on the pushed head:
The PR body is updated for the current head. No real provider credentials were used. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Thanks for the detailed security work and proof. Closing this branch as superseded by #1269, which now carries the canonical MiniMax and Alibaba endpoint-override hardening with the shared validator, broader authority checks, compatibility policy, documentation, tests, and a newer main integration. Maintaining both branches for the same provider boundary no longer makes sense; the work and discussion here remain useful provenance for #1269. |
Summary
This PR hardens the credentialed endpoint override boundary for the MiniMax and Alibaba Coding Plan providers.
Security issues covered
http://or another non-HTTPS endpoint during usage fetching.Before this PR
MINIMAX_*_URL,MINIMAX_HOST,ALIBABA_CODING_PLAN_QUOTA_URL, andALIBABA_CODING_PLAN_HOSTaccepted explicit URL schemes inconsistently across settings readers, URL builders, and fetch strategies.nilfor non-HTTPS values, but higher-level fetch paths could still continue into fallback behavior or credential discovery.After this PR
http://,ftp://, or any non-HTTPS scheme fail closed with provider-specificinvalidEndpointOverrideerrors.host:portvalues, remain supported and continue to be normalized to HTTPS where URL construction is needed.Compatibility decision: explicit
http://overrides fail closedThis PR intentionally treats explicit non-HTTPS endpoint override URLs as unsupported for MiniMax and Alibaba Coding Plan credentialed usage fetches. That is a compatibility break for local/debug setups that deliberately set values such as
MINIMAX_REMAINS_URL=http://localhost:8080/remainsorALIBABA_CODING_PLAN_QUOTA_URL=http://localhost:8080/data/api.json.The supported compatibility paths are:
https://...override URLs;host:portvalues such aslocalhost:8443/path, which are normalized to HTTPS by the application.The security decision is to fail closed for explicit
http://or other non-HTTPS schemes before credential discovery or request construction, rather than allowing provider cookies/API keys to be resolved for a plaintext endpoint.Why this matters
MiniMax and Alibaba Coding Plan usage fetches can attach sensitive provider session material to outbound requests. Endpoint override variables are useful for trusted local development and deployment customization, but a credentialed request should not proceed when an override explicitly selects plaintext HTTP or another non-HTTPS transport.
Failing closed at the settings and strategy boundary prevents accidental credential delivery to unsafe endpoints while keeping the intended trusted HTTPS override workflow intact.
Attack flow
Affected code
Sources/CodexBarCore/Providers/MiniMax/MiniMaxSettingsReader.swift,Sources/CodexBarCore/Providers/MiniMax/MiniMaxUsageFetcher.swift,Sources/CodexBarCore/Providers/MiniMax/MiniMaxProviderDescriptor.swift,Sources/CodexBarCore/Providers/Alibaba/AlibabaCodingPlanSettingsReader.swift,Sources/CodexBarCore/Providers/Alibaba/AlibabaCodingPlanUsageFetcher.swift,Sources/CodexBarCore/Providers/Alibaba/AlibabaCodingPlanProviderDescriptor.swiftRoot cause
Non-HTTPS credentialed endpoint overrides
CVSS assessment
CVSS:3.1/AV:L/AC:L/PR:L/UI:R/S:U/C:H/I:N/A:NRationale:
Safe reproduction steps
1. MiniMax invalid endpoint override
MINIMAX_REMAINS_URL=http://localhost:8080/remains.MiniMaxSettingsError.invalidEndpointOverride("MINIMAX_REMAINS_URL")before browser credential discovery.2. Alibaba Coding Plan invalid endpoint override
ALIBABA_CODING_PLAN_QUOTA_URL=http://localhost:8080/data/api.json.AlibabaCodingPlanSettingsError.invalidEndpointOverride("ALIBABA_CODING_PLAN_QUOTA_URL")before credentialed request construction.Expected vulnerable behavior
Changes in this PR
validateEndpointOverrideshelpers for MiniMax and Alibaba Coding Plan settings.host:portcompatibility.nilfor explicit non-HTTPS URLs.invalidEndpointOverrideerrors.Files changed
MiniMaxSettingsReader.swift,AlibabaCodingPlanSettingsReader.swiftMiniMaxUsageFetcher.swift,AlibabaCodingPlanUsageFetcher.swiftMiniMaxProviderDescriptor.swift,AlibabaCodingPlanProviderDescriptor.swiftMiniMaxProviderTests.swift,AlibabaCodingPlanProviderTests.swiftMaintainer impact
host:portvalues, still work and continue to be normalized to HTTPS.http://overrides for these credentialed provider fetches will now fail closed.Fix rationale
Type of change
Test plan
git diff --checkswift build --target CodexBarCoreswiftc: verified barehost:portvalues normalize to HTTPS/preserve host+port+path, and explicithttp://values reject for Alibaba and MiniMax./Scripts/lint.sh lintpartially completed: Codex parser hash was current, lint tools were pinned/current, SwiftFormat reported0/991 files require formatting; SwiftLint then failed in this local CLT environment while loadingsourcekitdInProc0/4 files require formattinglint-build-test,build-linux-cli (linux-x64, ubuntu-24.04),build-linux-cli (linux-arm64, ubuntu-24.04-arm), andGitGuardian Security Checkspassedswift test --filter 'AlibabaCodingPlanSettingsReaderTests|MiniMaxSettingsReaderTests'did not complete in this local toolchain because unrelated dependency/test-target compilation failed before the focused tests ranExecuted with:
The temporary smoke file and binary were removed after the proof run. No real provider credentials were used.
Disclosure notes
Latest validation update (55d63ed)
This follow-up addresses the ClawSweeper host:port resolver and pre-availability validation blockers on the current PR head.
Changes since the previous proof:
host:portvalues, solocalhost:8443is normalized as an HTTPS host override instead of being mistaken for a URL scheme.configuration, including fetch-attempt text that contains endpoint override wording before auth/API keywords.localhost:8443resolver paths, pre-credential invalid override rejection, and diagnostic categorization.Real usage-path smoke proof, run through a temporary external SwiftPM harness importing this checkout's
CodexBarCoreproduct with no real provider credentials:Local validation on the current pushed head:
The focused repository
swift testcommand is still blocked before the selected tests execute by this local Command Line Tools test-target/toolchain issue (TestsLinuximportingTesting). The security-relevant behavior above was exercised through the temporary external harness to avoid live credential prompts and avoid importing the repo test targets.Latest validation
Current pushed head:
5c0749eb887ec199a7f6b16820d87e7588352f33Local validation on this head:
git diff --check✅swift build --target CodexBarCore✅ (Build of target: 'CodexBarCore' complete! (6.96s))./Scripts/lint.sh lintpartially completed: parser hash current, lint tools current, SwiftFormat0/991 files require formatting; SwiftLint then fails locally while loadingsourcekitdInProcfrom this Command Line Tools environment.swiftformatis not on this PATH, so repo lint tooling was used for the SwiftFormat signal.swift testremains blocked locally by the existing Command Line Tools / test-target issue documented below.The latest code change after the prior security proof is formatting-only in the touched fetcher/test files; no real provider credentials were used.