-
Notifications
You must be signed in to change notification settings - Fork 43
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: support safeCurl for SSRF protection #32
Conversation
base on node-modules/urllib#279 |
个人觉得ip这块够了,但是几个可能的问题或需要实现的地方,因为ip限制比较容易被绕过,参考 http://www.freebuf.com/articles/web/135342.html:
|
可以看 urllib 的具体实现。
内网域名这种无法全部过滤干净,不如不支持为好,都通过 ip 段来屏蔽。 |
Codecov Report
@@ Coverage Diff @@
## master #32 +/- ##
==========================================
+ Coverage 95.75% 95.78% +0.03%
==========================================
Files 27 30 +3
Lines 448 475 +27
==========================================
+ Hits 429 455 +26
- Misses 19 20 +1
Continue to review full report at Codecov.
|
app/extend/agent.js
Outdated
if (this.config.security.ssrf && this.config.security.ssrf.checkAddress) { | ||
options.checkAddress = this.config.security.ssrf.checkAddress; | ||
} else { | ||
this.logger.warn('[egg-security] please configure `config.security.ssrf` first'); |
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.
这里不是应该提示配置 config.security.ssrf. checkAddress?
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.
agent 和 application 的重复代码,抽到 lib 里面去吧
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.
这里不是应该提示配置 config.security.ssrf. checkAddress?
还可以配置 ipBlackList
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.
从逻辑看只要没配 checkAddress 就 warn 了,其实有配置 ipBlackList 但没配 checkAddress 的情况。
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.
恩,前面 app.js 里面处理了一下,把 ipBlackList 转换成了一个 checkAdderss 的方法配置
// checkAddress has higher priority than ipBlackList | ||
if (config && config.ipBlackList && !config.checkAddress) { | ||
const containsList = config.ipBlackList.map(getContains); | ||
config.checkAddress = ip => { |
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.
加个 benchmark
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.
加个 lru?活跃的 ip 应该是有限的。
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.
先不加 lru 吧,相较于做一次 http 请求来说,这个性能算是可以接受的,现在也不会对 curl 造成影响。
我处理一下重复代码再合并 |
No description provided.