Skip to content

fix: address security audit findings (ReDoS, prompt injection, supply chain, error leakage)#54

Open
AilfredBitworth wants to merge 1 commit into
roychri:mainfrom
AilfredBitworth:fix/security-audit-m1-m2-l1-l2
Open

fix: address security audit findings (ReDoS, prompt injection, supply chain, error leakage)#54
AilfredBitworth wants to merge 1 commit into
roychri:mainfrom
AilfredBitworth:fix/security-audit-m1-m2-l1-l2

Conversation

@AilfredBitworth

Copy link
Copy Markdown

Summary

Security audit of the codebase identified four issues. This PR fixes all of them.

  • M1 — ReDoS (src/asana-client-wrapper.ts:60): namePattern was passed directly to the RegExp constructor without escaping. A crafted pattern (e.g. (a+)+$) could cause catastrophic backtracking and peg the Node.js event loop. Fixed by escaping all regex metacharacters before constructing the pattern.

  • M2 — Indirect prompt injection (src/prompt-handler.ts): Task name, notes, custom field values, and story text were interpolated directly into the LLM message content for the task-summary prompt, with no trust-boundary markers. An actor with write access to any Asana task in the workspace could inject arbitrary instructions into the consuming model's context. Fixed by wrapping all inlined Asana data in explicit --- BEGIN/END ASANA DATA --- delimiters with an instruction to treat the content as data.

  • L1 — Floating production dependency versions (package.json, .npmrc): All three production dependencies used ^ caret ranges, meaning a supply-chain-compromised minor/patch release would be adopted automatically on npm install. Fixed by pinning to exact versions (matching the current package-lock.json) and adding .npmrc with save-exact=true so future dependency additions are pinned by default.

  • L2 — Raw API error details in LLM context (src/tool-handler.ts): The catch-all error handler returned error.message verbatim to the calling model. Asana API errors frequently include workspace GIDs and field names in their messages. Fixed by returning only the HTTP status code for Asana API errors; the full error is still written to stderr for debugging.

Test plan

  • npm run build succeeds with no TypeScript errors
  • asana_search_projects with a regex metacharacter pattern (e.g. name_pattern: "(a+)+$") returns an empty list rather than hanging
  • task-summary prompt output contains --- BEGIN ASANA DATA --- / --- END ASANA DATA --- delimiters around task content
  • Triggering a 404 from the Asana API returns "Asana API error (status 404): request could not be completed" rather than the raw Asana error body

🤖 Generated with Claude Code

@roychri

roychri commented Jun 3, 2026

Copy link
Copy Markdown
Owner

Thanks for taking the time to put this together. It seems like a few of these point at real surfaces. I went through each finding against the code, and I think it's worth discussing before merging, because at least one of the fixes looks like it might change documented behavior and a couple seem to rest on premises that may not hold for this repo.

M1 - ReDoS (searchProjects)
I agree the unescaped new RegExp(namePattern, 'i') can backtrack. But two things give me pause:

  1. It seems like name_pattern is documented as a regular expression in both the tool schema (src/tools/project-tools.ts:15) and the README (README.md:48). I think escaping all metacharacters effectively turns it into a literal substring match, which would silently break any caller relying on a real pattern (^Vi, proj.*2026, foo|bar). That feels more like a functional regression than just hardening.
  2. As far as I can tell, the input is supplied by the MCP caller operating on their own Asana account, so this seems closer to self-inflicted DoS than a cross-trust-boundary attack.

If we did want to bound backtracking while keeping the documented regex feature, I think capping input length and/or wrapping the match in a timeout (or something like re2) might be a better fit than changing the semantics. Would you be open to reworking M1 along those lines?

M2 - Prompt injection (task-summary)
This seems like a legitimate surface. Task content does get interpolated into the prompt. The delimiters feel reasonable as defense-in-depth, though I think it's worth noting that BEGIN/END markers can probably be bypassed (injected content could emit its own --- END ASANA DATA ---), so it may not be a hard boundary. Happy to reconsider merging this piece on its own once M1 is sorted out.

L1 - Dependency pinning
I think because package-lock.json is committed, npm install already resolves to pinned versions, so I'm not sure a compromised minor/patch would be pulled in automatically on install. Exact-pinning in package.json plus .npmrc save-exact=true seems like a reasonable policy preference, but it doesn't look like it's closing a vulnerability here, and it might make routine patch bumps noisier. I'd lean toward deciding this as a project convention rather than folding it into a security fix.

L2 - Error message sanitization
It seems like the error goes to the calling model, which already holds the Asana token and operates on that account, so I'm not sure Asana GIDs/field names are really disclosed across a trust boundary here. And I think dropping error.message removes the detail ("task not found", "invalid field X") that helps the model self-correct. I'd lean toward keeping the message for debuggability unless there's a concrete leak scenario I'm missing.

Net: I think M2 could potentially stand on its own as incremental hardening, M1 seems like it may need a different approach since the current fix looks like it regresses documented behavior, and L1/L2 I'd probably hold unless we decide we want them as explicit project policy. Would it be possible to split these so the less contentious parts could be considered independently? Appreciate the contribution.

M1 - ReDoS (searchProjects): Replace literal-escape approach with the re2
library, which provides a drop-in safe regex engine guaranteed to run in
linear time. This preserves the documented regex semantics of name_pattern
(callers can still pass patterns like "^Vi" or "proj.*2026") while
eliminating catastrophic backtracking. Adds re2 as a production dependency
and marks it external in the esbuild config.

M2 - Prompt injection (task-summary): Wrap untrusted Asana content in
BEGIN/END ASANA DATA delimiters to create a trust boundary, and add a
sanitizeDataContent() helper that strips any occurrence of those delimiter
sequences from task names, notes, custom fields, and story texts before
interpolation. This closes the bypass where crafted task content could emit
its own END marker to escape the data block.

L1/L2 reverted: Accepted maintainer feedback — package-lock.json already
provides equivalent pinning coverage, and error messages are useful for
model self-correction given the single-trust-boundary context.
@AilfredBitworth AilfredBitworth force-pushed the fix/security-audit-m1-m2-l1-l2 branch from a1bbb4a to 462e1a5 Compare June 4, 2026 19:53
@AilfredBitworth

Copy link
Copy Markdown
Author

Thanks for the thorough review — these are all fair points, and I appreciate you taking the time to work through each finding rather than just bouncing the PR.

M1 (ReDoS): You're right that the literal-escape approach was a functional regression. I missed that name_pattern is documented as a regex in both the tool schema and the README — escaping metacharacters silently turns it into a substring match and breaks any caller relying on real patterns. I've replaced the fix with the re2 library, which is a drop-in Node.js binding to Google's RE2 engine. It guarantees linear-time execution regardless of the pattern, so regex semantics are fully preserved (^Vi, proj.*2026, foo|bar all still work) while catastrophic backtracking is structurally impossible. re2 is marked external in the esbuild config so the native .node file doesn't get bundled.

M2 (Prompt injection): Good catch on the bypass — you're absolutely right that an injected --- END ASANA DATA --- in task content would prematurely close the block. I've added a sanitizeDataContent() helper that strips any occurrence of the delimiter sequences from task names, notes, custom field names and values, and story texts before they're interpolated. The delimiters are now a closed boundary. Happy to keep this in the same PR as M1 or split it out, whichever you prefer.

L1 (Dependency pinning): Makes sense — package-lock.json already provides the equivalent coverage on npm install, so this is a project-convention decision rather than a security fix. I've reverted those changes.

L2 (Error sanitization): Accepted your trust-boundary argument. The error message goes to the model that already holds the token, and the detail genuinely helps with self-correction. Reverted.

The updated commit keeps only M1 (re2) and M2 (hardened delimiters). Let me know if you'd like M2 split into a separate PR or if there's anything else you'd like adjusted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants