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

fix: replace the JSON5 parser with JSON #6149

Merged
merged 2 commits into from
Apr 8, 2024
Merged

fix: replace the JSON5 parser with JSON #6149

merged 2 commits into from
Apr 8, 2024

Conversation

kmelve
Copy link
Member

@kmelve kmelve commented Mar 29, 2024

Description

sanity init in a Next.js project used the golden-fleece library to patch the tsconfig.json with the target property. This introduced a bug, since we inserted a JSON5 string into a JSON file.

This PR replaces the JSON5 parser with a JSON one (silver-fleece).

What to review

If the sanity init command now works (better) inside of a Next.js project.

Testing

  • Build the CLI
  • Use the built CLI and run ./path/to/sanity init inside a fresh Next.js project, choosing Y for TypeScript.

Notes for release

Fixes a bug where wrongly formatted configuration would be inserted into tsconfig.json when sanity init was run inside of a Next.js project.

@kmelve kmelve requested a review from a team as a code owner March 29, 2024 09:23
@kmelve kmelve requested review from cngonzalez and removed request for a team March 29, 2024 09:23
Copy link

vercel bot commented Mar 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
performance-studio ✅ Ready (Inspect) Visit Preview Apr 8, 2024 4:37pm
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 8, 2024 4:37pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Apr 8, 2024 4:37pm

Copy link

socket-security bot commented Mar 29, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@types/[email protected] None +1 9.65 kB types
npm/@types/[email protected] None +1 68.8 kB types
npm/[email protected] environment, filesystem +13 358 kB paulmillr
npm/[email protected] None 0 23.6 kB jedwatson
npm/[email protected] environment +1 60 kB qix
npm/[email protected] environment, filesystem, network, shell 0 135 kB evanw
npm/[email protected] environment, filesystem, network Transitive: eval, unsafe +48 1.54 MB ulisesgascon
npm/[email protected] Transitive: unsafe +1 8.39 kB sindresorhus
npm/[email protected] None +1 139 kB stipsan
npm/[email protected] None +2 125 kB npm-cli-ops
npm/[email protected] None 0 32.4 MB typescript-bot

🚮 Removed packages: npm/@babel/[email protected], npm/@babel/[email protected], npm/@babel/[email protected], npm/@babel/[email protected], npm/@babel/[email protected], npm/@babel/[email protected], npm/@babel/[email protected], npm/@babel/[email protected], npm/@bjoerge/[email protected], npm/@codemirror/[email protected], npm/@codemirror/[email protected], npm/@codemirror/[email protected], npm/@codemirror/[email protected], npm/@codemirror/[email protected], npm/@codemirror/[email protected], npm/@codemirror/[email protected], npm/@dnd-kit/[email protected], npm/@dnd-kit/[email protected], npm/@dnd-kit/[email protected], npm/@dnd-kit/[email protected], npm/@sanity/[email protected], npm/@sanity/[email protected], npm/@sanity/[email protected], npm/@sanity/[email protected], npm/@sanity/[email protected], npm/@sanity/[email protected], npm/@sanity/[email protected], npm/@sanity/[email protected], npm/@sanity/[email protected], npm/@sanity/[email protected], npm/@sanity/[email protected], npm/@sanity/[email protected], npm/@sanity/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected]

View full report↗︎

Copy link
Contributor

github-actions bot commented Mar 29, 2024

Package Documentation Change
@sanity/portable-text-editor +5%
sanity -3%
@sanity/mutator -12%
@sanity/diff -55%
Full Report
@sanity/portable-text-editor
This branch Next branch
23 documented 22 documented
44 not documented 45 not documented
@sanity/migrate
This branch Next branch
17 documented 17 documented
70 not documented 59 not documented
@sanity/block-tools
This branch Next branch
4 documented 4 documented
9 not documented 9 not documented
@sanity/types
This branch Next branch
55 documented 55 documented
234 not documented 240 not documented
sanity/desk
This branch Next branch
84 documented 84 documented
64 not documented 64 not documented
@sanity/cli
This branch Next branch
1 documented 1 documented
31 not documented 31 not documented
@sanity/codegen
This branch Next branch
7 documented 7 documented
2 not documented 3 not documented
sanity/structure
This branch Next branch
2 documented 2 documented
8 not documented 8 not documented
@sanity/util/concurrency-limiter
This branch Next branch
1 documented 1 documented
0 not documented 0 not documented
@sanity/util/legacyDateFormat
This branch Next branch
0 documented 0 documented
4 not documented 5 not documented
@sanity/schema/_internal
This branch Next branch
0 documented 0 documented
13 not documented 13 not documented
@sanity/util/paths
This branch Next branch
1 documented 1 documented
15 not documented 15 not documented
sanity/router
This branch Next branch
17 documented 17 documented
26 not documented 26 not documented
@sanity/schema
This branch Next branch
0 documented 0 documented
2 not documented 2 not documented
sanity/cli
This branch Next branch
2 documented 2 documented
0 not documented 0 not documented
@sanity/vision
This branch Next branch
0 documented 0 documented
3 not documented 3 not documented
@sanity/util/fs
This branch Next branch
0 documented 0 documented
3 not documented 3 not documented
sanity/_internal
This branch Next branch
0 documented 0 documented
1 not documented 1 not documented
@sanity/util/client
This branch Next branch
1 documented 1 documented
0 not documented 0 not documented
@sanity/util/createSafeJsonParser
This branch Next branch
1 documented 1 documented
0 not documented 0 not documented
sanity/_internalBrowser
This branch Next branch
0 documented 0 documented
3 not documented 4 not documented
@sanity/util/content
This branch Next branch
1 documented 1 documented
5 not documented 5 not documented
sanity
This branch Next branch
179 documented 186 documented
857 not documented 868 not documented
@sanity/mutator
This branch Next branch
7 documented 8 documented
4 not documented 3 not documented
@sanity/diff
This branch Next branch
13 documented 29 documented
16 not documented 0 not documented

Copy link
Contributor

github-actions bot commented Mar 29, 2024

Component Testing Report Updated Apr 8, 2024 4:42 PM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 34s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 6s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 25s 6 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 12s 4 2 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 12s 6 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 32s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 1m 2s 14 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 1s 18 0 0
formBuilder/inputs/PortableText/RangeDecoration.spec.tsx ✅ Passed (Inspect) 13s 6 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 13s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 20s 9 0 0

Copy link
Member

@cngonzalez cngonzalez left a comment

Choose a reason for hiding this comment

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

confirmed that the plaintext string no longer getting inserted!

I'm a little nervous about going from a package with 150k weekly downloads to one with 37 downloads. I'm not sure if the workaround (per my tests, something like:

config.compilerOptions["target"]= 'ES2017'

is preferable.

@kmelve
Copy link
Member Author

kmelve commented Apr 2, 2024

Got it!

I read through the source code, and it's fairly small/straightforward and has no external dependencies, so I felt the risk was low.

Not sure I follow the alternate solve in your comment? 🤔

@cngonzalez
Copy link
Member

cngonzalez commented Apr 2, 2024

Yep, it seemed like a pretty straightforward fork to me too -- and the original package hasn't been updated in years so it's not like we gain benefits there 😬

The code I put there (rather than using silver-fleece) seemed to insert the correct string. Are you not seeing the same thing? This package is a little obscure, it seems.

I tell a lie -- not sure why I thought that would work, TBH.

@cngonzalez cngonzalez self-requested a review April 2, 2024 19:50
Copy link
Member

@cngonzalez cngonzalez left a comment

Choose a reason for hiding this comment

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

Considering the lower impact of any future issues with this package (and annoying/difficult workarounds) let's move ahead! 👍

Copy link
Contributor

@jtpetty jtpetty left a comment

Choose a reason for hiding this comment

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

👍

packages/@sanity/cli/package.json Outdated Show resolved Hide resolved
Copy link

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert Package NoteSource
Install scripts npm/[email protected]
  • Install script: postinstall
  • Source: node -e "try{require('./postinstall')}catch(e){}"

View full report↗︎

Next steps

What is an install script?

Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts.

Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

@kmelve kmelve added this pull request to the merge queue Apr 8, 2024
Merged via the queue into next with commit fe11588 Apr 8, 2024
16 of 18 checks passed
@kmelve kmelve deleted the sdx-1217 branch April 8, 2024 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants