Skip to content

Commit ee19a0a

Browse files
daffldaffl
authored
fix(oauth): Patch open redirect and origin validation (#3653)
1 parent e4c0dac commit ee19a0a

File tree

4 files changed

+245
-2
lines changed

4 files changed

+245
-2
lines changed

‎packages/authentication-oauth/src/service.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,10 @@ export class OAuthService {
170170

171171
session.redirect = redirect
172172
session.query = restQuery
173-
session.headers = headers
173+
// Only store the referer header needed for origin validation
174+
session.headers = {
175+
referer: headers?.referer
176+
}
174177

175178
return this.handler('GET', handlerParams, {})
176179
}

‎packages/authentication-oauth/src/strategy.ts‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,17 @@ export class OAuthStrategy extends AuthenticationBaseStrategy {
7272

7373
if (Array.isArray(origins)) {
7474
const referer = params?.headers?.referer || origins[0]
75-
const allowedOrigin = origins.find((current) => referer.toLowerCase().startsWith(current.toLowerCase()))
75+
76+
// Parse the referer to get its origin for proper comparison
77+
let refererOrigin: string
78+
try {
79+
refererOrigin = new URL(referer).origin
80+
} catch {
81+
throw new NotAuthenticated(`Invalid referer "${referer}".`)
82+
}
83+
84+
// Compare full origins
85+
const allowedOrigin = origins.find((current) => refererOrigin.toLowerCase() === current.toLowerCase())
7686

7787
if (!allowedOrigin) {
7888
throw new NotAuthenticated(`Referer "${referer}" is not allowed.`)
@@ -95,6 +105,13 @@ export class OAuthStrategy extends AuthenticationBaseStrategy {
95105
return null
96106
}
97107

108+
// Validate redirect parameter to prevent open redirect via URL authority injection
109+
// Reject characters that could change the URL's authority: @, //, \
110+
// e.g., @attacker.com would make https://target.com@attacker.com redirect to attacker.com
111+
if (queryRedirect && /[@\\]|^\/\/|\/\//.test(queryRedirect)) {
112+
throw new NotAuthenticated('Invalid redirect path.')
113+
}
114+
98115
const redirectUrl = `${redirect}${queryRedirect}`
99116
const separator = redirectUrl.endsWith('?') ? '' : redirect.indexOf('#') !== -1 ? '?' : '#'
100117
const authResult: AuthenticationResult = data

‎packages/authentication-oauth/test/service.test.ts‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,84 @@ import axios, { AxiosResponse } from 'axios'
33
import { CookieJar } from 'tough-cookie'
44
import { expressFixture } from './utils/fixture'
55

6+
describe('@feathersjs/authentication-oauth service security', () => {
7+
const port = 9780
8+
const req = axios.create({
9+
withCredentials: true,
10+
maxRedirects: 0
11+
})
12+
let app: Awaited<ReturnType<typeof expressFixture>>
13+
14+
const fetchErrorResponse = async (url: string, headers?: Record<string, string>): Promise<AxiosResponse> => {
15+
try {
16+
await req.get(url, { headers })
17+
} catch (error: any) {
18+
return error.response
19+
}
20+
assert.fail('Should never get here')
21+
}
22+
23+
before(async () => {
24+
app = await expressFixture(port, 5117)
25+
})
26+
27+
after(async () => {
28+
await app.teardown()
29+
})
30+
31+
describe('internal headers exposure via session cookie', () => {
32+
it('should not store sensitive internal headers in session cookie', async () => {
33+
const host = `http://localhost:${port}`
34+
const location = `${host}/oauth/github`
35+
36+
// Make request with internal/sensitive headers that might be added by proxies
37+
const oauthResponse = await fetchErrorResponse(location, {
38+
'x-forwarded-for': '10.0.0.1',
39+
'x-internal-api-key': 'sk_live_secret123',
40+
'x-real-ip': '192.168.1.1'
41+
})
42+
43+
assert.equal(oauthResponse.status, 303)
44+
45+
// Get the session cookie
46+
const cookies = oauthResponse.headers['set-cookie']
47+
assert.ok(cookies, 'Should have set-cookie header')
48+
49+
// Find the oauth session cookie (express cookie-session uses 'feathers.oauth')
50+
const oauthCookie = cookies.find((c: string) => c.startsWith('feathers.oauth='))
51+
assert.ok(oauthCookie, 'Should have feathers.oauth session cookie')
52+
53+
// Extract the cookie value and decode it
54+
const match = oauthCookie.match(/feathers\.oauth=([^;]+)/)
55+
assert.ok(match, 'Should be able to extract cookie value')
56+
57+
const cookieValue = decodeURIComponent(match[1])
58+
// Cookie session uses base64 encoding
59+
const decoded = Buffer.from(cookieValue, 'base64').toString('utf-8')
60+
const sessionData = JSON.parse(decoded)
61+
62+
// The vulnerability: all headers are stored in session.headers
63+
// This test should FAIL if headers object contains sensitive internal headers
64+
assert.ok(sessionData.headers, 'Session should have headers stored')
65+
66+
// These assertions verify the FIX is in place - they should FAIL currently
67+
// because the vulnerable code stores ALL headers
68+
const storedHeaderKeys = Object.keys(sessionData.headers).map((k) => k.toLowerCase())
69+
70+
// Only 'referer' should be stored (if needed for origin validation)
71+
// Any other headers being stored is a security issue
72+
const sensitiveHeaders = ['x-forwarded-for', 'x-internal-api-key', 'x-real-ip', 'authorization', 'cookie']
73+
const exposedSensitiveHeaders = sensitiveHeaders.filter((h) => storedHeaderKeys.includes(h))
74+
75+
assert.deepEqual(
76+
exposedSensitiveHeaders,
77+
[],
78+
`Sensitive headers should not be stored in session cookie, but found: ${exposedSensitiveHeaders.join(', ')}`
79+
)
80+
})
81+
})
82+
})
83+
684
describe('@feathersjs/authentication-oauth service', () => {
785
const port = 9778
886
const req = axios.create({

‎packages/authentication-oauth/test/strategy.test.ts‎

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,151 @@ import { strict as assert } from 'assert'
22
import { expressFixture, TestOAuthStrategy } from './utils/fixture'
33
import { AuthenticationService } from '@feathersjs/authentication'
44

5+
describe('@feathersjs/authentication-oauth/strategy security', () => {
6+
let app: Awaited<ReturnType<typeof expressFixture>>
7+
let authService: AuthenticationService
8+
let strategy: TestOAuthStrategy
9+
10+
before(async () => {
11+
app = await expressFixture(9779, 5116)
12+
authService = app.service('authentication')
13+
strategy = authService.getStrategy('github') as TestOAuthStrategy
14+
})
15+
16+
after(async () => {
17+
await app.teardown()
18+
})
19+
20+
describe('open redirect via URL authority injection', () => {
21+
beforeEach(() => {
22+
app.get('authentication').oauth.origins = ['https://target.com']
23+
})
24+
25+
afterEach(() => {
26+
delete app.get('authentication').oauth.origins
27+
})
28+
29+
it('should reject redirect parameter containing @ character', async () => {
30+
// Attack: ?redirect=@attacker.com would result in https://target.com@attacker.com
31+
// which browsers parse as username "target.com" and host "attacker.com"
32+
await assert.rejects(
33+
() =>
34+
strategy.getRedirect(
35+
{ accessToken: 'testing' },
36+
{
37+
redirect: '@attacker.com',
38+
headers: {
39+
referer: 'https://target.com/login'
40+
}
41+
}
42+
),
43+
{
44+
name: 'NotAuthenticated'
45+
}
46+
)
47+
})
48+
49+
it('should reject redirect parameter containing // for protocol-relative URLs', async () => {
50+
// Attack: ?redirect=//attacker.com would result in https://target.com//attacker.com
51+
// which some parsers might interpret as protocol-relative URL
52+
await assert.rejects(
53+
() =>
54+
strategy.getRedirect(
55+
{ accessToken: 'testing' },
56+
{
57+
redirect: '//attacker.com',
58+
headers: {
59+
referer: 'https://target.com/login'
60+
}
61+
}
62+
),
63+
{
64+
name: 'NotAuthenticated'
65+
}
66+
)
67+
})
68+
69+
it('should reject redirect with backslash characters', async () => {
70+
// Some browsers treat backslash as forward slash
71+
await assert.rejects(
72+
() =>
73+
strategy.getRedirect(
74+
{ accessToken: 'testing' },
75+
{
76+
redirect: '\\\\attacker.com',
77+
headers: {
78+
referer: 'https://target.com/login'
79+
}
80+
}
81+
),
82+
{
83+
name: 'NotAuthenticated'
84+
}
85+
)
86+
})
87+
})
88+
89+
describe('origin validation bypass via startsWith', () => {
90+
beforeEach(() => {
91+
app.get('authentication').oauth.origins = ['https://target.com']
92+
})
93+
94+
afterEach(() => {
95+
delete app.get('authentication').oauth.origins
96+
})
97+
98+
it('should reject referer from domain that shares prefix with allowed origin', async () => {
99+
// Attack: attacker registers target.com.attacker.com
100+
// startsWith('https://target.com') would incorrectly return true
101+
await assert.rejects(
102+
() =>
103+
strategy.getRedirect(
104+
{ accessToken: 'testing' },
105+
{
106+
headers: {
107+
referer: 'https://target.com.attacker.com/login'
108+
}
109+
}
110+
),
111+
{
112+
message: 'Referer "https://target.com.attacker.com/login" is not allowed.'
113+
}
114+
)
115+
})
116+
117+
it('should reject referer with extra subdomain-like prefix', async () => {
118+
// Another variant: target.com-evil.attacker.com
119+
await assert.rejects(
120+
() =>
121+
strategy.getRedirect(
122+
{ accessToken: 'testing' },
123+
{
124+
headers: {
125+
referer: 'https://target.com-evil.attacker.com/login'
126+
}
127+
}
128+
),
129+
{
130+
message: 'Referer "https://target.com-evil.attacker.com/login" is not allowed.'
131+
}
132+
)
133+
})
134+
135+
it('should accept exact origin match with path', async () => {
136+
// Legitimate use case should still work
137+
const redirect = await strategy.getRedirect(
138+
{ accessToken: 'testing' },
139+
{
140+
headers: {
141+
referer: 'https://target.com/some/path'
142+
}
143+
}
144+
)
145+
assert.equal(redirect, 'https://target.com#access_token=testing')
146+
})
147+
})
148+
})
149+
5150
describe('@feathersjs/authentication-oauth/strategy', () => {
6151
let app: Awaited<ReturnType<typeof expressFixture>>
7152
let authService: AuthenticationService

0 commit comments

Comments
 (0)