Ensure lockfiles are files #1202
Conversation
This patch introduces a metadata check for lockfile and manifest paths to ensure that they're files instead of directories. Closes #1177.
There was a problem hiding this comment.
The code looks fine to me...with the exception of lockfile/src/csharp.rs, which is treated differently than the rest.
We'll want to update #1200 with the same changes made here, for the new lockfile/src/cyclonedx.rs (CC: @ejortega). A comment has been added to that review already.
The changes were tested locally and appear to work as expected. Automatic discovery of lockfiles will no longer include directories. Plus, the manual submission of a directory as a lockfile has better (less noisy) error messages:
../cli/delme 2 0 on lockfile_is_file [✘!?]
❯ l
total 0
drwxr-xr-x 4 maxrake staff 128B Aug 17 10:07 .
drwxr-xr-x 37 maxrake staff 1.2K Aug 17 10:10 ..
drwxr-xr-x 2 maxrake staff 64B Aug 17 10:07 Cargo.lock
drwxr-xr-x 2 maxrake staff 64B Aug 17 10:07 requirements.txt
../cli/delme 2 0 on lockfile_is_file [✘!?]
❯ ../target/debug/phylum parse requirements.txt
❗ Error: Is a directory (os error 21)
../cli/delme 2 0 on lockfile_is_file [✘!?]
❌ 1 ❯ ../target/debug/phylum parse Cargo.lock
❗ Error: Is a directory (os error 21)
One suggestion is to provide a bit more context in that error message by including the name of the directory that was submitted as a lockfile/manifest. Something like: Error: Cargo.lock is a directory but a file was expected.
Marking as "request changes" just to ensure the outstanding questions are answered before continuing.
| @@ -47,7 +47,7 @@ impl Parse for PackagesLock { | |||
| }; | |||
|
|
|||
| // Accept both `packages.lock.json` and `packages.<project_name>.lock.json`. | |||
| file_name.starts_with("packages.") && file_name.ends_with(".lock.json") | |||
| file_name.starts_with("packages.") && file_name.ends_with(".lock.json") && path.is_file() | |||
| } | |||
|
|
|||
| fn is_path_manifest(&self, path: &Path) -> bool { | |||
There was a problem hiding this comment.
Was the && path.is_file() check for manifest files left off here intentionally? If so, why is it treated differently?
The change ensures that lockfiles that are not files are filtered out before processing. This change aligns with the corresponding one made in the CLI (phylum-dev/cli#1202). It is done separately here because #244 (Replace lockfile detection with `phylum status`) has not been implemented yet. Plus, this allows for a fix without changing the minimum supported CLI version.
|
Closing because #1177 (comment). |
This patch introduces a metadata check for lockfile and manifest paths to ensure that they're files instead of directories.
Closes #1177.
This is in part somewhat of a request for comments, since I'm not entirely sure which approach we want to take here. In the CLI we can certainly always read the metadata, but I'm not sure if the same applies to the GitHub app?
Is this check beneficial in general, or are we maybe just introducing another point of failure that doesn't really matter in practice?
One alternative I have thought of myself is to check that the metadata is not a directory. This would still work if the file doesn't exist at all, but it does technically still try to read the path's metadata.