Opportunity to set base #41
Conversation
WalkthroughThe changes involve modifications to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (2)
packages/remix-express-dev-server/src/index.ts (2)
36-42: Consider adding base path validation and handling edge casesWhile the implementation is generally good, consider adding validation for the base path format and handling edge cases:
- The base path might need normalization similar to appDirectory
- There could be double slash issues if the base ends with a slash
Consider adding a normalization function for the base path:
+ function normalizeBasePath(base: string) { + // remove trailing slash + base = base.replace(/\/+$/, '') + // ensure leading slash + if (!base.startsWith('/')) base = `/${base}` + return base + } const appDirectory = normalizeAppDirectory( options?.appDirectory ?? defaultOptions.appDirectory, ) - const base = options?.base ?? defaultOptions.base + const base = normalizeBasePath(options?.base ?? defaultOptions.base)
Line range hint
6-11: Consider adding documentation for the base optionThe implementation looks good, but it would be helpful to add JSDoc comments explaining:
- The purpose and usage of the
baseoption- Expected format and constraints
- Example configurations
Add documentation like this:
export type DevServerOptions = { entry?: string exportName?: string appDirectory?: string + /** + * Base path for the application routes. + * @example + * // For an app mounted at /my-app + * base: '/my-app' + */ base?: string configureServer?: (server: http.Server) => void }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/remix-express-dev-server/src/index.ts(4 hunks)
🔇 Additional comments (2)
packages/remix-express-dev-server/src/index.ts (2)
10-10: LGTM: Type definition and default value are well implemented
The addition of the optional base property with a default empty string value is clean and follows TypeScript best practices.
Also applies to: 18-18
60-63: LGTM: URL exclusion patterns properly handle base path
The exclusion patterns are well implemented with proper RegExp usage and correctly incorporate the base path prefix.
| @@ -69,7 +76,7 @@ export function expressDevServer(options?: DevServerOptions): VitePlugin { | |||
| // check if url is a physical file in the app directory | |||
| if (appDirectoryPattern.test(req.url!)) { | |||
| const url = new URL(req.url!, 'http://localhost') | |||
| if (fs.existsSync(url.pathname.slice(1))) { | |||
| if (fs.existsSync(url.pathname.replace(base, "").slice(1))) { | |||
There was a problem hiding this comment.
💡 Codebase verification
Add path traversal protection to file existence check
The file existence check is part of a middleware that handles physical files in the app directory. While there's a pattern test (appDirectoryPattern.test) that provides some validation, there's no explicit protection against path traversal attacks. The suggested improvements are valid and necessary:
- The current implementation directly uses the URL pathname after base removal
- No error handling for malformed paths or filesystem errors
- Missing path normalization and traversal checks
- if (fs.existsSync(url.pathname.replace(base, "").slice(1))) {
+ const normalizedPath = path.normalize(url.pathname.replace(base, "").slice(1))
+ if (normalizedPath.includes('..')) {
+ return next(new Error('Path traversal not allowed'))
+ }
+ try {
+ if (fs.existsSync(normalizedPath)) {
+ return next()
+ }
+ } catch (error) {
+ console.error('Error checking file existence:', error)
+ return next(error)
+ }🔗 Analysis chain
Consider adding path traversal protection and improved error handling
While the base path removal works correctly, consider these security and reliability improvements:
- Add path traversal protection
- Add error handling for malformed URLs
- Consider using path.normalize for consistent path handling
Here's a suggested improvement:
- if (fs.existsSync(url.pathname.replace(base, "").slice(1))) {
+ const normalizedPath = path.normalize(url.pathname.replace(base, "").slice(1))
+ if (normalizedPath.includes('..')) {
+ return next(new Error('Path traversal not allowed'))
+ }
+ try {
+ if (fs.existsSync(normalizedPath)) {
+ return next()
+ }
+ } catch (error) {
+ console.error('Error checking file existence:', error)
+ return next(error)
+ }Let's verify if there are any existing security measures:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for path traversal protection in the codebase
rg -l "path\.normalize|\.\./" --type ts
Length of output: 41
Script:
#!/bin/bash
# Let's check for any existing path-related security measures using broader patterns
rg -A 3 "pathname|\.replace\(.*base" --type ts
# Also check for any URL or path validation
rg -A 3 "validatePath|sanitize|isValid.*Path" --type ts
# Look at the complete context of the file existence check
ast-grep --pattern 'if (fs.existsSync($_)) { $$$ }'
Length of output: 2790
Script:
#!/bin/bash
# Let's examine the complete middleware context to understand the security measures
rg -B 5 -A 10 "if \(fs\.existsSync\(url\.pathname" packages/remix-express-dev-server/src/index.ts
# Check if there's any URL sanitization happening before this point
ast-grep --pattern 'function $_(url: URL, $$$) { $$$ }'
Length of output: 674
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/remix-create-express-app/src/index.ts (2)
Line range hint
93-105: Ensure proper path normalization and security.The static file serving setup looks good, but there are some security considerations to address:
- Path normalization is important to prevent path traversal attacks
- The order of middleware (assets before general static files) is correct
Consider adding path normalization:
+ import { normalize } from 'path' // ... app.use( - `${basePath}/assets`, + normalize(`${basePath}/assets`).replace(/\\/g, '/'), express.static(`${buildDirectory}/client/assets`, { immutable: true, maxAge: '1y', }), ) app.use( - `${basePath}`, + normalize(basePath).replace(/\\/g, '/'), express.static(isProductionMode ? `${buildDirectory}/client` : 'public', { maxAge: '1h', }), )
Line range hint
40-105: Consider additional architectural implications.While the base path implementation is solid, consider documenting or addressing these scenarios:
- Interaction with reverse proxies (e.g., nginx with location blocks)
- WebSocket endpoint handling with base paths
- Nested Express.js applications (using
app.use(basePath, subApp))Would you like me to help create documentation covering these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/remix-create-express-app/src/index.ts(4 hunks)
🔇 Additional comments (1)
packages/remix-create-express-app/src/index.ts (1)
40-40: LGTM! Verify type consistency across the codebase.
The basePath type definition is correctly added as an optional string parameter.
Let's verify the consistency of basePath/base usage across the codebase:
✅ Verification successful
Type consistency for basePath parameter is verified
The verification shows that basePath is used consistently within the codebase:
- Defined as an optional string parameter
- Used with a default empty string value
- Applied correctly in static asset paths
- No conflicting or inconsistent type definitions found in other parts of the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any inconsistent base path type definitions
ast-grep --pattern 'type $_ = {
$$$
base: $_
$$$
}'
# Search for any other base path related configurations
rg -g '*.ts' -g '*.js' 'basePath|base.*Path'
Length of output: 375
| @@ -48,6 +49,7 @@ export async function createExpressApp({ | |||
| unstable_middleware, | |||
| buildDirectory = 'build', | |||
| serverBuildFile = 'index.js', | |||
| basePath = "" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for basePath parameter.
While the default empty string is appropriate, consider adding validation to ensure the basePath:
- Starts with a forward slash if non-empty
- Doesn't end with a trailing slash
- Contains valid URL characters
Consider adding this validation:
export async function createExpressApp({
// ... other params
basePath = ""
}: CreateExpressAppArgs = {}): Promise<Application> {
+ if (basePath) {
+ if (!basePath.startsWith('/')) {
+ throw new Error('basePath must start with "/"');
+ }
+ if (basePath.endsWith('/')) {
+ throw new Error('basePath must not end with "/"');
+ }
+ if (!/^[a-zA-Z0-9\-._~!$&'()*+,;=:@/]*$/.test(basePath)) {
+ throw new Error('basePath contains invalid characters');
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/remix-express-dev-server/src/index.ts (1)
36-42: Consider adding tests for base path scenariosWhile the implementation correctly supports base paths, consider adding tests for:
- Various base path configurations
- Edge cases (empty base, multiple slashes)
- Security scenarios (path traversal attempts)
- Integration with Vite's own base configuration
This will ensure robust handling of base paths across different deployment scenarios.
Also applies to: 79-79
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/remix-express-dev-server/src/index.ts(4 hunks)
🔇 Additional comments (5)
packages/remix-express-dev-server/src/index.ts (5)
10-11: LGTM: Type definition for base option
The addition of the optional base property to DevServerOptions is well-typed and aligns with Vite's configuration pattern.
18-19: LGTM: Sensible default for base option
Empty string as default maintains backward compatibility and matches Vite's default behavior.
36-42: LGTM: Proper pattern composition for base path
The pattern construction is safe and correct:
- Uses escapeRegExp for regex safety
- Maintains proper order (base then app directory)
- Correctly composes the patterns
60-63: LGTM: Updated exclusion patterns
The exclude patterns are correctly updated to handle base path for Vite's internal routes.
79-79:
Add path traversal protection to file existence check
The file existence check needs additional security measures when handling base paths.
Apply this diff to add path traversal protection:
- if (fs.existsSync(url.pathname.replace(base, "").slice(1))) {
+ const normalizedPath = path.normalize(url.pathname.replace(base, "").slice(1))
+ if (normalizedPath.includes('..')) {
+ return next(new Error('Path traversal not allowed'))
+ }
+ try {
+ if (fs.existsSync(normalizedPath)) {
+ return next()
+ }
+ } catch (error) {
+ console.error('Error checking file existence:', error)
+ return next(error)
+ }
|
CodeRabbit is definitely thorough. I can see where those options are useful. Thanks for the PR. Can you update the README to document those additions? |
|
@kiliman I have update the README files |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
packages/remix-create-express-app/README.md (2)
82-85: Documentation for basePath option needs more detailsWhile the addition of the
basePathoption is documented, it would be helpful to include:
- An example showing how to use the
basePathoption- Clarification on how it affects static asset serving
- The relationship between Vite's
baseconfiguration and Remix's routingConsider adding a usage example in the Examples section:
export const app = createExpressApp({ // If your app is served from a subdirectory like example.com/myapp basePath: '/myapp', configure: app => { app.use(compression()) // Static assets will be served from /myapp/public // Routes will be prefixed with /myapp } })
Line range hint
3-13: Remove duplicate contributors badge sectionThere are two identical ALL-CONTRIBUTORS-BADGE sections with different contributor counts. Please remove one of them to avoid confusion.
Keep only one badge section:
# remix-create-express-app <!-- ALL-CONTRIBUTORS-BADGE:START - Do not remove or modify this section --> [](#contributors-) <!-- ALL-CONTRIBUTORS-BADGE:END --> -<!-- ALL-CONTRIBUTORS-BADGE:START - Do not remove or modify this section --> -[](#contributors-) -<!-- ALL-CONTRIBUTORS-BADGE:END -->packages/remix-express-dev-server/README.md (3)
47-47: Enhance the documentation for thebaseoptionWhile the basic description is provided, it would be helpful to add:
- A practical example showing how to use the
baseoption- Common use cases (e.g., serving the app from a subdirectory)
- The relationship between this option and Vite/Remix base configuration
Consider adding this example section:
base?: string // base of your app without a trailing slash: default = '' (empty string) + // Example: + // If your app is served from https://example.com/my-app + // expressDevServer({ + // base: '/my-app' // no trailing slash + // })
Line range hint
1-124: Document thebasePathfeature for static assetsThe PR adds support for
basePathin thecreateExpressAppfunction for serving static assets, but this feature is not documented. Please add a section explaining:
- The
basePathoption increateExpressApp- How it affects static asset serving
- Its relationship with the new
baseoptionConsider adding this new section before the "Advanced configuration" heading:
### Static Assets Configuration When creating your Express app, you can specify a `basePath` for serving static assets: ```ts createExpressApp({ basePath: '/my-app/assets', // Base path for static files // ... other options })This is particularly useful when your app is served from a subdirectory and you need to adjust the static assets path accordingly.
--- Line range hint `1-124`: **Consider adding a "Common Configurations" section** To help users quickly find solutions for typical setups, consider adding a new section that shows complete configuration examples for common scenarios, such as: 1. Basic setup with default options 2. Setup with base URL configuration 3. Setup with static assets configuration 4. Combined setup with both base URL and static assets This would make it easier for users to understand how different options work together and find the configuration that best matches their needs. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 634ac86d3a89c6dc785ba5a4006457ee615db52b and 1369a0543165899dae361d740c22deca89509835. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `packages/remix-create-express-app/README.md` (1 hunks) * `packages/remix-express-dev-server/README.md` (1 hunks) </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
can we get a bump on this one |
I have a setup where I need to have base/baseName for Vite/Remix. expressDevServer doesn't really support such setup. So I think I have fixed it. In addition createExpressApp should have basePath for assets and other static files
Summary by CodeRabbit
Summary by CodeRabbit
New Features
baseproperty for configuring a base URL in the server options.basePathproperty for specifying a base path in the application setup.Improvements
basePath, allowing for more flexible URL structures.Documentation