feat: let the Contact Us form carry screenshots and recordings - #3563
Open
jelveh wants to merge 1 commit into
Open
feat: let the Contact Us form carry screenshots and recordings#3563jelveh wants to merge 1 commit into
jelveh wants to merge 1 commit into
Conversation
The form doubles as our bug-report channel, and the bugs worth reporting are often the ones that need a picture: a visual glitch, or something that takes five steps to reach. Up to 5 images or videos may now ride along with the message, delivered as attachments on the support email. Nothing the client says about a file is believed. The endpoint takes bare base64 and re-derives all three of the things that matter from the decoded bytes: - Type, sniffed from magic numbers and matched against an allow-list of PNG/JPEG/GIF/WebP and MP4/QuickTime/WebM. A declared MIME is never read, so it cannot smuggle anything past the list. SVG is excluded on purpose (it carries script, and support tooling renders what it is sent), as are the non-video brands that share MP4's `ftyp` box -- HEIC, M4A, JPEG 2000. - The file name, reduced to a display label with the extension taken from the sniffed type. PNG bytes named `payload.html` arrive as `payload.png`; a name can never carry the CR/LF that would break out of a Content-Disposition header, nor the bidi overrides that make `report<RLO>gnp.exe` render as `report.exe.mp4`. - Size: 10 MB per file, 15 MB per submission, counted on decoded bytes. Encoded length is capped before decoding, so an oversized payload costs a length check rather than a 10 MB allocation. The total stays well under the 25 MB most providers enforce, since the outgoing mail base64s these again. Base64 is decoded strictly, reusing the round-tripping decoder that already guards app icons -- `Buffer.from(s, 'base64')` silently drops characters it does not recognise, and the round-trip is what rejects bytes smuggled after the payload. That decoder and the image sniffer move from appIcon.ts to a new mediaSniff.ts, which gains the video counterpart. One request is now worth megabytes of parsing and outbound mail, so the existing per-user rate limit gains a per-IP backstop (which the per-user counter cannot see through freshly minted accounts), a concurrency cap, and a Content-Length gate that refuses an impossible body before anything decodes it. Payloads are not stored. They ride the email; the new `feedback.attachments` column records names, types and sizes only, so an abusive submission stays attributable once the mail has been dealt with. Also fixes the form posting an empty message when Send was pressed with nothing typed, and surfaces submit failures instead of leaving the button disabled with no explanation.
Contributor
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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
1 participant
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.
What
The Contact Us form now takes up to 5 images or videos alongside the message. They're delivered as attachments on the support email.
Why
From a user report:
That's the case exactly: the form doubles as our bug-report channel, and a visual glitch or a five-step repro is far cheaper to show than to describe.
Security
Everything below is enforced server-side, on the decoded bytes. The client-side checks in
helpers/contact_attachments.jsexist only to fail fast before a 40 MB upload; they're not a control.Type. Sniffed from magic numbers and matched against an allow-list: PNG, JPEG, GIF, WebP, MP4, QuickTime, WebM. A caller-declared MIME is never read, so it can't be used to smuggle anything past the list. Two deliberate exclusions:
ftypbox: HEIC,M4A, JPEG 2000. The brand decides, not the box.File name. Reduced to a display label; the extension is re-derived from the sniffed type, so PNG bytes named
payload.htmlarrive aspayload.pngand nothing can ever come out.exe. Stripped: path components, C0/C1 controls (a CR/LF in a name would break out ofContent-Disposition), bidi overrides (report<RLO>gnp.exe→reportgnp.png), quoting characters, leading dots.Size. 10 MB per file, 15 MB per submission, 5 files. Counted on decoded bytes — the encoded length is capped before decoding, so an oversized payload costs a length check rather than a 10 MB allocation. The total sits well under the 25 MB ceiling most mail providers enforce, since the outgoing mail base64s these again.
Encoding. Strict base64, reusing the round-tripping decoder that already guards app icons.
Buffer.from(s, 'base64')silently drops characters it doesn't recognise, so<png>" onerror=alert(1)decodes without complaint; the round-trip is what rejects it.Rate. One request is now worth megabytes of parsing and outbound mail. The existing 10/15min per-user limit gains a 40/24h per-IP backstop (the per-user counter can't see a single machine cycling fresh accounts),
concurrent: 2per user, and aContent-Lengthgate that 413s an impossible body before anything decodes.Storage. Payloads are not stored. They ride the email with
Content-Disposition: attachment(no inline rendering). A newfeedback.attachmentscolumn records{name, type, size}only — enough to keep an abusive submission attributable once the mail has been dealt with, without parking megabytes in the database.Two things worth a maintainer's call:
allowFullAccessToken: truestays on this route, so an app holding a full-access token can submit attachments programmatically. That's pre-existing for the message field; I left it alone rather than risk breaking a first-party caller, and the allow-list plus caps bound what it can do. Happy to narrow it if you'd rather.Notes on the diff
util/appIcon.tsinto a newutil/mediaSniff.ts, which gains the video counterpart.appIcon.tsre-exportssniffImageMimeso nothing downstream changes.attachmentstext column across all three engines (sqlite 0067, mysql 22, postgres 11); the sqlite schema-version constant inSqliteDatabaseClient.test.tsmoves with it.Testing
51 new unit tests (42 for the validator, 9 for the GUI helper) plus 8 controller cases covering the allow-list, the size and count caps, malformed entries, and each file-name attack.
Also driven end to end against a local dev backend, through the real desktop dialog:
feedbackrow.image/pngparts,Content-Disposition: attachment, sanitized filenames, and a body manifest so the recipient can tell a stripped attachment from one that never existed. A file name carrying\r\nBcc: victim@example.com"; x="yproduced noBccheader.Full backend suite is green apart from 30 pre-existing failures that reproduce on a clean tree (node can't resolve
puter.localhoston this machine).Also fixed in passing