Feathers: Open Redirect in OAuth callback enables account takeover

@@ -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, {})
   }

@@ -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.`)
@@ -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

@@ -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({

@@ -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