fix: resolve issue #19 security gaps and logging discrepancies#154
fix: resolve issue #19 security gaps and logging discrepancies#154stakeswky wants to merge 1 commit intoTinyAGI:mainfrom
Conversation
|
@greptileai review this pr |
Greptile SummaryThis PR addresses two concrete security gaps from issue #19: it replaces plaintext message content snippets in log calls with a privacy-safe Key issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant C as Channel Client<br/>(Discord/Telegram/WhatsApp)
participant P as privacy.ts<br/>messageSizeSummary()
participant L as logging.ts<br/>log() / emitEvent()
participant Q as queue-processor.ts
Note over C: Incoming message received
C->>P: messageSizeSummary(messageText)
P-->>C: "42 characters"
C->>L: log(INFO, "Message from X: 42 characters")
C->>Q: POST /api/message (full message body)
Note over Q: Queue processing
Q->>P: messageSizeSummary(rawMessage)
P-->>Q: "42 characters"
Q->>L: log(INFO, "Processing [...]: 42 characters")
Q->>L: emitEvent('message_received',<br/>{ message: rawMessage.substring(0,120) })
Note over L: ⚠️ Raw content still in event payload
Last reviewed commit: 127af2c |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| log('INFO', `Processing [${isInternal ? 'internal' : channel}] ${isInternal ? `@${dbMsg.from_agent}→@${dbMsg.agent}` : `from ${sender}`}: ${rawMessage.substring(0, 50)}...`); | ||
| log('INFO', `Processing [${isInternal ? 'internal' : channel}] ${isInternal ? `@${dbMsg.from_agent}→@${dbMsg.agent}` : `from ${sender}`}: ${messageSizeSummary(rawMessage)}`); | ||
| if (!isInternal) { | ||
| emitEvent('message_received', { channel, sender, message: rawMessage.substring(0, 120), messageId }); |
There was a problem hiding this comment.
emitEvent still passes raw message content
The log() call on the line above was correctly updated to use messageSizeSummary, but the adjacent emitEvent call still passes rawMessage.substring(0, 120) — a literal plaintext snippet of the message body — as part of its payload.
Since emitEvent is imported from ./lib/logging alongside log, any downstream consumer of message_received events (websocket clients, event log files, monitoring hooks, etc.) will still receive unredacted message content. This directly contradicts the PR's goal of eliminating plaintext message content snippets from the system.
| emitEvent('message_received', { channel, sender, message: rawMessage.substring(0, 120), messageId }); | |
| emitEvent('message_received', { channel, sender, message: messageSizeSummary(rawMessage), messageId }); |
| if (!disableSandbox) { | ||
| return baseArgs; | ||
| } |
There was a problem hiding this comment.
Sandbox-enabled default will break Chromium in Docker without opt-in
Returning baseArgs (no sandbox flags) when disableSandbox === false means Chromium launches with its kernel-level sandbox fully active. This is the correct security posture on bare-metal hosts, but it is a breaking change for the most common production deployment scenario — Docker containers.
Docker containers do not have the SYS_ADMIN capability or a permissive seccomp profile by default, both of which are required for Chromium's namespace sandbox to initialize. Without them, Puppeteer will fail to launch Chromium entirely, typically with an error like:
Failed to launch the browser process!
[...] FATAL:zygote_host_impl_linux.cc] No usable sandbox!
Previously, --no-sandbox was always included, so the WhatsApp client worked out of the box in Docker. After this change, existing Docker-based deployments will fail to start until operators explicitly set WHATSAPP_DISABLE_SANDBOX=true, which is not called out as a migration step anywhere in the PR or README.
Consider documenting the Docker requirement prominently (e.g. in the README or startup warning log), or detecting the environment at runtime and emitting a clearer error when the sandbox fails to initialize.
| export function messageSizeSummary(text: string): string { | ||
| const length = (text || '').length; |
There was a problem hiding this comment.
TypeScript signature doesn't match the defensive implementation
The function is typed as accepting string, but the body immediately applies (text || ''), which only has effect when text is null, undefined, or an empty string. Since TypeScript already guarantees a string at the call site, the || '' guard is unreachable under strict type-checking — yet it signals the author's expectation that callers may actually pass null/undefined.
Either tighten the type to match the guard:
| export function messageSizeSummary(text: string): string { | |
| const length = (text || '').length; | |
| export function messageSizeSummary(text: string | null | undefined): string { | |
| const length = (text || '').length; |
Or drop the defensive guard if callers are already typed as string. Additionally, the test suite does not cover a null/undefined input — if the broader intent is to be safe against those, a test case should be added to privacy.test.ts.
2 participants
Summary
Fixes key security and discrepancy items in #19:
WHATSAPP_DISABLE_SANDBOX=true) with warning logDetails
1) Sensitive message logging
Previous code logged
message.substring(0, 50)from user content in multiple places.This change introduces
messageSizeSummary()and logs only message length metadata, e.g.42 characters, avoiding plaintext snippets at rest in log files.Touched files:
src/queue-processor.tssrc/channels/discord-client.tssrc/channels/telegram-client.tssrc/channels/whatsapp-client.tssrc/lib/privacy.ts(new)2) WhatsApp Chromium sandbox
Previous launch args always disabled sandbox (
--no-sandbox,--disable-setuid-sandbox).Now sandbox remains enabled by default. Sandbox disable is opt-in via
WHATSAPP_DISABLE_SANDBOX=truefor environments that require it.Touched files:
src/channels/whatsapp-client.tssrc/lib/whatsapp-launch.ts(new)3) Tests
Added Node built-in test coverage:
src/test/privacy.test.tssrc/test/whatsapp-launch.test.tspackage.jsontest script:npm run build:main && node --test dist/test/**/*.test.jsValidation
npm run build:main✅npm run test✅ (4 tests passing)Notes
Issue #19 also discusses command injection and allowlist concerns.
On current
main, invocation already usesspawn(command, args)(no shell interpolation), and pairing-based sender gating is present in all channel clients. This PR focuses on the remaining concrete security/privacy deltas from that report (message-at-rest leakage and sandbox defaults).