Replace insecure temporary file creation in chem/molecular_data.py#1243
Conversation
…ata.save Replaced the use of `uuid.uuid4()` for temporary file names with `tempfile.NamedTemporaryFile`. This ensures that temporary files are created with restricted permissions (0600) and in a secure manner. The temporary file is now created in the same directory as the target file to ensure an atomic `shutil.move`. Removed the now-unused `uuid` import.
…ta-15634551765368123518
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the use of uuid with tempfile.NamedTemporaryFile for temporary file creation within the save method of molecular_data.py. A review comment identifies a potential resource leak where temporary files might not be cleaned up if an error occurs before the file is moved, suggesting the implementation of a try...finally block for better robustness.
This also pulls out the code to write the data into a separate method, but better readability and modularity.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the save method in molecular_data.py to use tempfile.NamedTemporaryFile and extracts the HDF5 writing logic into a private method. Feedback indicates a potential TOCTOU race condition introduced by closing the temporary file before opening it with h5py, with a suggestion to pass the file object directly instead.
| tmp_file.close() | ||
| try: | ||
| with h5py.File(tmp_name, "w") as hdf5_file: |
There was a problem hiding this comment.
While using tempfile.NamedTemporaryFile is a great improvement for secure temporary file creation, closing the file and then re-opening it by name introduces a Time-of-Check to Time-of-Use (TOCTOU) race condition. An attacker could potentially replace the temporary file between the close() and h5py.File() calls, leading to writing data to an unintended file.
To prevent this, you should pass the file object tmp_file directly to h5py.File, which supports file-like objects. This avoids referencing the file by name after it has been created.
| tmp_file.close() | |
| try: | |
| with h5py.File(tmp_name, "w") as hdf5_file: | |
| try: | |
| with h5py.File(tmp_file, "w") as hdf5_file: |
1 participant
Replace the use of
uuid.uuid4()for temporary file names withtempfile.NamedTemporaryFile. This ensures that temporary files are created with restricted permissions (0600) and in a secure manner. The temporary file is now created in the same directory as the target file to ensure an atomicshutil.move.In addition, to avoid excessively nested code and to improve modularity, the code is slightly refactored to split out the data-writing steps into a separate private function.