feat(app): support path-only hrefs for multi-hostname access#5595
feat(app): support path-only hrefs for multi-hostname access#5595mairas wants to merge 4 commits into
Conversation
|
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| JavaScript | Apr 30, 2026 12:25p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
|
Hey, thanks for your contribution we'll review and discuss this change in the team. If I understand it correctly you were able to configure Homarr to use subpaths |
Thanks! Homarr is the main landing page on the device, so it's always at The container apps are exposed to subpaths, e.g. |
|
Oh makes sense (yes that is what I meant) |
|
I'll take a look at this pr later tonight, sorry for the delay |
| integration: { | ||
| ...rest, | ||
| externalUrl: rest.app?.href ?? null, | ||
| externalUrl: rest.app ? resolveServerUrl(rest.app) : null, |
There was a problem hiding this comment.
I think here we can actually keep the app?.href ?? null because it is used on the client. Otherwise it will use the pingUrl. Same below
There was a problem hiding this comment.
Claude Code wanted to push back because externalUrl is also used server-side. I'm not sure I can completely follow the logic -- Claude's response is below. I'll implement accordingly. If you disagree, I'll revert. :-)
Good catch — externalUrl does flow back to the client through super.externalUrl(...).toString() in Jellyfin, Emby, Sonarr/Radarr/Readarr, Overseerr, Nextcloud and Ntfy, so for path-only-bound apps we genuinely want the path to pass through, not be collapsed to null.
The literal app?.href ?? null revert crashes, though: externalUrl is also consumed server-side in packages/integrations/src/base/integration.ts:85 (new URL(${baseUrl}${path})), and a path-only base like /cockpit/ throws Invalid URL. So I'd like to fix this properly rather than swap one crash for another.
Plan:
- Teach
createUrlto handle path-only bases — wheninputUrlis path-only, return a smallRenderablePathshim (toString()/pathname/hostname) instead of constructing aURL. 17 of 19 callers just.toString()the result; the two outliers (ntfy-integration.ts:57reading.hostname/.pathname,overseerr-integration.ts:319returning up to a.toString()caller) still behave sensibly. - Revert this middleware to
app?.href ?? nullat both call sites (70 and 132) as you suggested — safe oncecreateUrlhandles path-only. - Also patch
packages/request-handler/src/lib/cached-request-integration-job-handler.ts:97, sameapp?.href ?? nullpattern, same crash risk via the same code path. Pre-existing miss in our PR, surfaced while looking at this.
Net effect: integrations bound to path-only-href apps render their hrefs against the user's current origin, instead of falling back to integration.url (the server-internal address). Materially better for the multi-hostname use case.
resolveServerUrl stays in the ping router — the server genuinely can't ping a hostless URL, so CONFLICT is still right there. Pushing the change in a follow-up commit shortly.
There was a problem hiding this comment.
Implemented. I'll leave the conversation open for your verification.
No worries, real life intervenes etc. I'll go through your comments. They don't seem to bad. :-) |
Apply review comments from @Meierschlumpf on homarr-labs#5595. - packages/widgets/src/app/ping/ping-indicator.tsx: revert to useSuspenseQuery and move Suspense + ErrorBoundary into this file as a local wrapper. PingIndicatorInner now suspends as before; the ErrorBoundary fallback handles the path-only-without-pingUrl CONFLICT inline (orange IconLoader dot) and re-throws other tRPC errors so the widget-level boundary handles them as before. The fallback uses FallbackProps from react-error-boundary instead of an inline { error: unknown } signature for type-safety and idiom alignment. Restores the suspend-style data flow the reviewer asked for and keeps the CONFLICT special case local. Use orange (not blue) for the no- ping-URL state to differentiate it from the loading state. - packages/widgets/package.json: add react-error-boundary dependency (already in apps/nextjs and the workspace catalog). - packages/validation/src/app.ts: route both path-only rejections (character class + consecutive slashes) through createCustomErrorParams with new keys appHrefInvalid and appHrefConsecutiveSlashes. Translated user-facing messages now follow the same pattern as the rest of the validation package. - packages/translation/src/lang/*.json: add the two new keys under common.zod.errors.custom across all locales. Languages where adjacent custom-error keys already carry full translations (de, de-CH, fr, es, it, pt, nl, pl, ru, cs, sk, ja, zh, cn, hu, tr, uk, da, no, he, en, en-gb) receive a full translation. Languages where adjacent keys are empty Crowdin placeholders (sv, ko, ca, hr, ro, el, et, vi, lv, lt, sl) receive empty placeholders so Crowdin can fill them in later. cr.json is intentionally skipped because its crwdns/crwdne marker format uses Crowdin-internal IDs we cannot synthesize. - packages/validation/src/test/app.spec.ts: simplify the acceptance test.each to a flat string list (input === parsed output for all current cases), per reviewer suggestion. PR #1 comment (integration middleware externalUrl revert) intentionally deferred — that change is not safe because externalUrl is used server- side in createUrl() and a path-only string would crash new URL(). Response planning will happen in a follow-up.
Apply review comments from @Meierschlumpf on homarr-labs#5595. - packages/widgets/src/app/ping/ping-indicator.tsx: revert to useSuspenseQuery and move Suspense + ErrorBoundary into this file as a local wrapper. PingIndicatorInner now suspends as before; the ErrorBoundary fallback handles the path-only-without-pingUrl CONFLICT inline (orange IconLoader dot) and re-throws other tRPC errors so the widget-level boundary handles them as before. The fallback uses FallbackProps from react-error-boundary instead of an inline { error: unknown } signature for type-safety and idiom alignment. Restores the suspend-style data flow the reviewer asked for and keeps the CONFLICT special case local. Use orange (not blue) for the no- ping-URL state to differentiate it from the loading state. - packages/widgets/package.json: add react-error-boundary dependency (already in apps/nextjs and the workspace catalog). - packages/validation/src/app.ts: route both path-only rejections (character class + consecutive slashes) through createCustomErrorParams with new keys appHrefInvalid and appHrefConsecutiveSlashes. Translated user-facing messages now follow the same pattern as the rest of the validation package. - packages/translation/src/lang/*.json: add the two new keys under common.zod.errors.custom across all locales. Languages where adjacent custom-error keys already carry full translations (de, de-CH, fr, es, it, pt, nl, pl, ru, cs, sk, ja, zh, cn, hu, tr, uk, da, no, he, en, en-gb) receive a full translation. Languages where adjacent keys are empty Crowdin placeholders (sv, ko, ca, hr, ro, el, et, vi, lv, lt, sl) receive empty placeholders so Crowdin can fill them in later. cr.json is intentionally skipped because its crwdns/crwdne marker format uses Crowdin-internal IDs we cannot synthesize. - packages/validation/src/test/app.spec.ts: simplify the acceptance test.each to a flat string list (input === parsed output for all current cases), per reviewer suggestion. PR #1 comment (integration middleware externalUrl revert) intentionally deferred — that change is not safe because externalUrl is used server- side in createUrl() and a path-only string would crash new URL(). Response planning will happen in a follow-up.
🚨 Preview Deployment Blocked - Security ProtectionYour pull request was blocked from triggering preview deployments Why was this blocked?
How to resolve this:Option 1: Get Collaborator Access (Recommended) Option 2: Request Permission Override For Repository Administrators:To disable this security check ( This security measure protects against malicious code execution in preview deployments. Only trusted collaborators should have the ability to trigger deployments. 🛡️ Learn more about this security featureThis protection prevents unauthorized users from:
Preview deployments are powerful but require trust. Only users with repository write access can trigger them. |
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.
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.
0e4f1e3 to
ba7576a
Compare
|
Only add translation for locale |
ba7576a to
dac6e3c
Compare
|
Reverted the translations. |
|
IMO we should make it more explicit what the externalUrl and url are. Therefore I'm currently working on some nice typing improvements |
So, in practice - I should wait and then rebase and refactor to use the new types? |
|
I would push my changes into your branch containing the type changes and then we can review it again and if okay, merge it |
|
Okay nevermind I don't have access to your fork, can you give me access? |
Eh. I'm stumped - I usually tick the box while creating the PR, but now that it's not there, I can't find it... Gimme a moment. |
Looks like I can't: I can reopen the PR from my personal namespace but that'll of course wipe the comments so far. |
|
Hmm understood, no I think we can keep this open. I'll look into it tomorrow how we get my changes to you, for today I'll leave. See you soon |
Pull this PR branch into a branch on another fork and do your edits there, then comment here and ask me to pull from that fork. |
App hrefs now accept the path-only form (e.g. "/cockpit/") in addition
to absolute URLs. Path-only hrefs resolve against the current origin in
the browser, which lets a single dashboard work across multiple
hostnames (mDNS, VPN FQDN, DHCP DNS) without baking a hostname into
each card at registration time.
Path-only hrefs intentionally resolve to null server-side in the ping
widget. They are a browser-resolved form: the dashboard renders
<a href="/cockpit/"> and the browser navigates against the user's
current origin. The server has no canonical hostname for an app whose
href is path-only and must not synthesize one from request headers
(that would be a header-spoofing / SSRF vector). Apps that need
server-side ping coverage under multi-hostname deployments should set
an explicit pingUrl.
- packages/validation/src/app.ts: appHrefSchema accepts absolute URL
OR path-only form. Path-only rejects backslash, JS \s whitespace,
C0/C1 controls, and Unicode zero-width / bidi / formatting characters
anywhere in the path (display-spoofing protection). Rejects
javascript:, protocol-relative "//host/...", single-slash root,
consecutive slashes mid-path ("/foo//bar"), and bare strings. Both
rejection branches (character class + consecutive slashes) route
through createCustomErrorParams with translated keys
appHrefInvalid and appHrefConsecutiveSlashes.
- packages/common/src/url.ts: new resolveServerUrl(app) helper
centralises the pingUrl-or-absolute-href-or-null pattern used by the
ping endpoint. Path-only hrefs without an explicit pingUrl resolve
to null.
- packages/api/src/router/widgets/app.ts: path-only hrefs without an
explicit pingUrl produce a CONFLICT tRPC response from the ping
router (the server cannot ping a hostless URL).
- packages/widgets/src/app/{component,ping/ping-indicator}.tsx:
PingIndicator uses useSuspenseQuery wrapped in a local
ErrorBoundary + Suspense inside the file. The fallback handles the
CONFLICT case inline (orange IconLoader dot signalling
"ping not configured") and re-throws other tRPC errors so the
widget-level boundary still handles them as before.
- packages/widgets/src/bookmarks/sub-label.ts: extracted
getHrefSubLabel helper. Absolute -> host; path-only -> trailing-
slash-trimmed path; null -> empty. Branch-free given schema-
validated inputs.
- packages/translation/src/lang/*.json: new keys appHrefInvalid and
appHrefConsecutiveSlashes added under common.zod.errors.custom across
every locale. Languages where adjacent custom-error keys already
carry full translations receive a translation; languages where
adjacent keys are empty Crowdin placeholders receive empty
placeholders so Crowdin can fill them in later. cr.json (Crowdin
internal pseudolanguage) is intentionally skipped because its
crwdns/crwdne marker format uses Crowdin-internal IDs we cannot
synthesize.
- packages/widgets/package.json: react-error-boundary added from the
workspace catalog (already in use by apps/nextjs).
Tests:
- 33 schema cases (acceptance + rejection inc. backslash, whitespace,
control chars, zero-width / bidi, consecutive slashes; javascript:
fixture rows annotated with DeepSource JS-0087 skipcq comments to
satisfy the static analyser).
- 6 resolveServerUrl cases.
- 10 sub-label cases.
- 3 router-level ping cases (path-only-without-pingUrl CONFLICT,
path-only-with-pingUrl, absolute-href passthrough).
Backward compatibility: every change is additive. Existing absolute-
URL hrefs validate identically and render byte-identically. Path-only
support is a new branch that activates only for newly-shaped data.
Integrations build hrefs from `integration.externalUrl ?? integration.url`
via `createUrl()`. With path-only app hrefs now possible, externalUrl
can arrive as "/cockpit/" — which crashes `new URL` server-side.
This commit teaches the integration helpers to handle path-only bases
so the integration-rendered hrefs (Jellyfin "Watch movie", Sonarr
"Series", Radarr "Movie", Overseerr request links, etc.) resolve
against the user's current origin too, matching the multi-hostname
semantics the dashboard cards already enjoy.
- packages/integrations/src/base/integration.ts: new `RenderablePath`
class mirrors just enough of the WHATWG URL surface (`toString`,
`pathname`, `hostname`, `searchParams`) for the 19 existing
`super.externalUrl(...)` caller sites. 17 callers 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.
- `externalUrl()` branches on
`base.startsWith("/") && !base.startsWith("//")` and returns a
`RenderablePath` for path-only bases or a `URL` for absolute bases
(existing behavior). Scheme-relative bases are rejected at the
schema layer; the explicit `!startsWith("//")` guard here is
defense-in-depth so a malformed value can't cross-origin-escape
through the path-only branch.
- Fragment-handling invariant documented on the class. The
constructor splits on the first `?` only, not on `#`. This is
intentional: Jellyfin and Emby pass hash-bang routes like
`/web/index.html#!/details?id=abc` and expect the post-`?` params
to stay inside the hash. WHATWG URL would split them out into
`.hash` separately; mirroring that behavior would break the SPA-
routing callers this method exists to serve.
- packages/integrations/test/base.spec.ts: six new tests cover
absolute and path-only externalUrl, query-param merging in both
branches, path-embedded query strings combined with extra
queryParams for path-only, and null-externalUrl fallback to
`integration.url`. `FakeIntegration` extended with a
`callExternalUrl` helper that exposes the protected `externalUrl`
method.
The integration middleware (`packages/api/src/middlewares/integration.ts`)
keeps the existing `externalUrl: rest.app?.href ?? null` shape — path-
only hrefs now flow through cleanly to integrations and the new
`RenderablePath` branch handles them. The same fix automatically covers
the pre-existing pattern in
`packages/request-handler/src/lib/cached-request-integration-job-handler.ts`.
`resolveServerUrl` stays in `packages/api/src/router/widgets/app.ts`
only — the ping endpoint genuinely cannot ping a hostless URL, so
CONFLICT remains the right signal there.
634808f to
539e1c9
Compare
|
I've created a PR on hatlabs#4 |
Motivation
We run Homarr as the home dashboard of a small Linux distribution (HaLOS) where Traefik fronts a set of containerized apps and exposes each one at a distinct path under the same origin (
/cockpit/,/grafana/,/signalk-server/, etc.), with Authelia providing SSO across them.A given device can be reached on multiple hostnames in different contexts: locally over mDNS as
halos.local, and remotely over a VPN as something likehalos.example.com. Our Traefik and SSO setup copes with both — TLS SANs, Authelia per-host cookies, and OIDC redirect URIs all adapt per request — but Homarr does not. App hrefs must be absolute URLs today, so a card created while reaching the dashboard athalos.localpermanently links tohttps://halos.local/cockpit/. Clicking it from the VPN jumps the browser to a hostname that doesn't resolve in that context.Path-only hrefs (
/cockpit/) fix this cleanly: the browser resolves them against the current origin natively, so the same card works under every hostname the dashboard answers on. The change is purely additive — existing absolute hrefs are unaffected at every call site.What changes
1. Schema relaxation —
packages/validation/src/app.tsappHrefSchemanow accepts either:/[^/]...— must start with a single forward slash followed by a non-slash character.Path-only branch rejects:
javascript:and other dangerous schemes (existing protection preserved by branch ordering).//host/...)./) — too ambiguous to be a useful link target./foo//bar) — defends against accidental URL-parsing surprises in downstream consumers.\scharacters anywhere in the path./adminthat visually resemble/adminbut route differently).Empty-string-to-null transform behavior is preserved (existing).
2. Server-side resolver —
packages/common/src/url.tsNew
resolveServerUrl(app)helper centralizes the existingpingUrl ?? hrefpattern:Design choice: path-only hrefs intentionally resolve to
nullserver-side. Path-only hrefs are a browser-resolved form. The server has no canonical hostname for an app whose href is path-only, and must not synthesize one from request headers — that would be a header-spoofing / SSRF vector (a malicious client settingx-forwarded-host: internal.targetcould redirect server-side fetches arbitrarily).An earlier draft of this resolver expanded path-only hrefs against
extractBaseUrlFromHeaders(headers). We dropped that branch on review — it adds an attack surface for a feature path-only-href apps don't actually need. Apps that require server-side coverage under multi-hostname deployments should set an explicitpingUrl.3. Ping widget —
packages/api/src/router/widgets/app.ts+packages/widgets/src/app/{component,ping/ping-indicator}.tsxThe
pingUrl ?? hrefpattern in the router is routed throughresolveServerUrl. When it returnsnull(path-only without explicitpingUrl), the router responds with a tRPCCONFLICTerror instead of attempting a fetch.Client-side: switch from
useSuspenseQuerytouseQuery. The previous indicator useduseSuspenseQuerywrapped in a<Suspense>boundary incomponent.tsx. With Suspense, a tRPC error propagates to the nearest error boundary — which would treat the new "no ping URL available" CONFLICT response as a render-breaking error rather than the expected absence it actually is. The refactor:ping-indicator.tsx:useSuspenseQuery→useQuerywithretry: false. Readsquery.data/query.errordirectly. While loading or pending data, renders the blueIconLoaderdot inline. On CONFLICT (path-only without pingUrl), renders a permanent blueIconLoaderdot with the server's error message as tooltip — an explicit indeterminate state. Other tRPC errors (FORBIDDEN, NOT_FOUND, etc.) are re-thrown so the widget error boundary handles them as before.component.tsx: drops the<Suspense fallback={<PingDot icon={IconLoader}…/>}>wrapper since the loader is now handled inline by the indicator.Net behavior: existing apps with absolute href / explicit pingUrl render byte-identically to today (loader → dot transition is now in the indicator instead of via Suspense, but visually the same). Path-only-without-pingUrl apps display the indeterminate blue loader dot indefinitely rather than tripping the widget error boundary — visually communicating "ping not configured" without breaking the card.
4. Integration middleware —
packages/api/src/middlewares/integration.tsexternalUrl: rest.app?.href ?? nullis replaced withexternalUrl: resolveServerUrl(rest.app). This collapses path-only hrefs tonull, letting the existingexternalUrl ?? integration.urlfallback atpackages/integrations/src/base/integration.ts:85take over.Without this, integrations bound to path-only-href apps crash at runtime via
new URL("/cockpit/" + path)→TypeError [ERR_INVALID_URL].5. Bookmarks widget —
packages/widgets/src/bookmarks/sub-label.tsExtracted
getHrefSubLabelhelper. Branch-free given schema-validated inputs:docs.example.com)./cockpit/→/cockpit).Backward compatibility
Every change is additive:
pingUrlor absolutehref, the resolver returns the same string today'spingUrl ?? hrefpattern would. The newnullcase is reachable only for path-only hrefs withoutpingUrl— a state that is impossible under the old schema.new URL(app.href).hostname; the new helper produces the same hostname for all currently-valid hrefs.There is no migration; existing data continues to validate and render unchanged.
Tests
packages/validation/src/test/app.spec.ts— 33 schema cases: 12 acceptance, 21 rejection (backslash, whitespace, control chars, zero-width / bidi / formatting, consecutive slashes, javascript:, protocol-relative, bare strings, single-slash root).packages/common/src/test/url.spec.ts— 11 resolver cases.packages/widgets/src/bookmarks/test/sub-label.spec.ts— 10 sub-label render cases.packages/api/src/router/test/widgets/app.spec.ts— 3 router-level ping cases (path-only-without-pingUrl CONFLICT, path-only-with-pingUrl, absolute-href passthrough).PR template checklist
pnpm build, autofix withpnpm format:fix)devbranchx,y,ior any abbrevation)