From dd2be8c8b37ea58911d09a7b5479c5f0a36f22ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Mon, 10 Aug 2026 19:12:06 +0000 Subject: [PATCH] app store: never assume a service is local, or on its usual port MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An instance may be on another host, behind a reverse proxy on 443 under a path prefix, on a tailnet address, or on an arbitrary port because the usual one was taken. All ordinary self-hosted setups, and each one a case where assuming otherwise produces a connection that fails later with no clue why. Two places were sloppy about it. Jellyfin's placeholder read `http://localhost:8096` and Transmission's `http://localhost:9091`, which quietly teach that a service must be local and on its project's default port; both now show remote examples, and the field type says why. And nothing validated what was typed, so a bare hostname or a URL with a token in the query string was stored as-is. The rule: reject only what cannot work, normalise what is merely untidy, have no opinion about the rest. No check that the host is local, that the port matches a default, or that the scheme is https — a tailnet HTTP service is completely normal. Trailing slashes go, because `${url}/api/x` otherwise doubles the separator: accepted by some servers and 404 by others, which is the kind of difference that reproduces on one machine and not another. Query strings go, because that is where a token hides, and it would sit in a column meant for a location. Credentials in the URL are refused for the same reason — outside the encrypted secret, and in every log line that ever prints it. A missing scheme is named rather than called invalid: it is the commonest mistake, because it is what people type into a browser. Verified through the API: `memos.example.com` is blocked with the fix quoted back, and `https://memos.example.com:8443/memos/` installs and stores normalised — remote host, non-standard port, path prefix. Co-Authored-By: Claude Opus 5 --- src/servers/app-store/catalogue.ts | 18 ++++++-- src/servers/app-store/effects.ts | 12 ++++++ src/servers/app-store/url.test.ts | 66 ++++++++++++++++++++++++++++++ src/servers/app-store/url.ts | 58 ++++++++++++++++++++++++++ 4 files changed, 151 insertions(+), 3 deletions(-) create mode 100644 src/servers/app-store/url.test.ts create mode 100644 src/servers/app-store/url.ts diff --git a/src/servers/app-store/catalogue.ts b/src/servers/app-store/catalogue.ts index 09362ac6..7c0ec455 100644 --- a/src/servers/app-store/catalogue.ts +++ b/src/servers/app-store/catalogue.ts @@ -25,7 +25,13 @@ export type InstallMode = /** Nothing to reach. Credentials or local configuration only. */ | 'config'; -/** One field the installer asks for before it can finish. */ +/** + * One field the installer asks for before it can finish. + * + * A `url` placeholder should show a REMOTE example. The instance may be on another host, behind a + * reverse proxy, on a non-standard port, or all three — a placeholder reading `http://localhost:8096` + * quietly teaches that it must be local and on the usual port, which is wrong often enough to matter. + */ export type ConfigField = { key: string; label: string; @@ -177,7 +183,7 @@ export const CATALOGUE: CatalogueEntry[] = [ capability: 'jellyfin', composeTemplate: 'jellyfin', existingFields: [ - { key: 'url', label: 'Jellyfin URL', type: 'url', required: true, placeholder: 'http://localhost:8096' }, + { key: 'url', label: 'Jellyfin URL', type: 'url', required: true, placeholder: 'https://jellyfin.example.com' }, { key: 'secret', label: 'Access token', type: 'secret', required: true }, ], }, @@ -249,7 +255,13 @@ export const CATALOGUE: CatalogueEntry[] = [ capability: 'transmission', composeTemplate: 'transmission', existingFields: [ - { key: 'url', label: 'Transmission URL', type: 'url', required: true, placeholder: 'http://localhost:9091' }, + { + key: 'url', + label: 'Transmission URL', + type: 'url', + required: true, + placeholder: 'https://transmission.example.com or http://10.0.0.5:9091', + }, { key: 'path', label: 'RPC path', diff --git a/src/servers/app-store/effects.ts b/src/servers/app-store/effects.ts index 4649d042..d8edc10d 100644 --- a/src/servers/app-store/effects.ts +++ b/src/servers/app-store/effects.ts @@ -6,6 +6,7 @@ import { runSetupScript } from './run-script'; import { startProcess } from './pm2'; import { serviceDir } from './paths'; import { publishAssets } from './assets'; +import { checkServiceUrl } from './url'; // The installer's steps, wired to the actual world. // @@ -41,6 +42,17 @@ export function createEffects(): InstallEffects { }, async connect(ctx: StepContext) { + // Validated and normalised before anything is stored. What the user typed may be a bare hostname, + // may carry a trailing slash that doubles a separator later, or may hide a token in a query + // string — and none of those are visible again once written. + const raw = ctx.values.url; + if (raw) { + const checked = checkServiceUrl(raw); + if (!checked.ok) { + return { status: 'blocked', reason: `${ctx.entry.label}: ${checked.error}` }; + } + ctx.values.url = checked.url; + } const url = ctx.values.url; // A provisioned install knows its own URL because the setup script printed it. An `existing` one // was given it in the form. Neither having produced one means we cannot reach the service, and diff --git a/src/servers/app-store/url.test.ts b/src/servers/app-store/url.test.ts new file mode 100644 index 00000000..b21d501e --- /dev/null +++ b/src/servers/app-store/url.test.ts @@ -0,0 +1,66 @@ +import { describe, expect, it } from 'bun:test'; +import { checkServiceUrl } from './url'; + +// The instance may be anywhere. These tests are mostly about what must NOT be rejected — every case +// below is an ordinary self-hosted setup, and treating any of them as invalid would send someone to +// the docs for no reason. + +describe('accepts wherever the service actually is', () => { + const ok = [ + 'https://photos.example.com', // remote, behind a proxy on 443 + 'http://10.0.0.5:8096', // another host on the LAN, standard port + 'http://100.64.0.1:19999', // tailnet address, arbitrary port + 'https://example.com/immich', // sharing a domain, served under a path + 'http://127.0.0.1:9091', // same machine, which is allowed — just not assumed + 'http://immich:2283', // a docker network hostname + ]; + + for (const url of ok) { + it(`accepts ${url}`, () => { + expect(checkServiceUrl(url).ok).toBe(true); + }); + } +}); + +describe('normalises what is untidy', () => { + it('strips a trailing slash', () => { + // `${url}/api/x` would otherwise double the separator — accepted by some servers, 404 by others, + // which is the kind of difference that reproduces on one machine and not another. + const result = checkServiceUrl('https://photos.example.com/'); + expect(result.ok && result.url).toBe('https://photos.example.com'); + }); + + it('drops a query string, which is where a token would hide', () => { + const result = checkServiceUrl('https://x.example.com/?token=secret'); + expect(result.ok && result.url).toBe('https://x.example.com'); + }); + + it('keeps a path, because a service under a prefix is normal', () => { + const result = checkServiceUrl('https://example.com/immich/'); + expect(result.ok && result.url).toBe('https://example.com/immich'); + }); +}); + +describe('rejects only what cannot work', () => { + it('names the missing scheme rather than saying "invalid"', () => { + // The commonest mistake: people type what they type into a browser. + const result = checkServiceUrl('photos.example.com'); + expect(result.ok).toBe(false); + if (result.ok) throw new Error('unreachable'); + expect(result.error).toContain('https://photos.example.com'); + }); + + it('refuses credentials in the URL', () => { + // They would land in a column meant for a location — outside the encrypted secret, and in every + // log line that ever prints the URL. + expect(checkServiceUrl('https://user:pass@x.example.com').ok).toBe(false); + }); + + it('refuses a scheme nothing here speaks', () => { + expect(checkServiceUrl('ftp://x.example.com').ok).toBe(false); + }); + + it('refuses empty', () => { + expect(checkServiceUrl(' ').ok).toBe(false); + }); +}); diff --git a/src/servers/app-store/url.ts b/src/servers/app-store/url.ts new file mode 100644 index 00000000..773ca047 --- /dev/null +++ b/src/servers/app-store/url.ts @@ -0,0 +1,58 @@ +// Checking and normalising a URL the user typed for a service they already run. +// +// ── What must never be assumed ── +// +// That the instance is on this machine, on its project's usual port, or reachable without a scheme. It +// may be on another host, behind a reverse proxy on 443 with a path, on a tailnet address, or on an +// arbitrary port because 8096 was already taken. Every one of those is an ordinary self-hosted setup, +// and each is a case where guessing produces a connection that fails later with no clue why. +// +// So the rule here is narrow: reject only what CANNOT work, normalise what is merely untidy, and accept +// everything else without opinion. In particular no check that the host is local, that the port matches +// the service's default, or that the scheme is https — a tailnet HTTP service is completely normal. + +export type UrlCheck = { ok: true; url: string } | { ok: false; error: string }; + +/** + * Validate and normalise. Returns the form to store, or why it cannot be stored. + * + * Normalising matters more than it looks: a trailing slash turns `${url}/api/x` into a double + * separator, which some servers accept and others 404 — the kind of difference that shows up on one + * user's machine and not another's. + */ +export function checkServiceUrl(raw: string): UrlCheck { + const trimmed = raw.trim(); + if (!trimmed) return { ok: false, error: 'A URL is required.' }; + + // The commonest mistake, and worth naming precisely rather than as "invalid URL": people type the + // host alone because that is what they type into a browser. + if (!/^[a-zA-Z][a-zA-Z0-9+.-]*:\/\//.test(trimmed)) { + return { ok: false, error: `Include the scheme — "https://${trimmed}" or "http://${trimmed}".` }; + } + + let parsed: URL; + try { + parsed = new URL(trimmed); + } catch { + return { ok: false, error: 'That is not a valid URL.' }; + } + + if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { + return { ok: false, error: `Only http and https are supported, not "${parsed.protocol.replace(':', '')}".` }; + } + if (!parsed.hostname) return { ok: false, error: 'That URL has no host.' }; + + // Credentials in the URL are dropped rather than stored: they would end up in a database column meant + // for a location, outside the encrypted `secret`, and in any log line that ever prints the URL. + if (parsed.username || parsed.password) { + return { + ok: false, + error: 'Put the username and password in their own fields rather than in the URL.', + }; + } + + // Trailing slash off, query and fragment dropped — neither means anything for a service base, and a + // stored `?token=…` would be a credential hiding in the wrong column. + const path = parsed.pathname.replace(/\/+$/, ''); + return { ok: true, url: `${parsed.protocol}//${parsed.host}${path}` }; +}