-
Notifications
You must be signed in to change notification settings - Fork 43
fix: fix referrer-policy enum check #50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: fix referrer-policy enum check #50
Conversation
lib/middlewares/referrerPolicy.js
Outdated
@@ -21,7 +21,7 @@ module.exports = options => { | |||
const opts = utils.merge(options, ctx.securityOptions.refererPolicy); | |||
if (utils.checkIfIgnore(opts, ctx)) { return; } | |||
const policy = opts.value; | |||
if (policy in ALLOWED_POLICIES_ENUM) { | |||
if (ALLOWED_POLICIES_ENUM.indexOf(policy) < 0) { |
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.
使用 includes 更加合语义
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.
是的 用includes比较好
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.
if (ALLOWED_POLICIES_ENUM.indexOf(policy) < 0) { | |
if (!ALLOWED_POLICIES_ENUM.includes(policy)) { |
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.
完全同意,这块我已经修改了。
@@ -37,5 +37,14 @@ describe('test/referrer.test.js', function() { | |||
.expect(200, done); | |||
}); | |||
|
|||
it('should throw error when Referrer-Policy settings is invalid when configured', function(done) { | |||
const policy = 'oorigin'; |
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.
加上 '0' in ALLOWED_POLICIES_ENUM返回true
的测试用例。
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.
Referrer-Policy 设置为0 应该是不合法的Referrer-Policy的处理,这个测试用例已经在测试非法的Referrer-Policy应该抛出异常的情况了,抱歉其实我没看懂什么是'0' in ALLOWED_POLICIES_ENUM返回true
的测试用例
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.
我的意思是加上这个测试用例,如果我们不修复代码的话,就会触发这个 bug 了。
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.
好的,已经添加了这个测试用例,并添加了针对这个测试用例的注释link到这个pull request
我的坑。。这个是bug |
Codecov Report
@@ Coverage Diff @@
## master #50 +/- ##
=========================================
+ Coverage 95.92% 96.13% +0.2%
=========================================
Files 30 30
Lines 491 491
=========================================
+ Hits 471 472 +1
+ Misses 20 19 -1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #50 +/- ##
=========================================
+ Coverage 95.92% 96.13% +0.2%
=========================================
Files 30 30
Lines 491 491
=========================================
+ Hits 471 472 +1
+ Misses 20 19 -1
Continue to review full report at Codecov.
|
…is exist in ALLOWED_POLICIES_ENUM
+1 |
这个PR还有什么问题么? |
2.4.2 |
不知道是不是理解错了,这里的
ALLOWED_POLICIES_ENUM
是用于校验设定的referrer-policy
是否合法的,而源代码只有数组下标才会进入判断
比如
'origin' in ALLOWED_POLICIES_ENUM
返回false
'oorigin' in ALLOWED_POLICIES_ENUM
也返回false
'0' in ALLOWED_POLICIES_ENUM
返回true
这就失去了判断的意义