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

Server error for urls with special chars (for example %E9) #3850

Closed
ezhlobo opened this issue Oct 13, 2016 · 7 comments
Closed

Server error for urls with special chars (for example %E9) #3850

ezhlobo opened this issue Oct 13, 2016 · 7 comments

Comments

@ezhlobo
Copy link

ezhlobo commented Oct 13, 2016

Sails version: 0.12.7
Node version: v6.6.0
NPM version: 3.10.3
Operating system: OS X


Hello. I've experienced an error for urls with special chars in it. For example I want to open url /members/kbiadun/09xt660zt%E9n%E9r%E9/ but I get this:

error: Server Error:
error: Error: Failed to decode param 'members/kbiadun/09xt660zt%E9n%E9r%E9/'
    at Route.match (/Users/ezhlobo/Work/Datarockets/bikepics/node_modules/sails/node_modules/express/lib/router/route.js:65:17)
    at Router.matchRequest (/Users/ezhlobo/Work/Datarockets/bikepics/node_modules/sails/node_modules/express/lib/router/index.js:259:17)
    at pass (/Users/ezhlobo/Work/Datarockets/bikepics/node_modules/sails/node_modules/express/lib/router/index.js:104:30)
    at Router._dispatch (/Users/ezhlobo/Work/Datarockets/bikepics/node_modules/sails/node_modules/express/lib/router/index.js:173:5)
    at Object.router (/Users/ezhlobo/Work/Datarockets/bikepics/node_modules/sails/node_modules/express/lib/router/index.js:33:10)
    at next (/Users/ezhlobo/Work/Datarockets/bikepics/node_modules/sails/node_modules/express/node_modules/connect/lib/proto.js:174:15)
    at Object.xPoweredBy [as handle] (/Users/ezhlobo/Work/Datarockets/bikepics/node_modules/sails/lib/hooks/http/get-configured-http-middleware-fns.js:222:7)
    at next (/Users/ezhlobo/Work/Datarockets/bikepics/node_modules/sails/node_modules/express/node_modules/connect/lib/proto.js:174:15)
    at Object.methodOverride [as handle] (/Users/ezhlobo/Work/Datarockets/bikepics/node_modules/method-override/index.js:65:14)
    at next (/Users/ezhlobo/Work/Datarockets/bikepics/node_modules/sails/node_modules/express/node_modules/connect/lib/proto.js:174:15)
    at next (/Users/ezhlobo/Work/Datarockets/bikepics/node_modules/sails/node_modules/express/node_modules/connect/lib/proto.js:176:9)
    at Object._parseHTTPBody [as handle] (/Users/ezhlobo/Work/Datarockets/bikepics/node_modules/skipper/index.js:56:14)
    at next (/Users/ezhlobo/Work/Datarockets/bikepics/node_modules/sails/node_modules/express/node_modules/connect/lib/proto.js:174:15)
    at Object.addCurrentUserToCookie [as handle] (/Users/ezhlobo/Work/Datarockets/bikepics/config/http.js:93:14)
    at next (/Users/ezhlobo/Work/Datarockets/bikepics/node_modules/sails/node_modules/express/node_modules/connect/lib/proto.js:174:15)
    at require.readdir (/Users/ezhlobo/Work/Datarockets/bikepics/config/http.js:82:16)
    at FSReqWrap.oncomplete (fs.js:123:15)

No need any extra dependencies to reproduce it. However let me know if you need separate repository for this.

Also I see that this error related to express and I found that they've fixed it in 4.13 version:
expressjs/express#2652

Is it possible to run sails with the newest version of express js? I guess it helps me, but I'm not sure. Or how can I handle such routes with current sails version?

@not-an-aardvark
Copy link

I can reproduce this as well. It looks like express throws an error in this case, but sets the status of the error to 400, so this case is handled correctly by express on its own. Something about the interaction with sails is causing this error to be unhandled and sent to the client as a 500 error.

@mikermcneil
Copy link
Member

@not-an-aardvark @ezhlobo hey guys, thanks for bringing this up. In Express 4.13, you'll still see this error, as you pointed out-- the difference is that the lower-level error bubbles up.

In Sails v0.12.7, the error looks like this:
image

I agree that this would be better handled as a 400- @ezhlobo is that the behavior you're expecting as well?

@mikermcneil
Copy link
Member

@not-an-aardvark @ezhlobo would you guys take cb74c3c for a spin? It looks good to go from over here, but always nice to get some verification. There's a few other things I'd like to include in 0.12.8, but assuming this looks good, we'll be ready to go on that tomorrow. Thanks!

@not-an-aardvark
Copy link

I can confirm that cb74c3c fixes the problem. Thanks for resolving this so quickly!

@mikermcneil
Copy link
Member

@not-an-aardvark awesome, thanks (and likewise re testing it!)

@ezhlobo
Copy link
Author

ezhlobo commented Oct 21, 2016

@mikermcneil thank you! It looks much better!

Actually I wanted to work with such urls. I thought that we can use not encoded strings :). But nevermind, I'll do redirects in nginx (because according to spec we should encode all urls). Thanks for handling this issue as 400.

@mikermcneil
Copy link
Member

@ezhlobo @not-an-aardvark 🙇

Published in 0.12.8

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

No branches or pull requests

3 participants