mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
fix(oidc): bind the SSO auth code to the browser that earned it
The callback hands the finished login back as a code in a redirect URL, and /exchange turned any code into a session. That made the code the whole credential while it travelled somewhere URLs are readable: browser history on a shared machine, a referrer, a reverse proxy log. It cuts both ways. Redeem a code you saw over someone's shoulder and you are them; or complete a login as yourself, hand the victim's browser your code, and they are silently signed into your account and write their next trip into it. The callback now also sets a one-minute httpOnly cookie holding a random secret, and /exchange wants the code plus that cookie. Only the browser that came back from the provider holds both, which is the same shape the state cookie already gives the callback. The code is burnt on every attempt, right or wrong, and an unbound code answers exactly like an unknown one so presenting one confirms nothing.
This commit is contained in:
@@ -19,7 +19,7 @@ function svc(o: Partial<OidcService> = {}): OidcService {
|
||||
findOrCreateUser: vi.fn(),
|
||||
touchLastLogin: vi.fn(),
|
||||
generateToken: vi.fn().mockReturnValue('jwt'),
|
||||
createAuthCode: vi.fn().mockReturnValue('ac'),
|
||||
createAuthCode: vi.fn().mockReturnValue({ code: 'ac', binding: 'bnd' }),
|
||||
consumeAuthCode: vi.fn(),
|
||||
frontendUrl: vi.fn((p: string) => 'https://app' + p),
|
||||
setAuthCookie: vi.fn(),
|
||||
@@ -42,6 +42,8 @@ function makeRes() {
|
||||
}
|
||||
|
||||
const req = { query: {}, headers: {} } as Request;
|
||||
// Exchange request carrying the binding cookie the callback handed this browser.
|
||||
const reqEx = (binding = 'bnd') => ({ query: {}, headers: {}, cookies: { trek_oidc_exchange: binding } } as unknown as Request);
|
||||
// Callback request carrying the state-binding cookie a real browser would send
|
||||
// after going through /login.
|
||||
const reqCb = (state = 's') => ({ query: {}, headers: {}, cookies: { trek_oidc_state: state } } as unknown as Request);
|
||||
@@ -192,6 +194,9 @@ describe('OidcController /callback', () => {
|
||||
}));
|
||||
await c.callback('c', 's', undefined, reqCb('s'), ok);
|
||||
expect(ok.redirectedTo).toBe('https://app/login?oidc_code=ac');
|
||||
// The code alone is not the credential: its other half goes out as a cookie
|
||||
// that only this browser holds, and expires with the code.
|
||||
expect(ok.cookie).toHaveBeenCalledWith('trek_oidc_exchange', 'bnd', expect.objectContaining({ httpOnly: true, maxAge: 60000, sameSite: 'lax' }));
|
||||
});
|
||||
|
||||
it('rejects a callback whose state cookie does not match the query state', async () => {
|
||||
@@ -360,7 +365,7 @@ describe('OidcController /callback', () => {
|
||||
[undefined, false],
|
||||
] as const) {
|
||||
const generateToken = vi.fn().mockReturnValue('jwt');
|
||||
const createAuthCode = vi.fn().mockReturnValue('ac');
|
||||
const createAuthCode = vi.fn().mockReturnValue({ code: 'ac', binding: 'bnd' });
|
||||
const res = makeRes();
|
||||
await new OidcController(svc({
|
||||
consumeState: vi.fn().mockReturnValue({ redirectUri: 'https://app/api/auth/oidc/callback', codeVerifier: 'cv', inviteToken: undefined, remember }),
|
||||
@@ -380,19 +385,20 @@ describe('OidcController /callback', () => {
|
||||
describe('OidcController /exchange', () => {
|
||||
it('400 without a code, 400 on an invalid code, else sets the cookie + returns the token', () => {
|
||||
const r1 = makeRes();
|
||||
new OidcController(svc()).exchange(undefined, req, r1);
|
||||
new OidcController(svc()).exchange(undefined, reqEx(), r1);
|
||||
expect(r1.statusCode).toBe(400);
|
||||
expect(r1.body).toEqual({ error: 'Code required' });
|
||||
|
||||
const r2 = makeRes();
|
||||
new OidcController(svc({ consumeAuthCode: vi.fn().mockReturnValue({ error: 'invalid_code' }) })).exchange('x', req, r2);
|
||||
new OidcController(svc({ consumeAuthCode: vi.fn().mockReturnValue({ error: 'invalid_code' }) })).exchange('x', reqEx(), r2);
|
||||
expect(r2.statusCode).toBe(400);
|
||||
expect(r2.body).toEqual({ error: 'invalid_code' });
|
||||
|
||||
const r3 = makeRes();
|
||||
const setAuthCookie = vi.fn();
|
||||
new OidcController(svc({ consumeAuthCode: vi.fn().mockReturnValue({ token: 'jwt' }), setAuthCookie })).exchange('x', req, r3);
|
||||
expect(setAuthCookie).toHaveBeenCalledWith(r3, 'jwt', req, undefined);
|
||||
const req3 = reqEx();
|
||||
new OidcController(svc({ consumeAuthCode: vi.fn().mockReturnValue({ token: 'jwt' }), setAuthCookie })).exchange('x', req3, r3);
|
||||
expect(setAuthCookie).toHaveBeenCalledWith(r3, 'jwt', req3, undefined);
|
||||
expect(r3.body).toEqual({ token: 'jwt' });
|
||||
});
|
||||
|
||||
@@ -400,9 +406,34 @@ describe('OidcController /exchange', () => {
|
||||
for (const remember of [true, false] as const) {
|
||||
const res = makeRes();
|
||||
const setAuthCookie = vi.fn();
|
||||
new OidcController(svc({ consumeAuthCode: vi.fn().mockReturnValue({ token: 'jwt', remember }), setAuthCookie })).exchange('x', req, res);
|
||||
expect(setAuthCookie).toHaveBeenCalledWith(res, 'jwt', req, remember);
|
||||
const r = reqEx();
|
||||
new OidcController(svc({ consumeAuthCode: vi.fn().mockReturnValue({ token: 'jwt', remember }), setAuthCookie })).exchange('x', r, res);
|
||||
expect(setAuthCookie).toHaveBeenCalledWith(res, 'jwt', r, remember);
|
||||
expect(res.body).toEqual({ token: 'jwt' });
|
||||
}
|
||||
});
|
||||
|
||||
it('hands the binding cookie to consumeAuthCode and clears it on the way out', () => {
|
||||
const consumeAuthCode = vi.fn().mockReturnValue({ token: 'jwt' });
|
||||
const res = makeRes();
|
||||
new OidcController(svc({ consumeAuthCode })).exchange('x', reqEx('the-secret'), res);
|
||||
expect(consumeAuthCode).toHaveBeenCalledWith('x', 'the-secret');
|
||||
expect(res.clearCookie).toHaveBeenCalledWith('trek_oidc_exchange', expect.objectContaining({ httpOnly: true, path: '/' }));
|
||||
});
|
||||
|
||||
it('passes undefined when the browser has no binding cookie, and still clears it', () => {
|
||||
const consumeAuthCode = vi.fn().mockReturnValue({ error: 'Invalid or expired code' });
|
||||
const res = makeRes();
|
||||
// A request from any other context: no cookie jar for this origin at all.
|
||||
new OidcController(svc({ consumeAuthCode })).exchange('stolen', { query: {}, headers: {} } as Request, res);
|
||||
expect(consumeAuthCode).toHaveBeenCalledWith('stolen', undefined);
|
||||
expect(res.statusCode).toBe(400);
|
||||
expect(res.clearCookie).toHaveBeenCalledWith('trek_oidc_exchange', expect.anything());
|
||||
});
|
||||
|
||||
it('clears the binding cookie even when no code was presented', () => {
|
||||
const res = makeRes();
|
||||
new OidcController(svc()).exchange(undefined, reqEx(), res);
|
||||
expect(res.clearCookie).toHaveBeenCalledWith('trek_oidc_exchange', expect.anything());
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user