fix(bank-integration): preview XSS, header completeness, ref-number safeguards, per-file currency

- Escape t.ref_no / t.counterparty / t.purpose / error fields with
  escape_html when rendering the preview and error-detail dialogs. A
  malicious bank statement (or counterparty-supplied purpose text)
  containing HTML could otherwise execute in the importer's session.
- Parser now refuses to proceed if any standard field required by the
  selected Amount Mode is missing from the file headers — instead of
  silently dropping every row with reason "amount" / "date".
- Reference Number is now a required mapping for all Amount Modes,
  preventing a class of silent duplication on re-import. parse_excel_for_
  preview also returns empty_ref_count and the preview surfaces a warning
  about future duplicate risk when some rows have an empty Reference
  Number on disk.
- Add 'Use Currency From File' checkbox on Excel Preset (default off).
  When on, BT.currency uses the parsed Currency column with fallback to
  the Bank Account's currency. import_bulk_bt now takes preset_name and
  the background job reads the flag.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ali 2026-05-13 16:52:34 +00:00
parent 5dd1296221
commit 70215e200c
5 changed files with 74 additions and 16 deletions

View File

@ -25,6 +25,15 @@ STANDARD_FIELD_TO_CODE = {
} }
# Internal codes that must be present in the headers of any imported file, by
# amount_mode. Kept in sync with bank_integration_excel_preset.py:_REQUIRED_BY_MODE.
_REQUIRED_CODES_BY_MODE = {
"Single column with sign": ("date", "reference_number", "amount"),
"Separate debit/credit columns": ("date", "reference_number", "debit", "credit"),
"Single column + direction column": ("date", "reference_number", "amount", "direction"),
}
# Tried in order until one parses the value. Covers ISO-style, day-first (EU/AZ), # Tried in order until one parses the value. Covers ISO-style, day-first (EU/AZ),
# and month-first (US) on the four common separators, with and without time. # and month-first (US) on the four common separators, with and without time.
# Day-first variants are listed BEFORE month-first to bias towards the AZ region; # Day-first variants are listed BEFORE month-first to bias towards the AZ region;
@ -105,9 +114,24 @@ def parse_excel(file_url, preset_name):
f"found in Excel headers ({[h for h in headers if h]})" f"found in Excel headers ({[h for h in headers if h]})"
) )
amount_mode = preset.amount_mode or "Separate debit/credit columns"
found_codes = set(idx_to_standard.values())
required = _REQUIRED_CODES_BY_MODE.get(amount_mode, ())
missing_codes = [c for c in required if c not in found_codes]
if missing_codes:
# Reverse-map codes back to human labels for the message.
code_to_label = {v: k for k, v in STANDARD_FIELD_TO_CODE.items()}
missing_labels = [code_to_label.get(c, c) for c in missing_codes]
frappe.throw(
"Required column(s) for Amount Mode '{}' not found in the file headers: {}. "
"Either the Header Row on the preset points at the wrong line, or the Excel "
"column names in the preset don't match the file. Headers seen: {}".format(
amount_mode, ", ".join(missing_labels), [h for h in headers if h]
)
)
use_custom = bool(preset.get("use_custom_date_format")) use_custom = bool(preset.get("use_custom_date_format"))
custom_format = (preset.date_format or "").strip() if use_custom else "" custom_format = (preset.date_format or "").strip() if use_custom else ""
amount_mode = preset.amount_mode or "Separate debit/credit columns"
debit_aliases = _split_aliases(preset.get("direction_debit_values")) debit_aliases = _split_aliases(preset.get("direction_debit_values"))
credit_aliases = _split_aliases(preset.get("direction_credit_values")) credit_aliases = _split_aliases(preset.get("direction_credit_values"))

View File

@ -19,6 +19,11 @@ def parse_excel_for_preview(file_url, preset_name, bank_integration, bank_accoun
dropped_direction = result.get("dropped_direction", 0) dropped_direction = result.get("dropped_direction", 0)
unknown_directions = result.get("unknown_directions", []) unknown_directions = result.get("unknown_directions", [])
# Rows with an empty reference_number cannot be deduplicated against later
# imports — surface a warning so the user notices and fixes the source file
# or the Reference Number mapping in the preset.
empty_ref_count = sum(1 for t in transactions if not (t.get("ref_no") or "").strip())
if not transactions: if not transactions:
return { return {
"success": True, "success": True,
@ -29,6 +34,7 @@ def parse_excel_for_preview(file_url, preset_name, bank_integration, bank_accoun
"dropped_amount": dropped_amount, "dropped_amount": dropped_amount,
"dropped_direction": dropped_direction, "dropped_direction": dropped_direction,
"unknown_directions": unknown_directions, "unknown_directions": unknown_directions,
"empty_ref_count": 0,
} }
ref_nos = [t["ref_no"] for t in transactions if t.get("ref_no")] ref_nos = [t["ref_no"] for t in transactions if t.get("ref_no")]
@ -62,11 +68,12 @@ def parse_excel_for_preview(file_url, preset_name, bank_integration, bank_accoun
"dropped_amount": dropped_amount, "dropped_amount": dropped_amount,
"dropped_direction": dropped_direction, "dropped_direction": dropped_direction,
"unknown_directions": unknown_directions, "unknown_directions": unknown_directions,
"empty_ref_count": empty_ref_count,
} }
@frappe.whitelist() @frappe.whitelist()
def import_bulk_bt(txn_list, bank_integration, bank_account): def import_bulk_bt(txn_list, bank_integration, bank_account, preset_name=None):
"""Enqueue bulk Bank Transaction creation as a background job. """Enqueue bulk Bank Transaction creation as a background job.
Creates Bank Transactions only registries (Bank Integration Customer / Creates Bank Transactions only registries (Bank Integration Customer /
@ -84,6 +91,7 @@ def import_bulk_bt(txn_list, bank_integration, bank_account):
txn_list=txn_list, txn_list=txn_list,
bank_integration=bank_integration, bank_integration=bank_integration,
bank_account=bank_account, bank_account=bank_account,
preset_name=preset_name,
user=user, user=user,
queue="default", queue="default",
timeout=600, timeout=600,
@ -92,7 +100,7 @@ def import_bulk_bt(txn_list, bank_integration, bank_account):
return {"success": True, "enqueued": True, "total": len(txn_list)} return {"success": True, "enqueued": True, "total": len(txn_list)}
def _process_bulk_bt_import(txn_list, bank_integration, bank_account, user): def _process_bulk_bt_import(txn_list, bank_integration, bank_account, user, preset_name=None):
"""Background job: create one Bank Transaction per row.""" """Background job: create one Bank Transaction per row."""
frappe.set_user(user) frappe.set_user(user)
@ -105,6 +113,12 @@ def _process_bulk_bt_import(txn_list, bank_integration, bank_account, user):
frappe.db.get_value("Account", ba_account, "account_currency") if ba_account else None frappe.db.get_value("Account", ba_account, "account_currency") if ba_account else None
) or "AZN" ) or "AZN"
use_currency_from_file = False
if preset_name:
use_currency_from_file = bool(
frappe.db.get_value("Bank Integration Excel Preset", preset_name, "use_currency_from_file")
)
total = len(txn_list) total = len(txn_list)
imported_count = 0 imported_count = 0
errors = [] errors = []
@ -123,11 +137,13 @@ def _process_bulk_bt_import(txn_list, bank_integration, bank_account, user):
ref_no = (txn.get("ref_no") or "").strip() ref_no = (txn.get("ref_no") or "").strip()
amount = abs(flt(txn.get("amount", 0))) amount = abs(flt(txn.get("amount", 0)))
row_currency = (txn.get("currency") or "").strip().upper() if use_currency_from_file else ""
bt = frappe.new_doc("Bank Transaction") bt = frappe.new_doc("Bank Transaction")
bt.date = txn.get("date") bt.date = txn.get("date")
bt.deposit = amount if drcr == "C" else 0 bt.deposit = amount if drcr == "C" else 0
bt.withdrawal = amount if drcr == "D" else 0 bt.withdrawal = amount if drcr == "D" else 0
bt.currency = ba_currency bt.currency = row_currency or ba_currency
bt.description = txn.get("description") or purpose bt.description = txn.get("description") or purpose
bt.reference_number = ref_no bt.reference_number = ref_no
bt.transaction_id = ref_no bt.transaction_id = ref_no

View File

@ -12,6 +12,7 @@
"amount_mode", "amount_mode",
"direction_debit_values", "direction_debit_values",
"direction_credit_values", "direction_credit_values",
"use_currency_from_file",
"column_break_1", "column_break_1",
"use_custom_date_format", "use_custom_date_format",
"date_format", "date_format",
@ -70,6 +71,13 @@
"fieldtype": "Small Text", "fieldtype": "Small Text",
"label": "Credit Values" "label": "Credit Values"
}, },
{
"default": "0",
"description": "Off: every imported Bank Transaction gets its Bank Account's currency.\nOn: the parsed Currency column on each row is used, falling back to the Bank Account's currency when empty. Enable only for multi-currency Bank Accounts where the statement actually carries per-row currency.",
"fieldname": "use_currency_from_file",
"fieldtype": "Check",
"label": "Use Currency From File"
},
{ {
"fieldname": "column_break_1", "fieldname": "column_break_1",
"fieldtype": "Column Break" "fieldtype": "Column Break"

View File

@ -9,9 +9,9 @@ from frappe.model.document import Document
# Standard fields the parser needs depending on the chosen amount_mode. # Standard fields the parser needs depending on the chosen amount_mode.
# Date is always required. # Date is always required.
_REQUIRED_BY_MODE = { _REQUIRED_BY_MODE = {
"Single column with sign": ("Date", "Amount"), "Single column with sign": ("Date", "Reference Number", "Amount"),
"Separate debit/credit columns": ("Date", "Debit", "Credit"), "Separate debit/credit columns": ("Date", "Reference Number", "Debit", "Credit"),
"Single column + direction column": ("Date", "Amount", "Direction"), "Single column + direction column": ("Date", "Reference Number", "Amount", "Direction"),
} }

View File

@ -119,6 +119,7 @@ const BIExcelImport = {
const droppedAmount = r.message.dropped_amount || 0; const droppedAmount = r.message.dropped_amount || 0;
const droppedDirection = r.message.dropped_direction || 0; const droppedDirection = r.message.dropped_direction || 0;
const unknownDirections = r.message.unknown_directions || []; const unknownDirections = r.message.unknown_directions || [];
const emptyRefCount = r.message.empty_ref_count || 0;
if (droppedDate > 0 && !txns.length) { if (droppedDate > 0 && !txns.length) {
BIExcelImport._showDateParseHelp(droppedDate, values.preset); BIExcelImport._showDateParseHelp(droppedDate, values.preset);
@ -143,12 +144,12 @@ const BIExcelImport = {
return; return;
} }
BIExcelImport._showPreview(txns, skipped, values, listview, BIExcelImport._showPreview(txns, skipped, values, listview,
droppedDate, droppedAmount, droppedDirection, unknownDirections); droppedDate, droppedAmount, droppedDirection, unknownDirections, emptyRefCount);
}, },
}); });
}, },
_showPreview(txns, skipped, values, listview, droppedDate, droppedAmount, droppedDirection, unknownDirections) { _showPreview(txns, skipped, values, listview, droppedDate, droppedAmount, droppedDirection, unknownDirections, emptyRefCount) {
let table = '<div style="max-height: 500px; overflow-y: auto;">'; let table = '<div style="max-height: 500px; overflow-y: auto;">';
if (skipped > 0) { if (skipped > 0) {
table += '<div class="alert alert-info">' + table += '<div class="alert alert-info">' +
@ -174,6 +175,12 @@ const BIExcelImport = {
__('Dropped {0} row(s) — missing or zero amount.', [droppedAmount]) + __('Dropped {0} row(s) — missing or zero amount.', [droppedAmount]) +
'</div>'; '</div>';
} }
if (emptyRefCount > 0) {
table += '<div class="alert alert-warning">' +
__('{0} of {1} row(s) have no Reference Number — re-importing the same file will create duplicates because deduplication uses the Reference Number.',
[emptyRefCount, txns.length]) +
'</div>';
}
table += '<table class="table table-bordered" style="width:100%; table-layout:fixed;">'; table += '<table class="table table-bordered" style="width:100%; table-layout:fixed;">';
table += '<thead style="position:sticky; top:0; z-index:1; background:var(--bg-color);"><tr>'; table += '<thead style="position:sticky; top:0; z-index:1; background:var(--bg-color);"><tr>';
table += '<th style="width:4%;"><input type="checkbox" class="bi-select-all-txns"></th>'; table += '<th style="width:4%;"><input type="checkbox" class="bi-select-all-txns"></th>';
@ -185,6 +192,7 @@ const BIExcelImport = {
table += '<th style="width:28%;">' + __('Purpose / Description') + '</th>'; table += '<th style="width:28%;">' + __('Purpose / Description') + '</th>';
table += '</tr></thead><tbody>'; table += '</tr></thead><tbody>';
const esc = frappe.utils.escape_html;
txns.forEach((t, i) => { txns.forEach((t, i) => {
const dateStr = t.date ? moment(t.date).format('DD.MM.YYYY') : ''; const dateStr = t.date ? moment(t.date).format('DD.MM.YYYY') : '';
const amountStr = parseFloat(t.amount || 0).toFixed(2); const amountStr = parseFloat(t.amount || 0).toFixed(2);
@ -194,12 +202,12 @@ const BIExcelImport = {
const text = t.purpose || t.description || ''; const text = t.purpose || t.description || '';
table += '<tr>' + table += '<tr>' +
'<td><input type="checkbox" class="bi-select-txn" data-idx="' + i + '"></td>' + '<td><input type="checkbox" class="bi-select-txn" data-idx="' + i + '"></td>' +
'<td style="word-break:break-word;">' + (t.ref_no || '') + '</td>' + '<td style="word-break:break-word;">' + esc(t.ref_no || '') + '</td>' +
'<td>' + dateStr + '</td>' + '<td>' + dateStr + '</td>' +
'<td style="word-break:break-word;">' + (t.counterparty || '') + '</td>' + '<td style="word-break:break-word;">' + esc(t.counterparty || '') + '</td>' +
'<td style="text-align:right;">' + amountStr + '</td>' + '<td style="text-align:right;">' + amountStr + '</td>' +
'<td style="text-align:center;">' + drcrLabel + '</td>' + '<td style="text-align:center;">' + drcrLabel + '</td>' +
'<td style="word-break:break-word; font-size:0.9em;">' + text + '</td>' + '<td style="word-break:break-word; font-size:0.9em;">' + esc(text) + '</td>' +
'</tr>'; '</tr>';
}); });
table += '</tbody></table></div>'; table += '</tbody></table></div>';
@ -284,6 +292,7 @@ const BIExcelImport = {
txn_list: JSON.stringify(selected), txn_list: JSON.stringify(selected),
bank_integration: values.bank_integration, bank_integration: values.bank_integration,
bank_account: values.bank_account, bank_account: values.bank_account,
preset_name: values.preset,
}, },
error() { error() {
frappe.realtime.off('bi_bt_import_progress'); frappe.realtime.off('bi_bt_import_progress');
@ -387,14 +396,15 @@ const BIExcelImport = {
frappe.msgprint(__('No error details')); frappe.msgprint(__('No error details'));
return; return;
} }
const esc = frappe.utils.escape_html;
let html = '<div style="max-height:500px; overflow-y:auto;">'; let html = '<div style="max-height:500px; overflow-y:auto;">';
errs.forEach(e => { errs.forEach(e => {
html += '<div style="margin-bottom:12px; padding:10px; border:1px solid var(--border-color); border-radius:6px;">'; html += '<div style="margin-bottom:12px; padding:10px; border:1px solid var(--border-color); border-radius:6px;">';
html += '<strong>' + (e.ref_no || 'No ref') + '</strong>'; html += '<strong>' + esc(e.ref_no || 'No ref') + '</strong>';
html += ' <span class="label label-danger" style="margin-left:8px;">' + (e.error_type || 'error') + '</span>'; html += ' <span class="label label-danger" style="margin-left:8px;">' + esc(e.error_type || 'error') + '</span>';
html += '<div style="margin-top:6px; color:var(--text-muted);">' + html += '<div style="margin-top:6px; color:var(--text-muted);">' +
(e.counterparty || '') + ' — ' + parseFloat(e.amount || 0).toFixed(2) + '</div>'; esc(e.counterparty || '') + ' — ' + parseFloat(e.amount || 0).toFixed(2) + '</div>';
html += '<div style="margin-top:6px;"><code>' + (e.message || '') + '</code></div>'; html += '<div style="margin-top:6px;"><code>' + esc(e.message || '') + '</code></div>';
html += '</div>'; html += '</div>';
}); });
html += '</div>'; html += '</div>';