Skip to content

Commit 5aa5122

Browse files
RhysSullivanRhysSullivan
committed
Scope the insecure-origin warning and document the trusted-origins knob
The Secure-cookie warning fired whenever any trusted origin was http, which includes the plain http://localhost default, so every local boot printed it. Warn only for the mixed case an operator opts into: an http alias alongside an https canonical URL, where the canonical origin really does lose Secure cookies. Move the resolver below loadConfig with the other env knobs and state why each rejected origin shape is refused rather than trimmed. Cover the rejected shapes as a table, plus the blank-list and bare-hostname cases.
1 parent 019ba8f commit 5aa5122

3 files changed

Lines changed: 99 additions & 45 deletions

File tree

‎apps/host-selfhost/src/auth/better-auth.ts‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -65,13 +65,25 @@ let warnedInsecureTrustedOrigin = false;
6565

6666
const makeAuthOptions = (client: Client, getOrganizationId: () => string, gate?: SignupGate) => {
6767
const config = loadConfig();
68-
const hasInsecureTrustedOrigin = config.trustedOrigins.some(
69-
(origin) => new URL(origin).protocol === "http:",
68+
// A `Secure` session cookie is never sent back over plain HTTP, so an HTTP
69+
// alias can sign in and then look signed out on every later request. Drop the
70+
// attribute when ANY trusted origin is HTTP. This is not a new relaxation for
71+
// the common cases: Better Auth already infers `useSecureCookies` from the
72+
// baseURL scheme, so an all-HTTPS instance still gets `true` and the plain
73+
// `http://localhost` default still gets `false`. It only changes the mixed
74+
// case an operator opts into with EXECUTOR_TRUSTED_ORIGINS.
75+
const hasInsecureTrustedOrigin = config.trustedOrigins.some((origin) =>
76+
origin.startsWith("http://"),
7077
);
71-
if (hasInsecureTrustedOrigin && !warnedInsecureTrustedOrigin) {
78+
// Warn only for that mixed case. An HTTP-only instance (local dev, a LAN
79+
// deploy) never had Secure cookies to lose, and warning there would fire on
80+
// every default boot.
81+
const downgradesCanonicalCookies =
82+
hasInsecureTrustedOrigin && config.webBaseUrl.startsWith("https://");
83+
if (downgradesCanonicalCookies && !warnedInsecureTrustedOrigin) {
7284
warnedInsecureTrustedOrigin = true;
7385
console.warn(
74-
"[executor] HTTP trusted origins require session cookies without the Secure attribute. Use HTTPS-only origins to keep session cookies transport-secure.",
86+
"[executor] EXECUTOR_TRUSTED_ORIGINS contains an http:// origin, so session cookies drop the Secure attribute for every origin — including the https:// canonical URL. Use https:// aliases to keep session cookies transport-secure.",
7587
);
7688
}
7789
// Always resolved (generated + persisted when no env is set); this guards only

‎apps/host-selfhost/src/auth/origin-resolution.test.ts‎

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -76,14 +76,39 @@ test("additional trusted origins are trimmed, normalized, and deduplicated", ()
7676
]);
7777
});
7878

79-
test("additional trusted origins must be exact http(s) origins", () => {
79+
test("an empty or blank trusted-origins list leaves the canonical origin alone", () => {
8080
resetOriginEnv();
81-
process.env.EXECUTOR_TRUSTED_ORIGINS = "https://executor.example.com/login";
82-
expect(() => loadConfig()).toThrow(/exact http\(s\) origin/);
83-
84-
process.env.EXECUTOR_TRUSTED_ORIGINS = "file:///tmp/executor";
85-
expect(() => loadConfig()).toThrow(/exact http\(s\) origin/);
81+
process.env.EXECUTOR_WEB_BASE_URL = "https://executor.example.com";
82+
process.env.EXECUTOR_TRUSTED_ORIGINS = " , , ";
83+
expect(loadConfig().trustedOrigins).toEqual(["https://executor.example.com"]);
84+
});
8685

87-
process.env.EXECUTOR_TRUSTED_ORIGINS = "https://*.example.com";
86+
// Every rejected shape is one an operator could plausibly type and then believe
87+
// was in force. A wildcard host is the dangerous one: accepting it as a literal
88+
// hostname would silently allow nothing while reading like it allows a whole
89+
// domain. A path/query/fragment reads like a scoped grant that origins cannot
90+
// express, and credentials in the URL are almost always a copy-paste mistake.
91+
test.each([
92+
"https://executor.example.com/login",
93+
"https://executor.example.com/?next=/",
94+
"https://executor.example.com/#top",
95+
"https://user:pass@executor.example.com",
96+
"https://*.example.com",
97+
"file:///tmp/executor",
98+
"ftp://executor.example.com",
99+
])("a trusted origin that is not an exact http(s) origin (%s) refuses to boot", (raw) => {
100+
resetOriginEnv();
101+
process.env.EXECUTOR_TRUSTED_ORIGINS = raw;
88102
expect(() => loadConfig()).toThrow(/exact http\(s\) origin/);
89103
});
104+
105+
// A bare hostname is the most common typo, and it never parses as a URL at all,
106+
// so it gets the other message. Both name the variable.
107+
test.each(["executor.example.com", "//executor.example.com", "not a url"])(
108+
"a trusted origin that is not a URL (%s) refuses to boot",
109+
(raw) => {
110+
resetOriginEnv();
111+
process.env.EXECUTOR_TRUSTED_ORIGINS = raw;
112+
expect(() => loadConfig()).toThrow(/EXECUTOR_TRUSTED_ORIGINS/);
113+
},
114+
);

‎apps/host-selfhost/src/config.ts‎

Lines changed: 51 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -154,40 +154,6 @@ const resolveWebBaseUrl = (port: number): string => {
154154
return fallback;
155155
};
156156

157-
const normalizeTrustedOrigin = (value: string): string => {
158-
if (!URL.canParse(value)) {
159-
// oxlint-disable-next-line executor/no-try-catch-or-throw, executor/no-error-constructor -- boundary: invalid operator configuration must fail at boot instead of silently weakening or breaking origin checks
160-
throw new Error(
161-
`EXECUTOR_TRUSTED_ORIGINS contains ${JSON.stringify(value)}, which is not a valid URL origin`,
162-
);
163-
}
164-
const url = new URL(value);
165-
if (
166-
(url.protocol !== "http:" && url.protocol !== "https:") ||
167-
url.username.length > 0 ||
168-
url.password.length > 0 ||
169-
url.hostname.includes("*") ||
170-
(url.pathname !== "" && url.pathname !== "/") ||
171-
url.search.length > 0 ||
172-
url.hash.length > 0
173-
) {
174-
// oxlint-disable-next-line executor/no-try-catch-or-throw, executor/no-error-constructor -- boundary: invalid operator configuration must fail at boot instead of silently weakening or breaking origin checks
175-
throw new Error(
176-
`EXECUTOR_TRUSTED_ORIGINS entry ${JSON.stringify(value)} must be an exact http(s) origin (scheme, host, and optional port only)`,
177-
);
178-
}
179-
return url.origin;
180-
};
181-
182-
const resolveTrustedOrigins = (webBaseUrl: string): readonly string[] => {
183-
const additional = (process.env.EXECUTOR_TRUSTED_ORIGINS ?? "")
184-
.split(",")
185-
.map((value) => value.trim())
186-
.filter((value) => value.length > 0)
187-
.map(normalizeTrustedOrigin);
188-
return [...new Set([webBaseUrl, ...additional])];
189-
};
190-
191157
export const loadConfig = (): SelfHostConfig => {
192158
const port = Number.parseInt(process.env.PORT ?? "4788", 10);
193159
const dataDir = resolveDataDir();
@@ -244,6 +210,57 @@ const resolveMcpSessionIdleTtlMs = (): number | undefined => {
244210
return Math.floor(parsed);
245211
};
246212

213+
// EXECUTOR_TRUSTED_ORIGINS — extra browser origins allowed to send
214+
// cookie-authenticated requests when one instance is deliberately reachable
215+
// under more than one address (a LAN IP as well as a domain, say).
216+
//
217+
// This list widens ONLY Better Auth's origin/CSRF check. `webBaseUrl` stays the
218+
// single canonical origin for OAuth callbacks, MCP metadata, and every other
219+
// generated link, so an alias can never redirect a callback somewhere else.
220+
//
221+
// Entries must be exact origins. A path, query, fragment, credential, wildcard
222+
// host, or non-http(s) scheme is refused rather than trimmed off: an operator
223+
// who writes `https://*.example.com` means a pattern, and silently accepting it
224+
// as the literal host would leave them believing a wildcard is in force. Like
225+
// the other knobs here, a malformed value refuses to boot instead of quietly
226+
// leaving the browser locked out with an "Invalid origin" page.
227+
const normalizeTrustedOrigin = (value: string): string => {
228+
if (!URL.canParse(value)) {
229+
// oxlint-disable-next-line executor/no-try-catch-or-throw, executor/no-error-constructor -- boundary: refuse to boot on a malformed operator knob
230+
throw new Error(
231+
`EXECUTOR_TRUSTED_ORIGINS contains ${JSON.stringify(value)}, which is not a valid URL origin`,
232+
);
233+
}
234+
const url = new URL(value);
235+
if (
236+
(url.protocol !== "http:" && url.protocol !== "https:") ||
237+
url.username.length > 0 ||
238+
url.password.length > 0 ||
239+
url.hostname.includes("*") ||
240+
(url.pathname !== "" && url.pathname !== "/") ||
241+
url.search.length > 0 ||
242+
url.hash.length > 0
243+
) {
244+
// oxlint-disable-next-line executor/no-try-catch-or-throw, executor/no-error-constructor -- boundary: refuse to boot on a malformed operator knob
245+
throw new Error(
246+
`EXECUTOR_TRUSTED_ORIGINS entry ${JSON.stringify(value)} must be an exact http(s) origin (scheme, host, and optional port only)`,
247+
);
248+
}
249+
return url.origin;
250+
};
251+
252+
// The canonical origin always leads the list, so the unset case reproduces the
253+
// previous `[webBaseUrl]` exactly and an operator who repeats it in the env var
254+
// does not get a duplicate.
255+
const resolveTrustedOrigins = (webBaseUrl: string): readonly string[] => {
256+
const additional = (process.env.EXECUTOR_TRUSTED_ORIGINS ?? "")
257+
.split(",")
258+
.map((value) => value.trim())
259+
.filter((value) => value.length > 0)
260+
.map(normalizeTrustedOrigin);
261+
return [...new Set([webBaseUrl, ...additional])];
262+
};
263+
247264
// The org slug doubles as a URL segment (`/<slug>/policies`), so an
248265
// operator-set value must fit the shared grammar and avoid reserved root
249266
// segments (api, mcp, login, …) — a colliding slug would shadow real routes.

0 commit comments

Comments
 (0)