From 0ce64eccd8607bf50172ce9b87f76c26cd576b45 Mon Sep 17 00:00:00 2001 From: Ben Kalsky Date: Fri, 3 Jul 2026 01:49:25 +0300 Subject: [PATCH 1/3] fix: address Codex review findings in form parsing, redaction, SearchMode Three fixes flagged by Codex review on earlier PRs (#1, #14): - formToNestedObject: skip keys that yield no segments (e.g. `=x`) so hostile urlencoded input can't make webhook handlers throw. - redactSensitiveText: mask processor codes as `Upay_[REDACTED]` so isFailedStatus still sees the processor failure signal; responses whose only failure indicator is an Upay_* code classify as payment.failed again instead of sumit.trigger.unmapped. - buildCreateDocumentPayload: derive SearchMode from blank-stripped ids so a whitespace-only `id` can't select search-by-ID while ID is omitted from the payload. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 12 ++++++++++++ src/index.test.ts | 30 ++++++++++++++++++++++++++++-- src/index.ts | 18 +++++++++++++----- 3 files changed, 53 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b0d8ab..974ca40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,18 @@ All notable changes to this package are documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Fixed + +- `normalizeSumitIncomingPayload` no longer throws on urlencoded payloads that contain an empty key (e.g. `=x`) — malformed or hostile form input falls through to normal handling instead of a `TypeError`. +- `redactSensitiveText` masks processor codes as `Upay_[REDACTED]` instead of `[REDACTED]`, so failure classification keeps seeing the processor signal after redaction. Responses whose only failure indicator is an `Upay_*` code surface as `payment.failed` again instead of `sumit.trigger.unmapped`. +- `buildCreateDocumentPayload` derives `SearchMode` from the blank-stripped customer identifiers — a whitespace-only `id` no longer selects search-by-ID while `ID` is omitted from the payload, which skipped a valid `ExternalIdentifier` upsert. + +### Documentation + +- README documents that the create-document builder never sends `Payments` rows, so payment-bearing document types (`Receipt`, `InvoiceAndReceipt`) should be issued via the charge endpoints or the SUMIT UI. + ## [0.4.0] - 2026-06-26 ### Added diff --git a/src/index.test.ts b/src/index.test.ts index e70ede6..a01832a 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -210,14 +210,14 @@ describe("@deepclaw/sumit", () => { eventType: "payment.failed", status: "Failed", userErrorMessage: "Payment failed", - technicalErrorDetails: "הסכום נמוך מדי יש להקליד סכום גבוה יותר. (קוד [REDACTED])", + technicalErrorDetails: "הסכום נמוך מדי יש להקליד סכום גבוה יותר. (קוד Upay_[REDACTED])", diagnostic: { hasData: false, dataKeys: [], hasCustomerID: false, recurringItemCount: 0, userErrorMessage: "Payment failed", - technicalErrorDetails: "הסכום נמוך מדי יש להקליד סכום גבוה יותר. (קוד [REDACTED])", + technicalErrorDetails: "הסכום נמוך מדי יש להקליד סכום גבוה יותר. (קוד Upay_[REDACTED])", }, }); }); @@ -657,4 +657,30 @@ describe("@deepclaw/sumit", () => { expect(redacted.TechnicalErrorDetails).not.toContain("987654321"); expect(redacted.TechnicalErrorDetails).not.toContain("123456789"); }); + + it("ignores empty urlencoded keys instead of throwing", () => { + const form = new URLSearchParams("=x&Payment.Status=000&Payment.ValidPayment=true"); + const event = normalizeSumitIncomingPayload(form); + expect(event.eventType).toBe("payment.succeeded"); + }); + + it("classifies a failure whose only signal is a redacted Upay_* code", () => { + const event = normalizeChargeResponse({ TechnicalErrorDetails: "Upay_30001419" }); + expect(event.ok).toBe(false); + expect(event.eventType).toBe("payment.failed"); + expect(event.technicalErrorDetails).not.toContain("30001419"); + }); + + it("derives SearchMode from trimmed customer ids", () => { + const payload = buildCreateDocumentPayload({ + companyId: 1, + apiKey: "k", + documentType: 3, + customer: { name: "C", id: " ", externalIdentifier: "ext-1" }, + items: [{ name: "Item", unitPrice: 10 }], + }); + expect(payload.Details.Customer.SearchMode).toBe(2); + expect(payload.Details.Customer.ID).toBeUndefined(); + expect(payload.Details.Customer.ExternalIdentifier).toBe("ext-1"); + }); }); diff --git a/src/index.ts b/src/index.ts index 4e8f95a..cedbbf8 100644 --- a/src/index.ts +++ b/src/index.ts @@ -370,17 +370,20 @@ export function buildCreateDocumentPayload(params: BuildCreateDocumentPayloadPar // Derive SearchMode: if the caller didn't pick one, use 1 (find by SUMIT ID) // when an `id` is supplied, 2 (upsert by ExternalIdentifier) when one is // supplied, and 0 (create new) otherwise. Matches SUMIT's documented modes. + // Derived from the blank-stripped values so a whitespace-only `id` cannot select + // mode 1 while `compact` drops the `ID` field from the payload. + const customerId = blankToUndefined(params.customer.id); + const customerExternalIdentifier = blankToUndefined(params.customer.externalIdentifier); const derivedSearchMode: SumitCustomerSearchMode = - params.customer.searchMode ?? - (params.customer.id ? 1 : params.customer.externalIdentifier ? 2 : 0); + params.customer.searchMode ?? (customerId ? 1 : customerExternalIdentifier ? 2 : 0); const customer = compact({ SearchMode: derivedSearchMode, Name: blankToUndefined(params.customer.name) ?? params.customer.name, EmailAddress: blankToUndefined(params.customer.emailAddress), Phone: blankToUndefined(params.customer.phone), - ExternalIdentifier: blankToUndefined(params.customer.externalIdentifier), - ID: blankToUndefined(params.customer.id), + ExternalIdentifier: customerExternalIdentifier, + ID: customerId, CompanyNumber: blankToUndefined(params.customer.taxId), Address: blankToUndefined(params.customer.address), City: blankToUndefined(params.customer.city), @@ -635,7 +638,9 @@ export function redactSensitiveText(value: string): string { .replace(/(singleuse)?token\s*[:=]\s*[^\s;,]+/gi, "$1token=[REDACTED]") .replace(/api\s*key\s*[:=]\s*[^\s;,]+/gi, "api key=[REDACTED]") .replace(/card(number)?\s*[:=]\s*[^\s;,]+/gi, "card$1=[REDACTED]") - .replace(/\bUpay_\w+/gi, "[REDACTED]") + // Keep the `Upay_` marker: isFailedStatus classifies failures by it, and the + // sensitive part is the specific processor code, not the processor name. + .replace(/\bUpay_\w+/gi, "Upay_[REDACTED]") .replace(/\b[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\b/gi, "[REDACTED]") .replace(/\b\d{12,19}\b/g, "[REDACTED]") .replace(/((?:citizen(?:[\s_]*id)?|ת\.ז\.?|מ\.ז\.?)\W*)\d{9}\b/gi, "$1[REDACTED]"); @@ -703,6 +708,9 @@ function formToNestedObject(form: URLSearchParams): UnknownRecord { return [{ name, index: undefined as number | undefined }, ...indices.map((index) => ({ name: "", index }))]; }); + // An empty key (e.g. `new URLSearchParams("=x")`) yields no segments — skip it + // instead of letting `assign` index into `undefined` and throw on hostile input. + if (segments.length === 0) continue; if (segments.some((segment) => segment.index === undefined && FORBIDDEN_FORM_KEYS.has(segment.name))) continue; let current: UnknownRecord | unknown[] = root; From 1defd2a17f1470431ae39d4913b523eee3236434 Mon Sep 17 00:00:00 2001 From: Ben Kalsky Date: Fri, 3 Jul 2026 01:49:25 +0300 Subject: [PATCH 2/3] docs: note payment-bearing document types are out of builder scope The create-document builder never sends Payments rows, so Receipt and InvoiceAndReceipt would be rejected or issued without the payment they document (Codex finding on #13). Point those at the charge endpoints or the SUMIT UI. Co-Authored-By: Claude Fable 5 --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 04a4be4..a4f7512 100644 --- a/README.md +++ b/README.md @@ -137,6 +137,8 @@ const payload = buildCreateDocumentPayload({ `SUMIT_DOCUMENT_TYPE` covers SUMIT's `Accounting_Typed_DocumentType` enum — `Invoice` (0, חשבונית מס), `InvoiceAndReceipt` (1, חשבונית מס-קבלה), `Receipt` (2, קבלה), `ProformaInvoice` (3, חשבון עסקה), `PriceQuotation` (12, הצעת מחיר), credit/expense variants, and more. Pass any numeric code directly via `documentType` if needed. +> **Payment-bearing document types are out of scope for this builder.** It never sends `Payments` rows, so types that record a received payment — `Receipt` (2) and `InvoiceAndReceipt` (1) — will be rejected by SUMIT or issued without the payment they're supposed to document. Issue those through the charge endpoints (a successful charge creates the document) or from the SUMIT UI. + `language` accepts either a `SUMIT_LANGUAGE` numeric code or the shorthand strings `"he"`/`"en"`/`"ar"`/`"es"` (and their full English names) — the helper converts to the numeric enum SUMIT requires. Unknown strings are dropped silently. `customer.searchMode` is derived automatically when omitted: SUMIT id `1`, ExternalIdentifier `2`, otherwise `0` (create new). Pass an explicit value to override. From ef9844aa122078c2c8bc0b85d0f4087c74e57ddb Mon Sep 17 00:00:00 2001 From: Ben Kalsky Date: Fri, 3 Jul 2026 01:49:25 +0300 Subject: [PATCH 3/3] ci: harden publish workflows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only main may run the publish jobs (workflow_dispatch was unrestricted), and the GitHub Packages workflow no longer points installs at npm.pkg.github.com — the publish step configures that registry and its auth itself. Mirrors Codex findings on the sumit-react workflows, which share this shape. Co-Authored-By: Claude Fable 5 --- .github/workflows/publish-github-packages.yml | 5 ++++- .github/workflows/publish-npm.yml | 2 ++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/workflows/publish-github-packages.yml b/.github/workflows/publish-github-packages.yml index 4567edb..98d7837 100644 --- a/.github/workflows/publish-github-packages.yml +++ b/.github/workflows/publish-github-packages.yml @@ -22,6 +22,8 @@ concurrency: jobs: publish: + # workflow_dispatch can be launched from any ref — only main may publish. + if: github.ref == 'refs/heads/main' runs-on: ubuntu-latest steps: - name: Checkout @@ -31,11 +33,12 @@ jobs: with: version: 10.26.0 + # No registry-url here: install must resolve from npmjs. The publish step + # below configures the GitHub Packages registry and auth itself. - uses: actions/setup-node@v4 with: node-version: 22 cache: pnpm - registry-url: https://npm.pkg.github.com - name: Install run: pnpm install --frozen-lockfile diff --git a/.github/workflows/publish-npm.yml b/.github/workflows/publish-npm.yml index f638e7c..eaccb5f 100644 --- a/.github/workflows/publish-npm.yml +++ b/.github/workflows/publish-npm.yml @@ -21,6 +21,8 @@ concurrency: jobs: publish: + # workflow_dispatch can be launched from any ref — only main may publish. + if: github.ref == 'refs/heads/main' runs-on: ubuntu-latest steps: - name: Checkout