🐛 fix(context-url): add authorization, rate limiting, and input validation#4613
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request hardens the /api/v3/context-url endpoints to address an IDOR risk by enforcing project/password scoping in routes, adding an authorization chain in the controller, introducing request body JSON-schema validation, and adding rate limiting plus entity ownership checks before writing context URLs.
Changes:
- Updated v3 routes to include
id_projectandpasswordin the context-url route prefix. - Refactored
ContextUrlControllerto require validated project context, validate JSON body via a shared schema, rate-limit requests, and validate file/segment membership in the project. - Improved
SegmentMetadataDao::getBySegmentIds()by parameterizing theIN (...)clause; added new unit tests covering the new controller behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
lib/Routes/api_v3_routes.php |
Scopes context-url endpoints under /:id_project/:password to support project/password validation. |
lib/Controller/API/App/ContextUrlController.php |
Adds validator chain, JSON body validation, rate limiting, and entity existence/ownership checks. |
inc/validation/schema/segment_context_url.json |
Introduces a shared JSON schema for context-url payload validation. |
lib/Model/Segments/SegmentMetadataDao.php |
Parameterizes IN queries for segment metadata lookups. |
tests/unit/Core/Controllers/ContextUrlControllerTest.php |
Adds unit coverage for rate limiting, validation failures, authorization, existence checks, and happy paths. |
This comment was marked as outdated.
This comment was marked as outdated.
cb425e5 to
5b74d84
Compare
This comment was marked as outdated.
This comment was marked as outdated.
5b74d84 to
5298c59
Compare
Ostico
left a comment
There was a problem hiding this comment.
Both Copilot findings fixed in latest push (5298c59):
- Empty array guard —
getBySegmentIds()returns[]immediately for empty$ids - Cache key regression — explicit
$keyMap(self::_keymap_get_by_segment_ids . $key) passed to_fetchObjectMap()to preserve cache eviction
Unit test added: SegmentMetadataDaoTest::getBySegmentIds_returns_empty_array_for_empty_ids
This comment was marked as outdated.
This comment was marked as outdated.
5298c59 to
378bf4a
Compare
378bf4a to
2319d49
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
2319d49 to
c3d4b91
Compare
Ostico
left a comment
There was a problem hiding this comment.
All 3 new Copilot findings addressed in c3d4b91:
- XSS via dangerous URI schemes — JSON schema now uses
"pattern": "^https?://"+"minLength": 1instead of"format": "uri". Rejectsjavascript:,data:, and all non-http(s) schemes. Tests added for both. - Route type safety — Changed
[:id_project]to[i:id_project](Klein integer matcher). Non-integer paths rejected at routing level before reaching controller. - Empty body —
getValidatedBody()already throwsInvalidArgumentExceptionfor null body before JSON parsing.
This comment was marked as outdated.
This comment was marked as outdated.
…ation IDOR vulnerability: any authenticated user could modify context URLs for any project/file/segment. Added ProjectPasswordValidator + ProjectAccessValidator chain, RateLimiterService (IP + email), entity existence and domain checks, unified JSON schema validation across all endpoints, and parameterized SQL in SegmentMetadataDao to prevent injection.
c3d4b91 to
8295ef6
Compare
Ostico
left a comment
There was a problem hiding this comment.
All 4 findings from this review addressed in 8295ef6:
- Rate-limit test false positive (3 comments) — fixed. The same
$responseMockis now returned by theRateLimiterServicestub, soexpects(never())->method("json")asserts on the actual response object the controller uses after replacement. - Schema: add
format: "uri"— added alongsidepattern: "^https?://". Both constraints apply:formatvalidates URI structure,patternrestricts to http(s) schemes.
🧪 Test-Guard Report❌ FAIL — Some changed source files lack adequate test coverage. Coverage Analysis: ❌ FAILChanged lines: 87.0% covered (threshold: 80%) 📋 4 files: 1 ❌ fail, 3 ✅ pass
Test File Matching: ❌ FAILFile matching: 2 pass, 1 warning, 1 fail 📋 4 files: 1 ❌ fail, 1
|
| File | Verdict | Reason |
|---|---|---|
lib/Controller/API/App/ContextUrlController.php |
✅ pass | Test file modified in PR: tests/unit/Core/Controllers/ContextUrlControllerTest.php |
lib/Controller/Abstracts/KleinController.php |
Test file exists (tests/unit/Core/Controllers/Abstracts/KleinControllerTest.php) but was not modified in this PR | |
lib/Model/Segments/SegmentMetadataDao.php |
✅ pass | Test file modified in PR: tests/unit/Core/Model/Segments/SegmentMetadataDaoTest.php |
lib/Routes/api_v3_routes.php |
❌ fail | No matching test file found |
Per-File Evaluation: ❌ FAIL
Evaluated 4 files: 1 via AI (1 batch), 3 via shortcuts.
📋 4 files: 1 ❌ fail, 3 ✅ pass
| File | Verdict | Reason |
|---|---|---|
lib/Controller/API/App/ContextUrlController.php |
✅ pass | shortcut → coverage 87% ≥ 80% |
lib/Controller/Abstracts/KleinController.php |
✅ pass | shortcut → coverage 100% ≥ 80% |
lib/Model/Segments/SegmentMetadataDao.php |
✅ pass | shortcut → coverage 100% ≥ 80% |
lib/Routes/api_v3_routes.php |
❌ fail | No tests cover the new route parameter changes in the routing configuration. |
Result: ❌ FAIL
Why this FAIL?
- Coverage Analysis: lib/Routes/api_v3_routes.php has 0% diff coverage, meaning none of the changed lines in this routing configuration file are exercised by tests → add tests that exercise the new or changed route parameters.
- Test File Matching: No matching test file found for lib/Routes/api_v3_routes.php → create or modify a test file to cover this routing configuration.
- Per-File Evaluation: AI evaluation confirms no tests cover the new route parameter changes in lib/Routes/api_v3_routes.php → add targeted tests for the routing changes.
- Test File Matching: Warning for lib/Controller/Abstracts/KleinController.php as its test file exists but was not modified → no action needed unless the changes require test updates.
To resolve: add or update tests to cover the new or changed route parameters in lib/Routes/api_v3_routes.php.
Summary
Fix critical IDOR vulnerability in Context URL API: any authenticated user could read/write context URLs for any project, file, or segment. Added full authorization chain, rate limiting, entity validation, and unified JSON schema validation.
Type
fix— bug fixChanges
lib/Routes/api_v3_routes.php[:id_project]/[:password]to context-url route prefixlib/Controller/API/App/ContextUrlController.phpProjectPasswordValidator+ProjectAccessValidatorchain inafterConstruct,RateLimiterService(IP + email), entity existence and domain ownership checks, unified JSON body validation via schema, DI refactor for testabilityinc/validation/schema/segment_context_url.jsoncontext_url(format: uri), nullableid_segmentandid_filelib/Model/Segments/SegmentMetadataDao.phpgetBySegmentIds()IN clause to prevent SQL injectiontests/unit/Core/Controllers/ContextUrlControllerTest.phpTesting
vendor/bin/phpunit --exclude-group=ExternalServices --no-coveragepasses./vendor/bin/phpstanpasses (0 errors, with baseline)AI Disclosure
Claude Code (claude-opus-4-6)
Notes
id_projectandpasswordin the route path (/api/v3/context-url/:id_project/:password/...) and JSON body instead of form params