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
📝 WalkthroughWalkthroughThis PR adds two new protected API endpoints that return unique activity counts for speakers and submitters on a per-summit basis, with optional filter support. The implementation spans repository layer (data access), controller layer (HTTP endpoints), routing, and comprehensive test coverage. ChangesActivity Count Endpoints for Speakers and Submitters
Sequence DiagramsequenceDiagram
participant Client
participant Controller as Controller:<br/>getSpeakersActivitiesCount
participant Repository as Repository:<br/>getUniqueActivitiesCountBySummit
participant Database as Database
Client->>Controller: GET /summits/{id}/speakers/all/events/count<br/>?filter=...
Controller->>Controller: Load summit by ID
Controller->>Controller: Parse and validate filter
Controller->>Repository: Call with summit and filter
Repository->>Repository: Query distinct speaker IDs<br/>for summit
Repository->>Repository: Apply optional filter
Repository->>Database: SELECT COUNT(DISTINCT presentations)<br/>WHERE speaker_id IN (...)
Database-->>Repository: count integer
Repository-->>Controller: count integer
Controller-->>Client: 200 OK<br/>{ "count": N }
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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
| name: "id", | ||
| in: "path", | ||
| required: true, | ||
| description: "Summit ID or slug", | ||
| schema: new OA\Schema(type: "string") | ||
| ), |
There was a problem hiding this comment.
Don't cast away documented slug support.
This endpoint advertises "Summit ID or slug", but intval($summit_id) turns any non-numeric slug into 0, so slug-based requests will always 404. Either pass $summit_id through unchanged or narrow the contract to integer IDs only.
Suggested fix
- $summit = SummitFinderStrategyFactory::build($this->summit_repository, $this->getResourceServerContext())->find(intval($summit_id));
+ $summit = SummitFinderStrategyFactory::build($this->summit_repository, $this->getResourceServerContext())->find($summit_id);Also applies to: 543-544
🤖 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/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php`
around lines 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.
| try { | ||
| $profile = $build_default_payment_gateway_profile_strategy->build(IPaymentConstants::ApplicationTypeRegistration); | ||
| $serializer = SerializerRegistry::getInstance()->getSerializer($profile, $this->getSerializerType()); | ||
| if (!is_null($serializer)) | ||
| $values['payment_profiles'][] = $serializer->serialize(AbstractSerializer::filterExpandByPrefix($expand, 'payment_profiles')); | ||
| } catch (\Exception $ex) { | ||
| Log::warning($ex->getMessage()); | ||
| } |
There was a problem hiding this comment.
Log the full exception object, not just the message.
Logging only $ex->getMessage() loses the stack trace and exception type, making debugging difficult. Use Log::warning($ex) to capture complete context.
🐛 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 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.
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
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
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
New Features
Bug Fixes
Chores
Tests