From 92bedaac4aeda02ef0ec5781d6b1c03b2d002962 Mon Sep 17 00:00:00 2001 From: SkilLab HR and Tech Date: Thu, 15 Oct 2026 19:46:11 -0300 Subject: [PATCH] fix(server): treat a scheme-less public base URL as absent instead of crashing startup (#13727) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - At boot the server selects the origin that agents must dial for the control plane. It exports that origin to every run as `PAPERCLIP_API_URL` > - The selection starts from `auth.publicBaseUrl`. An operator writes this value by hand > - If the operator writes `host:port` and omits the scheme, the server does not reject the value. The value is a valid URL with an opaque scheme. The `origin` of such a URL serializes to the *string* `"null"` > - The string `"null"` is truthy. Thus it gets through the `if (value)` guard. The enclosing `try/catch` does not help, because the parse did not fail. A later `new URL("null")` then throws `TypeError: Invalid URL` on the startup path > - The result is that the server does not start. The error message names neither the configuration key nor the bad value > - This pull request parses the public base URL in one helper. The helper treats an unusable origin as absent configuration. This is the same treatment that input which fails to parse gets today > - The benefit is that a missing `https://` falls back to the existing derived candidates. It no longer stops the boot ## Linked Issues or Issue Description Fixes: #13576 — that issue reports this defect and includes a standalone reproduction. **Related PRs found by the duplicate search (none fix this defect).** I searched the open PR list for `runtime-api`, `publicBaseUrl`, `public base url`, `PAPERCLIP_API_URL` and `Invalid URL`. I then read the diff of every close neighbour. Refs #10517, Refs #9916, Refs #7342, Refs #12886, Refs #11564, Refs #9228. No one of them contains a test for an opaque origin, and no one of them compares an origin against `"null"`. They change *which reachable origin wins*. This pull request changes *how one unusable value is rejected*. The two subjects do not overlap. Refs #10517 and Refs #9916 are duplicates of each other. Neither touches `server/src/runtime-api.ts`. **One neighbour needs attention.** Refs #11564 adds a new function `resolveRuntimeApiUrl`. That function contains this code: ```ts try { const origin = new URL(explicitPublicBaseUrl).origin; return { url: origin, reason: "explicit-public-base-url", ... }; } catch { // Fall through to derived candidates if config parsing drifted. } ``` This is the same defect in a third location. A scheme-less value parses correctly. Thus the `catch` does not run, and the function returns the literal string `"null"` as the URL. The lines are pure additions, so there is no text conflict with this pull request. But if #11564 merges after this one, it puts the defect back in a new function. The helper in this pull request is module-private and available to that function. I can add the one-line change to this branch if the maintainers prefer that. ## What Changed `server/src/runtime-api.ts` — one new module-private helper, used at the two existing call sites: ```ts function parsePublicOrigin(rawUrl: string | null | undefined): string | null { const trimmed = rawUrl?.trim(); if (!trimmed) return null; let origin: string; try { origin = new URL(trimmed).origin; } catch { return null; } return origin === "null" ? null : origin; } ``` - `choosePrimaryRuntimeApiUrl` now calls the helper. Its inline `try/catch` is removed. - `buildRuntimeApiCandidateUrls` now calls the helper. Its inline IIFE is removed. - `server/src/__tests__/runtime-api.test.ts` gets two new cases. One case covers the wrong `"null"` result. The other case covers the startup exception. Counts: **+20 / −17** in `runtime-api.ts`. Five of the added lines are the comment that explains the opaque scheme. The test file is **+23 / −0**. There is no new dependency. There is no `any` type. No exported signature changes. The duplicated parse logic at the two call sites becomes one function. ## Verification I ran every step below against `master` at commit `c65fc9e3c81c41aafe421aa90a00514b84343285`. That commit is the merge base of this branch. - **Unit tests**, with the repository runner (`vitest`), on the two added cases: - Before the fix: `Tests 2 failed | 6 passed (8)`. The two failures are exactly the two new cases. The assertion error is `AssertionError: expected 'null' to be 'http://runtime-host.example.test:3102'`. - After the fix: `Tests 8 passed (8)`. - Both new cases are pure additions. No existing expectation changes, because no existing test uses a scheme-less `authPublicBaseUrl`. - **Standalone reproduction with 8 arms** (attached to #13576): **6/8** on unpatched `c65fc9e3`, and **8/8** with the patch. The 5 regression arms and the 1 control arm give the same result on both sides. The control arm uses input that truly fails to parse. Thus the reproduction discriminates between the two builds. It does not pass by accident. - **Typecheck**: `tsc --noEmit --strict --noEmitOnError --target ES2023 --module NodeNext` exits 0 on the patched file. It also exits 0 on the unmodified file. ## Risks **Low risk.** Reasons: - The change only affects input that makes the server crash today. No operator can depend on the present behaviour, because the present behaviour stops the boot. - The new path is not new code. The helper returns `null`, and `null` goes to the fallback that already exists for input that fails to parse. - The second call site previously put the string `"null"` into the candidate list. It now omits that entry. A `"null"` origin is not reachable, so no caller can lose a usable candidate. **The one behaviour change a reviewer must agree with.** A bad value now degrades quietly instead of failing loudly. An operator who writes `host:port` gets a server that starts on a derived origin. The operator does not get an error. I did not add a warning log, for one reason: the existing parse-failure path is also silent. A log would change behaviour that this pull request does not target, and it would enlarge the diff. If the maintainers want the warning, the correct fix is one log line inside `parsePublicOrigin`, which covers both input classes at once. This is now possible only because the two call sites share one function. Tell me if you want that, and I will add it. **Migration:** none. No schema change, no data change, no configuration change. ## Model Used - Provider and model: Anthropic Claude, Opus 5 - Exact model ID: `claude-opus-5[1m]` - Context window: 1M tokens - Mode: extended thinking - Capabilities used: tool use, shell execution, file editing, and GitHub API access - The human operator reviewed and authorized this submission. A human approved the publication of this pull request before I opened it. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work — I read the file. It has no item for the runtime API URL, for startup URL validation, or for configuration parsing. - [x] I have searched GitHub for duplicate or related PRs and linked them above — 5 queries, and I read the diff of each close neighbour. See the linked-issues section. - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details — the branch is `fix/scheme-less-public-base-url` - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes — no user-facing document describes this internal fallback, so no document change applies. - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green — not yet true, and I do not check a box that is not true. On commit `13ffb649`: 43 checks pass, 7 checks still run, 2 checks fail. Both failures are the PR-description gate that this update repairs. I will check this box when the gates are green. - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups — not yet true. Greptile gave 4/5 with one P2. That P2 is "Required PR sections missing", which this update repairs. - [x] I will address all Greptile and reviewer comments before requesting merge — this update is that work for the first review round. --------- Co-authored-by: Paperclip --- server/src/__tests__/runtime-api.test.ts | 23 +++++++++++++++ server/src/runtime-api.ts | 37 +++++++++++++----------- 2 files changed, 43 insertions(+), 17 deletions(-) diff --git a/server/src/__tests__/runtime-api.test.ts b/server/src/__tests__/runtime-api.test.ts index 5e46bbb06fa..6640af4d000 100644 --- a/server/src/__tests__/runtime-api.test.ts +++ b/server/src/__tests__/runtime-api.test.ts @@ -155,4 +155,27 @@ describe("runtime API discovery", () => { "fd7a:115c:a1e0::8a3a:a11d", ]); }); + + it("ignores a public base URL that has no scheme instead of returning the string \"null\"", () => { + expect( + choosePrimaryRuntimeApiUrl({ + authPublicBaseUrl: "runtime-host.example.test:3100", + allowedHostnames: ["runtime-host.example.test"], + bindHost: "0.0.0.0", + port: 3102, + }), + ).toBe("http://runtime-host.example.test:3102"); + }); + + it("builds candidates without throwing when the public base URL has no scheme", () => { + expect( + buildRuntimeApiCandidateUrls({ + authPublicBaseUrl: "runtime-host.example.test:3100", + allowedHostnames: ["runtime-host.example.test"], + bindHost: "0.0.0.0", + port: 3102, + networkInterfacesMap: {}, + }), + ).toEqual(["http://runtime-host.example.test:3102"]); + }); }); diff --git a/server/src/runtime-api.ts b/server/src/runtime-api.ts index bf4caf36eb7..6efa34ca14c 100644 --- a/server/src/runtime-api.ts +++ b/server/src/runtime-api.ts @@ -46,20 +46,31 @@ function pushCandidate( } } +// A base URL written as `host:port` (no `//`) is a VALID URL with an opaque +// scheme, and the origin of an opaque-scheme URL serializes to the *string* +// "null". That string is truthy, so it escapes every `if (origin)` guard and is +// then handed to `new URL(origin)`, which throws `TypeError: Invalid URL`. +// Treat it as absent config, exactly like input that fails to parse at all. +function parsePublicOrigin(rawUrl: string | null | undefined): string | null { + const trimmed = rawUrl?.trim(); + if (!trimmed) return null; + let origin: string; + try { + origin = new URL(trimmed).origin; + } catch { + return null; + } + return origin === "null" ? null : origin; +} + export function choosePrimaryRuntimeApiUrl(input: { authPublicBaseUrl?: string | null; allowedHostnames: string[]; bindHost: string; port: number; }): string { - const explicitPublicBaseUrl = input.authPublicBaseUrl?.trim(); - if (explicitPublicBaseUrl) { - try { - return new URL(explicitPublicBaseUrl).origin; - } catch { - // Fall through to derived candidates if config parsing drifted. - } - } + const explicitOrigin = parsePublicOrigin(input.authPublicBaseUrl); + if (explicitOrigin) return explicitOrigin; const bindHost = normalizeHost(input.bindHost); if (bindHost && !isWildcardHost(bindHost) && isLoopbackHost(bindHost)) { @@ -118,15 +129,7 @@ export function buildRuntimeApiCandidateUrls(input: { }): string[] { const candidates: string[] = []; const seen = new Set(); - const explicitPublicBaseUrl = input.authPublicBaseUrl?.trim() ?? ""; - const explicitOrigin = (() => { - if (!explicitPublicBaseUrl) return null; - try { - return new URL(explicitPublicBaseUrl).origin; - } catch { - return null; - } - })(); + const explicitOrigin = parsePublicOrigin(input.authPublicBaseUrl); const protocol = explicitOrigin ? new URL(explicitOrigin).protocol : "http:"; pushCandidate(candidates, seen, input.preferredApiUrl);