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

prevent unsupported operation call #154

Merged
merged 3 commits into from Sep 6, 2016
Merged

prevent unsupported operation call #154

merged 3 commits into from Sep 6, 2016

Conversation

yoshidan
Copy link
Contributor

I think fetcher.js should allow only 'read,create,update,delete' operations that fetchr.client supports.

because I could call any operation like 'constructor'. this may cause serious security problem.

curl -X POST http://localhost:3000/proxy -H "Content-Type:application/json" -d '{ "context":{},"requests":{ "g0": {"resource": "bookmarking", "operation": "constructor", "params":{}}}}' 

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 97.347% when pulling 6127574 on nysd:master into a7e3f8a on yahoo:master.

1 similar comment
@coveralls
Copy link

coveralls commented Aug 30, 2016

Coverage Status

Coverage increased (+0.005%) to 97.347% when pulling 6127574 on nysd:master into a7e3f8a on yahoo:master.

@@ -223,7 +224,7 @@ function executeRequest (request, resolve, reject) {
var service;
try {
service = Fetcher.getService(request.resource);
if (!service[op]) {
if ([OP_CREATE,OP_READ,OP_UPDATE,OP_DELETE].indexOf(op) < 0 || !service[op]) {
Copy link
Member

Choose a reason for hiding this comment

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

@nysd Thanks for your contribution!

How about moving this check before the line above, and checking using === explictly to avoid allocating an array and doing an indexOf every time request is executed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lingyan Thanks for your advice.

I agree with you and I moved this check from 'executeRequest' to 'Request' function.

Copy link
Member

Choose a reason for hiding this comment

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

@nysd Instead of moving this check to Request constructor and adding another try/catch, which is known to de-optimze the function for v8 engine, can we just do this check after line 431 the if (!Fetcher.isRegistered(singleRequest.resource)) { block, and call next(error) directly? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lingyan Thanks for your advice. I moved the operation check from Request constructor to after !Fetchr.isRegistered block. Please confirm my code.

@yahoocla
Copy link

CLA is valid!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 97.384% when pulling 6209a0d on nysd:master into a7e3f8a on yahoo:master.

1 similar comment
@coveralls
Copy link

coveralls commented Aug 31, 2016

Coverage Status

Coverage increased (+0.04%) to 97.384% when pulling 6209a0d on nysd:master into a7e3f8a on yahoo:master.

@coveralls
Copy link

coveralls commented Sep 5, 2016

Coverage Status

Coverage increased (+0.03%) to 97.374% when pulling 41c52fa on nysd:master into a7e3f8a on yahoo:master.

@lingyan
Copy link
Member

lingyan commented Sep 6, 2016

👍 Thanks! @nysd

@lingyan lingyan merged commit 84115b2 into yahoo:master Sep 6, 2016
@lingyan
Copy link
Member

lingyan commented Sep 6, 2016

Released in fetcher@0.5.34

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

4 participants