Skip to content

fix: reject malformed provider endpoint hosts - #1439

Merged
steipete merged 1 commit into
mainfrom
fix/provider-endpoint-malformed-hosts
Jun 11, 2026
Merged

fix: reject malformed provider endpoint hosts#1439
steipete merged 1 commit into
mainfrom
fix/provider-endpoint-malformed-hosts

Conversation

@steipete

Copy link
Copy Markdown
Owner

Summary

Verification

  • swift test --filter 'AlibabaCodingPlanSettingsReaderTests|MiniMaxEndpointOverrideSettingsTests' (28 tests)
  • make check
  • autoreview --mode branch --base origin/main (clean)
  • unreleased model-name gate: clean

@clawsweeper

clawsweeper Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed June 11, 2026, 10:05 AM ET / 14:05 UTC.

Summary
The PR makes the shared MiniMax and Alibaba endpoint validator reject decoded whitespace and control characters in host overrides and adds focused regression coverage for raw and percent-encoded malformed hosts.

Reproducibility: yes. at source level: current main’s shared validator lacks decoded whitespace/control rejection, and the focused inputs exercise that exact path for both providers. The review did not run tests because the checkout was required to remain byte-for-byte read-only.

Review metrics: 2 noteworthy metrics.

  • Patch scope: 3 files, 26 additions, 0 deletions. The change is isolated to one shared validator and two focused provider test suites.
  • Provider coverage: 2 providers, 12 assertions. Each provider exercises three malformed host forms through both host-only and complete-URL settings.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
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:

  • none.

Risk before merge

  • [P1] After merge, an existing MiniMax or Alibaba configuration that relies on a whitespace- or control-containing endpoint host will fail validation instead of making a credentialed request; this is intentional hardening but is still an observable compatibility change in provider configuration.

Maintainer options:

  1. Land the malformed-host rejection (recommended)
    Accept the narrow configuration compatibility change because hosts containing decoded whitespace or controls are malformed and unsafe for credentialed provider requests.
  2. Require an upgrade note first
    Document in the PR body or release context that malformed MiniMax and Alibaba endpoint overrides will now fail validation before merging.

Next step before merge

  • [P2] The patch is already owner-authored and appears correct, so it needs ordinary maintainer merge handling rather than an automated repair branch.

Security
Cleared: The patch strengthens credentialed endpoint validation and introduces no dependency, supply-chain, permission, code-execution, or secret-exposure concern.

Review details

Best possible solution:

Land the centralized decoded-host validation with its focused tests, preserving valid arbitrary HTTPS proxy/test hosts and the existing opt-in provider-owned-host policy.

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

Yes at source level: current main’s shared validator lacks decoded whitespace/control rejection, and the focused inputs exercise that exact path for both providers. The review did not run tests because the checkout was required to remain byte-for-byte read-only.

Is this the best way to solve the issue?

Yes. Adding the checks once in the existing shared validator is the narrowest maintainable fix and avoids provider-specific validation drift.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 1bb6d107664a.

Label changes

Label changes:

  • add P2: This closes a real credentialed-provider validation gap with limited provider-specific blast radius and no reported active outage or exploit.
  • add merge-risk: 🚨 compatibility: Existing configurations containing whitespace or control characters in endpoint hosts will change from accepted parsing to fail-closed validation.
  • add merge-risk: 🚨 auth-provider: The changed validator controls credentialed MiniMax and Alibaba endpoint routing, so an incorrect rejection could prevent those provider requests.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external-contributor real-behavior proof gate does not apply because this PR is repository-owner authored; its reported focused tests and checks remain useful supplemental validation.

Label justifications:

  • P2: This closes a real credentialed-provider validation gap with limited provider-specific blast radius and no reported active outage or exploit.
  • merge-risk: 🚨 compatibility: Existing configurations containing whitespace or control characters in endpoint hosts will change from accepted parsing to fail-closed validation.
  • merge-risk: 🚨 auth-provider: The changed validator controls credentialed MiniMax and Alibaba endpoint routing, so an incorrect rejection could prevent those provider requests.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external-contributor real-behavior proof gate does not apply because this PR is repository-owner authored; its reported focused tests and checks remain useful supplemental validation.
Evidence reviewed

What I checked:

Likely related people:

  • steipete: Authored this focused follow-up and has the largest historical contribution count across the central MiniMax and Alibaba settings paths. (role: recent area contributor; confidence: high; commits: 1b11a1dfeaa4, af202b462bdf, 04c02e2c0ff5; files: Sources/CodexBarCore/ProviderEndpointOverrideValidator.swift, Sources/CodexBarCore/Providers/MiniMax/MiniMaxSettingsReader.swift, Sources/CodexBarCore/Providers/Alibaba/AlibabaCodingPlanSettingsReader.swift)
  • Hinotobi: Introduced the shared validator and the current MiniMax/Alibaba endpoint-override security boundary in the immediately preceding merged change. (role: introduced behavior; confidence: high; commits: 1bb6d107664a; files: Sources/CodexBarCore/ProviderEndpointOverrideValidator.swift, Sources/CodexBarCore/Providers/MiniMax/MiniMaxSettingsReader.swift, Sources/CodexBarCore/Providers/Alibaba/AlibabaCodingPlanSettingsReader.swift)
  • ratulsarna: Introduced the Alibaba Coding Plan provider path later adapted to use the shared endpoint validator. (role: original provider contributor; confidence: medium; commits: 043cebd3830e; files: Sources/CodexBarCore/Providers/Alibaba/AlibabaCodingPlanSettingsReader.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.

@clawsweeper clawsweeper Bot added 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. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. labels Jun 11, 2026
@steipete
steipete merged commit bea2a84 into main Jun 11, 2026
6 of 7 checks passed
@steipete

Copy link
Copy Markdown
Owner Author

Validated exact head 1b11a1dfeaa47e9eb9da8724fb29f1e53bdb6ef7.
Landed as bea2a84e862fde2b8a3eb3f064c128aa9cbd3c6e.

  • 28 focused Alibaba and MiniMax endpoint validation tests
  • rejects decoded whitespace and control characters in endpoint hosts
  • make check
  • autoreview clean
  • released/fictitious model-name gate clean
  • GitHub CI green on the exact head

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. P2 Normal priority bug or improvement with limited blast radius. 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.

1 participant