diff --git a/.github/workflows/deployment-docker-image.yml b/.github/workflows/deployment-docker-image.yml index cec31c4890..9d8f84e4ac 100644 --- a/.github/workflows/deployment-docker-image.yml +++ b/.github/workflows/deployment-docker-image.yml @@ -31,6 +31,7 @@ concurrency: jobs: release: name: Create tag and release + if: github.repository == 'homarr-labs/homarr' runs-on: ubuntu-latest timeout-minutes: 5 env: diff --git a/.github/workflows/deployment-fork-image.yml b/.github/workflows/deployment-fork-image.yml new file mode 100644 index 0000000000..7af0bdff88 --- /dev/null +++ b/.github/workflows/deployment-fork-image.yml @@ -0,0 +1,149 @@ +name: "[Fork] Build & publish HaLOS image" + +# Builds a multi-arch container image for the hatlabs fork and publishes it to +# ghcr.io/hatlabs/homarr. See FORK.md for the tag scheme. +# +# Triggered by: +# - pushing a tag matching v*-halos.* (release build) +# - workflow_dispatch (manual rebuild of an existing ref) + +on: + push: + tags: + - "v*-halos.*" + workflow_dispatch: + inputs: + tag: + description: "Image tag to publish (e.g. v1.59.3-halos.1). Defaults to the ref name." + required: false + type: string + +permissions: + contents: read + packages: write + +env: + REGISTRY: ghcr.io + GHCR_REPO: ghcr.io/${{ github.repository }} + SKIP_ENV_VALIDATION: "true" + TURBO_TELEMETRY_DISABLED: "1" + +concurrency: + group: ${{ github.workflow }}-${{ github.ref_name }} + +jobs: + resolve-tag: + name: Resolve image tag + if: github.repository == 'hatlabs/homarr' + runs-on: ubuntu-latest + outputs: + tag: ${{ steps.pick.outputs.tag }} + is_release: ${{ steps.pick.outputs.is_release }} + steps: + - id: pick + run: | + if [ "${{ github.event_name }}" = "push" ]; then + tag="${GITHUB_REF#refs/tags/}" + is_release=true + elif [ -n "${{ inputs.tag }}" ]; then + tag="${{ inputs.tag }}" + is_release=true + else + tag="$(echo "${{ github.ref_name }}" | sed 's/[^a-zA-Z0-9._-]/-/g')" + is_release=false + fi + echo "tag=$tag" >> "$GITHUB_OUTPUT" + echo "is_release=$is_release" >> "$GITHUB_OUTPUT" + + build-amd64: + name: Build amd64 image + needs: resolve-tag + runs-on: ubuntu-latest + timeout-minutes: 30 + outputs: + digest: ${{ steps.build.outputs.digest }} + steps: + - uses: actions/checkout@v6 + + - uses: docker/metadata-action@v6 + id: meta + with: + images: ${{ env.GHCR_REPO }} + + - uses: docker/login-action@v4 + with: + registry: ${{ env.REGISTRY }} + username: ${{ github.actor }} + password: ${{ secrets.GITHUB_TOKEN }} + + - uses: docker/setup-buildx-action@v4 + + - id: build + uses: docker/build-push-action@v7 + with: + context: . + network: host + platforms: linux/amd64 + labels: ${{ steps.meta.outputs.labels }} + outputs: type=image,"name=${{ env.GHCR_REPO }}",push-by-digest=true,name-canonical=true,push=true + env: + SKIP_ENV_VALIDATION: "true" + + build-arm64: + name: Build arm64 image + needs: resolve-tag + runs-on: ubuntu-24.04-arm + timeout-minutes: 45 + outputs: + digest: ${{ steps.build.outputs.digest }} + steps: + - uses: actions/checkout@v6 + + - uses: docker/metadata-action@v6 + id: meta + with: + images: ${{ env.GHCR_REPO }} + + - uses: docker/login-action@v4 + with: + registry: ${{ env.REGISTRY }} + username: ${{ github.actor }} + password: ${{ secrets.GITHUB_TOKEN }} + + - uses: docker/setup-buildx-action@v4 + + - id: build + uses: docker/build-push-action@v7 + with: + context: . + network: host + platforms: linux/arm64 + labels: ${{ steps.meta.outputs.labels }} + outputs: type=image,"name=${{ env.GHCR_REPO }}",push-by-digest=true,name-canonical=true,push=true + env: + SKIP_ENV_VALIDATION: "true" + + publish: + name: Publish multi-arch manifest + needs: [resolve-tag, build-amd64, build-arm64] + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: docker/login-action@v4 + with: + registry: ${{ env.REGISTRY }} + username: ${{ github.actor }} + password: ${{ secrets.GITHUB_TOKEN }} + + - name: Publish tag + run: | + docker buildx imagetools create -t ${{ env.GHCR_REPO }}:${{ needs.resolve-tag.outputs.tag }} \ + ${{ env.GHCR_REPO }}@${{ needs.build-amd64.outputs.digest }} \ + ${{ env.GHCR_REPO }}@${{ needs.build-arm64.outputs.digest }} + + - name: Update latest-halos + if: needs.resolve-tag.outputs.is_release == 'true' + run: | + docker buildx imagetools create -t ${{ env.GHCR_REPO }}:latest-halos \ + ${{ env.GHCR_REPO }}@${{ needs.build-amd64.outputs.digest }} \ + ${{ env.GHCR_REPO }}@${{ needs.build-arm64.outputs.digest }} diff --git a/.github/workflows/deployment-weekly-release.yml b/.github/workflows/deployment-weekly-release.yml index 37d0c43646..f023c7e79a 100644 --- a/.github/workflows/deployment-weekly-release.yml +++ b/.github/workflows/deployment-weekly-release.yml @@ -17,6 +17,7 @@ permissions: jobs: create-and-merge-pr: + if: github.repository == 'homarr-labs/homarr' runs-on: ubuntu-latest timeout-minutes: 2 steps: diff --git a/FORK.md b/FORK.md new file mode 100644 index 0000000000..752d847555 --- /dev/null +++ b/FORK.md @@ -0,0 +1,76 @@ +# `hatlabs/homarr` fork + +This repository is a fork of [`homarr-labs/homarr`](https://github.com/homarr-labs/homarr) +maintained by Hat Labs to ship the Homarr container that powers +[HaLOS](https://halos.fi). + +The fork carries a small set of patches that have not yet landed upstream. +Patches are kept on top of clean upstream release tags so they can be carried +forward by rebase rather than merge. + +## Tag scheme + +Fork release images are tagged as `v-halos.`: + +- `` — the upstream Homarr release the fork was rebased onto + (e.g. `v1.59.3`). +- `` — fork iteration number (1-based) on top of that upstream release. + +Example: `v1.59.3-halos.1` is the first fork release built on top of +upstream `v1.59.3`. + +When upstream cuts a new release the fork rebases its branch onto it and the +counter resets: + +``` +v1.59.3-halos.1 first fork release on v1.59.3 +v1.59.3-halos.2 second fork release (still on v1.59.3) +v1.59.4-halos.1 first fork release after rebasing onto v1.59.4 +``` + +## Container images + +Images are built by `.github/workflows/deployment-fork-image.yml` and pushed to +`ghcr.io/hatlabs/homarr`: + +- `ghcr.io/hatlabs/homarr:v-halos.` — the immutable build per + fork release tag. +- `ghcr.io/hatlabs/homarr:latest-halos` — moves with the most recent fork + release. + +The workflow is triggered by pushing a `v*-halos.*` tag, or manually via +`workflow_dispatch` with a tag argument. + +## Upstream workflow + +The upstream `deployment-docker-image.yml` and `deployment-weekly-release.yml` +workflows are guarded with `if: github.repository == 'homarr-labs/homarr'` so +they no-op on the fork. This keeps the upstream files close to verbatim and +minimizes rebase conflicts when pulling new upstream commits. + +## Carried patches + +Each fork branch should land via a normal hatlabs internal PR. The eventual +upstream PR (where applicable) is prepared as a separate, narrower branch +rebased onto the upstream merge target (`dev`). + +### Commit hygiene for upstream-bound branches + +Fork branches that are intended to also land upstream must keep +**fork-only** changes in dedicated commits, separate from the +upstream-relevant change. Fork-only material includes: + +- `FORK.md` +- `.github/workflows/deployment-fork-image.yml` +- The `if: github.repository == 'homarr-labs/homarr'` guards added to + upstream workflows +- `docs/halos/` — fork-specific notes and learnings (organised under + `learnings/` with YAML frontmatter; relevant when implementing or + debugging fork-side patterns) +- Anything else that only makes sense in the `hatlabs/homarr` context + +This way the upstream PR can be prepared by cherry-picking the +upstream-relevant commits only — typically a single contiguous range — +without manual file-level surgery. Convention: one leading commit +(`ci(fork): …`) carries every fork-only file; subsequent commits carry +the upstream-bound changes. diff --git a/docs/halos/learnings/2026-04-30-widget-conditional-trpc-error-handling.md b/docs/halos/learnings/2026-04-30-widget-conditional-trpc-error-handling.md new file mode 100644 index 0000000000..725957f048 --- /dev/null +++ b/docs/halos/learnings/2026-04-30-widget-conditional-trpc-error-handling.md @@ -0,0 +1,154 @@ +--- +title: Selectively rethrow tRPC errors in widgets inside an ErrorBoundary +date: 2026-04-30 +module: packages/widgets/src/app +problem_type: best_practice +component: tooling +severity: medium +applies_when: + - "Widget uses tRPC + react-query inside a React ErrorBoundary" + - "One specific TRPCError code is a normal/expected condition, not a real failure" + - "Other error codes from the same query should still surface as the widget's error UI" + - "Replacing useSuspenseQuery with useQuery to gain access to query.error before render" +tags: + - trpc + - react-query + - error-boundary + - use-suspense-query + - use-query + - widget-pattern + - graceful-degradation +--- + +# Selectively rethrow tRPC errors in widgets inside an ErrorBoundary + +## Context + +The fork adds path-only hrefs (`/cockpit/`) so app cards work across multiple origins (mDNS, VPN FQDN, raw LAN IP). The browser resolves them against whatever origin the user is currently on. The server cannot follow that href to ping the app — synthesising an absolute URL from `X-Forwarded-Host` would be a header-spoofing / SSRF surface — so the ping router throws `TRPCError({code: "CONFLICT"})` when no explicit `pingUrl` is configured. That is a *valid* config, not a failure. + +The friction: Homarr's ping indicator was originally written with `useSuspenseQuery`, and the widget sits inside a parent React `ErrorBoundary`. Every thrown tRPC error — including this expected CONFLICT — escaped Suspense and replaced the entire app card with a loud "Try again" error panel. Genuine misconfig (FORBIDDEN, NOT_FOUND) deserves that treatment; an intentionally non-pingable app does not. + +## Guidance + +When a tRPC query lives inside an outer error boundary and *some* of its error codes represent expected, normal configuration rather than faults, switch the call site from `useSuspenseQuery` to `useQuery` and discriminate on `error.data.code`: + +1. **Replace `useSuspenseQuery` with `useQuery`** and disable retries (`retry: false`) so the expected error doesn't trigger backoff churn. +2. **Inspect `query.error.data?.code`** — render an in-place degraded UI for the known-good code(s), `throw query.error` for everything else so the outer boundary still catches genuine faults. +3. **Move the loading state inside the component.** Since you're no longer suspending, drop the parent `` and render your own placeholder when `query.data` is undefined. +4. **Prefer derivation over `useState` + `useEffect`** when merging query data with an override stream (e.g. a websocket subscription): `const result = override ?? query.data ?? null`. This avoids the one-render lag described in tradeoffs. + +The discriminator pattern: + +```tsx +if (query.error) { + if (query.error.data?.code === "CONFLICT") { + return ; + } + throw query.error; // FORBIDDEN, NOT_FOUND, INTERNAL_SERVER_ERROR → boundary +} +``` + +## Why This Matters + +- **Preserves the safety net.** The error boundary still catches genuinely broken state — auth failures, missing resources, server crashes — exactly as it did before. We narrow what's swallowed, we don't disable the boundary. +- **Turns expected config into normal UI.** A path-only href without `pingUrl` is a deliberate deployment choice, not a fault. Treating it as one is the bug; rendering a calm indeterminate dot is the fix. +- **Keeps the failure mode legible.** A typed code check (`error.data.code === "CONFLICT"`) is greppable, fails loudly if the server changes the code, and survives message rewording. Matching on `error.message` substrings would not. +- **Avoids worse alternatives.** Server-side "never throw, return null" loses the typed discriminator at the boundary and forces every consumer to recheck. An error-boundary reset button forces user interaction for a non-error. + +## When to Apply + +Apply this pattern when **all** of: + +- The query runs inside a React `ErrorBoundary` (directly or via a parent widget framework). +- At least one tRPC error code returned by the procedure represents *expected, valid* runtime state (config-driven, not a fault). +- The component can render a meaningful degraded UI for that case. + +Do **not** apply when: + +- *Every* error from the procedure is genuinely a fault — `useSuspenseQuery` + boundary is simpler and correct. +- The component relies on React 18 streaming SSR for above-the-fold / SEO-critical content. `useQuery` is client-first; you lose the streaming integration. Dashboard widgets behind auth don't care; public landing-page content does. +- You'd be tempted to catch *all* errors generically. That defeats the boundary. Discriminate on a specific known code. + +Alternatives considered and why they're worse: + +- **Server returns `null` instead of throwing CONFLICT.** Loses the typed signal; every client must re-derive "is this a real null or a config-degraded null". Erodes the procedure's contract. +- **Error boundary with reset on CONFLICT.** Forces the user to click through a non-error. Also fragile: the boundary has to introspect the error to decide whether to auto-reset, which is the same discriminator logic in a worse place. +- **Try/catch around `useSuspenseQuery`.** Doesn't work — Suspense throws promises during render; you cannot catch the eventual error synchronously at the call site. + +## Examples + +Before — `packages/widgets/src/app/ping/ping-indicator.tsx`: + +```tsx +const [ping] = clientApi.widget.app.ping.useSuspenseQuery( + { id: appId }, + { refetchOnMount: false, refetchOnWindowFocus: false }, +); +const [pingResult, setPingResult] = useState(ping); +``` + +…wrapped at the call site in `packages/widgets/src/app/component.tsx` with `}>`. + +After: + +```tsx +const query = clientApi.widget.app.ping.useQuery( + { id: appId }, + { + refetchOnMount: false, + refetchOnWindowFocus: false, + retry: false, + }, +); + +const [pingResult, setPingResult] = useState( + query.data ?? null, +); + +useEffect(() => { + if (query.data) setPingResult(query.data); +}, [query.data]); + +clientApi.widget.app.updatedPing.useSubscription( + { id: appId }, + { onData(data) { setPingResult(data); } }, +); + +// Apps without a server-pingable URL (e.g. path-only href without an explicit +// pingUrl) yield a CONFLICT. Render an indeterminate dot for that case so the +// card stays usable. Other tRPC errors (FORBIDDEN, NOT_FOUND) still bubble to +// the widget error boundary as before. +if (query.error) { + if (query.error.data?.code === "CONFLICT") { + return ; + } + throw query.error; +} + +if (!pingResult) { + return ; +} +``` + +In `component.tsx`, the `` wrapper around `` is removed along with the now-unused `IconLoader` / `useI18n` / `PingDot` imports — loading state lives inside `PingIndicator` now. + +A cleaner variant that avoids the one-render lag (see tradeoffs): + +```tsx +const query = clientApi.widget.app.ping.useQuery(/* … */); +const [override, setOverride] = useState(null); + +clientApi.widget.app.updatedPing.useSubscription( + { id: appId }, + { onData: setOverride }, +); + +const pingResult = override ?? query.data ?? null; +// no useEffect needed; render is a pure derivation +``` + +## Tradeoffs + +- **One-render visual lag on initial data.** With `useState(query.data ?? null)` + `useEffect`, the first render after `query.data` resolves shows the loading placeholder; the synced state lands one render later. Mitigation: derive `pingResult = override ?? query.data ?? null` instead of using `useState` + `useEffect`. The shipped code uses the useEffect form for symmetry with the subscription override; the derived form is preferable in new code. +- **Lost streaming-SSR integration.** `useSuspenseQuery` participates in React 18 streaming SSR; `useQuery` is client-first and renders the loading placeholder on the server. Irrelevant for authenticated dashboard widgets, material for public SEO-critical content. +- **`data` becomes nullable.** Consumers must handle `query.data === undefined` (loading) and the discriminated error case explicitly. The Suspense version made `data` non-null by construction; this version trades that ergonomic guarantee for the ability to keep rendering on expected errors. diff --git a/packages/api/src/middlewares/integration.ts b/packages/api/src/middlewares/integration.ts index 320cc7f665..ec49408dcf 100644 --- a/packages/api/src/middlewares/integration.ts +++ b/packages/api/src/middlewares/integration.ts @@ -4,6 +4,7 @@ import { z } from "zod/v4"; import type { Session } from "@homarr/auth"; import { hasQueryAccessToIntegrationsAsync } from "@homarr/auth/server"; import { constructIntegrationPermissions } from "@homarr/auth/shared"; +import { resolveServerUrl } from "@homarr/common"; import { decryptSecret } from "@homarr/common/server"; import type { AtLeastOneOf } from "@homarr/common/types"; import type { Database } from "@homarr/db"; @@ -66,7 +67,7 @@ export const createOneIntegrationMiddleware = ( ctx: { integration: { ...rest, - externalUrl: rest.app?.href ?? null, + externalUrl: rest.app ? resolveServerUrl(rest.app) : null, kind: kind as TKind, decryptedSecrets: secrets.map((secret) => ({ ...secret, @@ -128,7 +129,7 @@ export const createManyIntegrationMiddleware = ( integrations: dbIntegrations.map( ({ secrets, kind, items: _ignore1, groupPermissions: _ignore2, userPermissions: _ignore3, ...rest }) => ({ ...rest, - externalUrl: rest.app?.href ?? null, + externalUrl: rest.app ? resolveServerUrl(rest.app) : null, kind: kind as TKind, decryptedSecrets: secrets.map((secret) => ({ ...secret, diff --git a/packages/api/src/router/test/widgets/app.spec.ts b/packages/api/src/router/test/widgets/app.spec.ts index 0a14c7c965..4fd4a443f4 100644 --- a/packages/api/src/router/test/widgets/app.spec.ts +++ b/packages/api/src/router/test/widgets/app.spec.ts @@ -1,3 +1,4 @@ +import { TRPCError } from "@trpc/server"; import { describe, expect, test, vi } from "vitest"; import type { Session } from "@homarr/auth"; @@ -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; + +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); + }); +}); diff --git a/packages/api/src/router/widgets/app.ts b/packages/api/src/router/widgets/app.ts index d072417258..7dc30cdcf7 100644 --- a/packages/api/src/router/widgets/app.ts +++ b/packages/api/src/router/widgets/app.ts @@ -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"; @@ -29,7 +30,7 @@ export const appRouter = createTRPCRouter({ }); } - const pingUrl = app.pingUrl ?? app.href; + const pingUrl = resolveServerUrl(app); if (!pingUrl) { throw new TRPCError({ @@ -70,7 +71,7 @@ export const appRouter = createTRPCRouter({ }); } - const pingUrl = app.pingUrl ?? app.href; + const pingUrl = resolveServerUrl(app); if (!pingUrl) { throw new TRPCError({ diff --git a/packages/common/src/test/url.spec.ts b/packages/common/src/test/url.spec.ts index eafee07dd7..47848f9a27 100644 --- a/packages/common/src/test/url.spec.ts +++ b/packages/common/src/test/url.spec.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "vitest"; -import { getPortFromUrl } from "../url"; +import { getPortFromUrl, resolveServerUrl } from "../url"; describe("getPortFromUrl", () => { test.each([ @@ -38,3 +38,29 @@ describe("getPortFromUrl", () => { expect(act).toThrowError("Unsupported protocol: ftp:"); }); }); + +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 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/", + ); + }); +}); diff --git a/packages/common/src/url.ts b/packages/common/src/url.ts index 0bdf322fe6..47ec55dcee 100644 --- a/packages/common/src/url.ts +++ b/packages/common/src/url.ts @@ -22,6 +22,29 @@ export const extractBaseUrlFromHeaders = ( return `${protocol}://${host}`; }; +/** + * Resolves an app to the absolute URL the server should use, or null. + * 1. explicit `pingUrl` -> as-is + * 2. absolute `href` -> as-is + * 3. path-only or null `href` -> null + * + * Path-only 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 && !app.href.startsWith("/")) { + return app.href; + } + + return null; +}; + export const getPortFromUrl = (url: URL): number => { const port = url.port; if (port) { diff --git a/packages/validation/src/app.ts b/packages/validation/src/app.ts index 21a4335be1..35c5b3f673 100644 --- a/packages/validation/src/app.ts +++ b/packages/validation/src/app.ts @@ -1,10 +1,51 @@ import { z } from "zod/v4"; -export const appHrefSchema = z +// `appHrefSchema` accepts: +// - empty string -> null +// - absolute URL with http/https scheme (or any non-javascript scheme) +// - path-only URL starting with "/" followed by a non-"/" character +// +// Path-only hrefs are resolved against the current origin in the browser, and +// against the request origin server-side via `resolveServerUrl`. This lets a +// single dashboard work across multiple hostnames (mDNS, VPN FQDN, DHCP DNS). +// +// Rejects: javascript: scheme, protocol-relative ("//host/..."), single-slash +// root ("/"), bare strings without scheme or leading slash. +const absoluteHrefSchema = z .string() .trim() .url() - .regex(/^(?!javascript)[a-zA-Z]*:\/\//i) // javascript: is not allowed, i for case insensitive (so Javascript: is also not allowed) + .regex(/^(?!javascript)[a-zA-Z]*:\/\//i); + +// Disallowed characters anywhere in a path-only href: +// - "/" past the leading position would either produce protocol-relative +// "//host" (already rejected outright) or run-on slashes "/foo//bar" +// (cosmetic, also rejected for consistency). +// - "\" — WHATWG URL parser normalizes backslash to forward slash for +// http(s), so "/\evil.example.com" rendered into navigates +// cross-origin. JS regex \s does not catch this; explicit reject. +// - JS \s whitespace. +// - C0 / C1 control bytes (U+0000-U+001F, U+007F-U+009F) — invisible in +// admin UI, may survive into the DOM. +// - Bidi / zero-width / formatting characters (U+200B-U+200F, U+2028-U+202F, +// U+2066-U+2069, U+FEFF). These bypass `\s` and enable display-spoofing +// in the rendered sub-label without affecting the resolved navigation +// target — admin-confusion / phishing of secondary navigation. +const PATH_DISALLOWED = "\\\\\\s\\u0000-\\u001F\\u007F-\\u009F\\u200B-\\u200F\\u2028-\\u202F\\u2066-\\u2069\\uFEFF"; + +const pathOnlyHrefSchema = z + .string() + .trim() + // Leading "/" followed by at least one non-"/" / non-disallowed char, + // then any mix of allowed chars (including "/" for internal segments). + .regex(new RegExp(`^/[^/${PATH_DISALLOWED}][^${PATH_DISALLOWED}]*$`)) + // Reject consecutive slashes anywhere ("/foo//bar"). Cosmetic on its own + // but keeps the rendered sub-label canonical and matches the leading-"/" + // exclusion of "//host/...". + .refine((s) => !s.includes("//"), { message: "consecutive slashes are not allowed" }); + +export const appHrefSchema = absoluteHrefSchema + .or(pathOnlyHrefSchema) .or(z.literal("")) .transform((value) => (value.length === 0 ? null : value)) .nullable(); diff --git a/packages/validation/src/test/app.spec.ts b/packages/validation/src/test/app.spec.ts new file mode 100644 index 0000000000..cc3a7e126c --- /dev/null +++ b/packages/validation/src/test/app.spec.ts @@ -0,0 +1,61 @@ +import { describe, expect, test } from "vitest"; + +import { appHrefSchema } from "../app"; + +describe("appHrefSchema", () => { + test.each([ + ["https://example.com/path", "https://example.com/path"], + ["http://example.com/path", "http://example.com/path"], + ["https://example.com", "https://example.com"], + ["/cockpit/", "/cockpit/"], + ["/signalk-server/@signalk/freeboard-sk/", "/signalk-server/@signalk/freeboard-sk/"], + ["/x", "/x"], + ])("accepts %s", (input, expected) => { + expect(appHrefSchema.parse(input)).toBe(expected); + }); + + test("transforms empty string to null", () => { + expect(appHrefSchema.parse("")).toBeNull(); + }); + + test("accepts null", () => { + expect(appHrefSchema.parse(null)).toBeNull(); + }); + + test.each([ + ["javascript:alert(1)"], + ["JavaScript:alert(1)"], + ["//evil.example.com/path"], + ["/"], + ["cockpit/"], + ["not-a-url"], + ["./relative"], + ["../relative"], + // Browser-normalized cross-origin escapes — WHATWG URL parser collapses + // backslash to forward slash for http(s), so these would navigate + // off-origin if rendered into . Reject up front. + ["/\\evil.example.com/x"], + ["/\\\\evil.example.com/x"], + // Whitespace / control characters anywhere in the path. + ["/foo bar"], + ["/foo\tbar"], + ["/foo\nbar"], + ["/\tfoo"], + // C0 / C1 controls (invisible). + ["/foo"], + ["/foo"], + ["/foo"], + ["/foo"], + // Zero-width / bidi / formatting characters (display-spoofing class). + ["/​foo"], // ZWSP + ["/‌foo"], // ZWNJ + ["/‍foo"], // ZWJ + ["/‮foo"], // RTL override + ["/foo"], // BOM + // Consecutive slashes mid-path. + ["/foo//bar"], + ["/cockpit//"], + ])("rejects %s", (input) => { + expect(() => appHrefSchema.parse(input)).toThrow(); + }); +}); diff --git a/packages/widgets/src/app/component.tsx b/packages/widgets/src/app/component.tsx index 1caa1ce56f..52214d3c8c 100644 --- a/packages/widgets/src/app/component.tsx +++ b/packages/widgets/src/app/component.tsx @@ -1,25 +1,21 @@ "use client"; import type { PropsWithChildren } from "react"; -import { Fragment, Suspense } from "react"; +import { Fragment } from "react"; import { Flex, rem, Stack, Text, Tooltip, UnstyledButton } from "@mantine/core"; -import { IconLoader } from "@tabler/icons-react"; import combineClasses from "clsx"; import { clientApi } from "@homarr/api/client"; import { useRequiredBoard } from "@homarr/boards/context"; import { useSettings } from "@homarr/settings"; import { useRegisterSpotlightContextResults } from "@homarr/spotlight"; -import { useI18n } from "@homarr/translation/client"; import { MaskedOrNormalImage } from "@homarr/ui"; import type { WidgetComponentProps } from "../definition"; import classes from "./app.module.css"; -import { PingDot } from "./ping/ping-dot"; import { PingIndicator } from "./ping/ping-indicator"; export default function AppWidget({ options, isEditMode, height, width }: WidgetComponentProps<"app">) { - const t = useI18n(); const settings = useSettings(); const board = useRequiredBoard(); const [app] = clientApi.app.byId.useSuspenseQuery( @@ -130,9 +126,7 @@ export default function AppWidget({ options, isEditMode, height, width }: Widget {options.pingEnabled && !settings.forceDisableStatus && !board.disableStatus && app.href ? ( - }> - - + ) : null} ); diff --git a/packages/widgets/src/app/ping/ping-indicator.tsx b/packages/widgets/src/app/ping/ping-indicator.tsx index a6299ed95a..01c78ca417 100644 --- a/packages/widgets/src/app/ping/ping-indicator.tsx +++ b/packages/widgets/src/app/ping/ping-indicator.tsx @@ -1,5 +1,5 @@ -import { useState } from "react"; -import { IconCheck, IconX } from "@tabler/icons-react"; +import { useEffect, useState } from "react"; +import { IconCheck, IconLoader, IconX } from "@tabler/icons-react"; import type { RouterOutputs } from "@homarr/api"; import { clientApi } from "@homarr/api/client"; @@ -11,17 +11,20 @@ interface PingIndicatorProps { } export const PingIndicator = ({ appId }: PingIndicatorProps) => { - const [ping] = clientApi.widget.app.ping.useSuspenseQuery( - { - id: appId, - }, + const query = clientApi.widget.app.ping.useQuery( + { id: appId }, { refetchOnMount: false, refetchOnWindowFocus: false, + retry: false, }, ); - const [pingResult, setPingResult] = useState(ping); + const [pingResult, setPingResult] = useState(query.data ?? null); + + useEffect(() => { + if (query.data) setPingResult(query.data); + }, [query.data]); clientApi.widget.app.updatedPing.useSubscription( { id: appId }, @@ -32,6 +35,21 @@ export const PingIndicator = ({ appId }: PingIndicatorProps) => { }, ); + // Apps without a server-pingable URL (e.g. path-only href without an explicit + // pingUrl) yield a CONFLICT. Render an indeterminate dot for that case so the + // card stays usable. Other tRPC errors (FORBIDDEN, NOT_FOUND) still bubble to + // the widget error boundary as before. + if (query.error) { + if (query.error.data?.code === "CONFLICT") { + return ; + } + throw query.error as unknown as Error; + } + + if (!pingResult) { + return ; + } + const isError = "error" in pingResult || pingResult.statusCode >= 500; return ( diff --git a/packages/widgets/src/bookmarks/component.tsx b/packages/widgets/src/bookmarks/component.tsx index 09066f90d2..e8083a1d40 100644 --- a/packages/widgets/src/bookmarks/component.tsx +++ b/packages/widgets/src/bookmarks/component.tsx @@ -11,6 +11,7 @@ import { MaskedOrNormalImage } from "@homarr/ui"; import type { WidgetComponentProps } from "../definition"; import classes from "./bookmark.module.css"; +import { getHrefSubLabel } from "./sub-label"; export default function BookmarksWidget({ options, itemId }: WidgetComponentProps<"bookmarks">) { const board = useRequiredBoard(); @@ -232,7 +233,7 @@ const VerticalItem = ({ )} {!hideHostname && ( - {app.href ? new URL(app.href).hostname : undefined} + {getHrefSubLabel(app.href)} )} @@ -279,7 +280,7 @@ const HorizontalItem = ({ {!hideHostname && ( - {app.href ? new URL(app.href).hostname : undefined} + {getHrefSubLabel(app.href)} )} diff --git a/packages/widgets/src/bookmarks/sub-label.ts b/packages/widgets/src/bookmarks/sub-label.ts new file mode 100644 index 0000000000..78f4fcf737 --- /dev/null +++ b/packages/widgets/src/bookmarks/sub-label.ts @@ -0,0 +1,8 @@ +import { removeTrailingSlash } from "@homarr/common"; + +// Path-only hrefs render as the path itself; absolute hrefs render the host. +export const getHrefSubLabel = (href: string | null | undefined): string | undefined => { + if (!href) return undefined; + if (href.startsWith("/")) return removeTrailingSlash(href); + return new URL(href).hostname; +}; diff --git a/packages/widgets/src/bookmarks/test/sub-label.spec.ts b/packages/widgets/src/bookmarks/test/sub-label.spec.ts new file mode 100644 index 0000000000..0998dd1cf6 --- /dev/null +++ b/packages/widgets/src/bookmarks/test/sub-label.spec.ts @@ -0,0 +1,26 @@ +import { describe, expect, test } from "vitest"; + +import { getHrefSubLabel } from "../sub-label"; + +describe("getHrefSubLabel", () => { + test.each([[null], [undefined], [""]])("returns undefined for %s", (input) => { + expect(getHrefSubLabel(input)).toBeUndefined(); + }); + + test.each([ + ["https://docs.halos.fi", "docs.halos.fi"], + ["https://docs.halos.fi/path", "docs.halos.fi"], + ["http://example.com:8080/x", "example.com"], + ])("returns hostname for absolute %s", (input, expected) => { + expect(getHrefSubLabel(input)).toBe(expected); + }); + + test.each([ + ["/cockpit/", "/cockpit"], + ["/cockpit", "/cockpit"], + ["/signalk-server/@signalk/freeboard-sk/", "/signalk-server/@signalk/freeboard-sk"], + ["/x", "/x"], + ])("returns trimmed path for path-only %s", (input, expected) => { + expect(getHrefSubLabel(input)).toBe(expected); + }); +});