Normalize entry params

[alexej-d]

Changes in this pull request

Resolves #321

Additional info

Needed in addition to the main patch.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/Email/Service/EmailLogService.php:181

• The linked issue and “main patch” are about adding search/filtering to the sent-mails list, but this change only alters the email-parameter response. It neither implements nor supports the /emails/search flow in UI PR #3946, so it cannot resolve the cited issue as described. Please correct the PR description/linkage if this normalization is the intended scope, or include the backend search support that the description claims.

                $this->normalizeParamValue($value)

[jcPimcore]

Good fix — EmailLogEntryParameter's constructor requires ?string under strict_types=1, but Mail::setParam() accepts mixed, so any logged email with a bool/int/array template param would TypeError on GET /emails/{id}/params. normalizeParamValue() fixes this at the right spot (the service producing the value, not the schema contract), and correctly leaves the resend path (setEmailDocumentContent) untouched since that needs the original typed value.

One ask before merging: this repo has a solid per-service unit-test convention (tests/Unit/<Module>/Service/*ServiceTest.php), but there's no tests/Unit/Email suite at all. Since this fixes an actual crash, could you add a small test for getEntryParams()/normalizeParamValue covering bool/int/array values?

[alexej-d]

Good fix — EmailLogEntryParameter's constructor requires ?string under strict_types=1, but Mail::setParam() accepts mixed, so any logged email with a bool/int/array template param would TypeError on GET /emails/{id}/params. normalizeParamValue() fixes this at the right spot (the service producing the value, not the schema contract), and correctly leaves the resend path (setEmailDocumentContent) untouched since that needs the original typed value.

One ask before merging: this repo has a solid per-service unit-test convention (tests/Unit/<Module>/Service/*ServiceTest.php), but there's no tests/Unit/Email suite at all. Since this fixes an actual crash, could you add a small test for getEntryParams()/normalizeParamValue covering bool/int/array values?

Check the new commit @jcPimcore

[jcPimcore]

Confirmed — EmailLogServiceTest::testGetEntryParamsNormalizesNonStringValues correctly exercises the fixed path (bool → 'true', int → '118', array → JSON) and matches this repo's Codeception/makeEmpty convention. Thanks!

[jcPimcore]

Hi @alexej-d
Thank you for the update! Looks good! 👍
Since it is related to pimcore/studio-ui-bundle#3946 we will not merge this immediately but waiting for the other PR.

[alexej-d]

Hi @alexej-d Thank you for the update! Looks good! 👍 Since it is related to pimcore/studio-ui-bundle#3946 we will not merge this immediately but waiting for the other PR.

I have adjusted my code in both bundles @jcPimcore

[jcPimcore]

Hi @alexej-d
Great work again! 👍

There was something found which was not adressed before. Can you please implement this?

Security: /emails/search broadens exposure to an ORDER-BY injection sink

POST /emails/search (added in studio-backend-bundle#2004) passes the request's sortFilter.key straight through EmailLogRepository::applySearchOptions() → Listing::setOrderKey() → AbstractDao::getOrder(), which builds ORDER BY via raw string concatenation — no allow-list, no escaping.

This sink already exists (reachable today via POST /gdpr/search?provider=sent_mails), but that path requires the gdpr_data_extractor permission. The new endpoint gates the same sink behind emails instead — a much more commonly granted permission — so this PR meaningfully widens who can reach it.

Suggested fix: allow-list the sortable columns at EmailLogRepository::applySearchOptions() before calling setOrderKey(), e.g.:

private const array SORTABLE_KEYS = ['id', 'sentDate', 'from', 'to', 'subject'];

$sortFilter = $filter->getSortFilter();
if ($sortFilter->getKey() && in_array($sortFilter->getKey(), self::SORTABLE_KEYS, true) && $sortFilter->getDirection()) {
$listing->setOrderKey($sortFilter->getKey());
...
}

falling back to the existing default order otherwise. That's the smallest fix at the right boundary — it doesn't touch shared core (AbstractDao) and mirrors how a fixed version of this pattern should look.

---

Thanks again in advance!

Jochen

[jcPimcore]

Security fix as commented.

[alexej-d]

Security fix as commented.

Hi @jcPimcore – the fix is implemented as requested. I also added a test

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud