Skip to content

fix(links): use the declared public base URL for the OIDC redirect, pairing QRs and reset email - #219

Open
camarigor wants to merge 1 commit into
ZL154:mainfrom
camarigor:fix/public-base-url
Open

camarigor wants to merge 1 commit into
ZL154:mainfrom
camarigor:fix/public-base-url

Conversation

@camarigor

Copy link
Copy Markdown
Contributor

Summary

Fixes the wrong host in every link the plugin hands to someone else, reported by @Heldenkrieger01 while testing #218 on an LG TV: the OIDC redirect_uri came out as the private LAN address instead of the public name the IdP has registered, and he saw the same thing in the password reset email.

The links are all derived from the incoming request. That is correct behind a reverse proxy, where the forwarded headers carry the public name once the peer is in TrustedProxyCidrs. It cannot work for a client that reaches Jellyfin directly: a smart TV connects to 192.168.x.y, no proxy is in the path, and there is no header to read. The plugin then uses the address it was reached on, which is not the address the outside world uses. No proxy setting can fix that, because the proxy never sees the request; the server has to be told its public address.

This adds one place that answers "what is this server's public base URL", and routes the four externally facing links through it.

Type of change

  • Bug fix (non-breaking)
  • New feature (non-breaking)
  • Breaking change (existing behaviour, config, or API changes)
  • Security fix
  • Documentation only
  • CI / build / tests only
  • Refactor (no functional change)

Nothing changes for a server where the request already carries the right host: with neither source configured, the previous derivation runs untouched.

Related issues

Reported in #218 (the QR PR) while testing #216. The two are independent and complementary: #218 shows the sign-in link as a QR, this one makes sure the URL inside it is the right one. The reporter's case needs both.

How was this tested?

  • Added or updated unit tests
  • Added or updated integration tests
  • Tested manually against a running Jellyfin server (state version)
  • N/A (explain why below)

Server tested against: Jellyfin 12.0.0, official Docker image, with Dex 2.41.1 as the identity provider, reading the redirect_uri out of the authorize URL that Oidc/LoginInfo returns.

Configuration redirect_uri produced
Neither source set (today's behaviour) http://127.0.0.1:8121/TwoFactorAuth/Oidc/Callback/dexprov
Jellyfin PublishedServerUriBySubnet = all=https://jellyfin.example.test https://jellyfin.example.test/TwoFactorAuth/Oidc/Callback/dexprov
Plus plugin PublicBaseUrl = https://tv.example.test/jellyfin https://tv.example.test/jellyfin/TwoFactorAuth/Oidc/Callback/dexprov
Plugin setting set to a malformed value falls back to Jellyfin's, https://jellyfin.example.test/...

With #218 merged in locally on top, the QR returned by the same endpoint is a valid PNG that re-renders byte for byte from the authorize URL, and the redirect_uri inside it is the corrected public one. That is the combination the reporter needs.

Local, with the .NET SDK 9.0.316 that CI installs: dotnet build JellyfinSecurity.sln -c Release has 0 errors and 0 warnings, dotnet test -c Release is 532 passed and 0 failed (511 before this branch), node --test tests/oidc-bridge.test.mjs is 8 passed, and node --check on the admin script passes. With the .NET SDK 10.0.400 the Jellyfin 12 build has 0 warnings and 0 errors.

What changed

  • Services/ExternalUrlResolver.cs (new): resolves the public base once, first hit wins.

    1. The plugin's PublicBaseUrl, for deployments where nothing else is right.
    2. Jellyfin's own published server URI, the external entry first and then all. A subnet-specific entry is deliberately ignored, since by definition it is not the address an IdP or a phone on mobile data would use.
    3. Nothing, and each caller keeps its existing request-derived behaviour.

    Normalize accepts an absolute http(s) URL and returns scheme://host[:port][/path] with no trailing slash. A value carrying a query, a fragment, credentials or another scheme is refused rather than trimmed into shape, because a link built from a half-understood value fails at the IdP with no clue why; refusing sends the caller back to the old behaviour, which at least works on a directly reachable server. Reading Jellyfin's configuration is wrapped so it can never break a sign-in.

  • Configuration/PluginConfiguration.cs: the PublicBaseUrl setting, documented with when to leave it empty.

  • Services/OidcRedirectUriBuilder.cs: Build takes an optional publicBaseUrl that short-circuits the derivation. The existing scheme and host logic, including the trusted-proxy gate, is untouched for everyone else.

  • Api/SecurityController.cs: the OIDC redirect_uri passes the resolved base.

  • Api/TwoFactorAuthController.cs: the two pairing QR URLs and the password reset email origin use it too. The reset origin was not even proxy-aware before, it read Request.Scheme directly; it now goes through the same resolution.

  • Pages/admin.html, Pages/admin-script.js, Pages/translations/*.json: the setting in the Settings tab with a help line, two new keys in all eight languages.

  • tests/.../ExternalUrlResolverTests.cs (new, 21 cases): which published entry counts as the outside world and the precedence between them, what shape a usable base has, everything that is refused, and that the redirect URI is replaced with the declared base while the no-configuration path stays exactly as it was.

Checklist

  • My code follows the existing style (run dotnet format if unsure)
  • I've added comments only where the why isn't obvious from the code
  • I've updated the README / SECURITY.md / docs if behaviour or config changed: the setting carries its own help text in the admin page; say the word if you want a README line as well
  • I've considered backwards compatibility (config migration, file formats, on-disk state): one new string setting that defaults to empty, and empty means the behaviour you have today
  • I've checked the security implications (auth bypass, secret handling, input validation): the public base comes only from admin-controlled configuration, never from a request header, so it cannot be poisoned by a client; the trusted-proxy gate on forwarded headers is unchanged; a malformed value is refused rather than reflected into a redirect
  • CI passes (dotnet build + dotnet test green)

Additional notes

  • Honouring Jellyfin's own published server URI is what makes this work with no new configuration for most people. An admin who publishes Jellyfin under a domain has already declared it there, which is exactly what the reporter's setup looks like.
  • The translations for the seven non-English strings are mine; corrections from native speakers are welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant