fix(oauth2): use exact matching for JS client origin validation#545
fix(oauth2): use exact matching for JS client origin validation#545mulldug wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughOAuth2 middleware origin validation is refactored from substring-based checking to exact-match membership testing with URL normalization. Allowed origins are parsed as space-delimited, normalized, and validated against request origins after trailing-slash trimming. Comprehensive unit tests verify acceptance of exact and normalized origins, rejection of bare hostnames and missing headers. ChangesOAuth2 Bearer Token Origin Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-545/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/ModelSerializers/Summit/SummitSerializer.php (1)
335-342:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLog the full exception object, not just the message.
Same issue as the registration profile handler above.
🐛 Proposed fix
} catch (\Exception $ex) { - Log::warning($ex->getMessage()); + Log::warning($ex); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/ModelSerializers/Summit/SummitSerializer.php` around lines 335 - 342, The catch block around $build_default_payment_gateway_profile_strategy->build in SummitSerializer:: (where you call SerializerRegistry::getInstance(), $serializer->serialize and AbstractSerializer::filterExpandByPrefix) currently logs only $ex->getMessage(); change it to log the full exception (message plus stack trace) so you capture context—e.g., pass the exception object or its full string/trace to Log::warning instead of only getMessage(); follow the same pattern you used in the registration profile handler to ensure consistency.
🧹 Nitpick comments (3)
tests/oauth2/OAuth2SummitSpeakersApiTest.php (3)
1903-1936: ⚡ Quick winConsider using
assertGreaterThanOrEqual(0)for count assertions.Line 1918 uses
assertGreaterThan(0)which assumes test data always contains speaker activities. For consistency with the submitters tests and improved robustness, consider usingassertGreaterThanOrEqual(0).♻️ Recommended adjustment
$this->assertResponseStatus(200); $unfilteredData = json_decode($unfilteredResponse->getContent()); - $this->assertGreaterThan(0, $unfilteredData->count); + $this->assertGreaterThanOrEqual(0, $unfilteredData->count);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/oauth2/OAuth2SummitSpeakersApiTest.php` around lines 1903 - 1936, In testGetCurrentSummitSpeakersActivitiesCountFilteredBySelPlan update the initial unfiltered count assertion to allow zero results by replacing the strict assertGreaterThan(0) with assertGreaterThanOrEqual(0) so the test no longer assumes there is at least one speaker activity; modify the assertion in this test method where $unfilteredData->count is checked to use assertGreaterThanOrEqual(0).
1874-1901: ⚡ Quick winConsider using
assertGreaterThanOrEqual(0)for count assertions.Line 1900 uses
assertGreaterThan(0)which requires the count to be strictly positive. This makes the test dependent on specific test data always having speaker activities. The equivalent submitters test (line 269 in OAuth2SummitSubmittersApiTest.php) usesassertGreaterThanOrEqual(0), which is more robust since a count of zero is a valid response for an activities count endpoint.♻️ Recommended adjustment for consistency and robustness
$this->assertNotNull($data); $this->assertTrue(isset($data->count)); - $this->assertGreaterThan(0, $data->count); + $this->assertGreaterThanOrEqual(0, $data->count);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/oauth2/OAuth2SummitSpeakersApiTest.php` around lines 1874 - 1901, The test method testGetCurrentSummitSpeakersActivitiesCount currently asserts a strictly positive count using $this->assertGreaterThan(0); change this to allow zero by using $this->assertGreaterThanOrEqual(0) in the same method so the speakers activities count can be zero without failing the test; update the assertion call in testGetCurrentSummitSpeakersActivitiesCount accordingly for consistency with the submitters test.
1938-1968: ⚡ Quick winConsider using
assertGreaterThanOrEqual(0)for count assertions.Line 1967 uses
assertGreaterThan(0), which is inconsistent with the submitters tests. A count of zero is a valid response when filtering yields no results, soassertGreaterThanOrEqual(0)would be more appropriate and consistent.♻️ Recommended adjustment
$this->assertNotNull($data); $this->assertTrue(isset($data->count)); - $this->assertGreaterThan(0, $data->count); + $this->assertGreaterThanOrEqual(0, $data->count);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/oauth2/OAuth2SummitSpeakersApiTest.php` around lines 1938 - 1968, In testGetCurrentSummitSpeakersActivitiesCountWithAcceptedPresentations replace the strict positive assertion with a non-negative one: change the assertion on $data->count (currently assertGreaterThan(0)) to assertGreaterThanOrEqual(0) so the test accepts zero results when the filter yields none; update the assertion call in that method accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php`:
- Around line 510-515: The controller currently casts the path parameter
$summit_id with intval, which converts any slug to 0 and breaks slug support;
update OAuth2SummitSubmittersApiController to stop unconditionally calling
intval($summit_id) — either pass $summit_id through unchanged to downstream
lookups or implement a conditional: if (is_numeric($summit_id)) cast to int and
use numeric lookup, otherwise treat it as a slug and call the slug-based lookup
routine; replace occurrences of intval($summit_id) (including the instances
around $summit_id at the top of the file and the later usage) so the endpoint
honors the documented “Summit ID or slug” contract.
In `@app/ModelSerializers/Summit/SummitSerializer.php`:
- Around line 323-330: The catch block currently logs only $ex->getMessage(),
losing stack and type; update the catch in the try around
$build_default_payment_gateway_profile_strategy->build(...) /
SerializerRegistry::getInstance()->getSerializer(...) so the full exception is
logged (for example pass the exception object to Log::warning or include it in
the context), e.g. replace Log::warning($ex->getMessage()) with a call that logs
the whole $ex so stack trace and exception type are preserved.
---
Duplicate comments:
In `@app/ModelSerializers/Summit/SummitSerializer.php`:
- Around line 335-342: The catch block around
$build_default_payment_gateway_profile_strategy->build in SummitSerializer::
(where you call SerializerRegistry::getInstance(), $serializer->serialize and
AbstractSerializer::filterExpandByPrefix) currently logs only $ex->getMessage();
change it to log the full exception (message plus stack trace) so you capture
context—e.g., pass the exception object or its full string/trace to Log::warning
instead of only getMessage(); follow the same pattern you used in the
registration profile handler to ensure consistency.
---
Nitpick comments:
In `@tests/oauth2/OAuth2SummitSpeakersApiTest.php`:
- Around line 1903-1936: In
testGetCurrentSummitSpeakersActivitiesCountFilteredBySelPlan update the initial
unfiltered count assertion to allow zero results by replacing the strict
assertGreaterThan(0) with assertGreaterThanOrEqual(0) so the test no longer
assumes there is at least one speaker activity; modify the assertion in this
test method where $unfilteredData->count is checked to use
assertGreaterThanOrEqual(0).
- Around line 1874-1901: The test method
testGetCurrentSummitSpeakersActivitiesCount currently asserts a strictly
positive count using $this->assertGreaterThan(0); change this to allow zero by
using $this->assertGreaterThanOrEqual(0) in the same method so the speakers
activities count can be zero without failing the test; update the assertion call
in testGetCurrentSummitSpeakersActivitiesCount accordingly for consistency with
the submitters test.
- Around line 1938-1968: In
testGetCurrentSummitSpeakersActivitiesCountWithAcceptedPresentations replace the
strict positive assertion with a non-negative one: change the assertion on
$data->count (currently assertGreaterThan(0)) to assertGreaterThanOrEqual(0) so
the test accepts zero results when the filter yields none; update the assertion
call in that method accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62fea433-b047-49d2-8cc2-668acf17a485
📒 Files selected for processing (17)
.env.example.gitignoreapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.phpapp/Http/Middleware/OAuth2BearerAccessTokenRequestValidator.phpapp/ModelSerializers/Summit/SummitSerializer.phpapp/Models/Foundation/Main/Repositories/IMemberRepository.phpapp/Models/Foundation/Summit/Repositories/ISpeakerRepository.phpapp/Repositories/Summit/DoctrineMemberRepository.phpapp/Repositories/Summit/DoctrineSpeakerRepository.phpdatabase/seeders/ApiEndpointsSeeder.phpdatabase/seeders/ConfigSeeder.phpdocker-compose.override.testing.ymlroutes/api_v1.phptests/SubmitterRepositoryTest.phptests/oauth2/OAuth2SummitSpeakersApiTest.phptests/oauth2/OAuth2SummitSubmittersApiTest.php
mulldug
force-pushed
the
fix/oauth2-exact-origin-matching
branch
from
68235b1 to
f305115
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-545/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Pull request overview
This PR tightens OAuth2 JS client origin validation by replacing substring matching with exact matching against normalized allowed origins.
Changes:
- Parses the space-delimited
allowed_originstoken field into individual entries. - Trims entries and removes trailing slashes before comparison.
- Uses strict
in_arrayexact matching to prevent substring-origin bypasses.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ( | ||
| $token_info->getApplicationType() === 'JS_CLIENT' | ||
| && (is_null($origin) || empty($origin)|| str_contains($token_info->getAllowedOrigins(), $origin) === false ) | ||
| && (is_null($origin) || empty($origin) || !in_array(rtrim($origin, '/'), $allowedOrigins, true)) |
There was a problem hiding this comment.
@mulldug
Asymmetric normalization — risk of false 403s for legitimate JS clients
$origin is built by RequestUtils::getOrigin(), which runs the header through URL\Normalizer (lowercases scheme/host, strips default ports :80/:443, canonicalizes percent-encoding, resolves dot segments, may add/strip trailing /).
$allowedOrigins here only gets trim() + rtrim('/') — no host-lowercasing, no port normalization, no percent-encoding canonicalization.
Combined with strict in_array(..., true) (byte-exact, case-sensitive), any IDP record stored as e.g. https://Example.com, https://example.com:443, or with percent-encoded chars will silently start returning 403 unauthorized_client after this ships — even though the old str_contains accidentally tolerated those mismatches.
Failure mode is silent and uniform across affected clients, and local tests won't catch it (fixtures use allowed_origins => '', so the JS_CLIENT branch is skipped).
Suggested fix: normalize each entry through the same URL\Normalizer before the strict compare, so both sides come out of the same pipeline:
$allowedOrigins = array_filter(array_map(function ($o) {
$o = trim($o);
if ($o === '') return '';
try { $o = (new \URL\Normalizer($o))->normalize(); } catch (\Throwable $e) {}
return rtrim($o, '/');
}, explode(' ', $token_info->getAllowedOrigins() ?? '')));Recommended pre-rollout check: query the token store for allowed_origins entries containing uppercase letters, :80/:443, or %-encoding to surface clients at risk.
|
@mulldug all proposed PR needs
|
| ); | ||
| throw new InvalidGrantTypeException(OAuth2Protocol::OAuth2Protocol_Error_InvalidToken); | ||
| } | ||
| $allowedOrigins = array_filter(array_map( |
There was a problem hiding this comment.
@mulldug
Move the array build inside the JS_CLIENT branch so server-to-server tokens do not pay the cost
Replace str_contains substring check with in_array exact match to prevent crafted short origins from satisfying the allowed-origins guard on JS_CLIENT tokens. Splits the space-delimited allowed_origins string from the introspection response into individual entries before comparing.
mulldug
force-pushed
the
fix/oauth2-exact-origin-matching
branch
from
f305115 to
1cc4102
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-545/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Unit/OAuth2BearerAccessTokenRequestValidatorTest.php (1)
87-87: ⚡ Quick winConsider adding a test for the second allowed origin.
The token stub includes
https://foo.barin the allowed origins string, but no test explicitly validates it. Adding a test case for this second origin would verify that space-delimited parsing correctly handles multiple origins.➕ Suggested additional test
Add after line 153:
public function test_second_allowed_origin_is_accepted(): void { $this->context->expects($this->once())->method('setAuthorizationContext'); $response = $this->buildValidator('https://foo.bar') ->handle($this->buildRequest('https://foo.bar'), $this->next()); $this->assertEquals(200, $response->getStatusCode()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/OAuth2BearerAccessTokenRequestValidatorTest.php` at line 87, Add a new unit test method named test_second_allowed_origin_is_accepted that mirrors the existing origin test but uses 'https://foo.bar' to verify space-delimited parsing in getAllowedOrigins; in the test call buildValidator('https://foo.bar')->handle(buildRequest('https://foo.bar'), $this->next()), expect $this->context->expects($this->once())->method('setAuthorizationContext') and assert the response status is 200 to confirm the second allowed origin is accepted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/Unit/OAuth2BearerAccessTokenRequestValidatorTest.php`:
- Line 87: Add a new unit test method named
test_second_allowed_origin_is_accepted that mirrors the existing origin test but
uses 'https://foo.bar' to verify space-delimited parsing in getAllowedOrigins;
in the test call
buildValidator('https://foo.bar')->handle(buildRequest('https://foo.bar'),
$this->next()), expect
$this->context->expects($this->once())->method('setAuthorizationContext') and
assert the response status is 200 to confirm the second allowed origin is
accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f51ea411-191b-4c11-8872-03ecbba4d46d
📒 Files selected for processing (3)
.gitignoreapp/Http/Middleware/OAuth2BearerAccessTokenRequestValidator.phptests/Unit/OAuth2BearerAccessTokenRequestValidatorTest.php
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Http/Middleware/OAuth2BearerAccessTokenRequestValidator.php
ref: https://app.clickup.com/t/86b9xky5x
Summary
str_containssubstring check within_arrayexact match inOAuth2BearerAccessTokenRequestValidatorfor JS_CLIENT origin validationallowed_originsstring from the IDP introspection response into individual entries, trims each, and strips trailing slashes before comparingexample) from satisfying the guard by matching as a substring of a legitimate allowed originNotes
This is a pre-existing bug, not introduced by recent work. Separated into its own PR to keep the security fix reviewable independently.
Test plan
examplewhenhttps://example.comis allowed) are rejected🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests