Handle plugin over reverse proxy auth #4388
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @kgarner7, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request implements a crucial bug fix related to authentication handling, particularly for Subsonic plugin requests when reverse proxy authentication is enabled. The core change involves reordering the authentication checks to first attempt internal authentication, and only if that fails, then fall back to reverse proxy authentication. This prevents erroneous attempts to check for reverse proxy IPs when an internal user is already authenticated, thereby eliminating persistent error logs and improving the stability of the authentication mechanism.
Highlights
- Authentication Logic Refinement: This pull request refactors the authentication flow within Subsonic API middleware to prioritize internal authentication. Previously, the logic would simultaneously check both internal and reverse proxy authentication, which could lead to issues when internal authentication was present but no reverse proxy IP was available.
- Bug Fix: Reverse Proxy Error Logs: The primary goal of this change is to address and resolve the recurring error log
ReverseProxyWhitelist enabled but no proxy IP found in request context. Please report this error.This error occurred because Subsonic plugin requests, authenticated internally, were still attempting reverse proxy checks, which lack the necessary proxy IP context. - Improved Robustness for Plugin Requests: The changes ensure that Subsonic plugin requests, which often rely on internal authentication, correctly bypass reverse proxy authentication checks when an internal user is already identified. This leads to a more robust and error-free authentication process for such requests.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request fixes a bug where Subsonic plugin requests were failing with reverse proxy authentication enabled. The change ensures internal authentication is checked before falling back to reverse proxy authentication. There is duplicated logic in checkRequiredParameters and authenticate that should be refactored into a shared helper function.
server/subsonic/middlewares.go
Outdated
| username := server.InternalAuth(r) | ||
| // If the username comes from internal auth, do not also do reverse proxy auth, as | ||
| // the request will have no reverse proxy IP | ||
| if username == "" { | ||
| username = server.UsernameFromReverseProxyHeader(r) | ||
| } |
There was a problem hiding this comment.
This logic for determining the username is duplicated in the authenticate function (lines 102-107). Duplicating code can lead to maintenance issues, as changes will need to be made in multiple places.
Consider refactoring this into a shared helper function to avoid repetition.
| username := server.InternalAuth(r) | |
| // If the username comes from internal auth, do not also do reverse proxy auth, as | |
| // the request will have no reverse proxy IP | |
| if username == "" { | |
| username = server.UsernameFromReverseProxyHeader(r) | |
| } | |
| username := server.InternalAuth(r) | |
| // If the username comes from internal auth, do not also do reverse proxy auth, as | |
| // the request will have no reverse proxy IP | |
| if username == "" { | |
| username = server.UsernameFromReverseProxyHeader(r) | |
| } |
server/subsonic/middlewares.go
Outdated
| username := server.InternalAuth(r) | ||
| // If the username comes from internal auth, do not also do reverse proxy auth, as | ||
| // the request will have no reverse proxy IP | ||
| if username == "" { | ||
| username = server.UsernameFromReverseProxyHeader(r) | ||
| } else { | ||
| isInternalAuth = true | ||
| } |
There was a problem hiding this comment.
This logic for determining the username is duplicated in the checkRequiredParameters function (lines 54-59). Duplicating code can lead to maintenance issues, as changes will need to be made in multiple places.
Consider refactoring this into a shared helper function to avoid repetition.
username := server.InternalAuth(r)
// If the username comes from internal auth, do not also do reverse proxy auth, as
// the request will have no reverse proxy IP
if username == "" {
username = server.UsernameFromReverseProxyHeader(r)
}|
Thanks! |
Description
When reverse proxy authentication is enabled (
ReverseProxyWhitelistis nonempty), if a subsonic plugin request is made, Navidrome will still try check the reverse proxy authentication.This is a problem, because the internal authentication does not provide a reverse proxy IP, so every request results in error log
ReverseProxyWhitelist enabled but no proxy IP found in request context. Please report this error.being present.This PR fixes the order, by first checking internal authentication, and only if empty fallback to reverse proxy.
Related Issues
Type of Change
Checklist
Please review and check all that apply:
How to Test
I don't think there's an authentication test for this specifically.
I tested this by enabling
ReverseProxyWhitelist = "@"and then using a plugin which makes Subsonic requests, and verifying that the error goes away with the PR.Screenshots / Demos (if applicable)
Additional Notes