Skip to content

Commit 8ec1cfa

Browse files
simonchrzsimonchrz
committed
fix: address Copilot review feedback
- Use array_key_exists() instead of isset() in LenientOidcDiscoveryMetadataPolicy to reject explicit null - Use JSON_THROW_ON_ERROR in handleRegistration() and reject JSON arrays (non-objects) - Preserve original response headers in enrichAuthServerMetadata() instead of rebuilding response - Add Cache-Control: no-store to registration responses containing client_secret
1 parent 7a31879 commit 8ec1cfa

4 files changed

Lines changed: 72 additions & 14 deletions

File tree

‎src/Server/Transport/Http/Middleware/ClientRegistrationMiddleware.php‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -70,15 +70,23 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
7070
private function handleRegistration(ServerRequestInterface $request): ResponseInterface
7171
{
7272
$body = $request->getBody()->__toString();
73-
$data = json_decode($body, true);
7473

75-
if (!\is_array($data)) {
74+
try {
75+
$data = json_decode($body, true, 512, \JSON_THROW_ON_ERROR);
76+
} catch (\JsonException) {
7677
return $this->jsonResponse(400, [
7778
'error' => 'invalid_client_metadata',
7879
'error_description' => 'Request body must be valid JSON.',
7980
]);
8081
}
8182

83+
if (!\is_array($data) || ([] !== $data && array_is_list($data))) {
84+
return $this->jsonResponse(400, [
85+
'error' => 'invalid_client_metadata',
86+
'error_description' => 'Request body must be a JSON object.',
87+
]);
88+
}
89+
8290
try {
8391
$result = $this->registrar->register($data);
8492
} catch (ClientRegistrationException $e) {
@@ -88,7 +96,9 @@ private function handleRegistration(ServerRequestInterface $request): ResponseIn
8896
]);
8997
}
9098

91-
return $this->jsonResponse(201, $result);
99+
return $this->jsonResponse(201, $result, [
100+
'Cache-Control' => 'no-store',
101+
]);
92102
}
93103

94104
private function enrichAuthServerMetadata(ResponseInterface $response): ResponseInterface
@@ -111,9 +121,11 @@ private function enrichAuthServerMetadata(ResponseInterface $response): Response
111121

112122
$metadata['registration_endpoint'] = rtrim($this->localBaseUrl, '/').self::REGISTRATION_PATH;
113123

114-
return $this->jsonResponse(200, $metadata, [
115-
'Cache-Control' => $response->getHeaderLine('Cache-Control'),
116-
]);
124+
return $response
125+
->withBody($this->streamFactory->createStream(
126+
json_encode($metadata, \JSON_THROW_ON_ERROR | \JSON_UNESCAPED_SLASHES),
127+
))
128+
->withHeader('Content-Type', 'application/json');
117129
}
118130

119131
/**

‎src/Server/Transport/Http/OAuth/LenientOidcDiscoveryMetadataPolicy.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ public function isValid(mixed $metadata): bool
3737
return false;
3838
}
3939

40-
if (isset($metadata['code_challenge_methods_supported'])) {
40+
if (\array_key_exists('code_challenge_methods_supported', $metadata)) {
4141
if (!\is_array($metadata['code_challenge_methods_supported']) || [] === $metadata['code_challenge_methods_supported']) {
4242
return false;
4343
}

‎tests/Unit/Server/Transport/Http/Middleware/ClientRegistrationMiddlewareTest.php‎

Lines changed: 39 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ public function testRegistrationSuccess(): void
5050

5151
$this->assertSame(201, $response->getStatusCode());
5252
$this->assertSame('application/json', $response->getHeaderLine('Content-Type'));
53+
$this->assertSame('no-store', $response->getHeaderLine('Cache-Control'));
5354

5455
$payload = json_decode($response->getBody()->__toString(), true, 512, \JSON_THROW_ON_ERROR);
5556
$this->assertSame('new-client', $payload['client_id']);
@@ -75,6 +76,25 @@ public function testRegistrationWithInvalidJson(): void
7576
$this->assertSame('Request body must be valid JSON.', $payload['error_description']);
7677
}
7778

79+
#[TestDox('POST /register with JSON array instead of object returns 400')]
80+
public function testRegistrationWithJsonArrayReturns400(): void
81+
{
82+
$registrar = $this->createStub(ClientRegistrarInterface::class);
83+
84+
$middleware = $this->createMiddleware($registrar);
85+
86+
$request = $this->factory->createServerRequest('POST', 'http://localhost:8000/register')
87+
->withBody($this->factory->createStream('["not","an","object"]'));
88+
89+
$response = $middleware->process($request, $this->createPassthroughHandler(404));
90+
91+
$this->assertSame(400, $response->getStatusCode());
92+
93+
$payload = json_decode($response->getBody()->__toString(), true, 512, \JSON_THROW_ON_ERROR);
94+
$this->assertSame('invalid_client_metadata', $payload['error']);
95+
$this->assertSame('Request body must be a JSON object.', $payload['error_description']);
96+
}
97+
7898
#[TestDox('POST /register returns 400 when registrar throws ClientRegistrationException')]
7999
public function testRegistrationWithRegistrarException(): void
80100
{
@@ -121,18 +141,23 @@ public function testMetadataEnrichment(): void
121141
$this->assertSame('http://localhost:8000/authorize', $payload['authorization_endpoint']);
122142
}
123143

124-
#[TestDox('GET /.well-known/oauth-authorization-server preserves Cache-Control header')]
125-
public function testMetadataEnrichmentPreservesCacheControl(): void
144+
#[TestDox('GET /.well-known/oauth-authorization-server preserves original response headers')]
145+
public function testMetadataEnrichmentPreservesOriginalHeaders(): void
126146
{
127147
$registrar = $this->createStub(ClientRegistrarInterface::class);
128148
$middleware = $this->createMiddleware($registrar);
129149

130150
$request = $this->factory->createServerRequest('GET', 'http://localhost:8000/.well-known/oauth-authorization-server');
131-
$handler = $this->createJsonHandler(200, ['issuer' => 'http://localhost:8000'], 'max-age=3600');
151+
$handler = $this->createJsonHandler(200, ['issuer' => 'http://localhost:8000'], 'max-age=3600', [
152+
'X-Custom' => 'preserved',
153+
'Vary' => 'Origin',
154+
]);
132155

133156
$response = $middleware->process($request, $handler);
134157

135158
$this->assertSame('max-age=3600', $response->getHeaderLine('Cache-Control'));
159+
$this->assertSame('preserved', $response->getHeaderLine('X-Custom'));
160+
$this->assertSame('Origin', $response->getHeaderLine('Vary'));
136161
}
137162

138163
#[TestDox('GET /.well-known/oauth-authorization-server with non-200 status passes through unchanged')]
@@ -226,21 +251,24 @@ public function handle(ServerRequestInterface $request): ResponseInterface
226251
}
227252

228253
/**
229-
* @param array<string, mixed> $data
254+
* @param array<string, mixed> $data
255+
* @param array<string, string> $extraHeaders
230256
*/
231-
private function createJsonHandler(int $status, array $data, string $cacheControl = ''): RequestHandlerInterface
257+
private function createJsonHandler(int $status, array $data, string $cacheControl = '', array $extraHeaders = []): RequestHandlerInterface
232258
{
233259
$factory = $this->factory;
234260

235-
return new class($factory, $status, $data, $cacheControl) implements RequestHandlerInterface {
261+
return new class($factory, $status, $data, $cacheControl, $extraHeaders) implements RequestHandlerInterface {
236262
/**
237-
* @param array<string, mixed> $data
263+
* @param array<string, mixed> $data
264+
* @param array<string, string> $extraHeaders
238265
*/
239266
public function __construct(
240267
private readonly ResponseFactoryInterface $factory,
241268
private readonly int $status,
242269
private readonly array $data,
243270
private readonly string $cacheControl,
271+
private readonly array $extraHeaders,
244272
) {
245273
}
246274

@@ -256,6 +284,10 @@ public function handle(ServerRequestInterface $request): ResponseInterface
256284
$response = $response->withHeader('Cache-Control', $this->cacheControl);
257285
}
258286

287+
foreach ($this->extraHeaders as $name => $value) {
288+
$response = $response->withHeader($name, $value);
289+
}
290+
259291
return $response;
260292
}
261293
};

‎tests/Unit/Server/Transport/Http/OAuth/LenientOidcDiscoveryMetadataPolicyTest.php‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,20 @@ public function testEmptyStringEndpointIsInvalid(): void
101101
]));
102102
}
103103

104+
#[TestDox('null code challenge methods is invalid')]
105+
public function testNullCodeChallengeMethodsIsInvalid(): void
106+
{
107+
$policy = new LenientOidcDiscoveryMetadataPolicy();
108+
$metadata = [
109+
'authorization_endpoint' => 'https://auth.example.com/authorize',
110+
'token_endpoint' => 'https://auth.example.com/token',
111+
'jwks_uri' => 'https://auth.example.com/jwks',
112+
'code_challenge_methods_supported' => null,
113+
];
114+
115+
$this->assertFalse($policy->isValid($metadata));
116+
}
117+
104118
#[TestDox('non-array input is invalid')]
105119
public function testNonArrayInputIsInvalid(): void
106120
{

0 commit comments

Comments
 (0)