Skip to content

fix: address critical security vulnerabilities#73

Open
staging-devin-ai-integration[bot] wants to merge 1 commit intomainfrom
devin/1775619001-security-fixes
Open

fix: address critical security vulnerabilities#73
staging-devin-ai-integration[bot] wants to merge 1 commit intomainfrom
devin/1775619001-security-fixes

Conversation

@staging-devin-ai-integration
Copy link
Copy Markdown

Summary

Security audit of the codebase identified several vulnerabilities. This PR fixes the critical and high-severity code-level issues:

  1. 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.

  2. Add prototype pollution guards in evaluate.ts and runtime.ts — The Proxy-based tw objects now reject __proto__, constructor, and prototype property accesses, preventing potential prototype pollution attacks.

  3. Add .env files to root .gitignore — The root .gitignore was missing .env patterns, meaning any .env file added at the project root would be tracked by git and could accidentally expose secrets.

  4. Add path traversal validation in utils.ts — The configPath from package.json's typewind field is now validated to ensure it resolves within the project directory, preventing directory traversal attacks (e.g., ../../etc/passwd).

Review & Testing Checklist for Human

  • Verify the backtick escaping fix in babel.ts — test with a tw expression containing multiple backticks to confirm they're all escaped
  • Verify the path traversal guard in utils.ts — test with a configPath like ../../outside/config.js in package.json typewind field and confirm it throws an error
  • Run npm test to confirm existing transform tests still pass
  • Review that the prototype pollution guards in evaluate.ts and runtime.ts don't interfere with normal tw property chaining

Notes

Additional findings NOT fixed in this PR (require manual attention):

  • 21 dependency vulnerabilities (2 critical, 7 high, 8 moderate, 4 low) found via npm audit. npm audit fix fails due to workspace peer dependency conflicts (e.g., @next/font vs next version 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.
  • eval package usage in babel.ts and utils.ts — the eval npm package executes arbitrary code by design. While this is inherent to how the babel plugin evaluates tw expressions at build time, it represents a supply-chain risk. Consider whether a sandboxed alternative (e.g., vm2 or Node's built-in vm module) could be used instead.
  • No SRI hashes on external scripts in site/pages/_app.tsx (loads platform.twitter.com/widgets.js without integrity verification).
  • Wide "./dist/*" export in package.json exposes all dist files — consider restricting to specific entry points.

Link to Devin session: https://staging.itsdev.in/sessions/6a00cad66b0345a48fea96f31c355e18
Requested by: @Mokshit06

- 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>
@staging-devin-ai-integration
Copy link
Copy Markdown
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 8, 2026

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

Project Deployment Actions Updated (UTC)
typewind Error Error Apr 8, 2026 3:31am

Request Review

@staging-devin-ai-integration
Copy link
Copy Markdown
Author

Code Review: PR #73 — Address Critical Security Vulnerabilities

Overall Assessment

This PR addresses four security concerns. The backtick fix and .gitignore update are solid. The prototype pollution guards and path traversal validation have issues worth discussing.


🔴 Bugs / High-Risk Issues

1. Prototype pollution guard returns thisTw instead of blocking — may mask legitimate errors

In both evaluate.ts and runtime.ts:

if (p === '__proto__' || p === 'constructor' || p === 'prototype') return thisTw;

Returning thisTw means tw.__proto__.flex silently resolves to tw.flex — the poisoned property access is swallowed and chaining continues. This could mask bugs where user code accidentally accesses these properties.

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 undefined or null to stop the chain and make the error surface naturally.

2. Prototype pollution guard is incomplete — missing from the outer tw proxy

Both evaluate.ts (line 93) and runtime.ts (line 123) have an outer tw proxy that delegates to twUsed()[p]. This outer proxy does NOT have the prototype pollution guard:

// 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 tw.__proto__ which hits this outer proxy first, bypasses the guard, and passes '__proto__' into twUsed()[p], which accesses .__proto__ on the inner proxy. The inner proxy's guard catches it — but only because the inner get trap fires. If someone modifies the inner proxy to use Reflect.get for some properties, this could become a real vulnerability.

Recommendation: Add the same guard to the outer proxy for defense in depth.


🟡 Edge Cases / Concerns

3. 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`);
}

path.resolve() resolves .. segments but does NOT resolve symlinks. If a symlink inside the project points outside it, configPath would pass the startsWith check but the actual file read would follow the symlink to an external location. Use fs.realpathSync() on both cwd and configPath for a robust check.

4. Path traversal check fails on Windows

path.sep is \ on Windows, but cwd + path.sep assumes the resolved path uses the same separator as the CWD. While this project likely only runs on macOS/Linux in practice, the check is non-portable. Consider using path.relative() instead:

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 / Minor

6. Backtick escaping fix is correct and well-targeted

Changing ''.replace('', ...) to .replace(//g, ...)` correctly fixes the first-occurrence-only bug.

However, the same pre-existing issue noted in PR #71 applies: ${...} expressions in user code would still be evaluated inside the template literal. Consider also escaping ${:

code.replace(/`/g, '\\`').replace(/\$\{/g, '\\${')

7. .gitignore additions are appropriate

Adding .env, .env.*, and !.env.example is standard practice. Good.

8. Note: this PR and PR #71 both modify the same line in babel.ts

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.


Summary

The 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.

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