Configuration file JSON parse error

ghengeveld commented 3 months ago

From AP-4348

How is the user affected? And what is the expected behavior?

When a user enables the Visual tests addon in their Storybook with an existing configuration where he/she has already provided a configuration file (i.e., chromatic.config.json), the addon introduces the following issues:

1. It extends the configuration file with the projectId key following the standard configuration file format for the Visual tests addon. This seems like a regression from the early stages of the addon where both items were required to be provided for the addon to work in the first place.

2. During the project selection process, the addon throws an error in the CLI when the user selects the project from the list of projects, leaving the CLI in a broken state and providing a misleading error message as the file format is not the issue here.

3. When the user clicks the Catch a UI change button, there's a flicker in the UI showcasing the onboarding workflow for the Visual tests addon and immediately after that, the user is redirected to the the latest snapshot of the selected component.

For additional context here's a recording of the issue:

https://github.com/chromaui/addon-visual-tests/assets/22988955/761ba913-b4ad-4ad4-a750-e37ead16fe6e

Additionally, based on some additional testing, if the user already has the projectToken key and value in the chromatic.config.json file and other configuration options the error is still present as demonstrated in the following screenshot.

chromatic-config-vt-addon-existing-config-clash

Considering the above, the Visual tests addon is not handling the configuration file properly and does not provide the user with the correct error message. Additionally, we're exposing ourselves and the user to a considerable security risk by asking them to commit the configuration file to their repository as part of the messaging in the addon's UI.

This issue should be addressed as soon as possible and we should decide on a solution that doesn't expose the user and us to a security risk, a potential billing issue with the user's account. Leading to a breach of trust and a possible loss of users.

Open to any suggestions and feedback on this issue.

How many and/or what class of users does this impact?

All

What are the steps for reproducing the issue?

Create a new Storybook project with a framework of your choice.
Create a Chromatic project and link it to your Storybook project.
Create a chromatic.config.json file with the provided token.
Run a build to establish a baseline.
Manually add the Visual tests addon to the project
Run Storybook and navigate to the Visual tests addon panel and connect it.

MichaelArestad commented 3 months ago

Bumping up the severity because of mentioned security risk.

ghengeveld commented 3 months ago

@MichaelArestad I don't think this is a serious security risk. The projectToken should preferably be an environment variable but there are valid situations for it to be hardcoded (usually in an open source project that's forked). It's not really a security issue if this happens because this token has explicit restrictions as to what it can be used for. Historically we have recommended folks to put the token in their package.json, so this isn't new.

chromaui / addon-visual-tests