feat: support custom pathToRegexpModule #102
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| module.exports = function(app) { | ||
| app.get('/', controller); | ||
| app.get('/foo', controller); | ||
| app.get('/hello', controller); | ||
| app.get('/hello/other/world', controller); | ||
| app.get('/world/12', controller); | ||
|
|
||
| app.get('/options', options, controller); | ||
|
|
||
| async function controller() { | ||
| this.body = 'body'; | ||
| } | ||
|
|
||
| async function options(ctx, next) { | ||
| ctx.securityOptions.xframe = { | ||
| value: 'ALLOW-FROM http://www.domain.com', | ||
| }; | ||
| return next(); | ||
| } | ||
| }; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| exports.keys = 'test key'; | ||
|
|
||
| exports.security = { | ||
| defaultMiddleware: 'xframe', | ||
| xframe: { | ||
| ignore: ['/hello', '/world/:id'], | ||
| }, | ||
| }; |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| { | ||
| "name": "iframe-with-pathToRegexpModule" | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| const { strict: assert } = require('node:assert'); | ||
| const mm = require('egg-mock'); | ||
|
|
||
| describe('test/xframe-with-pathToRegexpModule.test.js', () => { | ||
| let app; | ||
| let app2; | ||
| let app3; | ||
| let app4; | ||
| before(async () => { | ||
| app = mm.app({ | ||
| baseDir: 'apps/iframe-with-pathToRegexpModule', | ||
| plugin: 'security', | ||
| pathToRegexpModule: require.resolve('path-to-regexp-v8'), | ||
| }); | ||
| await app.ready(); | ||
|
|
||
| app2 = mm.app({ | ||
| baseDir: 'apps/iframe-novalue', | ||
| plugin: 'security', | ||
| pathToRegexpModule: require.resolve('path-to-regexp-v8'), | ||
| }); | ||
| await app2.ready(); | ||
|
|
||
| app3 = mm.app({ | ||
| baseDir: 'apps/iframe-allowfrom', | ||
| plugin: 'security', | ||
| pathToRegexpModule: require.resolve('path-to-regexp-v8'), | ||
| }); | ||
| await app3.ready(); | ||
|
|
||
| app4 = mm.app({ | ||
| baseDir: 'apps/iframe-black-urls', | ||
| plugin: 'security', | ||
| pathToRegexpModule: require.resolve('path-to-regexp-v8'), | ||
| }); | ||
| await app4.ready(); | ||
| }); | ||
|
|
||
| afterEach(mm.restore); | ||
|
|
||
| it('should contain X-Frame-Options: SAMEORIGIN', async () => { | ||
| await app.httpRequest() | ||
| .get('/') | ||
| .set('accept', 'text/html') | ||
| .expect('x-frame-options', 'SAMEORIGIN'); | ||
|
|
||
| await app.httpRequest() | ||
| .get('/foo') | ||
| .set('accept', 'text/html') | ||
| .expect('x-frame-options', 'SAMEORIGIN'); | ||
| }); | ||
|
|
||
| it('should contain X-Frame-Options: ALLOW-FROM http://www.domain.com by this.securityOptions', async () => { | ||
| const res = await app.httpRequest() | ||
| .get('/options') | ||
| .set('accept', 'text/html'); | ||
| assert.equal(res.status, 200); | ||
| assert.equal(res.headers['x-frame-options'], 'ALLOW-FROM http://www.domain.com'); | ||
| }); | ||
|
|
||
| it('should contain X-Frame-Options: SAMEORIGIN when dont set value option', function(done) { | ||
| app2.httpRequest() | ||
| .get('/foo') | ||
| .set('accept', 'text/html') | ||
| .expect('x-frame-options', 'SAMEORIGIN', done); | ||
| }); | ||
|
|
||
| it('should contain X-Frame-Options: ALLOW-FROM with page when set ALLOW-FROM and page option', function(done) { | ||
| app3.httpRequest() | ||
| .get('/foo') | ||
| .set('accept', 'text/html') | ||
| .expect('x-frame-options', 'ALLOW-FROM http://www.domain.com', done); | ||
| }); | ||
|
|
||
| it('should not contain X-Frame-Options: SAMEORIGIN when use ignore', async () => { | ||
| let res = await app.httpRequest() | ||
| .get('/hello') | ||
| .set('accept', 'text/html') | ||
| .expect(200); | ||
| assert.equal(res.headers['x-frame-options'], undefined); | ||
|
|
||
| // '/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'); | ||
|
Comment on lines
+82
to
+87
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:
🔗 Analysis chainDocument breaking change in path matching behavior. Important behavioral change noted in the test:
This is a breaking change that should be documented in the changelog. Let's check if this is documented: 🏁 Scripts executedThe 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 |
||
|
|
||
| res = await app4.httpRequest() | ||
| .get('/hello') | ||
| .set('accept', 'text/html') | ||
| .expect(200); | ||
| assert.equal(res.headers['x-frame-options'], undefined); | ||
|
|
||
| res = await app.httpRequest() | ||
| .get('/world/12') | ||
| .set('accept', 'text/html') | ||
| .expect(200); | ||
| assert.equal(res.headers['x-frame-options'], undefined); | ||
|
|
||
| res = await app.httpRequest() | ||
| .get('/world/12?xx=xx') | ||
| .set('accept', 'text/html') | ||
| .expect(200); | ||
| assert.equal(res.headers['x-frame-options'], undefined); | ||
|
|
||
| res = await app2.httpRequest() | ||
| .get('/hello') | ||
| .set('accept', 'text/html') | ||
| .expect(200); | ||
| assert.equal(res.headers['x-frame-options'], undefined); | ||
|
|
||
| res = await app2.httpRequest() | ||
| .get('/world/12') | ||
| .set('accept', 'text/html') | ||
| .expect(200); | ||
| assert.equal(res.headers['x-frame-options'], undefined); | ||
|
|
||
| res = await app2.httpRequest() | ||
| .get('/world/12?xx=xx') | ||
| .set('accept', 'text/html') | ||
| .expect(200); | ||
| assert.equal(res.headers['x-frame-options'], undefined); | ||
| }); | ||
| }); | ||
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Review security implications of ALLOW-FROM directive.
The middleware sets X-Frame-Options to
ALLOW-FROM http://www.domain.com. Note that: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