feat: Add support for sub-path based serving#1430
Open
mdrkrg wants to merge 10 commits into
Open
Conversation
Add a plugin api-base to inject base URL at build time.
Contributor
WalkthroughIntroduces configurable app base path across backend and frontend: new WebConfig.AppBase with normalization, routes and OIDC flows updated to use AppBase, cookie paths derived from computed base URL, and multiple frontend URL builders and runtime settings adjusted to respect app.baseURL. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser as Browser
participant Frontend as Frontend (Nuxt app.baseURL)
participant Backend as Backend (API, routes use AppBase)
participant IdP as OIDC Provider
Browser->>Frontend: Click "Login with OIDC"
Frontend->>Backend: GET {app.baseURL}api/v1/users/login/oidc
Backend->>IdP: Redirect to IdP (redirect_uri includes API base)
IdP->>Browser: Redirect to IdP login
Browser->>IdP: Authenticate
IdP->>Backend: Callback to {app.baseURL}api/v1/users/oidc/callback
Backend->>Backend: handleCallback (set/clear cookies with Path from parsed base URL)
Backend->>Browser: Redirect to {app.baseURL}home or error query
Browser->>Frontend: Render post-login page
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Security Recommendations
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 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
requested review from
katosdev,
tankerkiller125 and
tonyaellie
coderabbitai
Bot
added
⬆️ enhancement
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/api/handlers/v1/v1_ctrl_auth.go (1)
190-238:⚠️ Potential issue | 🔴 CriticalUse consistent, configurable cookie paths for local authentication to match OIDC provider and prevent cookie leakage in subpath deployments.
Both
setCookies()andunsetCookies()hardcodePath: "/", while the OIDC provider correctly uses dynamicPath: u.Pathextracted from the request. This inconsistency creates security and functional issues:
- Cookie isolation in subpath deployments: With
Path: "/", cookies are scoped to the domain root, making them accessible to all applications at that domain (e.g., if deployed at/homebox/, tokens leak to/other-app/).- OIDC/local auth mismatch: The two auth paths set cookies with different paths, breaking interoperability in subpath deployments.
- Logout failures:
unsetCookies()also usesPath: "/", so clearing cookies may fail if the original path differs.Since
ctrl.config.Web.APIBaseis already available (and used elsewhere in this file at line 346), update both functions to use the configured API base path instead of hardcoding"/". This mirrors the OIDC provider's approach and ensures proper cookie isolation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/handlers/v1/v1_ctrl_auth.go` around lines 190 - 238, setCookies currently hardcodes Path: "/" which leaks cookies across subpaths; update setCookies to use the configured API base path (ctrl.config.Web.APIBase) for the Cookie.Path (mirroring the OIDC provider's use of u.Path) so cookies are scoped to the app subpath. Do the same in unsetCookies: replace Path: "/" with the same ctrl.config.Web.APIBase value used elsewhere in this file so setting and clearing cookies use the identical, configurable path and prevent cross-app leakage and logout failures. Ensure both functions reference ctrl.config.Web.APIBase (or a normalized variant if needed) for all cookies including the attachment token and session cookie.
🧹 Nitpick comments (1)
frontend/pages/index.vue (1)
22-22: Remove manual import - composables are auto-imported.As per coding guidelines, composables from the
composables/directory are auto-imported by Nuxt. This manual import is unnecessary.♻️ Proposed fix
- import { useAppBase } from "~/composables/use-app-base";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/index.vue` at line 22, The manual import of the composable is unnecessary: remove the line importing useAppBase from "~/composables/use-app-base" in frontend/pages/index.vue and rely on Nuxt's auto-imported composables so references to useAppBase in this file continue to work; ensure no other explicit imports of useAppBase remain in this file and run the dev build to verify no unresolved symbol errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/nuxt.config.ts`:
- Around line 3-4: The env-provided base paths (baseURL and devAPIBase) may lack
a trailing slash which causes later concatenation like base + "api" to produce
"/homeboxapi"; update the code that reads process.env.NUXT_APP_BASE_URL and
process.env.DEV_API_BASE to normalize values to always end with a single "/"
(treat empty or undefined as "/"), e.g., implement a small helper
(normalizeBasePath) and apply it to baseURL and devAPIBase so downstream string
concatenation (references to baseURL and devAPIBase in this file) is safe.
- Around line 66-70: The PWA API regex is constructed using baseURL with its
trailing slash, producing a double slash (e.g., ^//api); update the RegExp
builders used in navigateFallbackDenylist and runtimeCaching → urlPattern to
strip any trailing slash from baseURL before concatenating "/api" (e.g., use
baseURL with .replace to remove a trailing "/" or otherwise normalize it) so the
resulting patterns match "/api" correctly for both the default "/" and subpath
bases; ensure you change both navigateFallbackDenylist and the runtimeCaching
urlPattern occurrences.
In `@frontend/plugins/api-base.ts`:
- Around line 8-10: The plugin currently uses config.app.baseURL when calling
overrideParts in defineNuxtPlugin; change it to read a dedicated runtime public
API base (e.g., config.public.apiBase) instead: update the useRuntimeConfig()
usage and pass config.public.apiBase into overrideParts rather than
config.app.baseURL, and ensure nuxt.config exposes public.apiBase so frontend
API requests are decoupled from app.baseURL.
---
Outside diff comments:
In `@backend/app/api/handlers/v1/v1_ctrl_auth.go`:
- Around line 190-238: setCookies currently hardcodes Path: "/" which leaks
cookies across subpaths; update setCookies to use the configured API base path
(ctrl.config.Web.APIBase) for the Cookie.Path (mirroring the OIDC provider's use
of u.Path) so cookies are scoped to the app subpath. Do the same in
unsetCookies: replace Path: "/" with the same ctrl.config.Web.APIBase value used
elsewhere in this file so setting and clearing cookies use the identical,
configurable path and prevent cross-app leakage and logout failures. Ensure both
functions reference ctrl.config.Web.APIBase (or a normalized variant if needed)
for all cookies including the attachment token and session cookie.
---
Nitpick comments:
In `@frontend/pages/index.vue`:
- Line 22: The manual import of the composable is unnecessary: remove the line
importing useAppBase from "~/composables/use-app-base" in
frontend/pages/index.vue and rely on Nuxt's auto-imported composables so
references to useAppBase in this file continue to work; ensure no other explicit
imports of useAppBase remain in this file and run the dev build to verify no
unresolved symbol errors.
🪄 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: adc5c173-257f-45ad-a232-43fd0d69e0d6
📒 Files selected for processing (15)
.dockerignorebackend/app/api/handlers/v1/controller.gobackend/app/api/handlers/v1/v1_ctrl_auth.gobackend/app/api/providers/oidc.gobackend/app/api/routes.gobackend/internal/sys/config/conf.gofrontend/.env.examplefrontend/components/Collection/JoinModal.vuefrontend/composables/use-app-base.tsfrontend/composables/use-server-events.tsfrontend/lib/api/base/urls.tsfrontend/nuxt.config.tsfrontend/pages/collection/index/invites.vuefrontend/pages/index.vuefrontend/plugins/api-base.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
backend/app/api/providers/oidc.go (2)
458-465: Same defensive error handling recommended in handleCallback.The
url.Parseerror is also ignored here. For consistency and security robustness, handle the error.🛡️ Defensive error handling
// Helper to clear state cookie using computed domain baseURL := p.getBaseURL(r) - u, _ := url.Parse(baseURL) + u, err := url.Parse(baseURL) + if err != nil { + log.Err(err).Str("baseURL", baseURL).Msg("failed to parse base URL for OIDC callback") + return services.UserAuthTokenDetail{}, fmt.Errorf("internal server error") + } domain := u.Hostname()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/providers/oidc.go` around lines 458 - 465, The url.Parse error in handleCallback is currently ignored; update handleCallback to capture and handle the error returned by url.Parse(p.getBaseURL(r))—check the error, log or return a descriptive error (or wrap it) and avoid proceeding with a nil/invalid URL; ensure you still compute domain using u.Hostname() only after a successful parse and fall back to noPort(r.Host) when appropriate, similar to the defensive handling used elsewhere in the provider code.
405-411: Consider handling the url.Parse error for defensive security.The error from
url.Parse(baseURL)is discarded. WhilebaseURLis constructed internally and should always be valid, explicitly handling the error prevents unexpected behavior if future changes introduce edge cases. A malformed URL could result in an emptyu.Path, potentially scoping cookies to/instead of the intended subpath.🛡️ Defensive error handling
// Get base URL from request baseURL := p.getBaseURL(r) - u, _ := url.Parse(baseURL) + u, err := url.Parse(baseURL) + if err != nil { + log.Err(err).Str("baseURL", baseURL).Msg("failed to parse base URL for OIDC flow") + return services.UserAuthTokenDetail{}, fmt.Errorf("internal server error") + } domain := u.Hostname()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/providers/oidc.go` around lines 405 - 411, Handle the error returned by url.Parse when parsing baseURL (from p.getBaseURL(r)) instead of discarding it; check the returned error and, on parse failure, fall back to a safe default such as using noPort(r.Host) for domain or logging the parse error before using the fallback so that u.Path isn't relied on when malformed. Update the code around url.Parse(baseURL) and the subsequent use of u (variable u, domain, noPort, getBaseURL, and r.Host) to perform this defensive check and graceful fallback.backend/internal/sys/config/conf.go (1)
147-161: Address the static analysis finding: use compound assignment.The linter flagged line 158. This is a minor style issue but should be fixed to pass CI.
♻️ Use compound assignment operator
func normalizePath(p string) string { if p == "" { return "/" } if !strings.HasPrefix(p, "/") { p = "/" + p } if !strings.HasSuffix(p, "/") { - p = p + "/" + p += "/" } return p }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/sys/config/conf.go` around lines 147 - 161, The linter flagged a non-compound assignment in normalizePath; update the concatenation "p = p + \"/\"" to use the compound operator (p += "/") in the normalizePath function so behavior is unchanged and the style issue is resolved; ensure you only change that assignment and keep the existing checks for leading/trailing slashes in function normalizePath.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/app/api/providers/oidc.go`:
- Around line 458-465: The url.Parse error in handleCallback is currently
ignored; update handleCallback to capture and handle the error returned by
url.Parse(p.getBaseURL(r))—check the error, log or return a descriptive error
(or wrap it) and avoid proceeding with a nil/invalid URL; ensure you still
compute domain using u.Hostname() only after a successful parse and fall back to
noPort(r.Host) when appropriate, similar to the defensive handling used
elsewhere in the provider code.
- Around line 405-411: Handle the error returned by url.Parse when parsing
baseURL (from p.getBaseURL(r)) instead of discarding it; check the returned
error and, on parse failure, fall back to a safe default such as using
noPort(r.Host) for domain or logging the parse error before using the fallback
so that u.Path isn't relied on when malformed. Update the code around
url.Parse(baseURL) and the subsequent use of u (variable u, domain, noPort,
getBaseURL, and r.Host) to perform this defensive check and graceful fallback.
In `@backend/internal/sys/config/conf.go`:
- Around line 147-161: The linter flagged a non-compound assignment in
normalizePath; update the concatenation "p = p + \"/\"" to use the compound
operator (p += "/") in the normalizePath function so behavior is unchanged and
the style issue is resolved; ensure you only change that assignment and keep the
existing checks for leading/trailing slashes in function normalizePath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cd06096f-f8a0-4593-ba87-cd996851ec95
📒 Files selected for processing (6)
backend/app/api/handlers/v1/v1_ctrl_auth.gobackend/app/api/providers/oidc.gobackend/app/api/routes.gobackend/internal/sys/config/conf.gofrontend/.env.examplefrontend/nuxt.config.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/app/api/routes.go
- backend/app/api/handlers/v1/v1_ctrl_auth.go
- frontend/.env.example
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.
What type of PR is this?
What this PR does / why we need it:
Add support for serving the app under a subpath (e.g.,
https://example.com/homebox/).This enables admins who cannot control a
*.example.comdomain DNS but want to deploy multiple apps to serve all applications under different URL prefixes.Backend
conf.go: AddAPIBaseandFrontendBaseconfig fields (default/).routes.go: Mount API routes atAPIBase + /api + /v1instead of hardcoded/api/v1.oidc.go:getBaseURL()to includeAPIBasefor correct callback URLsFrontendBaseinstead of hardcoded/Pathfrom/tou.Pathso cookies work under subpathsFrontend
nuxt.config.ts: UseNUXT_APP_BASE_URLandDEV_API_BASEenvironment variable. Updates:app.baseURLfor subpath routingDEV_API_BASEworkbox,manifest.start_url)use-app-base.ts: Returnsconfig.app.baseURLfor use in componentsurl.ts: Include base URL in route generationfrontend/pages/index.vue: UseuseAppBase()to construct the OIDC login redirect URLuse-server-event.ts: UseuseAppBase()for correct WebSocket connection URLuseAppBase()inJoinModal.vueandinvites.vuefor correct invite URLsWhich issue(s) this PR fixes:
None, new feature.
Special notes for your reviewer:
APIBaseandFrontendBasein config. TheAPIBaseis used for both route registration and URL generation. TheFrontendBaseis only used for URL generation.Path: "/"works.Testing
Summary by CodeRabbit
New Features
Chores