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

Responses with invalid redirects may crash the server #234

Closed
Rob--W opened this issue May 6, 2020 · 1 comment
Closed

Responses with invalid redirects may crash the server #234

Rob--W opened this issue May 6, 2020 · 1 comment

Comments

@Rob--W
Copy link
Owner

Rob--W commented May 6, 2020

There have been reports of the server being unreachable (e.g. #233). I started to watch the log stream, and found the following runtime error:

/app/lib/cors-anywhere.js:82
 req.url = location.path;
 ^
 
 TypeError: Cannot read property 'path' of null
 at proxyRequest (/app/lib/cors-anywhere.js:82:22)
 at onProxyResponse (/app/lib/cors-anywhere.js:202:11)
 at ClientRequest.<anonymous> (/app/lib/cors-anywhere.js:103:17)
 at ClientRequest.emit (events.js:315:20)
 at HTTPParser.parserOnIncomingClient [as onIncoming] (_http_client.js:632:27)
 at HTTPParser.parserOnHeadersComplete (_http_common.js:117:17)
 at HTTPParser.execute (<anonymous>)
 at Socket.socketOnData (_http_client.js:501:22)
 at Socket.emit (events.js:315:20)
 at addChunk (_stream_readable.js:300:12)

This could happen if the server receives a redirect to an invalid URL. I recently made the URL validation a bit more strict, which probably increased the likelihood of triggering this bug.

@Rob--W Rob--W closed this as completed in a9e06a9 May 6, 2020
Rob--W added a commit that referenced this issue May 6, 2020
- Reject invalid URLs in redirects (fixes regression from 0.4.2) (#234)
- Update memory tests for recent Node versions.
@egfx
Copy link

egfx commented May 6, 2020

good catch!

meenie pushed a commit to meenie/cors-anywhere that referenced this issue Feb 12, 2021
meenie pushed a commit to meenie/cors-anywhere that referenced this issue Feb 12, 2021
- Reject invalid URLs in redirects (fixes regression from 0.4.2) (Rob--W#234)
- Update memory tests for recent Node versions.
DiegoFleitas added a commit to DiegoFleitas/cors-anywhere that referenced this issue May 26, 2022
* Extend supported Node.js from <=9 to <=14

* test-memory: destroy response to free socket

Starting from Node 12, the test started to fail because of
intermittent socket errors, such as ECONNRESET and "socket hang up".

Destroying the response before triggering a new request resolves it.

* Explicit early out for invalid URLs

* Version 0.4.2

- Reject invalid URLs earlier instead of trying to continue with the
  request (and failing anyway).
- Explicitly close the response when an error occurs for Node 13+.
- Update tests to cover up to Node 14 (was up to 9).

* Update test expectation for Node 12.x

* test-memory: fix test by passing --max-http-header-size

The test broke because Node lowered the maximum header size to defend
against large headers ( CVE-2018-12121 ).

In the test, we do actually want to pass large headers, because all
processing in CORS Anywhere is based on headers (the request body would
just be forwarded to the destination server).

The test failed intermittently with ECONNRESET or "socket hang up"
because the server (under test) would close the socket upon receiving
a request with too large request headers.

* Pass --max-http-header-size in supported versions only

* Reject invalid redirects

Fixes Rob--W#234.

* Version 0.4.3

- Reject invalid URLs in redirects (fixes regression from 0.4.2) (Rob--W#234)
- Update memory tests for recent Node versions.

* only send Access-Control-Max-Age if preflight request, not POST/GET

-Access-Control-Max-Age header only has meaning for preflights, not
 POST or GET, saves wire bytes by excluding it from POST/GET/etc,
 and future problems if ACMA on a content HTTP method is given
 meaning by W3C or a browser vendor

-fix expectNoHeader() test helper func ,this was a no-op before by
 accident and would NEVER fail,
 supertest/test.js:Test.prototype._assertFunction requires an retval of
 class type Error if test fail, not a string or a number or Object

* remove Heroku specific Req headers from being sent to Origin

-saves bytes, and avoids triggering IDS/WAF alarms since browser finger
 printing will prove these headers are unnatural and on SSL must be a MITM
 attack

-leave x-forwarded-* intact since they can be used to block CORS proxy
 abuse if the not-CORS origin webmaster really has to block the proxy
 and they are not unique to Heroku platform

* Remove obsolete values from server.js's removeHeaders

`X-Heroku-Dynos-In-Use`, `X-Heroku-Queue-Depth` and
`X-Heroku-Queue-Wait-Time` have already been dropped in 2013:
https://devcenter.heroku.com/changelog-items/218

* Add handleInitialRequest option to support Rob--W#301

The custom filtering logic is not part of the public repository, to
keep the project clean.

* Expand handleInitialRequest documentation Rob--W#335

* Add note about availability of public demo server

Referencing Rob--W#301

* Update gTLD list

* Version 0.4.4

- Omit unnecessary `Access-Control-Max-Age` (Rob--W#277)
- Remove more Heroku-specific headers (Rob--W#278)
- Add `handleInitialRequest` option (Rob--W#335)
- Document access requirements for public demo (Rob--W#301)
- Update gTLD list

* Support NODE_TLS_REJECT_UNAUTHORIZED=0 to ignore client errors Rob--W#341

Apparently `NODE_TLS_REJECT_UNAUTHORIZED` is only effective if
`rejectUnauthorized` was not overridden by the code:
https://github.com/nodejs/node/blob/85e6089c4db4da23dd88358fe0a12edefcd411f2/lib/_tls_wrap.js#L1583-L1591

But the underlying library does override it:
https://github.com/http-party/node-http-proxy/blob/v1.11.1/lib/http-proxy/common.js#L53-L55

Fix this by overriding the option via the library's "secure" option.

* Fix test expectation for old node

* Migrate travis-ci from .org to .com

* Add Node 15.x to Travis

* Show "400 Missing slash" when needed Rob--W#238

* Add LICENSE file based on README.md Rob--W#297

* Fix typo

Co-authored-by: Rob Wu <[email protected]>
Co-authored-by: bulk88 <[email protected]>
Co-authored-by: Noodles <[email protected]>
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