Skip to content
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

Jump to bottom

Expose E2E build errors #940

Merged
merged 10 commits into from
Mar 7, 2024
Merged

Expose E2E build errors #940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
afc343c
pipe stderr to terminal rather than build file
tevanoff Mar 6, 2024
15a6990
handle all E2E build errors
tevanoff Mar 6, 2024
c990d37
remove redundant check
tevanoff Mar 6, 2024
e8e3156
try to determine if build command not found error
tevanoff Mar 6, 2024
f2d04f7
add tests
tevanoff Mar 6, 2024
9f0d8b2
small cleanup
tevanoff Mar 6, 2024
c4d7e6b
tweak test
tevanoff Mar 7, 2024
9c40e93
add comments
tevanoff Mar 7, 2024
b70ddb6
use test instead of match
tevanoff Mar 7, 2024
b0c32ae
clean up test
tevanoff Mar 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions node-src/lib/setExitCode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ export const exitCodes = {
STORYBOOK_START_FAILED: 22,
STORYBOOK_BROKEN: 23,

// E2E errors
E2E_BUILD_FAILED: 51,

tevanoff marked this conversation as resolved.
Show resolved Hide resolved
// Subprocess errors
GIT_NOT_CLEAN: 101,
GIT_OUT_OF_DATE: 102,
Expand Down
44 changes: 44 additions & 0 deletions node-src/tasks/build.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,3 +166,47 @@ describe('buildStorybook', () => {
);
});
});

describe('buildStorybook E2E', () => {
// Error messages that we expect to result in the missing dependency error
const missingDependencyErrorMessages = [
{ name: 'not found 1', error: 'Command not found: build-archive-storybook' },
{ name: 'not found 2', error: 'Command "build-archive-storybook" not found' },
{ name: 'npm not found', error: 'NPM error code E404\n\nMore error info' },
{ name: 'exit code not found', error: 'Command failed with exit code 127: some command\n\nsome error line\n\n' },
{ name: 'single line command failure', error: 'Command failed with exit code 1: npm exec build-archive-storybook --output-dir /tmp/chromatic--4210-0cyodqfYZabe' },
];

it.each(
missingDependencyErrorMessages
)('fails with missing dependency error when error message is $name', async ({ error }) => {
const ctx = {
buildCommand: 'npm exec build-archive-storybook',
options: { buildScriptName: '', playwright: true },
env: { STORYBOOK_BUILD_TIMEOUT: 0 },
log: { debug: vi.fn(), error: vi.fn() },
} as any;

command.mockRejectedValueOnce(new Error(error));
await expect(buildStorybook(ctx)).rejects.toThrow('Command failed');
expect(ctx.log.error).toHaveBeenCalledWith(expect.stringContaining('Failed to import `@chromatic-com/playwright`'));

ctx.log.error.mockClear();
});

it('fails with generic error message when not missing dependency error', async () => {
const ctx = {
buildCommand: 'npm exec build-archive-storybook',
options: { buildScriptName: '', playwright: true },
env: { STORYBOOK_BUILD_TIMEOUT: 0 },
log: { debug: vi.fn(), error: vi.fn() },
} as any;

const errorMessage = 'Command failed with exit code 1: npm exec build-archive-storybook --output-dir /tmp/chromatic--4210-0cyodqfYZabe\n\nMore error message lines\n\nAnd more';
skitterm marked this conversation as resolved.
Show resolved Hide resolved
command.mockRejectedValueOnce(new Error(errorMessage));
await expect(buildStorybook(ctx)).rejects.toThrow('Command failed');
expect(ctx.log.error).not.toHaveBeenCalledWith(expect.stringContaining('Failed to import `@chromatic-com/playwright`'));
expect(ctx.log.error).toHaveBeenCalledWith(expect.stringContaining('Failed to run `chromatic --playwright`'));
expect(ctx.log.error).toHaveBeenCalledWith(expect.stringContaining(errorMessage));
});
});
53 changes: 41 additions & 12 deletions node-src/tasks/build.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
import buildFailed from '../ui/messages/errors/buildFailed';
import { failed, initial, pending, skipped, success } from '../ui/tasks/build';
import { getPackageManagerRunCommand } from '../lib/getPackageManager';
import { buildBinName as e2EbuildBinName, getE2EBuildCommand, isE2EBuild } from '../lib/e2e';
import { buildBinName as e2eBuildBinName, getE2EBuildCommand, isE2EBuild } from '../lib/e2e';
import e2eBuildFailed from '../ui/messages/errors/e2eBuildFailed';
import missingDependency from '../ui/messages/errors/missingDependency';

export const setSourceDir = async (ctx: Context) => {
Expand Down Expand Up @@ -60,6 +61,40 @@
const timeoutAfter = (ms) =>
new Promise((resolve, reject) => setTimeout(reject, ms, new Error(`Operation timed out`)));

function isE2EBuildCommandNotFoundError(errorMessage: string) {
// It's hard to know if this is the case as each package manager has a different type of
// error for this, but we'll try to figure it out.
const errorRegexes = [
'command not found', // `Command not found: build-archive-storybook`
`[\\W]?${e2eBuildBinName}[\\W]? not found`, // `Command "build-archive-storybook" not found`
'code E404', // npm not found error can include this code
'exit code 127', // Exit code 127 is a generic not found exit code
`command failed.*${e2eBuildBinName}.*$`]; // A single line error from execa like `Command failed: yarn build-archive-storybook ...`
skitterm marked this conversation as resolved.
Show resolved Hide resolved

return errorRegexes.some((regex) => (new RegExp(regex, 'gi')).test(errorMessage));

Check warning on line 74 in node-src/tasks/build.ts

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

node-src/tasks/build.ts#L74 

RegExp() called with a `regex` function argument, this might allow an attacker to cause a Regular Expression Denial-of-Service (ReDoS) within your application as RegExP blocks the main thread.

Check warning on line 74 in node-src/tasks/build.ts

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

node-src/tasks/build.ts#L74 

The `RegExp` constructor was called with a non-literal value.
Copy link

Choose a reason for hiding this comment

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

⚠️ Codacy found a medium Security issue: The RegExp constructor was called with a non-literal value.

The security issue identified by Semgrep is related to the use of the RegExp constructor with a non-literal value. This can potentially lead to a Regular Expression Denial of Service (ReDoS) attack if an attacker is able to control the pattern passed to RegExp, especially if they can provide a complex pattern that can cause the application to hang due to excessive backtracking.

In this specific case, the variable e2eBuildBinName is interpolated into a regex pattern, which could be a concern if e2eBuildBinName is user-controlled or can be manipulated. However, without more context, it's unclear if e2eBuildBinName is a constant, an environment variable, or user input.

Assuming e2eBuildBinName is a safe, constant value that does not come from user input, you could pre-compile the regexes with the interpolated value to both improve performance and satisfy the linter. If e2eBuildBinName is not a constant and can vary at runtime, it's important to sanitize or escape it before using it in a regex pattern to prevent ReDoS attacks.

Here's a single line code suggestion that pre-compiles the regexes assuming e2eBuildBinName is a safe, constant value:

Suggested change
return errorRegexes.some((regex) => (new RegExp(regex, 'gi')).test(errorMessage));
return errorRegexes.some((regex) => RegExp(regex.replace('${e2eBuildBinName}', e2eBuildBinName), 'gi').test(errorMessage));

This suggestion replaces the backticks and ${e2eBuildBinName} interpolation with a replace call that only happens once, assuming e2eBuildBinName is not dynamic. If e2eBuildBinName is dynamic, you would need to ensure it is properly escaped to prevent it from being used in an attack.

This comment was generated by an experimental AI tool.

Copy link

Choose a reason for hiding this comment

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

⚠️ Codacy found a medium Security issue: RegExp() called with a regex function argument, this might allow an attacker to cause a Regular Expression Denial-of-Service (ReDoS) within your application as RegExP blocks the main thread.

The issue identified by Semgrep is related to the potential for a Regular Expression Denial of Service (ReDoS) attack. This can happen when user input or a variable that can be influenced by user input is used to dynamically create a regular expression. If an attacker provides a specially crafted input, it can create a regular expression that takes a very long time to evaluate, effectively blocking the main thread and causing a denial of service.

In the provided code, the variable e2eBuildBinName is interpolated into a string that is then used to create a regular expression. If e2eBuildBinName is or can be influenced by user input, it could be exploited to cause a ReDoS attack.

To mitigate this, we need to sanitize or escape the variable e2eBuildBinName before using it in the regular expression. However, since the code suggestion must be a single line change and we don't have the context for where e2eBuildBinName is coming from, a general solution could be to use a library like lodash to escape RegExp special characters.

Here's a single line code suggestion that uses the escapeRegExp function from lodash to escape any special characters in e2eBuildBinName before using it in the regex:

Suggested change
return errorRegexes.some((regex) => (new RegExp(regex, 'gi')).test(errorMessage));
`[\\W]?${_.escapeRegExp(e2eBuildBinName)}[\\W]? not found`, // `Command "build-archive-storybook" not found`

Please note that for this suggestion to work, you must ensure that the lodash library is installed and imported in your code as _. If lodash is not already a dependency, you will need to add it to your project and import the escapeRegExp function:

Suggested change
return errorRegexes.some((regex) => (new RegExp(regex, 'gi')).test(errorMessage));
import _ from 'lodash';

Or, if you prefer to import only the needed function:

Suggested change
return errorRegexes.some((regex) => (new RegExp(regex, 'gi')).test(errorMessage));
import { escapeRegExp } from 'lodash';

This comment was generated by an experimental AI tool.

}

function e2eBuildErrorMessage(err, workingDir: string, ctx: Context): { exitCode: number, message: string } {
const flag = ctx.options.playwright ? 'playwright' : 'cypress';
const errorMessage = err.message;

// If we tried to run the E2E package's bin directly (due to being in the action)
// and it failed, that means we couldn't find it. This probably means they haven't
// installed the right dependency or run from the right directory.
if (isE2EBuildCommandNotFoundError(errorMessage)) {
const dependencyName = `@chromatic-com/${flag}`;
return {
exitCode: exitCodes.MISSING_DEPENDENCY,
message: missingDependency({ dependencyName, flag, workingDir }),
};
}

return {
exitCode: exitCodes.E2E_BUILD_FAILED,
message: e2eBuildFailed({ flag, errorMessage }),
};
}

export const buildStorybook = async (ctx: Context) => {
let logFile = null;
if (ctx.options.storybookLogFile) {
Expand All @@ -77,7 +112,7 @@
ctx.log.debug('Runtime metadata:', JSON.stringify(ctx.runtimeMetadata, null, 2));

const subprocess = execaCommand(ctx.buildCommand, {
stdio: [null, logFile, logFile],
stdio: [null, logFile, null],
tevanoff marked this conversation as resolved.
Show resolved Hide resolved
signal,
env: { NODE_ENV: ctx.env.STORYBOOK_NODE_ENV || 'production' },
});
Expand All @@ -86,16 +121,10 @@
// If we tried to run the E2E package's bin directly (due to being in the action)
// and it failed, that means we couldn't find it. This probably means they haven't
// installed the right dependency or run from the right directory
if (
ctx.options.inAction &&
(isE2EBuild(ctx.options)) &&
e.message.match(e2EbuildBinName)
) {
const flag = ctx.options.playwright ? 'playwright' : 'cypress';
const dependencyName = `@chromatic-com/${flag}`;
ctx.log.error(missingDependency({ dependencyName, flag, workingDir: process.cwd() }));
ctx.log.debug(e);
setExitCode(ctx, exitCodes.MISSING_DEPENDENCY, true);
if (isE2EBuild(ctx.options)) {
const errorInfo = e2eBuildErrorMessage(e, process.cwd(), ctx);
ctx.log.error(errorInfo.message);
setExitCode(ctx, errorInfo.exitCode, true);
throw new Error(failed(ctx).output);
}

Expand Down
8 changes: 8 additions & 0 deletions node-src/ui/messages/errors/e2eBuildFailed.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import e2eBuildFailed from './e2eBuildFailed';

export default {
title: 'CLI/Messages/Errors',
};

export const E2EBuildFailed = () =>
e2eBuildFailed({ flag: 'playwright', errorMessage: 'Error Message' });
17 changes: 17 additions & 0 deletions node-src/ui/messages/errors/e2eBuildFailed.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import chalk from 'chalk';
import { dedent } from 'ts-dedent';
import { error } from '../../components/icons';

export default ({
flag,
errorMessage,
}: {
flag: string;
errorMessage: string;
}) => {
return dedent(chalk`
${error} Failed to run \`chromatic --${flag}\`:

${errorMessage}
tevanoff marked this conversation as resolved.
Show resolved Hide resolved
`);
};