Repository navigation
Conversation
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security Review Summary
This PR introduces a significant new feature for Lark knowledge intake with PDF/DOCX file processing. I've identified 4 critical security vulnerabilities that must be addressed before merge:
Critical Issues Found
- Path Traversal Vulnerability (CWE-22): Untrusted file paths used in temporary directory operations without validation
- Command Injection Risk (CWE-78): External command execution (
pdfinfo,pdftotext,pdftoppm,tesseract) with insufficient input sanitization - Unrestricted File Upload (CWE-434): No malware scanning on user-uploaded files stored directly in database
- Crash Risk (CWE-755): XML decoder processing untrusted DOCX content without panic recovery
Security Architecture Concerns
The feature processes untrusted user files through external binaries, which creates multiple attack surfaces. While the code includes some protections (size limits, timeouts, expansion limits), the fundamental security controls are insufficient for production deployment:
- No malware scanning before storage
- External command execution without resource isolation
- Path operations without traversal prevention
- Missing panic recovery for untrusted document parsing
Recommendation
Block merge until the identified security vulnerabilities are remediated. Consider implementing: sandboxed file processing, malware scanning integration, strict path validation, and comprehensive error recovery for all untrusted input processing.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| input := filepath.Join(dir, "source.pdf") | ||
| if err = os.WriteFile(input, data, 0600); err != nil { |
There was a problem hiding this comment.
🛑 Security Vulnerability: Path traversal risk when writing untrusted PDF data to temporary directory. An attacker could craft a malicious filename in the Lark message to escape the temp directory and write files to arbitrary locations on the filesystem.1
The input path constructed at line 171-172 uses unsanitized data that originates from user-controlled Lark messages. While os.MkdirTemp creates a random directory, the fixed filename "source.pdf" provides a predictable target. More critically, the subsequent command execution at lines 175, 187, and 206-209 pass this path to external binaries (pdfinfo, pdftotext, pdftoppm, tesseract) without validation.
Add explicit path validation to ensure the constructed paths remain within the temporary directory before executing any external commands.
Footnotes
-
CWE-22: Path Traversal - https://cwe.mitre.org/data/definitions/22.html ↩
| func knowledgeCommand(ctx context.Context, name string, args ...string) ([]byte, error) { | ||
| cmd := exec.CommandContext(ctx, name, args...) |
There was a problem hiding this comment.
🛑 Security Vulnerability: Command injection risk through unsanitized external command execution. The knowledgeCommand function executes external binaries (pdfinfo, pdftotext, pdftoppm, tesseract) with paths derived from untrusted user input without proper validation.1
While Go's exec.CommandContext prevents shell injection when args are passed as separate parameters, the file paths themselves originate from user-controlled Lark message data. An attacker could exploit this through: (1) Path traversal in the temp directory reference, (2) Symbolic link attacks if the temp directory is predictable, or (3) Resource exhaustion by uploading specially crafted PDFs that cause these external tools to hang or consume excessive resources.
The 60-second timeout provides some protection against hangs, but does not prevent resource exhaustion within that window. Add validation to ensure all paths stay within the expected temporary directory, and consider adding resource limits (memory, CPU) to the external process execution.
Footnotes
-
CWE-78: OS Command Injection - https://cwe.mitre.org/data/definitions/78.html ↩
| } | ||
| digest := sha256.Sum256(data) | ||
| j.State.Digest = hex.EncodeToString(digest[:]) | ||
| err = g.pool.QueryRow(ctx, `INSERT INTO lark_knowledge_file(installation_id,chat_id,digest,filename,original) VALUES($1,$2,$3,$4,$5) ON CONFLICT(installation_id,chat_id,digest) DO UPDATE SET digest=EXCLUDED.digest RETURNING id`, g.policy.InstallationID, g.policy.ChatID, j.State.Digest, knowledgeFilename(file.Name), data).Scan(&j.State.FileID) |
There was a problem hiding this comment.
🛑 Security Vulnerability: Untrusted file data stored directly in database without malware scanning. Files up to 20 MiB from Lark users are stored in the lark_knowledge_file.original column without any malware or content validation beyond format checks.1
An attacker could upload malicious payloads disguised as PDF/DOCX files. While the extraction logic has some ZIP bomb protection for DOCX files, it does not scan for: (1) Embedded malware in PDF JavaScript/attachments, (2) Exploit payloads targeting PDF reader vulnerabilities, or (3) Malicious macros or embedded objects in Office documents.
Consider integrating malware scanning before storing files, or at minimum add content security policy checks and sandboxing for file processing operations.
Footnotes
-
CWE-434: Unrestricted Upload of File with Dangerous Type - https://cwe.mitre.org/data/definitions/434.html ↩
| decoder := xml.NewDecoder(bytes.NewReader(raw)) | ||
| var out strings.Builder | ||
| paragraph := 0 | ||
| inText := false | ||
| readable := 0 | ||
| const wordNS = "http://schemas.openxmlformats.org/wordprocessingml/2006/main" | ||
| for { | ||
| if err := ctx.Err(); err != nil { | ||
| return "", err | ||
| } | ||
| tok, err := decoder.Token() | ||
| if errors.Is(err, io.EOF) { | ||
| break | ||
| } | ||
| if err != nil { | ||
| return "", permanentKnowledgeError("invalid DOCX XML; original retained") |
There was a problem hiding this comment.
🛑 Crash Risk: Missing error handling for XML decoder allows malformed DOCX files to panic the server. The xml.NewDecoder at line 77 processes untrusted ZIP archive content without comprehensive error recovery.1
While the code checks for io.EOF and basic XML errors, deeply nested or malformed XML structures could trigger decoder panics that aren't caught. The Go XML decoder has known issues with certain malformed inputs that can cause runtime panics rather than returning errors.
Wrap the entire XML parsing section (lines 83-122) in a recover block to catch and convert any decoder panics into permanentKnowledgeError results.
Footnotes
-
CWE-755: Improper Handling of Exceptional Conditions - https://cwe.mitre.org/data/definitions/755.html ↩
1 participant
What does this PR do?
Adds a disabled-by-default, single-group Lark knowledge pilot for the CTO search. Ordinary group messages currently stop at the private agent's mention gate, leaving file intake dependent on a laptop monitor. This extends the existing authenticated persistent connection with durable capture before that gate, a resumable file pipeline and a separate source-only group Q&A route.
The first release handles human messages/thread replies and uploaded PDF/DOCX files. It preserves private candidate assessments and human hiring decisions. Linked-document/wiki discovery remains outside this release.
Related Issue
Implements the reviewed Tarley event-driven intake plan. Operator boundaries and activation steps are recorded in
docs/runbooks/lark-group-knowledge.md.Type of Change
Changes Made
Intent and Risks
captureoverlaps the existing monitor without model calls, candidate writes or knowledge replies.processneeds a coordinated single-writer cutover. The policy remains blank by default. No deployment, production subscriptions/scopes, heartbeat settings or real group/candidate messages were changed.Activation still requires the dedicated app's approved all-group-message permission, verified history/resource/member access, an authorized model/cost boundary and a genuine upload/Q&A/outage-recovery check. Local fixtures are not live activation proof. The daily model limit counts invocations, not dollars or SDK retry requests. Reconciliation is bounded; incomplete passes retain the watermark and expose failure. Operational rollback returns to capture mode and retains evidence; destructive down migrations are not the rollback procedure.
How to Test
Validated in the managed isolated PostgreSQL environment with synthetic inputs:
make env-exec ARGS='-- env GOFLAGS=-p=2 GOMAXPROCS=2 bash scripts/test-go.sh --only regular'.make env-exec ARGS='-- bash scripts/test-go.sh --only agent'(no real-agent smoke).git diff --check.The initial broad parallel run exposed daemon timeout/process-tree cleanup failures; a bounded-concurrency regular run passed. A later full run passed all packages except two media-reconciler tests while the local API shared the fixture database; after stopping the API, the service package passed in isolation. The runbook now requires stopping background workers before database-backed suites. No frontend/mobile changes, live model calls, real candidate files or production messaging tests.
Checklist
apps/docs/content/docs/developers/conventions.zh.mdxNo UI changes; screenshots are not applicable. No new runtime, coding tool or UI tab.
AI Disclosure
AI tool used: Codex, with sequential CE specialist and local adversarial reviews. The optional Claude pass could not start because its acpx dependency was unavailable; no code was sent to that provider.
Prompt / approach: Implement the reviewed, narrowly scoped intake plan in an isolated worktree; verify durable recovery, native-thread routing, source-only group authority and private candidate preservation with synthetic inputs. Prepare a ready-for-review PR only; do not merge or deploy.