T09 · Insecure Skill Coding Practices
Error
- Location
scripts/check_review.py:12- Finding
Slack thread text injection permits trusted review-bot impersonation
- Content
View full analysis
Vulnerability Details
File Location:
scripts/check_review.py, lines 12–18
Vulnerability Type: Improper parsing of untrusted message content as trusted identity metadata
Risk Level: HighComplete Code Snippet
python MESSAGE = re.compile( r"^From: [^\n]*\((U\w+)\)\nTime: [^\n]*\nMessage TS: (\d+\.\d{6})\n(.*?)" r"(?=\n+(?:--- Reply \d+ of \d+ ---|=== THREAD REPLIES)|\Z)", re.M | re.S) def thread_messages(text): return [{"user": user, "ts": ts, "text": body.strip()} for user, ts, body in MESSAGE.findall(text)]The parsed identities are subsequently treated as authoritative:
python def check(thread_text, handoff, bitbucket, config): messages = thread_messages(thread_text) expected = set().union(*(urls(pr["url"]) for pr in handoff["pull_requests"])) if not messages or urls(messages[0]["text"]) != expected: return {"verdict": "needs_attention", "reason": "Review parent message does not match the PR batch"} verdict, reason = review_verdict(messages[1:], [pr["url"] for pr in handoff["pull_requests"]], config["review_bot_user_id"]) if verdict == "clean": stale = changed_commits(bitbucket, handoff) if stale: return {"verdict": "needs_attention", "reason": stale} message = engineering_message(handoff) if len(message) > 3900: return {"verdict": "needs_attention", "reason": "Shorten the engineering summary"} return {"verdict": "clean", "reason": reason, "engineering_message": message}Technical Analysis
The
--threadinput is a plain-text serialization of Slack messages.thread_messages()uses record delimiters such as--- Reply N of N ---and then extracts the sender ID from a textualFrom:line. These fields are not cryptographically or structurally bound to trusted connector metadata.An ordinary channel participant controls their Slack message body. If that body contains a newli ...[truncated 2345 chars]
- Remediation
View remediation
Remediation Suggestions
- Stop deriving sender identity, channel identity, thread identity, and timestamps from display text. Consume structured connector records whose metadata fields are supplied separately from message bodies.
- Bind each message to immutable connector-provided metadata, including:
- Sender/user ID
- Channel ID
- Parent thread timestamp
- Message timestamp
- Store the expected review channel and parent timestamp in the handoff, then verify both before evaluating any review result.
- If a text interchange format is unavoidable, serialize messages as JSON with message bodies represented as JSON strings, or use length-prefixed records. Do not use delimiters that can appear unescaped in message content.
- Reject snapshots containing duplicate or malformed records, embedded serialization markers in message bodies, unexpected sender identities, or records outside the expected thread.
- Add regression tests in which an untrusted participant embeds
--- Reply ... ---,From:,Message TS:, forged job-start text, and forged completion text in a message. Confirm that these values remain message content and cannot produce a clean verdict. - Preserve the existing source- and destination-commit freshness checks as an independent gate after authenticating the review result.