Skip to content

New generic mode that always returns * (less secure, better caching) #32

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

New generic mode that always returns * (less secure, better caching) #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
const corsMiddleware = require('restify-cors-middleware')

const cors = corsMiddleware({
preflightMaxAge: 5, //Optional
preflightMaxAge: 5,
origins: ['http://api.myapp.com', 'http://web.myapp.com'],
allowHeaders: ['API-Token'],
exposeHeaders: ['API-Token-Expiry']
Expand All @@ -31,23 +31,25 @@ server.use(cors.actual)

## Allowed origins

You can specify the full list of domains and subdomains allowed in your application:
You can specify the full list of domains and subdomains allowed in your application.
As a convenience method, you can use the `*` character as a wildcard.

```js
origins: [
'http://myapp.com',
'http://*.myapp.com'
'http://*.myotherapp.com'
]
```

For added security, this middleware sets `Access-Control-Allow-Origin` to the origin that matched, not the configured wildcard.
This means callers won't know about other domains that are supported.
The `Access-Control-Allow-Origin` header will be set to the actual origin that matched, on a per-request basis. The person making the request will not know about the full configuration, like other allowed domains or any wildcards in use.

Setting `origins: ['*']` is also valid, although it comes with obvious security implications. Note that it will still return a customised response (matching Origin), so any caching layer (reverse proxy or CDN) will grow in size accordingly.
The main side-effect is that every response will include `Vary: Origin`, since the response headers depend on the origin. This is the safest setup, but will decrease your cache hit-rate / increase your cache size with every origin.

## Troubleshooting
## Open CORS setup

As per the spec, requests without an `Origin` will not receive any headers. Requests with a matching `Origin` will receive the appropriate response headers. Always be careful that any reverse proxies (e.g. Varnish) very their cache depending on the origin, so you don't serve CORS headers to the wrong request.
Using `origins: ['*']` is also a valid setup, which comes with obvious security implications. This means **any** domain will be able to query your API. However it does have performance benefits. When using `['*']`, the middleware always responds with `Access-Control-Allow-Origin: *` which means responses can be cached regardless of origins.

Each API should weigh the security and performance angles before choosing this approach.

## Compliance to the spec

Expand Down
32 changes: 18 additions & 14 deletions src/actual.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,25 @@ exports.handler = function (options) {
return function (req, res, next) {
var originHeader = req.headers['origin']

// If either no origin was set, or the origin isn't supported, continue
// without setting any headers
if (!originHeader || !matcher(originHeader)) {
if (originMatcher.generic(options.origins)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to handle this without treating the whole block as a special case? A nested if/else of this size increases the complexity of the code base quite a lot and fragments related changes across multiple lines in the code base.

res.setHeader(constants['AC_ALLOW_ORIGIN'], '*')
res.setHeader(constants['AC_ALLOW_CREDS'], false) // not compatible with *
res.setHeader(constants['AC_EXPOSE_HEADERS'], options.exposeHeaders.join(', '))
return next()
} else {
// If either no origin was set, or the origin isn't supported, continue
// without setting any headers
if (!originHeader || !matcher(originHeader)) {
return next()
}
// if match was found, let's set some headers.
res.setHeader(constants['AC_ALLOW_ORIGIN'], originHeader)
res.setHeader(constants['STR_VARY'], constants['STR_ORIGIN'])
if (options.credentials) {
res.setHeader(constants['AC_ALLOW_CREDS'], 'true')
}
res.setHeader(constants['AC_EXPOSE_HEADERS'], options.exposeHeaders.join(', '))
return next()
}

// if match was found, let's set some headers.
res.setHeader(constants['AC_ALLOW_ORIGIN'], originHeader)
res.setHeader(constants['STR_VARY'], constants['STR_ORIGIN'])
if (options.credentials) {
res.setHeader(constants['AC_ALLOW_CREDS'], 'true')
}
res.setHeader(constants['AC_EXPOSE_HEADERS'],
options.exposeHeaders.join(', '))

return next()
}
}
4 changes: 4 additions & 0 deletions src/origin-matcher.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@

exports.generic = function (list) {
return list.length === 1 && list[0] === '*'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a case where we would want to explicitly match some domains, and wildcard the rest? Would it be surprising that ['*'] would have one behaviour for a domain matched against the wildcard and an entirely different behaviour for a domain matched against the wildcard in this list: ['foo.bar', '*']

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that's quite strange, not 100% happy with that design. Do you think it would make more sense to have some sort of option flag like

{
  // specify one or more origins, wildcard supported
  // will only return the matched origin, with a Vary header
  origins: ['https://myapp.com', 'https://*.myapp.com']

  // or use the generic mode
  // which always returns "Accept", and can be cached across the board
  // but has security implications
  allowAllOrigins: true
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the allowAllOrigins override. It seems like it would both simplify the implementation and make the user's intent explicit.

}

exports.create = function (allowedOrigins) {
// pre-compile list of matchers, so regexes are only built once
var matchers = allowedOrigins.map(createMatcher)
Expand Down
84 changes: 56 additions & 28 deletions src/preflight.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,42 +3,70 @@ var constants = require('./constants.js')

exports.handler = function (options) {
var matcher = originMatcher.create(options.origins)
return function (req, res, next) {
if (req.method !== 'OPTIONS') return next()

// 6.2.1 and 6.2.2
var originHeader = req.headers['origin']
if (!matcher(originHeader)) return next()
if (originMatcher.generic(options.origins)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above, but to a much greater extent. Is there a way we can do this that doesn't treat the whole block as a special case? We are adding an incredibly amount of complexity here to support this.

//
// If origins = ['*'] then we always set generic CORS headers
// This is the simplest case, similar to what restify.fullResponse() used to do
// Must must keep the headers generic because they can be cached by reverse proxies
//

// 6.2.3
var requestedMethod = req.headers[constants['AC_REQ_METHOD']]
if (!requestedMethod) return next()
return function (req, res, next) {
if (req.method !== 'OPTIONS') return next()
res.once('header', function () {
res.header(constants['AC_ALLOW_ORIGIN'], '*')
res.header(constants['AC_ALLOW_CREDS'], false) // not compatible with *
res.header(constants['AC_ALLOW_METHODS'], 'GET, PUT, POST, DELETE, OPTIONS')
res.header(constants['AC_ALLOW_HEADERS'], options.allowHeaders.join(', '))
})
res.send(constants['HTTP_NO_CONTENT'])
}
} else {
//
// Full CORS mode
// This is the "better" option where we have a list of origins
// In this case, we return customised CORS headers for each request
// And must set the "Vary: Origin" header
//

// 6.2.4
// var requestedHeaders = req.headers[constants['AC_REQ_HEADERS']]
// requestedHeaders = requestedHeaders ? requestedHeaders.split(', ') : []
return function (req, res, next) {
if (req.method !== 'OPTIONS') return next()

var allowedMethods = [requestedMethod, 'OPTIONS']
var allowedHeaders = constants['ALLOW_HEADERS']
.concat(options.allowHeaders)
// 6.2.1 and 6.2.2
var originHeader = req.headers['origin']
if (!matcher(originHeader)) return next()

res.once('header', function () {
// 6.2.7
res.header(constants['AC_ALLOW_ORIGIN'], originHeader)
res.header(constants['AC_ALLOW_CREDS'], true)
// 6.2.3
var requestedMethod = req.headers[constants['AC_REQ_METHOD']]
if (!requestedMethod) return next()

// 6.2.8
if (options.preflightMaxAge) {
res.header(constants['AC_MAX_AGE'], options.preflightMaxAge)
}
// 6.2.4
// var requestedHeaders = req.headers[constants['AC_REQ_HEADERS']]
// requestedHeaders = requestedHeaders ? requestedHeaders.split(', ') : []
var allowedMethods = [requestedMethod, 'OPTIONS']
var allowedHeaders = constants['ALLOW_HEADERS'].concat(options.allowHeaders)

// 6.2.9
res.header(constants['AC_ALLOW_METHODS'], allowedMethods.join(', '))
res.once('header', function () {
// 6.2.7
res.header(constants['AC_ALLOW_ORIGIN'], originHeader)
res.header(constants['AC_ALLOW_CREDS'], true)

// 6.2.10
res.header(constants['AC_ALLOW_HEADERS'], allowedHeaders.join(', '))
})
// 6.2.8
if (options.preflightMaxAge) {
res.header(constants['AC_MAX_AGE'], options.preflightMaxAge)
}

res.send(constants['HTTP_NO_CONTENT'])
// 6.2.9
res.header(constants['AC_ALLOW_METHODS'], allowedMethods.join(', '))

// 6.2.10
res.header(constants['AC_ALLOW_HEADERS'], allowedHeaders.join(', '))

// 6.4
res.header('Vary', 'Origin')
})

res.send(constants['HTTP_NO_CONTENT'])
}
}
}
26 changes: 26 additions & 0 deletions test/cors.actual.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,4 +94,30 @@ describe('CORS: simple / actual requests', function () {
.expect(200)
.end(done)
})

xit('6.4 FIXME Does not need the Vary header if it accepts all Origins', function (done) {
var server = test.corsServer({
origins: ['*']
})
request(server)
.get('/test')
.set('Origin', 'http://api.myapp.com')
.expect('access-control-allow-origin', '*')
.expect(test.noHeader('vary'))
.expect(200)
.end(done)
})

xit('6.4 FIXME Sets the Vary header if it returns the actual origin', function (done) {
var server = test.corsServer({
origins: ['http://api.myapp.com']
})
request(server)
.get('/test')
.set('Origin', 'http://api.myapp.com')
.expect('access-control-allow-origin', 'http://api.myapp.com')
.expect('vary', 'Origin')
.expect(200)
.end(done)
})
})
Loading