fix: insecure routes not working for SSO #1587
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughUpdates OIDC redirect handling to use full request origin, adjusts REST controller to be proxy-aware, and expands tests. Introduces SSO login path in Unraid PHP patch, lowering SSO token-length threshold from 800 to 500 and integrating UI include. Workflow updates fully refresh Docusaurus API docs and images with existence checks. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant REST as REST Controller
participant OIDC as OidcAuthService
participant IdP as OIDC Provider
Client->>REST: GET /oidc/authorize
REST->>REST: Derive requestOrigin (proto + host)
REST->>OIDC: getAuthorizationUrl(providerId, state, requestOrigin)
OIDC->>OIDC: Build redirectUri from origin or BASE_URL
OIDC->>IdP: Construct authorization URL
IdP-->>Client: Redirect to IdP login
Client->>REST: GET /oidc/callback
REST->>REST: Derive requestOrigin and fullUrl
REST->>OIDC: handleCallback(providerId, code, state, requestOrigin, fullUrl)
OIDC->>IdP: Exchange code for tokens (redirectUri)
IdP-->>OIDC: Tokens
OIDC-->>REST: Session/redirect response
REST-->>Client: Final redirect
sequenceDiagram
participant User
participant PHP as .login.php
participant System as getent/shadow
participant RC as rc.unraid-api
User->>PHP: Submit username/password
PHP->>System: Verify password hash
System-->>PHP: Success/Failure
alt password ok
PHP-->>User: Login success
else password fail and len(password) > 500
PHP->>RC: sso validate-token "<token>"
RC-->>PHP: JSON output + exit code
alt valid==true
PHP-->>User: Login success
else
PHP-->>User: Login failure
end
else
PHP-->>User: Login failure
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (7)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Claude finished @elibosley's task —— View job Code Review AnalysisTodo List:
Analysis Complete✅ No critical issues found in changes The diff shows improvements to OIDC redirect URI handling that properly parse full origins (protocol + host + port) instead of just hostnames. Key changes:
All changes appear security-conscious and improve proxy/reverse-proxy compatibility as intended. No authentication bypass, data validation issues, or breaking changes detected in the modified lines. |
- Updated the create-docusaurus-pr.yml workflow to ensure that images from both the API and parent docs directories are copied to the appropriate location in the generated documentation. This maintains the directory structure and improves the completeness of the documentation.
- Enhanced the create-docusaurus-pr.yml workflow to cleanly remove old API images and ensure proper copying of images from both the API and parent documentation directories. This update maintains the directory structure and improves the accuracy of the generated documentation.
- Renamed `requestHost` to `requestOrigin` in OidcAuthService methods for clarity. - Enhanced `getRedirectUri` method to handle full origin URLs, including protocol and non-standard ports. - Updated tests to reflect changes in redirect URI generation for various scenarios, including localhost and non-localhost hosts. - Modified RestController to construct request information using protocol and host headers for improved compatibility.
elibosley
force-pushed
the
fix/http-endpoints-not-working
branch
from
5178b9b to
0eb7127
Compare
- Updated the password length validation in the SSO login process from 800 to 500 characters to enhance security and prevent potential issues with excessively long tokens. - Corresponding changes made in the related snapshot and patch files to ensure consistency across the codebase.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/create-docusaurus-pr.yml(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts(4 hunks)api/src/unraid-api/rest/rest.controller.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/unraid-api/rest/rest.controller.tsapi/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsapi/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts
api/src/unraid-api/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/rest/rest.controller.tsapi/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsapi/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/unraid-api/rest/rest.controller.tsapi/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsapi/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts
api/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)
Files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/web-testing-rules.mdc)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts
🧬 Code Graph Analysis (1)
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts (1)
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts (1)
getRedirectUri(641-675)
🪛 YAMLlint (1.37.1)
.github/workflows/create-docusaurus-pr.yml
[error] 43-43: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 50-50: trailing spaces
(trailing-spaces)
[error] 55-55: trailing spaces
(trailing-spaces)
🔇 Additional comments (12)
.github/workflows/create-docusaurus-pr.yml (1)
34-59: The workflow refactoring improves API documentation handling.The changes correctly implement a complete refresh of API docs by removing old content, recreating directories, and copying from multiple image sources. The logic properly handles both
public/imagesanddocs/imagesdirectories, which provides better coverage for documentation assets.api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts (5)
39-43: LGTM! Public method signature updated to use requestOrigin.The method signature change from
requestHosttorequestOriginis more descriptive and aligns with the updated redirect URI logic that uses full origin parsing.
49-49: LGTM! Correctly passes requestOrigin to private helper method.The call to the private
getRedirectUrimethod correctly passes therequestOriginparameter.
113-118: LGTM! Consistent parameter naming update in handleCallback.The parameter name change from
requestHosttorequestOriginmaintains consistency with thegetAuthorizationUrlmethod signature.
126-126: LGTM! Correctly passes requestOrigin to getRedirectUri.The call correctly passes the updated
requestOriginparameter to the private helper method.
641-670: LGTM! Robust origin-based redirect URI construction.The updated
getRedirectUrimethod correctly:
- Parses the full origin URL to extract protocol, hostname, and port
- Handles port normalization by removing default ports (80 for HTTP, 443 for HTTPS)
- Provides special handling for localhost:3000 development scenarios
- Includes proper error handling with fallback to base URL
- Constructs consistent redirect URIs with the proper callback path
The implementation is more robust than the previous host-based approach and properly handles reverse proxy scenarios.
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts (4)
1515-1518: LGTM! Test correctly validates localhost development scenario.The test properly uses a full URL (
http://localhost:3000) as input and verifies the expected callback URI is generated.
1520-1525: LGTM! Test validates non-localhost HTTPS scenario.The test correctly uses a full HTTPS URL and verifies the proper callback URI construction.
1527-1532: LGTM! Test validates HTTP protocol for non-localhost hosts.This test case ensures HTTP protocol support works correctly for non-localhost hosts like
tower.local, which is important for internal/development environments.
1534-1539: LGTM! Test validates non-standard port handling.The test correctly verifies that non-standard ports are preserved in the redirect URI, which is essential for proper callback handling in various deployment scenarios.
api/src/unraid-api/rest/rest.controller.ts (2)
78-87: LGTM! Improved origin handling with proper reverse proxy support.The updated code correctly:
- Derives protocol from
x-forwarded-protoheader with fallback toreq.protocolor 'http'- Extracts host from
x-forwarded-hostheader with fallback toreq.headers.host- Constructs a proper
requestInfostring with protocol and host- Passes the
requestInfoto the OIDC service instead of just the hostThis implementation properly handles reverse proxy scenarios commonly found in production deployments.
134-144: LGTM! Consistent origin handling in callback method.The callback method correctly applies the same origin construction logic as the authorize method and properly passes the
requestInfoparameter to the OIDC service. ThefullUrlconstruction remains unchanged, which is appropriate as it serves a different purpose.
| # Remove old API docs but preserve other folders | ||
| rm -rf docs-repo/docs/API/ | ||
| mkdir -p docs-repo/docs/API | ||
| # Copy all markdown files and maintain directory structure | ||
| cp -r source-repo/api/docs/public/. docs-repo/docs/API/ | ||
| # Clean and copy images directory specifically | ||
| rm -rf docs-repo/docs/API/images/ | ||
| mkdir -p docs-repo/docs/API/images | ||
| # Copy images from public/images if they exist | ||
| if [ -d "source-repo/api/docs/public/images" ]; then | ||
| cp -r source-repo/api/docs/public/images/. docs-repo/docs/API/images/ | ||
| fi | ||
| # Also copy any images from the parent docs/images directory | ||
| if [ -d "source-repo/api/docs/images" ]; then | ||
| cp -r source-repo/api/docs/images/. docs-repo/docs/API/images/ | ||
| fi |
There was a problem hiding this comment.
Fix trailing whitespaces to address YAMLlint errors.
The static analysis tool detected trailing spaces on multiple lines that should be cleaned up for consistency.
Apply this diff to remove trailing whitespace:
- # Remove old API docs but preserve other folders
+ # Remove old API docs but preserve other folders
rm -rf docs-repo/docs/API/
mkdir -p docs-repo/docs/API
-
+
- # Copy all markdown files and maintain directory structure
+ # Copy all markdown files and maintain directory structure
cp -r source-repo/api/docs/public/. docs-repo/docs/API/
-
+
- # Clean and copy images directory specifically
+ # Clean and copy images directory specifically
rm -rf docs-repo/docs/API/images/
mkdir -p docs-repo/docs/API/images
-
+
- # Copy images from public/images if they exist
+ # Copy images from public/images if they exist
if [ -d "source-repo/api/docs/public/images" ]; then
cp -r source-repo/api/docs/public/images/. docs-repo/docs/API/images/
fi
-
+
- # Also copy any images from the parent docs/images directory
+ # Also copy any images from the parent docs/images directory
if [ -d "source-repo/api/docs/images" ]; then
cp -r source-repo/api/docs/images/. docs-repo/docs/API/images/
fi📝 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.
| # Remove old API docs but preserve other folders | |
| rm -rf docs-repo/docs/API/ | |
| mkdir -p docs-repo/docs/API | |
| # Copy all markdown files and maintain directory structure | |
| cp -r source-repo/api/docs/public/. docs-repo/docs/API/ | |
| # Clean and copy images directory specifically | |
| rm -rf docs-repo/docs/API/images/ | |
| mkdir -p docs-repo/docs/API/images | |
| # Copy images from public/images if they exist | |
| if [ -d "source-repo/api/docs/public/images" ]; then | |
| cp -r source-repo/api/docs/public/images/. docs-repo/docs/API/images/ | |
| fi | |
| # Also copy any images from the parent docs/images directory | |
| if [ -d "source-repo/api/docs/images" ]; then | |
| cp -r source-repo/api/docs/images/. docs-repo/docs/API/images/ | |
| fi | |
| # Remove old API docs but preserve other folders | |
| rm -rf docs-repo/docs/API/ | |
| mkdir -p docs-repo/docs/API | |
| # Copy all markdown files and maintain directory structure | |
| cp -r source-repo/api/docs/public/. docs-repo/docs/API/ | |
| # Clean and copy images directory specifically | |
| rm -rf docs-repo/docs/API/images/ | |
| mkdir -p docs-repo/docs/API/images | |
| # Copy images from public/images if they exist | |
| if [ -d "source-repo/api/docs/public/images" ]; then | |
| cp -r source-repo/api/docs/public/images/. docs-repo/docs/API/images/ | |
| fi | |
| # Also copy any images from the parent docs/images directory | |
| if [ -d "source-repo/api/docs/images" ]; then | |
| cp -r source-repo/api/docs/images/. docs-repo/docs/API/images/ | |
| fi |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 43-43: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 50-50: trailing spaces
(trailing-spaces)
[error] 55-55: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
.github/workflows/create-docusaurus-pr.yml around lines 40 to 59: several lines
contain trailing whitespace triggering YAMLlint errors; remove all trailing
spaces at the ends of lines in this block (including blank lines), ensure
indentation and line endings remain unchanged, and re-run the linter (or git
diff) to confirm no trailing whitespace remains before committing.
Summary by CodeRabbit
New Features
Documentation
Tests