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

Error when decoding URI component #5

Closed
defunctzombie opened this issue Sep 23, 2012 · 5 comments
Closed

Error when decoding URI component #5

defunctzombie opened this issue Sep 23, 2012 · 5 comments

Comments

@defunctzombie
Copy link
Contributor

https://github.com/senchalabs/connect/blob/master/lib/middleware/cookieParser.js#L49
https://github.com/shtylman/node-cookie/blob/master/index.js#L46

This line uses the parse function from the cookie module. When the cookie value cannot be decoded properly this function throws (since the decodeURIComponent function throws). My concern is that this appears as a system error (plain error object) which usually results in the server responding with a 5xx error response versus a bad request or possibly not parsing the given value.

Now, I could wrap the decode call in the cookie module and just set the value to the raw value versus the decoded one. I am thinking this would be the proper thing to do (as I don't think failing with unable to decode URI component is good behavior here) but I wanted to first get some feedback since it would technically be a change in behavior.

Issue senchalabs/connect#652 is related.

/ping @visionmedia

@tj
Copy link

tj commented Sep 24, 2012

tough call, I'd +1 400 personally but most people request that it would just warn / ignore, my votes for a 400

@defunctzombie
Copy link
Contributor Author

Do you think it would be wrong to silently catch the failure to decode and just set the value to the raw value in that case?

@tj
Copy link

tj commented Sep 24, 2012

I think so yeah, if it's an invalid uri I dont see why it shouldn't be 400

@defunctzombie
Copy link
Contributor Author

The 400 response would need to come from connect in that case and just leave this module to throw. Want me to make the patch for that?

@tj
Copy link

tj commented Sep 24, 2012

sure! sounds good to me, if we tack a err.status = 400 on that error then connect will respond correctly

fengmk2 added a commit to fengmk2/node-cookie that referenced this issue Oct 29, 2012
fengmk2 added a commit to fengmk2/node-cookie that referenced this issue Oct 29, 2012
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

No branches or pull requests

2 participants