Skip to content

Conversation

guoshencheng
Copy link
Contributor

不知道是不是理解错了,这里的ALLOWED_POLICIES_ENUM是用于校验设定的referrer-policy是否合法的,而源代码

 if (policy in ALLOWED_POLICIES_ENUM) {
     throw new Error('"' + policy + '" is not available."');
 }

只有数组下标才会进入判断

比如
'origin' in ALLOWED_POLICIES_ENUM返回false
'oorigin' in ALLOWED_POLICIES_ENUM也返回false
'0' in ALLOWED_POLICIES_ENUM返回true

这就失去了判断的意义

@atian25 atian25 requested review from jtyjty99999 and whxaxes January 3, 2019 08:40
@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

使用 includes 更加合语义

Copy link
Member

@jtyjty99999 jtyjty99999 Jan 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

是的 用includes比较好

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (ALLOWED_POLICIES_ENUM.indexOf(policy) < 0) {
if (!ALLOWED_POLICIES_ENUM.includes(policy)) {

Copy link
Contributor Author

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';
Copy link
Member

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 的测试用例。

Copy link
Contributor Author

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的测试用例

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我的意思是加上这个测试用例,如果我们不修复代码的话,就会触发这个 bug 了。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

好的,已经添加了这个测试用例,并添加了针对这个测试用例的注释link到这个pull request

@fengmk2 fengmk2 added the bug label Jan 4, 2019
@jtyjty99999
Copy link
Member

我的坑。。这个是bug

@codecov
Copy link

codecov bot commented Jan 4, 2019

Codecov Report

Merging #50 into master will increase coverage by 0.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
lib/middlewares/referrerPolicy.js 100% <100%> (+9.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ba7024...0c6634c. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 4, 2019

Codecov Report

Merging #50 into master will increase coverage by 0.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
lib/middlewares/referrerPolicy.js 100% <100%> (+9.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ba7024...2a26ca4. Read the comment docs.

@jtyjty99999
Copy link
Member

+1

@guoshencheng
Copy link
Contributor Author

这个PR还有什么问题么?

@dead-horse dead-horse merged commit ad21465 into eggjs:master Jan 4, 2019
@dead-horse
Copy link
Member

2.4.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants