-
Notifications
You must be signed in to change notification settings - Fork 0
fix: improve stack frame sanitization to prevent ReDoS vulnerabilities #33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughReplaces global-regex stack-trace sanitization with line-by-line processing to mitigate ReDoS and avoid regex backtracking. Adds defensive checks (skip lines >1000 chars, early pattern tests), uses index-based extraction for two stack-line patterns, preserves prior redaction semantics, and makes no public API changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/enclave-vm/src/double-vm/double-vm-wrapper.ts (1)
56-57: Consider redacting or truncating long lines instead of skipping.Lines exceeding 1000 characters are skipped entirely without redaction. If a maliciously crafted or unusual stack trace contains sensitive paths in a long line, they would leak through unredacted.
Consider either:
- Truncating the line and appending
[REDACTED]- Replacing the entire line with
[REDACTED - line too long]♻️ Suggested approach
// Defense-in-depth: skip very long lines - if (line.length > 1000) continue; + if (line.length > 1000) { + lines[i] = ' at [REDACTED - line too long]'; + continue; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/enclave-vm/src/adapters/vm-adapter.tslibs/enclave-vm/src/double-vm/double-vm-wrapper.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/enclave-vm/src/adapters/vm-adapter.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use ESLint flat config with unused parameters and variables prefixed with underscore_
Prefer workspace import paths (e.g.,import { validate } from 'ast-guard') over deep cross-library relative imports
Files:
libs/enclave-vm/src/double-vm/double-vm-wrapper.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/enclave-vm/src/double-vm/double-vm-wrapper.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Performance
🔇 Additional comments (3)
libs/enclave-vm/src/double-vm/double-vm-wrapper.ts (3)
36-40: LGTM!Clear documentation of the ReDoS vulnerability being mitigated and the approach taken. This helps future maintainers understand the security rationale.
75-81: LGTM!Pattern 2 handling is correct. The regex
/:\d+:\d+$/is anchored at the end, limiting backtracking potential. The combination of line-length limits and pre-checks makes this safe from ReDoS.The overall line-by-line approach successfully mitigates the ReDoS vulnerability while preserving the redaction semantics.
59-73: Pattern 1 does not handleasyncfunction stack frames.The logic extracts the function name by finding the first space after
at, then checking if the next character is(. For standard frames likeat foo (path:1:2), this works correctly. However, for async frames likeat async foo (path:1:2), the first space comes afterasync(position 5), so the checkafterAt.charAt(spaceIdx + 1) === '('fails with'f'instead of'(', and the line is not redacted. Pattern 2 also won't match since it expects lines ending with:line:columnwithout parentheses.While async operations are blocked by default in the enclave, errors could still propagate with async frames in their stack traces. If this needs to be handled, consider:
Potential fix
const atIdx = line.indexOf('at '); if (atIdx !== -1) { const afterAt = line.substring(atIdx + 3).trimStart(); + // Skip "async " prefix if present + const prefix = afterAt.startsWith('async ') ? 'async ' : ''; + const nameStart = afterAt.substring(prefix.length); - const spaceIdx = afterAt.indexOf(' '); + const spaceIdx = nameStart.indexOf(' '); if (spaceIdx !== -1 && afterAt.charAt(spaceIdx + 1 + prefix.length) === '(') { - const funcName = afterAt.substring(0, spaceIdx); + const funcName = nameStart.substring(0, spaceIdx); lines[i] = line.substring(0, atIdx) + 'at ' + funcName + ' ([REDACTED])'; continue; } }
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.