refactor (provider-utils): copy relevant code from secure-json-parse into codebase
#5566
Conversation
…ts with original code as reference
- `@ai-sdk/secure-json-parse` is not using a custom reviver or options, so I removed the two arguments - I simplified the regexes as we only target modern JS runtime environment and shouldn't encounter problems with unicode support. I made the change in the original package to make sure tests are passsing. I also ran the benchamrs with the original and simpilified regexes to make sure there are no regressions. Benchmarks original code: ``` > node valid.js secure-json-parse parse x 1,721,006 ops/sec ±0.29% (101 runs sampled) secure-json-parse parse proto x 1,839,405 ops/sec ±0.27% (100 runs sampled) secure-json-parse safeParse x 1,681,051 ops/sec ±1.42% (93 runs sampled) secure-json-parse safeParse proto x 1,106,173 ops/sec ±1.47% (96 runs sampled) > node ignore.js secure-json-parse parse x 1,124,978 ops/sec ±1.13% (93 runs sampled) secure-json-parse safeParse x 1,094,985 ops/sec ±0.50% (95 runs sampled) > node no__proto__.js secure-json-parse parse x 1,019,390 ops/sec ±0.83% (92 runs sampled) secure-json-parse safeParse x 1,046,083 ops/sec ±0.23% (97 runs sampled) > node remove.js secure-json-parse parse x 618,079 ops/sec ±1.08% (97 runs sampled) secure-json-parse safeParse x 1,071,897 ops/sec ±0.28% (94 runs sampled) > node throw.js secure-json-parse parse x 399,504 ops/sec ±0.34% (98 runs sampled) secure-json-parse safeParse x 473,815 ops/sec ±0.33% (92 runs sampled) ``` Benchmarks simplified code: ``` > node valid.js secure-json-parse parse x 1,689,387 ops/sec ±0.69% (99 runs sampled) secure-json-parse parse proto x 1,785,980 ops/sec ±0.94% (96 runs sampled) secure-json-parse safeParse x 1,678,833 ops/sec ±0.78% (96 runs sampled) secure-json-parse safeParse proto x 1,119,962 ops/sec ±0.48% (96 runs sampled) > node ignore.js secure-json-parse parse x 1,116,215 ops/sec ±0.27% (96 runs sampled) secure-json-parse safeParse x 1,090,075 ops/sec ±0.86% (94 runs sampled) > node no__proto__.js secure-json-parse parse x 1,063,566 ops/sec ±0.23% (99 runs sampled) secure-json-parse safeParse x 1,067,426 ops/sec ±0.24% (95 runs sampled) > node remove.js secure-json-parse parse x 645,046 ops/sec ±0.45% (92 runs sampled) secure-json-parse safeParse x 1,108,105 ops/sec ±1.67% (96 runs sampled) > node throw.js secure-json-parse parse x 389,421 ops/sec ±1.82% (94 runs sampled) secure-json-parse safeParse x 473,508 ops/sec ±0.37% (93 runs sampled) ```
| // utf8 BOM checker (https://github.com/fastify/secure-json-parse/pull/5) | ||
| if (text && text.charCodeAt(0) === 0xfeff) { | ||
| text = text.slice(1); | ||
| } |
There was a problem hiding this comment.
I'm not sure if this is relevant in the context of @ai-sdk/provider-utils
There was a problem hiding this comment.
you're right, lets remove this.
|
Oh didn't realize it's fairly simple. Can you add tests as well (esp regarding the security issues it resolves), in case they have them? |
ha, good point, will do! |
….test.ts with original code as reference
…om `secure-json-parse`
| const { stackTraceLimit } = Error; | ||
| Error.stackTraceLimit = 0; | ||
| try { | ||
| return _parse(text); | ||
| } finally { | ||
| Error.stackTraceLimit = stackTraceLimit; | ||
| } |
There was a problem hiding this comment.
@gr2m do you know what this is for? I usually don't like messing w/ globals - but this might be for security reasons?
There was a problem hiding this comment.
It's for performance reasons, see fastify/secure-json-parse#90. Let me know if you want to keep it or not, I will add a comment with the pull request URL for future reference.
Fun fact: I know @Uzlopak, the author of that PR quite well, he is helping co-maintain @probot and @octokit for years now.
There was a problem hiding this comment.
let's keep it - might be worth adding a comment that this is for performance reasons
There was a problem hiding this comment.
ah nevermind the comment is there
|
Continued in #5622 |
Similar to #5543.
@lgrammel and I discussed that
secure-json-parsecould be removed as dependency in order to further reduce the number of external dependencies.Todos