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

phylum package returns results for nonexistent packages #351

Open
kylewillmon opened this issue May 5, 2022 · 14 comments · Fixed by #1518
Open

phylum package returns results for nonexistent packages #351

kylewillmon opened this issue May 5, 2022 · 14 comments · Fixed by #1518
Assignees
Labels
bug Something isn't working medium priority Should be handled as soon as possible

Comments

@kylewillmon
Copy link
Contributor

Running phylum package with a nonexistent package name and version still returns results.

❯ phylum package -t npm nonexistent 8675309 
                                                                                  
 Package Name:    nonexistent    Package Version:                      8675309    
 License:             Unknown    Last updated:       1969-12-31T18:00:00-06:00    
 Num Deps:                  0    Num Vulns:                                  0    
 Ecosystem:               NPM                                                     
                                                                                  

 Risk Vectors:
                                  
    Author Risk:             100  
    Engineering Risk:        100  
    License Risk:            100  
    Malicious Code Risk:     100  
    Vulnerability Risk:      100  

Expected Behavior

Some indication that the package does not exist

@kylewillmon kylewillmon added bug Something isn't working needs triage Needs to be reviewed or assigned labels May 5, 2022
@louislang louislang added medium priority Should be handled as soon as possible and removed needs triage Needs to be reviewed or assigned labels May 5, 2022
@cd-work
Copy link
Contributor

cd-work commented May 5, 2022

Is CLI the right place for this? Since the CLI basically just prints what the API responds here, it seems like this might be better handled on the API side?

@kylewillmon
Copy link
Contributor Author

Is CLI the right place for this? Since the CLI basically just prints what the API responds here, it seems like this might be better handled on the API side?

I think there are probably changes to make on both the API and the CLI side.

Looking at the debug output:

[2022-05-05T18:26:29Z DEBUG reqwest::async_impl::client] response '200 OK' for https://api.staging.phylum.io/api/v0/job/packages/npm/nonexistent/8675309
[2022-05-05T18:26:29Z DEBUG phylum_cli::commands::packages] ==> Ok(PackageStatusExtended { basic_status: PackageStatus { name: "nonexistent", version: "8675309", status: Incomplete, last_updated: 0, license: Some("Unknown"), package_score: Some(1.0), num_dependencies: 0, num_vulnerabilities: 0 }, package_type: Npm, risk_vectors: {"engineering": 1.0, "vulnerability": 1.0, "author": 1.0, "license": 1.0, "malicious_code": 1.0}, dependencies: {}, issues: [] })

The API is returning status as Incomplete, but we show no indication of that. If this were a package that was still being processed, the CLI should probably print an error saying that this dependency is still being processed and we don't have results for it yet.

The API probably should be returning a 404 here, but the CLI doesn't handle that well either (see #155).

One final note: I would expect a 404 on this endpoint for both packages that don't exist and packages that haven't been submitted yet. So the CLI message should be carefully worded to make that clear.

@cd-work
Copy link
Contributor

cd-work commented May 6, 2022

The API is returning status as Incomplete, but we show no indication of that. If this were a package that was still being processed, the CLI should probably print an error saying that this dependency is still being processed and we don't have results for it yet.

I think this is also true for the phylum analyze command, right? If so it should probably be changed everywhere.

@kylewillmon
Copy link
Contributor Author

I think this is also true for the phylum analyze command, right? If so it should probably be changed everywhere.

Yes, to an extent. For phylum analyze (and phylum history), it's more likely that partial results are useful, so it's good to display a bit more than an error message there.

@cd-work
Copy link
Contributor

cd-work commented Mar 9, 2023

Since submission is now done automatically for packages that did not exist yet, this will show the following message:

⚠️  Thank you for submitting this package. Please check back later for results.

I still think we should print that the package doesn't exist (assuming we can get that info), but it seems less troublesome than reporting the package as not having any issues.

@kylewillmon
Copy link
Contributor Author

I still think we should print that the package doesn't exist (assuming we can get that info), but it seems less troublesome than reporting the package as not having any issues.

The API will be updated in the future to return a different result for nonexistent packages. Until then, this is probably the best that can be expected.

Closing this issue since any future work will happen on the API side.

@kylewillmon
Copy link
Contributor Author

Re-opening after finding this...

> phylum package -t npm pyyaml 5.3.1
                                                                             
 Package Name:    pyyaml    Package Version:                        5.3.1    
 License:                   Last updated:       1970-01-01T00:00:00+00:00    
 Num Deps:             0    Num Vulns:                                  0    
 Ecosystem:          npm                                                     
                                                                             

 Risk Vectors:
                                  
    Total Risk:              100  
    Author Risk:             100  
    Engineering Risk:        100  
    License Risk:            100  
    Malicious Code Risk:     100  
    Vulnerability Risk:      100  

But it seems this one also fools the UI so I don't think this is actually a CLI-specific issue.

@kylewillmon kylewillmon reopened this Apr 3, 2023
@cd-work
Copy link
Contributor

cd-work commented Apr 3, 2023

Closing this issue since any future work will happen on the API side.

I don't think this is actually a CLI-specific issue.

It sounds like the status of this issue didn't really change. Are you working on it right now?

@kylewillmon
Copy link
Contributor Author

Are you working on it right now?

Just a bit of investigation. As mentioned in your previous comment, we expect a "Thank you for submitting" message for nonexistent packages.

But for some reason we don't get that with phylum package -t npm pyyaml 5.3.1. So either there is something special about that particular package or there is some case that we didn't consider before on this issue.

@cd-work
Copy link
Contributor

cd-work commented Apr 3, 2023

But for some reason we don't get that with phylum package -t npm pyyaml 5.3.1. So either there is something special about that particular package or there is some case that we didn't consider before on this issue.

The main thing I was curious about is why this issue is re-opened in CLI. There's nothing wrong with looking into this and tracking it somewhere, but it sounded like we already confirmed that this is not an issue with the CLI and I don't think that has changed?

@kylewillmon
Copy link
Contributor Author

You are correct. I just haven't thought of any better place to track it.

@maxrake
Copy link
Contributor

maxrake commented Oct 9, 2024

Is this still an issue? I don't see a -t option on the phylum package command anymore and the message Thank you for submitting this package. Please check back later for results. seems to happen as expected now.

@cd-work
Copy link
Contributor

cd-work commented Oct 9, 2024

This is still an issue, you get the behavior after trying again once you get the Thank you for submitting... message. I've confirmed this by submitting npm pyyaml 9.99.9.

@cd-work cd-work self-assigned this Oct 9, 2024
cd-work added a commit that referenced this issue Oct 10, 2024
This fixes an issue where packages would show up as passing Phylum's
analysis if they failed at any point of the pipeline.

Closes #351.
cd-work added a commit that referenced this issue Oct 10, 2024
This fixes an issue where packages would show up as passing Phylum's
analysis if they failed at any point of the pipeline.

Closes #351.
cd-work added a commit that referenced this issue Oct 10, 2024
This fixes an issue where packages would show up as passing Phylum's
analysis if they failed at any point of the pipeline.

Closes #351.
cd-work added a commit that referenced this issue Oct 10, 2024
This fixes an issue where packages would show up as passing Phylum's
analysis if they failed at any point of the pipeline.

Closes #351.
@kylewillmon
Copy link
Contributor Author

kylewillmon commented Oct 16, 2024

I think #1518 helps, but this is still an issue.

This is current main CLI against prod:

> phylum package maven androidx.collection:collection 1.4.0
 Package Name:    androidx.collection:collection    Package Version:                        1.4.0
 License:                                           Last updated:       1970-01-01T00:00:00+00:00
 Num Deps:                                     0    Num Vulns:                                  0
 Ecosystem:                                maven


 Risk Vectors:

    Total Risk:              100
    Author Risk:             100
    Engineering Risk:        100
    License Risk:            100
    Malicious Code Risk:     100
    Vulnerability Risk:      100

@kylewillmon kylewillmon reopened this Oct 16, 2024
@kylewillmon kylewillmon self-assigned this Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working medium priority Should be handled as soon as possible
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants