fix: review_1.10-11-12-13-14

[hduchesne]

Description

Harden form submission flow: sanitize forwarded field names, clarify chunked size enforcement, remove stored PII, and fail closed on JCR metadata/action errors

Checklist

Source code

• I've shared and documented any breaking change
• I've reviewed and updated the jahia-depends

Tests

• I've provided Unit and/or Integration Tests
• I've updated the parent issue with required manual validations

Tip

Documentation to guide the reviews: How to do a code review

[github-actions]

📝 Documentation Guidelines

Thank you for contributing to our documentation! To ensure your contributions meet our standards, please review these resources:

• How to push markdown documents to the Academy - Learn the process for publishing documentation
• Documentation style guide and writing guidelines - Follow our style conventions

This comment is posted automatically when changes are detected in the docs/ folder.

[Copilot]

Pull request overview

Hardens the form submission flow with four focused fixes: sanitizing multipart field names when forwarding submissions, clarifying that Content-Length enforcement is an early-reject optimization (chunked transfers fall through to ServletFileUpload.setSizeMax), removing client PII (ipAddress, submitterUsername, userAgent) from newly stored submissions and from CSV/JSON exports, and turning previously swallowed JCR errors during action resolution and field-metadata collection into fail-closed SubmissionExceptions with explicit error codes (FMDB_008, FMDB_500). New unit tests cover the sanitizer and both fail-closed paths.

Changes:

• Add ContentDispositionUtils.escapeFormFieldName and use it in ForwardSubmissionFormAction.writePart to prevent header injection via field names.
• Make FormFieldMetadataCollector.collect and FormSubmissionPipeline.resolveActionNodes/collectFormFieldInfo propagate RepositoryException as SubmissionException (fail-closed), and inject the metadata collector/JCRTemplate provider for testability.
• Drop ipAddress/submitterUsername/userAgent from SaveToJcrFormAction persistence and from CSV/JSON exports; update related docs.

Reviewed changes

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

Show a summary per file

File	Description
formidable-engine/src/main/java/org/jahia/modules/formidable/engine/actions/ContentDispositionUtils.java	New escapeFormFieldName helper that strips CR/LF and percent-encodes quotes.
formidable-engine/src/main/java/org/jahia/modules/formidable/engine/actions/forward/ForwardSubmissionFormAction.java	Uses the new escaper when emitting the name="..." attribute.
formidable-engine/src/main/java/org/jahia/modules/formidable/engine/servlet/FormSubmissionPipeline.java	Adds injectable collector/template providers; propagates JCR errors to FMDB_008/FMDB_500; clarifies guardContentLength comment.
formidable-engine/src/main/java/org/jahia/modules/formidable/engine/servlet/FormFieldMetadataCollector.java	Removes intermediate try/catch blocks so RepositoryException propagates to the pipeline.
formidable-engine/src/main/java/org/jahia/modules/formidable/engine/actions/storage/SaveToJcrFormAction.java	Stops persisting ipAddress, submitterUsername, userAgent.
formidable-engine/src/javascript/FormResults/export/formats/json.ts	Drops the three PII fields from JSON export.
formidable-engine/src/javascript/FormResults/export/formats/csv.ts	Drops the three PII columns from CSV export.
formidable-engine/src/test/java/.../FormSubmissionPipelineTest.java	Adds tests for the two new fail-closed branches via reflection.
formidable-engine/src/test/java/.../ContentDispositionUtilsTest.java	Adds tests for the new field-name escaper.
docs/save-to-jcr.md	Documents that legacy PII properties are no longer persisted.
docs/form-submission-flow.md	Documents chunked-encoding behavior and that setSizeMax is the authoritative limit.
docs/export.md	Removes PII columns from the SubmissionRow table.

[jkevan]

Status summary

Item	Title	Status
1.10	writePart — field name CRLF injection	✅ Addressed
1.11	guardContentLength chunked transfer undocumented	✅ Addressed
1.12	SaveToJcrFormAction stores PII without control	✅ Addressed (hard removal)
1.13	resolveActionNodes swallows RepositoryException	✅ Addressed — ⚠️ minor issues below
1.14	FormFieldMetadataCollector silently returns partial result	✅ Addressed

---

Item-by-item analysis

1.10 — writePart: Content-Disposition field name injection ✅

Review asked: escape CRLF and " in the name attribute written to the forwarded multipart Content-Disposition header.

What was done:

• New ContentDispositionUtils.escapeFormFieldName(String name) method added.
  • Strips \r and \n (prevents header injection).
  • Percent-encodes " → %22 (prevents attribute boundary break).
  • Fallback to "field" when the escaped result is blank.
• ForwardSubmissionFormAction.writePart(...) now calls ContentDispositionUtils.escapeFormFieldName(name) instead of passing name raw.
• ContentDispositionUtilsTest added with two test cases covering quote encoding and the blank/null fallback.

Assessment: Correctly implemented. The two-step approach (strip controls, encode quotes) matches the HTTP specification requirement for quoted-string values.

Minor observation (non-blocking): The fallback value "field" is a safe default but could cause silent name collisions if two form fields produce an empty name after stripping. In practice, JCR node names are always non-empty and are not contributor-supplied directly, so the risk is theoretical.

---

1.11 — guardContentLength: chunked transfer behaviour undocumented ✅

Review asked: document why Content-Length: -1 does not trigger the size guard.

What was done:

• A comment was added directly inside guardContentLength:

  // Early-reject optimization only: chunked requests legitimately report -1 here.
  // The definitive request-size enforcement still happens later in FormDataParser
  // via ServletFileUpload.setSizeMax(...) when the multipart stream is consumed.

• docs/form-submission-flow.md updated: the pipeline table entry for step 3 now reads "early reject oversized requests when Content-Length is present" and the body explains the chunked behaviour.

Assessment: Fully addressed. The Javadoc comment and the doc update together make the intentional skip unambiguous.

---

1.12 — SaveToJcrFormAction stores PII without operator control ✅

Review asked: add an operator-level toggle (storeSubmitterMetadata, default true) in FormidableConfig and document what personal data is persisted.

What was done:

• ipAddress, submitterUsername, and userAgent properties are no longer written to fmdb:formSubmission nodes.
• referer is kept and documented as the only remaining request-derived metadata.
• docs/save-to-jcr.md gains a "Personal Data" section explaining the decision: "IP address, username, and user-agent are not stored. Only the HTTP Referer header is retained to support form analytics."
• The CND declaration of the removed properties (ipAddress, submitterUsername, userAgent) is intentionally preserved in definitions.cnd for backward compatibility — existing nodes are not broken.

Assessment: The fix is correct and defensible. Hard removal is more conservative than the requested toggle and eliminates a configuration footgun.

Divergence from review ask: The review explicitly requested a configurable toggle. The implementation chose permanent hard removal instead. This is a stronger privacy-first stance but it is a one-way door — the fields cannot be re-enabled without a code change. If an operator had a legitimate use for IP logging (e.g. fraud detection), they have no way to opt back in. This trade-off should be acknowledged explicitly in the PR or the docs.

referer retention accepted: referer is still written unconditionally and is documented in save-to-jcr.md. The primary PII concern from the review was IP address storage; keeping referer for analytics purposes is acceptable.

---

1.13 — resolveActionNodes swallows RepositoryException ✅ with minor issues

Review asked: reject the submission (fail-closed) when the JCR action list cannot be read, instead of silently continuing with an empty action list.

What was done:

• resolveActionNodes() now declares throws SubmissionException.
• After the catch (RepositoryException e) block, throw new SubmissionException(ErrorCode.FMDB_008, ...) is added.
• A test case resolveActionNodesRejectsSubmissionWhenSystemReadFails covers this path.
• The JcrTemplateProvider functional interface + secondary constructor allow injection of a failing stub for the test.

Assessment: The fail-closed path is correctly implemented and tested.

Issue 1 — FMDB_008 semantic mismatch 🟠

ErrorCode.FMDB_008 is documented in docs/error-codes.md as:

"An action in the pipeline failed (e.g. forward target returned non-2xx, email could not be sent)"

It is now also used for "could not read the action list from JCR". These are operationally different failure modes — one is a downstream service error; the other is an internal repository read error. An operator monitoring the errorCode field in response bodies cannot distinguish between them. Either:

• Introduce a new code (e.g. FMDB_011) for "action list resolution failed", and update error-codes.md; or
• Update the FMDB_008 description in error-codes.md to cover both cases explicitly.

As-is, error-codes.md is stale and misleading.

Issue 2 — log.warn inconsistent with fail-closed pattern 🟡

All other fail-closed paths in FormSubmissionPipeline use log.error:

• Line 151: log.error("... rejecting submission (fail-closed)")
• Line 170: log.error("... rejecting submission (fail-closed)")
• Line 194: log.error("... rejecting submission (fail-closed)")

But resolveActionNodes line 311 uses:

log.warn("[FormSubmissionPipeline] Could not read actions from form '{}'", formId, e);

with no mention of fail-closed. A WARN log level followed by a 500-equivalent rejection will confuse operators who scan for ERROR to detect submission failures. Change to log.error and add "rejecting submission (fail-closed)" to match the established pattern.

---

1.14 — FormFieldMetadataCollector.collect returns partial metadata on JCR error ✅

Review asked: propagate the RepositoryException up to the caller and reject the submission fail-closed.

What was done:

• collect(String formId, Locale locale) now declares throws RepositoryException.
• The internal try/catch wrapper is removed; the exception propagates naturally up through traverseRecursively and registerField, which also declare throws RepositoryException.
• FormSubmissionPipeline.collectFormFieldInfo() catches RepositoryException and throws SubmissionException(ErrorCode.FMDB_500, ...) with log.error(..., "(fail-closed)").
• Test case collectFormFieldInfoRejectsSubmissionWhenMetadataCollectionFails (FMDB-500 path) added via the FieldMetadataCollectorAdapter injection point.

Assessment: Cleanly addressed. The propagation approach (let the exception bubble rather than re-catching at every level) is the correct pattern for this case.

---

Test infrastructure improvements

The PR introduces two package-private functional interfaces in FormSubmissionPipeline:

@FunctionalInterface
interface FieldMetadataCollectorAdapter {
    FormFieldMetadataCollector.Result collect(String formId, Locale locale) throws RepositoryException;
}

@FunctionalInterface
interface JcrTemplateProvider {
    JCRTemplate get();
}

These replace the need for reflection-based field injection to test the collectFormFieldInfo and resolveActionNodes paths. The secondary constructor is package-private and production code uses the one-argument constructor. This is a clean and idiomatic testing seam.

---

Summary of new issues surfaced

Severity	Location	Description
🟠 Medium	FormSubmissionPipeline.resolveActionNodes + error-codes.md	FMDB_008 reused for a semantically different failure; error-codes.md not updated
🟡 Low	FormSubmissionPipeline.resolveActionNodes line 311	log.warn used for fail-closed rejection — should be log.error with "rejecting submission (fail-closed)" for consistency

[jkevan]

One new commit added to PR #83 after the v1 review:

SHA	Message
97948ec	fix: Issue 1 — FMDB_008 semantic mismatch & Issue 2 — log.warn inconsistent with fail-closed pattern

---

Status summary

Finding (v1)	Description	Status
🟠 FMDB_008 semantic mismatch	Action list resolution failure reused same code as action execution failure; error-codes.md not updated	✅ Fixed
🟡 log.warn inconsistency	resolveActionNodes used WARN instead of ERROR for a fail-closed rejection	✅ Fixed

---

Detailed analysis

FMDB_008 semantic mismatch ✅

A new error code FMDB_012 (HTTP 500) is introduced specifically for action list resolution failures:

• ErrorCode.java: FMDB_012(500) added with comment "Action list could not be resolved from the repository"
• error-codes.md: new row FMDB-012 | 500 | Action list resolution failed — the pipeline could not read the configured actions node from the repository
• FormSubmissionPipeline.resolveActionNodes: now throws SubmissionException(ErrorCode.FMDB_012, ...)
• FormSubmissionPipelineTest: assertion updated to assertEquals(ErrorCode.FMDB_012, error.errorCode) with an updated comment explaining the distinction

FMDB_008 now unambiguously means "a downstream action execution failed", and FMDB_012 covers the upstream "could not even read the action list from JCR" case. Operators monitoring error codes in response bodies can now distinguish the two failure modes.

log.warn → log.error ✅

-log.warn("[FormSubmissionPipeline] Could not read actions from form '{}'", formId, e);
+log.error("[FormSubmissionPipeline] Could not read actions from form '{}' — rejecting submission (fail-closed)", formId, e);

Now consistent with all other fail-closed paths in FormSubmissionPipeline (lines 151, 170, 194). The "rejecting submission (fail-closed)" suffix is present, matching the established log pattern.

---

Summary

All findings from v1 are resolved. PR #83 is clean.

The base branch was changed.

[github-actions]

🦜 Chachalog

formidable patch

• Removed ipAddress, submitterUsername, and userAgent from newly stored form submissions; existing submissions keep these legacy properties but new CSV and JSON exports no longer include them.
  Hardened forwarded form submissions by sanitizing multipart field names to prevent header injection.
  Changed form submissions to be rejected with FMDB-012 when the configured action list cannot be read, and with FMDB-500 when form field metadata cannot be collected, instead of silently continuing.

Create a new entry online or run npx chachalog@0.5.1 prompt to create a new entry locally.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

[jahia-sonarqube]

Quality Gate failed

Failed conditions
B Security Rating on New Code (required ≥ A)
59 New Maintainability Issues (required ≤ 0)
E Reliability Rating on New Code (required ≥ A)
8 New Reliability Issues (required ≤ 0)
13 New Security Issues (required ≤ 0)
887 Added Technical Debt (required ≤ 0)

See analysis details on SonarQube