From 77fb8e4ad02e342a59e0413af6a316cc50e1cbb2 Mon Sep 17 00:00:00 2001 From: Adam Moussa <166072409+amoussa1229@users.noreply.github.com> Date: Tue, 21 Jul 2026 20:28:13 -0400 Subject: [PATCH] fix(csv): M/D/YY date parsing with loud rejects; stop canceled rows zeroing stored fields (#72) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(csv): parse M/D/YY dates with loud rejects; stop canceled rows zeroing stored fields (#65, #68) Stampli exports switched from MM/DD/YYYY to M/D/YY, which toISODate mangled into garbage ISO strings and slackAppHome's parseMDYLocal turned into year-1926 dates. Canceled rows also blank 'Amount in USD' and can blank dates, which the unconditional upsert wrote over previously stored real values (116 records at amount_usd=0 as of the 2026-07-21 reconciliation). - src/dates.js: shared strict parser for M/D/YY + MM/DD/YYYY (real-date validation, 2-digit years 2000-based, plausibility window helper) - processPaymentCsv: canonicalize send_payment_on to MM/DD/YYYY; reject rows with unparseable/implausible dates and fail the invocation after valid rows are processed (surfaces via errors alarm + async DLQ; retry-safe since upserts are idempotent and existing checks are not re-offered to BoA); only overwrite amount_usd/send_payment_on with real values, falling back to the 'Amount' column on cancels - BoA add_Issue/cancel_Issue now use stored record values when the CSV row is blank (cancel_Issue previously sent amount 0.00 for voids) and skip registration on missing date/amount instead of sending garbage; same guard on the backfill path - slackAppHome: use the shared parser (fixes 1926 aging) * fix(csv): validate BoA registration data before writes; harden logging (security review) Hardening from the /sh-security-review pass on this branch: - BoA eligibility (resolvable amount + issue date) is now decided and validated BEFORE the DDB upsert: a cancel-transition row we cannot act on is rejected with the record untouched, so the transition gate stays open for a later clean file instead of silently losing the cancel forever (the skip guard previously ran after the status write) - First-seen rows that are already canceled are stored but never offered to add_Issue — registering a voided check as an active issue created a live issue with no cancel to follow (worsened by the Amount fallback making the previously-failing call succeed) - isPlausibleSendYear window is now relative (year-7..year+2) instead of hardcoded 2020-2035 - Untrusted CSV values are JSON-encoded and truncated before log interpolation (quoted CSV cells carry newlines -> CloudWatch log-line forgery, CWE-117) - OAuth token-exchange failures no longer throw the raw BoA authn response body (aligns with the PR #63 log-scrub posture) Verified: unit smoke on the date module; full replay of the real 2026-07-21 export (1,197 rows) against live table state produces 1,197 upserts, 0 rejects, 0 spurious BoA submissions. * fix(csv): comma-tolerant amount parsing in backfill (PR #72 review) Number() on a hand-inserted comma-formatted string amount would NaN and skip the check during backfill; hoist the CSV path's parseAmount to module scope and share it. No live records are affected (all amount_usd values are DDB Number type) — defensive consistency. --- src/dates.js | 53 +++++++++++++ src/processPaymentCsv.js | 166 ++++++++++++++++++++++++++------------- src/slackAppHome.js | 10 +-- 3 files changed, 167 insertions(+), 62 deletions(-) create mode 100644 src/dates.js diff --git a/src/dates.js b/src/dates.js new file mode 100644 index 0000000..77fb5ec --- /dev/null +++ b/src/dates.js @@ -0,0 +1,53 @@ +// Shared date parsing for Stampli M/D/YY and MM/DD/YYYY formats. +// Stampli exports switched from MM/DD/YYYY to M/D/YY in 2026-06/07; both +// formats must parse, and anything else must be rejected (null), never +// passed through as a malformed string (payments-dashboard#65). + +// Parse "M/D/YY" or "MM/DD/YYYY" into { year, month, day }, or null. +// Two-digit years are 2000-based. Validates the date is real (no 2/30). +export function parseMDY(value) { + const match = /^(\d{1,2})\/(\d{1,2})\/(\d{2}|\d{4})$/.exec(String(value ?? "").trim()); + if (!match) return null; + const month = parseInt(match[1], 10); + const day = parseInt(match[2], 10); + let year = parseInt(match[3], 10); + if (match[3].length === 2) year += 2000; + const dt = new Date(year, month - 1, day); + if (dt.getFullYear() !== year || dt.getMonth() !== month - 1 || dt.getDate() !== day) { + return null; + } + return { year, month, day }; +} + +// "YYYY-MM-DD" or null. +export function toISODate(value) { + const p = parseMDY(value); + if (!p) return null; + return `${p.year}-${String(p.month).padStart(2, "0")}-${String(p.day).padStart(2, "0")}`; +} + +// Canonical "MM/DD/YYYY" or null. Stored form for send_payment_on. +export function toCanonicalMDY(value) { + const p = parseMDY(value); + if (!p) return null; + return `${String(p.month).padStart(2, "0")}/${String(p.day).padStart(2, "0")}/${p.year}`; +} + +// Local-midnight Date or null. Replaces slackAppHome's parseMDYLocal, which +// built year-1926 dates from 2-digit years via new Date(26, ...). +export function parseMDYLocal(value) { + const p = parseMDY(value); + if (!p) return null; + return new Date(p.year, p.month - 1, p.day); +} + +// Plausibility window for ingested send dates. Anything outside is treated +// as parser garbage and the row is rejected loudly rather than stored. +// Relative to the current year so it never expires (7 years back covers +// historical re-ingests; 2 years forward covers future-scheduled checks). +export function isPlausibleSendYear(value) { + const p = parseMDY(value); + if (p === null) return false; + const now = new Date().getFullYear(); + return p.year >= now - 7 && p.year <= now + 2; +} diff --git a/src/processPaymentCsv.js b/src/processPaymentCsv.js index 8a77f3f..bb29558 100644 --- a/src/processPaymentCsv.js +++ b/src/processPaymentCsv.js @@ -3,6 +3,7 @@ import { DynamoDBClient } from "@aws-sdk/client-dynamodb"; import { DynamoDBDocumentClient, GetCommand, PutCommand, UpdateCommand, ScanCommand } from "@aws-sdk/lib-dynamodb"; import { SecretsManagerClient, GetSecretValueCommand } from "@aws-sdk/client-secrets-manager"; import { parse } from "csv-parse/sync"; +import { toISODate, toCanonicalMDY, isPlausibleSendYear } from "./dates.js"; const s3 = new S3Client(); const ddb = DynamoDBDocumentClient.from(new DynamoDBClient()); @@ -31,21 +32,24 @@ async function getAccessToken(applicationID, clientId, clientSecret) { }); if (!res.ok) { - const text = await res.text(); - throw new Error(`OAuth token exchange failed: ${res.status} - ${text}`); + throw new Error(`OAuth token exchange failed: HTTP ${res.status}`); } const data = await res.json(); return data.access_token; } -// Convert MM/DD/YYYY to YYYY-MM-DD -function toISODate(mdyDate) { - const parts = String(mdyDate).split("/"); - if (parts.length !== 3) return null; - const [mm, dd, yyyy] = parts; - return `${yyyy}-${mm.padStart(2, "0")}-${dd.padStart(2, "0")}`; -} +// Untrusted values (CSV cells, record fields) must be encoded before log +// interpolation — quoted CSV cells can carry newlines, which would forge +// CloudWatch log lines (CWE-117). +const logSafe = (v) => JSON.stringify(String(v ?? "").slice(0, 64)); + +// Comma-tolerant amount parsing, shared by the CSV path and the backfill so +// a hand-inserted "1,234.56" string record can't NaN out of registration. +const parseAmount = (value) => { + const num = parseFloat(String(value || "0").replace(/,/g, "").trim()); + return isNaN(num) ? 0 : num; +}; async function backfillBoA() { const cancelStatuses = ["voided", "cancelled", "canceled", "marked as void"]; @@ -84,10 +88,16 @@ async function backfillBoA() { if (submitted.has(item.check_number)) continue; if (cancelStatuses.includes((item.status || "").toLowerCase())) continue; if (item.check_number.length > 10) continue; + const issueDate = toISODate(item.send_payment_on); + const amount = parseAmount(item.amount_usd); + if (!issueDate || !(amount > 0)) { + console.error(`Backfill skipping check ${logSafe(item.check_number)}: missing issue date or amount`); + continue; + } toSubmit.push({ checkNumber: item.check_number, - amount: Number(item.amount_usd).toFixed(2), - issueDate: toISODate(item.send_payment_on), + amount: amount.toFixed(2), + issueDate, }); } payKey = payScan.LastEvaluatedKey; @@ -253,11 +263,6 @@ export const handler = async (event) => { return clean; }); - const parseAmount = (value) => { - const num = parseFloat(String(value || "0").replace(/,/g, "").trim()); - return isNaN(num) ? 0 : num; - }; - const cancelStatuses = ["voided", "cancelled", "canceled", "marked as void"]; // Status progression ranks — higher number = further along in lifecycle @@ -272,6 +277,7 @@ export const handler = async (event) => { const newChecks = []; const cancelChecks = []; + const rejectedRows = []; // Upsert each payment, tracking new and cancelled checks let count = 0; @@ -281,7 +287,17 @@ export const handler = async (event) => { const method = (row["Method"] || "").trim(); let status = (row["Status"] || "").trim(); - const sendOn = (row["Send Payment On"] || "").trim(); + const rawSendOn = (row["Send Payment On"] || "").trim(); + + // Reject rows with unparseable/implausible dates rather than storing + // garbage (Stampli switched MM/DD/YYYY -> M/D/YY once already; a future + // format change must fail loudly, not corrupt issueDate/aging). Blank is + // allowed — canceled payments legitimately have no send date. + const sendOn = rawSendOn ? toCanonicalMDY(rawSendOn) : ""; + if (rawSendOn && (sendOn === null || !isPlausibleSendYear(rawSendOn))) { + rejectedRows.push({ checkNumber, field: "Send Payment On", value: rawSendOn }); + continue; + } // ACH payments clear automatically on their send date if (method === "ACH" && !cancelStatuses.includes(status.toLowerCase())) { @@ -319,58 +335,87 @@ export const handler = async (event) => { } } + // Stampli blanks "Amount in USD" (and can blank dates) on canceled rows. + // Never let a blank overwrite a previously stored real value; fall back + // to the "Amount" column, which keeps its value on cancels. + const usdAmount = parseAmount(row["Amount in USD"]); + const amount = usdAmount > 0 ? usdAmount : parseAmount(row["Amount"]); + + // Decide the BoA action BEFORE writing DDB. If a row demands a BoA call + // we cannot make (no resolvable amount/issue date), reject it with the + // record untouched — writing first would flip the status and permanently + // close the transition gate, silently losing the cancel. First-seen rows + // that are already canceled are stored but never registered: add_Issue + // for a voided check would create an active issue with no cancel to follow. + const isCancelRow = cancelStatuses.includes(status.toLowerCase()); + const boaEligible = method === "Check" && checkNumber.length <= 10; + const boaAmount = amount > 0 ? amount : parseAmount(existing?.amount_usd); + const boaIssueDate = toISODate(sendOn) || toISODate(existing?.send_payment_on); + const needsAdd = boaEligible && !existing && !isCancelRow; + const needsCancel = + boaEligible && + existing && + isCancelRow && + !cancelStatuses.includes((existing.status || "").toLowerCase()); + if ((needsAdd || needsCancel) && (!boaIssueDate || !(boaAmount > 0))) { + rejectedRows.push({ + checkNumber, + field: needsAdd ? "add_Issue data" : "cancel_Issue data", + value: `issueDate=${boaIssueDate}, amount=${boaAmount}`, + }); + continue; + } + + const sets = [ + "#method = :method", + "payee = :payee", + "check_number = :check_number", + "invoice_numbers = :invoice_numbers", + "#status = :status", + "company_subsidiary = :company_subsidiary", + ]; + const values = { + ":method": method, + ":payee": (row["Payee"] || "").trim(), + ":check_number": checkNumber, + ":invoice_numbers": (row["Invoice Numbers"] || "").trim(), + ":status": status, + ":company_subsidiary": (row["Company/Subsidiary"] || "").trim(), + }; + if (amount > 0 || !existing) { + sets.push("amount_usd = :amount_usd"); + values[":amount_usd"] = amount; + } + if (sendOn || !existing) { + sets.push("send_payment_on = :send_payment_on"); + values[":send_payment_on"] = sendOn; + } + await ddb.send( new UpdateCommand({ TableName: TABLE_NAME, Key: { pk }, - UpdateExpression: ` - SET #method = :method, - payee = :payee, - check_number = :check_number, - invoice_numbers = :invoice_numbers, - send_payment_on = :send_payment_on, - amount_usd = :amount_usd, - #status = :status, - company_subsidiary = :company_subsidiary - `, + UpdateExpression: `SET ${sets.join(", ")}`, ExpressionAttributeNames: { "#method": "method", "#status": "status", }, - ExpressionAttributeValues: { - ":method": method, - ":payee": (row["Payee"] || "").trim(), - ":check_number": checkNumber, - ":invoice_numbers": (row["Invoice Numbers"] || "").trim(), - ":send_payment_on": sendOn, - ":amount_usd": parseAmount(row["Amount in USD"]), - ":status": status, - ":company_subsidiary": (row["Company/Subsidiary"] || "").trim(), - }, + ExpressionAttributeValues: values, }) ); count++; - // Only process checks for CashPro (BoA rejects check numbers > 10 digits) - if (method !== "Check") continue; - if (checkNumber.length > 10) continue; - - if (!existing) { - // New check → issue + if (needsAdd) { newChecks.push({ checkNumber, - amount: parseAmount(row["Amount in USD"]).toFixed(2), - issueDate: toISODate(sendOn), + amount: boaAmount.toFixed(2), + issueDate: boaIssueDate, }); - } else if ( - cancelStatuses.includes(status.toLowerCase()) && - !cancelStatuses.includes((existing.status || "").toLowerCase()) - ) { - // Existing check now voided/cancelled → cancel + } else if (needsCancel) { cancelChecks.push({ checkNumber, - amount: parseAmount(row["Amount in USD"]).toFixed(2), - issueDate: toISODate(sendOn), + amount: boaAmount.toFixed(2), + issueDate: boaIssueDate, }); } } @@ -481,13 +526,28 @@ export const handler = async (event) => { file_name: key.split("/").pop(), last_updated: new Date().toISOString(), last_file_count: count, + last_rejected_count: rejectedRows.length, }, }) ); console.log( - `Upserted ${count} payments, ${newChecks.length} issued, ${cancelChecks.length} cancelled` + `Upserted ${count} payments, ${newChecks.length} issued, ${cancelChecks.length} cancelled, ${rejectedRows.length} rejected` ); + + // All valid rows are processed and BoA submissions are done; now fail the + // invocation so rejects surface via the errors alarm + async DLQ. Retries + // are safe: upserts are idempotent and already-existing checks are not + // re-offered to BoA. + if (rejectedRows.length) { + for (const r of rejectedRows) { + console.error(`Rejected row: check ${logSafe(r.checkNumber)}, ${r.field}=${logSafe(r.value)}`); + } + throw new Error( + `${rejectedRows.length} of ${normalizedRows.length} rows rejected (bad dates or missing BoA data; ${count} valid rows processed)` + ); + } + return { statusCode: 200, body: `Upserted ${count}, issued ${newChecks.length}, cancelled ${cancelChecks.length}`, diff --git a/src/slackAppHome.js b/src/slackAppHome.js index 61741e2..4ff5587 100644 --- a/src/slackAppHome.js +++ b/src/slackAppHome.js @@ -2,6 +2,7 @@ import crypto from "node:crypto"; import { DynamoDBClient } from "@aws-sdk/client-dynamodb"; import { DynamoDBDocumentClient, GetCommand, ScanCommand } from "@aws-sdk/lib-dynamodb"; import { SecretsManagerClient, GetSecretValueCommand } from "@aws-sdk/client-secrets-manager"; +import { parseMDYLocal } from "./dates.js"; const ddb = DynamoDBDocumentClient.from(new DynamoDBClient()); const secrets = new SecretsManagerClient(); @@ -53,15 +54,6 @@ const formatCurrency = (value) => Number(value || 0) ); -const parseMDYLocal = (value) => { - if (!value) return null; - const parts = String(value).split("/"); - if (parts.length !== 3) return null; - const [mm, dd, yyyy] = parts.map((p) => parseInt(p, 10)); - if (!mm || !dd || !yyyy) return null; - return new Date(yyyy, mm - 1, dd); -}; - const formatDisplayDate = (date) => date.toLocaleDateString("en-US", { weekday: "short", month: "short", day: "numeric", year: "numeric" });