fix(oidc): trust Fastify proxy metadata#1982
Merged
Conversation
Contributor
|
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:
WalkthroughCentralizes request-origin extraction into new utilities, updates OIDC flows to consume precomputed origin info (protocol/host/fullUrl/baseUrl), refactors handler/service/controller interactions and redirect-URI validation to use extracted origin, sets Fastify trustProxy to "loopback", and updates tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant RestController as RestController
participant RequestHandler as OidcRequestHandler
participant OidcService as OidcService
participant RedirectService as RedirectUriService
Client->>RestController: GET /auth/authorize?provider=...&callback=...
RestController->>RequestHandler: extractRequestInfo(req)
RequestHandler-->>RestController: RequestInfo { protocol, host, fullUrl, baseUrl }
RestController->>OidcService: getAuthorizationUrl({ providerId, requestOriginInfo })
OidcService->>RedirectService: getRedirectUri(requestOrigin, requestOriginInfo)
RedirectService-->>OidcService: validated redirect URI
OidcService-->>RestController: authorization URL (with redirect_uri & state)
RestController-->>Client: 302 Redirect -> authorization URL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1982 +/- ##
==========================================
+ Coverage 52.21% 52.36% +0.14%
==========================================
Files 1032 1033 +1
Lines 71646 71626 -20
Branches 8146 8144 -2
==========================================
+ Hits 37412 37508 +96
+ Misses 34109 33993 -116
Partials 125 125 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/unraid-api/rest/rest.controller.ts`:
- Around line 80-87: The code in rest.controller.ts reads x-forwarded-proto and
x-forwarded-host into forwardedProto/forwardedHost and uses them to set
protocol/host for redirect validation, but these headers are untrusted unless
Fastify's trustProxy is enabled or the headers are validated; either enable
trustProxy on the FastifyAdapter when creating the Nest app (so Fastify will
safely trust and parse forwarded headers) or add a validation step before using
forwardedProto/forwardedHost: verify the request source (e.g., check req.ip or
req.raw.socket.remoteAddress) against your trusted proxy list and only use
forwardedProto/forwardedHost when the source is a trusted proxy, otherwise fall
back to req.headers.host/req.hostname; update the logic around forwardedProto,
forwardedHost, protocol, and host to enforce this.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2af04daa-3023-4514-8b98-7787b80bf87f
📒 Files selected for processing (2)
api/src/unraid-api/rest/rest.controller.test.tsapi/src/unraid-api/rest/rest.controller.ts
elibosley
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.ts (2)
20-32: Redundant fallback inextractRequestInfo.Line 23 applies
host || 'localhost:3000'butextractRequestOriginInfoon line 21 already guaranteeshostwill never be falsy (it has the same fallback). This is dead code given the current implementation.♻️ Suggested simplification
export function extractRequestInfo(req: FastifyRequest): RequestInfo { const { protocol, host } = extractRequestOriginInfo(req); - const resolvedHost = host || 'localhost:3000'; - const fullUrl = `${protocol}://${resolvedHost}${req.url}`; - const baseUrl = `${protocol}://${resolvedHost}`; + const fullUrl = `${protocol}://${host}${req.url}`; + const baseUrl = `${protocol}://${host}`; return { protocol, - host: resolvedHost, + host, fullUrl, baseUrl, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.ts` around lines 20 - 32, The assignment in extractRequestInfo redundantly falls back to 'localhost:3000' even though extractRequestOriginInfo already returns a non-falsy host; remove the redundant fallback by using the host value returned from extractRequestOriginInfo directly (i.e., change resolvedHost = host || 'localhost:3000' to resolvedHost = host or just use host when building fullUrl/baseUrl), keeping function names extractRequestInfo and extractRequestOriginInfo intact and leaving protocol, fullUrl, and baseUrl construction unchanged.
3-6: Type inconsistency:hostis always defined but typed as optional.The
RequestOriginInfointerface declareshost: string | undefined, butextractRequestOriginInfoalways returns a defined string due to the fallback chain (req.host || req.hostname || req.headers.host || 'localhost:3000'). This discrepancy can cause unnecessary null checks downstream and makes the contract confusing.Consider aligning the type with the actual behavior:
♻️ Suggested fix
export interface RequestOriginInfo { protocol: string; - host: string | undefined; + host: string; }Also applies to: 13-18
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.ts` around lines 3 - 6, The RequestOriginInfo interface declares host as possibly undefined but extractRequestOriginInfo always returns a defined string; change RequestOriginInfo.host from "string | undefined" to "string" and update any other matching declarations/usages (e.g., where RequestOriginInfo is consumed) so consumers no longer check for undefined; verify extractRequestOriginInfo (and any other function returning RequestOriginInfo) continues to supply a fallback like req.host || req.hostname || req.headers.host || 'localhost:3000' to guarantee a string.api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.ts (1)
6-10: Reuse the already-normalizedrequestInfohere.Line 44 already computes the trusted protocol/host for this flow. Re-extracting from
reqon Line 55 creates a second normalization path to keep in sync; buildingrequestOriginInfofromrequestInfokeeps the logged, validated, and forwarded values locked together.Suggested refactor
import { extractRequestInfo, - extractRequestOriginInfo, RequestInfo, } from '@app/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.js'; ... - requestOriginInfo: extractRequestOriginInfo(req), + requestOriginInfo: { + protocol: requestInfo.protocol, + host: requestInfo.host, + },Also applies to: 44-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.ts` around lines 6 - 10, The code currently computes requestInfo via extractRequestInfo(...) and then re-extracts origin info from req again; instead use the already-normalized requestInfo to build requestOriginInfo by calling extractRequestOriginInfo(requestInfo) (replace the second extractRequestOriginInfo(req) call), ensuring you pass the RequestInfo object and adjust any typings/uses of requestOriginInfo accordingly so logged/validated/forwarded values remain consistent with requestInfo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.test.ts`:
- Around line 68-73: Replace the assertions that pass UnauthorizedException into
.rejects.toThrow with bare .rejects.toThrow() in the async error tests for the
OIDC redirect URI service tests: update the expect calls that invoke
service.getRedirectUri(...) and the other failing test that asserts on
getClient(...) to use .rejects.toThrow() (no argument) so they only assert
rejection without coupling to a specific exception type.
In `@api/src/unraid-api/main.ts`:
- Around line 66-75: The trustProxy value passed into new FastifyAdapter is
using an incorrect base address for the /8 CIDR ("127.0.0.1/8"); update the
FastifyAdapter options in the NestFactory.create call (where FastifyAdapter is
instantiated for AppModule in main.ts) to use the correct CIDR "127.0.0.0/8" or,
even better for clarity, replace the array/string with Fastify's predefined
alias "loopback" (equivalent to ['127.0.0.0/8','::1/128']) so loopback proxy
trust is correctly and explicitly configured.
In `@api/src/unraid-api/rest/rest.controller.oidc.integration.test.ts`:
- Line 32: The test currently uses a weak Partial<FastifyReply> for mockReply
causing runtime guards and an as FastifyReply cast; replace it with a concrete
mock type that only includes the methods/properties you call (e.g., an
interface/type with header: (name: string, value: string) => FastifyReply & {
status: (code: number) => FastifyReply }, status: (code: number) =>
FastifyReply, send: (body?: any) => void) and declare mockReply with that type,
or alternatively keep a separate headerSpy/headerMock variable for the header
function and type mockReply without header to avoid casting; update the
beforeEach to initialize mockReply and headerSpy concretely and remove the as
FastifyReply cast when passing mockReply into the controller.
---
Nitpick comments:
In `@api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.ts`:
- Around line 6-10: The code currently computes requestInfo via
extractRequestInfo(...) and then re-extracts origin info from req again; instead
use the already-normalized requestInfo to build requestOriginInfo by calling
extractRequestOriginInfo(requestInfo) (replace the second
extractRequestOriginInfo(req) call), ensuring you pass the RequestInfo object
and adjust any typings/uses of requestOriginInfo accordingly so
logged/validated/forwarded values remain consistent with requestInfo.
In `@api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.ts`:
- Around line 20-32: The assignment in extractRequestInfo redundantly falls back
to 'localhost:3000' even though extractRequestOriginInfo already returns a
non-falsy host; remove the redundant fallback by using the host value returned
from extractRequestOriginInfo directly (i.e., change resolvedHost = host ||
'localhost:3000' to resolvedHost = host or just use host when building
fullUrl/baseUrl), keeping function names extractRequestInfo and
extractRequestOriginInfo intact and leaving protocol, fullUrl, and baseUrl
construction unchanged.
- Around line 3-6: The RequestOriginInfo interface declares host as possibly
undefined but extractRequestOriginInfo always returns a defined string; change
RequestOriginInfo.host from "string | undefined" to "string" and update any
other matching declarations/usages (e.g., where RequestOriginInfo is consumed)
so consumers no longer check for undefined; verify extractRequestOriginInfo (and
any other function returning RequestOriginInfo) continues to supply a fallback
like req.host || req.hostname || req.headers.host || 'localhost:3000' to
guarantee a string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f9842c35-3e6e-48e1-a10b-6b079ca381e7
📒 Files selected for processing (14)
api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.test.tsapi/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.tsapi/src/unraid-api/graph/resolvers/sso/core/oidc.service.integration.test.tsapi/src/unraid-api/graph/resolvers/sso/core/oidc.service.test.tsapi/src/unraid-api/graph/resolvers/sso/core/oidc.service.tsapi/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.spec.tsapi/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.tsapi/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.test.tsapi/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.tsapi/src/unraid-api/main.test.tsapi/src/unraid-api/main.tsapi/src/unraid-api/rest/rest.controller.oidc.integration.test.tsapi/src/unraid-api/rest/rest.controller.test.tsapi/src/unraid-api/rest/rest.controller.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- api/src/unraid-api/rest/rest.controller.ts
- api/src/unraid-api/rest/rest.controller.test.ts
api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.test.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/rest/rest.controller.oidc.integration.test.ts
Outdated
Show resolved
Hide resolved
- Purpose: make OIDC authorize redirect URI validation work when requests arrive through reverse proxies that send forwarded header chains. - Before: the REST authorize endpoint used raw x-forwarded-host and x-forwarded-proto header values, so comma-separated or array-style forwarded headers could be passed straight into URL validation. - Problem: proxied domain-based logins could fail with a misleading 'Invalid redirect_uri format' error even when the callback URI in Management Access was correct, while direct IP-based access still worked. - Change: trim forwarded header chains to the first hop before validation and preserve the endpoint's existing host/protocol fallback behavior for non-proxied localhost and IP flows. - Verification: added controller tests covering comma-separated and array-valued forwarded headers and ran the focused Vitest suites for the REST controller and redirect URI validator.
- Purpose: make OIDC authorize redirect validation rely on trusted Fastify request origin data instead of raw forwarded headers. - Before: the REST authorize path and redirect URI service could derive origin information differently, so proxied custom-domain requests could pass one validation layer and fail the next with a generic provider/configuration error. - Why: using raw x-forwarded-* values in a security decision is unsafe unless the proxy is trusted, and duplicating origin parsing across layers made the bug easy to reintroduce. - What: configure Fastify trustProxy for loopback proxies, centralize request-origin extraction in a shared helper, pass normalized origin info through the OIDC authorize flow, and add controller/service/bootstrap regression coverage. - How: RestController, OidcRequestHandler, OidcService, and OidcRedirectUriService now consume the same trusted protocol/host values from Fastify request metadata, with focused Vitest coverage for proxied domain flows and bootstrap configuration.
- Purpose: align the recent OIDC proxy-trust changes with the formatter output used by the API package. - Before: several touched files still had non-Prettier wrapping and import ordering from the manual edits. - Why: leaving formatting drift in the PR adds noise to review and makes later diffs less clear. - What: ran Prettier on the changed OIDC files and committed the resulting formatting-only updates. - How: applied package-local Prettier output to the touched tests and utilities without changing runtime behavior.
- Purpose: unblock the failing API CI type-check after adding the proxied-domain integration coverage. - Before: the new integration test read the mocked Location header through an optional chained lookup that Vitest handled at runtime but strict TypeScript rejected in CI. - Why: the API workflow runs Version 4.8.4 tsc: The TypeScript Compiler - Version 4.8.4 COMMON COMMANDS tsc Compiles the current project (tsconfig.json in the working directory.) tsc app.ts util.ts Ignoring tsconfig.json, compiles the specified files with default compiler options. tsc -b Build a composite project in the working directory. tsc --init Creates a tsconfig.json with the recommended settings in the working directory. tsc -p ./path/to/tsconfig.json Compiles the TypeScript project located at the specified path. tsc --help --all An expanded version of this information, showing all possible compiler options tsc --noEmit tsc --target esnext Compiles the current project, with additional settings. COMMAND LINE FLAGS --help, -h Print this message. --watch, -w Watch input files. --all Show all compiler options. --version, -v Print the compiler's version. --init Initializes a TypeScript project and creates a tsconfig.json file. --project, -p Compile the project given the path to its configuration file, or to a folder with a 'tsconfig.json'. --build, -b Build one or more projects and their dependencies, if out of date --showConfig Print the final configuration instead of building. COMMON COMPILER OPTIONS --pretty Enable color and formatting in TypeScript's output to make compiler errors easier to read. type: boolean default: true --target, -t Set the JavaScript language version for emitted JavaScript and include compatible library declarations. one of: es3, es5, es6/es2015, es2016, es2017, es2018, es2019, es2020, es2021, es2022, esnext default: es3 --module, -m Specify what module code is generated. one of: none, commonjs, amd, umd, system, es6/es2015, es2020, es2022, esnext, node16, nodenext default: undefined --lib Specify a set of bundled library declaration files that describe the target runtime environment. one or more: es5, es6/es2015, es7/es2016, es2017, es2018, es2019, es2020, es2021, es2022, esnext, dom, dom.iterable, webworker, webworker.importscripts, webworker.iterable, scripthost, es2015.core, es2015.collection, es2015.generator, es2015.iterable, es2015.promise, es2015.proxy, es2015.reflect, es2015.symbol, es2015.symbol.wellknown, es2016.array.include, es2017.object, es2017.sharedmemory, es2017.string, es2017.intl, es2017.typedarrays, es2018.asyncgenerator, es2018.asynciterable/esnext.asynciterable, es2018.intl, es2018.promise, es2018.regexp, es2019.array, es2019.object, es2019.string, es2019.symbol/esnext.symbol, es2020.bigint/esnext.bigint, es2020.date, es2020.promise, es2020.sharedmemory, es2020.string, es2020.symbol.wellknown, es2020.intl, es2020.number, es2021.promise/esnext.promise, es2021.string, es2021.weakref/esnext.weakref, es2021.intl, es2022.array/esnext.array, es2022.error, es2022.intl, es2022.object, es2022.sharedmemory, es2022.string/esnext.string, esnext.intl default: undefined --allowJs Allow JavaScript files to be a part of your program. Use the 'checkJS' option to get errors from these files. type: boolean default: false --checkJs Enable error reporting in type-checked JavaScript files. type: boolean default: false --jsx Specify what JSX code is generated. one of: preserve, react, react-native, react-jsx, react-jsxdev default: undefined --declaration, -d Generate .d.ts files from TypeScript and JavaScript files in your project. type: boolean default: `false`, unless `composite` is set --declarationMap Create sourcemaps for d.ts files. type: boolean default: false --emitDeclarationOnly Only output d.ts files and not JavaScript files. type: boolean default: false --sourceMap Create source map files for emitted JavaScript files. type: boolean default: false --outFile Specify a file that bundles all outputs into one JavaScript file. If 'declaration' is true, also designates a file that bundles all .d.ts output. --outDir Specify an output folder for all emitted files. --removeComments Disable emitting comments. type: boolean default: false --noEmit Disable emitting files from a compilation. type: boolean default: false --strict Enable all strict type-checking options. type: boolean default: false --types Specify type package names to be included without being referenced in a source file. --esModuleInterop Emit additional JavaScript to ease support for importing CommonJS modules. This enables 'allowSyntheticDefaultImports' for type compatibility. type: boolean default: false You can learn about all of the compiler options at https://aka.ms/tsc, so the test suite must satisfy compile-time nullability rules even when runtime assertions already guarantee the value. - What: narrowed the reply header mock explicitly before reading call arguments and removed the unsafe cast when constructing the redirect URL. - How: assert the header mock exists, derive the matching call from the narrowed mock, and guard the extracted header value before passing it to .
- Purpose: address the remaining review feedback on proxy trust configuration, request-origin typing, and OIDC regression test brittleness. - Before: Fastify trustProxy used a misleading loopback CIDR string, request-origin utilities still carried an unnecessary optional host contract, and a few tests were more tightly coupled to implementation details than needed. - Why: the runtime behavior was mostly correct, but the follow-up cleanup reduces ambiguity around trusted proxy behavior and keeps the new OIDC coverage maintainable under strict type-checking. - What: switch Fastify trustProxy to the explicit loopback alias, make RequestOriginInfo.host always defined, reuse normalized request info in the authorize handler, relax async error assertions, and harden the authorize integration reply mock. - How: update the bootstrap/test expectations, simplify request-origin construction, pass only protocol/host from normalized request info, and rework the integration test spies so they satisfy both Vitest and TypeScript without unsafe call-site assumptions.
elibosley
force-pushed
the
codex/fix-oidc-proxy-redirect-uri
branch
from
637aad5 to
182f27d
Compare
Contributor
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.ts (1)
35-58: Pass the already-normalizedRequestInfointohandleAuthorize().
oidcAuthorize()now normalizes the request once before validation, buthandleAuthorize()immediately recomputes it fromreq. Threading the existingrequestInfothrough here would keep validation and authorization pinned to the same trusted object and remove the last duplicated origin derivation in this path.♻️ Proposed refactor
static async handleAuthorize( providerId: string, state: string, redirectUri: string, - req: FastifyRequest, + requestInfo: RequestInfo, oidcService: OidcService, logger: Logger ): Promise<string> { - const requestInfo = this.extractRequestInfo(req); - logger.debug(`Authorization request - Provider: ${providerId}`); logger.debug(`Authorization request - Full URL: ${requestInfo.fullUrl}`); logger.debug(`Authorization request - Redirect URI: ${redirectUri}`);The controller can then pass the
requestInfoit already computed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.ts` around lines 35 - 58, handleAuthorize currently re-extracts RequestInfo via extractRequestInfo(req) causing a duplicate origin derivation; change handleAuthorize signature to accept a pre-normalized RequestInfo (e.g., add parameter requestInfo: RequestInfo) and use that value when calling oidcService.getAuthorizationUrl (pass requestInfo.protocol and requestInfo.host and requestInfo.fullUrl to logs) instead of calling extractRequestInfo; update the caller (oidcAuthorize) to pass the already-computed RequestInfo into handleAuthorize and remove the redundant extractRequestInfo invocation inside handleAuthorize, ensuring types for RequestInfo are imported/kept consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/unraid-api/rest/rest.controller.test.ts`:
- Around line 20-34: The headers parameter in createMockRequest is currently
typed as Record<string, any>, which weakens type safety; change its type to
FastifyRequest['headers'] and update the default value to an empty object casted
or typed as that same Headers type (e.g., headers: FastifyRequest['headers'] =
{} as FastifyRequest['headers']). Ensure the FastifyRequest type is
imported/available in the test file and keep the rest of createMockRequest
(hostname, options, returned object cast to FastifyRequest) unchanged.
---
Nitpick comments:
In `@api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.ts`:
- Around line 35-58: handleAuthorize currently re-extracts RequestInfo via
extractRequestInfo(req) causing a duplicate origin derivation; change
handleAuthorize signature to accept a pre-normalized RequestInfo (e.g., add
parameter requestInfo: RequestInfo) and use that value when calling
oidcService.getAuthorizationUrl (pass requestInfo.protocol and requestInfo.host
and requestInfo.fullUrl to logs) instead of calling extractRequestInfo; update
the caller (oidcAuthorize) to pass the already-computed RequestInfo into
handleAuthorize and remove the redundant extractRequestInfo invocation inside
handleAuthorize, ensuring types for RequestInfo are imported/kept consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9719c4c9-7d30-4eb1-8ca8-e8282fe5d5f2
📒 Files selected for processing (14)
api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.test.tsapi/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.tsapi/src/unraid-api/graph/resolvers/sso/core/oidc.service.integration.test.tsapi/src/unraid-api/graph/resolvers/sso/core/oidc.service.test.tsapi/src/unraid-api/graph/resolvers/sso/core/oidc.service.tsapi/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.spec.tsapi/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.tsapi/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.test.tsapi/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.tsapi/src/unraid-api/main.test.tsapi/src/unraid-api/main.tsapi/src/unraid-api/rest/rest.controller.oidc.integration.test.tsapi/src/unraid-api/rest/rest.controller.test.tsapi/src/unraid-api/rest/rest.controller.ts
✅ Files skipped from review due to trivial changes (3)
- api/src/unraid-api/main.test.ts
- api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.test.ts
- api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- api/src/unraid-api/graph/resolvers/sso/core/oidc.service.integration.test.ts
- api/src/unraid-api/graph/resolvers/sso/core/oidc.service.ts
- api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.ts
- api/src/unraid-api/graph/resolvers/sso/core/oidc.service.test.ts
Comment on lines
+20
to
34
| const createMockRequest = ( | ||
| hostname?: string, | ||
| headers: Record<string, any> = {}, | ||
| options: { | ||
| host?: string; | ||
| protocol?: string; | ||
| } = {} | ||
| ): FastifyRequest => { | ||
| return { | ||
| headers, | ||
| hostname, | ||
| host: options.host ?? hostname ?? headers.host, | ||
| url: '/test', | ||
| protocol: 'https', | ||
| protocol: options.protocol ?? 'https', | ||
| } as FastifyRequest; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n api/src/unraid-api/rest/rest.controller.test.ts | head -50Repository: unraid/api
Length of output: 2294
🏁 Script executed:
rg -A 5 "const createMockRequest" api/src/unraid-api/rest/rest.controller.test.tsRepository: unraid/api
Length of output: 232
🏁 Script executed:
rg "import.*Fastify" api/src/unraid-api/rest/rest.controller.test.ts -A 2 -B 2Repository: unraid/api
Length of output: 396
🏁 Script executed:
cat api/src/unraid-api/types/fastify.ts | head -100Repository: unraid/api
Length of output: 894
🏁 Script executed:
rg "headers" api/src/unraid-api/types/fastify.ts -A 2 -B 2Repository: unraid/api
Length of output: 349
🏁 Script executed:
rg "createMockRequest" api/src/unraid-api/rest/rest.controller.test.ts -A 2 -B 1Repository: unraid/api
Length of output: 4445
🏁 Script executed:
cat api/src/unraid-api/types/fastify.tsRepository: unraid/api
Length of output: 894
Tighten the mock header type.
Record<string, any> violates the coding guideline and weakens type safety in these proxy regression tests. Use FastifyRequest['headers'] to properly reference the project's custom Headers type, which constrains headers to the correct shape.
♻️ Proposed fix
- headers: Record<string, any> = {},
+ headers: FastifyRequest['headers'] = {},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const createMockRequest = ( | |
| hostname?: string, | |
| headers: Record<string, any> = {}, | |
| options: { | |
| host?: string; | |
| protocol?: string; | |
| } = {} | |
| ): FastifyRequest => { | |
| return { | |
| headers, | |
| hostname, | |
| host: options.host ?? hostname ?? headers.host, | |
| url: '/test', | |
| protocol: 'https', | |
| protocol: options.protocol ?? 'https', | |
| } as FastifyRequest; | |
| const createMockRequest = ( | |
| hostname?: string, | |
| headers: FastifyRequest['headers'] = {}, | |
| options: { | |
| host?: string; | |
| protocol?: string; | |
| } = {} | |
| ): FastifyRequest => { | |
| return { | |
| headers, | |
| hostname, | |
| host: options.host ?? hostname ?? headers.host, | |
| url: '/test', | |
| protocol: options.protocol ?? 'https', | |
| } as FastifyRequest; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/unraid-api/rest/rest.controller.test.ts` around lines 20 - 34, The
headers parameter in createMockRequest is currently typed as Record<string,
any>, which weakens type safety; change its type to FastifyRequest['headers']
and update the default value to an empty object casted or typed as that same
Headers type (e.g., headers: FastifyRequest['headers'] = {} as
FastifyRequest['headers']). Ensure the FastifyRequest type is imported/available
in the test file and keep the rest of createMockRequest (hostname, options,
returned object cast to FastifyRequest) unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Problem
A valid callback like
https://nas.domain.com/graphql/api/auth/oidc/callbackcould fail behind the built-in proxy path even though it was allowed in Management Access. The authorize flow had duplicated origin derivation, one layer previously relied on raw forwarded headers, and the follow-up regression coverage still had a few reviewable rough edges around proxy trust clarity and test contracts.Fix
Use Fastify's explicit
loopbacktrust proxy alias, centralize OIDC request-origin extraction, and pass only normalized protocol/host information through the authorize flow. The request-origin utilities now guarantee a defined host, and the OIDC tests assert rejection behavior and reply interactions without coupling to exception classes or unsafe mock casts.Testing
pnpm --filter ./api type-checkpnpm --filter ./api exec vitest run src/unraid-api/main.test.ts src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.test.ts src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.test.ts src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.spec.ts src/unraid-api/rest/rest.controller.oidc.integration.test.tspnpm --filter ./api exec vitest run src/unraid-api/main.test.ts src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.test.ts src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.test.ts src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.spec.ts src/unraid-api/graph/resolvers/sso/core/oidc.service.test.ts src/unraid-api/graph/resolvers/sso/core/oidc.service.integration.test.ts src/unraid-api/rest/rest.controller.test.ts src/unraid-api/rest/rest.controller.oidc.integration.test.tsDeployment Note
webguimount proxies/graphqlto the API overunix:/var/run/unraid-api.sockand forwardsHost, so these changes should not disrupt existing users on the standard nginx/socket path.X-Forwarded-*over a non-loopback TCP proxy hop,trustProxymay need to be widened to that explicit trusted network instead of loopback.Work Intent
[UNRD]from the reported issue contextSummary by CodeRabbit
New Features & Improvements
Tests