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

Detect parsing method from while list #7

Merged
merged 2 commits into from
Sep 18, 2014

Conversation

tonyseek
Copy link
Collaborator

The white list could protect us from injection attacking.

@lepture Please review it. Thanks.

to prevent from injection attacking.
@lepture
Copy link
Owner

lepture commented Sep 17, 2014

@tonyseek 如何 injection?

func = getattr(self, 'parse_%s' % msg_type)

这个可以保证了呀。我本意是想让它更容易扩展,添加一个 type 时,只用再加一个 parse_xxx 就好了,不用再维护一个列表了。

@tonyseek
Copy link
Collaborator Author

@lepture 能导致安全问题的方式我没想到,但是如果 msg_type 是预期以外的也足以引起 AttributeError 导致服务器端 500 而不是 400。其实我最开始的修改是前置了 if msg_type in ('text', ...) 的判断,后来觉得这样反而不如显式列出支持的 typeparse_xxx 的映射。况且字节串远不止两百多个 ASCII,隔离反射类接口和用户输入可以把将来变更中带入潜在风险的可能性降到最小。

如果你希望不维护列表而用 getattr(self, 'parse_%s' % msg_type) 这种写法,我也可以改回去,变成 getattr(self, 'parse_%s' % msg_type, None) 以避免 AttributeError。但我个人更倾向多维护一个列表避免带入一些需要主动记忆的假设。

怎么看呢 →_→

@qingfeng
Copy link

也可以考虑捕获AttributeError,然后做一些反馈,这样就不用500了

@tonyseek
Copy link
Collaborator Author

@qingfeng 哇,清风老师…

觉得捕获异常 AttributeError 有个小问题,就是这个异常可能是从下层调用(比如 parse_xxx 里面)抛出来的,这样也许会造成 parse_xxx 本身不存在的误解……

(其实觉得捕获 ImportError 证明模块不存在、对一个非内置函数调用捕获 KeyError 都有类似的问题)

@lepture
Copy link
Owner

lepture commented Sep 17, 2014

@tonyseek getattr(self, 'parse_%s' % msg_type, None) 这种吧。

@tonyseek
Copy link
Collaborator Author

@lepture 现在呢?

lepture added a commit that referenced this pull request Sep 18, 2014
Detect parsing method from while list
@lepture lepture merged commit d0d965d into lepture:master Sep 18, 2014
@tonyseek tonyseek deleted the protect-injection branch September 18, 2014 07:45
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.

None yet

3 participants