Skip to content

fix: replace NotImplementedException in ChangePassword, add null guard on stream#2

Open
flowcool wants to merge 1 commit into
UlysseM:mainfrom
flowcool:fix/defensive-coding-changepassword-null-stream
Open

fix: replace NotImplementedException in ChangePassword, add null guard on stream#2
flowcool wants to merge 1 commit into
UlysseM:mainfrom
flowcool:fix/defensive-coding-changepassword-null-stream

Conversation

@flowcool

@flowcool flowcool commented Jun 8, 2026

Copy link
Copy Markdown

Two small defensive fixes found during a code review (see #1 for the full context).

Changes

1. ChangePassword — replace NotImplementedException with a no-op

IAuthenticationProvider.ChangePassword was throwing NotImplementedException. If Jellyfin ever calls this on an HttpAuth account (e.g. an admin attempting a password reset), it raises an unhandled exception and may crash the calling workflow silently.

Since password management is entirely handled by the reverse proxy in this plugin's model, the correct behaviour is to do nothing and log a message explaining why.

// Before
throw new NotImplementedException();

// After
_logger.LogInformation("ChangePassword is not supported for HttpAuth users — passwords are managed by the reverse proxy.");
return Task.CompletedTask;

2. InjectAuthController.Login — null guard on GetManifestResourceStream

Assembly.GetManifestResourceStream returns null when the embedded resource is not found (namespace mismatch, broken build, etc.). Passing null to File() throws a NullReferenceException. An explicit NotFound() makes the failure visible and diagnosable.

// Before
return File(stream, "text/javascript");

// After
if (stream is null)
    return NotFound();
return File(stream, "text/javascript");

Notes

  • No behaviour change for the happy path.
  • Both fixes are purely defensive; no new dependencies or configuration.

@UlysseM

UlysseM commented Jun 10, 2026

Copy link
Copy Markdown
Owner

I double checked other implementation, including this official plugin:

https://github.com/jellyfin/jellyfin-plugin-ldapauth/blob/master/LDAP-Auth/LDAPAuthenticationProviderPlugin.cs#L301-L337

clearly seems to be fine with returning exception. There seems to be a benefit in not returning "CompletedTask" back to the client if the task failed, as we wouldn't want the client to think they made a change.

I'm pretty sure that jellyfin would catch and log the exception at a higher level.

Does this actually solve a real issue?

…d on stream

ChangePassword threw NotImplementedException which could crash admin
workflows (e.g. password reset attempt on an HttpAuth account). Replaced
with a log message and Task.CompletedTask since password management is
handled entirely by the reverse proxy.

GetManifestResourceStream can return null if the embedded resource is
missing (broken build, namespace mismatch). Added an early return with
NotFound() to fail explicitly rather than passing null to File().

Ref: UlysseM#1
@flowcool flowcool force-pushed the fix/defensive-coding-changepassword-null-stream branch from 801b9b5 to 2ac186c Compare June 10, 2026 07:26
@flowcool

Copy link
Copy Markdown
Author

Fair point — you're right that returning Task.CompletedTask would silently mislead the client into thinking the password was changed. Throwing is the correct behaviour here.

The only thing I'd push back on gently: NotImplementedException conventionally signals "I haven't written this yet", whereas NotSupportedException signals "this is intentionally not supported". Since HttpAuth passwords are always managed by the reverse proxy, NotSupportedException reads more accurately and is what Jellyfin itself uses for unsupported auth operations.

Would you be open to changing just the exception type, keeping a short log so it's diagnosable in traces?

_logger.LogDebug("ChangePassword is not supported for HttpAuth users — passwords are managed by the reverse proxy.");
throw new NotSupportedException("Password changes are not supported for HttpAuth users.");

If you'd rather keep NotImplementedException for consistency with the LDAP plugin, I'm happy to narrow this PR to just that change.


Also rebased on main — the null guard in InjectAuthController.cs was already added in your latest commit, so fix #2 is now gone from this PR.

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.

2 participants