Support legacy seed in perf #1256
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return await readStudies(filesWithContent, false); | ||
| } catch { | ||
| console.log(`Failed to read studies ${revision}, use seed.json fallback:`); | ||
| const seedContent = execSync(`git show "${revision}":seed/seed.json`, { |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Detected calls to child_process from a function argument revision. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.
Source: https://semgrep.dev/r/javascript.lang.security.detect-child-process.detect-child-process
Cc @thypon @kdenhartog
There was a problem hiding this comment.
execSync should be called with an array, instead of a string. This would avoid command injections
There was a problem hiding this comment.
execSync() doesn't support string[]. I've made a wrapper over spawnSync() to address this.
There was a problem hiding this comment.
Hm, git show <rev>:<filename> has to be called in shell=True anyway. spawnSync() actually doesn't provide more security.
There was a problem hiding this comment.
you can add a check to ensure the parameter is a git hash [a-z0-9]+ so it's safe to pass it as is with shell=True.
atuchin-m
force-pushed
the
support-legacy-seed-in-perf
branch
from
1cf9d12
to
0559762
Compare
| return await readStudies(filesWithContent, false); | ||
| } catch { | ||
| console.log(`Failed to read studies ${revision}, use seed.json fallback:`); | ||
| const seedContent = execSync(`git show "${revision}":seed/seed.json`, { |
There was a problem hiding this comment.
you can add a check to ensure the parameter is a git hash [a-z0-9]+ so it's safe to pass it as is with shell=True.
| it('test1_git_revision', () => runTest('test1', 'HEAD')); | ||
|
|
||
| // Check creating seed using git history for legacy seed.json. | ||
| it('legacy_seed', () => runTest('legacy_seed', '3f3eb03e')); |
| @@ -0,0 +1,2556 @@ | |||
| { | |||
There was a problem hiding this comment.
wdyt about re-building all expected_seed.json with useProtoFieldName: true, in this PR?
or do you want to commit this file as is and then rewrite it separately?
There was a problem hiding this comment.
updated test files
| return await readStudies(filesWithContent, false); | ||
| } catch { | ||
| console.log(`Failed to read studies ${revision}, use seed.json fallback:`); | ||
| const seedContent = execSync(`git show "${revision}":seed/seed.json`, { |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Detected calls to child_process from a function argument revision. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.
Source: https://semgrep.dev/r/javascript.lang.security.detect-child-process.detect-child-process
Cc @thypon @kdenhartog
There was a problem hiding this comment.
The validation code has been added above.
Also we have a follow up issue for sanitizing all the args: #1250
| @@ -203,6 +210,16 @@ describe('create command', () => { | |||
| serialNumberPath, | |||
| ]), | |||
| ).rejects.toThrowError('process.exit(1)'); | |||
|
|
|||
| const expectedError = ( | |||
There was a problem hiding this comment.
That part was missed and the tests ignored expected_errors.txt
The PR support
npm run seed_tools create -- --revision <old_revision_with_seed_json>