fix(meta): guard non-string mimetype before image check (fixes #2678) - #2691
fix(meta): guard non-string mimetype before image check (fixes #2678)#2691MatheusMartinho wants to merge 2 commits into
Conversation
Sorry, something went wrong.
…ion-foundation#2678) mimeTypes.lookup() returns `false`, not `undefined`, when it cannot resolve a type, e.g. a Chatwoot ActiveStorage audio URL with no file extension. Optional chaining only short-circuits on null/undefined, so `false?.startsWith('image/')` threw "TypeError: mimetype?.startsWith is not a function" in sendMessageWithTyping() before the request reached Meta. Check `typeof === 'string'` before calling startsWith; behavior for string mimetypes is unchanged.
Reviewer's guide (collapsed on small PRs)
Reviewer's GuideThis PR hardens the WhatsApp Business integration by ensuring mime type checks only run on string values, preventing crashes when mime-type resolution returns false for extension-less URLs (e.g., Chatwoot ActiveStorage audio attachments). Sequence diagram for guarded mimetype image check in WhatsApp Business servicesequenceDiagram
actor Chatwoot
participant BusinessStartupService
participant mimeTypes
Chatwoot->>BusinessStartupService: sendAttachment(url)
BusinessStartupService->>mimeTypes: lookup(url)
mimeTypes-->>BusinessStartupService: mimetype=false
BusinessStartupService->>BusinessStartupService: processAudio sets message[mimetype]=false
alt typeof message[mimetype] === 'string'
BusinessStartupService->>BusinessStartupService: isImage = message[mimetype].startsWith('image/')
else typeof message[mimetype] !== 'string'
BusinessStartupService->>BusinessStartupService: isImage = false (no crash)
end
File-Level Changes
Possibly linked issues
Tips and commands Interacting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Sorry, something went wrong.
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since mimetype is known to be string | false, consider normalizing it at the point of assignment (e.g., mapping false to '') or encapsulating the image check in a small helper to avoid scattered typeof === 'string' guards and keep usage sites simpler.
- The new inline comment is quite detailed and Chatwoot-specific; you might trim it to a shorter, implementation-focused note about mimeTypes.lookup returning false so future readers understand the guard without cross-referencing external flows.
Please address the comments from this code review:
## Overall Comments
- Since `mimetype` is known to be `string | false`, consider normalizing it at the point of assignment (e.g., mapping `false` to `''`) or encapsulating the image check in a small helper to avoid scattered `typeof === 'string'` guards and keep usage sites simpler.
- The new inline comment is quite detailed and Chatwoot-specific; you might trim it to a shorter, implementation-focused note about `mimeTypes.lookup` returning `false` so future readers understand the guard without cross-referencing external flows.Sourcery is free for open source - if you like our reviews please consider sharing them ✨ Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sorry, something went wrong.
|
Thanks for the review. On the comment: agreed, trimmed it to the mechanism and dropped the Chatwoot-specific context, since this file is the Meta channel service. On normalizing at assignment: I looked at whether the guards would be scattered, and startsWith('image/') has exactly one call site in src/, this one, so there is a single place to guard today. I did consider normalizing at the producer. The reason I did not is that processAudio() and processMedia() both use the string | false pattern and both feed this same line, so fixing only processAudio() would leave processMedia() exposed, and fixing both means touching two producers to protect one consumer. I also leaned away from mapping false to '' because an empty string asserts a known-empty type, while the real state is "unknown". That said, this is a design call for the maintainers, not for me. If you would rather have the producers hand back a clean value, say the word and I will move the normalization into processAudio() and processMedia() and drop the guard here. |
Sorry, something went wrong.
|
Thanks for the review — going through both points. On the comment: already trimmed in 7336c3b, it now just states the mechanism (mimeTypes.lookup() returns false, and optional chaining does not guard it) without the Chatwoot walkthrough. On normalizing at the assignment site: I looked at doing that instead of guarding at the use site. mimetype is assigned from mimeTypes.lookup() in four places (prepareMediaMessage and prepareAudioMessage, each for the URL and filename branches) and then flows into prepareMedia.mimetype, which is also read as contentType when uploading media. Mapping false to '' there is the deeper fix, but it changes what those other consumers receive, so it felt like a wider change than the crash this PR closes — hence the single guard at the one place that calls .startsWith(). A helper felt like indirection for one call site, but if you'd rather have the normalization at the source I'm happy to push it in this PR — just say which shape you prefer. |
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
📋 Description
mimeTypes.lookup() returns false, not undefined, when it cannot resolve a type. Chatwoot serves attachments through Rails ActiveStorage URLs that carry no file extension, so for every Chatwoot audio the lookup in processAudio() returns false and prepareMedia.mimetype is set to false.
Line 1128 of whatsapp.business.service.ts then runs:
Optional chaining only short-circuits on null and undefined. false is neither, so the call reaches false.startsWith and throws TypeError: mimetype?.startsWith is not a function before the request ever leaves for Meta.
This changes the check to an explicit type guard:
🔗 Related Issue
Closes #2678
🧪 Type of Change
🧪 Testing
I reproduced the failure without a Chatwoot instance, by running the repository's own mime-types dependency against the exact expression from line 1128:
STEP 1 - what mimeTypes.lookup() returns false Chatwoot (ActiveStorage, no extension) false Chatwoot (bare blob) "audio/ogg" normal URL with extension STEP 2 - the object processAudio() returns {"mediaType":"audio","type":"link","mimetype":false} typeof message.mimetype: boolean STEP 3 - line 1128 on develop, before the fix THROWS -> TypeError: message.mimetype?.startsWith is not a functionWith the guard in place all four cases behave, and the existing behaviour for real MIME strings is unchanged:
Gates run locally on this branch (based on the current develop tip):
I did not run npm test: the script points at ./test/all.test.ts, which does not exist on develop.
📸 Screenshots (if applicable)
N/A — backend-only change.
✅ Checklist
📝 Additional Notes
Why only audio is affected. In chatwoot.service.ts, sendAttachment() downloads the file specifically to read its Content-Type when the extension-based lookup fails:
It then uses that value to route the message, but on the audio branch it passes only the raw URL to audioWhatsapp() and the resolved MIME type is dropped. processAudio() re-derives it from the same extension-less URL and gets false.
This looks like an oversight rather than intent. The line above, in chatwoot.service.ts, already normalizes the same call with || ''. The false return value is understood elsewhere in the codebase, it is just not handled at this consumer. processAudio() and processMedia() even declare let mimetype: string | false, so the type is documented in the signature.
Why the guard is here and not in the producers. startsWith('image/') has exactly one call site in src/, this one. processMedia() (lines 1379 and 1383 on develop) uses the same string | false pattern and feeds the same line, so an image or document sent from an extension-less URL would hit the identical crash. One guard at the point of use covers every producer. Normalizing false to undefined in processAudio() alone would fix audio only.
No behaviour change on the audio path. For audio, isImage is only read inside a spread that already excludes audio, so the payload sent to Meta is byte-for-byte the same. The change removes the exception and nothing else.
Happy to move the normalization into processAudio() and processMedia() instead if you would prefer the producers to hand back a clean value, and equally happy to be told I have misread something. I am new to this codebase.