Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion packages/authentication-oauth/src/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,10 @@ export class OAuthService {

session.redirect = redirect
session.query = restQuery
session.headers = headers
// Only store the referer header needed for origin validation
session.headers = {
referer: headers?.referer
}

return this.handler('GET', handlerParams, {})
}
Expand Down
19 changes: 18 additions & 1 deletion packages/authentication-oauth/src/strategy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,17 @@ export class OAuthStrategy extends AuthenticationBaseStrategy {

if (Array.isArray(origins)) {
const referer = params?.headers?.referer || origins[0]
const allowedOrigin = origins.find((current) => referer.toLowerCase().startsWith(current.toLowerCase()))

// Parse the referer to get its origin for proper comparison
let refererOrigin: string
try {
refererOrigin = new URL(referer).origin
} catch {
throw new NotAuthenticated(`Invalid referer "${referer}".`)
}

// Compare full origins
const allowedOrigin = origins.find((current) => refererOrigin.toLowerCase() === current.toLowerCase())

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

// Validate redirect parameter to prevent open redirect via URL authority injection
// Reject characters that could change the URL's authority: @, //, \
// e.g., @attacker.com would make https://target.com@attacker.com redirect to attacker.com
if (queryRedirect && /[@\\]|^\/\/|\/\//.test(queryRedirect)) {
throw new NotAuthenticated('Invalid redirect path.')
}

const redirectUrl = `${redirect}${queryRedirect}`
const separator = redirectUrl.endsWith('?') ? '' : redirect.indexOf('#') !== -1 ? '?' : '#'
const authResult: AuthenticationResult = data
Expand Down
78 changes: 78 additions & 0 deletions packages/authentication-oauth/test/service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,84 @@ import axios, { AxiosResponse } from 'axios'
import { CookieJar } from 'tough-cookie'
import { expressFixture } from './utils/fixture'

describe('@feathersjs/authentication-oauth service security', () => {
const port = 9780
const req = axios.create({
withCredentials: true,
maxRedirects: 0
})
let app: Awaited<ReturnType<typeof expressFixture>>

const fetchErrorResponse = async (url: string, headers?: Record<string, string>): Promise<AxiosResponse> => {
try {
await req.get(url, { headers })
} catch (error: any) {
return error.response
}
assert.fail('Should never get here')
}

before(async () => {
app = await expressFixture(port, 5117)
})

after(async () => {
await app.teardown()
})

describe('internal headers exposure via session cookie', () => {
it('should not store sensitive internal headers in session cookie', async () => {
const host = `http://localhost:${port}`
const location = `${host}/oauth/github`

// Make request with internal/sensitive headers that might be added by proxies
const oauthResponse = await fetchErrorResponse(location, {
'x-forwarded-for': '10.0.0.1',
'x-internal-api-key': 'sk_live_secret123',
'x-real-ip': '192.168.1.1'
})

assert.equal(oauthResponse.status, 303)

// Get the session cookie
const cookies = oauthResponse.headers['set-cookie']
assert.ok(cookies, 'Should have set-cookie header')

// Find the oauth session cookie (express cookie-session uses 'feathers.oauth')
const oauthCookie = cookies.find((c: string) => c.startsWith('feathers.oauth='))
assert.ok(oauthCookie, 'Should have feathers.oauth session cookie')

// Extract the cookie value and decode it
const match = oauthCookie.match(/feathers\.oauth=([^;]+)/)
assert.ok(match, 'Should be able to extract cookie value')

const cookieValue = decodeURIComponent(match[1])
// Cookie session uses base64 encoding
const decoded = Buffer.from(cookieValue, 'base64').toString('utf-8')
const sessionData = JSON.parse(decoded)

// The vulnerability: all headers are stored in session.headers
// This test should FAIL if headers object contains sensitive internal headers
assert.ok(sessionData.headers, 'Session should have headers stored')

// These assertions verify the FIX is in place - they should FAIL currently
// because the vulnerable code stores ALL headers
const storedHeaderKeys = Object.keys(sessionData.headers).map((k) => k.toLowerCase())

// Only 'referer' should be stored (if needed for origin validation)
// Any other headers being stored is a security issue
const sensitiveHeaders = ['x-forwarded-for', 'x-internal-api-key', 'x-real-ip', 'authorization', 'cookie']
const exposedSensitiveHeaders = sensitiveHeaders.filter((h) => storedHeaderKeys.includes(h))

assert.deepEqual(
exposedSensitiveHeaders,
[],
`Sensitive headers should not be stored in session cookie, but found: ${exposedSensitiveHeaders.join(', ')}`
)
})
})
})

describe('@feathersjs/authentication-oauth service', () => {
const port = 9778
const req = axios.create({
Expand Down
145 changes: 145 additions & 0 deletions packages/authentication-oauth/test/strategy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,151 @@ import { strict as assert } from 'assert'
import { expressFixture, TestOAuthStrategy } from './utils/fixture'
import { AuthenticationService } from '@feathersjs/authentication'

describe('@feathersjs/authentication-oauth/strategy security', () => {
let app: Awaited<ReturnType<typeof expressFixture>>
let authService: AuthenticationService
let strategy: TestOAuthStrategy

before(async () => {
app = await expressFixture(9779, 5116)
authService = app.service('authentication')
strategy = authService.getStrategy('github') as TestOAuthStrategy
})

after(async () => {
await app.teardown()
})

describe('open redirect via URL authority injection', () => {
beforeEach(() => {
app.get('authentication').oauth.origins = ['https://target.com']
})

afterEach(() => {
delete app.get('authentication').oauth.origins
})

it('should reject redirect parameter containing @ character', async () => {
// Attack: ?redirect=@attacker.com would result in https://target.com@attacker.com
// which browsers parse as username "target.com" and host "attacker.com"
await assert.rejects(
() =>
strategy.getRedirect(
{ accessToken: 'testing' },
{
redirect: '@attacker.com',
headers: {
referer: 'https://target.com/login'
}
}
),
{
name: 'NotAuthenticated'
}
)
})

it('should reject redirect parameter containing // for protocol-relative URLs', async () => {
// Attack: ?redirect=//attacker.com would result in https://target.com//attacker.com
// which some parsers might interpret as protocol-relative URL
await assert.rejects(
() =>
strategy.getRedirect(
{ accessToken: 'testing' },
{
redirect: '//attacker.com',
headers: {
referer: 'https://target.com/login'
}
}
),
{
name: 'NotAuthenticated'
}
)
})

it('should reject redirect with backslash characters', async () => {
// Some browsers treat backslash as forward slash
await assert.rejects(
() =>
strategy.getRedirect(
{ accessToken: 'testing' },
{
redirect: '\\\\attacker.com',
headers: {
referer: 'https://target.com/login'
}
}
),
{
name: 'NotAuthenticated'
}
)
})
})

describe('origin validation bypass via startsWith', () => {
beforeEach(() => {
app.get('authentication').oauth.origins = ['https://target.com']
})

afterEach(() => {
delete app.get('authentication').oauth.origins
})

it('should reject referer from domain that shares prefix with allowed origin', async () => {
// Attack: attacker registers target.com.attacker.com
// startsWith('https://target.com') would incorrectly return true
await assert.rejects(
() =>
strategy.getRedirect(
{ accessToken: 'testing' },
{
headers: {
referer: 'https://target.com.attacker.com/login'
}
}
),
{
message: 'Referer "https://target.com.attacker.com/login" is not allowed.'
}
)
})

it('should reject referer with extra subdomain-like prefix', async () => {
// Another variant: target.com-evil.attacker.com
await assert.rejects(
() =>
strategy.getRedirect(
{ accessToken: 'testing' },
{
headers: {
referer: 'https://target.com-evil.attacker.com/login'
}
}
),
{
message: 'Referer "https://target.com-evil.attacker.com/login" is not allowed.'
}
)
})

it('should accept exact origin match with path', async () => {
// Legitimate use case should still work
const redirect = await strategy.getRedirect(
{ accessToken: 'testing' },
{
headers: {
referer: 'https://target.com/some/path'
}
}
)
assert.equal(redirect, 'https://target.com#access_token=testing')
})
})
})

describe('@feathersjs/authentication-oauth/strategy', () => {
let app: Awaited<ReturnType<typeof expressFixture>>
let authService: AuthenticationService
Expand Down