Skip to content

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

Merged
steipete merged 3 commits into
steipete:mainfrom
Hinotoi-agent:security/validate-provider-endpoint-overrides
Jun 11, 2026
Merged

[security] fix(providers): validate sibling endpoint overrides#1269
steipete merged 3 commits into
steipete:mainfrom
Hinotoi-agent:security/validate-provider-endpoint-overrides

Conversation

@Hinotoi-agent

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

Copy link
Copy Markdown
Contributor

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

clawsweeper Bot commented Jun 1, 2026

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed June 11, 2026, 9:20 AM ET / 13:20 UTC.

Summary
The branch adds shared MiniMax and Alibaba Coding Plan endpoint override validation, fail-closed credentialed fetch checks, strict provider-owned host options, diagnostics, tests, docs, and a changelog entry.

Reproducibility: yes. Current-main source accepts MiniMax and Alibaba endpoint overrides without the PR's HTTPS/userinfo/encoded-delimiter validation, and the PR body includes redacted terminal proof for the fixed validator paths rather than live credential probes.

Review metrics: 3 noteworthy metrics.

  • Diff Surface: 14 files changed, +628/-83. The security-sensitive provider change spans implementation, tests, diagnostics, docs, and release notes.
  • Provider Scope: 2 credentialed providers. The behavior affects MiniMax and Alibaba Coding Plan credentialed endpoint override paths.
  • Release-Owned File: 1 CHANGELOG.md entry added. The remaining mechanical cleanup is easy to spot and remove before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

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

Rank-up moves:

  • Remove the CHANGELOG.md entry.
  • Wait for current-head checks and maintainer endpoint-policy sign-off.

Risk before merge

  • [P1] Existing MiniMax or Alibaba Coding Plan users with explicit http, embedded-userinfo, encoded-delimiter, or otherwise rejected endpoint override values will now fail closed before credentialed fetches.
  • [P1] The PR deliberately keeps arbitrary custom HTTPS hosts accepted by default, so maintainers should explicitly approve that provider cookies or API credentials may still go to config-controlled HTTPS endpoints unless strict mode is enabled.
  • [P1] Current PR metadata reports an unstable merge state, so merge should wait for current-head checks to complete successfully.

Maintainer options:

  1. Approve The Current Endpoint Boundary (recommended)
    Maintainers can intentionally accept fail-closed invalid overrides while preserving arbitrary custom HTTPS domains by default and provider-owned strict mode as an opt-in.
  2. Add An Unsafe Dev Escape Hatch
    If explicit HTTP localhost or proxy setups must keep working, add a clearly named unsafe opt-in with focused tests and docs before merge.
  3. Tighten The Default Host Policy
    If provider credentials should never go to non-provider hosts by default, revise the PR to make provider-owned hosts the default and document the migration path.

Next step before merge

  • [P2] A narrow automated repair can remove the release-owned changelog entry; endpoint policy and final merge approval remain maintainer decisions.

Security
Cleared: The security pass found no added supply-chain or secret-handling regression; the PR hardens credentialed endpoint override hygiene, with policy tradeoffs tracked as merge risk.

Review findings

  • [P3] Drop the release-owned changelog entry — CHANGELOG.md:14
Review details

Best possible solution:

Merge after the changelog edit is dropped, current-head checks are green, and maintainers explicitly accept the endpoint boundary: unsafe overrides fail closed, custom HTTPS remains compatible by default, and provider-owned strict mode is opt-in.

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

Yes. Current-main source accepts MiniMax and Alibaba endpoint overrides without the PR's HTTPS/userinfo/encoded-delimiter validation, and the PR body includes redacted terminal proof for the fixed validator paths rather than live credential probes.

Is this the best way to solve the issue?

Mostly yes. The shared validator plus focused provider tests is a narrow maintainable fix, but the changelog entry should be dropped and maintainers still need to approve the compatibility/security boundary before merge.

Full review comments:

  • [P3] Drop the release-owned changelog entry — CHANGELOG.md:14
    CHANGELOG.md is release-owned for this repository's review flow, so this normal implementation PR should keep release-note text in the PR body or commits and leave the changelog for the release process.
    Confidence: 0.9

Overall correctness: patch is correct
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 67a9f43f58bf.

Label changes

Label changes:

  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body contains redacted terminal output from a local smoke executable exercising the real CodexBarCore settings and fetcher validator paths after the fix.
  • remove status: ⏳ waiting on author: Current PR status label is status: 👀 ready for maintainer look.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
  • remove merge-risk: 🚨 security-boundary: Current PR review merge-risk labels are merge-risk: 🚨 compatibility, merge-risk: 🚨 auth-provider.

Label justifications:

  • P1: This security hardening affects credentialed provider request routing and should receive prompt maintainer review.
  • merge-risk: 🚨 compatibility: Fail-closed validation can intentionally break existing setups that configured now-rejected endpoint override values.
  • merge-risk: 🚨 auth-provider: The diff changes when MiniMax and Alibaba credentialed provider fetches stop before using cookies or API keys.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body contains redacted terminal output from a local smoke executable exercising the real CodexBarCore settings and fetcher validator paths after the fix.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body contains redacted terminal output from a local smoke executable exercising the real CodexBarCore settings and fetcher validator paths after the fix.
Evidence reviewed

Acceptance criteria:

  • [P1] git diff --check.

What I checked:

Likely related people:

  • steipete: Available local blame for the existing MiniMax and Alibaba endpoint override readers points to the v0.33.0 release commit by Peter Steinberger, and the latest PR head commit preserving provider error API compatibility is also by Peter Steinberger. (role: current area contributor and recent branch committer; confidence: medium; commits: 6cf422512061, 5bdfad9cc849; files: Sources/CodexBarCore/Providers/MiniMax/MiniMaxSettingsReader.swift, Sources/CodexBarCore/Providers/Alibaba/AlibabaCodingPlanSettingsReader.swift, Sources/CodexBarCore/ProviderEndpointOverrideValidator.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.

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

Copy link
Copy Markdown

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

clawsweeper Bot commented Jun 2, 2026

Copy link
Copy Markdown

🦞🧹
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

clawsweeper Bot commented Jun 2, 2026

Copy link
Copy Markdown

🦞🧹
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

clawsweeper Bot commented Jun 2, 2026

Copy link
Copy Markdown

🦞🧹
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: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 2, 2026
@clawsweeper clawsweeper Bot added 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. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed 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. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 5, 2026
@clawsweeper

clawsweeper Bot commented Jun 9, 2026

Copy link
Copy Markdown

🦞🧹
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:

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. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants