Parse MPMA CSV with the module written for it - #248
Closed
droplister wants to merge 1 commit into
Closed
Conversation
validation/csv.ts and validation/file.ts were unreferenced, each the better implementation of something a caller did inline and less carefully. This wires csv.ts up, and file.ts where it actually fits. The MPMA form parsed CSV with a regex over split lines. parseCSV replaces it and adds what that loop never had: a row cap, proper quoted-field parsing, and rejection of spreadsheet formula injection. Address checking is left off so the checksum-validating validateBitcoinAddress the form already used keeps running, rather than the module's format-only test - the surrounding divisibility lookup and memo length check are unchanged. One message changed: an unparseable quantity now reads "Quantity must be a number" rather than "Invalid quantity", which says why rather than only that. The test asserting the old wording is updated. The CSV upload had no size limit at all, only a name check - the whole file was read into memory whatever its size. validateFile now bounds it at 2MB and owns the extension check. Malicious-pattern detection is deliberately off: parseCSV already screens every field for injection, which covers the whole file rather than the first kilobyte, and a scan for script markers could trip on a legitimate memo. file.ts is NOT wired into the inscription upload, and should not be. Its detectMaliciousContent rejects any text file containing <script, <iframe or javascript:, and an HTML inscription carrying a script tag is exactly what that feature publishes. Its type and extension checks are wrong there for the same reason. The size-only check those forms do is correct for content meant to be arbitrary. Adds tests for both new protections: a row carrying =cmd|calc is rejected, and a quoted memo containing a comma survives as one field. Claude-Session: https://claude.ai/code/session_01CcjnCrgosSeshymXLBxdGj
Member
Author
|
Squashed into #249. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #247. Wires up the two validators #246 deliberately left undeleted — and finds that one of them must not go where I first said.
csv.ts → MPMA parsing
The form parsed CSV with
line.match(/(".*?"|[^,]+)(?=\s*,|\s*$)/g)overtext.split('\n').parseCSVreplaces that and adds what the loop never had:=cmd|calcwas accepted beforeAddress checking is left off (
validateAddresses: false) so the form's existingvalidateBitcoinAddress— which verifies the checksum — keeps running instead of the module's format-only test. The divisibility lookup and memo-length check are untouched.One message changed. An unparseable quantity now reads "Quantity must be a number" rather than "Invalid quantity" — it says why rather than only that. The test asserting the old wording is updated.
file.ts → the CSV upload, not the inscription upload
The CSV upload had no size limit at all — only
file.name.endsWith('.csv'), then the whole file was read into memory regardless of size.validateFilenow bounds it at 2 MB and owns the extension check.detectMaliciousPatternsis deliberately off:parseCSValready screens every field for injection, which covers the whole file rather than the first kilobyte, and a script-marker scan could trip on a legitimate memo.Why file.ts is not wired into inscriptions
I said earlier that
file-upload-input.tsx"validates nothing" and the inscription path was a gap. Checking the module changed that conclusion.detectMaliciousContentrejects anytext/*file containing<script,<iframeorjavascript:. An HTML inscription carrying a script tag is exactly what that feature publishes — wiring this in would have broken inscriptions, which only started working in 0.7.0. Its type and extension checks are wrong there for the same reason: an inscription is arbitrary content by definition.The size-only check those forms do is correct.
file.tswas written for uploads that should be constrained; inscriptions aren't.Tests
Two new cases covering exactly what the change buys:
=cmd|calcis rejectedVerification
tsc --noEmitclean;biome check srcclean (672 files)vitest run src— 3965 passed, 49 skippedwxt buildsucceedsplaywright test e2e/pages/compose/send/mpma.spec.ts— 6 passed