Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 43 additions & 1 deletion packages/api/src/router/test/widgets/app.spec.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { TRPCError } from "@trpc/server";
import { describe, expect, test, vi } from "vitest";

import type { Session } from "@homarr/auth";
Expand Down Expand Up @@ -67,10 +68,51 @@ describe("ping should call sendPingRequestAsync with url and return result", ()
});
});

const createApp = ({ url }: { url: string }) =>
const createApp = ({ url, pingUrl }: { url: string | null; pingUrl?: string | null }) =>
({
id: createId(),
iconUrl: "",
name: "Test App",
href: url,
pingUrl: pingUrl ?? null,
}) satisfies InferInsertModel<typeof apps>;

describe("ping resolution under path-only hrefs", () => {
test("path-only href without pingUrl throws CONFLICT (no server-side host synthesis)", async () => {
const sendSpy = vi.spyOn(ping, "sendPingRequestAsync");
const db = createDb();
const app = createApp({ url: "/cockpit/" });
await db.insert(apps).values(app);
const caller = appRouter.createCaller({ db, deviceType: undefined, session: null });

await expect(caller.ping({ id: app.id })).rejects.toThrow(TRPCError);
await expect(caller.ping({ id: app.id })).rejects.toMatchObject({ code: "CONFLICT" });
expect(sendSpy).not.toHaveBeenCalled();
});

test("path-only href with explicit pingUrl uses pingUrl", async () => {
const expectedUrl = "https://host.docker.internal/cockpit/";
vi.spyOn(ping, "sendPingRequestAsync").mockResolvedValueOnce({ statusCode: 204, durationMs: 7 });
const db = createDb();
const app = createApp({ url: "/cockpit/", pingUrl: expectedUrl });
await db.insert(apps).values(app);
const caller = appRouter.createCaller({ db, deviceType: undefined, session: null });

const result = await caller.ping({ id: app.id });

expect(result.url).toBe(expectedUrl);
});

test("absolute href without pingUrl pings the href", async () => {
const expectedUrl = "https://docs.halos.fi";
vi.spyOn(ping, "sendPingRequestAsync").mockResolvedValueOnce({ statusCode: 200, durationMs: 11 });
const db = createDb();
const app = createApp({ url: expectedUrl });
await db.insert(apps).values(app);
const caller = appRouter.createCaller({ db, deviceType: undefined, session: null });

const result = await caller.ping({ id: app.id });

expect(result.url).toBe(expectedUrl);
});
});
5 changes: 3 additions & 2 deletions packages/api/src/router/widgets/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { TRPCError } from "@trpc/server";
import { observable } from "@trpc/server/observable";
import { z } from "zod/v4";

import { resolveServerUrl } from "@homarr/common";
import { getServerSettingByKeyAsync } from "@homarr/db/queries";
import { sendPingRequestAsync } from "@homarr/ping";
import { pingChannel, pingUrlChannel } from "@homarr/redis";
Expand Down Expand Up @@ -29,7 +30,7 @@ export const appRouter = createTRPCRouter({
});
}

const pingUrl = app.pingUrl ?? app.href;
const pingUrl = resolveServerUrl(app);

if (!pingUrl) {
throw new TRPCError({
Expand Down Expand Up @@ -70,7 +71,7 @@ export const appRouter = createTRPCRouter({
});
}

const pingUrl = app.pingUrl ?? app.href;
const pingUrl = resolveServerUrl(app);

if (!pingUrl) {
throw new TRPCError({
Expand Down
32 changes: 31 additions & 1 deletion packages/common/src/test/url.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { describe, expect, test } from "vitest";

import { getPortFromUrl, isAbsoluteUrl } from "../url.js";
import { getPortFromUrl, isAbsoluteUrl, resolveServerUrl } from "../url.js";

describe("getPortFromUrl", () => {
test.each([
Expand Down Expand Up @@ -56,3 +56,33 @@ describe("isAbsoluteUrl", () => {
expect(result).toBe(expected);
});
});

describe("resolveServerUrl", () => {
test("returns explicit pingUrl when set", () => {
expect(resolveServerUrl({ pingUrl: "http://x.local/ping", href: "/anything/" })).toBe("http://x.local/ping");
});

test("returns absolute href as-is when no pingUrl", () => {
expect(resolveServerUrl({ pingUrl: null, href: "https://abs.example.com/x" })).toBe("https://abs.example.com/x");
});

test("returns null for path-only href (no server-side expansion)", () => {
expect(resolveServerUrl({ pingUrl: null, href: "/cockpit/" })).toBeNull();
});

test("returns null for schemeless relative href", () => {
expect(resolveServerUrl({ pingUrl: null, href: "relative/path" })).toBeNull();
});

test("returns null when both pingUrl and href are null", () => {
expect(resolveServerUrl({ pingUrl: null, href: null })).toBeNull();
});

test("path-only href with explicit pingUrl returns the pingUrl", () => {
// The HaLOS-shipped-card scenario: adapter sets explicit pingUrl while
// href stays path-only for browser-side multi-hostname resolution.
expect(resolveServerUrl({ pingUrl: "https://host.docker.internal/cockpit/", href: "/cockpit/" })).toBe(
"https://host.docker.internal/cockpit/",
);
});
});
24 changes: 24 additions & 0 deletions packages/common/src/url.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,27 @@ const absoluteUrlRegex = /^[a-z]+:(\/\/)?/;
export const isAbsoluteUrl = (urlOrPath: string): boolean => {
return absoluteUrlRegex.test(urlOrPath.toLowerCase());
};

/**
* Resolves an app to the absolute URL the server should use, or null.
* 1. explicit `pingUrl` -> as-is
* 2. absolute `href` -> as-is
* 3. non-absolute `href` -> null (path-only `/cockpit/`, schemeless `foo/bar`)
* 4. null/empty `href` -> null (short-circuits before the absoluteness check)
*
* Non-absolute hrefs are intentionally null server-side: synthesizing them
* from request headers would be a header-spoofing vector, and the browser
* already resolves them against the current origin natively. Apps that need
* server-side ping coverage should carry an explicit `pingUrl`.
*/
export const resolveServerUrl = (app: { href: string | null; pingUrl: string | null }): string | null => {
if (app.pingUrl) {
return app.pingUrl;
}

if (app.href && isAbsoluteUrl(app.href)) {
return app.href;
}

return null;
};
80 changes: 78 additions & 2 deletions packages/integrations/src/base/integration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,18 @@ export abstract class Integration {
protected externalUrl(
path: `/${string}`,
queryParams?: Record<string, string | Date | number | boolean | null | undefined>,
) {
return this.createUrl(this.integration.externalUrl ?? this.integration.url, path, queryParams);
): URL | RenderablePath {
const base = this.integration.externalUrl ?? this.integration.url;
// Path-only externalUrl (e.g. "/cockpit/") cannot be parsed by `new URL`,
// but the rendered href ends up on the client where the browser resolves
// it against the current origin. Build a hostless RenderablePath instead.
// Scheme-relative bases ("//host/...") are rejected at the schema layer
// (packages/validation/src/app.ts) but explicitly excluded here too so
// that a malformed value can't cross-origin-escape through this branch.
if (base.startsWith("/") && !base.startsWith("//")) {
return new RenderablePath(base, path, queryParams);
}
return this.createUrl(base, path, queryParams);
}

public async testConnectionAsync(): Promise<TestingResult> {
Expand Down Expand Up @@ -125,3 +135,69 @@ export abstract class Integration {
*/
protected abstract testingAsync(input: IntegrationTestingInput): Promise<TestingResult>;
}

/**
* URL-shaped wrapper for path-only externalUrl bases (e.g. "/cockpit/").
*
* Path-only externalUrl is meaningful only on the client, where the browser
* resolves it against the current origin. The integration helpers can't use
* `new URL` server-side because there's no host to parse. RenderablePath
* mirrors just enough of the `URL` surface — `toString()`, `pathname`,
* `hostname`, `searchParams` — to keep the 19 caller sites of
* `super.externalUrl(...)` working unchanged. `hostname` is always the empty
* string for a path-only base; `pathname` is the joined base + path.
*
* Fragment handling note: the constructor splits on the first `?` only, not
* `#`. Path arguments that contain a fragment (e.g. Jellyfin / Emby hash-bang
* routes like `/web/index.html#!/details?id=abc`) keep the fragment as part
* of `pathname`, and any `?` inside the hash-bang is treated as the query
* separator. This matches what Jellyfin / Emby SPA routers expect: the
* post-`?` params must stay inside the hash, not be hoisted before it. Don't
* combine fragment-containing paths with extra `queryParams` here — the
* merged params would land inside the hash too. WHATWG URL behaves
* differently (it places `.hash` after `.search`), but mirroring that would
* break the SPA-routing callers this method exists to serve.
*/
export class RenderablePath {
public readonly searchParams: URLSearchParams;
private readonly pathOnly: string;

constructor(
base: string,
path: `/${string}`,
queryParams?: Record<string, string | Date | number | boolean | null | undefined>,
) {
const trimmedBase = removeTrailingSlash(base);
const combined = `${trimmedBase}${path}`;
const queryIndex = combined.indexOf("?");
if (queryIndex === -1) {
this.pathOnly = combined;
this.searchParams = new URLSearchParams();
} else {
this.pathOnly = combined.substring(0, queryIndex);
this.searchParams = new URLSearchParams(combined.substring(queryIndex + 1));
}

if (queryParams) {
for (const [key, value] of Object.entries(queryParams)) {
if (value === null || value === undefined) {
continue;
}
this.searchParams.set(key, value instanceof Date ? value.toISOString() : value.toString());
}
}
}

public get hostname(): string {
return "";
}

public get pathname(): string {
return this.pathOnly;
}

public toString(): string {
const query = this.searchParams.toString();
return query ? `${this.pathOnly}?${query}` : this.pathOnly;
}
}
55 changes: 53 additions & 2 deletions packages/integrations/test/base.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { ResponseError } from "@homarr/common/server";
import { createDb } from "@homarr/db/test";

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

vi.mock("@homarr/db", async (importActual) => {
Expand Down Expand Up @@ -42,22 +42,73 @@ describe("Base integration", () => {
expect(result.error.data.url).toContain("https://example.com");
expect(result.error.data.reason).toBe("internalServerError");
});

describe("externalUrl", () => {
test("returns a URL for an absolute externalUrl", () => {
const integration = new FakeIntegration(undefined, undefined, "https://example.com");
const result = integration.callExternalUrl("/items/42");
expect(result).toBeInstanceOf(URL);
expect(result.toString()).toBe("https://example.com/items/42");
});

test("merges queryParams onto an absolute externalUrl", () => {
const integration = new FakeIntegration(undefined, undefined, "https://example.com");
const result = integration.callExternalUrl("/items", { id: "42", since: new Date("2026-01-01T00:00:00Z") });
expect(result.toString()).toBe("https://example.com/items?id=42&since=2026-01-01T00%3A00%3A00.000Z");
});

test("returns a RenderablePath for a path-only externalUrl", () => {
const integration = new FakeIntegration(undefined, undefined, "/cockpit/");
const result = integration.callExternalUrl("/web/index.html");
expect(result).toBeInstanceOf(RenderablePath);
expect(result.toString()).toBe("/cockpit/web/index.html");
expect(result.pathname).toBe("/cockpit/web/index.html");
expect(result.hostname).toBe("");
});

test("merges queryParams onto a path-only externalUrl", () => {
const integration = new FakeIntegration(undefined, undefined, "/cockpit/");
const result = integration.callExternalUrl("/web/index.html", { id: "42" });
expect(result.toString()).toBe("/cockpit/web/index.html?id=42");
});

test("merges path-embedded query with extra queryParams on a path-only externalUrl", () => {
const integration = new FakeIntegration(undefined, undefined, "/signalk-server/");
const result = integration.callExternalUrl("/items/42?width=100", { quality: "90" });
expect(result.pathname).toBe("/signalk-server/items/42");
expect(result.toString()).toBe("/signalk-server/items/42?width=100&quality=90");
});

test("falls back to integration.url when externalUrl is null and the integration url is absolute", () => {
const integration = new FakeIntegration(undefined, undefined, null);
const result = integration.callExternalUrl("/items/42");
expect(result.toString()).toBe("https://example.com/items/42");
});
});
});

class FakeIntegration extends Integration {
constructor(
private testingResult?: TestingResult,
private error?: Error,
externalUrl: string | null = null,
) {
super({
id: "test",
name: "Test",
url: "https://example.com",
decryptedSecrets: [],
externalUrl: null,
externalUrl,
});
}

public callExternalUrl(
path: `/${string}`,
queryParams?: Record<string, string | Date | number | boolean | null | undefined>,
) {
return this.externalUrl(path, queryParams);
}

// eslint-disable-next-line no-restricted-syntax
protected testingAsync(_: IntegrationTestingInput): Promise<TestingResult> {
if (this.error) {
Expand Down
4 changes: 3 additions & 1 deletion packages/translation/src/lang/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -1190,7 +1190,9 @@
"invalidFileName": "Invalid file name",
"fileTooLarge": "File is too large, maximum size is {maxSize}",
"invalidConfiguration": "Invalid configuration",
"groupNameTaken": "Group name already taken"
"groupNameTaken": "Group name already taken",
"appHrefInvalid": "Must be an absolute URL (https://example.com) or a path starting with / (e.g. /cockpit/)",
"appHrefConsecutiveSlashes": "Path must not contain consecutive slashes"
}
}
},
Expand Down
Loading