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.

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

Jump to bottom

Add Vary header only for non-static origin option #288

Merged
merged 1 commit into from
Jan 26, 2024
Merged

Add Vary header only for non-static origin option #288

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
15 changes: 6 additions & 9 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,11 @@ function normalizeCorsOptions (opts) {
}

function addCorsHeadersHandler (fastify, options, req, reply, next) {
// Always set Vary header
// https://github.com/rs/cors/issues/10
addOriginToVaryHeader(reply)
if (typeof options.origin !== 'string' && options.origin !== false) {
// Always set Vary header for non-static origin option
// https://fetch.spec.whatwg.org/#cors-protocol-and-http-caches
addOriginToVaryHeader(reply)
}

const resolveOriginOption = typeof options.origin === 'function' ? resolveOriginWrapper(fastify, options.origin) : (_, cb) => cb(null, options.origin)

Expand Down Expand Up @@ -263,13 +265,8 @@ function resolveOriginWrapper (fastify, origin) {
}

function getAccessControlAllowOriginHeader (reqOrigin, originOption) {
if (originOption === '*') {
// allow any origin
return '*'
}

if (typeof originOption === 'string') {
// fixed origin
// fixed or any origin ('*')
return originOption
}

Expand Down
51 changes: 24 additions & 27 deletions test/cors.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ test('Should add cors headers when payload is a stream', t => {
})

test('Should add cors headers (custom values)', t => {
t.plan(8)
t.plan(10)

const fastify = Fastify()
fastify.register(cors, {
Expand Down Expand Up @@ -94,7 +94,6 @@ test('Should add cors headers (custom values)', t => {
t.equal(res.payload, '')
t.match(res.headers, {
'access-control-allow-origin': 'example.com',
vary: 'Origin',
'access-control-allow-credentials': 'true',
'access-control-expose-headers': 'foo, bar',
'access-control-allow-methods': 'GET',
Expand All @@ -103,6 +102,7 @@ test('Should add cors headers (custom values)', t => {
'cache-control': 'max-age=321',
'content-length': '0'
})
t.notMatch(res.headers, { vary: 'Origin' })
})

fastify.inject({
Expand All @@ -115,16 +115,16 @@ test('Should add cors headers (custom values)', t => {
t.equal(res.payload, 'ok')
t.match(res.headers, {
'access-control-allow-origin': 'example.com',
vary: 'Origin',
'access-control-allow-credentials': 'true',
'access-control-expose-headers': 'foo, bar',
'content-length': '2'
})
t.notMatch(res.headers, { vary: 'Origin' })
})
})

test('Should support dynamic config (callback)', t => {
t.plan(16)
t.plan(18)

const configs = [{
origin: 'example.com',
Expand Down Expand Up @@ -175,11 +175,11 @@ test('Should support dynamic config (callback)', t => {
t.equal(res.payload, 'ok')
t.match(res.headers, {
'access-control-allow-origin': 'example.com',
vary: 'Origin',
Copy link
Contributor

Choose a reason for hiding this comment

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

the test case says Should support dynamic config (callback)

And should Vary on origin.

Copy link
Contributor

Choose a reason for hiding this comment

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

and now?

Copy link
Contributor

Choose a reason for hiding this comment

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

#291

'access-control-allow-credentials': 'true',
'access-control-expose-headers': 'foo, bar',
'content-length': '2'
})
t.notMatch(res.headers, { vary: 'Origin' })
})

fastify.inject({
Expand All @@ -196,7 +196,6 @@ test('Should support dynamic config (callback)', t => {
t.equal(res.payload, '')
t.match(res.headers, {
'access-control-allow-origin': 'sample.com',
vary: 'Origin',
'access-control-allow-credentials': 'true',
'access-control-expose-headers': 'zoo, bar',
'access-control-allow-methods': 'GET',
Expand All @@ -205,6 +204,7 @@ test('Should support dynamic config (callback)', t => {
'cache-control': '456',
'content-length': '0'
})
t.notMatch(res.headers, { vary: 'Origin' })
})

fastify.inject({
Expand All @@ -221,7 +221,7 @@ test('Should support dynamic config (callback)', t => {
})

test('Should support dynamic config (Promise)', t => {
t.plan(23)
t.plan(26)

const configs = [{
origin: 'example.com',
Expand Down Expand Up @@ -280,11 +280,11 @@ test('Should support dynamic config (Promise)', t => {
t.equal(res.payload, 'ok')
t.match(res.headers, {
'access-control-allow-origin': 'example.com',
vary: 'Origin',
'access-control-allow-credentials': 'true',
'access-control-expose-headers': 'foo, bar',
'content-length': '2'
})
t.notMatch(res.headers, { vary: 'Origin' })
})

fastify.inject({
Expand All @@ -301,14 +301,14 @@ test('Should support dynamic config (Promise)', t => {
t.equal(res.payload, '')
t.match(res.headers, {
'access-control-allow-origin': 'sample.com',
vary: 'Origin',
'access-control-allow-credentials': 'true',
'access-control-expose-headers': 'zoo, bar',
'access-control-allow-methods': 'GET',
'access-control-allow-headers': 'baz, foo',
'access-control-max-age': '321',
'content-length': '0'
})
t.notMatch(res.headers, { vary: 'Origin' })
t.equal(res.headers['cache-control'], undefined, 'cache-control omitted (invalid value)')
})

Expand All @@ -326,7 +326,6 @@ test('Should support dynamic config (Promise)', t => {
t.equal(res.payload, '')
t.match(res.headers, {
'access-control-allow-origin': 'sample.com',
vary: 'Origin',
'access-control-allow-credentials': 'true',
'access-control-expose-headers': 'zoo, bar',
'access-control-allow-methods': 'GET',
Expand All @@ -335,6 +334,7 @@ test('Should support dynamic config (Promise)', t => {
'cache-control': 'public, max-age=456', // cache-control included (custom string)
'content-length': '0'
})
t.notMatch(res.headers, { vary: 'Origin' })
})

fastify.inject({
Expand Down Expand Up @@ -564,7 +564,7 @@ test('Dynamic origin resolution (errored - promises)', t => {
})
})

test('Should reply 404 without cors headers other than `vary` when origin is false', t => {
test('Should reply 404 without cors headers when origin is false', t => {
t.plan(8)

const fastify = Fastify()
Expand Down Expand Up @@ -592,8 +592,7 @@ test('Should reply 404 without cors headers other than `vary` when origin is fal
t.same(res.headers, {
'content-length': '76',
'content-type': 'application/json; charset=utf-8',
connection: 'keep-alive',
vary: 'Origin'
connection: 'keep-alive'
})
})

Expand All @@ -608,8 +607,7 @@ test('Should reply 404 without cors headers other than `vary` when origin is fal
t.same(res.headers, {
'content-length': '2',
'content-type': 'text/plain; charset=utf-8',
connection: 'keep-alive',
vary: 'Origin'
connection: 'keep-alive'
})
})
})
Expand All @@ -632,14 +630,13 @@ test('Server error if origin option is falsy but not false', t => {
t.same(res.headers, {
'content-length': '89',
'content-type': 'application/json; charset=utf-8',
connection: 'keep-alive',
vary: 'Origin'
connection: 'keep-alive'
})
})
})

test('Allow only request from a specific origin', t => {
t.plan(4)
t.plan(5)

const fastify = Fastify()
fastify.register(cors, { origin: 'other.io' })
Expand All @@ -658,9 +655,9 @@ test('Allow only request from a specific origin', t => {
t.equal(res.statusCode, 200)
t.equal(res.payload, 'ok')
t.match(res.headers, {
'access-control-allow-origin': 'other.io',
vary: 'Origin'
'access-control-allow-origin': 'other.io'
})
t.notMatch(res.headers, { vary: 'Origin' })
})
})

Expand Down Expand Up @@ -772,11 +769,11 @@ test('Disable preflight', t => {
})
})

test('Should always add vary header to `Origin` by default', t => {
test('Should always add vary header to `Origin` for reflected origin', t => {
t.plan(12)

const fastify = Fastify()
fastify.register(cors)
fastify.register(cors, { origin: true })

fastify.get('/', (req, reply) => {
reply.send('ok')
Expand Down Expand Up @@ -829,15 +826,15 @@ test('Should always add vary header to `Origin` by default', t => {
})
})

test('Should always add vary header to `Origin` by default (vary is array)', t => {
test('Should always add vary header to `Origin` for reflected origin (vary is array)', t => {
t.plan(4)

const fastify = Fastify()

// Mock getHeader function
fastify.decorateReply('getHeader', (name) => ['foo', 'bar'])

fastify.register(cors)
fastify.register(cors, { origin: true })

fastify.get('/', (req, reply) => {
reply.send('ok')
Expand All @@ -858,7 +855,7 @@ test('Should always add vary header to `Origin` by default (vary is array)', t =
})

test('Allow only request from with specific headers', t => {
t.plan(7)
t.plan(8)

const fastify = Fastify()
fastify.register(cors, {
Expand All @@ -882,9 +879,9 @@ test('Allow only request from with specific headers', t => {
delete res.headers.date
t.equal(res.statusCode, 204)
t.match(res.headers, {
'access-control-allow-headers': 'foo',
vary: 'Origin'
'access-control-allow-headers': 'foo'
})
t.notMatch(res.headers, { vary: 'Origin' })
})

fastify.inject({
Expand Down
Loading