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 zip-slip noise in tests #3586

Merged
merged 1 commit into from
Feb 9, 2024
Merged

fix zip-slip noise in tests #3586

merged 1 commit into from
Feb 9, 2024

Conversation

olegbespalov
Copy link
Contributor

@olegbespalov olegbespalov commented Feb 9, 2024

What?

This attempts to fix the "noise" from the security tab (CodeQL reported).

Why?

Fixing such cases helps us focus on the right things.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

@olegbespalov olegbespalov self-assigned this Feb 9, 2024
@olegbespalov olegbespalov requested a review from a team as a code owner February 9, 2024 08:09
@olegbespalov olegbespalov requested review from mstoykov and joanlopez and removed request for a team February 9, 2024 08:09
@codecov-commenter
Copy link

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (1b478bb) 72.96% compared to head (00f789b) 72.94%.

❗ Current head 00f789b differs from pull request most recent head 4e2c93d. Consider uploading reports for the commit 4e2c93d to get more accurate results

Files Patch % Lines
lib/testutils/untar.go 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3586      +/-   ##
==========================================
- Coverage   72.96%   72.94%   -0.02%     
==========================================
  Files         280      280              
  Lines       20949    20952       +3     
==========================================
- Hits        15285    15283       -2     
- Misses       4693     4697       +4     
- Partials      971      972       +1     
Flag Coverage Δ
macos 72.88% <50.00%> (?)
ubuntu 72.89% <50.00%> (-0.02%) ⬇️
windows 72.79% <50.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM in general.

I don't think we have ever been fulnerable to this as this more or less only goes in memory, but I guess some shenanigans were still possible with ../https/hostname.com/path .. but I still have no idea why that will be useful 🤷

I personally do not want to touch code that plays with filepath as unfortunately it is quite ... funky with us having paths from windows but running on linux and vice-versa.

This seems to not break any tests, and given that filepath.Join already does Clean (which is by far the thing that IIRC breaks most stuff) - hopefully nothing new will break 🤞

return errors.New("tar file contains non-local file names")
}

target := filepath.Join(destination, filepath.Clean(fileName))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Pretty sure there is no need to call Clean before Join

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having no filepath.Clean here will trigger golang-ci lint (see the omitting lines).

@olegbespalov
Copy link
Contributor Author

olegbespalov commented Feb 9, 2024

First, I share with you the opinion that the code is not vulnerable for the reasons you mention (like in memory).

but I still have no idea why that will be useful 🤷

Still, the primary purpose of doing this for me is to fix the Security tab noise. I'd like to have these 0 issues as the standard state. And if something appears there, it's a signal for checking. And for sure, I don't want to make calculations like there are two known, so when it will be 3, I need to check. Using the simple flow, 0, or anything else is better.

Now, we have two options to reach this state: omit it inline or try fixing it. In choosing between these two options, I feel that it's worth trying to go with the fixing first instead of just omitting inline.

@olegbespalov olegbespalov added this to the v0.50.0 milestone Feb 9, 2024
@olegbespalov olegbespalov merged commit aabf8a1 into master Feb 9, 2024
24 checks passed
@olegbespalov olegbespalov deleted the chore/fix-zip-slip branch February 9, 2024 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants