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: Hash detection affects all Search results #107

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mrxiaozhuox
Copy link

Description

The current Hash check in the code will directly cause the program to throw an error. When using Search, if there is even one Hash check failure, no information can be displayed.

I have fixed this issue and retained the error throw in the Install section. However, I don't think it's necessary to make the program fail to run specifically because of this issue.

The possible useful solutions I have thought of, which I can modify if you are willing to accept, are:

  1. Automatically ignore all packages with Hash check failures.
  2. Give users the right to choose, informing them that the content does not match, but they can still install it.

@mrxiaozhuox
Copy link
Author

It seems that directly adding a dirty field in the pkgs is not a good idea, so we can only wrap a layer of status or directly filter out the data that failed the hash verification.

@mrxiaozhuox
Copy link
Author

or add a new flag to decide how to control hash check failed.

@mrxiaozhuox
Copy link
Author

mrxiaozhuox commented Nov 21, 2024

I pushed a new update to fix the issue without affecting the structure.

  • for nupm search: hidden hash check failed package.
  • for nupm install: throw hash check failed error.

@amtoine
Copy link
Member

amtoine commented Nov 25, 2024

hopefully superseded by #111

@mrxiaozhuox
Copy link
Author

hopefully superseded by #111

I believe there is a substantial difference between these two PRs. Your PR seems to fix the Hash operation, but what this PR does is: if there is a Hash check failure, it does not affect the results of nupm search, because a hash check failure would directly terminate the entire program, and if there is one failure in the registry, none of the search results can be displayed.

Do you intend to fix this issue in your PR?

Thank you.

@amtoine
Copy link
Member

amtoine commented Nov 27, 2024

aah okok my bad.

well yeah, maybe once the fix lands, we can think about what should happen if a hash fails 😉

@kubouch
Copy link
Contributor

kubouch commented Nov 27, 2024

Thanks for the PR, sorry for the late reply.

I think the logic could be simplified: Inside search-package, don't throw an error at all, but just add a new column “hash_mismatch” (instead of “dirty”, I think it's more descriptive) with true/false depending on whether the hash failed or not. Then, applications that want to throw an error (nupm publish, nupm install) can do so afterward, and nupm search can just filter it out. nupm search could return the search result, but if there are some mismatches, it could print a warning, for example.

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

Successfully merging this pull request may close these issues.

3 participants