Expose E2E build errors

[tevanoff]

Currently, any error thrown during the build step of an E2E build is treated as a missing dependency error, and the error message output is very specific to that. However, the build can fail for other reasons, such as archive directory not found. These other legit errors are swallowed and the resulting output is confusing.

This flips things around to try to determine if the error is due to a missing dependency, where the error usually has something to do about a command not being found. If the error message doesn't match, this will output a more generic error message that includes the original error message.

How to test

• With a local project, install the canary CLI and try to run the chromatic command in both of the following scenarios and see that the errors are correct:
  • With the E2E dep installed, but with the archive directory removed
  • With the archive directory in place, but without the E2E dep installed
• Using the canary github action (chromaui/action-canary@v1), test a project with the above two scenarios and see that the errors are correct.

📦 Published PR as canary version: 11.0.7--canary.940.8195253022.0

✨ Test out this PR locally via:

npm install [email protected]
# or 
yarn add [email protected]

[tmeasday]

This is decent advice (be careful about dynamic regexps) from Codacy but the solution doesn't make sense to me -- I could be off though. cc @paulelliott I think the AI has jumped off the deep end a bit:

a) you can't do regex.replace('${e2eBuildBinName}',... in a previously interpolated string regex, it's already been replaced.
b) it doesn't change anything to do it at that stage anyway (say if we changed the original string :

// from
`[\\W]?${e2eBuildBinName}[\\W]? not found`

// to 
'[\\W]?${e2eBuildBinName}[\\W]? not found'

Then the string wouldn't automatically get interpolated with the var, but I don't understand why it would be better to later do a .replace() to manually interpolate.

[tmeasday]

@paulelliott my feeling is the AI is getting really wordy here and I am glazing over and missing the key points:

1. Be careful about dynamic strings in regexps
2. You can comment away the warning
3. However, be super careful if you are going to do that, and document why it's safe.

[tmeasday]

I like it, thanks for working through it @tevanoff

[codacy-production]

⚠️ 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.

[codacy-production]

⚠️ 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.

[ghengeveld]

🚀 PR was released in v11.0.6 🚀