Skip to content

Commit 9d4d61d

Browse files
committed
feat(integrations): teach externalUrl to handle path-only bases
Address review comment from @Meierschlumpf on PR homarr-labs#5595 about the integration-middleware externalUrl pass-through. Reverting middleware to `app?.href ?? null` would have crashed `new URL` inside `createUrl` for path-only bases (e.g. "/cockpit/"). This commit makes path-only externalUrl a first-class state instead. - packages/integrations/src/base/integration.ts: new RenderablePath class mirrors just enough of the WHATWG URL surface (toString, pathname, hostname, searchParams) for the existing externalUrl caller set. The `externalUrl` method branches on `startsWith("/")` and returns a RenderablePath for path-only bases or a URL for absolute bases (existing behavior). All 19 caller sites continue to work unchanged: 17 immediately call `.toString()` and the two outliers (ntfy reads `.hostname` and `.pathname` for a fallback title; overseerr returns the value up to a `.toString()` caller in constructAvatarUrl) keep behaving sensibly for path-only. - packages/api/src/middlewares/integration.ts: restore `externalUrl: rest.app?.href ?? null` at both call sites per the reviewer's suggestion. Path-only hrefs now flow through to integrations untouched, where the new RenderablePath branch in createUrl handles them. The same fix automatically resolves the pre-existing pattern in packages/request-handler/src/lib/cached-request-integration-job-handler.ts:97 where externalUrl is built the same way. - packages/integrations/test/base.spec.ts: six new tests covering absolute and path-only externalUrl, query-param merging in both, path-embedded query strings combined with extra query-params for path-only, and the null-externalUrl fallback to integration.url. Net effect: integrations bound to path-only-href apps render their hrefs against the user's current origin (e.g. Jellyfin "Watch movie" links resolve as /signalk-server/... instead of the internal http://signalk:3000/...). Materially better for the multi-hostname use case this PR is built for. resolveServerUrl stays in packages/api/src/router/widgets/app.ts for the ping endpoint - the server genuinely cannot ping a hostless URL, so CONFLICT is still the right signal there. The middleware case is different because the value is rendered, not fetched.
1 parent d7bcb27 commit 9d4d61d

3 files changed

Lines changed: 119 additions & 7 deletions

File tree

packages/api/src/middlewares/integration.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import { z } from "zod/v4";
44
import type { Session } from "@homarr/auth";
55
import { hasQueryAccessToIntegrationsAsync } from "@homarr/auth/server";
66
import { constructIntegrationPermissions } from "@homarr/auth/shared";
7-
import { resolveServerUrl } from "@homarr/common";
87
import { decryptSecret } from "@homarr/common/server";
98
import type { AtLeastOneOf } from "@homarr/common/types";
109
import type { Database } from "@homarr/db";
@@ -67,7 +66,7 @@ export const createOneIntegrationMiddleware = <TKind extends IntegrationKind>(
6766
ctx: {
6867
integration: {
6968
...rest,
70-
externalUrl: rest.app ? resolveServerUrl(rest.app) : null,
69+
externalUrl: rest.app?.href ?? null,
7170
kind: kind as TKind,
7271
decryptedSecrets: secrets.map((secret) => ({
7372
...secret,
@@ -129,7 +128,7 @@ export const createManyIntegrationMiddleware = <TKind extends IntegrationKind>(
129128
integrations: dbIntegrations.map(
130129
({ secrets, kind, items: _ignore1, groupPermissions: _ignore2, userPermissions: _ignore3, ...rest }) => ({
131130
...rest,
132-
externalUrl: rest.app ? resolveServerUrl(rest.app) : null,
131+
externalUrl: rest.app?.href ?? null,
133132
kind: kind as TKind,
134133
decryptedSecrets: secrets.map((secret) => ({
135134
...secret,

packages/integrations/src/base/integration.ts

Lines changed: 64 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,15 @@ export abstract class Integration {
8181
protected externalUrl(
8282
path: `/${string}`,
8383
queryParams?: Record<string, string | Date | number | boolean | null | undefined>,
84-
) {
85-
return this.createUrl(this.integration.externalUrl ?? this.integration.url, path, queryParams);
84+
): URL | RenderablePath {
85+
const base = this.integration.externalUrl ?? this.integration.url;
86+
// Path-only externalUrl (e.g. "/cockpit/") cannot be parsed by `new URL`,
87+
// but the rendered href ends up on the client where the browser resolves
88+
// it against the current origin. Build a hostless RenderablePath instead.
89+
if (base.startsWith("/")) {
90+
return new RenderablePath(base, path, queryParams);
91+
}
92+
return this.createUrl(base, path, queryParams);
8693
}
8794

8895
public async testConnectionAsync(): Promise<TestingResult> {
@@ -125,3 +132,58 @@ export abstract class Integration {
125132
*/
126133
protected abstract testingAsync(input: IntegrationTestingInput): Promise<TestingResult>;
127134
}
135+
136+
/**
137+
* URL-shaped wrapper for path-only externalUrl bases (e.g. "/cockpit/").
138+
*
139+
* Path-only externalUrl is meaningful only on the client, where the browser
140+
* resolves it against the current origin. The integration helpers can't use
141+
* `new URL` server-side because there's no host to parse. RenderablePath
142+
* mirrors just enough of the `URL` surface — `toString()`, `pathname`,
143+
* `hostname`, `searchParams` — to keep the 19 caller sites of
144+
* `super.externalUrl(...)` working unchanged. `hostname` is always the empty
145+
* string for a path-only base; `pathname` is the joined base + path.
146+
*/
147+
export class RenderablePath {
148+
public readonly searchParams: URLSearchParams;
149+
private readonly pathOnly: string;
150+
151+
constructor(
152+
base: string,
153+
path: `/${string}`,
154+
queryParams?: Record<string, string | Date | number | boolean | null | undefined>,
155+
) {
156+
const trimmedBase = removeTrailingSlash(base);
157+
const combined = `${trimmedBase}${path}`;
158+
const queryIndex = combined.indexOf("?");
159+
if (queryIndex === -1) {
160+
this.pathOnly = combined;
161+
this.searchParams = new URLSearchParams();
162+
} else {
163+
this.pathOnly = combined.substring(0, queryIndex);
164+
this.searchParams = new URLSearchParams(combined.substring(queryIndex + 1));
165+
}
166+
167+
if (queryParams) {
168+
for (const [key, value] of Object.entries(queryParams)) {
169+
if (value === null || value === undefined) {
170+
continue;
171+
}
172+
this.searchParams.set(key, value instanceof Date ? value.toISOString() : value.toString());
173+
}
174+
}
175+
}
176+
177+
public get hostname(): string {
178+
return "";
179+
}
180+
181+
public get pathname(): string {
182+
return this.pathOnly;
183+
}
184+
185+
public toString(): string {
186+
const query = this.searchParams.toString();
187+
return query ? `${this.pathOnly}?${query}` : this.pathOnly;
188+
}
189+
}

packages/integrations/test/base.spec.ts

Lines changed: 53 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { ResponseError } from "@homarr/common/server";
44
import { createDb } from "@homarr/db/test";
55

66
import type { IntegrationTestingInput } from "../src/base/integration";
7-
import { Integration } from "../src/base/integration";
7+
import { Integration, RenderablePath } from "../src/base/integration";
88
import type { TestingResult } from "../src/base/test-connection/test-connection-service";
99

1010
vi.mock("@homarr/db", async (importActual) => {
@@ -42,22 +42,73 @@ describe("Base integration", () => {
4242
expect(result.error.data.url).toContain("https://example.com");
4343
expect(result.error.data.reason).toBe("internalServerError");
4444
});
45+
46+
describe("externalUrl", () => {
47+
test("returns a URL for an absolute externalUrl", () => {
48+
const integration = new FakeIntegration(undefined, undefined, "https://example.com");
49+
const result = integration.callExternalUrl("/items/42");
50+
expect(result).toBeInstanceOf(URL);
51+
expect(result.toString()).toBe("https://example.com/items/42");
52+
});
53+
54+
test("merges queryParams onto an absolute externalUrl", () => {
55+
const integration = new FakeIntegration(undefined, undefined, "https://example.com");
56+
const result = integration.callExternalUrl("/items", { id: "42", since: new Date("2026-01-01T00:00:00Z") });
57+
expect(result.toString()).toBe("https://example.com/items?id=42&since=2026-01-01T00%3A00%3A00.000Z");
58+
});
59+
60+
test("returns a RenderablePath for a path-only externalUrl", () => {
61+
const integration = new FakeIntegration(undefined, undefined, "/cockpit/");
62+
const result = integration.callExternalUrl("/web/index.html");
63+
expect(result).toBeInstanceOf(RenderablePath);
64+
expect(result.toString()).toBe("/cockpit/web/index.html");
65+
expect(result.pathname).toBe("/cockpit/web/index.html");
66+
expect(result.hostname).toBe("");
67+
});
68+
69+
test("merges queryParams onto a path-only externalUrl", () => {
70+
const integration = new FakeIntegration(undefined, undefined, "/cockpit/");
71+
const result = integration.callExternalUrl("/web/index.html", { id: "42" });
72+
expect(result.toString()).toBe("/cockpit/web/index.html?id=42");
73+
});
74+
75+
test("merges path-embedded query with extra queryParams on a path-only externalUrl", () => {
76+
const integration = new FakeIntegration(undefined, undefined, "/signalk-server/");
77+
const result = integration.callExternalUrl("/items/42?width=100", { quality: "90" });
78+
expect(result.pathname).toBe("/signalk-server/items/42");
79+
expect(result.toString()).toBe("/signalk-server/items/42?width=100&quality=90");
80+
});
81+
82+
test("falls back to integration.url when externalUrl is null and the integration url is absolute", () => {
83+
const integration = new FakeIntegration(undefined, undefined, null);
84+
const result = integration.callExternalUrl("/items/42");
85+
expect(result.toString()).toBe("https://example.com/items/42");
86+
});
87+
});
4588
});
4689

4790
class FakeIntegration extends Integration {
4891
constructor(
4992
private testingResult?: TestingResult,
5093
private error?: Error,
94+
externalUrl: string | null = null,
5195
) {
5296
super({
5397
id: "test",
5498
name: "Test",
5599
url: "https://example.com",
56100
decryptedSecrets: [],
57-
externalUrl: null,
101+
externalUrl,
58102
});
59103
}
60104

105+
public callExternalUrl(
106+
path: `/${string}`,
107+
queryParams?: Record<string, string | Date | number | boolean | null | undefined>,
108+
) {
109+
return this.externalUrl(path, queryParams);
110+
}
111+
61112
// eslint-disable-next-line no-restricted-syntax
62113
protected testingAsync(_: IntegrationTestingInput): Promise<TestingResult> {
63114
if (this.error) {

0 commit comments

Comments
 (0)