Skip to content
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

feat: add config for Access-Control-Allow-Origin #7

Merged
merged 7 commits into from
Jul 24, 2017
Merged

feat: add config for Access-Control-Allow-Origin #7

merged 7 commits into from
Jul 24, 2017

Conversation

brickyang
Copy link
Contributor

@brickyang brickyang commented Jul 1, 2017

feat: add config for Access-Control-Allow-Origin

Now you can use config.cors.origin to set Access-Control-Allow-Origin response header.

If origin is set, the plugin will ignore the security.domainWhiteList. Otherwise, domainWhiteList will do its job.

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

none

kcors 默认 `Access-Control-Allow-Origin: '*'`,取消这一默认项的确有安全方面的考虑,但是完全和 egg-security 的 `domainWhiteList` 绑定,不一定是最好的。

而且 egg-cors 虽然与安全有关,但毕竟是一个独立插件,有自己的配置项,如果必须同时依赖 egg-security 的设置,在设计上也值得商榷。

不作为默认,但允许用户自定义,也许更好,兼顾安全与灵活。
@mention-bot
Copy link

@brickyang, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fengmk2 to be a potential reviewer.

@codecov
Copy link

codecov bot commented Jul 1, 2017

Codecov Report

Merging #7 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master     #7   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines           9      9           
=====================================
  Hits            9      9
Impacted Files Coverage Δ
app.js 100% <100%> (ø) ⬆️

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 cbc9ee6...3c14e0b. Read the comment docs.

@atian25
Copy link
Member

atian25 commented Jul 3, 2017

egg-security 是 egg 的核心能力之一,egg-cors 这些外围插件,依赖它问题不大吧。

而且这个插件也就几行代码,如果真的需要的话,其实都可以直接在应用里面写个 middleware 引入 kcors 即可,不需要这个插件了。

@atian25 atian25 requested a review from fengmk2 July 3, 2017 01:26
@brickyang
Copy link
Contributor Author

brickyang commented Jul 3, 2017

@atian25 是的,你说的这两个问题我都考虑过。

我认为现在的做法不是最优的原因有这样几个:

1. 配置不一致
egg-cors 基于 kcors,但屏蔽了 config.origin,与 kcors 配置不一致(且文档缺少足够提示,「Support all configurations in kcors.」);

2. 一个插件,两处配置
egg-cors 有自己的 config.eggs,domainWhiteList 属于 config.security,一个插件的配置需要写在两个地方。但这其实是非必要的。

3. domainWhiteList 缺少扩展性
Origin 对 CORS 来说是最重要的配置之一,而 domainWhiteList 不支持通配符,也不支持函数,没有可扩展性,可用性大打折扣。自定义 origin 应该不是一个特别非常规的需求。

所以我认为允许 config.cors.origin 是一个简单的解决办法:

  1. 与 kcors 配置保持一致;
  2. 只需要一处配置;
  3. 完全可扩展(这一点也是与 kcors 保持一致)。

而且这个方案对现在的一切都没有影响,origins 默认仍然使用 config.security.domainWhiteList,只是允许用户手动覆盖它。没有损失安全性,只增加了扩展性。

自己引入 kcors 也是一个选择。但我认为,「origin 应该有扩展性」并没有超出 egg-cors 的范围(而是应有的),所以我们应该去改进它(而不是另起炉灶)。

个人浅见供参考 :)

@atian25
Copy link
Member

atian25 commented Jul 3, 2017

@jtyjty99999 你的看法?cc @eggjs/core

@dead-horse
Copy link
Member

同意 @brickyang ,security 的作为默认值即可

@atian25
Copy link
Member

atian25 commented Jul 3, 2017

@brickyang 加个测试用例吧

@brickyang
Copy link
Contributor Author

OK

@atian25 atian25 changed the title 允许自定义 Access-Control-Allow-Origin feat: add config for Access-Control-Allow-Origin Jul 3, 2017
@fengmk2
Copy link
Member

fengmk2 commented Jul 3, 2017

请忽略 package-lock.json

@fengmk2
Copy link
Member

fengmk2 commented Jul 3, 2017

commit log 请改成英文的。

package.json Outdated
@@ -20,17 +20,17 @@
"egg-plugin"
],
"dependencies": {
"kcors": "^1.3.2"
"kcors": "^2.2.1"
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/koajs/cors/blob/master/History.md#200--2016-02-20 kcors 是基于koa@2的,egg 目前还是基于 koa@1 的,目前还不能升级 kcors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

抱歉,已改。

@fengmk2
Copy link
Member

fengmk2 commented Jul 4, 2017

@brickyang 请将 commit log 改成英文版本

image

@atian25
Copy link
Member

atian25 commented Jul 4, 2017

我们自己 squash 改也行吧,下次 PR 注意就好了。

@brickyang
Copy link
Contributor Author

@fengmk2 抱歉处理得不及时。

@atian25 我第一次参与,很多东西不懂,非常需要被指出错误以学习改进,只是担心给大家带来太多麻烦。

@atian25
Copy link
Member

atian25 commented Jul 4, 2017

没事的,多搞几次就熟悉了,没啥的。

可以看下 https://eggjs.org/zh-cn/contributing.html

我当年的第一个 PR 更惨,哈哈

@atian25 atian25 merged commit 0cc030e into eggjs:master Jul 24, 2017
@atian25
Copy link
Member

atian25 commented Jul 24, 2017

都忘了这个 PR 了,先合了,等 #8 一起发版本。

@atian25
Copy link
Member

atian25 commented Jul 24, 2017

@brickyang 1.2.0

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

Successfully merging this pull request may close these issues.

5 participants