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

bulk88 · 2020-09-24T22:04:09Z

-Access-Control-Max-Age header only has meaning for preflights, not
POST or GET, saves wire bytes by excluding it from POST/GET/etc

Rob--W · 2020-09-25T09:51:37Z

Its presence has a negligible impact on the size of the response headers.
Why are you interested in removing the Access-Control-Max-Age response header? It is not the only header that only serves a purpose in the preflight request.

bulk88 · 2020-09-25T15:40:37Z

I am not removing Access-Control-Max-Age from OPTIONS preflight. I am removing it from GET/POST/PUT/DELETE. Why are you sending a preflight-only CORS specific resp header on a GET and POST resp? non-CORS POST resps can't even be cached, yet the server is implying they can be by sending Access-Control-Max-Age on a POST, but browsers rightfully ignore it bc they follow spec, but there could be a bug/breakage down the road if Access-Control-Max-Age on a GET/POST is given meaning by W3C in a revised future spec. Its best to NOT SEND it if its ignored today by browsers.

Rob--W · 2020-09-25T17:11:01Z

Why are you sending a preflight-only CORS specific resp header on a GET and POST resp?

Because, as you can see in the implementation, it was easier to implement, with no negative impact. You're claiming that removing the header has the positive impact of reducing bandwidth usage. I don't put that much weight in the claim, but if I did, then I would also take a critical look at other response headers (Access-Control-Max-Age is not the only CORS header that is meaningless outside a preflight request).

non-CORS POST resps can't even be cached, yet the server is implying they can be by sending Access-Control-Max-Age on a POST,

This is incorrect. The Access-Control-Max-Age directive does not apply to the content, but solely to the CORS headers. It allows the browser to re-use a previous preflight response, by caching said preflight response. For more details, see https://fetch.spec.whatwg.org/#cors-preflight-cache

but there could be a bug/breakage down the road if Access-Control-Max-Age on a GET/POST is given meaning by W3C in a revised future spec.

This is not realistic.

Its best to NOT SEND it if its ignored today by browsers.

Since I'm not convinced of the premises/assumptions, I'm inclined to close this PR, unless you can come up with a clear advantage in favor of merging it.

bulk88 · 2020-09-25T23:12:01Z

You are admitting that sending Access-Control-Max-Age on a content resp where it has no meaning by spec, is done because it was "Easier to implement", which means laziness. My patch fixes your lack of dev time to fix the original implementation from 2017 https://github.com/Rob--W/cors-anywhere/pull/77 Sending Max-Age on a GET/POST is undefined behavior by spec, and therefore wrong.

then I would also take a critical look at other response headers (Access-Control-Max-Age is not the only CORS header that is meaningless outside a preflight request).

I dont see any other preflight specific headers being sent for GET/POST. A GET currently correctly has only Access-Control-Expose-Headers and a GET currently correctly does NOT have Access-Control-Allow-Headers (preflight only). cors-anywhere OPTIONS response incorrectly sends Access-Control-Expose-Headers which is a content-only header but that is for another bug ticket and different rational of keeping or removing that.

Also the bug of sending Max-Age on a GET/POST only happens on private cors-anywhere implementations that explicitly turn on createServer->corsMaxAge to non zero. Not on https://cors-anywhere.herokuapp.com since https://cors-anywhere.herokuapp.com has max-age=0/off by default.

bulk88 · 2020-09-26T17:14:47Z

I repushed the branch, I fixed up the tests, and a test helper func which broken before and never failed regardless of input.

coveralls · 2020-09-26T17:16:47Z

Coverage remained the same at 100.0% when pulling b3a13b0 on bulk88:no_AC_max_age_header_on_get_post_meth into a0309e6 on Rob--W:master.

coveralls · 2020-09-26T17:16:47Z

Coverage decreased (-0.4%) to 99.569% when pulling 0f740b275bfd99340f5eba01dcd9c943dc326fdb on bulk88:no_AC_max_age_header_on_get_post_meth into a0309e6 on Rob--W:master.

Rob--W

The fix itself is reasonable, so I'm going to merge it after the one minor issue below is addressed. Could you squash your changes in one commit when you update your revision?

Rob--W · 2020-09-27T11:21:57Z

test/test.js

@@ -23,7 +23,7 @@ request.Test.prototype.expectJSON = function(json, done) {
 request.Test.prototype.expectNoHeader = function(header, done) {
  this.expect(function(res) {
    if (header.toLowerCase() in res.headers) {
-      return 'Unexpected header in response: ' + header;
+      return Error('Unexpected header in response: ' + header);


Could you prepend the new keyword before the Error constructor?

revised single commit pushed

-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

Rob--W

LGTM, thanks!

Rob--W · 2020-09-27T20:44:09Z

test/test.js

@@ -23,7 +23,7 @@ request.Test.prototype.expectJSON = function(json, done) {
 request.Test.prototype.expectNoHeader = function(header, done) {
  this.expect(function(res) {
    if (header.toLowerCase() in res.headers) {
-      return 'Unexpected header in response: ' + header;
+      return new Error('Unexpected header in response: ' + header);


For future reference, this change fixes the issue because the supertest library treats Error instances as errors (whether returned or thrown), per https://github.com/visionmedia/supertest/blob/v2.0.0/lib/test.js#L265-L269.

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

* 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]>

Rob--W reviewed Sep 27, 2020

View reviewed changes

Rob--W approved these changes Sep 27, 2020

View reviewed changes

Rob--W merged commit 3bab870 into Rob--W:master Sep 27, 2020

bulk88 deleted the no_AC_max_age_header_on_get_post_meth branch September 29, 2020 06:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bulk88 commented Sep 24, 2020

Rob--W commented Sep 25, 2020

bulk88 commented Sep 25, 2020

Rob--W commented Sep 25, 2020

bulk88 commented Sep 25, 2020

bulk88 commented Sep 26, 2020

coveralls commented Sep 26, 2020 •

edited

Loading

coveralls commented Sep 26, 2020

Rob--W left a comment

Rob--W Sep 27, 2020

bulk88 Sep 27, 2020

Rob--W left a comment

Rob--W Sep 27, 2020

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

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

Conversation

bulk88 commented Sep 24, 2020

Rob--W commented Sep 25, 2020

bulk88 commented Sep 25, 2020

Rob--W commented Sep 25, 2020

bulk88 commented Sep 25, 2020

bulk88 commented Sep 26, 2020

coveralls commented Sep 26, 2020 • edited Loading

coveralls commented Sep 26, 2020

Rob--W left a comment

Choose a reason for hiding this comment

Rob--W Sep 27, 2020

Choose a reason for hiding this comment

bulk88 Sep 27, 2020

Choose a reason for hiding this comment

Rob--W left a comment

Choose a reason for hiding this comment

Rob--W Sep 27, 2020

Choose a reason for hiding this comment

coveralls commented Sep 26, 2020 •

edited

Loading