Skip to content

Commit

Permalink
Merge pull request from GHSA-qxrj-hx23-xp82
Browse files Browse the repository at this point in the history 
This is a security update for the Same Origin Policy (SOP),
and also a BREAKING CHANGE.

closes GHSA-qxrj-hx23-xp82
  • Loading branch information
@fengmk2
fengmk2 authored Dec 11, 2023
1 parent 0f3f948 commit f31dac9
Show file tree
Hide file tree
Showing 5 changed files with 83 additions and 26 deletions.
4 changes: 1 addition & 3 deletions .github/workflows/nodejs.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ on:
branches:
- main
- master
schedule:
- cron: '0 2 * * *'

jobs:
build:
Expand All @@ -22,7 +20,7 @@ jobs:
strategy:
fail-fast: false
matrix:
node-version: [14, 16, 18]
node-version: [14, 16, 18, 20]
os: [ubuntu-latest]

steps:
Expand Down
18 changes: 15 additions & 3 deletions README.md
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
@koa/cors
=======
# @koa/cors

[![NPM version][npm-image]][npm-url]
[![Node.js CI](https://github.com/koajs/cors/actions/workflows/nodejs.yml/badge.svg)](https://github.com/koajs/cors/actions/workflows/nodejs.yml)
Expand Down Expand Up @@ -43,7 +42,8 @@ app.use(cors());
* CORS middleware
*
* @param {Object} [options]
* - {String|Function(ctx)} origin `Access-Control-Allow-Origin`, default is request Origin header
* - {String|Function(ctx)} origin `Access-Control-Allow-Origin`, default is '*'
* If `credentials` set and return `true, the `origin` default value will set to the request `Origin` header
* - {String|Array} allowMethods `Access-Control-Allow-Methods`, default is 'GET,HEAD,PUT,POST,DELETE,PATCH'
* - {String|Array} exposeHeaders `Access-Control-Expose-Headers`
* - {String|Array} allowHeaders `Access-Control-Allow-Headers`
Expand All @@ -57,6 +57,18 @@ app.use(cors());
*/
```

## Breaking change between 5.0 and 4.0

The default `origin` is set to `*`, if you want to keep the 4.0 behavior, you can set the `origin` handler like this:

```js
app.use(cors({
origin(ctx) {
return ctx.get('Origin') || '*';
},
}));
```

## License

[MIT](./LICENSE)
Expand Down
11 changes: 6 additions & 5 deletions index.js
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
'use strict';

const vary = require('vary');

/**
* CORS middleware
*
* @param {Object} [options]
* - {String|Function(ctx)} origin `Access-Control-Allow-Origin`, default is request Origin header
* - {String|Function(ctx)} origin `Access-Control-Allow-Origin`, default is '*'
* If `credentials` set and return `true, the `origin` default value will set to the request `Origin` header
* - {String|Array} allowMethods `Access-Control-Allow-Methods`, default is 'GET,HEAD,PUT,POST,DELETE,PATCH'
* - {String|Array} exposeHeaders `Access-Control-Expose-Headers`
* - {String|Array} allowHeaders `Access-Control-Allow-Headers`
Expand Down Expand Up @@ -61,9 +60,11 @@ module.exports = function(options) {
let origin;
if (typeof options.origin === 'function') {
origin = await options.origin(ctx);
if (!origin) return await next();
if (!origin) {
return await next();
}
} else {
origin = options.origin || requestOrigin;
origin = options.origin || '*';

This comment has been minimized.

Sign in to view

Copy link
@julienw

julienw Dec 12, 2023 •
edited
Loading

Contributor

I'm puzzled because I don't see how this fixes the security advisory. Indeed '*' means any origin, so this is essentially the same behavior as before. Unless I'm missing something.

I believe the advisory author would prefer that this behavior is explicit, that is as a user of the library I should specify "*" explicitely in the origin parameter.

(I would myself be happy enough with just a stronger emphasis in the doc)

This comment has been minimized.

Sign in to view

Copy link
@julienw

julienw Dec 12, 2023

Contributor

It looks like there is a different behavior related to cookies, but my understanding is that it's not the subject of the advisory.

This comment has been minimized.

Sign in to view

Copy link
@fengmk2

fengmk2 Dec 12, 2023

Author Member

Turning on this middleware means that cors will be turned on by default and return *.

You are more than welcome to contribute to documentation improvements :)

This comment has been minimized.

Sign in to view

Copy link
@julienw

julienw Dec 12, 2023

Contributor

That's what I mean: this doesn't address the concerns in the security advisory, to quote it: "This behavior completely disables one of the most crucial elements of browsers - the Same Origin Policy (SOP), this could cause a very serious security threat to the users of this middleware."

(again, I don't necessarily agree with it, I think users should already know the implications when they enable it)

This comment has been minimized.

Sign in to view

Copy link
@julienw

julienw Dec 12, 2023

Contributor

I made #91 to update the documentation, tell me what you think :-)

This comment has been minimized.

Sign in to view

Copy link
@Beace

Beace Dec 12, 2023

@fengmk2 @julienw I actually have a similar question, if credentials is true, the behavior before and after this change is consistent. Because no matter what the origin is, it will be set as requestOrigin. But it is crucial for credentials to be true. I don't understand the significance of this change.

This comment has been minimized.

Sign in to view

Copy link
@fengmk2

fengmk2 Dec 13, 2023

Author Member

@Beace This modification is to ensure that the default configuration is the lowest risk. Setting credentials is the expected behavior of developers who explicitly want to return requestOrigin.
The previous default configuration returned requestOrigin, which was very risky.

This comment has been minimized.

Sign in to view

Copy link
@julienw

julienw Jan 4, 2024

Contributor

Can you please elaborate on the risks you're mentioning when you write "lowest risk" and "risky"?

Here is my understanding:

  • without CORS, the same-origin policy prevents webpage from doing requests to servers with another origin. (except <script> and friends)
  • with CORS, a server can define which webpages can do requests to this server (and methods, etc)
  • returning requestOrigin or * is basically identical (any webpage can do a request) except for one point: credentials (cookies and other similar things): requests need to be made without credentials https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS/Errors/CORSNotSupportingCredentials
  • My understanding is that without credentials, Access-Control-Allow-Credentials wasn't set before, so credentials were already rejected with the previous version, even when returning requestOrigin. Therefore I think that the change doesn't change anything in practice.
  • Therefore I don't see that the current situation is "the lowest risk". The lowest risk would be to make the option origin mandatory, and do not add the header if it's not present.

Note that the advisory doesn't say anything about cookies or credentials, but instead about the fact that the middleware is configured by default to allow all origins. This change doesn't change that!

Please tell me if I missed anything :-)

This comment has been minimized.

Sign in to view

Copy link
@prabhu

prabhu Jan 25, 2024

#PointlessCVE
}

let credentials;
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
"node": ">= 14.0.0"
},
"ci": {
"version": "14, 16, 18",
"version": "14, 16, 18, 20",
"os": "linux"
},
"author": "fengmk2 <fengmk2@gmail.com> (http://github.com/fengmk2)",
Expand Down
74 changes: 60 additions & 14 deletions test/cors.test.js
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
'use strict';

const assert = require('assert');
const Koa = require('koa');
const request = require('supertest');
Expand All @@ -13,22 +11,19 @@ describe('cors.test.js', function() {
ctx.body = { foo: 'bar' };
});

it('should not set `Access-Control-Allow-Origin` when request Origin header missing', function(done) {
it('should set `Access-Control-Allow-Origin` to `*` when request Origin header missing', function(done) {
request(app.listen())
.get('/')
.expect({ foo: 'bar' })
.expect(200, function(err, res) {
assert(!err);
assert(!res.headers['access-control-allow-origin']);
done();
});
.expect('access-control-allow-origin', '*')
.expect(200, done);
});

it('should set `Access-Control-Allow-Origin` to request origin header', function(done) {
it('should set `Access-Control-Allow-Origin` to `*`', function(done) {
request(app.listen())
.get('/')
.set('Origin', 'http://koajs.com')
.expect('Access-Control-Allow-Origin', 'http://koajs.com')
.expect('Access-Control-Allow-Origin', '*')
.expect({ foo: 'bar' })
.expect(200, done);
});
Expand All @@ -38,7 +33,7 @@ describe('cors.test.js', function() {
.options('/')
.set('Origin', 'http://koajs.com')
.set('Access-Control-Request-Method', 'PUT')
.expect('Access-Control-Allow-Origin', 'http://koajs.com')
.expect('Access-Control-Allow-Origin', '*')
.expect('Access-Control-Allow-Methods', 'GET,HEAD,PUT,POST,DELETE,PATCH')
.expect(204, done);
});
Expand Down Expand Up @@ -87,6 +82,44 @@ describe('cors.test.js', function() {
});
});

describe('options.origin set the request Origin header', function() {
const app = new Koa();
app.use(cors({
origin(ctx) {
return ctx.get('Origin') || '*';
},
}));
app.use(function(ctx) {
ctx.body = { foo: 'bar' };
});

it('should set `Access-Control-Allow-Origin` to request `Origin` header', function(done) {
request(app.listen())
.get('/')
.set('Origin', 'http://koajs.com')
.expect('Access-Control-Allow-Origin', 'http://koajs.com')
.expect({ foo: 'bar' })
.expect(200, done);
});

it('should set `Access-Control-Allow-Origin` to request `origin` header', function(done) {
request(app.listen())
.get('/')
.set('origin', 'http://origin.koajs.com')
.expect('Access-Control-Allow-Origin', 'http://origin.koajs.com')
.expect({ foo: 'bar' })
.expect(200, done);
});

it('should set `Access-Control-Allow-Origin` to `*`, even if no Origin is passed on request', function(done) {
request(app.listen())
.get('/')
.expect('Access-Control-Allow-Origin', '*')
.expect({ foo: 'bar' })
.expect(200, done);
});
});

describe('options.secureContext=true', function() {
const app = new Koa();
app.use(cors({
Expand Down Expand Up @@ -651,7 +684,11 @@ describe('cors.test.js', function() {
describe('options.headersKeptOnError', function() {
it('should keep CORS headers after an error', function(done) {
const app = new Koa();
app.use(cors());
app.use(cors({
origin(ctx) {
return ctx.get('Origin') || '*';
},
}));
app.use(function(ctx) {
ctx.body = { foo: 'bar' };
throw new Error('Whoops!');
Expand All @@ -668,7 +705,11 @@ describe('cors.test.js', function() {

it('should not affect OPTIONS requests', function(done) {
const app = new Koa();
app.use(cors());
app.use(cors({
origin(ctx) {
return ctx.get('Origin') || '*';
},
}));
app.use(function(ctx) {
ctx.body = { foo: 'bar' };
throw new Error('Whoops!');
Expand All @@ -684,7 +725,11 @@ describe('cors.test.js', function() {

it('should not keep unrelated headers', function(done) {
const app = new Koa();
app.use(cors());
app.use(cors({
origin(ctx) {
return ctx.get('Origin') || '*';
},
}));
app.use(function(ctx) {
ctx.body = { foo: 'bar' };
ctx.set('X-Example', 'Value');
Expand Down Expand Up @@ -752,6 +797,7 @@ describe('cors.test.js', function() {
.expect(200, done);
});
});

describe('other middleware has set vary header on Error', function() {
it('should append `Origin to other `Vary` header', function(done) {
const app = new Koa();
Expand Down

0 comments on commit f31dac9

Please sign in to comment.