fix: address critical security vulnerabilities#73
fix: address critical security vulnerabilities#73staging-devin-ai-integration[bot] wants to merge 1 commit intomainfrom
Conversation
- Fix incomplete backtick escaping in babel.ts (code injection risk) - Add prototype pollution guards in evaluate.ts and runtime.ts - Add .env files to root .gitignore to prevent accidental secret commits - Add path traversal validation in utils.ts for configPath Co-Authored-By: mokshitjain2006+coggitgrant0704 <mokshitjain2006@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Code Review: PR #73 — Address Critical Security VulnerabilitiesOverall AssessmentThis PR addresses four security concerns. The backtick fix and 🔴 Bugs / High-Risk Issues1. Prototype pollution guard returns In both if (p === '__proto__' || p === 'constructor' || p === 'prototype') return thisTw;Returning Recommendation: Throw an error instead: if (p === '__proto__' || p === 'constructor' || p === 'prototype') {
throw new Error(`Typewind: accessing '${p}' on tw is not allowed`);
}Alternatively, return 2. Prototype pollution guard is incomplete — missing from the outer Both // evaluate.ts line 90-99
const tw = new Proxy({}, {
get(_target, p) {
if (typeof p !== 'string') return Reflect.get(...arguments);
return twUsed()[p]; // ← no guard here
},
});An attacker could do Recommendation: Add the same guard to the outer proxy for defense in depth. 🟡 Edge Cases / Concerns3. Path traversal validation has a bypass via symlinks const configPath = path.resolve(cwd, configFile);
if (!configPath.startsWith(cwd + path.sep) && configPath !== cwd) {
throw new Error(`Tailwind config path "${configFile}" resolves outside the project directory`);
}
4. Path traversal check fails on Windows
const relative = path.relative(cwd, configPath);
if (relative.startsWith('..') || path.isAbsolute(relative)) {
throw new Error(...);
}5. Error re-throw logic is fragile } catch (err) {
if (err instanceof Error && err.message.includes('resolves outside')) {
throw err;
}
}Matching on message text is brittle — if the error message wording changes, the re-throw silently stops working and the path traversal error gets swallowed. Use a custom error class or a sentinel property: class PathTraversalError extends Error {}
// ...
throw new PathTraversalError(`Tailwind config path...`);
// ...
} catch (err) {
if (err instanceof PathTraversalError) throw err;
}🟢 Good / Minor6. Backtick escaping fix is correct and well-targeted ✓ Changing However, the same pre-existing issue noted in PR #71 applies: code.replace(/`/g, '\\`').replace(/\$\{/g, '\\${')7. Adding 8. Note: this PR and PR #71 both modify the same line in Both PRs change the backtick escaping on the same line. Whichever merges second will have a merge conflict — though the fix is identical in both, so resolution is trivial. SummaryThe security hardening intent is good, but the prototype pollution guards should throw instead of silently continuing, and the path traversal check needs hardening against symlinks and message-based error matching. The backtick fix is correct. |
Summary
Security audit of the codebase identified several vulnerabilities. This PR fixes the critical and high-severity code-level issues:
Fix incomplete backtick escaping in
babel.ts(code injection risk) —String.replace()with a string pattern only replaces the first occurrence. Changed to a global regex/\/gso all backticks in user expressions are properly escaped before being interpolated into the_eval()` template literal.Add prototype pollution guards in
evaluate.tsandruntime.ts— The Proxy-basedtwobjects now reject__proto__,constructor, andprototypeproperty accesses, preventing potential prototype pollution attacks.Add
.envfiles to root.gitignore— The root.gitignorewas missing.envpatterns, meaning any.envfile added at the project root would be tracked by git and could accidentally expose secrets.Add path traversal validation in
utils.ts— TheconfigPathfrompackage.json'stypewindfield is now validated to ensure it resolves within the project directory, preventing directory traversal attacks (e.g.,../../etc/passwd).Review & Testing Checklist for Human
babel.ts— test with atwexpression containing multiple backticks to confirm they're all escapedutils.ts— test with aconfigPathlike../../outside/config.jsinpackage.jsontypewindfield and confirm it throws an errornpm testto confirm existing transform tests still passevaluate.tsandruntime.tsdon't interfere with normaltwproperty chainingNotes
Additional findings NOT fixed in this PR (require manual attention):
npm audit.npm audit fixfails due to workspace peer dependency conflicts (e.g.,@next/fontvsnextversion mismatch). Key vulnerable packages:next@14.x(auth bypass, cache poisoning — critical),basic-ftp(path traversal — critical),lodash(prototype pollution — high),rollup(XSS, path traversal — high). Recommend updating these dependencies manually.evalpackage usage inbabel.tsandutils.ts— theevalnpm package executes arbitrary code by design. While this is inherent to how the babel plugin evaluatestwexpressions at build time, it represents a supply-chain risk. Consider whether a sandboxed alternative (e.g.,vm2or Node's built-invmmodule) could be used instead.site/pages/_app.tsx(loadsplatform.twitter.com/widgets.jswithout integrity verification)."./dist/*"export inpackage.jsonexposes all dist files — consider restricting to specific entry points.Link to Devin session: https://staging.itsdev.in/sessions/6a00cad66b0345a48fea96f31c355e18
Requested by: @Mokshit06