Issue #1121: Fix blank Email options popup when adding an attachment

[franciscofourcade]

Fixes #1121 — ETP-4868

Root cause

PrintInvoices (and every other print controller) is mapped to three URLs: /invoices/send.html, /invoices/print.html and /invoices/PrintOptions.html. The Email options popup is opened through send.html, but its own form posts back to PrintOptions.html:

<form id="form" method="POST" action="PrintOptions.html" name="frmMain" enctype="multipart/form-data">

So the ADD command issued by "Add Attachment" arrives on PrintOptions.html, where openOptionsPage() matched neither branch:

• isPrintPath() → false, because the literal substring print.html is not contained in printoptions.html (the characters after print are options, not .html)
• isSendPath() → false, no send.html substring

The method returned without writing anything to the response, producing the empty 200 OK and the blank popup. No exception anywhere, because it was a silent no-op rather than a failure.

This is a regression from the ETP-4197 refactor (#1049, released in 26.1.10). Before it, the ADD command in PrintController.post() was an if/else with a fallback:

} else if (vars.commandIn("ADD")) {
  if (request.getServletPath().toLowerCase().indexOf(PRINT_PATH) != -1) {
    createPrintOptionsPage(...);
  } else {
    createEmailOptionsPage(...);
  }

The refactor turned that into two independent ifs with no fallback.

Fix

Restore the if/else shape in PrintControllerCommandHandler.openOptionsPage(): print.html renders the print options page, every other path falls back to the email options page. This was preferred over adding isPrintOptionsPath() as a third condition because it removes the whole class of failure — an unmatched path can no longer leave the response empty — and it matches the pre-regression behaviour exactly.

isSendPath() becomes unused and is removed (isPrintOptionsPath() stays, it is still used by validateSenderConfiguration()).

Tests

PrintControllerCommandHandlerTest had encoded the bug: testHandle_addCommand_otherPath_callsNeither() asserted that neither page builder was called. That expectation was replaced with correct coverage:

• testHandle_addCommand_printOptionsPath_callsCreateEmailOptionsPage — the exact regression
• testHandle_addCommand_printOptionsPath_salesOrder_callsCreateEmailOptionsPage — the handler is shared across document types
• testHandle_addCommand_otherPath_fallsBackToEmailOptionsPage — the response is never left empty
• testIsPrintPath_printOptionsHtml_returnsFalse — documents the substring trap that caused this

The three new ADD tests fail against the pre-fix code and pass with it. The whole reporting.printing test package (13 classes, 181 tests) passes.

Manual verification

Verified on a local 26.2.7 instance whose core sources were confirmed byte-for-byte identical to main before patching:

1. Reproduced the blank popup on the unpatched instance.
2. Deployed the fixed class, restarted, repeated the flow with a real file attached → the popup re-renders with all fields plus the new entry in "Attached Documents".
3. Checked the Print options popup (print.html path) still renders correctly — no regression on that branch.

Also included

Two malformed-markup fixes in the "Attached Documents" block of EmailOptions.html: an unclosed <th> and a value=""" attribute. The deprecated width attribute on the same line was converted to an inline style so the change does not introduce a new Web-analyzer violation on a modified line.

Deliberately not touched: the unbalanced div/tr/td nesting around the same block. Those <div id="sectionDetail"> / <div id="sectionDetail2"> elements are declared as <SECTION> in EmailOptions.xml, so they are XmlEngine section markers and their boundaries define which markup repeats per row. Rebalancing them would change repetition semantics, not just formatting — it belongs in its own change with its own validation.

[sonarscanetendo]

Quality Gate passed

Issues

• 0 New Issues
• 0 Fixed Issues
• 0 Accepted Issues

Measures

• 0 Security Hotspots
• 0.00% Coverage (0.00% Estimated after merge)
• 0.00% Duplicated Code (10.40% Estimated after merge)

Project ID: etendosoftware_etendo_core_AYOKvQCAuJ79WHyLB97g

View in SonarQube

[franciscofourcade]

SnykCode alert 3828 (java/RegexInjection) dismissed as a false positive

The 280-char limit on the dismissal comment does not fit the reasoning, so it is recorded here.

Where the alert points. PrintControllerCommandHandler.java:372 — the controller.createEmailOptionsPage(...) call. That line was not added by this PR; it was dedented out of the if (isSendPath()) block it used to sit in, which is enough for Snyk to treat it as changed and re-report a flow that already existed.

Why it does not hold. The sink is VariablesBase.transformNumber():

// src-core/src/org/openbravo/base/VariablesBase.java:813-827
String groupSeparator   = getSessionValue("#GROUPSEPARATOR|"   + DEFAULT_FORMAT_NAME);
String decimalSeparator = getSessionValue("#DECIMALSEPARATOR|" + DEFAULT_FORMAT_NAME);
...
value = value.replaceAll(groupSeparator, "");

In replaceAll(regex, replacement) the pattern is groupSeparator. It is written once per session at login, from the server-side Format.xml:

// src/org/openbravo/base/secureApp/LoginUtils.java:530-533
vars.setSessionValue("#DecimalSeparator|" + strNumberName,
    NumberElement.getAttributes().getNamedItem("decimal").getNodeValue());
vars.setSessionValue("#GroupSeparator|" + strNumberName,
    NumberElement.getAttributes().getNamedItem("grouping").getNodeValue());

That is the only write to those session keys in the codebase — no request parameter reaches them. The HTTP-controlled data flows into value, the subject string being scanned, not into the pattern. A ReDoS requires control over the pattern, so the precondition for this rule is not met.

Alert 3829 (Trust Boundary Violation, note) is left open. It is not a false positive and it does not fail the check. Its sink is VariablesBase.setSessionObject (src-core), untouched by this PR.

Separately, PrintController.createEmailOptionsPage() now builds the pocData session key with sanitizeDocumentIdentifier() instead of normalizeDocumentId(), matching the sanitizer PrintControllerCommandHandler already uses to read that same key back. That was a real write/read key mismatch found while tracing this alert, not a Snyk workaround.