Repository navigation
Security: fix authenticated unsafe reflection - #455
Merged
Merged
Conversation
Reviewer's GuideThe PR hardens authenticated GenerateUrl AJAX handling against CWE-470 by replacing arbitrary service/method resolution with strict module-model and callback-method allowlists, while adding regression tests for both valid URL generation and the original exploit scenarios. Sequence diagram for secured authenticated URL generationsequenceDiagram
participant Client as AuthenticatedClient
participant Repository as MetaRepository
participant Model as ModuleModel
Client->>Repository: generateUrl(url, class, method, parameters)
Repository->>Repository: guardCallback(class, method)
alt Allowed module model and static callback
Repository->>Model: call_user_func_array([class, method], actualParameters)
Model-->>Repository: Generated URL
Repository-->>Client: Generated URL
else Invalid class or method
Repository-->>Client: Exception
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/Common/Doctrine/Repository/MetaRepository.php" line_range="20" />
<code_context>
+ * The only methods that may be invoked as a meta-url callback. Every module registers its URL
+ * generator through one of these (see Meta::setUrlCallback / AddMetaSubscriber).
+ */
+ private const ALLOWED_CALLBACK_METHODS = ['getUrl', 'getUrlForCategory'];
+
public function __construct(ManagerRegistry $registry)
</code_context>
<issue_to_address>
**Meta forms fail to save**
When an application uses AddMetaSubscriber with a service ID or a public URL-generating method other than getUrl or getUrlForCategory, metaRepository::guardCallback() rejects the configured service ID or method because it falls outside the module-model namespace or method allowlist, so MetaType cannot generate the meta data during submission and the content is not saved.
Update guardCallback() to accept the callback classes and public URL-generating methods allowed by the AddMetaSubscriber contract instead of restricting them to the module-model namespace and hard-coded method list.
Also at `src/Common/Doctrine/Repository/MetaRepository.php:78-84`.
</issue_to_address>
### Comment 2
<location path="src/Backend/Core/Tests/Ajax/GenerateUrlTest.php" line_range="27" />
<code_context>
+ 'className' => \Backend\Modules\Blog\Engine\Model::class,
+ 'methodName' => 'getUrl',
+ 'url' => 'Some Example Title',
+ 'parameters' => '',
+ ]
+ );
</code_context>
<issue_to_address>
**URL callback regression tests fail**
When the regression tests run with their empty `parameters` values, `GenerateUrl` unserializes each empty `parameters` string to `false`, then passes it to `MetaRepository::generateUrl()` whose parameter requires an array; the valid-callback test receives an error instead of HTTP 200, while the rejection tests pass for that type error without exercising the callback guard.
Pass `serialize([])` in these tests, or normalize a failed `unserialize()` result to an empty array before calling `generateUrl()`.
Also at `src/Backend/Core/Tests/Ajax/GenerateUrlTest.php:42`, `src/Backend/Core/Tests/Ajax/GenerateUrlTest.php:50`.
</issue_to_address>Sourcery assessment
Approval pending. 2 findings to address first.
Blocking findings: src/Common/Doctrine/Repository/MetaRepository.php:20, src/Backend/Core/Tests/Ajax/GenerateUrlTest.php:27
tijsverkoyen
approved these changes
Oct 12, 2026
jonasdekeukelaere
approved these changes
Oct 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Prevent authenticated meta URL generation from being used as an arbitrary callable vulnerability.
Bug Fixes:
Documentation:
Tests: