Skip to content

[security] fix(providers): reject sibling HTTP endpoint overrides#1256

Open
Hinotoi-agent wants to merge 2 commits into
steipete:mainfrom
Hinotoi-agent:harden-sibling-provider-endpoint-overrides
Open

[security] fix(providers): reject sibling HTTP endpoint overrides#1256
Hinotoi-agent wants to merge 2 commits into
steipete:mainfrom
Hinotoi-agent:harden-sibling-provider-endpoint-overrides

Conversation

@Hinotoi-agent

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

Copy link
Copy Markdown
Contributor

Summary

This PR hardens sibling provider API endpoint overrides for credentialed usage calls.

  • Rejects explicit non-HTTPS endpoint overrides for OpenRouter, Codebuff, Groq, and ElevenLabs.
  • Preserves the existing convenience behavior where bare host/path override values are normalized to HTTPS.
  • Adds provider-level validation before credentialed usage fetches construct requests.
  • Adds regression coverage for HTTPS acceptance, bare-host normalization, and explicit HTTP rejection.

Security issues covered

Issue Impact Severity
Credentialed provider endpoint overrides accepted explicit HTTP URLs across OpenRouter, Codebuff, Groq, and ElevenLabs A local configuration or environment override could direct provider-owned credentialed API calls to plaintext HTTP, exposing bearer/API-key authenticated traffic to network interception or downgrade paths. Medium

Before this PR

  • OPENROUTER_API_URL, CODEBUFF_API_URL, GROQ_API_URL, and ELEVENLABS_API_URL could be set to explicit http://... URLs.
  • The corresponding usage fetchers built credentialed requests using the configured base URL.
  • The sibling provider paths did not have regression coverage matching the endpoint hardening already added for Alibaba/MiniMax.

After this PR

  • Explicit non-HTTPS endpoint overrides are rejected during provider settings validation.
  • Usage fetchers call the validation path before constructing credentialed requests.
  • Bare host/path overrides still infer HTTPS for compatibility.
  • A shared regression test locks the behavior for OpenRouter, Codebuff, Groq, and ElevenLabs.

Why this matters

These API URL settings are provider-owned credentialed endpoints. Once a token/API key is present, allowing an explicit HTTP override creates an unnecessary downgrade path for authenticated traffic. Rejecting non-HTTPS overrides keeps the secure default while preserving a safe HTTPS override mechanism for trusted local testing or alternate provider-compatible endpoints.

How this differs from related issue/PR

This is a sibling-provider follow-up to #1236. That PR covers Alibaba and MiniMax endpoint overrides. This PR applies the same boundary to OpenRouter, Codebuff, Groq, and ElevenLabs, whose settings readers and usage fetchers are separate code paths.

Attack flow

local configuration/environment sets provider API URL to http://...
    -> provider settings reader accepts the override
        -> usage fetcher builds a credentialed request from that base URL
            -> bearer/API-key authenticated provider traffic can be sent over plaintext HTTP

Root cause

  • API URL override readers normalized or accepted caller-controlled base URLs without rejecting explicit non-HTTPS schemes.
  • Credentialed usage fetchers trusted those base URLs before sending provider authentication headers.
  • Endpoint override validation was inconsistent across providers.

Changes in this PR

  • Adds explicit endpoint override validation to OpenRouter and Codebuff settings errors/descriptors.
  • Adds validateEndpointOverrides(environment:) guards to OpenRouter, Codebuff, Groq, and ElevenLabs credentialed usage fetchers.
  • Keeps HTTPS inference for bare host/path override values.
  • Adds ProviderEndpointOverrideSecurityTests covering accepted HTTPS, bare-host normalization, rejected HTTP fallback, and thrown validation errors.

Maintainer impact

  • Narrow provider settings/fetcher hardening only.
  • No real provider calls or production credentials are required by the new tests.
  • Existing HTTPS endpoint overrides and bare-host override values remain supported.
  • Explicit plaintext HTTP endpoint overrides are now rejected for credentialed provider-owned API calls.

Type of change

  • Security fix
  • Tests
  • Documentation update
  • Refactor with no behavior change

Test plan

  • swift build --target CodexBarCore
  • Focused standalone Swift smoke for OpenRouter/Codebuff/Groq/ElevenLabs endpoint override behavior with fake test values, including host:port bare overrides
  • swiftformat lint mode via make check reported 0/998 files require formatting
  • Direct SwiftLint on the touched files with the local CommandLineTools SourceKit path reported 0 violations
  • swift test --filter ProviderEndpointOverrideSecurityTests — blocked locally by existing KeyboardShortcuts #Preview macro/plugin build failure before these tests run:
    • external macro implementation type 'PreviewsMacros.SwiftUIView' could not be found for macro 'Preview(_:body:)'
  • Full make check — blocked locally after SwiftFormat passes because the wrapper SwiftLint invocation traps while loading SourceKit:
    • Fatal error: Loading sourcekitdInProc.framework/Versions/A/sourcekitdInProc failed

Executed with redacted/fake endpoint values only; no provider services or real credentials were used.

$ swift build --target CodexBarCore
Build of target: 'CodexBarCore' complete! (5.46s)

$ swiftc /tmp/codexbar_endpoint_override_smoke.swift \
  Sources/CodexBarCore/Providers/OpenRouter/OpenRouterSettingsReader.swift \
  Sources/CodexBarCore/Providers/Codebuff/CodebuffSettingsReader.swift \
  Sources/CodexBarCore/Providers/Groq/GroqSettingsReader.swift \
  Sources/CodexBarCore/Providers/ElevenLabs/ElevenLabsSettingsReader.swift \
  -o /tmp/codexbar_endpoint_override_smoke
$ /tmp/codexbar_endpoint_override_smoke
endpoint override smoke passed: host:port bare overrides normalize to HTTPS; explicit HTTP rejects

$ python3 - <<'PY'
from pathlib import Path
files = [
    'Sources/CodexBarCore/Providers/OpenRouter/OpenRouterSettingsReader.swift',
    'Sources/CodexBarCore/Providers/Codebuff/CodebuffSettingsReader.swift',
    'Sources/CodexBarCore/Providers/Groq/GroqSettingsReader.swift',
    'Sources/CodexBarCore/Providers/ElevenLabs/ElevenLabsSettingsReader.swift',
    'Tests/CodexBarTests/ProviderEndpointOverrideSecurityTests.swift',
]
for f in files:
    for i, line in enumerate(Path(f).read_text().splitlines(), 1):
        if len(line) > 120:
            raise SystemExit(f'{f}:{i}: line length {len(line)}')
print('line length <= 120 for endpoint override files')
PY
line length <= 120 for endpoint override files

$ DYLD_FRAMEWORK_PATH=/Library/Developer/CommandLineTools/usr/lib \
  .build/lint-tools/bin/swiftlint --strict --config .swiftlint.yml \
  Tests/CodexBarTests/ProviderEndpointOverrideSecurityTests.swift \
  Sources/CodexBarCore/Providers/OpenRouter/OpenRouterSettingsReader.swift \
  Sources/CodexBarCore/Providers/Codebuff/CodebuffSettingsReader.swift \
  Sources/CodexBarCore/Providers/Groq/GroqSettingsReader.swift \
  Sources/CodexBarCore/Providers/ElevenLabs/ElevenLabsSettingsReader.swift
Done linting! Found 0 violations, 0 serious in 5 files.

$ swift test --filter ProviderEndpointOverrideSecurityTests
/private/tmp/codexbar-main-security-audit/.build/checkouts/KeyboardShortcuts/Sources/KeyboardShortcuts/Recorder.swift:172:1: error: external macro implementation type 'PreviewsMacros.SwiftUIView' could not be found for macro 'Preview(_:body:)'; plugin for module 'PreviewsMacros' not found

$ make check
SwiftFormat completed in 0.16s.
0/998 files require formatting.
SourceKittenFramework/library_wrapper.swift:58: Fatal error: Loading sourcekitdInProc.framework/Versions/A/sourcekitdInProc failed

The smoke script asserts:

  • localhost:8080 and localhost:8080/v1 are treated as bare host/path overrides and normalize to HTTPS.
  • http://attacker.test... remains rejected by the validation path for OpenRouter, Codebuff, Groq, and ElevenLabs.

Disclosure notes

@clawsweeper

clawsweeper Bot commented Jun 1, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed May 31, 2026, 11:47 PM ET / 03:47 UTC.

Summary
The PR rejects explicit non-HTTPS endpoint overrides for OpenRouter, Codebuff, Groq, and ElevenLabs usage fetches, preserves bare-host HTTPS normalization, and adds provider regression tests.

Reproducibility: yes. source inspection gives a high-confidence reproduction path: current main accepts explicit http:// endpoint override URLs and then builds authenticated provider requests from those base URLs. I did not run live provider or keychain validation because repository policy steers this path toward fake endpoint/parser checks.

Review metrics: 3 noteworthy metrics.

  • Provider surfaces: 4 providers changed. OpenRouter, Codebuff, Groq, and ElevenLabs each get endpoint override validation before credentialed usage requests.
  • Diff scope: 11 files, +282/-18. The patch is narrow but cross-provider, so maintainers should review behavior consistency across all touched providers.
  • Regression coverage: 1 new test file with 4 provider cases. The tests cover HTTPS acceptance, bare-host normalization, and explicit HTTP rejection for each provider.

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:

  • [P2] Maintainer should explicitly accept or reject the HTTPS-only override compatibility tradeoff.
  • [P2] Gate merge on required CI or an equivalent focused ProviderEndpointOverrideSecurityTests validation pass.

Risk before merge

  • [P1] Explicit http:// endpoint overrides for OpenRouter, Codebuff, Groq, and ElevenLabs will fail closed after merge, so existing local plaintext proxy or debug setups must switch to HTTPS or a bare-host value if maintainers accept the new boundary.
  • [P1] The contributor reports that the focused SwiftPM test and full make check did not complete in their local toolchain, so merge should still wait for required CI or an equivalent maintainer validation pass.
  • [P1] The PR base is one commit behind current main, but the observed drift only touches release/test stabilization files and does not show a concrete conflict with these provider paths.

Maintainer options:

  1. Accept HTTPS-only provider overrides (recommended)
    Maintainers can intentionally accept that explicit plaintext endpoint overrides now fail closed because these calls carry provider credentials.
  2. Preserve a local HTTP debug path
    If plaintext local proxies must remain supported, add a documented opt-in escape hatch or narrower development-only path before merging this fail-closed behavior.
  3. Hold for validation evidence
    Wait for required CI or an equivalent focused ProviderEndpointOverrideSecurityTests run before merging the cross-provider settings change.

Next step before merge

  • [P1] Human maintainer review should decide the fail-closed HTTP override policy and validation gate; there is no narrow automated code repair left from this review.

Security
Cleared: The diff tightens credentialed endpoint transport handling and does not add dependencies, workflows, scripts, secret storage, or other supply-chain-sensitive surfaces.

Review details

Best possible solution:

Land the HTTPS-only override hardening after maintainer acceptance of the compatibility tradeoff and required CI or focused provider tests pass.

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

Yes, source inspection gives a high-confidence reproduction path: current main accepts explicit http:// endpoint override URLs and then builds authenticated provider requests from those base URLs. I did not run live provider or keychain validation because repository policy steers this path toward fake endpoint/parser checks.

Is this the best way to solve the issue?

Yes, with maintainer acceptance: validating endpoint overrides before request construction is the narrow security fix and preserves HTTPS plus bare-host normalization. The remaining question is whether the fail-closed behavior for explicit plaintext overrides is an acceptable compatibility tradeoff.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a bounded security hardening for optional provider endpoint configuration, with limited blast radius but real credential-transport impact.
  • merge-risk: 🚨 compatibility: Existing users who intentionally configured explicit plaintext endpoint overrides will see those provider usage fetches fail closed after merge.
  • 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 and follow-up comment include copied redacted terminal output from a focused after-fix smoke run showing bare host:port normalization and explicit HTTP rejection with fake endpoint values.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body and follow-up comment include copied redacted terminal output from a focused after-fix smoke run showing bare host:port normalization and explicit HTTP rejection with fake endpoint values.
Evidence reviewed

What I checked:

Likely related people:

  • Peter Steinberger: Recent provider expansion and the current release snapshot touch the provider surface and docs around these paths. (role: recent area contributor; confidence: high; commits: 37bc49f756ba, d715648cf23d; files: Sources/CodexBarCore/Providers/Groq, Sources/CodexBarCore/Providers/ElevenLabs, docs/providers.md)
  • Ratul Sarna: History shows Codebuff provider support and substantial OpenRouter follow-up work in the affected provider areas. (role: OpenRouter and Codebuff feature contributor; confidence: high; commits: d20a9bf2e953, 34a903941cc8, 9d6b32b8789f; files: Sources/CodexBarCore/Providers/OpenRouter, Sources/CodexBarCore/Providers/Codebuff)
  • chountalas: The original OpenRouter provider implementation introduced the affected OpenRouter settings/fetcher area. (role: original OpenRouter contributor; confidence: medium; commits: 8348c85cd8d4; files: Sources/CodexBarCore/Providers/OpenRouter)
  • Mikael Goderdzishvili: Recent settings-reader quote cleanup work touched the same class of provider configuration parsing code. (role: recent settings-reader contributor; confidence: medium; commits: 97f9ecb2b96c; files: Sources/CodexBarCore/Providers/OpenRouter, Sources/CodexBarCore/Providers/Codebuff, Sources/CodexBarCore/Providers/Groq)
  • Misty: The merged Alibaba endpoint hardening commit is the closest prior implementation pattern for rejecting non-HTTPS provider overrides. (role: adjacent endpoint-hardening contributor; confidence: medium; commits: fd4878ba669c; files: Sources/CodexBarCore/Providers/Alibaba/AlibabaTokenPlanSettingsReader.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: 🧂 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 1, 2026
@clawsweeper clawsweeper Bot added 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. 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. 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 1, 2026
@Hinotoi-agent

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review please — updated the branch at f13d1e9a to address the host:port bare override parsing and strict-lint feedback.

I also updated the PR body with copied redacted after-fix output, including:

  • swift build --target CodexBarCore completing successfully.
  • Focused Swift smoke output: endpoint override smoke passed: host:port bare overrides normalize to HTTPS; explicit HTTP rejects.
  • Line-length check output for the touched endpoint override files/tests.
  • Direct strict SwiftLint output showing Done linting! Found 0 violations, 0 serious in 5 files.

The local swift test --filter ProviderEndpointOverrideSecurityTests and full make check blockers are still documented separately in the PR body; they occur before the touched tests run / inside the wrapper SourceKit load path, while the focused after-fix behavior proof uses fake/redacted endpoint values only.

@clawsweeper

clawsweeper Bot commented Jun 1, 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: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. 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.

1 participant