Skip to content

fix(presets): require two slashes after an http(s) baseUrl scheme and refuse userinfo in mcp env and plugin proxies - #2351

Merged
freeqaz merged 4 commits into
mainfrom
fix-1259
Oct 13, 2026
Merged

freeqaz merged 4 commits into
mainfrom
fix-1259

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Finding 1259, left by #2350:

  • The URL parser treats a special scheme's slashes as optional. So a model baseUrl of https:/v1, https:v1 or https:/api.example.com/v1 passed isRuntimeUrl and parsed with the first segment as its host. A Preset https:{{ vars.h }}/v1 saved, and so did https:${HOST}/v1. A ${...} reference skipped the baseUrl check entirely.
  • fix(presets): refuse url userinfo in proxy, channel, mcp and proxy env fields, an empty baseUrl host and non-string url variables #2350 refused URL userinfo in env and env.vars proxy variables, but not in the other fields that hold proxy URLs. On main these had no check at all:
    • mcp.servers.*.env and nodeHost.mcp.servers.*.env: only the server url was checked.
    • Codex's plugins.entries.codex.config.appServer.networkProxy.proxyUrl and socksUrl.
    • The acpx plugin's plugins.entries.acpx.config.mcpServers.*.env.
    • A speech provider's proxy, which the Microsoft speech plugin reads, in any TTS block: tts, agents.entries.*.tts, channels.discord.voice.tts and its accounts' blocks, voice-call's tts, and voice-call's numbers.*.tts, including each block's personas.*.providers. The same applies to talk.providers and talk.realtime.providers.

Change

  • isRuntimeUrl now requires exactly two slashes after an http: or https: scheme. It ignores case, leading C0 controls and spaces, and tab or newline characters. A backslash counts as a slash, so https:\\host still saves, as before. A Preset variable cannot supply the slashes: https:{{ vars.h }} and HTTPS:/{{ vars.h }}/v1 are refused at save.
  • For a baseUrl that holds a ${VAR} reference, the new writesSchemeSlashes checks only the slashes after a literal http(s) scheme. It checks each fallback rendering, as the userinfo rule does, so ${URL:-https:/h/v1} and https://${H:-}/v1 are refused. The runtime still owns the rest of the URL.
  • validateModelCredentialReferences applies the *_PROXY userinfo rule to every MCP server's env map, including acpx's and a name that waits on a variable. It also refuses userinfo in the Codex network proxy URLs and in speech and Talk provider proxy settings.
  • Test change: one fix(presets): find url userinfo when a variable fills the host or port #2347 userinfo case wrote a variable before the slashes in a baseUrl. That URL is now refused by the slash rule first, so the case moved to request.proxy.url, which has no slash rule. It still covers the userinfo check.
  • Breaking-changes notice: one sentence and one step on the 2026-10-13 model credential entry. The page is at 2481 words by countMarkdownWords.

Verification

  • tests/integration/configuration-controller.test.mjs has new refused and accepted cases in the 1255 userinfo test and the 1256 baseUrl test:
    • accepted: HTTPS://…, https:\\…, http://localhost:${MODEL_PORT}/v1, https://${MODEL_HOST}/v1, ${MODEL_BASE_URL}/v1, a plain MCP env proxy, a Codex proxyUrl, and a ${SPEECH_PROXY} reference
    • a Preset launch filled with https:/v1 is refused
  • Every new refused case fails on main and passes here. I checked them one at a time against main's two source files (all 20 accepted on main, all 20 refused here). The two touched tests also fail when run against main's sources.
  • Both touched tests passed 20 of 20 times with taskset -c 0,1.
  • configuration-controller.test.mjs, kubernetes-configuration.test.mjs and occ-api.test.mjs: 111 of 111 pass.
  • tsc --build, eslint and prettier are clean on the touched files.
  • A bypass review drove the second commit: agents.entries (not the retired agents.list), the Discord voice and Talk speech proxies, acpx MCP env, and fallback rendering for the slash rule.

Risks

  • Breaking: these saves are now refused:

    • an http(s) baseUrl written with zero, one or three or more slashes, or with a variable or reference in place of the slashes
    • userinfo in the fields above

    Stored Configurations still read and deploy as before. Only a save or Preset launch that keeps the value is refused.

  • Refusing slash-less spellings that WHATWG accepts is a provisional maintainer decision, made by precedent (pre-alpha, fail-early at save). A human may revisit it. It is limited to model baseUrl. Proxy and MCP URLs keep the parser's reading.

  • For a baseUrl with a ${VAR} reference, only the slashes are checked. The text after them stays the runtime's to validate.

  • Other child-process env maps (skills.entries.*.env, sandbox Docker env, a provider's localService.env, hook and secrets exec provider env) are still not checked for proxy userinfo. They are tracked as a follow-up finding.

… check mcp env and plugin proxy userinfo

A baseUrl such as https:/v1 or https:v1 parsed with v1 as its host
(finding 1259): the URL parser treats a special scheme's slashes as
optional. Refuse any http(s) baseUrl that does not write exactly two
slashes (or backslashes) after the scheme, including where a Preset
variable or an environment reference would have to supply them.

Also refuse URL userinfo in an MCP server's proxy environment variables
(mcp and nodeHost.mcp), Codex's network proxy URLs and speech provider
proxies, as for env.vars since #2350.
…ronment reference

Checking the whole URL around a reference refused shapes the userinfo
rule names more precisely. Only the two slashes after a literal http(s)
scheme are now checked there; the rest is the runtime's. Move the
variable-before-slashes userinfo case to request.proxy.url, which has no
slash rule, and add the breaking-change sentence.
@freeqaz
freeqaz requested a review from a team as a code owner October 13, 2026 17:43
@clawsweeper

clawsweeper Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review blocked

Automated review did not run, so no review verdict was produced.

Reason: The input-safety check rejected material in this revision. No detected value, path, or scanner output is reproduced here.

Automatic review is on hold, including after source changes. Request a fresh re-review after resolving the failure; maintainers can also release the hold through an intentional scanner-policy update.

Next step: If this is a genuine credential, remove and rotate it. If it is an intentional test fixture, a maintainer must review and qualify it.

View the workflow run.

…env and env fallbacks

Review of the first cut: per-agent TTS lives under agents.entries at the
pinned runtime, Discord voice and Talk providers also hand a proxy to
the Microsoft speech provider, and the acpx plugin's MCP servers take an
env map. A baseUrl fallback (${URL:-https:/v1}) is now rendered for the
slash check, as the userinfo check already did.

This branch was previously deployed

1 inactive deployment
integration-qa-pr — 3f1ff735 Deployed Oct 13, 2026 by freeqaz via QA advisory (kubernetes) #690
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant