Skip to content

Commit d01b286

Browse files
PieterKasclaude
andcommitted
authorization-server/dpop: fix null alg-values hole; extract + test
The round-4 non-array guard carved out `null` (advertised !== null), so metadata with "dpop_signing_alg_values_supported": null passed the support gate (which only tests === undefined), skipped the guard, and fell through to the ES256 fallback — the exact binding mis-score the fix targeted. Extract the negotiation to an exported pure function negotiateProofAlg(advertised) and treat ANY present-but-non-array shape (string, null, number, object) as malformed → null (SKIP). Only an empty array still falls back to ES256. Correct the docstring (an absent field never reaches here — the support gate SKIPs upstream). Add unit tests for every shape (array / empty / no-overlap / string / null / number / object), pinning the fix against a silent refactor regression. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 83760fb commit d01b286

2 files changed

Lines changed: 58 additions & 28 deletions

File tree

‎src/scenarios/authorization-server/dpop.test.ts‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import {
66
import { ServerLifecycle } from '../client/auth/helpers/serverLifecycle';
77
import { testScenarioContext } from '../../mock-server/testing';
88
import type { CheckStatus, ConformanceCheck } from '../../types';
9-
import { DPoPAuthorizationServerScenario } from './dpop';
9+
import { DPoPAuthorizationServerScenario, negotiateProofAlg } from './dpop';
1010

1111
const ALL_IDS = [
1212
'sep-1932-as-metadata-alg-values',
@@ -142,3 +142,27 @@ describe('DPoPAuthorizationServerScenario — skip conditions', () => {
142142
expect(checks.filter((c) => c.status === 'FAILURE')).toHaveLength(0);
143143
});
144144
});
145+
146+
describe('negotiateProofAlg (dpop_signing_alg_values_supported shapes)', () => {
147+
it('picks the first supported alg from a non-empty array', () => {
148+
expect(negotiateProofAlg(['ES256'])).toBe('ES256');
149+
expect(negotiateProofAlg(['RS256', 'ES256'])).toBe('RS256');
150+
});
151+
152+
it('returns null for a non-empty array with no supported alg (→ SKIP)', () => {
153+
expect(negotiateProofAlg(['ES256K'])).toBeNull();
154+
});
155+
156+
it('falls back to ES256 only for an empty array', () => {
157+
expect(negotiateProofAlg([])).toBe('ES256');
158+
});
159+
160+
it('returns null for a present-but-non-array (malformed) value (→ SKIP)', () => {
161+
// Regression guard: a string or JSON null must NOT fall through to the
162+
// ES256 fallback, which would mis-score token binding.
163+
expect(negotiateProofAlg('RS256')).toBeNull();
164+
expect(negotiateProofAlg(null)).toBeNull();
165+
expect(negotiateProofAlg(42)).toBeNull();
166+
expect(negotiateProofAlg({ 0: 'ES256' })).toBeNull();
167+
});
168+
});

‎src/scenarios/authorization-server/dpop.ts‎

Lines changed: 33 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,37 @@ const SUPPORTED_PROOF_ALGS = [
9494
'EdDSA'
9595
];
9696

97+
/**
98+
* Pick a proof-signing algorithm the harness can produce that the AS also
99+
* advertises (RFC 9449 §5.1), given `dpop_signing_alg_values_supported`.
100+
*
101+
* Returns null (→ the caller SKIPs the binding check) when the value is a
102+
* non-empty array with no algorithm we support, OR any present-but-non-array
103+
* shape — a string, `null`, number, or object are all malformed metadata, not
104+
* "unspecified", so we must not fall back to ES256 (which the AS would reject,
105+
* mis-scoring binding). Only an EMPTY array falls back to ES256 as a best-effort
106+
* to still exercise the binding (an empty list is itself flagged by the metadata
107+
* check). An absent field never reaches here — the scenario's support gate SKIPs
108+
* the whole scenario upstream — but is treated as the empty case for safety.
109+
*/
110+
export function negotiateProofAlg(
111+
advertised: unknown,
112+
supported: readonly string[] = SUPPORTED_PROOF_ALGS
113+
): string | null {
114+
// Any present-but-non-array value (string / null / number / object) is
115+
// malformed — SKIP rather than fall back to ES256.
116+
if (advertised !== undefined && !Array.isArray(advertised)) {
117+
return null;
118+
}
119+
if (Array.isArray(advertised) && advertised.length > 0) {
120+
const match = advertised.find(
121+
(a) => typeof a === 'string' && supported.includes(a)
122+
);
123+
return typeof match === 'string' ? match : null;
124+
}
125+
return 'ES256';
126+
}
127+
97128
/** Strip query + fragment from a URL for use as an `htu` (RFC 9449 §4.2). */
98129
function stripUrlQuery(url: string): string {
99130
try {
@@ -393,34 +424,9 @@ browser login + callback for login-gated servers.`;
393424
}
394425
}
395426

396-
/**
397-
* Pick a proof-signing algorithm the harness can produce that the AS also
398-
* advertises (RFC 9449 §5.1). Returns null when the AS advertises a non-empty
399-
* list with no algorithm we support, OR a present-but-non-array (malformed)
400-
* value — the caller then SKIPs rather than sending an unadvertised alg (e.g.
401-
* defaulting to ES256) that the AS would legitimately reject and we'd mis-score
402-
* as a binding failure. Only an absent or empty list (itself flagged by the
403-
* metadata check) falls back to ES256 as a best effort to still exercise the
404-
* binding.
405-
*/
427+
/** See the module-level {@link negotiateProofAlg}. */
406428
private negotiateProofAlg(metadata: Record<string, any>): string | null {
407-
const advertised = metadata.dpop_signing_alg_values_supported;
408-
// A present-but-non-array value (e.g. the string "RS256") is malformed
409-
// metadata, not "unspecified" — SKIP rather than fall back to ES256.
410-
if (
411-
advertised !== undefined &&
412-
advertised !== null &&
413-
!Array.isArray(advertised)
414-
) {
415-
return null;
416-
}
417-
if (Array.isArray(advertised) && advertised.length > 0) {
418-
const match = advertised.find(
419-
(a) => typeof a === 'string' && SUPPORTED_PROOF_ALGS.includes(a)
420-
);
421-
return typeof match === 'string' ? match : null;
422-
}
423-
return 'ES256';
429+
return negotiateProofAlg(metadata.dpop_signing_alg_values_supported);
424430
}
425431

426432
/**

0 commit comments

Comments
 (0)