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.

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

Configuration file JSON parse error #247

Closed
ghengeveld opened this issue Mar 22, 2024 · 2 comments · Fixed by chromaui/chromatic-cli#961 or #278
Closed

Configuration file JSON parse error #247

ghengeveld opened this issue Mar 22, 2024 · 2 comments · Fixed by chromaui/chromatic-cli#961 or #278
Assignees
Labels
bug Classification: Something isn't working sev:S2

Comments

@ghengeveld
Copy link
Member

ghengeveld commented Mar 22, 2024

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:

chromatic-mixed-config.mp4

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.
@ghengeveld ghengeveld added bug Classification: Something isn't working sev:S4 labels Mar 22, 2024
@MichaelArestad
Copy link
Contributor

Bumping up the severity because of mentioned security risk.

@ghengeveld ghengeveld changed the title Issue with existing configuration Configuration file JSON parse error Mar 28, 2024
@shilman shilman added this to the include in sb upgrade milestone Mar 28, 2024
@ghengeveld
Copy link
Member Author

ghengeveld commented Mar 28, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Classification: Something isn't working sev:S2
Projects
None yet
3 participants