Skip to content

Commit e4cbcf0

Browse files
committed
Fix CORS preflight handling and origin matching
- Add OPTIONS handler to respond 204 to preflight requests; without it they fell through to the 404 handler - Fix origin matching in findMatchingOrigin to use exact equality instead of the inverted substring check that caused subdomains to be rejected - Expand auto-origin FQDNs to http/https variants to match the Origin header format browsers send - Remove the host header fallback, which matched the destination not the source and had no intentional rationale - Replace node-fetch with native fetch in tests, which is now possible since tests send Origin instead of Host
1 parent 78055be commit e4cbcf0

8 files changed

Lines changed: 17 additions & 104 deletions

File tree

package-lock.json

Lines changed: 0 additions & 92 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

package.json

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,6 @@
6464
"is-url": "^1.2.4",
6565
"is-valid-domain": "^0.1.6",
6666
"mongoose": "^9.2.1",
67-
"node-fetch": "^3.3.2",
6867
"node-schedule": "^2.1.1",
6968
"normalize-url": "^9.0.0",
7069
"request-ip": "^3.3.0",

src/server.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,11 @@ const apolloServer = createApolloServer({
6969
// Apply CORS middleware
7070
app.use(attachCorsHeaders)
7171

72+
// Respond to preflight requests
73+
app.options('/{*path}', (request, response) => {
74+
response.sendStatus(204)
75+
})
76+
7277
// Serve static files
7378
app.get('/', async (request, response) => {
7479
response.setHeader('Content-Type', 'text/html; charset=utf-8')

src/utils/findMatchingOrigin.js

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
import fullyQualifiedDomainNames from './fullyQualifiedDomainNames.js'
22

33
const findOrigin = (request, origins) => {
4-
return origins.find((origin) => origin.includes(request.headers.origin) || origin.includes(request.headers.host))
4+
return origins.find((origin) => origin === request.headers.origin)
55
}
66

77
export default async (request, allowedOrigins, autoOrigin) => {
88
if (autoOrigin === true) {
9-
const origins = await fullyQualifiedDomainNames()
9+
const names = await fullyQualifiedDomainNames()
10+
const origins = names.flatMap((name) => [`http://${name}`, `https://${name}`])
1011
return findOrigin(request, origins)
1112
}
1213

test/_utils.js

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,10 @@
1-
import fetch from 'node-fetch'
2-
31
export const api = async (base, body, token, headers = {}) => {
42
const url = new URL('/api', await base)
53

64
const defaultHeaders = {}
75
defaultHeaders['Content-Type'] = 'application/json'
86
defaultHeaders['Authorization'] = token == null ? undefined : `Bearer ${token}`
97

10-
// Use node-fetch as the built-in fetch does not support modifying the Host header,
11-
// which is required for some tests.
128
const result = await fetch(url.href, {
139
method: 'post',
1410
body: JSON.stringify(body),

test/serverWithAutoCors.js

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,18 @@ test('return cors headers based on fully qualifed domain names', async (t) => {
2525
ACKEE_AUTO_ORIGIN: 'true',
2626
})
2727

28-
const { headers: fqdnHeaders } = await api(base, { query: '{ __typename }' }, null, { Host: 'fqdn.example.com' })
28+
const { headers: fqdnHeaders } = await api(base, { query: '{ __typename }' }, null, {
29+
Origin: 'https://fqdn.example.com',
30+
})
2931

30-
t.is(fqdnHeaders.get('Access-Control-Allow-Origin'), 'fqdn.example.com')
32+
t.is(fqdnHeaders.get('Access-Control-Allow-Origin'), 'https://fqdn.example.com')
3133
t.is(fqdnHeaders.get('Access-Control-Allow-Methods'), 'GET, POST, PATCH, OPTIONS')
3234
t.is(fqdnHeaders.get('Access-Control-Allow-Headers'), 'Content-Type, Authorization, Time-Zone')
3335
t.is(fqdnHeaders.get('Access-Control-Allow-Credentials'), 'true')
3436

35-
const { headers: noFqdnHeaders } = await api(base, { query: '{ __typename }' }, null, { Host: 'No fqdn' })
37+
const { headers: noFqdnHeaders } = await api(base, { query: '{ __typename }' }, null, {
38+
Origin: 'https://not-in-list.example.com',
39+
})
3640

3741
t.is(noFqdnHeaders.get('Access-Control-Allow-Origin'), null)
3842
t.is(noFqdnHeaders.get('Access-Control-Allow-Methods'), null)

test/serverWithCors.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ test('return cors headers if env var specifies one', async (t) => {
2020
ACKEE_ALLOW_ORIGIN: origin,
2121
})
2222

23-
const { headers } = await api(base, { query: '{ __typename }' })
23+
const { headers } = await api(base, { query: '{ __typename }' }, null, { Origin: origin })
2424

2525
t.is(headers.get('Access-Control-Allow-Origin'), origin)
2626
t.is(headers.get('Access-Control-Allow-Methods'), 'GET, POST, PATCH, OPTIONS')

test/serverWithMultipleCors.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ test('return cors headers with corresponding origin if env var specifies multipl
2020
ACKEE_ALLOW_ORIGIN: `https://example.com,${origin}`,
2121
})
2222

23-
const { headers } = await api(base, { query: '{ __typename }' })
23+
const { headers } = await api(base, { query: '{ __typename }' }, null, { Origin: origin })
2424

2525
t.is(headers.get('Access-Control-Allow-Origin'), origin)
2626
t.is(headers.get('Access-Control-Allow-Methods'), 'GET, POST, PATCH, OPTIONS')

0 commit comments

Comments
 (0)