fix(cli,ui): add consistent basePath handling for subpath deployments (N8N_PATH) - #19638
fix(cli,ui): add consistent basePath handling for subpath deployments (N8N_PATH)#19638skyplane23 wants to merge 28 commits into
Conversation
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="packages/cli/src/server.ts">
<violation number="1" location="packages/cli/src/server.ts:325">
This change removes authentication from the `/types/*.json` endpoints, making them publicly accessible. This is a security regression that discloses potentially sensitive information about the instance's configured node and credential types. These endpoints should be protected by authentication as they were previously.</violation>
</file>
<file name="packages/cli/src/push/index.ts">
<violation number="1" location="packages/cli/src/push/index.ts:74">
Using pathname.includes may match unintended subpaths; use endsWith (with optional trailing slash) to target the exact push endpoint.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
|
Hey @skyplane23, Thank you for your contribution. We appreciate the time and effort you’ve taken to submit this pull request. Before we can proceed, please ensure the following: Regarding new nodes: If your node integrates with an AI service that you own or represent, please email nodes@n8n.io and we will be happy to discuss the best approach. About review timelines: Thank you again for contributing to n8n. |
|
Hi @skyplane23 Thank you for your PR. Really appreciate it. You are correct that the current support for deploying n8n under a subpath is quite lacking, and something we want to fix. Unfortunately this is breaking change for existing users who use the env var and have configured a reverse proxy to strip it out. We are currently evaluating how to handle this, but I'm personally leaning more towards introducing a new env var that adds proper support, and deprecate Regarding the implementation, we also need to scope the auth cookie to the configured subpath. |
|
@skyplane23 we have decided the following:
So to get this merged, the following changes are needed in your PR:
We also need e2e tests for this:
Make sure the current behaviour with If you have any questions let me know! |
Ok, I will start working on it ASAP. |
624ac9e to
fa487bd
Compare
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
fa487bd to
e9db540
Compare
|
Hi! @tomi I've implemented all the requested changes: Summary
All path construction throughout the codebase has been centralized into Before mergingI'd like to run the Playwright E2E tests locally to verify the custom-base-path tests pass. Could you point me to the recommended way to run them? I tried in my dev container but hit compatibility issues with Alpine Linux. QuestionThe PR is currently outdated. Since v2 has been released, which branch should I rebase from? Let me know if you have any questions! |
There was a problem hiding this comment.
2 issues found across 32 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/__test__/templates.test.ts">
<violation number="1" location="packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/__test__/templates.test.ts:346">
P2: Test claims to cover missing basePath but still passes basePath via defaultParams, so defaulting logic isn’t exercised</violation>
</file>
<file name="packages/cli/src/public-api/index.ts">
<violation number="1" location="packages/cli/src/public-api/index.ts:114">
P2: Rule violated: **Prefer Typeguards over Type casting**
Avoid using `as` for type narrowing; use a type annotation instead (Rule: Prefer Typeguards over Type casting).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
61fd862 to
30383d8
Compare
There was a problem hiding this comment.
All reported issues were addressed across 28 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 5 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 10 unresolved issues from previous reviews.
Re-trigger cubic
- Added basePath property to AbstractServer for dynamic endpoint management. - Updated health check and webhook routes to utilize basePath. - Modified ControllerRegistry to construct prefixes using basePath. - Adjusted LoadNodesAndCredentials to resolve icons with basePath. - Refactored Server class to pass basePath to various endpoint handlers. - Enhanced Push class to support basePath in push endpoint setup.
…ket connection handling
…h for dynamic endpoint handling
…use dynamic basePath
…amic URL handling
…ndpoint path resolution across the application - Added PathResolvingService to encapsulate logic for resolving full endpoint paths with the configured base path. - Updated various components to utilize PathResolvingService for consistent URL construction, including server routes, public API endpoints, and static asset paths. - Enhanced tests to verify correct behavior under custom base paths.
…ceBaseUrl for URL handling - Set default value for instanceBaseUrl in createPage function to '/'. - Refactor tests to replace basePath with instanceBaseUrl for clarity and consistency in redirect URL handling.
…h for dynamic endpoint handling
…ndpoint path resolution across the application - Added PathResolvingService to encapsulate logic for resolving full endpoint paths with the configured base path. - Updated various components to utilize PathResolvingService for consistent URL construction, including server routes, public API endpoints, and static asset paths. - Enhanced tests to verify correct behavior under custom base paths.
…ew isolated test for custom base path functionality - Deleted the old custom-base-path.spec.ts file which contained outdated tests. - Introduced a new test file that verifies n8n's behavior when deployed under a custom base path, ensuring UI and API functionality is intact. - The new tests run in an isolated container to apply the custom base path environment variable.
…unction - Updated createPage function to ensure instanceBaseUrl is consistently normalized by adding a trailing slash if missing. - Refactored related tests to verify correct URL handling for both absolute and relative instanceBaseUrl values, ensuring proper redirection and fetch requests.
- Updated the base URL construction to include a configurable base path, allowing n8n to be hosted under a custom path. - Adjusted main container URL generation to append the base path, ensuring correct URL resolution for both UI and API interactions.
- Introduced new tests to verify n8n's behavior under a custom base path, including UI serving, health checks, and webhook trigger routes. - Refactored existing tests to utilize a constant for the base path, improving maintainability and clarity. - Ensured that routes are correctly bound to the configured base path, preventing accidental exposure at the root.
…ht tests - Replaced the static wait strategy with a dynamic function to accommodate custom base paths in n8n. - Updated navigation and API request handling to support base path configuration across various components. - Improved test cases to validate behavior under custom base paths, ensuring correct URL resolution and functionality.
…solution
- Updated LoadNodesAndCredentials to handle root basePath ('/') correctly, preventing incorrect URL slicing.
- Added a test case to verify the correct behavior when resolving icons with a root basePath.
- Enhanced path resolution logic in PathResolvingService to accommodate customized health endpoints.
- Updated the normalizePathSegment function to strip multiple trailing slashes instead of just one, improving path normalization. - Added new test cases to verify the correct behavior of normalizeBasePath when handling multiple trailing slashes.
- Removed unused Logger import from controller.registry.ts to streamline the code. - Updated the icon resolution test to correctly handle the root basePath, ensuring accurate URL generation for icons.
- Added `assertPathAndBasePathAreNotBothSet` to enforce configuration rules for `N8N_PATH` and `N8N_BASE_PATH`. - Updated `normalizeBasePath` to improve handling of base paths, ensuring correct normalization. - Removed outdated tests and added new tests for `normalizeBasePath` and the new assertion function, verifying behavior under various configurations. - Refactored related components to utilize the updated path resolution logic, ensuring consistent URL handling across the application.
- Introduced basePath property to GlobalConfig across multiple test files to ensure consistent handling of base paths. - Updated mock configurations in tests to include basePath, enhancing test coverage for path-related functionality. - Ensured that all relevant services and commands reflect the new basePath property for improved URL resolution.
…der custom base paths - Modified the server to mount the overwrite endpoint middleware using the resolved path, ensuring compatibility when n8n is hosted under a custom base path. - Removed hardcoded path usage to enhance flexibility and maintainability in URL handling.
…th resolution - Modified the authorization endpoint in AuthService to utilize a dynamic prefix, ensuring correct URL generation under custom base paths. - This change enhances flexibility and maintains consistency with recent updates to base path handling across the application.
…se path resolution - Updated the DynamicCredentialsController to use PathResolvingService for constructing return URLs, ensuring correct handling of base paths. - Enhanced tests to verify the behavior of the sign-in return URL generation, preventing duplication of the base path in the final URL.
- Modified the AbstractServer class to utilize PathResolvingService for generating the webhook waiting endpoint, enhancing flexibility in URL handling under custom base paths.
- Added support for rendering static image URLs based on the configured base path in the N8nMarkdown component. - Introduced a new global type declaration for BASE_PATH to facilitate dynamic path resolution. - Enhanced tests to verify correct rendering of images under both root and configured base paths.
…solution - Updated the Server class to use PathResolvingService for resolving base paths and API endpoints, enhancing flexibility in URL handling. - Simplified the configuration logic by removing hardcoded paths and ensuring consistent path resolution across various endpoints. - Adjusted tests to verify the correct behavior of cookie clearing in the AuthService, ensuring proper path handling.
…mic base path resolution - Updated the ChatTrigger templates to utilize dynamic base path resolution for login and sign-in URLs, enhancing flexibility and consistency. - Adjusted related tests to verify the correct behavior of URL generation under various base path configurations, ensuring proper escaping of special characters.
…n redirects - Added middleware to extract BrowserId from request headers for easier access in the Server class. - Refactored the DynamicCredentialsController to correctly append the base path to the sign-in URL when the editor URL omits it, ensuring proper URL resolution. - Updated tests to reflect changes in sign-in URL generation and verify correct behavior under various base path configurations.
Summary
This PR adds first-class subpath support by consistently honoring
N8N_PATH(akabasePath) across the backend and frontend. It fixes the long-standing issue where n8n appears to work only with reverse-proxy rewrites (stripping the subpath) and breaks features that depend on exact URLs.What changed
Backend (CLI/server):
basePathinAbstractServerand propagate it everywhere routes are registered./, ensure leading/) and reuse for all mounts.basePath:this.app.use('${basePath}/', historyApiHandler, express.static(...))basePath(resolveIcon(basePath, packageName, req.originalUrl)).basePath:/${basePath}/${restEndpoint}/schemas/:node/:version/:resource/:operation.json${basePath}/healthzand${basePath}/healthz/readiness.basePath:${basePath}/${endpointForm}/*path${basePath}/${endpointWebhook}/*path${basePath}/${endpointFormWaiting},${basePath}/${endpointWebhookWaiting}${basePath}/${endpointMcp}/*path,${basePath}/${endpointMcpTest}/*path${basePath}/${restEndpoint}/test-webhook/:idbasePath:${basePath}/${restEndpoint}/pushpathname.includes(\/${restEndpoint}/push`)withbasePath` awareness.basePath(and normalizes it) instead of hardcoding root/..some(skipEndpoint => endpoint.includes(skipEndpoint))for robustness withbasePath.restEndpoint.)Frontend (editor-ui):
window.BASE_PATHwhen building the fallback redirect.const redirect = to.query.redirect ?? encodeURIComponent(\${url.pathname.replace(new RegExp(`^${window.BASE_PATH}`), '/')}${url.search}`);`Key effect: All UI, API, push, schema, health, icons, and webhook endpoints are now mounted relative to
N8N_PATH. Reverse-proxy rewrite hacks are no longer required for subpath deployments.Why
When n8n is hosted at
/my-n8n, many routes ignoredN8N_PATH. Users could mask this via proxy rewrites, but Human-in-the-Loop (HITL) and other URL-sensitive features would fail because the browser used a subpath URL while the server verified against a rewritten (root) URL. CentralizingbasePathand applying it consistently prevents these mismatches.How to test
Environment
Start n8n without any reverse-proxy path rewrites (no stripping
/my-n8n).Editor UI
http://localhost:5678/my-n8n//my-n8n.Static assets & icons
${basePath}/icons/....Health endpoints
GET http://localhost:5678/my-n8n/healthz→{ status: "ok" }GET http://localhost:5678/my-n8n/healthz/readiness→ 200 once DB ready.REST & schemas
${basePath}/${restEndpoint}/schemas/...resolves.Push/SSE/WS
${basePath}/${restEndpoint}/push.Webhooks & forms (incl. HITL)
${basePath}and function./my-n8n. Signature verification should succeed (no mismatch).Test webhook deletion
DELETE ${basePath}/${restEndpoint}/test-webhook/:idworks.Breaking/behavior changes
${basePath}:/healthz→${basePath}/healthz/healthz/readiness→${basePath}/healthz/readinessIf you previously relied on root-mounted paths while also setting
N8N_PATH, update any external monitors or allowlists accordingly.Backwards compatibility:
With
N8N_PATHunset or empty, normalizedbasePathis'', preserving current root behavior.Related Linear tickets, Github issues, and Community forum posts
closes #19635(CAT-1439) — “Subpath deployment via N8N_PATH breaks: endpoints ignore basePath and Human-in-the-Loop fails”.Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)