From 55122f3ab3d67c39f2984673e7734535b04d911e Mon Sep 17 00:00:00 2001 From: Jeremy Keys Date: Fri, 22 Mar 2024 18:48:32 -0600 Subject: [PATCH 1/6] feat: Add callback for configuring Vary header --- lib/index.js | 33 +++++---- package.json | 2 +- test/test.js | 192 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 213 insertions(+), 14 deletions(-) diff --git a/lib/index.js b/lib/index.js index ad899ca..d433980 100644 --- a/lib/index.js +++ b/lib/index.js @@ -9,7 +9,8 @@ origin: '*', methods: 'GET,HEAD,PUT,PATCH,POST,DELETE', preflightContinue: false, - optionsSuccessStatus: 204 + optionsSuccessStatus: 204, + shouldSetVaryHeader: function(req, header) { return true } }; function isString(s) { @@ -50,10 +51,12 @@ key: 'Access-Control-Allow-Origin', value: options.origin }]); - headers.push([{ - key: 'Vary', - value: 'Origin' - }]); + if (options.shouldSetVaryHeader(req, 'Origin')) { + headers.push([{ + key: 'Vary', + value: 'Origin' + }]); + } } else { isAllowed = isOriginAllowed(requestOrigin, options.origin); // reflect origin @@ -61,10 +64,12 @@ key: 'Access-Control-Allow-Origin', value: isAllowed ? requestOrigin : false }]); - headers.push([{ - key: 'Vary', - value: 'Origin' - }]); + if (options.shouldSetVaryHeader(req, 'Origin')) { + headers.push([{ + key: 'Vary', + value: 'Origin' + }]); + } } return headers; @@ -97,10 +102,12 @@ if (!allowedHeaders) { allowedHeaders = req.headers['access-control-request-headers']; // .headers wasn't specified, so reflect the request headers - headers.push([{ - key: 'Vary', - value: 'Access-Control-Request-Headers' - }]); + if (options.shouldSetVaryHeader(req, 'Access-Control-Request-Headers')) { + headers.push([{ + key: 'Vary', + value: 'Access-Control-Request-Headers' + }]); + } } else if (allowedHeaders.join) { allowedHeaders = allowedHeaders.join(','); // .headers is an array, so turn it into a string } diff --git a/package.json b/package.json index 3368389..9b755d1 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "cors", "description": "Node.js CORS middleware", - "version": "2.8.5", + "version": "2.9.0", "author": "Troy Goode (https://github.com/troygoode/)", "license": "MIT", "keywords": [ diff --git a/test/test.js b/test/test.js index f2a2e94..21a8cd3 100644 --- a/test/test.js +++ b/test/test.js @@ -150,6 +150,198 @@ var util = require('util') }) }); + describe('shouldSetVaryHeader', function() { + it('Vary: "Access-Control-Request-Headers" header is set by default for pre-flight requests', function (done) { + var cb = after(1, done) + + var req = new FakeRequest('OPTIONS') + var res = new FakeResponse() + req.host = 'http://example.com' + req.originalUrl = '/images/asdf.png' + + + res.on('finish', function () { + assert.equal(res.statusCode, 204) + assert.equal(res.getHeader('Vary'), 'Access-Control-Request-Headers') + cb() + }) + + cors()(req, res, function (err) { + cb(err || new Error('should not be called')) + }) + }) + + it('Vary: "Origin, Access-Control-Request-Headers" header is set by default for pre-flight requests with origins', function (done) { + var cb = after(1, done) + + var req = new FakeRequest('OPTIONS') + var res = new FakeResponse() + req.host = 'http://example.com' + req.originalUrl = '/images/asdf.png' + var options = { + origin: 'https://example.com' + } + + res.on('finish', function () { + assert.equal(res.statusCode, 204) + assert.equal(res.getHeader('Vary'), 'Origin, Access-Control-Request-Headers') + cb() + }) + + cors(options)(req, res, function (err) { + cb(err || new Error('should not be called')) + }) + }) + + it('Vary: "Access-Control-Request-Headers" header is set for pre-flight requests and shouldSetVaryHeader(req, \'Origin\') configured to return false with origins', function (done) { + var cb = after(1, done) + + var req = new FakeRequest('OPTIONS') + var res = new FakeResponse() + req.host = 'http://example.com' + req.originalUrl = '/images/asdf.png' + var options = { + origin: 'https://example.com', + shouldSetVaryHeader: function(req, header) { + if (header === 'Access-Control-Request-Headers') { + return true + } + + if (req.originalUrl.startsWith('/images') && header === 'Origin') { + return false + } else { + return true + } + } + } + + res.on('finish', function () { + assert.equal(res.statusCode, 204) + assert.equal(res.getHeader('Vary'), 'Access-Control-Request-Headers') + cb() + }) + + cors(options)(req, res, function (err) { + cb(err || new Error('should not be called')) + }) + }) + + it('Vary: "Access-Control-Request-Headers" header is not set for pre-flight requests when shouldSetVaryHeader(req, \'Access-Control-Request-Headers\') configured to return false', function (done) { + var cb = after(1, done) + + var req = new FakeRequest('OPTIONS') + var res = new FakeResponse() + req.host = 'http://example.com' + req.originalUrl = '/images/asdf.png' + var options = { + origin: 'https://example.com', + shouldSetVaryHeader: function(req, header) { + if (header === 'Access-Control-Request-Headers') { + return false + } + + if (req.originalUrl.startsWith('/images') && header === 'Origin') { + return false + } else { + return true + } + } + } + + res.on('finish', function () { + assert.equal(res.statusCode, 204) + assert.equal(res.getHeader('Vary'), undefined) + cb() + }) + + cors(options)(req, res, function (err) { + cb(err || new Error('should not be called')) + }) + }) + + it('Vary: "Origin" header is set by default', function (done) { + var req = fakeRequest('GET') + var res = fakeResponse() + req.host = 'http://example.com' + req.originalUrl = '/images/asdf.png' + var options = { + origin: 'http://example.com' + } + + var next = function () { + // assert + assert.equal(res.statusCode, 200) + assert.equal(res.getHeader('Vary'), 'Origin') + done(); + }; + + cors(options)(req, res, next) + }) + + it('Vary: "Origin" header is not set when shouldSetVaryHeader(req, \'Origin\') configured to return false', function (done) { + var req = fakeRequest('GET') + var res = fakeResponse() + req.host = 'http://example.com' + req.originalUrl = '/images/asdf.png' + + var options = { + origin: ['http://example.com', 'http://foo.com'], + shouldSetVaryHeader: function(req, header) { + if (header === 'Access-Control-Request-Headers') { + return true + } + + if (req.originalUrl.startsWith('/images') && header === 'Origin') { + return false + } else { + return true + } + } + } + + var next = function () { + // assert + assert.equal(res.statusCode, 200) + assert.equal(res.getHeader('Vary'), undefined) + done(); + }; + + cors(options)(req, res, next) + }) + + it('Vary: "Origin" header is set when shouldSetVaryHeader(req, \'Origin\') configured to return true', function (done) { + var req = fakeRequest('GET') + var res = fakeResponse() + req.host = 'http://example.com' + + req.originalUrl = '/some/other/path' + + var options = { + origin: ['http://example.com', 'http://foo.com'], + shouldSetVaryHeader: function(req, header) { + if (header === 'Access-Control-Request-Headers') { + return true + } + + if (req.originalUrl.startsWith('/images') && header === 'Origin') { + return false + } else { + return true + } + } + } + + var next = function () { + // assert + assert.equal(res.statusCode, 200) + assert.equal(res.getHeader('Vary'), 'Origin') + done(); + }; + + cors(options)(req, res, next) + }) + }) + describe('passing static options', function () { it('overrides defaults', function (done) { var cb = after(1, done) From 970e19d02c2ae4c43ae7efb7d881fd06bbe87dd8 Mon Sep 17 00:00:00 2001 From: Jeremy Keys Date: Mon, 1 Apr 2024 10:46:11 -0600 Subject: [PATCH 2/6] PR feedback: use indexOf, per @joeyguerra --- test/test.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/test.js b/test/test.js index 21a8cd3..5ad19df 100644 --- a/test/test.js +++ b/test/test.js @@ -207,7 +207,7 @@ var util = require('util') return true } - if (req.originalUrl.startsWith('/images') && header === 'Origin') { + if (req.originalUrl.indexOf('/images') === 0 && header === 'Origin') { return false } else { return true @@ -240,7 +240,7 @@ var util = require('util') return false } - if (req.originalUrl.startsWith('/images') && header === 'Origin') { + if (req.originalUrl.indexOf('/images') === 0 && header === 'Origin') { return false } else { return true @@ -291,7 +291,7 @@ var util = require('util') return true } - if (req.originalUrl.startsWith('/images') && header === 'Origin') { + if (req.originalUrl.indexOf('/images') === 0 && header === 'Origin') { return false } else { return true @@ -323,7 +323,7 @@ var util = require('util') return true } - if (req.originalUrl.startsWith('/images') && header === 'Origin') { + if (req.originalUrl.indexOf('/images') === 0 && header === 'Origin') { return false } else { return true From 75e2ac514db468ea16d589266ec66cf9d326ff60 Mon Sep 17 00:00:00 2001 From: Jeremy Keys Date: Sun, 15 Sep 2024 18:30:19 -0600 Subject: [PATCH 3/6] revert version change --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 9b755d1..3368389 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "cors", "description": "Node.js CORS middleware", - "version": "2.9.0", + "version": "2.8.5", "author": "Troy Goode (https://github.com/troygoode/)", "license": "MIT", "keywords": [ From 3321019525ff1b6fe5c5c06dd0a6ee3c8f5580cf Mon Sep 17 00:00:00 2001 From: Jeremy Keys Date: Sun, 15 Sep 2024 18:53:10 -0600 Subject: [PATCH 4/6] Make shouldSetVaryHeader do nothing for OPTIONS requests; update tests to match; update README --- README.md | 1 + lib/index.js | 10 ++++------ test/test.js | 33 --------------------------------- 3 files changed, 5 insertions(+), 39 deletions(-) diff --git a/README.md b/README.md index c42d4a7..fc7d4a1 100644 --- a/README.md +++ b/README.md @@ -204,6 +204,7 @@ app.listen(80, function () { * `maxAge`: Configures the **Access-Control-Max-Age** CORS header. Set to an integer to pass the header, otherwise it is omitted. * `preflightContinue`: Pass the CORS preflight response to the next handler. * `optionsSuccessStatus`: Provides a status code to use for successful `OPTIONS` requests, since some legacy browsers (IE11, various SmartTVs) choke on `204`. +* `shouldSetVaryHeader`: takes a `req` and a header name, and determines whether the Vary header should be set for that header name. Does nothing for pre-flight OPTIONS requests. As an example, `shouldSetVaryHeader(req, header) { !req.originalUrl.startsWith('/images')) }` would set the Vary header to `Vary: Origin` for all request URIs besides those that start with `/images`; for requests that start with `/images`, it would not set the `Vary` header. This is mostly useful for working with caching and edge services that do not support the `Vary` header, such as [Cloudflare Polish](https://developers.cloudflare.com/images/polish/compression/); it is also useful for working with CDNs in general, because the `Vary` header can cause cache fragmentation and lower cache hit ratios depending on how it is configured. The default configuration is the equivalent of: diff --git a/lib/index.js b/lib/index.js index d433980..ab4af9b 100644 --- a/lib/index.js +++ b/lib/index.js @@ -102,12 +102,10 @@ if (!allowedHeaders) { allowedHeaders = req.headers['access-control-request-headers']; // .headers wasn't specified, so reflect the request headers - if (options.shouldSetVaryHeader(req, 'Access-Control-Request-Headers')) { - headers.push([{ - key: 'Vary', - value: 'Access-Control-Request-Headers' - }]); - } + headers.push([{ + key: 'Vary', + value: 'Access-Control-Request-Headers' + }]); } else if (allowedHeaders.join) { allowedHeaders = allowedHeaders.join(','); // .headers is an array, so turn it into a string } diff --git a/test/test.js b/test/test.js index 5ad19df..2edc4c3 100644 --- a/test/test.js +++ b/test/test.js @@ -226,39 +226,6 @@ var util = require('util') }) }) - it('Vary: "Access-Control-Request-Headers" header is not set for pre-flight requests when shouldSetVaryHeader(req, \'Access-Control-Request-Headers\') configured to return false', function (done) { - var cb = after(1, done) - - var req = new FakeRequest('OPTIONS') - var res = new FakeResponse() - req.host = 'http://example.com' - req.originalUrl = '/images/asdf.png' - var options = { - origin: 'https://example.com', - shouldSetVaryHeader: function(req, header) { - if (header === 'Access-Control-Request-Headers') { - return false - } - - if (req.originalUrl.indexOf('/images') === 0 && header === 'Origin') { - return false - } else { - return true - } - } - } - - res.on('finish', function () { - assert.equal(res.statusCode, 204) - assert.equal(res.getHeader('Vary'), undefined) - cb() - }) - - cors(options)(req, res, function (err) { - cb(err || new Error('should not be called')) - }) - }) - it('Vary: "Origin" header is set by default', function (done) { var req = fakeRequest('GET') var res = fakeResponse() From 861dd67b8112e70c94a386fa959b0e155c9debf7 Mon Sep 17 00:00:00 2001 From: Jeremy Keys Date: Sun, 15 Sep 2024 18:57:52 -0600 Subject: [PATCH 5/6] simplify tests to match simplified implementation --- test/test.js | 112 ++++++++++----------------------------------------- 1 file changed, 22 insertions(+), 90 deletions(-) diff --git a/test/test.js b/test/test.js index 2edc4c3..070182e 100644 --- a/test/test.js +++ b/test/test.js @@ -151,118 +151,58 @@ var util = require('util') }); describe('shouldSetVaryHeader', function() { - it('Vary: "Access-Control-Request-Headers" header is set by default for pre-flight requests', function (done) { - var cb = after(1, done) - - var req = new FakeRequest('OPTIONS') - var res = new FakeResponse() - req.host = 'http://example.com' - req.originalUrl = '/images/asdf.png' - - - res.on('finish', function () { - assert.equal(res.statusCode, 204) - assert.equal(res.getHeader('Vary'), 'Access-Control-Request-Headers') - cb() - }) - - cors()(req, res, function (err) { - cb(err || new Error('should not be called')) - }) - }) - - it('Vary: "Origin, Access-Control-Request-Headers" header is set by default for pre-flight requests with origins', function (done) { - var cb = after(1, done) - - var req = new FakeRequest('OPTIONS') - var res = new FakeResponse() - req.host = 'http://example.com' - req.originalUrl = '/images/asdf.png' - var options = { - origin: 'https://example.com' - } - - res.on('finish', function () { - assert.equal(res.statusCode, 204) - assert.equal(res.getHeader('Vary'), 'Origin, Access-Control-Request-Headers') - cb() - }) - - cors(options)(req, res, function (err) { - cb(err || new Error('should not be called')) - }) - }) - - it('Vary: "Access-Control-Request-Headers" header is set for pre-flight requests and shouldSetVaryHeader(req, \'Origin\') configured to return false with origins', function (done) { - var cb = after(1, done) - - var req = new FakeRequest('OPTIONS') - var res = new FakeResponse() + it('Vary: "Origin" header is set by default', function (done) { + var req = fakeRequest('GET') + var res = fakeResponse() req.host = 'http://example.com' req.originalUrl = '/images/asdf.png' var options = { - origin: 'https://example.com', - shouldSetVaryHeader: function(req, header) { - if (header === 'Access-Control-Request-Headers') { - return true - } - - if (req.originalUrl.indexOf('/images') === 0 && header === 'Origin') { - return false - } else { - return true - } - } + origin: 'http://example.com' } - res.on('finish', function () { - assert.equal(res.statusCode, 204) - assert.equal(res.getHeader('Vary'), 'Access-Control-Request-Headers') - cb() - }) + var next = function () { + // assert + assert.equal(res.statusCode, 200) + assert.equal(res.getHeader('Vary'), 'Origin') + done(); + }; - cors(options)(req, res, function (err) { - cb(err || new Error('should not be called')) - }) + cors(options)(req, res, next) }) - it('Vary: "Origin" header is set by default', function (done) { + it('Vary: "Origin" header is not set when shouldSetVaryHeader(req, \'Origin\') configured to return false', function (done) { var req = fakeRequest('GET') var res = fakeResponse() req.host = 'http://example.com' req.originalUrl = '/images/asdf.png' + var options = { - origin: 'http://example.com' + origin: ['http://example.com', 'http://foo.com'], + shouldSetVaryHeader: function(req, header) { + return req.originalUrl.indexOf('/images') !== 0 + } } var next = function () { // assert assert.equal(res.statusCode, 200) - assert.equal(res.getHeader('Vary'), 'Origin') + assert.equal(res.getHeader('Vary'), undefined) done(); }; cors(options)(req, res, next) }) - it('Vary: "Origin" header is not set when shouldSetVaryHeader(req, \'Origin\') configured to return false', function (done) { + it('Vary: "Origin" header is not set when shouldSetVaryHeader(req, \'Origin\') configured to return false and origin is string', function (done) { var req = fakeRequest('GET') var res = fakeResponse() req.host = 'http://example.com' req.originalUrl = '/images/asdf.png' var options = { - origin: ['http://example.com', 'http://foo.com'], + origin: 'http://example.com', shouldSetVaryHeader: function(req, header) { - if (header === 'Access-Control-Request-Headers') { - return true - } - - if (req.originalUrl.indexOf('/images') === 0 && header === 'Origin') { - return false - } else { - return true - } + return req.originalUrl.indexOf('/images') !== 0 } } @@ -286,15 +226,7 @@ var util = require('util') var options = { origin: ['http://example.com', 'http://foo.com'], shouldSetVaryHeader: function(req, header) { - if (header === 'Access-Control-Request-Headers') { - return true - } - - if (req.originalUrl.indexOf('/images') === 0 && header === 'Origin') { - return false - } else { - return true - } + return req.originalUrl.indexOf('/images') !== 0 } } From 41065753801a01385bf8cd511043a083945951ec Mon Sep 17 00:00:00 2001 From: Jeremy Keys Date: Sun, 15 Sep 2024 19:05:19 -0600 Subject: [PATCH 6/6] shouldSetVaryHeader -> shouldSetVaryOriginHeader --- README.md | 2 +- lib/index.js | 6 +++--- test/test.js | 14 +++++++------- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index fc7d4a1..c1bf5e0 100644 --- a/README.md +++ b/README.md @@ -204,7 +204,7 @@ app.listen(80, function () { * `maxAge`: Configures the **Access-Control-Max-Age** CORS header. Set to an integer to pass the header, otherwise it is omitted. * `preflightContinue`: Pass the CORS preflight response to the next handler. * `optionsSuccessStatus`: Provides a status code to use for successful `OPTIONS` requests, since some legacy browsers (IE11, various SmartTVs) choke on `204`. -* `shouldSetVaryHeader`: takes a `req` and a header name, and determines whether the Vary header should be set for that header name. Does nothing for pre-flight OPTIONS requests. As an example, `shouldSetVaryHeader(req, header) { !req.originalUrl.startsWith('/images')) }` would set the Vary header to `Vary: Origin` for all request URIs besides those that start with `/images`; for requests that start with `/images`, it would not set the `Vary` header. This is mostly useful for working with caching and edge services that do not support the `Vary` header, such as [Cloudflare Polish](https://developers.cloudflare.com/images/polish/compression/); it is also useful for working with CDNs in general, because the `Vary` header can cause cache fragmentation and lower cache hit ratios depending on how it is configured. +* `shouldSetVaryOriginHeader`: takes a `req`, and determines whether the `Vary: Origin` header should be set for that header name. Does nothing for pre-flight OPTIONS requests. As an example, `shouldSetVaryOriginHeader(req) { !req.originalUrl.startsWith('/images')) }` would set the Vary header to `Vary: Origin` for all request URIs besides those that start with `/images`; for requests that start with `/images`, it would not set the `Vary` header. This is mostly useful for working with caching and edge services that do not support the `Vary` header, such as [Cloudflare Polish](https://developers.cloudflare.com/images/polish/compression/); it is also useful for working with CDNs in general, because the `Vary` header can cause cache fragmentation and lower cache hit ratios depending on how it is configured. The default configuration is the equivalent of: diff --git a/lib/index.js b/lib/index.js index ab4af9b..c4ab7c7 100644 --- a/lib/index.js +++ b/lib/index.js @@ -10,7 +10,7 @@ methods: 'GET,HEAD,PUT,PATCH,POST,DELETE', preflightContinue: false, optionsSuccessStatus: 204, - shouldSetVaryHeader: function(req, header) { return true } + shouldSetVaryOriginHeader: function(req) { return true } }; function isString(s) { @@ -51,7 +51,7 @@ key: 'Access-Control-Allow-Origin', value: options.origin }]); - if (options.shouldSetVaryHeader(req, 'Origin')) { + if (options.shouldSetVaryOriginHeader(req)) { headers.push([{ key: 'Vary', value: 'Origin' @@ -64,7 +64,7 @@ key: 'Access-Control-Allow-Origin', value: isAllowed ? requestOrigin : false }]); - if (options.shouldSetVaryHeader(req, 'Origin')) { + if (options.shouldSetVaryOriginHeader(req)) { headers.push([{ key: 'Vary', value: 'Origin' diff --git a/test/test.js b/test/test.js index 070182e..9180877 100644 --- a/test/test.js +++ b/test/test.js @@ -150,7 +150,7 @@ var util = require('util') }) }); - describe('shouldSetVaryHeader', function() { + describe('shouldSetVaryOriginHeader', function() { it('Vary: "Origin" header is set by default', function (done) { var req = fakeRequest('GET') var res = fakeResponse() @@ -170,7 +170,7 @@ var util = require('util') cors(options)(req, res, next) }) - it('Vary: "Origin" header is not set when shouldSetVaryHeader(req, \'Origin\') configured to return false', function (done) { + it('Vary: "Origin" header is not set when shouldSetVaryOriginHeader(req) configured to return false', function (done) { var req = fakeRequest('GET') var res = fakeResponse() req.host = 'http://example.com' @@ -178,7 +178,7 @@ var util = require('util') var options = { origin: ['http://example.com', 'http://foo.com'], - shouldSetVaryHeader: function(req, header) { + shouldSetVaryOriginHeader: function(req) { return req.originalUrl.indexOf('/images') !== 0 } } @@ -193,7 +193,7 @@ var util = require('util') cors(options)(req, res, next) }) - it('Vary: "Origin" header is not set when shouldSetVaryHeader(req, \'Origin\') configured to return false and origin is string', function (done) { + it('Vary: "Origin" header is not set when shouldSetVaryOriginHeader(req) configured to return false and origin is string', function (done) { var req = fakeRequest('GET') var res = fakeResponse() req.host = 'http://example.com' @@ -201,7 +201,7 @@ var util = require('util') var options = { origin: 'http://example.com', - shouldSetVaryHeader: function(req, header) { + shouldSetVaryOriginHeader: function(req) { return req.originalUrl.indexOf('/images') !== 0 } } @@ -216,7 +216,7 @@ var util = require('util') cors(options)(req, res, next) }) - it('Vary: "Origin" header is set when shouldSetVaryHeader(req, \'Origin\') configured to return true', function (done) { + it('Vary: "Origin" header is set when shouldSetVaryOriginHeader(req) configured to return true', function (done) { var req = fakeRequest('GET') var res = fakeResponse() req.host = 'http://example.com' @@ -225,7 +225,7 @@ var util = require('util') var options = { origin: ['http://example.com', 'http://foo.com'], - shouldSetVaryHeader: function(req, header) { + shouldSetVaryOriginHeader: function(req) { return req.originalUrl.indexOf('/images') !== 0 } }