Skip to content

[security] fix(providers): validate sibling endpoint overrides#1269

Open
Hinotoi-agent wants to merge 9 commits into
steipete:mainfrom
Hinotoi-agent:security/validate-provider-endpoint-overrides
Open

[security] fix(providers): validate sibling endpoint overrides#1269
Hinotoi-agent wants to merge 9 commits into
steipete:mainfrom
Hinotoi-agent:security/validate-provider-endpoint-overrides

Conversation

@Hinotoi-agent
Copy link
Copy Markdown
Contributor

@Hinotoi-agent Hinotoi-agent commented Jun 1, 2026

Summary

  • Add a shared provider endpoint override validator for MiniMax and Alibaba Coding Plan credentialed provider requests.
  • Reject non-HTTPS endpoint overrides, embedded userinfo, and percent-encoded authority/host delimiters before cookies or API credentials are attached to follow-up requests.
  • Preserve custom HTTPS proxy/test-host compatibility by default: arbitrary HTTPS hosts continue to work, while insecure or authority-confusing URLs fail closed.
  • Add optional strict provider-owned-host mode for deployments that want a tighter allowlist:
    • MINIMAX_REQUIRE_PROVIDER_ENDPOINT_OVERRIDES=true
    • ALIBABA_CODING_PLAN_REQUIRE_PROVIDER_ENDPOINT_OVERRIDES=true
  • Keep explicit rejected overrides fail-closed instead of silently falling back to the default credentialed provider endpoint.
  • Add focused regression coverage for default custom HTTPS compatibility, strict provider-owned-host enforcement, rejected http:// overrides, rejected user:pass@ URLs, encoded-delimiter bypasses, accepted provider-owned HTTPS/bare host overrides, and fail-closed fetcher behavior.
  • Update MiniMax and Alibaba provider docs to document the HTTPS-only default and optional strict provider-host mode.

Compatibility note

This revision keeps the compatibility path requested in review: custom HTTPS proxy/test domains remain accepted by default for MiniMax and Alibaba Coding Plan endpoint overrides.

The hard security boundary is now scheme/authority hygiene:

  • https://... custom proxy/test domains are accepted by default.
  • Bare hosts are normalized to HTTPS.
  • http://... overrides are rejected so cookies/API credentials are not sent in cleartext.
  • URLs with embedded userinfo are rejected.
  • Percent-encoded host/authority delimiter tricks are rejected.
  • If an explicit override is rejected, the fetcher fails closed and reports the rejected config key instead of silently falling back to the default provider endpoint.

Operators that prefer provider-owned hosts only can opt into strict mode with MINIMAX_REQUIRE_PROVIDER_ENDPOINT_OVERRIDES=true or ALIBABA_CODING_PLAN_REQUIRE_PROVIDER_ENDPOINT_OVERRIDES=true.

Validation

  • git diff --check — passed.
  • swift build --target CodexBarCore — passed:
Build of target: 'CodexBarCore' complete! (1.21s)
  • Temporary local smoke executable depending on this checkout's real CodexBarCore — passed. The proof exercised the actual MiniMax/Alibaba settings readers and fetcher validators with redacted credentials/cookies:
PASS MiniMax default preserves custom HTTPS host
PASS MiniMax custom HTTPS not rejected by default
PASS MiniMax strict mode rejects custom host
PASS MiniMax strict mode reports rejected host key
PASS Alibaba default preserves custom HTTPS host
PASS Alibaba custom HTTPS not rejected by default
PASS Alibaba strict mode rejects custom host
PASS Alibaba strict mode reports rejected host key
PASS MiniMax rejects HTTP host override
PASS Alibaba rejects userinfo URL override
  • swift test --filter 'MiniMaxProviderTests|AlibabaCodingPlanProviderTests' — still blocked before the focused tests by the existing local KeyboardShortcuts/SwiftUI #Preview macro toolchain issue (PreviewsMacros plugin not found) with the active developer directory using CommandLineTools rather than full Xcode.

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 1, 2026

Codex review: needs changes before merge. Reviewed June 5, 2026, 12:07 PM ET / 16:07 UTC.

Summary
The PR adds shared endpoint override validation, diagnostics, docs, and focused tests for MiniMax and Alibaba Coding Plan credentialed provider requests.

Reproducibility: yes. Source inspection of PR head shows validatedHost returns URL.host, which omits an explicit port, while current main returns the cleaned host override directly before URL reconstruction.

Review metrics: 2 noteworthy metrics.

  • Provider surfaces changed: 2 credentialed providers. MiniMax and Alibaba Coding Plan both attach cookies or API keys to URLs derived from these endpoint override settings.
  • Diff size: 13 files, +428/-55. The patch spans core, docs, and tests, so focused compatibility checks matter before merge.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦞 diamond lobster
Patch quality: 🦐 gold shrimp
Result: needs maintainer review before merge.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Preserve explicit ports for host-base endpoint overrides.
  • [P2] Fix the MiniMax custom-host test so it matches the documented default compatibility mode.

Risk before merge

  • [P1] Host-base overrides such as MINIMAX_HOST=proxy.example.test:8443 or ALIBABA_CODING_PLAN_HOST=proxy.example.test:8443 can be accepted but rebuilt without the explicit port, breaking proxy/test setups or routing credentials to the same host on the default HTTPS port.
  • [P1] Provider-owned strict mode and default custom-HTTPS compatibility are auth/security policy-sensitive behaviors that still need maintainer sign-off after the mechanical blockers are repaired.

Maintainer options:

  1. Repair validator and tests before merge (recommended)
    Preserve explicit ports for host-base overrides, add regression coverage for the port case, and remove the contradictory MiniMax custom-host expectation before maintainers make the final policy call.
  2. Pause for provider auth policy sign-off
    A maintainer can hold the PR until they explicitly accept the default compatibility mode and optional strict provider-host mode for credentialed provider requests.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Preserve explicit ports in MiniMax and Alibaba host-base endpoint overrides while keeping HTTPS/userinfo/encoded-delimiter rejection, then align the MiniMax custom-host tests with default compatibility mode and add explicit-port regression coverage.

Next step before merge

  • [P2] The remaining blockers are narrow mechanical repairs in the validator and tests; final merge still needs maintainer awareness because credentialed provider endpoint policy is sensitive.

Security
Needs attention: The patch improves endpoint override hygiene but still needs the explicit-port routing bug fixed before the credential boundary is safe to merge.

Review findings

  • [P1] Preserve explicit ports in host overrides — Sources/CodexBarCore/ProviderEndpointOverrideValidator.swift:18-20
  • [P2] Align the MiniMax custom-host expectation — Tests/CodexBarTests/MiniMaxProviderTests.swift:30-41
Review details

Best possible solution:

Keep the shared validator, but carry a validated host authority that preserves explicit ports for host-base overrides and make the focused tests reflect default custom-HTTPS compatibility plus optional strict provider-host mode.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection of PR head shows validatedHost returns URL.host, which omits an explicit port, while current main returns the cleaned host override directly before URL reconstruction.

Is this the best way to solve the issue?

No, not yet. The shared validator is the right repair direction, but it needs a validated host-authority path that preserves ports and a corrected MiniMax test expectation before this is the narrowest safe solution.

Full review comments:

  • [P1] Preserve explicit ports in host overrides — Sources/CodexBarCore/ProviderEndpointOverrideValidator.swift:18-20
    validatedHost returns only URL.host, so MINIMAX_HOST=proxy.example.test:8443 or the Alibaba equivalent validates but then resolves as https://proxy.example.test/.... Current main preserves the raw host-base override through URL reconstruction, so this breaks existing proxy/test setups and can route credentials to the same host on the default HTTPS port.
    Confidence: 0.89
  • [P2] Align the MiniMax custom-host expectation — Tests/CodexBarTests/MiniMaxProviderTests.swift:30-41
    This test still expects arbitrary HTTPS MiniMax endpoint overrides to be rejected, but the PR body, docs, and the later test in this same block say custom HTTPS proxy/test hosts are accepted by default. As written, the focused MiniMax suite either fails or preserves the wrong contract for future changes.
    Confidence: 0.94

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 65e39f4dcb3a.

Label changes

Label changes:

  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.

Label justifications:

  • P1: The PR hardens credentialed provider endpoint routing but still has merge-blocking compatibility and security-boundary details.
  • merge-risk: 🚨 compatibility: The proposed host validator changes existing endpoint override behavior and can break explicit-port proxy/test configurations.
  • merge-risk: 🚨 auth-provider: MiniMax and Alibaba requests use cookies or API keys, so endpoint validation affects where credentials are sent.
  • merge-risk: 🚨 security-boundary: The PR defines the URL authority checks that prevent provider credentials from being sent to unsafe endpoint overrides.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body includes redacted terminal proof from a temporary executable using the real CodexBarCore settings readers and validator paths after the fix.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes redacted terminal proof from a temporary executable using the real CodexBarCore settings readers and validator paths after the fix.
Evidence reviewed

Security concerns:

  • [medium] Explicit-port host overrides are silently rewritten — Sources/CodexBarCore/ProviderEndpointOverrideValidator.swift:18
    Dropping the port from validated host-base overrides can send credentialed provider requests to a different service than the operator configured.
    Confidence: 0.88

Acceptance criteria:

  • [P1] git diff --check.
  • [P1] swift build --target CodexBarCore.
  • [P1] swift test --filter 'MiniMaxEndpointOverrideSettingsTests|AlibabaCodingPlanSettingsReaderTests|AlibabaCodingPlanRegionTests|ProviderDiagnosticExportTests'.
  • [P1] make check if available in the PR environment without Keychain prompts.

What I checked:

Likely related people:

  • steipete: Recent commits touched MiniMax billing, Alibaba quota fallback behavior, provider HTTP response centralization, and current checkout blame for the settings readers points to Peter Steinberger's release commit. (role: recent area contributor; confidence: high; commits: af202b462bdf, ad33b32773bd, 197a2df946f2; files: Sources/CodexBarCore/Providers/MiniMax/MiniMaxUsageFetcher.swift, Sources/CodexBarCore/Providers/Alibaba/AlibabaCodingPlanUsageFetcher.swift, Sources/CodexBarCore/Providers/MiniMax/MiniMaxSettingsReader.swift)
  • m1qaweb: Recent settings-reader quote-cleanup work touched the same family of environment/config parsing helpers used by these provider override readers. (role: recent settings-reader contributor; confidence: medium; commits: 97f9ecb2b96c; files: Sources/CodexBarCore/Providers/MiniMax/MiniMaxSettingsReader.swift, Sources/CodexBarCore/Providers/Alibaba/AlibabaCodingPlanSettingsReader.swift)
  • ratulsarna: The Alibaba Coding Plan provider and its settings, usage fetcher, docs, and tests appear to have been introduced in the provider merge commit. (role: introduced Alibaba provider behavior; confidence: medium; commits: 043cebd3830e; files: Sources/CodexBarCore/Providers/Alibaba/AlibabaCodingPlanSettingsReader.swift, Sources/CodexBarCore/Providers/Alibaba/AlibabaCodingPlanUsageFetcher.swift, Tests/CodexBarTests/AlibabaCodingPlanProviderTests.swift)
  • Yuxin-Qiao: Recent MiniMax quota and usage-card work touched the MiniMax usage fetcher area affected by endpoint override resolution. (role: recent MiniMax provider contributor; confidence: medium; commits: 645ca833df31; files: Sources/CodexBarCore/Providers/MiniMax/MiniMaxUsageFetcher.swift, Tests/CodexBarTests/MiniMaxProviderTests.swift)
  • serezha93: The shared provider HTTP transport seam used by the Alibaba test changes was recently added in this area. (role: adjacent provider transport contributor; confidence: medium; commits: f62bb8c8d564; files: Sources/CodexBarCore/Providers/Alibaba/AlibabaCodingPlanUsageFetcher.swift, Sources/CodexBarCore/Providers/MiniMax/MiniMaxUsageFetcher.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9e898bc703

ℹ️ 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".

Comment thread Sources/CodexBarCore/ProviderEndpointOverrideValidator.swift Outdated
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jun 1, 2026
@Hinotoi-agent Hinotoi-agent force-pushed the security/validate-provider-endpoint-overrides branch 2 times, most recently from 7ad669a to 8c40a44 Compare June 2, 2026 00:22
@Hinotoi-agent
Copy link
Copy Markdown
Contributor Author

Updated for the review blockers:

  • Rejected explicit endpoint overrides now fail closed before credentialed provider requests instead of falling back silently.
  • Added fail-closed regression coverage for MiniMax and Alibaba Coding Plan API/cookie paths.
  • Added redacted real local env/settings-path proof to the PR body showing accepted provider HTTPS overrides and rejected attacker/HTTP/userinfo/encoded-host overrides.

Maintainers: please explicitly sign off on the provider-owned-only endpoint policy. This intentionally breaks arbitrary proxy/test override domains for these credentialed provider paths; HTTPS provider-owned hosts and bare provider hosts remain supported.

@clawsweeper re-review

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 2, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 2, 2026
@Hinotoi-agent Hinotoi-agent force-pushed the security/validate-provider-endpoint-overrides branch from 8c40a44 to af676e4 Compare June 2, 2026 00:51
@Hinotoi-agent
Copy link
Copy Markdown
Contributor Author

Updated again for the Alibaba diagnostic categorization blocker and pushed af676e46.

What changed:

  • AlibabaCodingPlanUsageError.invalidEndpointOverride(...) now maps explicitly to diagnostic category configuration, matching MiniMax and avoiding the generic localized-text classifier that could see “API”/“HTTPS” and misclassify it as api.
  • Added focused regression coverage for the Alibaba invalid endpoint override diagnostic category and safe description.

Validation:

swift build --target CodexBarCore
Build of target: 'CodexBarCore' complete!
git diff --check
# passed

Standalone diagnostic proof from a temporary local package depending on this checkout's CodexBarCore:

category=configuration
safeDescription=Configuration issue - check provider source and settings
PASS Alibaba invalid endpoint override diagnostic is configuration

The direct focused test command is still blocked locally by the existing KeyboardShortcuts / SwiftUI #Preview macro toolchain issue (PreviewsMacros plugin not found), so I used the standalone package proof above to exercise the diagnostic path.

Maintainers: still requesting explicit sign-off on the provider-owned-only endpoint policy for these credentialed provider overrides.

@clawsweeper re-review

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 2, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@Hinotoi-agent Hinotoi-agent force-pushed the security/validate-provider-endpoint-overrides branch from af676e4 to 59b2f45 Compare June 2, 2026 09:43
@Hinotoi-agent
Copy link
Copy Markdown
Contributor Author

Addressed the latest ClawSweeper blocker and pushed an amended commit (59b2f45c).

What changed:

  • Added the explicit Alibaba invalid endpoint override diagnostic regression so AlibabaCodingPlanUsageError.invalidEndpointOverride("ALIBABA_CODING_PLAN_QUOTA_URL") maps to configuration, not api/fallback wording.
  • Preserved the safe generic diagnostic text: Configuration issue - check provider source and settings.
  • Fixed the CI SwiftLint line-length failures in the MiniMax/Alibaba invalid override error strings.

Validation:

swift build --target CodexBarCore
Build of target: 'CodexBarCore' complete!

git diff --check
PASS

swift run DiagnosticSmoke
PASS Alibaba invalid endpoint override diagnostic category: configuration
PASS Alibaba invalid endpoint override safe description: Configuration issue - check provider source and settings

Local focused swift test --filter ProviderDiagnosticExportTests/test_alibabaInvalidEndpointOverrideUsesConfigurationCategory is still blocked before the test runs by the existing local KeyboardShortcuts / SwiftUI preview macro issue:

external macro implementation type 'PreviewsMacros.SwiftUIView' could not be found for macro 'Preview(_:body:)'; plugin for module 'PreviewsMacros' not found

The temporary smoke executable imports the real CodexBarCore product and exercises the same exported diagnostic path without credentials.

@clawsweeper could you re-review this update when you get a chance?

@clawsweeper clawsweeper Bot added rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. and removed proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 2, 2026
@Hinotoi-agent
Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 2, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jun 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants