Skip to content

fix(jsx-email): port build/compile path fixes to v3 #386

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.

Sign up for GitHub

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

Open
charliecreates wants to merge 1 commit into
base: next/v3
Choose a base branch
from
Open

fix(jsx-email): port build/compile path fixes to v3 #386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/jsx-email/src/cli/commands/build.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import os from 'node:os';
import chalkTmpl from 'chalk-template';
import { globby } from 'globby';
import micromatch from 'micromatch';
import { basename, dirname, extname, join, posix, resolve, win32 } from 'path';
import { basename, dirname, extname, join, posix, relative, resolve, win32 } from 'path';
import { isWindows } from 'std-env';
import { pathToFileURL } from 'url';
import type { InferOutput as Infer } from 'valibot';
Expand Down Expand Up @@ -121,8 +121,9 @@ export const build = async (options: BuildOptions): Promise<BuildResult> => {
const templateName = basename(path, fileExt).replace(/-[^-]{8}$/, '');
const component = componentExport(renderProps);
const baseDir = dirname(path);
const relativeBaseDir = outputBasePath ? relative(outputBasePath, baseDir) : '';
const writePath = outputBasePath
? join(out!, baseDir.replace(outputBasePath, ''), templateName)
? join(out!, relativeBaseDir, templateName)
Comment on lines 123 to +126
Copy link
Contributor Author

@charliecreates charliecreates bot Dec 9, 2025

Choose a reason for hiding this comment

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

path.relative(outputBasePath, baseDir) can return paths like .. when baseDir is outside outputBasePath (or when one is realpathed and the other isn’t). That would cause join(out!, relativeBaseDir, templateName) to write outside of out (directory traversal) or into unexpected locations. The old replace hack was also flawed, but it typically wouldn’t introduce .. segments.

Given this is a CLI that writes to disk based on user-provided inputs/flags, it’s worth hardening: either ensure baseDir is actually under outputBasePath (after normalizing/realpathing both), or clamp/fallback to '' when relativeBaseDir escapes.

Suggestion

Consider validating that the computed relative path stays within out.

For example:

import { relative, isAbsolute, sep, resolve as resolvePath } from 'node:path';

const relativeBaseDir = outputBasePath
  ? relative(outputBasePath, baseDir)
  : '';

// Prevent writing outside of outDir due to `..` segments or absolute rels
const safeRelativeBaseDir =
  relativeBaseDir &&
  !relativeBaseDir.startsWith(`..${sep}`) &&
  relativeBaseDir !== '..' &&
  !isAbsolute(relativeBaseDir)
    ? relativeBaseDir
    : '';

const writePath = outputBasePath
  ? join(out!, safeRelativeBaseDir, templateName)
  : join(out!, templateName);

If you’d like, reply with "@CharlieHelps yes please" and I can add a commit with this guard (plus a small unit test around the escaping case if the code is easily exercised).

: join(out!, templateName);
// const writePath = outputBasePath
// ? join(out!, baseDir.replace(outputBasePath, ''), templateName + extension)
Expand Down
2 changes: 1 addition & 1 deletion packages/jsx-email/src/renderer/compile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export const compile = async (options: CompileOptions): Promise<CompileResult[]>
if (!entryPoint) return null;
return {
entryPoint,
path: resolve('/', path)
path: resolve(originalCwd, path)
};
})
.filter<CompileResult>(Boolean as any);
Expand Down
38 changes: 38 additions & 0 deletions packages/jsx-email/test/render/compile-path.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises';
import { join } from 'node:path';
import { pathToFileURL } from 'node:url';

import { isWindows } from 'std-env';

import { compile } from '../../src/renderer/compile.js';

describe('compile', () => {
it('returns an importable path for nested entrypoints', async () => {
const tmpRoot = await mkdtemp(join(process.cwd(), '.tmp-jsx-email-compile-'));

try {
const entryDir = join(tmpRoot, 'templates', 'nested');
await mkdir(entryDir, { recursive: true });

const entryPoint = join(entryDir, 'template.tsx');
await writeFile(
entryPoint,
`export const Template = ({ name }: { name: string }) => <h1>Hello {name}</h1>;\n`,
'utf8'
);
Comment on lines +17 to +22
Copy link
Contributor Author

@charliecreates charliecreates bot Dec 9, 2025

Choose a reason for hiding this comment

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

The test creates a .tsx file containing JSX (<h1>...) but doesn’t include an explicit React import or JSX runtime directive. If your compile pipeline relies on project defaults it may be fine, but it makes this test more brittle across config changes (e.g. switching jsx/jsxImportSource or esbuild jsx settings).

To keep the test focused on path correctness (not JSX configuration), consider using a no-JSX module or an explicit runtime import/directive so the test fails only for the intended reason.

Suggestion

Make the entrypoint independent of JSX settings by exporting plain JS/TS:

await writeFile(
  entryPoint,
  `export const Template = ({ name }: { name: string }) => "Hello " + name;\n`,
  'utf8'
);

If you specifically want JSX coverage, add an explicit directive/import appropriate to your expected runtime (e.g. /** @jsxImportSource react */) so it’s resilient to config drift.

Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.


const outDir = join(tmpRoot, 'out');
const results = await compile({ files: [entryPoint], outDir });
const result = results[0];

if (!result) throw new Error('Expected compile to return at least one output');

const compiledImportPath = isWindows ? pathToFileURL(result.path).toString() : result.path;

const mod = await import(compiledImportPath);
expect(typeof mod.Template).toBe('function');
Comment on lines +24 to +33
Copy link
Contributor Author

@charliecreates charliecreates bot Dec 9, 2025

Choose a reason for hiding this comment

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

The new test verifies importability, but it never asserts that the compiled output landed under outDir (the core behavior this change is meant to preserve/fix). A regression could still return an importable path while writing meta/output somewhere else (especially if originalCwd changes in the future).

Also, dynamically importing result.path without ensuring it’s a URL on Windows is handled, but on non-Windows this assumes Node will always accept the absolute filesystem path string. That’s generally fine in modern Node, but the test would be more robust if it always used pathToFileURL (and kept the assertion focused on the returned value being correct).

Suggestion

Tighten the test so it asserts both location and importability.

Example adjustments:

import { pathToFileURL } from 'node:url';
import { resolve, relative, sep } from 'node:path';

// ...
const results = await compile({ files: [entryPoint], outDir });
const result = results[0];
if (!result) throw new Error('Expected compile to return at least one output');

// Assert it is inside outDir (no traversal)
const relToOut = relative(outDir, result.path);
expect(relToOut === '' || (!relToOut.startsWith(`..${sep}`) && relToOut !== '..')).toBe(true);

// Always import via file URL for cross-platform consistency
const mod = await import(pathToFileURL(result.path).toString());
expect(typeof mod.Template).toBe('function');

Reply with "@CharlieHelps yes please" if you want me to add a commit that strengthens the test in this way.

} finally {
await rm(tmpRoot, { recursive: true, force: true });
}
});
});