Security fixes#3749
Merged
Merged
Conversation
The Relation API resource is readable with `api:get` (PUBLIC_ACCESS) and embeds both `fromContent` and `toContent`, but ContentExtension only filtered the Content and Field resources. Anonymous users could therefore enumerate relations involving unpublished, draft or viewless content, leaking its existence, IDs and the relationship graph. Apply the same published/non-viewless filter to Relation queries, requiring both ends of the relation to be publicly visible. Add tests asserting the collection and item queries constrain both ends; they fail if the filter is removed.
Bind content-type slugs as query parameters instead of interpolating them into raw SQL strings. The split() helper now returns an array of slugs bound via ArrayParameterType::STRING rather than a pre-quoted SQL fragment. Add regression tests proving the slugs are passed as bound parameters and never reach the SQL string.
The /embed endpoint fetched any user-supplied URL server-side via the embed library after only a CSRF check, with the same SSRF exposure as the upload-from-URL endpoint. Guard it with UrlSafetyChecker before the fetch. Add controller tests for both /embed and /upload-url asserting that internal/private/metadata hosts and disallowed schemes are rejected with a 400. These tests fail if the UrlSafetyChecker guard is removed.
The /upload-url endpoint fetched any user-supplied URL server-side via Filesystem::copy() after only a format/scheme check, allowing requests to internal services and cloud metadata endpoints (e.g. 169.254.169.254). Add UrlSafetyChecker, which rejects non-http(s) schemes and any URL whose host resolves to a private, reserved, loopback or link-local address (including IPv4-mapped IPv6), and call it before fetching the URL. Add unit tests covering allowed public URLs and blocked schemes, private ranges, loopback, link-local/metadata and malformed input.
bobvandevijver
force-pushed
the
security-fixes
branch
from
f01e308 to
f146336
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces several security hardening changes across URL-fetching endpoints, SQL query construction, API filtering, and authentication redirects, with accompanying PHPUnit coverage.
Changes:
- Add SSRF protection via a new
UrlSafetyCheckerand enforce it in async upload-from-URL and embed endpoints. - Prevent SQL injection in
ListFormatHelperby switching to bound parameters (including array parameters forIN (...)). - Tighten public Relation API exposure by filtering out relations where either end is unpublished/viewless; and mitigate open redirect on login failure redirects.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/php/Utils/UrlSafetyCheckerTest.php | Adds unit tests for allowed/blocked URLs and malformed inputs. |
| tests/php/Utils/ListFormatHelperTest.php | Verifies content type slugs are bound as parameters (no interpolation). |
| tests/php/Controller/Backend/Async/UploadControllerTest.php | Ensures unsafe URLs are rejected before any fetch in upload-from-URL. |
| tests/php/Controller/Backend/Async/EmbedControllerTest.php | Ensures unsafe URLs are rejected before any server-side embed fetch. |
| tests/php/Api/ContentExtensionTest.php | Asserts Relation queries constrain both ends to published content. |
| src/Utils/UrlSafetyChecker.php | Introduces SSRF guard validating scheme + resolved IP public-ness. |
| src/Utils/ListFormatHelper.php | Switches to parameterized SQL (including array params for IN). |
| src/Security/LoginFormAuthenticator.php | Replaces referer-based redirect on failure; uses HttpUtils for success redirect creation. |
| src/Controller/Backend/Async/UploadController.php | Enforces SSRF guard prior to copying remote content. |
| src/Controller/Backend/Async/EmbedController.php | Enforces SSRF guard prior to fetching embed metadata. |
| src/Api/Extensions/ContentExtension.php | Filters Relation API results to avoid leaking unpublished/viewless endpoints. |
| phpstan-baseline.php | Updates baseline ignores. |
| composer.json | Adds an api-platform/core conflict bound and pins ECS dev dependency version. |
| CHANGELOG.md | Documents the security fixes in the 6.1.4 release notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mb_trim is not yet available in PHP 8.2 Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.
No description provided.