Skip to content

fix(security): close ReDoS, zip-bomb, SSRF and redaction gaps found by audit - #6013

Closed
waleedlatif1 wants to merge 2 commits into
stagingfrom
dep-audit-findings
Closed

fix(security): close ReDoS, zip-bomb, SSRF and redaction gaps found by audit#6013
waleedlatif1 wants to merge 2 commits into
stagingfrom
dep-audit-findings

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

A dependency re-audit asked "does this library actually deliver the guarantee the calling code assumes" rather than "is it popular". These are the results, after five rounds of independent adversarial review — each of which found real defects, several in the fixes themselves.

Security

ReDoS in copilot VFS glob(). A caller-controlled glob pattern went to micromatch.isMatch, whose picomatch backend compiles * to [^/]*? on the backtracking engine. Measured: *a×12 + b (25 chars) against a 48-char path took 41 seconds; micromatch on the worst legal 32-wildcard pattern did not finish a 40-char path in 90 seconds. Matching now runs on RE2 — 0.007 ms for the same input — which is what the grep() sibling in the same file already did.

This required replacing the matching engine, not just adding caps: micromatch.makeRe() emits lookaheads RE2 cannot express, so picomatch's generated source is translated (segment markers turn dot:false assertions into character exclusions; surrogate halves are remapped so RE2's code-point matching mirrors picomatch's UTF-16 semantics). Equivalence is pinned by a differential test of 6,871,636 pattern/path comparisons against micromatch.isMatch, 0 mismatches. Patterns the translator cannot represent fail closed (no matches + warn) and never fall back to the backtracking engine. Caps: 1000 chars / 32 wildcards, which throw.

Zip-bomb guard: two bypasses closed, one false positive fixed. doc-parser was the only OOXML-adjacent parser without assertOoxmlArchiveWithinLimits, and officeparser sniffs content rather than extension — so a docx renamed .doc reached an unguarded unzip. Fixing that exposed two more:

  • Appended EOCD. The guard trusted one EOCD record. yauzl, JSZip and SheetJS each select a different one (JSZip even rebases its reader on a cdOffset mismatch), so an appended empty EOCD made the guard measure ~0 while a parser inflated the real directory. It now enumerates every candidate, rejects any whose declared entry count disagrees with the records actually present, and takes the worst case across all of them.
  • Prepended bytes. Prepending one byte shifts the central directory while EOCD offsets stay absolute, so no candidate resolved and the fail-closed branch — which keyed on byte 0 — read the file as "not a ZIP". A 64 MiB bomb passed while JSZip inflated it (+164 MB RSS). It now fails closed whenever an EOCD signature is present but unresolvable.
  • False positive. The original locator required the EOCD at the exact tail, so any trailing byte (appended signatures, padding) turned a valid document into a ZipBombError.

Unguarded yaml.load. json-yaml-chunker parsed with no alias-expansion cap and js-yaml has no maxAliasCount. Both call sites now route through assertYamlWithinLimits.

SSRF coverage gaps. Video tools and falai fetched provider-supplied URLs raw, including one replaying Authorization: Key <apiKey> to a polled status_url every 5s for up to 90 minutes. Those are now origin-pinned and revalidated per poll. Cross-origin redirects drop credential headers by default, keyed on registrable domain rather than exact origin — ~97 call sites were enumerated first, and origin equality would have broken Zoom, self-hosted GitLab/Grafana/ClickHouse, and Slack (files.slack.comfiles-origin.slack.com is same-site and genuinely needs the bearer).

Secrets were reaching logs. Redaction key patterns were fully anchored (^api[_-]?key$), so openai_api_key, x-api-key, set-cookie, secretAccessKey, session_id and others passed through unredacted on the persisted execution-log path. Now caught via key normalization plus targeted patterns. Value-shaped credentials (Tailscale tskey-…, JWTs, signed-URL query tokens) are also caught, closing two leaks a key-name matcher structurally cannot reach.

ffmpeg filter-option injection. escapeDrawtext did not match ffmpeg's quoting grammar. Reproduced against the real binary: a':x=90:fontcolor=red caused ffmpeg to parse fontcolor as a genuine option. Text now goes through textfile=, so it never enters the filter string. A per-character backslash matrix against ffmpeg 8.0 established double-escaping as the unique correct level for the remaining path argument.

Loop-condition injection. A resolved value (API/webhook/LLM output) was interpolated into evaluated JS by hand; a " escaped the literal. Now JSON.stringify.

Supply chain

  • Vendored free-email-domains' data. Its postinstall fetched a CDN CSV and overwrote the shipped domains.json, so the lockfile hash covered the tarball but not the data actually used — inert under Bun only because the package sits outside trustedDependencies. Content verified byte-identical (4,779 entries, exact set match).
  • Documented the xlsx CDN pin — npm's copy is frozen at 0.18.5 with CVE-2023-30533 and CVE-2024-22363 unfixed, and a URL dependency is invisible to Dependabot.

Dependencies

@daytonaio/sdk@daytona/sdk (deprecated; official rename). @react-email/components 0.5.7 → 1.0.12 (0.x line deprecated; zero source edits — no template uses <Tailwind>). Lazy-load the pptx preview so echarts (~330 KB gzip) leaves the Home, Files and share-page bundles. Drop autoprefixer (absent from the PostCSS config, so it never ran) and tailwind-merge (no importers; @sim/emcn declares its own).

Type of Change

  • Bug fix
  • Security

Testing

Type-check 19/19, lint 19/19, check:api-validation, check:boundaries, bun install --frozen-lockfile all clean. Full suite 15,299 passing, with one failure (cloud-review-tools.test.ts) that needs a real rg binary on PATH and fails identically on an untouched checkout.

Verification worth noting: the glob equivalence grid (6.87M comparisons); zip attacks proven by inflating the payload with JSZip inside the test rather than asserting on a shape; ffmpeg escaping validated against the real binary; fal.ai queue URL shapes confirmed by live probes plus both official clients; free-email-domains set-compared in both directions.

Behavior changes reviewers should weigh

  • Redirect credential-stripping is a default change across every integration, not just the routes touched here — it flows through the generic tool HTTP path. Two flows change on the wire: Veo loses x-goog-api-key on www.googleapis.comstorage.googleapis.com, and Twilio no longer forwards Basic to media.twiliocdn.com. Neither breaks — both targets are pre-signed, and forwarding Basic to Twilio's legacy S3 path returned 400 Unsupported Authorization Type, so that one is a latent-bug fix. The Veo case is reasoned from redirect shape and the GitHub/S3 precedent, not observed — it wants a live smoke test.
  • Redaction footprint is unchanged in size (20 keys over 5,134 tool-output identifiers, same as before) but shifted: 8 newly redacted (incl. sessionId, deliberately — a Browserbase session id is a live bearer handle — and resetPasswordUrl), 8 newly kept (pagination cursors, boolean flags).
  • ~5.8% of degenerate glob patterns fail closed (unbalanced parens, POSIX classes behind a star). All realistic patterns compile; a test pins that.
  • @daytona/[email protected] opens a Socket.IO connection in its constructor. No leak — it falls back to polling — but it is new outbound WSS egress, costing a 5s timeout per sandbox create behind an HTTPS-only policy. Opt-out: useDeprecatedPolling: true.

Known gaps, deliberately not closed

  • tools/vercel/get_env_vars.ts returns decrypted env values under a key literally named value — uncatchable by key-name or value-shape matching; needs a per-tool sensitiveOutputs declaration.
  • The credential-header deny-list matches -key/-token/-secret/-password/-signature but not underscore forms or -sig/-hmac. data-drains/destinations/webhook.ts lets a user name their own signature header, so a custom name ships the HMAC to a redirect target.
  • allowPrivateDomains gives tenant isolation only for providers on the PSL private section; atlassian.net, sharepoint.com, okta.com, auth0.com still resolve tenants to one registrable domain. Documented in the TSDoc.
  • Slack download paths should set maxRedirects: 0 and reject text/html (pre-existing).
  • yaml.load rejects multi-document --- bundles, so k8s manifests degrade to text chunking (pre-existing).
  • @react-email/components is deprecated across all 135 versions including 1.0.12 — not a regression, but the bump buys less than it appears to.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@waleedlatif1
waleedlatif1 requested a review from a team as a code owner July 28, 2026 19:08
@vercel

vercel Bot commented Jul 28, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
docs Ready Ready Preview, Comment Jul 28, 2026 11:47pm

Request Review

@cursor

cursor Bot commented Jul 28, 2026

Copy link
Copy Markdown

PR Summary

High Risk
Touches authentication-adjacent redirect header policy, SSRF-guarded outbound fetches for all video providers, and copilot glob semantics—behavior changes that can break integrations or widen/narrow file matching if wrong.

Overview
Closes multiple audit findings across SSRF, ReDoS, zip/YAML bombs, credential leakage, and injection paths.

Copilot VFS glob no longer uses backtracking micromatch.isMatch; patterns compile through a picomatch→RE2 translator (with length/wildcard caps), and unrepresentable patterns fail closed. Grep scopes share the same matcher; GlobPatternError is surfaced from vfs tools like glob.

Video generation downloads provider URLs and Fal.ai queue polls through DNS-validated, IP-pinned fetches, with queue URL normalization/stripping of invalid /response paths, response size caps, and Veo API keys only on genuine https *.googleapis.com hosts.

OOXML zip guard is extended (doc parser, document-style extraction) with stricter EOCD handling (prepended bytes, decoys, ZIP64); YAML chunking uses shared alias-expansion limits. Loop while-conditions stringify string outputs with JSON.stringify instead of manual quoting.

Secure fetch redirects drop credential-bearing headers on cross-site hops (eTLD+1 + private-suffix rules) while keeping them on same-site provider hops; stripAuthOnRedirect still strips Authorization on same-site redirects.

Log redaction broadens key normalization, value-shaped secrets (JWTs, tskey-…, signed URL params), credential-container recursion, and string redaction in redactApiKeys / sanitizeEventData.

File viewer: PowerPoint preview is lazy-loaded with PreviewErrorBoundary recovery (reload vs try again). Daytona import moves to @daytona/sdk. Minor docs on xlsx CDN pin and OneDrive upload route.

Large test additions cover glob equivalence, zip attacks, redirects, video providers, preview boundaries, and redaction regressions.

Reviewed by Cursor Bugbot for commit 7567216. Configure here.

@greptile-apps

greptile-apps Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Closes multiple security gaps from a dependency re-audit (ReDoS, zip-bomb, SSRF, redaction, YAML, ffmpeg/loop injection) and cleans related supply-chain/deps.

  • Copilot VFS glob() matching moved to RE2 with pattern caps and fail-closed translation; large micromatch equivalence tests added.
  • OOXML zip-bomb guard hardened (multi-EOCD, prepend/append cases); applied to doc-parser and related paths; YAML chunking routed through size/alias limits.
  • Video/fal.ai fetches origin-pinned with per-poll revalidation; generic redirect credential stripping on registrable domain; redaction patterns broadened for keys and value shapes.
  • ffmpeg drawtext via textfile=; loop conditions use JSON.stringify; free-email domains vendored; dependency renames/cleanup and lazy pptx preview.

Confidence Score: 5/5

This follow-up pass finds no remaining blocking failures eligible to report against prior threads, so the PR appears safe to merge from that scope.

No blocking failure remains in the scoring set for this follow-up review (no prior Greptile inline threads were available to re-evaluate, and no new follow-up-eligible comments were accepted).

Important Files Changed

Filename Overview
apps/sim/lib/copilot/vfs/operations.ts Replaces micromatch-backed glob matching with RE2 translation, caps, and fail-closed unsupported patterns.
apps/sim/lib/file-parsers/zip-guard.ts Hardens ZIP EOCD/central-directory limits against multi-EOCD and prepended-byte zip-bomb bypasses.
apps/sim/lib/core/security/redaction.ts Broadens key normalization and value-shape redaction for credentials in logs/events.
apps/sim/lib/media/falai.ts Pins fal queue/status URLs and revalidates origins during polling.
apps/sim/app/api/tools/video/route.ts Applies secure fetch/origin pinning for provider video URLs.
apps/sim/lib/media/ffmpeg.ts Moves drawtext content to textfile and tightens path escaping for filter args.
apps/sim/executor/orchestrators/loop.ts Serializes loop condition values with JSON.stringify to block quote injection.
apps/sim/lib/chunkers/json-yaml-chunker.ts Routes YAML parse through assertYamlWithinLimits at both call sites.

Reviews (10): Last reviewed commit: "fix(security): close glob translation fa..." | Re-trigger Greptile

@gitguardian

gitguardian Bot commented Jul 28, 2026

Copy link
Copy Markdown

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/lib/core/security/redaction.ts Outdated
Comment thread apps/sim/app/api/tools/video/route.ts Outdated
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

2 issues from previous reviews remain unresolved.

Fix All in Cursor

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 93dd4f1. Configure here.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/lib/file-parsers/zip-guard.ts Outdated
Comment thread apps/sim/app/api/tools/video/route.ts
Comment thread apps/sim/lib/media/falai.ts
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

1 issue from previous review remains unresolved.

Fix All in Cursor

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 064d1c6. Configure here.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit e545299. Configure here.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 5cc09b3. Configure here.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit afd303c. Configure here.

Waleed Latif and others added 2 commits July 28, 2026 16:38
…y audit

A dependency re-audit asked "does this library actually deliver the guarantee
the calling code assumes" rather than "is it popular". These are the results.

Security:
- Copilot VFS glob() handed a caller-controlled pattern to micromatch, whose
  picomatch backend compiles `*` into a backtracking regex. A 25-char pattern
  took 41s; 29 chars exceeded 180s. Matching now runs on RE2, mirroring the
  grep() sibling in the same file that already did this. micromatch.makeRe()
  emits lookaheads RE2 cannot express, so picomatch's dot-assertions are
  translated into character exclusions; a differential test covers 519,688
  pattern/path pairs against micromatch with no mismatches. Patterns that
  still carry an assertion fail closed rather than falling back.
- doc-parser was the only OOXML-adjacent parser without the zip-bomb guard.
  officeparser sniffs content rather than extension, so a docx renamed .doc
  reached an unguarded unzip and could OOM the shared server. Genuine OLE2
  .doc files are unaffected. Same guard added to the copilot style reader.
- json-yaml-chunker called yaml.load with no alias-expansion cap. js-yaml has
  no maxAliasCount, so this is routed through assertYamlWithinLimits like the
  file parser already was.
- Video tools and falai fetched provider-supplied URLs raw, including one that
  replayed an Authorization header to a polled status_url. Those URLs are now
  origin-pinned and revalidated on every poll. Cross-origin redirects drop
  credential headers by default, keyed on registrable domain rather than exact
  origin so same-site subdomain hops and http->https upgrades keep working.
- Redaction key patterns were fully anchored, so openai_api_key, x-api-key,
  set-cookie, secretAccessKey and session_id were logged in the clear. The
  matcher now tokenizes keys, with exemptions so token-usage fields such as
  promptTokens stay readable.
- drawtext escaping did not match ffmpeg's quoting grammar; a quote in agent
  supplied text escaped into filter options. Text is passed via textfile= so
  it never enters the filter string.
- Loop conditions interpolated a resolved value into a JS string by hand.

Supply chain:
- Vendor free-email-domains' data. Its postinstall fetched a CDN CSV and
  overwrote the shipped list, so the lockfile hash covered the tarball but not
  the data actually used. It is inert under Bun today only because the package
  is outside trustedDependencies.
- Document why xlsx is pinned to the SheetJS CDN: the npm copy is frozen at
  0.18.5 with two unfixed CVEs, and a URL dependency is invisible to audit
  tooling, so upgrades have to be tracked by hand.

Dependencies:
- @daytonaio/sdk is deprecated in favour of @daytona/sdk (same API).
- The @react-email/components 0.x line is deprecated; 1.0.12 needs no source
  changes here since no template uses <Tailwind> and render() already moved.
- Load the pptx preview lazily so echarts leaves the Home, Files and share
  page bundles.
- Drop autoprefixer (absent from the PostCSS config, so it never ran) and
  tailwind-merge (no importers; @sim/emcn declares its own).
Glob matcher (RE2 translation layer):
- reject picomatch escape passthrough (\A, \z, \p{L}) that RE2 reads as
  anchors and Unicode classes, bypassing the dot:false guarantee
- reject character-class ranges straddling the private-use segment markers
- restore negated (!) glob semantics instead of silently matching nothing
- parse zero-padded repeat bounds numerically

Zip guard:
- scan the whole buffer for EOCD candidates, matching JSZip and SheetJS
  rather than the spec window
- treat a resolvable-but-empty central directory as unverifiable
- keep stray EOCD byte sequences in non-ZIP documents a no-op so the
  OLE2 and plaintext fallbacks still run
- bound central-directory scanning with a shared record budget

Redaction:
- fix quadratic acronym-boundary backtracking (73s to 1ms at 400k chars)
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 7567216. Configure here.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

Split into four focused PRs so each can be verified on its own terms:

The split is lossless: all 38 files and all 8 package.json changes are accounted for, and every source file is byte-identical to what was here.

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.

1 participant