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 .lib false positives in binary artifacts #1879

Merged
merged 5 commits into from
May 3, 2022

Conversation

laurentsimon
Copy link
Contributor

Fix .lib false positives in binary artifacts
no breaking changes

Fix .lib false positives in binary artifacts

closes #1868

@codecov
Copy link

codecov bot commented May 2, 2022

Codecov Report

Merging #1879 (4bb8c93) into main (2cb6541) will increase coverage by 3.05%.
The diff coverage is 52.94%.

@@            Coverage Diff             @@
##             main    #1879      +/-   ##
==========================================
+ Coverage   51.42%   54.47%   +3.05%     
==========================================
  Files          79       79              
  Lines        6693     6709      +16     
==========================================
+ Hits         3442     3655     +213     
+ Misses       3024     2820     -204     
- Partials      227      234       +7     

Copy link
Member

@naveensrinivasan naveensrinivasan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@laurentsimon laurentsimon enabled auto-merge (squash) May 2, 2022 17:12
@laurentsimon laurentsimon force-pushed the bug/printable branch 2 times, most recently from 47dc22d to c95a257 Compare May 2, 2022 20:59
@laurentsimon laurentsimon temporarily deployed to integration-test May 3, 2022 16:19 Inactive
@laurentsimon laurentsimon temporarily deployed to integration-test May 3, 2022 16:20 Inactive
@azeemshaikh38 azeemshaikh38 disabled auto-merge May 3, 2022 16:36
@github-actions
Copy link

github-actions bot commented May 3, 2022

Integration tests success for
[98dbc29]
(https://github.com/ossf/scorecard/actions/runs/2265020234)

@github-actions
Copy link

github-actions bot commented May 3, 2022

Integration tests success for
[ef246fc]
(https://github.com/ossf/scorecard/actions/runs/2265028847)

Copy link
Contributor

@azeemshaikh38 azeemshaikh38 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do the sanity test iff exists1 == false. I would trust the magic number inference more than the character search logic we have here and we can fallback to it if the magic number inference fails.

if exists1 {
  // Binary file
}

if !isText(content) && exists2 {
  // Binary file
}

As a follow up, we could consider using common parsers (like Makefile, YAML, JSON etc.) to check whether the file has text content. Basically, if any parsers are able to successfully parse the file we can safely infer that the file is text only.

@laurentsimon
Copy link
Contributor Author

Let's do the sanity test iff exists1 == false. I would trust the magic number inference more than the character search logic we have here and we can fallback to it if the magic number inference fails.

if exists1 {
  // Binary file
}

if !isText(content) && exists2 {
  // Binary file
}

done.

@laurentsimon laurentsimon temporarily deployed to integration-test May 3, 2022 19:39 Inactive
@laurentsimon laurentsimon enabled auto-merge (squash) May 3, 2022 19:39
@github-actions
Copy link

github-actions bot commented May 3, 2022

Integration tests success for
[4bb8c93]
(https://github.com/ossf/scorecard/actions/runs/2265963886)

Copy link
Contributor

@azeemshaikh38 azeemshaikh38 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

checks/raw/binary_artifact.go Show resolved Hide resolved
@laurentsimon laurentsimon disabled auto-merge May 3, 2022 20:29
@laurentsimon laurentsimon enabled auto-merge (squash) May 3, 2022 20:30
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.

BUG Binary Artifacts False positive scripts/Makefile.lib
3 participants