Skip to content

Fixed DoS vector in middleware implementation #101

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

Closed
wants to merge 1 commit into from

Conversation

tj
Copy link
Contributor

@tj tj commented Sep 1, 2011

it's pretty trivial to trigger a next(err) for almost any middleware, by-design they should be handled in some unified fashion, however the one in this lib just throws. You could also emit an error event with the req/res to conditionally allow users to customize the handling if necessary

@mikeal
Copy link

mikeal commented Sep 1, 2011

+1

@mikeal
Copy link

mikeal commented Sep 1, 2011

one concern i have is that this 500 is indistinguishable from a 500 returned by the applications behind the proxy.

we could add a header 'x-node-proxy-error', or change the status code to one that is proxy specific, the problem is that there isn't one that fits perfectly.

10.5.3 502 Bad Gateway

The server, while acting as a gateway or proxy, received an invalid response 
from the upstream server it accessed in attempting to fulfill the request.

10.5.5 504 Gateway Timeout

The server, while acting as a gateway or proxy, did not receive a 
timely response from the upstream server specified by the URI 
(e.g. HTTP, FTP, LDAP) or some other auxiliary server (e.g. DNS) 
it needed to access in attempting to complete the request.

@tj
Copy link
Contributor Author

tj commented Sep 1, 2011

true true, not sure what convention is there

@mikeal
Copy link

mikeal commented Sep 1, 2011

bah, fuck it, let's just change the text body to "Internal Proxy Server Error" :)

@tj
Copy link
Contributor Author

tj commented Sep 1, 2011

beats nothing. looks like squid adds custom 6xx range status codes


Broken Server Software
600
Squid: header parsing error
601
Squid: header size overflow detected while parsing
601
roundcube: software configuration error
603
roundcube: invalid authorization

@mikeal
Copy link

mikeal commented Sep 1, 2011

node.js should define all kinds of new status codes that all begin with 31337 ;)

@indexzero
Copy link
Member

Thanks. I'll push this in tonight.

@indexzero
Copy link
Member

Sorry, been busy with NodeConf SummerCamp. Will push this out when I'm back in NYC tonight.

@tj
Copy link
Contributor Author

tj commented Sep 8, 2011

sounds good thanks man!

@indexzero
Copy link
Member

Cherry-picked in 07c8d2e. Will be published in 0.6.7

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.

3 participants