security(import): harden DocumentBuilderFactory against XXE (#528) #610
Reference in New Issue
Block a user
Delete Branch "feat/issue-528-xxe-hardening"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
XxeSafeXmlParserhelper inimporting/with a statichardenedFactory()method applying all 6 OWASP-recommended XXE protection features toDocumentBuilderFactoryDocumentBuilderFactory.newInstance()inMassImportService.readOds()withXxeSafeXmlParser.hardenedFactory()readOds_rejects_xxe_doctype_payload) and valid-ODS guard test (readOds_parses_valid_ods_correctly).semgrep/security.ymlwith CWE-611 rules forDocumentBuilderFactory,SAXParserFactory, andXMLInputFactorysemgrep-scanCI job (parallel, local rules,--errorflag)POI 5.5.0 note: POI does not protect against this. The vulnerable parser is a custom
DocumentBuilderFactorycall insidereadOds(), not inside POI's internal ODS reader. This hardening is the primary (and only) control.Test plan
&xxe;resolved silently — no exception), green after (SAXParseException: DOCTYPE is disallowed)semgrep --config .semgrep/security.yml --metrics=off backend/src/— exits 0 (no unprotected factories found)Closes #528
🤖 Generated with Claude Code
🏗️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
What I checked against the doc-update matrix
No new Flyway migration, no new backend package/domain module, no new SvelteKit route, no new Docker service, no new external system integration.
XxeSafeXmlParseris a package-private internal helper inside the existingimporting/package — not a new domain module. No documentation updates are required per the architecture doc-update table.Architecture observations
XxeSafeXmlParserplacement is correct. It lives inimporting/, is the only call site, and is correctly scoped as package-private. If other domains ever need XXE-safe XML parsing, the class can be promoted at that time — extracting it now would be premature.Visibility change on
readOds(private→ package-private) is a pragmatic testability trade-off. The alternatives (reflection, restructuring the method out of the service, or an inner static class) would all introduce more complexity than the small visibility leak costs. Acceptable.CI job parallelism is correctly wired — no
needs:dependency means it runs in parallel withbackend-unit-testsas intended. The comment documents the reason for local-rules-only (act_runnerOIDC limitation). Good.Suggestion (non-blocking)
The Semgrep rule messages for
sax-xxe-defaultandstax-xxe-defaultsay "Apply features before use" but don't point to a safe wrapper (unlikedbf-xxe-defaultwhich says "CallXxeSafeXmlParser.hardenedFactory()"). If a future developer hits one of those rules on a new SAX or StAX parser, they'd need to work out the fix themselves. Consider either extendingXxeSafeXmlParserwithhardenedSaxFactory()/hardenedStaxFactory()helpers and updating those rule messages, or adding a link to the OWASP XXE prevention cheat sheet in the message body.Not a blocker — the rules fire and block the build correctly as-is.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
TDD evidence
The PR description documents the red phase ("entity
&xxe;resolved silently — no exception") before the green phase ("SAXParseException: DOCTYPE is disallowed"). Tests were demonstrably written before the fix. ✅Code quality
XxeSafeXmlParseris clean: 20 lines, does one thing, private constructor prevents accidental instantiation, static factory method with a name that reveals intent. ✅Factory helpers in the test (
buildXxeOds,buildValidOds,writeOdsZip) are well-extracted and reusable. ✅Minor concerns (non-blocking)
1.
readOdsvisibility changeThe method changed from
privateto package-private to allowMassImportServiceTestto call it directly. This is a common trade-off but means the test is reaching into an internal method. A@VisibleForTestingannotation (from Guava or a local equivalent) would document the intent and signal that callers outside the package shouldn't rely on it. Not a blocker — the pattern is standard.2. Unescaped
cellValueinbuildValidOdsIf
cellValueever contains<,>, or&, the constructed XML will be malformed andbuilder.parse()will throw a confusingSAXParseExceptionunrelated to the XXE assertion. The current call site ("Mustermann") is safe, but the helper is silently fragile. A brief comment noting this limitation would prevent future confusion.3. Test comment includes a ticket reference
"Introduced by issue #528" belongs in a commit message, not source code. It will rot as issues get closed and GitHub/Gitea links decay. The first sentence is the valuable rule; the second is noise. Drop it or move to a commit footer.
Otherwise clean. No naming concerns, no function-doing-two-things violations, no dead code.
🔧 Tobias Wendt — DevOps & Platform Engineer
Verdict: ⚠️ Approved with concerns
What's done well
actions/checkout@v4andactions/setup-python@v5are pinned to current major versions ✅--metrics=offdisables Semgrep telemetry (correct for a self-hosted environment with no SEMGREP_APP_TOKEN) ✅--errorflag correctly treats all findings as blocking ✅needs:declared, runs immediately alongside other jobs ✅Blocker
pip install semgrepis unpinned.This installs the latest Semgrep at every CI run. When Semgrep ships a major release, rule syntax or analysis behavior can change without warning, silently breaking the scan or producing false positives that block unrelated PRs. Pin the version:
Renovate can track this automatically if
pipis added to its dependency discovery config.Suggestion (non-blocking)
Add pip caching.
setup-python@v5natively supports caching via thecacheparameter — no separateactions/cachestep needed:Saves ~10-15 seconds per CI run at no cost. Without it,
pip install semgrepdownloads ~30 MB on every job.Non-issue
runs-on: ubuntu-latestis unpinned, but this is consistent with every other job in the workflow. I won't ask for a change here in isolation — if we pin runners, we do it across the board in a separate PR.📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Requirements traceability
Issue #528 requested XXE hardening for the
DocumentBuilderFactorycall inreadOds(). The implementation addresses this precisely:&xxe;resolved silently)SAXParseException: DOCTYPE is disallowed)readOds_parses_valid_ods_correctlyScope is well-bounded
The "POI 5.5.0 note" in the PR body correctly distinguishes between the custom
DocumentBuilderFactorycall insidereadOds()(the vulnerable site, now hardened) and POI's internal ODS reader (not vulnerable here). This is the kind of requirements precision that prevents future rework.Positive scope expansion (intentional, not creep)
The Semgrep rules cover
SAXParserFactoryandXMLInputFactoryin addition toDocumentBuilderFactory. These additional rules have no corresponding application code changes — they act as preventive guardrails for future parser usage. This is appropriate scope expansion for a security hardening PR.One open question (non-blocking)
The Semgrep rules correctly detect absence of the
disallow-doctype-declfeature. But they don't verify thatsetNamespaceAware(true)doesn't inadvertently re-enable something. This is not a known attack vector — I'm flagging it as a documentation gap, not a vulnerability. If there's internal documentation on why the 6-feature set is sufficient, a link inXxeSafeXmlParserwould close this question permanently for future auditors.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
XXE hardening correctness
All 6 OWASP-recommended controls are applied in
XxeSafeXmlParser.hardenedFactory():disallow-doctype-decl = trueexternal-general-entities = false&xxe;entity expansionexternal-parameter-entities = false%xxe;parameter entities in DTDload-external-dtd = falsesetXIncludeAware(false)setExpandEntityReferences(false)The
disallow-doctype-declfeature is the strongest control — it terminates parsing before any DOCTYPE is processed, making the remaining 5 controls defense-in-depth for JVM implementations with quirky partial-protection behavior. This is textbook OWASP XXE prevention.Regression test quality
readOds()call, not a mock — confirms the hardening is wired correctly end-to-end ✅SAXParseExceptionwith "DOCTYPE is disallowed" is the exact exception thrown bydisallow-doctype-decl— not a generic assertion ✅file:///etc/hostnameis a safe but realistic entity target (benign data, no side effects in CI) ✅<text:p>&xxe;</text:p>) — proves the parse would have succeeded without protection ✅Semgrep rules
The rules correctly use
pattern-not-insideto flagnewInstance()calls without thedisallow-doctype-declfeature in scope. The...ellipsis allows for intervening lines between factory creation and the feature call, which is the right semantic.One observation (non-blocking): The rules use
severity: WARNINGbut CI runs--errorwhich treats warnings as blocking. This works correctly, but a developer reading.semgrep/security.ymlin isolation will see "WARNING" and potentially underestimate the actual build impact. Consider usingseverity: ERRORto match the CI intent — OWASP top-10 issues warrant ERROR, not WARNING.No residual XXE surface
The
readOds()method passes thefactory(now hardened) tofactory.newDocumentBuilder(). The returnedDocumentBuilderinherits all factory constraints. No bypass path remains.Clean fix. The regression test stays in the suite permanently — this is exactly how security fixes should be codified.
🧪 Sara Holt — Senior QA Engineer
Verdict: ✅ Approved
Test structure
readOds_rejects_xxe_doctype_payload,readOds_parses_valid_ods_correctly@TempDirfor file isolationassertThatThrownBywith specific type + messageassertThrows(Exception.class)buildXxeOds,buildValidOds,writeOdsZipMockitoExtensionclass setupOne concern (non-blocking)
buildValidOdsdoesn't escapecellValuebefore XML insertion:If
cellValuecontains XML metacharacters (<,>,&),builder.parse()throws a confusingSAXParseExceptionabout malformed XML rather than the intended test assertion. The current call site ("Mustermann") is safe, but the helper is silently fragile for future use. A brief// cellValue must not contain XML metacharacterscomment would prevent a confusing debugging session.What's missing (non-blocking, out of scope for this PR)
The existing
readOdstest suite doesn't cover: malformed ZIP (nocontent.xmlentry), empty file, null file path. These are pre-existing gaps and not the responsibility of this PR — flagging them for backlog consideration.Layer appropriateness
These tests sit at the right layer: unit-style (no Spring context, no database) exercising file-system I/O through a temp directory. They run fast and catch the exact behavioral contract. The
readOdspackage-private visibility change is the right trade-off to avoid reflection in tests.Good quality. The regression test will catch any future developer who tries to "simplify" the parser setup.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
No frontend, UI, or accessibility changes in this PR. All 5 changed files are backend/infrastructure:
MassImportService.java— file parsing logic onlyXxeSafeXmlParser.java— internal security utilityMassImportServiceTest.java— backend tests.semgrep/security.yml— static analysis rules.gitea/workflows/ci.yml— CI pipelineNo Svelte components, no Tailwind classes, no i18n keys, no design tokens, no user-facing error messages, no routes, no form fields. From a UX perspective, this PR makes the application more secure without changing any user-visible behavior — exactly what a well-scoped security hardening PR should do.
Nothing to flag here. LGTM from my side.
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
This is a textbook XXE remediation. The implementation follows the OWASP XML External Entity Prevention Cheat Sheet to the letter, and the regression test pattern is exactly right: red before fix, green after, stays in the suite permanently.
What I Checked
CWE-611 (XXE) remediation —
XxeSafeXmlParser.javaAll 6 OWASP-recommended
DocumentBuilderFactorycontrols are applied in the correct order:disallow-doctype-decl = true— primary control, kills DOCTYPE entirelyexternal-general-entities = false— beltexternal-parameter-entities = false— beltload-external-dtd = false— beltsetXIncludeAware(false)— kills XInclude injection vectorsetExpandEntityReferences(false)— defense-in-depth against entity expansion (billion laughs)The primary control (
disallow-doctype-decl) is sufficient on its own with Xerces, but applying all 6 is correct defense-in-depth practice — the belt-and-suspenders approach is appropriate for a security utility class.Regression test quality —
MassImportServiceTest.javareadOds_rejects_xxe_doctype_payloadis the canonical security regression test pattern:file:///etc/hostname)SAXParseExceptionwith message"DOCTYPE is disallowed"— not just any exception// Security regression — do not remove.is exactly the right annotationreadOds_parses_valid_ods_correctlyas a companion guard test is good practice — verifies hardening didn't break the happy path.Semgrep rules —
.semgrep/security.ymlThree rules covering all three Java XML parser factory types (DOM, SAX, StAX). The
pattern-not-insidenegative match fordisallow-doctype-declcorrectly gates on the primary control. The--errorflag in the CI job ensures new unprotected factories block the build.No false sense of security from POI
The PR description correctly notes that this is a custom
DocumentBuilderFactorycall, not POI's internal parser. The fix targets the actual vulnerable code path. Well understood.Suggestions (non-blocking)
1. Consider adding a
XxeSafeXmlParserTestunit testThe
hardenedFactory()method currently has no dedicated unit test — it's only indirectly tested viaMassImportServiceTest. A small direct test would prove the factory properties are set regardless of howreadOdsuses it:Not a blocker — the integration-style test in
MassImportServiceTestprovides sufficient regression coverage.2. Semgrep rule gap:
DocumentBuilderFactory.newInstance()insideXxeSafeXmlParseritselfThe
dbf-xxe-defaultrule will flag theDocumentBuilderFactory.newInstance()call insidehardenedFactory()because thepattern-not-insidelooks for$X.setFeature(...)— but$Xis the local variable bound tonewInstance(), and the rule might not bind correctly across the assignment. Worth verifying withsemgrep --config .semgrep/security.yml --metrics=off backend/src/locally. The PR description says it exits 0, so this is likely already handled — just worth a confirm.Overall Assessment
This PR closes a real vulnerability, follows the red/green/refactor TDD discipline for security fixes, adds a CI gate to prevent regression at the class level, and the implementation is correct. No blockers from a security perspective.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Clean, focused, and test-first. The change does one thing and does it well. My usual checklist found very little to flag.
What I Checked
TDD evidence
The PR description explicitly states "Regression test was red before the fix... green after." Both test methods (
readOds_rejects_xxe_doctype_payload,readOds_parses_valid_ods_correctly) were written to prove the behavior, not after the fact. Red/green/refactor is visible in the commit history.XxeSafeXmlParser.java— clean code assessmenthardenedFactory()is a single-purpose method, under 20 lines — passes the one-job rulehardenedFactory()is self-documentingclass, notpublic class) is appropriate — the factory is animporting-domain detail, not a shared utilityMassImportService.java— visibility wideningThe diff changes
readOdsfromprivateto package-private (List<List<String>> readOds(...)). This is necessary to allowMassImportServiceTest(same package) to call it directly. Understood — but it's worth knowing this makesreadOdscallable by any class in theimportingpackage, not justMassImportService. Acceptable tradeoff for testability in this case; there's no other class in the package that should be calling it directly.Test helper methods —
buildXxeOds,buildValidOds,writeOdsZipwriteOdsZipis correctly extracted as a shared helperbuildValidOdsnotingcellValue must not contain XML metacharactersis a good, intentional "why" commentNaming
hardenedFactory()— clearbuildXxeOds/buildValidOds/writeOdsZip— all reveal intentreadOds_rejects_xxe_doctype_payload— reads as a behavior sentenceSuggestions (non-blocking)
XxeSafeXmlParser— consider making it package-accessible only viahardenedFactoryThe class is already package-private (
class, notpublic class). This is correct. Just noting it explicitly for future reviewers: if someone tries to subclass or extendXxeSafeXmlParser, the private constructor plus package-private class will prevent it at compile time. No action needed.One minor nit:
buildXxeOdsjavadocThe
buildXxeOdsmethod has no Javadoc. The correspondingbuildValidOdshas one noting the metacharacter constraint. Adding a brief comment tobuildXxeOdsabout the entity target parameter format would be consistent — very low priority.Overall
Tight, TDD-compliant, readable. No blockers. The scope is minimal and correct. The visibility change on
readOdsis the one thing to keep in mind if theimportingpackage grows.🏛️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
The fix is architecturally sound. My concern is documentation: a new class has been added to the
importingpackage, and the doc update table requires a check against C4 diagrams andCLAUDE.mdfor new backend components.Architecture Assessment
Package placement
XxeSafeXmlParserlives inorg.raddatz.familienarchiv.importing. This is correct — it's a private implementation detail of the import domain, not a shared infrastructure utility. The package-private visibility (class, notpublic class) enforces this boundary at compile time. Well done.Static utility pattern
I noted in the architect persona that static utility methods that hide dependencies can make testing harder. In this case,
hardenedFactory()is a pure factory method with no external state — it takes no dependencies, produces a deterministic output, and is tested directly throughMassImportService. The static pattern is appropriate here; injecting aDocumentBuilderFactorysupplier would add complexity with no benefit.No layer boundary violations
MassImportServicecallsXxeSafeXmlParser.hardenedFactory()— both are in theimportingpackage. No cross-domain boundary is crossed. Services-calling-repositories pattern is unchanged.readOdsvisibility wideningThe method changes from
privateto package-private to allow direct testing. This is an accepted TDD tradeoff. There is currently only one other class in theimportingpackage (XxeSafeXmlParser), so the surface area is minimal.Blocker: Documentation check required
Per the architecture documentation rules, adding a new class to an existing backend domain requires checking whether the C4 diagram for that domain needs updating:
importing/)docs/architecture/c4/l3-backend-*.pumlfor that domainPlease confirm whether
docs/architecture/c4/contains animportingdomain diagram. If it does,XxeSafeXmlParsershould appear in it. If the diagram does not model individual classes (only services and controllers), this can be confirmed and closed.Also: this PR introduces
XxeSafeXmlParseras a new security utility pattern in the codebase. If this is the intended pattern for all future XML parsing (which the Semgrep rule strongly implies it should be), an ADR entry or a note inCLAUDE.md's importing section would prevent future developers from re-inventing this. Not a hard blocker if the scope is intentionally limited to this single fix, but worth a decision.Semgrep CI job placement
The new
semgrep-scanjob runs in parallel with other jobs rather than being gated on unit tests. This is correct — static analysis should give fast feedback independently. The--errorflag ensures failures block the build. No concerns here.Summary
Architecturally clean. The only action required is a quick check on whether the C4 L3 diagram for the
importingdomain needs updating withXxeSafeXmlParser. Everything else is in order.🚀 Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer
Verdict: ✅ Approved
The CI job is clean, minimal, and fits the existing pipeline structure. A few observations, nothing blocking.
What I Checked
semgrep-scanjob in.gitea/workflows/ci.ymlVersion pinning ✅
Semgrep is pinned to
1.163.0. This is exactly right — unpinned tools in CI produce non-reproducible builds. Renovate should pick this up for automated version bump PRs.Python version ✅
setup-python@v5is current (not deprecated). Python 3.11 is appropriate.cache: 'pip'reduces install time on subsequent runs.--metrics=offflag ✅Prevents Semgrep from phoning home during CI runs. Correct for a self-hosted setup where outbound traffic from the runner should be minimal.
--errorflag ✅Exits non-zero on any finding. This means new unprotected XML parser factories block the build. Exactly what a security gate should do.
Parallelism ✅
The job runs in parallel with
backend-unit-tests(noneeds:dependency). This is intentional per the comment and correct — static analysis should run independently for fast feedback.Suggestions (non-blocking)
1. No cache for the Semgrep pip install beyond
setup-python's built-incache: 'pip'The
pip install semgrep==1.163.0will be cached bysetup-python'scache: 'pip'on subsequent runs with the same Python version. This is sufficient. No additionalactions/cachestep needed.2. Consider adding Semgrep to the Renovate config
If Renovate is configured for this repo, the
semgrep==1.163.0version pin in the workflow file should be covered by a RenovatepippackageRule to get automated update PRs. Worth checkingrenovate.jsonor.renovatercto confirm Semgrep is in scope. Not blocking — just a maintenance hygiene note.3. Checkout action version
actions/checkout@v4is current. ✅Overall
Clean CI job. Minimal, correct, and reproducible. No action required from a DevOps perspective.
🧪 Sara Holt (@saraholt) — QA Engineer & Test Strategist
Verdict: ✅ Approved
Both tests are well-structured, correctly placed in the test pyramid, and the regression test is permanently anchored with a
// Security regression — do not remove.comment. Coverage of the fix is complete.Test Quality Assessment
Test names — descriptive sentence format ✅
Both names read as behavior descriptions. When they fail in CI, the developer knows what broke without opening the test body.
Arrange-Act-Assert structure ✅
readOds_rejects_xxe_doctype_payload:buildXxeOds(tempDir, "file:///etc/hostname")— realistic, specific payloadassertThatThrownBy(() -> service.readOds(malicious)).isInstanceOf(SAXParseException.class).hasMessageContaining("DOCTYPE is disallowed")readOds_parses_valid_ods_correctly:buildValidOds(tempDir, "Mustermann")service.readOds(valid)rows.isNotEmpty()+rows.get(0).contains("Mustermann")One logical assertion per test ✅
The XXE test asserts on the exception type AND message in a single
assertThatThrownBychain — this is one logical assertion (exception with specific characteristics). The guard test asserts both non-empty and cell value, which is acceptable since they describe the same behavior (ODS parsed correctly).Test layer placement ✅
These are unit-ish tests:
MassImportServiceTestuses@ExtendWith(MockitoExtension.class)with mocked dependencies. ThereadOdstests use@TempDirand real ZIP construction — no Spring context, no database. They belong exactly here: fast, isolated, sub-second execution.Helper method quality ✅
buildXxeOds,buildValidOds, andwriteOdsZipare all under 20 lines and well-factored.writeOdsZipis correctly extracted as a shared helper rather than duplicated. The parameterization ofentityTargetinbuildXxeOdsallows future tests to target different payloads (local file, remote URL, etc.) without new helpers.@TempDirusage ✅@TempDir Path tempDiris the correct JUnit 5 pattern for temporary file tests. Files are automatically cleaned up after each test — no@AfterEachteardown needed.Suggestions (non-blocking)
1. Consider a second XXE payload variant — network-based SSRF
The current test uses
file:///etc/hostname. A network-based entity target (http://169.254.169.254/latest/meta-data/) would cover the SSRF via XXE vector as well. Not required —disallow-doctype-decl = trueblocks both equally — but it would make the test's coverage of the threat model more explicit:Non-blocking — the current test is sufficient. This is a "nice to have" for documentation-by-test purposes.
2. Guard test assertion could be more specific
containschecks that the list contains the string"Mustermann"as one element. This is fine. If the ODS parsing ever changes row structure, this will catch it. No issue.Coverage Assessment
The security fix in
XxeSafeXmlParser.hardenedFactory()is covered indirectly throughMassImportServiceTest. Direct unit tests on the factory method (as suggested by Nora) would give more granular failure messages if a feature flag were ever inadvertently removed, but the current integration-style coverage is not a gap for test strategy purposes.Overall
The test suite for this fix is solid. Both behavioral and regression coverage are present. No blockers.
📋 Elicit — Requirements Engineer & Business Analyst
Verdict: ✅ Approved
This PR directly closes issue #528. The implemented scope matches the stated requirements precisely, with no scope creep and no missing acceptance criteria.
Requirements Traceability Check
Issue #528 scope vs. implementation
Based on the PR description and diff, the delivered scope is:
DocumentBuilderFactoryagainst XXEXxeSafeXmlParser.hardenedFactory()applies all 6 OWASP controlsMassImportService.readOds()factory.setNamespaceAware(true)preservedreadOds_rejects_xxe_doctype_payloadwith realistic payloadreadOds_parses_valid_ods_correctlysemgrep-scanjob with--errorflagAcceptance criteria coverage
The PR's test plan section mirrors the acceptance criteria of issue #528 exactly. All four items are verified by the implementation and test suite.
Scope Assessment
No gold-plating ✅
The PR does not extend hardening to other XML contexts that weren't part of the issue (e.g., OCR service, other parsers). The Semgrep rules ensure those would be caught if they emerge — the detection capability is established without over-reaching the fix.
No scope gaps ✅
The PR description notes that POI does not expose the vulnerable code path — the vulnerable factory is the custom call in
readOds(). This is correctly scoped.Non-functional requirements ✅
One Open Question (informational, not blocking)
The Semgrep rules cover
DocumentBuilderFactory,SAXParserFactory, andXMLInputFactory. The PR only fixes aDocumentBuilderFactorycall. If the codebase were ever to add SAX or StAX parser usage, the rules would catch it — but there's no corresponding hardened utility for those factory types yet (XxeSafeXmlParser.hardenedFactory()returns onlyDocumentBuilderFactory).This is not a gap for this PR (those factory types aren't in use), but it's worth noting in the issue as a future consideration: if SAX or StAX parsers are added, the pattern should be extended. Recommend adding a note to issue #528 before closing, or creating a follow-up item.
Overall
Precisely scoped to the issue. Requirements are fully met. No missing acceptance criteria, no untracked scope additions.
🎨 Leonie Voss (@leonievoss) — UI/UX Design Lead
Verdict: ✅ Approved
This PR is entirely backend and CI — no frontend components, no routes, no UI changes, no Svelte files, no Tailwind classes, no accessibility concerns.
What I Checked
This PR touches:
backend/src/main/java/org/raddatz/familienarchiv/importing/XxeSafeXmlParser.java— new Java utility classbackend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java— one-line factory replacementbackend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java— test additions.semgrep/security.yml— new Semgrep rules.gitea/workflows/ci.yml— new CI jobNone of these files have any user-facing UI surface. There are no Svelte components, no route changes, no i18n keys added or removed, no design tokens touched, and no frontend code of any kind.
From a UX perspective, this PR reduces the risk of a security incident (XXE file exfiltration) that could have impacted user trust and data confidentiality — which is ultimately a UX concern. But the implementation is invisible to users, which is exactly as it should be for a security hardening fix.
No UI findings. LGTM.