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

(bugbash) fixes potential file inclusion via variable. #490

Merged
merged 3 commits into from
Jun 29, 2021

Conversation

rahulgrover99
Copy link
Contributor

Signed-off-by: Rahul Grover [email protected]

Description

gosec was showing the following warning "Potential file inclusion via variable". This is because we are trying to open files using dynamic variables. Hence, I've cleaned the bad file paths using filepath.Clean()

Fixes:
G304 Potential file inclusion via variable

@markjacksonfishing
Copy link
Contributor

Thank you for this pr and welcome to the community

@codecov
Copy link

codecov bot commented May 4, 2021

Codecov Report

Merging #490 (0b23ac8) into master (1a02f92) will not change coverage.
The diff coverage is 16.66%.

❗ Current head 0b23ac8 differs from pull request most recent head ec0d9ff. Consider uploading reports for the commit ec0d9ff to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #490   +/-   ##
=======================================
  Coverage   32.70%   32.70%           
=======================================
  Files          44       44           
  Lines        3137     3137           
=======================================
  Hits         1026     1026           
  Misses       2019     2019           
  Partials       92       92           
Impacted Files Coverage Δ
cmd/tink-cli/cmd/hardware/push.go 0.00% <0.00%> (ø)
cmd/tink-cli/cmd/template/create.go 0.00% <0.00%> (ø)
cmd/tink-worker/internal/worker.go 0.00% <0.00%> (ø)
grpc-server/grpc_server.go 0.00% <0.00%> (ø)
workflow/template_validator.go 82.08% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a02f92...ec0d9ff. Read the comment docs.

Copy link
Contributor

@markjacksonfishing markjacksonfishing left a comment

Choose a reason for hiding this comment

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

We'll need better test coverage before we can approve and merge. Let me know if you need any help with that.

@rahulgrover99
Copy link
Contributor Author

Oh, I thought the changes won't require any test coverage. Can you please point me to where I could get started for this?

@markjacksonfishing
Copy link
Contributor

We'll need better test coverage before we can approve and merge. Let me know if you need any help with that.

@rahulgrover99 take a look at the current set of test and see if you can add

Copy link
Member

@nshalman nshalman left a comment

Choose a reason for hiding this comment

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

All the Codecov complaints are warnings, and this seems like a good change from a security perspective so I will be merging it. Thank you!

@nshalman nshalman dismissed markjacksonfishing’s stale review June 29, 2021 15:58

The Codecov complaints are only warnings and this seems like a valuable change to take now.

@nshalman nshalman merged commit 7f829f5 into tinkerbell:master Jun 29, 2021
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