ossf / scorecard

OpenSSF Scorecard - Security health metrics for Open Source
https://scorecard.dev
Apache License 2.0
4.25k stars 460 forks source link

šŸ› Use direct endpoint instead of search to find repository URL from npm database #4118

Closed aklevans closed 3 weeks ago

aklevans commented 1 month ago

What kind of change does this PR introduce?

This PR is a bug fix. When finding the github repository URL for an npm package, the application uses the GETĀ·/{package}/latest endpoint instead of the GETĀ·/{package}

What is the current behavior?

The application finds the github URL of an npm package by using the search functionality. If a user incorrectly enters a github URL instead of an npm package name, the user will not receive feedback about their mistake and the application will run the checks on the first package that the search finds.

What is the new behavior (if this is a feature change)?**

When a user enters a github URL instead of an npm package name, an error message is displayed saying that the npm package could not be found.

Which issue(s) this PR fixes

Fixes #3166.

Special notes for your reviewer

Does this PR introduce a user-facing change?

NO

NONE
raghavkaul commented 1 month ago

Thanks for the contribution! Could you add some unit tests that expect an error when a GitHub/NPM URL is passed to the --npm flag instead of a project name? Even better to test the projects from #2144 and #3166, which silently score the wrong projects because of the inexact name match.

aklevans commented 1 month ago

I added unit tests for #3166 and #2441 with mock http responses similar to the existing tests

raghavkaul commented 1 month ago

Still getting linter issues, try make fix-linter.

For package_manager_test.go, can we make the diff smaller by only returning the parts of the HTTP/JSON response in args.result that we'll actually end up parsing?

aklevans commented 1 month ago

I don't think fix-linter is working correctly on my machine. It just adds line breaks between every single line. Any reason this may happen?

raghavkaul commented 1 month ago

Hm, that's strange that --fix has that behavior, I think you'll need to fix those issues manually then. It looks like some lints were fixed but there are still some issues. Make sure that make all passes, because our CI requires that for merge.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 72.54902% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 60.12%. Comparing base (6815161) to head (d322d70). Report is 20 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #4118 +/- ## ========================================== - Coverage 66.06% 60.12% -5.95% ========================================== Files 226 214 -12 Lines 16291 15621 -670 ========================================== - Hits 10763 9392 -1371 - Misses 4854 5537 +683 - Partials 674 692 +18 ```
spencerschrock commented 1 month ago

I don't think fix-linter is working correctly on my machine. It just adds line breaks between every single line. Any reason this may happen?

Can you share a little bit about your environment? You're running the linter with make check-linter? The double-space the whole file approach should be reverted.

aklevans commented 1 month ago

I think the issue was stemming from the fact I was working with windows. I have switched to Linux and I believe it is working now.

spencerschrock commented 4 weeks ago

Thanks. I'll update the branch later and get this merged in. The DCO bot is seemingly not running, so can't at the moment.