feat: support custom pathToRegexpModule #102
Conversation
WalkthroughThe pull request introduces changes to middleware securities, package dependencies, and test configurations. The primary modifications include updating the Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Middleware as Security Middleware
participant Router as Route Handler
participant Controller as Controller
App->>Middleware: Configure Security Options
Middleware->>Router: Apply X-Frame Options
Router->>Controller: Route Request
Controller-->>Router: Generate Response
Router-->>Middleware: Apply Security Headers
Middleware-->>App: Return Secured Response
Poem
✨ Finishing Touches
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 提一个 issue 跟进此功能会 pick 到 master 分支 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Tip For best results, initiate chat on the files or code changes. @fengmk2 好的，我已经创建了一个 issue (#103) 来跟进这个功能会被 pick 到 master 分支。 🔗 Analysis chain@fengmk2 好的，我已经创建了一个 issue 来跟进这个功能会被 pick 到 master 分支。 🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 446 |
commit: |
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.x #102 +/- ##
======================================
Coverage ? 97.29%
======================================
Files ? 32
Lines ? 1367
Branches ? 344
======================================
Hits ? 1330
Misses ? 37
Partials ? 0 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/middleware/securities.js(2 hunks)package.json(2 hunks)test/fixtures/apps/iframe-with-pathToRegexpModule/app/router.js(1 hunks)test/fixtures/apps/iframe-with-pathToRegexpModule/config/config.js(1 hunks)test/fixtures/apps/iframe-with-pathToRegexpModule/package.json(1 hunks)test/xframe-with-pathToRegexpModule.test.js(1 hunks)test/xframe.test.js(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- test/fixtures/apps/iframe-with-pathToRegexpModule/package.json
- test/xframe.test.js
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18)
- GitHub Check: Node.js / Test (windows-latest, 14.20.0)
- GitHub Check: Node.js / Test (windows-latest, 16)
- GitHub Check: Node.js / Test (windows-latest, 14)
🔇 Additional comments (10)
test/fixtures/apps/iframe-with-pathToRegexpModule/config/config.js (2)
1-1: LGTM: Test key is appropriate for test environment.The test key value is suitable for a test configuration.
3-8: Verify security implications of ignored paths.The configuration ignores X-Frame-Options header for
/helloand/world/:idpaths. While this is a test configuration, it's worth noting that in production:
- Ignoring X-Frame-Options should be done with caution
- Dynamic routes (
:id) in security ignore lists require careful considerationLet's verify the usage of these ignore patterns across the codebase:
✅ Verification successful
Test-only configuration verified as safe
This is a test fixture configuration specifically designed to verify the X-Frame-Options middleware's path-to-regexp integration. The ignored paths are test examples and don't pose any security risks as they're isolated to the test environment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar security configurations with ignored paths rg -l "xframe.*ignore.*world|hello" --type jsLength of output: 1529
test/fixtures/apps/iframe-with-pathToRegexpModule/app/router.js (1)
10-12: LGTM: Controller implementation is correct.The async controller function correctly sets the response body.
app/middleware/securities.js (2)
16-18: LGTM: Fixed variable name typo.Corrected the variable name from
orginalCookieDomaintooriginalCookieDomain.
49-52: Verify spread operator usage with security options.The spread operator with security options requires careful review:
- Ensure no sensitive options are accidentally exposed
- Verify that pathToRegexpModule is properly sanitized
Let's verify the usage:
✅ Verification successful
Security review completed - spread operator and pathToRegexpModule usage is safe
The spread operator is used in a controlled context after proper validation of the opt object. The pathToRegexpModule is safely sourced from app configuration and used appropriately for URL pattern matching. No security concerns were identified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of security option spreading rg "\.\.\.opt" --type js # Check pathToRegexpModule usage rg "pathToRegexpModule.*=|=.*pathToRegexpModule" --type jsLength of output: 238
Script:
#!/bin/bash # Check createMatch implementation and usage ast-grep --pattern 'function createMatch' # Look for opt object usage and definition rg "opt\." --type js -A 2Length of output: 1411
test/xframe-with-pathToRegexpModule.test.js (2)
41-51: LGTM: Comprehensive security header testing.The tests thoroughly verify X-Frame-Options header behavior:
- Default SAMEORIGIN value
- Custom ALLOW-FROM value
- Different configurations across multiple apps
Also applies to: 61-66, 68-73
75-124: LGTM: Thorough path matching tests.Excellent test coverage for ignore patterns:
- Exact path matches
- Dynamic route parameters
- Query string handling
- Different path matching behaviors
package.json (3)
Line range hint
1-77: LGTM! Package structure and dependencies are well-organized.The changes maintain proper JSON structure and dependency organization.
46-47: Verify the path-to-regexp v8 dependency.The addition of path-to-regexp@8 as a dev dependency suggests it's used for testing the custom pathToRegexpModule feature.
Let's verify the package and check for any known issues:
✅ Verification successful
✓ The path-to-regexp v8 dependency is correctly configured and secure
The package is properly aliased, uses the latest major version (8.x.x) which has no known vulnerabilities, and its usage as a dev dependency aligns with testing purposes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if version 8 exists and get its publish date curl -s https://registry.npmjs.org/path-to-regexp | jq '.versions["8.0.0"]?.publish_time' # Get the latest version curl -s https://registry.npmjs.org/path-to-regexp | jq -r '."dist-tags".latest' # Check for any security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "path-to-regexp") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange } } }'Length of output: 1372
24-24: Verify the egg-path-matching version upgrade.The upgrade to egg-path-matching@^1.2.0 seems related to the custom pathToRegexpModule support feature.
Let's verify the version and check its changelog:
✅ Verification successful
Version upgrade to egg-path-matching@^1.2.0 is correct and aligned with the PR feature
The upgrade corresponds to the newly released version that specifically adds support for custom pathToRegexpModule, which is the main feature of this PR.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if version 1.2.0 exists and get its publish date curl -s https://registry.npmjs.org/egg-path-matching | jq '.versions["1.2.0"]?.publish_time' # Get the latest version curl -s https://registry.npmjs.org/egg-path-matching | jq -r '."dist-tags".latest' # Check for any security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "egg-path-matching") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange } } }'Length of output: 486
Script:
#!/bin/bash # List all available versions curl -s https://registry.npmjs.org/egg-path-matching | jq -r '.versions | keys[]' # Get repository URL curl -s https://registry.npmjs.org/egg-path-matching | jq -r '.repository.url'Length of output: 251
Script:
#!/bin/bash # Fetch release notes for v1.2.0 gh api repos/eggjs/egg-path-matching/releases/tags/v1.2.0 # As fallback, get commit history around the tag gh api repos/eggjs/egg-path-matching/commits?per_page=5&path=CHANGELOG.mdLength of output: 25318
| async function options(ctx, next) { | ||
| ctx.securityOptions.xframe = { | ||
| value: 'ALLOW-FROM http://www.domain.com', | ||
| }; | ||
| return next(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review security implications of ALLOW-FROM directive.
The middleware sets X-Frame-Options to ALLOW-FROM http://www.domain.com. Note that:
- ALLOW-FROM is deprecated in modern browsers
- Consider using CSP frame-ancestors instead
Consider updating to use Content-Security-Policy:
async function options(ctx, next) {
- ctx.securityOptions.xframe = {
- value: 'ALLOW-FROM http://www.domain.com',
- };
+ ctx.securityOptions.csp = {
+ value: "frame-ancestors 'self' http://www.domain.com"
+ };
return next();
}📝 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.
| async function options(ctx, next) { | |
| ctx.securityOptions.xframe = { | |
| value: 'ALLOW-FROM http://www.domain.com', | |
| }; | |
| return next(); | |
| } | |
| async function options(ctx, next) { | |
| ctx.securityOptions.csp = { | |
| value: "frame-ancestors 'self' http://www.domain.com" | |
| }; | |
| return next(); | |
| } |
| // '/hello' won't match '/hello/other/world' on path-to-regexp@8 | ||
| res = await app.httpRequest() | ||
| .get('/hello/other/world') | ||
| .set('accept', 'text/html') | ||
| .expect(200); | ||
| assert.equal(res.headers['x-frame-options'], 'SAMEORIGIN'); |
There was a problem hiding this comment.
💡 Codebase verification
Breaking change in path-to-regexp v8 path matching needs documentation
The change in path matching behavior where '/hello' no longer matches '/hello/other/world' in path-to-regexp v8 is not documented in the CHANGELOG.md. This is a significant breaking change that could affect applications upgrading to the new version.
Please update the CHANGELOG.md to include:
- The path matching behavior change in path-to-regexp v8
- Examples of affected patterns
- Migration guidance for users updating their routes
🔗 Analysis chain
Document breaking change in path matching behavior.
Important behavioral change noted in the test:
'/hello' won't match '/hello/other/world' on path-to-regexp@8
This is a breaking change that should be documented in the changelog.
Let's check if this is documented:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for changelog or documentation updates
rg -l "path-to-regexp.*8|breaking.*change" --type md
Length of output: 54
Script:
#!/bin/bash
# Check package.json for path-to-regexp version
fd package.json --type f --exec cat {} \; | jq -r 'select(.dependencies["path-to-regexp"] or .devDependencies["path-to-regexp"]) | {name: .name, dependencies: .dependencies["path-to-regexp"], devDependencies: .devDependencies["path-to-regexp"]}'
# Search for documentation files with broader patterns
fd -e md -e txt -e rst | grep -i -E 'change|upgrade|migration|release|history|breaking' || true
Length of output: 353
Script:
#!/bin/bash
# Check CHANGELOG.md content
echo "=== Checking CHANGELOG.md for path-to-regexp mentions ==="
rg -i "path-to-regexp" CHANGELOG.md -C 2 || true
echo -e "\n=== Checking for breaking changes sections ==="
rg "BREAKING|Breaking Changes" CHANGELOG.md -C 2 || true
Length of output: 386
[skip ci] ## [3.8.0](v3.7.0...v3.8.0) (2025-01-22) ### Features * support custom pathToRegexpModule ([#102](#102)) ([8e12a8e](8e12a8e))
part of eggjs/core#290
Summary by CodeRabbit
Dependencies
egg-path-matchingto version 1.2.0path-to-regexp-v8dependencySecurity Enhancements
Testing