Fix interrupted response when provided an unknown user#5354
Fix interrupted response when provided an unknown user#5354jrh3k5 wants to merge 1 commit intoOmbi-app:developfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughStops middleware from continuing the request pipeline when an API-key impersonation header names a nonexistent user; adds unit tests for that case and for API-key auth without Changes
sequenceDiagram
autonumber
participant C as Client
participant M as ApiKeyMiddleware
participant S as ISettingsService/OmbiSettings
participant U as OmbiUserManager
participant D as Downstream (RequestDelegate)
C->>M: HTTP request (ApiKey + optional UserName headers)
M->>S: GetSettingsAsync()
S-->>M: OmbiSettings (ApiKey matches)
alt UserName header present
M->>U: Find user by UserName
U-->>M: No matching user
M-->>C: Write 401 "Invalid User" and return
Note over M,D: Downstream not invoked
else No UserName header
M->>M: Create API identity (Name = "API")
M-->>D: Invoke downstream
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Ombi.Tests/Middlewear/ApiKeyMiddlewearTests.cs (1)
97-126: Good test coverage for the fix.The test correctly validates that:
- A 401 status is returned for an invalid user
- The response body contains "Invalid User"
- The downstream
RequestDelegateis never invokedMinor hygiene suggestion: consider wrapping the
StreamReaderin ausingstatement to ensure proper disposal.♻️ Optional: dispose StreamReader
Assert.That(context.Response.StatusCode, Is.EqualTo(401)); bodyStream.Position = 0; - var reader = new StreamReader(bodyStream); - var body = await reader.ReadToEndAsync(); + using var reader = new StreamReader(bodyStream); + var body = await reader.ReadToEndAsync(); Assert.That(body, Is.EqualTo("Invalid User"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Ombi.Tests/Middlewear/ApiKeyMiddlewearTests.cs` around lines 97 - 126, The StreamReader created in the ValidateApiKey_InvalidUser test is not disposed; wrap the StreamReader (constructed from bodyStream) in a using (or use await using) to ensure it is properly disposed after reading the response body in the test method ValidateApiKey_InvalidUser so resources are released and test hygiene is improved.src/Ombi/Middleware/ApiKeyMiddlewear.cs (1)
104-116: Pre-existing logic concern: empty username handling may have unintended behaviour.When
username.IsNullOrEmpty()is true,UseApiUseris called, but execution then continues past the if-else to lines 113-116 where user lookup occurs. Since no user will match an empty/null normalised username, this will result in a 401 "Invalid User" response.This differs from the no-header case (lines 128-130) which calls
UseApiUserand then proceeds tonext.Invoke. If an emptyUserNameheader should behave the same as no header, consider adding areturnor restructuring the flow.This is pre-existing and out of scope for this PR, but worth noting for future consideration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Ombi/Middleware/ApiKeyMiddlewear.cs` around lines 104 - 116, When username.IsNullOrEmpty() is true the code calls UseApiUser(context) but then continues to run the user lookup (um.Users.FirstOrDefaultAsync) which will fail and return 401; change the control flow so that when username.IsNullOrEmpty() you stop further processing and behave like the no-header case — either return/await next.Invoke after calling UseApiUser(context) or set username to the API user’s normalized name before the lookup. Update the block around username.IsNullOrEmpty(), UseApiUser(context), and the subsequent um (OmbiUserManager) Users.FirstOrDefaultAsync call to ensure no lookup occurs for an empty header and execution continues to next.Invoke.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Ombi.Tests/Middlewear/ApiKeyMiddlewearTests.cs`:
- Around line 97-126: The StreamReader created in the ValidateApiKey_InvalidUser
test is not disposed; wrap the StreamReader (constructed from bodyStream) in a
using (or use await using) to ensure it is properly disposed after reading the
response body in the test method ValidateApiKey_InvalidUser so resources are
released and test hygiene is improved.
In `@src/Ombi/Middleware/ApiKeyMiddlewear.cs`:
- Around line 104-116: When username.IsNullOrEmpty() is true the code calls
UseApiUser(context) but then continues to run the user lookup
(um.Users.FirstOrDefaultAsync) which will fail and return 401; change the
control flow so that when username.IsNullOrEmpty() you stop further processing
and behave like the no-header case — either return/await next.Invoke after
calling UseApiUser(context) or set username to the API user’s normalized name
before the lookup. Update the block around username.IsNullOrEmpty(),
UseApiUser(context), and the subsequent um (OmbiUserManager)
Users.FirstOrDefaultAsync call to ensure no lookup occurs for an empty header
and execution continues to next.Invoke.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1239053-e6dc-44d6-9d43-09eee2e32a1a
📒 Files selected for processing (2)
src/Ombi.Tests/Middlewear/ApiKeyMiddlewearTests.cssrc/Ombi/Middleware/ApiKeyMiddlewear.cs
|
Thanks for the contribution! |
There was a problem hiding this comment.
Pull request overview
This PR fixes API key authentication middleware so that when a request attempts to impersonate an unknown user (via the UserName header), the middleware stops processing instead of continuing the request pipeline.
Changes:
- Stop request pipeline execution on “Invalid User” during API key validation (
returninstead of invokingnext). - Add a unit test asserting
401+ body message and thatnextis not invoked for an invalid impersonated user.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Ombi/Middleware/ApiKeyMiddlewear.cs |
Halts middleware execution when an impersonated user cannot be resolved. |
src/Ombi.Tests/Middlewear/ApiKeyMiddlewearTests.cs |
Adds coverage for the invalid-user impersonation path (ensures 401 + no downstream invocation). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Ombi.Tests/Middlewear/ApiKeyMiddlewearTests.cs (1)
111-115: Consider extracting duplicated settings mock setup into a helper.The
ISettingsService<OmbiSettings>arrangement is repeated in multiple tests; a small helper would reduce boilerplate and keep tests easier to evolve.Also applies to: 145-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Ombi.Tests/Middlewear/ApiKeyMiddlewearTests.cs` around lines 111 - 115, Extract the duplicated setup for ISettingsService<OmbiSettings> into a single helper method (e.g., CreateSettingsServiceMock or SetupOmbiSettingsMock) inside the ApiKeyMiddlewearTests class that returns a Mock<ISettingsService<OmbiSettings>> (or configures the _mocker to return the mock), initialise it with new OmbiSettings { ApiKey = "validkey" }, and update the tests that currently create settingsMock (the occurrences around settingsMock and the _mocker.Setup for IServiceProvider) to call that helper instead to remove boilerplate and centralise future changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Ombi.Tests/Middlewear/ApiKeyMiddlewearTests.cs`:
- Around line 111-115: Extract the duplicated setup for
ISettingsService<OmbiSettings> into a single helper method (e.g.,
CreateSettingsServiceMock or SetupOmbiSettingsMock) inside the
ApiKeyMiddlewearTests class that returns a Mock<ISettingsService<OmbiSettings>>
(or configures the _mocker to return the mock), initialise it with new
OmbiSettings { ApiKey = "validkey" }, and update the tests that currently create
settingsMock (the occurrences around settingsMock and the _mocker.Setup for
IServiceProvider) to call that helper instead to remove boilerplate and
centralise future changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e55f3612-a5b7-43ec-afd6-c5762e77f276
📒 Files selected for processing (2)
src/Ombi.Tests/Middlewear/ApiKeyMiddlewearTests.cssrc/Ombi/Middleware/ApiKeyMiddlewear.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Ombi/Middleware/ApiKeyMiddlewear.cs
Review Summary by QodoFix handler chain continuation on unknown user rejection
WalkthroughsDescription• Fix interrupted response when unknown user provided via UserName header • Return early instead of continuing handler chain after rejection • Add unit tests for invalid user and missing username scenarios • Update test assertions and nullable annotations for C# 8.0 compatibility Diagramflowchart LR
A["Request with UserName header"] --> B{"Username provided?"}
B -->|No| C["Use API user"]
B -->|Yes| D{"User exists?"}
D -->|Yes| E["Set user context"]
D -->|No| F["Return 401 Unauthorized"]
C --> G["Continue handler chain"]
E --> G
F --> H["Stop execution"]
File Changes1. src/Ombi.Tests/Middlewear/ApiKeyMiddlewearTests.cs
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Ombi.Tests/Middlewear/ApiKeyMiddlewearTests.cs (1)
139-161: Add a companion test for emptyUserNameheaderPlease add a test where
UserNameheader is present but empty (""or whitespace) to cover theusername.IsNullOrEmpty()branch inApiKeyMiddlewear.ValidateApiKeyand prevent regressions around the recent null/blank handling changes.Proposed test addition
+ [Test] + public async Task ValidateApiKey_EmptyUserNameHeader_UsesApiUser() + { + var context = GetContext(); + context.Request.Path = "/api"; + context.Request.Headers["ApiKey"] = "validkey"; + context.Request.Headers["UserName"] = ""; + + var settingsMock = new Mock<ISettingsService<OmbiSettings>>(); + settingsMock.Setup(x => x.GetSettingsAsync()).ReturnsAsync(new OmbiSettings { ApiKey = "validkey" }); +#nullable enable + _mocker.Setup<IServiceProvider, object?>(x => x.GetService(typeof(ISettingsService<OmbiSettings>))).Returns(settingsMock.Object); +#nullable disable + + var nextMock = new Mock<RequestDelegate>(); + nextMock.Setup(n => n.Invoke(It.IsAny<HttpContext>())).Returns(Task.CompletedTask); + var subject = new ApiKeyMiddlewear(nextMock.Object); + + await subject.Invoke(context); + + Assert.That(context.Response.StatusCode, Is.EqualTo(200)); + Assert.That(context.User.Identity.Name, Is.EqualTo("API")); + nextMock.Verify(x => x.Invoke(It.IsAny<HttpContext>()), Times.Once); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Ombi.Tests/Middlewear/ApiKeyMiddlewearTests.cs` around lines 139 - 161, Add a new unit test (e.g., ValidateApiKey_EmptyUserNameHeader) that mirrors ValidateApiKey_NoUserNameHeader but sets the "UserName" request header to an empty string or whitespace (e.g., "" or " ") to exercise the username.IsNullOrEmpty() branch in ApiKeyMiddlewear.ValidateApiKey; keep the "ApiKey" header and ISettingsService<OmbiSettings> mock returning the valid key, use the same RequestDelegate mock and ApiKeyMiddlewear instantiation, await subject.Invoke(context), and assert response.StatusCode == 200, context.User.Identity.Name == "API", and that nextMock.Invoke was called once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Ombi.Tests/Middlewear/ApiKeyMiddlewearTests.cs`:
- Around line 139-161: Add a new unit test (e.g.,
ValidateApiKey_EmptyUserNameHeader) that mirrors ValidateApiKey_NoUserNameHeader
but sets the "UserName" request header to an empty string or whitespace (e.g.,
"" or " ") to exercise the username.IsNullOrEmpty() branch in
ApiKeyMiddlewear.ValidateApiKey; keep the "ApiKey" header and
ISettingsService<OmbiSettings> mock returning the valid key, use the same
RequestDelegate mock and ApiKeyMiddlewear instantiation, await
subject.Invoke(context), and assert response.StatusCode == 200,
context.User.Identity.Name == "API", and that nextMock.Invoke was called once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e1a5826-d299-4254-9d83-f20474628097
📒 Files selected for processing (1)
src/Ombi.Tests/Middlewear/ApiKeyMiddlewearTests.cs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/cypress/support/request.commands.ts (1)
21-21: Consider adding a guard for missing token (optional).Unlike
createUserincommands.ts, these commands don't validate that the token exists before making the request. If called beforecy.login(), the request will silently send"Bearer undefined". A guard similar tocreateUserwould provide clearer error messages during test debugging.💡 Example guard pattern (apply to each command if desired)
Cypress.Commands.add('requestGenericMovie', () => { + const token = Cypress.env('access_token'); + if (!token) { + throw new Error('No authentication token found. Please login first.'); + } cy.request({ method: 'POST', url: '/api/v1/request/movie', body: { TheMovieDbId: 299536 }, headers: { - 'Authorization': 'Bearer ' + Cypress.env('access_token'), + 'Authorization': 'Bearer ' + token, } }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cypress/support/request.commands.ts` at line 21, Add a guard that validates Cypress.env('access_token') before adding the Authorization header to prevent sending "Bearer undefined"; check the token at the start of each command that sets 'Authorization' (the spot that currently builds 'Authorization': 'Bearer ' + Cypress.env('access_token') ) and throw or fail the test with a clear message (similar to the createUser guard in commands.ts) when the token is missing so callers get an immediate, descriptive error instead of a malformed header.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/cypress/support/request.commands.ts`:
- Line 21: Add a guard that validates Cypress.env('access_token') before adding
the Authorization header to prevent sending "Bearer undefined"; check the token
at the start of each command that sets 'Authorization' (the spot that currently
builds 'Authorization': 'Bearer ' + Cypress.env('access_token') ) and throw or
fail the test with a clear message (similar to the createUser guard in
commands.ts) when the token is missing so callers get an immediate, descriptive
error instead of a malformed header.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b19d71c5-5afe-42d6-a24c-9cd5c5b241a9
📒 Files selected for processing (3)
tests/cypress/support/commands.tstests/cypress/support/plex-settings.commands.tstests/cypress/support/request.commands.ts
jrh3k5
force-pushed
the
user-unknown-error
branch
3 times, most recently
from
de6d464 to
a4f70e3
Compare
jrh3k5
force-pushed
the
user-unknown-error
branch
from
a4f70e3 to
452e7d6
Compare
|
@tidusjar - have to confess, the test failures have me stumped. They seem pretty isolated to my branch, but nothing in my changes seems to alter the authentication scheme in a way that would regress the service in a way that seems to be happening. Further, when I try to reproduce the error manually - e.g., the setup failure for in I'm wondering if I've accidentally exposed a latent issue in how the tests manage and supply JWTs? I tried to Copilot my way through the issue (I'm not at all familiar with Cypress), but wasn't able to produce anything fruitful. |
|
The handler code change and unit tests were written by GitHub Copilot. Subsequent edits, PR description, and manual verification were done by me.
📝 Description
If a
UserNameheader that identifies a user not known to Ombi is supplied, it un-gracefully closes the connection, causing errors like this one that I encountered in a Node app talking to Ombi:(all API keys mentioned below were generated only for the local development instance of Ombi, not the one my actual Ombi instance uses)
This is because, when the app does not recognize a username, it writes a response but then continues invoking the handler chain. That causes the
transfer closed with outstanding read data remainingerror when invoking withcurl:It also caused this error in the server:
With this change in place, the
curlcommand yields a non-erroring response (HTTP 401 response notwithstanding):In the course of drafting the PR, I received automated feedback that there remained a hole in the logic for when there was no username provided, so I addressed that, too (and provided a unit test), by nesting the username validation within the
elseblock that triggers if the username is provided and not blank.I then addressed all highlighted warnings (pre-existing and newly-introduced by myself or Copilot).
🔗 Related Issues
🧪 Testing
I made sure the new unit test failed its assertion that the handler was not invoked by reverting the change to the handler.
📸 Screenshots (if applicable)
N/A
📋 Checklist
🎯 Type of Change
📚 Additional Notes
Summary by CodeRabbit
Bug Fixes
Tests