Ignore escape error and return original value better than throw the error #8

Merged
merged 1 commit into from Oct 29, 2012

Conversation

Projects
None yet
7 participants
@fengmk2
Contributor

fengmk2 commented Oct 29, 2012

I don't agree throw 400 response when cookie parse error.

Most normal user don't know how to clean up the browser cookies.

If their cookies was modified by accident, they will always got the 400 response.

I think return the original value is better than throw the escape error.

cc @visionmedia

@coolme200

This comment has been minimized.

Show comment Hide comment
@coolme200

coolme200 Oct 29, 2012

+1

+1

@FlashSoft

This comment has been minimized.

Show comment Hide comment
@FlashSoft

FlashSoft Oct 29, 2012

+1

+1

@mengxy

This comment has been minimized.

Show comment Hide comment
@mengxy

mengxy Oct 29, 2012

+1

mengxy commented Oct 29, 2012

+1

@defunctzombie

This comment has been minimized.

Show comment Hide comment
@defunctzombie

defunctzombie Oct 29, 2012

Contributor

I also think that this module should return the original cookie value and leave it up to higher level frameworks to decide on the action.

As for the pull request, I don't think you should have the try/catch over encode and would lean towards having the try/catch for decode at the call site and not in the decode function.

Contributor

defunctzombie commented Oct 29, 2012

I also think that this module should return the original cookie value and leave it up to higher level frameworks to decide on the action.

As for the pull request, I don't think you should have the try/catch over encode and would lean towards having the try/catch for decode at the call site and not in the decode function.

@fengmk2

This comment has been minimized.

Show comment Hide comment
@fengmk2

fengmk2 Oct 29, 2012

Contributor

@shtylman try/catch in serialize() and parse()?

Contributor

fengmk2 commented Oct 29, 2012

@shtylman try/catch in serialize() and parse()?

@defunctzombie

This comment has been minimized.

Show comment Hide comment
@defunctzombie

defunctzombie Oct 29, 2012

Contributor

I don't think you need it for serialize. Is there a value which will make encoding fail?

Contributor

defunctzombie commented Oct 29, 2012

I don't think you need it for serialize. Is there a value which will make encoding fail?

@fengmk2

This comment has been minimized.

Show comment Hide comment
@fengmk2

fengmk2 Oct 29, 2012

Contributor

Yes, Please view my test cases: encodeURIComponent(String.fromCharCode(0xDFFF))

Contributor

fengmk2 commented Oct 29, 2012

Yes, Please view my test cases: encodeURIComponent(String.fromCharCode(0xDFFF))

@defunctzombie

This comment has been minimized.

Show comment Hide comment
@defunctzombie

defunctzombie Oct 29, 2012

Contributor

Did you actually encounter this error in a production system with that character code? I guess I am just trying to get a sense of why these invalid codes are showing up (especially on the outgoing end where you should be the one controlling the cookie values).

In this case I would actually be in favor of having the exception happen since you are using an invalid code point (as far as my quick googling shows) maybe that is not the case.

For incoming values, I do think it would be good to go ahead and pass those along if decoding fails. I don't agree about outgoing. In my mind I think this actually indicates some other failure you should be aware of and look into. I think the encoding example is contrived but am happy to be proven wrong by an actual use case :)

Contributor

defunctzombie commented Oct 29, 2012

Did you actually encounter this error in a production system with that character code? I guess I am just trying to get a sense of why these invalid codes are showing up (especially on the outgoing end where you should be the one controlling the cookie values).

In this case I would actually be in favor of having the exception happen since you are using an invalid code point (as far as my quick googling shows) maybe that is not the case.

For incoming values, I do think it would be good to go ahead and pass those along if decoding fails. I don't agree about outgoing. In my mind I think this actually indicates some other failure you should be aware of and look into. I think the encoding example is contrived but am happy to be proven wrong by an actual use case :)

@fengmk2

This comment has been minimized.

Show comment Hide comment
@fengmk2

fengmk2 Oct 29, 2012

Contributor

I got your point, I will change my pull request right away.

Contributor

fengmk2 commented Oct 29, 2012

I got your point, I will change my pull request right away.

@fengmk2

This comment has been minimized.

Show comment Hide comment
@fengmk2

fengmk2 Oct 29, 2012

Contributor

I rebase the commit and pull request again.

Contributor

fengmk2 commented Oct 29, 2012

I rebase the commit and pull request again.

@defunctzombie

This comment has been minimized.

Show comment Hide comment
@defunctzombie

defunctzombie Oct 29, 2012

Contributor

Thanks for the quick turn around. I am gonna give @visionmedia a chance to take a look and weigh in (since I believe connect/express is the biggest current user).

Contributor

defunctzombie commented Oct 29, 2012

Thanks for the quick turn around. I am gonna give @visionmedia a chance to take a look and weigh in (since I believe connect/express is the biggest current user).

@fengmk2

This comment has been minimized.

Show comment Hide comment
@fengmk2

fengmk2 Oct 29, 2012

Contributor

OK. Before connect start using 'node-cookie', @visionmedia also try/catch in utils.js: parseCookie() https://github.com/senchalabs/connect/pull/308/files

Contributor

fengmk2 commented Oct 29, 2012

OK. Before connect start using 'node-cookie', @visionmedia also try/catch in utils.js: parseCookie() https://github.com/senchalabs/connect/pull/308/files

@defunctzombie

This comment has been minimized.

Show comment Hide comment
@defunctzombie

defunctzombie Oct 29, 2012

Contributor

Oh, good to know. Thank you for referencing that. Should be a no brainer then ;)

Contributor

defunctzombie commented Oct 29, 2012

Oh, good to know. Thank you for referencing that. Should be a no brainer then ;)

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Oct 29, 2012

gah yeah i guess it's one of those things, technically it's more correct to 400 but I suppose if users are ending up with bad cookies it's not their fault, though I would question how that occurs in the first place, this should not be a frequent problem

tj commented Oct 29, 2012

gah yeah i guess it's one of those things, technically it's more correct to 400 but I suppose if users are ending up with bad cookies it's not their fault, though I would question how that occurs in the first place, this should not be a frequent problem

@defunctzombie

This comment has been minimized.

Show comment Hide comment
@defunctzombie

defunctzombie Oct 29, 2012

Contributor

internet

So good to go? No bad side effects in connect/express (that we aren't prepared for)?

Contributor

defunctzombie commented Oct 29, 2012

internet

So good to go? No bad side effects in connect/express (that we aren't prepared for)?

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Oct 29, 2012

hahaha. yeah it should be fine, not the most elegant thing but whatever

tj commented Oct 29, 2012

hahaha. yeah it should be fine, not the most elegant thing but whatever

defunctzombie added a commit that referenced this pull request Oct 29, 2012

Merge pull request #8 from fengmk2/ignoreEscapeError
Ignore escape error and return original value

Instead of throwing when a cookie cannot be decoded, return the original value.

@defunctzombie defunctzombie merged commit b0b2673 into jshttp:master Oct 29, 2012

@defunctzombie

This comment has been minimized.

Show comment Hide comment
@defunctzombie

defunctzombie Oct 29, 2012

Contributor

published with version 0.0.5

Contributor

defunctzombie commented Oct 29, 2012

published with version 0.0.5

@fengmk2

This comment has been minimized.

Show comment Hide comment
@fengmk2

fengmk2 Oct 29, 2012

Contributor

Thanks all!

Contributor

fengmk2 commented Oct 29, 2012

Thanks all!

@fishbar

This comment has been minimized.

Show comment Hide comment
@fishbar

fishbar Oct 29, 2012

速度的,哈哈

fishbar commented Oct 29, 2012

速度的,哈哈

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Oct 29, 2012

I have to say I'm pretty curious how users are getting such messed up cookies in the first place

tj commented Oct 29, 2012

I have to say I'm pretty curious how users are getting such messed up cookies in the first place

@fengmk2

This comment has been minimized.

Show comment Hide comment
@fengmk2

fengmk2 Oct 29, 2012

Contributor

@visionmedia I'm working on the "shu" web application on "taobao.com" subdomain: http://shu.taobao.com/ .

And there many other applications and shops hosting on "xxx.taobao.com", we can't stop them add the messed up cookies on ".taobao.com". If anyone do that, it will cause our "shu" web application response error page to the user.

Contributor

fengmk2 commented Oct 29, 2012

@visionmedia I'm working on the "shu" web application on "taobao.com" subdomain: http://shu.taobao.com/ .

And there many other applications and shops hosting on "xxx.taobao.com", we can't stop them add the messed up cookies on ".taobao.com". If anyone do that, it will cause our "shu" web application response error page to the user.

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Oct 29, 2012

ah i see

tj commented Oct 29, 2012

ah i see

@af af referenced this pull request in expressjs/express Jan 1, 2013

Closed

URIError: URI Malformed #1331

@fishbar

This comment has been minimized.

Show comment Hide comment
@fishbar

fishbar May 21, 2014

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