Repository navigation
fix(ui): render templates from the path checked against the emails directory - #3840
dielduarte wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 34f86fc The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 7 files
Confidence score: 3/5
- In
resolve-within-emails-directory.ts, a missing target falls back to its lexical path, so an existing symlink parent can bypass the directory check and let a later render reach a file outside the emails directory. Resolve existing parent components before accepting the path. - In
render-email-by-path.tsx, symlinked templates use the target path for the preview title and render-mode branch, which can make the preview behave differently from the requested template. Derive these values from the requested path.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/ui/src/utils/resolve-within-emails-directory.ts">
<violation number="1" location="packages/ui/src/utils/resolve-within-emails-directory.ts:8">
P1: `safeRealpath` falls back to the lexical path when the target is missing, so existing symlink parents are not checked. A file created through such a parent after validation can then be rendered outside the emails directory; resolve the nearest existing parent before checking, or fail closed when the target cannot be canonicalized.</violation>
</file>
<file name="packages/ui/src/actions/render-email-by-path.tsx">
<violation number="1" location="packages/ui/src/actions/render-email-by-path.tsx:79">
P2: `renderEmailByPath` now derives `basename`, `extname`, and the render-mode branch from the symlink target's real path, not the requested template path. For a symlinked template the preview title (page.tsx) and the metadata `basename` can disagree, and a target with a different extension changes the render mode. If this is intended, keep the requested path for display and only use the real path for file access.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| try { | ||
| return fs.realpathSync(target); | ||
| } catch { | ||
| return path.resolve(target); |
There was a problem hiding this comment.
P1: safeRealpath falls back to the lexical path when the target is missing, so existing symlink parents are not checked. A file created through such a parent after validation can then be rendered outside the emails directory; resolve the nearest existing parent before checking, or fail closed when the target cannot be canonicalized.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/ui/src/utils/resolve-within-emails-directory.ts, line 8:
<comment>`safeRealpath` falls back to the lexical path when the target is missing, so existing symlink parents are not checked. A file created through such a parent after validation can then be rendered outside the emails directory; resolve the nearest existing parent before checking, or fail closed when the target cannot be canonicalized.</comment>
<file context>
@@ -0,0 +1,40 @@
+ try {
+ return fs.realpathSync(target);
+ } catch {
+ return path.resolve(target);
+ }
+};
</file context>
| export const invalidateEmailRenderingCache = async ( | ||
| requestedEmailPath: string, | ||
| ) => { | ||
| const emailPath = resolveWithinEmailsDirectory(requestedEmailPath); |
There was a problem hiding this comment.
P2: renderEmailByPath now derives basename, extname, and the render-mode branch from the symlink target's real path, not the requested template path. For a symlinked template the preview title (page.tsx) and the metadata basename can disagree, and a target with a different extension changes the render mode. If this is intended, keep the requested path for display and only use the real path for file access.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/ui/src/actions/render-email-by-path.tsx, line 79:
<comment>`renderEmailByPath` now derives `basename`, `extname`, and the render-mode branch from the symlink target's real path, not the requested template path. For a symlinked template the preview title (page.tsx) and the metadata `basename` can disagree, and a target with a different extension changes the render mode. If this is intended, keep the requested path for display and only use the real path for file access.</comment>
<file context>
@@ -73,8 +73,11 @@ const invalidateCacheFor = (emailPath: string) => {
+export const invalidateEmailRenderingCache = async (
+ requestedEmailPath: string,
+) => {
+ const emailPath = resolveWithinEmailsDirectory(requestedEmailPath);
+ if (!emailPath) return;
invalidateCacheFor(emailPath);
</file context>
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
View guided diff | Re-trigger cubic
|
Closing: the path check mismatch is only reachable through the local preview server's same-origin server action, so we're accepting the risk and dismissing the alert instead. |
Fixes code scanning alert #327 (
js/path-injection).renderEmailByPathis a server action, so it receives arbitrary strings from the client. It checked them withisPathWithinEmailsDirectory, which resolves a relative path against the emails directory, but then used the original string, whichgetEmailComponentandfs.readFileresolve against the working directory. A relative path likewelcome.tsxpassed the check as<emails>/welcome.tsxand was loaded from<cwd>/welcome.tsx.Our own pages always pass absolute paths, and server actions only accept same-origin calls, so this needed local access to the preview server. The fix makes the checked path the only one that gets used.
Changes
utils/is-path-within-emails-directory.ts→utils/resolve-within-emails-directory.ts:isPathWithinEmailsDirectory(path): booleanbecomesresolveWithinEmailsDirectory(path): string | undefined, returning the real path it checked (orundefinedoutside the emails directory). The check is a singlestartsWith(root + sep)on the real path, the pattern CodeQL recognizes as a guard; an earlier version that also accepted the emails directory itself (=== root || startsWith) kept the alert open. Rendering the directory itself was never meaningful, so it's now rejected too. Otherwise the same rules: traversal, absolute paths, sibling prefixes and escaping symlinks are rejected, and it fails closed without the env var.actions/render-email-by-path.tsx:renderEmailByPathandinvalidateEmailRenderingCacheresolve once and use only the resolved path from then on (rendering, cache keys).actions/get-email-path-from-slug.ts: uses the new function; still returns the absolute candidate path..changeset/ui-resolve-email-path-once.md: patch for@react-email/ui.Testing
canary:renderEmailByPath('raw-html-email.html')renders the template from the emails directory.renderEmailByPath('vercel-invite-user.tsx')bundles and renders the template from the emails directory (thegetEmailComponentpath CodeQL flagged).pnpm build,pnpm typecheckand biome pass.email devonapps/demorenders all 62 templates with HTML identical to before this change, and hot reload (which goes throughinvalidateEmailRenderingCache) still updates the preview.Summary by cubic
Fixes a path-injection vulnerability in
@react-email/uiwhere a relative template path passed the emails-directory check but was then resolved against the working directory, reading files outside the emails directory.renderEmailByPathandinvalidateEmailRenderingCacheresolve the requested path once, against the emails directory, and use only that resolved path for rendering and cache invalidation.isPathWithinEmailsDirectorywithresolveWithinEmailsDirectory, which returns the real path it validated (orundefinedoutside the directory) so callers can't read a different file..tsxtemplates.Written for commit 34f86fc. Summary will update on new commits.