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(Config Schema): Safe generation of standalone AJV validator #10680

Merged
merged 1 commit into from Feb 14, 2022

Conversation

pgrzesik
Copy link
Contributor

As noticed by @medikoo, current implementation of saving the file might not be safe and prone to race conditions.

Addresses: #10679

@codecov
Copy link

codecov bot commented Feb 10, 2022

Codecov Report

Merging #10680 (f7db43c) into main (4201b30) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head f7db43c differs from pull request most recent head 4ef79d5. Consider uploading reports for the commit 4ef79d5 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #10680   +/-   ##
=======================================
  Coverage   86.42%   86.42%           
=======================================
  Files         300      300           
  Lines       12398    12402    +4     
=======================================
+ Hits        10715    10719    +4     
  Misses       1683     1683           
Impacted Files Coverage Δ
...sses/config-schema-handler/resolve-ajv-validate.js 88.88% <100.00%> (+1.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4201b30...4ef79d5. Read the comment docs.

await fs.promises.writeFile(cachePath, moduleCode);

const tmpCachePath = path.resolve(getTmpDirPath(), filename);
await fse.outputFile(tmpCachePath, moduleCode);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'll be cleaner to follow same approach as in some other places as:

  1. Get process-specific tmpdir via process-utils/tmpdir
  2. Write file with regular fsp.writeFile to it
  3. Safely move a file

We do it when handling deprecation report:

const tmpHealthStatusFilename = path.resolve(await resolveTmpdir(), 'health-status');
await Promise.all([
fse.ensureDir(path.dirname(healthStatusFilename)),
fsp.writeFile(tmpHealthStatusFilename, healthStatus.join('\n')),
]);
await safeMoveFile(tmpHealthStatusFilename, healthStatusFilename);
(still here we do not need to ensure target dir, as it's already ensured by ensureDir)

@pgrzesik pgrzesik force-pushed the improve-ajv-schema-generation branch 3 times, most recently from da0ed42 to 3ef2e80 Compare February 11, 2022 10:59
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Looks great 👍

@pgrzesik pgrzesik merged commit 6093ee4 into main Feb 14, 2022
@pgrzesik pgrzesik deleted the improve-ajv-schema-generation branch February 14, 2022 14:14
@perrin4869
Copy link
Contributor

I was encountering this bug when trying to upgrade to v3, thanks for solving it!!!

@pgrzesik
Copy link
Contributor Author

Sorry for the trouble @perrin4869 - hopefully this fix will address this problem 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants