Skip to content

Commit b9122fe

Browse files
committed
fix(oidc-client): address PAR code review — correctness, security, error handling
- Fix: spurious await on Micro value in client.store authorize path - Fix: validateParResponse uses toDispatchError for SerializedError, preserving diagnostic info instead of silently dropping it - Fix: optional chaining on wellknown?.require_pushed_authorization_requests in authorizeµ default param (prevents synchronous throw outside Micro) - Fix: scope PAR dispatch tapError inside flatMap to prevent double-logging with misleading "Error dispatching PAR authorize request" message - Fix: dispatchAuthorizeIframe validates code+state shape before succeeding - Fix: add credentials: include to PAR endpoint FetchArgs for SSO support - Fix: correct misleading log in authorizeIframe error path - Feat: surface expires_in from PAR response; warn when TTL < 30s - Test: add PAR 400 error e2e scenario to mock server and par.spec.ts - Chore: remove console.log debug artifacts from client.store.test.ts
1 parent df031cb commit b9122fe

10 files changed

Lines changed: 139 additions & 38 deletions

File tree

e2e/am-mock-api/src/app/routes.auth.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -666,6 +666,12 @@ export default function (app) {
666666
app.get('/callback', (req, res) => res.status(200).send('ok'));
667667

668668
app.post(authPaths.par, (req, res) => {
669+
if (req.query.scenario === 'error') {
670+
return res.status(400).json({
671+
error: 'invalid_request',
672+
error_description: 'Missing required PAR parameter',
673+
});
674+
}
669675
res.status(201).json(parResponse);
670676
});
671677

e2e/oidc-suites/src/par.spec.ts

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,54 @@ async function loginJourney(page, username: string, password: string) {
1717
await expect(page.locator('#journey-status')).toContainText('Session established');
1818
}
1919

20+
// Synthetic PAR error endpoint — intercepted by Playwright before any real network call
21+
const SYNTHETIC_PAR_ERROR_URL = 'http://localhost:8443/synthetic-par-error-endpoint';
22+
// The real wellknown used by the PAR app (intercepted to inject the synthetic PAR endpoint)
23+
const DEFAULT_WELLKNOWN_PATTERN =
24+
'**/openam-sdks.forgeblocks.com/am/oauth2/alpha/.well-known/openid-configuration*';
25+
2026
test.describe('PAR (Pushed Authorization Request) login tests', () => {
27+
test('PAR authorize returns 400 error — SDK surfaces error to the UI without redirecting', async ({
28+
page,
29+
}) => {
30+
const { navigate } = asyncEvents(page);
31+
32+
// Intercept the wellknown to inject our synthetic PAR endpoint URL
33+
await page.route(DEFAULT_WELLKNOWN_PATTERN, async (route) => {
34+
const response = await route.fetch();
35+
const json = await response.json();
36+
await route.fulfill({
37+
status: 200,
38+
contentType: 'application/json',
39+
body: JSON.stringify({
40+
...json,
41+
pushed_authorization_request_endpoint: SYNTHETIC_PAR_ERROR_URL,
42+
}),
43+
});
44+
});
45+
46+
// Intercept the synthetic PAR endpoint and return a 400 error
47+
await page.route(SYNTHETIC_PAR_ERROR_URL, (route) => {
48+
route.fulfill({
49+
status: 400,
50+
contentType: 'application/json',
51+
body: JSON.stringify({
52+
error: 'invalid_request',
53+
error_description: 'Missing required PAR parameter',
54+
}),
55+
});
56+
});
57+
58+
await navigate('/par/');
59+
60+
// Clicking redirect login triggers PAR → receives 400 → SDK should surface an error
61+
await page.getByRole('button', { name: 'Login (Redirect' }).click();
62+
63+
// The SDK should surface an error in the UI instead of redirecting away
64+
await expect(page.locator('.error')).toBeVisible({ timeout: 10000 });
65+
await expect(page.locator('.error')).toContainText('PAR_ERROR');
66+
});
67+
2168
test('background login with PAR enabled (ParClient) obtains access token', async ({ page }) => {
2269
const { navigate } = asyncEvents(page);
2370

packages/oidc-client/src/lib/authorize.request.micros.test.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -408,3 +408,24 @@ it.effect('dispatchAuthorizeIframe fails with unknown_error when data is undefin
408408
expect(exit.cause.error.type).toBe('unknown_error');
409409
}),
410410
);
411+
412+
it.effect('dispatchAuthorizeIframe fails with unknown_error when data has no code or state', () =>
413+
Micro.gen(function* () {
414+
vi.mocked(mockStore.dispatch).mockResolvedValueOnce({
415+
data: { unexpected: 'shape' },
416+
} as unknown as ReturnType<typeof mockStore.dispatch>);
417+
418+
const exit = yield* Micro.exit(
419+
dispatchAuthorizeIframe(
420+
mockStore,
421+
'https://example.com/authorize?foo=bar',
422+
wellknown,
423+
{} as import('@forgerock/sdk-types').GetAuthorizationUrlOptions,
424+
),
425+
);
426+
expect(Micro.exitIsFailure(exit)).toBe(true);
427+
if (!Micro.exitIsFailure(exit) || !Micro.causeIsFail(exit.cause)) return;
428+
expect(exit.cause.error.type).toBe('unknown_error');
429+
expect(exit.cause.error.error).toBe('Unknown_Error');
430+
}),
431+
);

packages/oidc-client/src/lib/authorize.request.micros.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,10 +100,13 @@ export const dispatchAuthorizeIframe = (
100100
}).pipe(
101101
Micro.flatMap(({ error, data }) => {
102102
if (error) return handleDispatchError(error, wellknown, options);
103-
if (data !== undefined) return Micro.succeed(data);
103+
const d = data as { code?: unknown; state?: unknown } | undefined;
104+
if (d !== undefined && typeof d.code === 'string' && typeof d.state === 'string') {
105+
return Micro.succeed(d as AuthorizationSuccess);
106+
}
104107
return Micro.fail({
105108
error: 'Unknown_Error',
106-
error_description: 'Response data was empty',
109+
error_description: 'Response data did not contain expected code and state fields',
107110
type: 'unknown_error',
108111
} as const);
109112
}),

packages/oidc-client/src/lib/authorize.request.ts

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ function dispatchAuthorizeµ(
7070
export function parAuthorizeµ(
7171
wellknown: WellknownResponse,
7272
config: OidcConfig,
73+
log: CustomLogger,
7374
store: ClientStore,
7475
options?: OptionalAuthorizeOptions,
7576
): Micro.Micro<string, AuthorizationError, never> {
@@ -96,7 +97,14 @@ export function parAuthorizeµ(
9697
prompt,
9798
);
9899
const parResult = yield* dispatchParRequest(store, parEndpoint, body);
99-
const { request_uri } = yield* validateParResponse(parResult);
100+
const { request_uri, expires_in } = yield* validateParResponse(parResult);
101+
if (expires_in < 30) {
102+
yield* Micro.sync(() =>
103+
log.warn(
104+
`PAR request_uri expires in ${expires_in}s — authorize must complete before expiry`,
105+
),
106+
);
107+
}
100108
yield* storeAuthOptions(storeOptions);
101109
return yield* buildParSlimUrl(
102110
wellknown.authorization_endpoint,
@@ -120,7 +128,7 @@ export function authorizeµ(
120128
log: CustomLogger,
121129
store: ClientStore,
122130
options?: GetAuthorizationUrlOptions,
123-
useParFlow = config.par ?? wellknown.require_pushed_authorization_requests === true,
131+
useParFlow = config.par ?? wellknown?.require_pushed_authorization_requests === true,
124132
): Micro.Micro<AuthorizationSuccess, AuthorizationError, never> {
125133
const parDispatchOptions: GetAuthorizationUrlOptions = {
126134
clientId: config.clientId,
@@ -130,14 +138,17 @@ export function authorizeµ(
130138
...options,
131139
};
132140

133-
const parFlow = parAuthorizeµ(wellknown, config, store, options).pipe(
141+
const parFlow = parAuthorizeµ(wellknown, config, log, store, options).pipe(
134142
Micro.tap((url) => log.debug('PAR authorize URL created', url)),
135143
Micro.tapError((err) =>
136144
Micro.sync(() => log.error(`PAR authorize failed [${err.type}]: ${err.error}`, err)),
137145
),
138-
Micro.flatMap((url) => dispatchAuthorizeµ(url, parDispatchOptions, wellknown, store, log)),
139-
Micro.tapError((err) =>
140-
Micro.sync(() => log.error('Error dispatching PAR authorize request', err)),
146+
Micro.flatMap((url) =>
147+
dispatchAuthorizeµ(url, parDispatchOptions, wellknown, store, log).pipe(
148+
Micro.tapError((err) =>
149+
Micro.sync(() => log.error('Error dispatching PAR authorize request', err)),
150+
),
151+
),
141152
),
142153
);
143154

packages/oidc-client/src/lib/authorize.request.utils.test.ts

Lines changed: 31 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,13 @@ const mockStore = {
5555
dispatch: vi.fn(),
5656
} as unknown as ClientStore;
5757

58+
const mockLog = {
59+
debug: vi.fn(),
60+
error: vi.fn(),
61+
info: vi.fn(),
62+
warn: vi.fn(),
63+
} as unknown as import('@forgerock/sdk-logger').CustomLogger;
64+
5865
const sessionStorageStub = { getItem: vi.fn(), setItem: vi.fn(), removeItem: vi.fn() };
5966

6067
afterEach(() => {
@@ -126,7 +133,7 @@ it('toDispatchError delegates to toAuthorizationError for FetchBaseQueryError',
126133
it.effect('parAuthorizeµ fails with wellknown_error when par endpoint is missing', () =>
127134
Micro.gen(function* () {
128135
const configWithPar: OidcConfig = { ...config, par: true };
129-
const result = yield* Micro.exit(parAuthorizeµ(wellknown, configWithPar, mockStore));
136+
const result = yield* Micro.exit(parAuthorizeµ(wellknown, configWithPar, mockLog, mockStore));
130137
expect(Micro.exitIsFailure(result)).toBe(true);
131138
if (!Micro.exitIsFailure(result)) return;
132139
expect(Micro.causeIsFail(result.cause)).toBe(true);
@@ -149,7 +156,7 @@ it.effect('parAuthorizeµ succeeds and returns slim authorize URL', () =>
149156
data: { request_uri: requestUri, expires_in: 60 },
150157
} as unknown as ReturnType<typeof mockStore.dispatch>);
151158

152-
const url = yield* parAuthorizeµ(wellknownWithPar, configWithPar, mockStore);
159+
const url = yield* parAuthorizeµ(wellknownWithPar, configWithPar, mockLog, mockStore);
153160

154161
expect(url).toContain('client_id=123456789');
155162
expect(url).toContain(`request_uri=${encodeURIComponent(requestUri)}`);
@@ -172,7 +179,9 @@ it.effect('parAuthorizeµ fails with network_error when PAR POST returns error',
172179
},
173180
} as unknown as ReturnType<typeof mockStore.dispatch>);
174181

175-
const result = yield* Micro.exit(parAuthorizeµ(wellknownWithPar, configWithPar, mockStore));
182+
const result = yield* Micro.exit(
183+
parAuthorizeµ(wellknownWithPar, configWithPar, mockLog, mockStore),
184+
);
176185

177186
expect(Micro.exitIsFailure(result)).toBe(true);
178187
if (!Micro.exitIsFailure(result)) return;
@@ -193,7 +202,9 @@ it.effect('parAuthorizeµ fails with network_error when PAR response is missing
193202
data: {},
194203
} as unknown as ReturnType<typeof mockStore.dispatch>);
195204

196-
const result = yield* Micro.exit(parAuthorizeµ(wellknownWithPar, configWithPar, mockStore));
205+
const result = yield* Micro.exit(
206+
parAuthorizeµ(wellknownWithPar, configWithPar, mockLog, mockStore),
207+
);
197208

198209
expect(Micro.exitIsFailure(result)).toBe(true);
199210
if (!Micro.exitIsFailure(result)) return;
@@ -223,7 +234,7 @@ it.effect(
223234
data: { request_uri: requestUri, expires_in: 60 },
224235
} as unknown as ReturnType<typeof mockStore.dispatch>);
225236

226-
const url = yield* parAuthorizeµ(wellknownWithPar, configWithPar, mockStore, {
237+
const url = yield* parAuthorizeµ(wellknownWithPar, configWithPar, mockLog, mockStore, {
227238
prompt: 'none',
228239
});
229240

@@ -261,6 +272,21 @@ it('hasPushRequestUri returns false when request_uri is missing', () => {
261272
expect(hasPushRequestUri('string')).toBe(false);
262273
});
263274

275+
// ─── validateParResponse ─────────────────────────────────────────────────────
276+
277+
import { validateParResponse } from './authorize.request.utils.js';
278+
279+
it.effect('validateParResponse with SerializedError preserves error message', () =>
280+
Micro.gen(function* () {
281+
const serializedError = { name: 'Error', message: 'network timeout', code: 'FETCH_ERROR' };
282+
const exit = yield* Micro.exit(validateParResponse({ error: serializedError }));
283+
expect(Micro.exitIsFailure(exit)).toBe(true);
284+
if (!Micro.exitIsFailure(exit) || !Micro.causeIsFail(exit.cause)) return;
285+
// Should surface the actual message, not generic "Unknown_Error"
286+
expect(exit.cause.error.error_description).toContain('network timeout');
287+
}),
288+
);
289+
264290
// ─── buildParAuthorizeUrl ────────────────────────────────────────────────────
265291

266292
it('buildParAuthorizeUrl produces slim URL with client_id and request_uri', () => {
@@ -340,13 +366,6 @@ it('buildAuthorizeOptions falls back to "openid" scope and "code" responseType w
340366

341367
// ─── authorizeµ flow routing ──────────────────────────────────────────────────
342368

343-
const mockLog = {
344-
debug: vi.fn(),
345-
error: vi.fn(),
346-
info: vi.fn(),
347-
warn: vi.fn(),
348-
} as unknown as import('@forgerock/sdk-logger').CustomLogger;
349-
350369
it.effect('authorizeµ uses PAR flow when useParFlow=true', () =>
351370
Micro.gen(function* () {
352371
const requestUri = 'urn:ietf:params:oauth:request_uri:par-routing-test';

packages/oidc-client/src/lib/authorize.request.utils.ts

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -200,11 +200,9 @@ export const buildAuthorizeRedirectUrl = (
200200
export const validateParResponse = (result: {
201201
error?: FetchBaseQueryError | SerializedError;
202202
data?: unknown;
203-
}): Micro.Micro<{ request_uri: string }, AuthorizationError, never> => {
203+
}): Micro.Micro<{ request_uri: string; expires_in: number }, AuthorizationError, never> => {
204204
if (result.error) {
205-
return Micro.fail(
206-
toAuthorizationError(isFetchBaseQueryError(result.error) ? result.error.data : undefined),
207-
);
205+
return Micro.fail(toDispatchError(result.error));
208206
}
209207
if (!hasPushRequestUri(result.data)) {
210208
return Micro.fail({
@@ -213,7 +211,8 @@ export const validateParResponse = (result: {
213211
type: 'network_error',
214212
} as const);
215213
}
216-
return Micro.succeed(result.data);
214+
const d = result.data as { request_uri: string; expires_in?: number };
215+
return Micro.succeed({ request_uri: d.request_uri, expires_in: d.expires_in ?? 60 });
217216
};
218217

219218
export const handleDispatchError = (

packages/oidc-client/src/lib/client.store.test.ts

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ vi.stubGlobal(
2222

2323
return {
2424
getItem(key: string) {
25-
console.log('getItem', key);
2625
return store[key] || null;
2726
},
2827
setItem(key: string) {
@@ -43,15 +42,14 @@ const parRequestUri = 'urn:ietf:params:oauth:request_uri:test-par-request-uri';
4342

4443
const server = setupServer(
4544
// P1 Revoke
46-
http.post('*/as/authorize', async () => {
47-
console.log('authorize request received');
48-
return HttpResponse.json({
45+
http.post('*/as/authorize', async () =>
46+
HttpResponse.json({
4947
authorizeResponse: {
5048
code: 123,
5149
state: 'NzUyNDUyMDAxOTMyNDUxNzI1NjkxNDc2MjEyMzUwMjQzMzQyMjE4OQ',
5250
},
53-
});
54-
}),
51+
}),
52+
),
5553
http.post('*/as/token', async () =>
5654
HttpResponse.json({
5755
access_token: 'abcdefghijklmnop',
@@ -204,7 +202,6 @@ describe('PingOne token get method', async () => {
204202
}
205203
const tokens = await oidcClient.token.get({ backgroundRenew: true });
206204
if ('error' in tokens) {
207-
console.log('tokens error', tokens);
208205
expect.fail();
209206
}
210207
expect(tokens.accessToken).toBe('1234567890');
@@ -229,7 +226,6 @@ describe('PingOne token get method', async () => {
229226
}
230227
const tokens = await oidcClient.token.get({ backgroundRenew: true });
231228
if ('error' in tokens) {
232-
console.log('tokens error', tokens);
233229
expect.fail();
234230
}
235231
expect(tokens.accessToken).toBe('abcdefghijklmnop');
@@ -254,7 +250,6 @@ describe('PingOne token get method', async () => {
254250
}
255251
const tokens = await oidcClient.token.get({ backgroundRenew: true });
256252
if ('error' in tokens) {
257-
console.log('tokens error', tokens);
258253
expect.fail();
259254
}
260255
expect(tokens.accessToken).toBe('abcdefghijklmnop');
@@ -279,7 +274,6 @@ describe('PingOne token get method', async () => {
279274
}
280275
const tokens = await oidcClient.token.get({ backgroundRenew: true, forceRenew: true });
281276
if ('error' in tokens) {
282-
console.log('tokens error', tokens);
283277
expect.fail();
284278
}
285279
expect(tokens.accessToken).toBe('abcdefghijklmnop');

packages/oidc-client/src/lib/client.store.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ export async function oidc<ActionType extends ActionTypes = ActionTypes>({
132132

133133
if (useParFlow) {
134134
const result = await Micro.runPromiseExit(
135-
parAuthorizeµ(wellknown, config, store, options).pipe(
135+
parAuthorizeµ(wellknown, config, log, store, options).pipe(
136136
Micro.tapError((err) =>
137137
Micro.sync(() =>
138138
log.error(`PAR authorize.url() failed [${err.type}]: ${err.error}`, err),
@@ -192,7 +192,7 @@ export async function oidc<ActionType extends ActionTypes = ActionTypes>({
192192
}
193193

194194
const result = await Micro.runPromiseExit(
195-
await authorizeµ(wellknown, config, log, store, options, useParFlow),
195+
authorizeµ(wellknown, config, log, store, options, useParFlow),
196196
);
197197

198198
if (exitIsSuccess(result)) {

packages/oidc-client/src/lib/oidc.api.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@ export const oidcApi = createApi({
114114
Accept: 'application/json',
115115
'Content-Type': 'application/x-www-form-urlencoded',
116116
},
117+
credentials: 'include',
117118
body,
118119
};
119120

@@ -210,7 +211,7 @@ export const oidcApi = createApi({
210211
});
211212

212213
if ('error' in response) {
213-
logger.error('Received authorization code', response);
214+
logger.error('Error in authorizeIframe response', response);
214215
return {
215216
error: {
216217
status: 400,

0 commit comments

Comments
 (0)