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

encrypt.go: Fix panic on reading empty file #485

Closed
wants to merge 1 commit into from
Closed

encrypt.go: Fix panic on reading empty file #485

wants to merge 1 commit into from

Conversation

mr-karan
Copy link

@mr-karan mr-karan commented Jul 1, 2019

Fixes a panic crash when a file is loaded without any contents.
Adds a new error FileContentsEmpty in codes.go.

Fixes #483

Fixes a panic crash when a file is loaded without any contents.
Adds a new error `FileContentsEmpty` in `codes.go`.
@codecov-io
Copy link

codecov-io commented Jul 1, 2019

Codecov Report

Merging #485 (66c9248) into develop (d61906a) will decrease coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #485      +/-   ##
===========================================
- Coverage    36.38%   36.26%   -0.12%     
===========================================
  Files           20       20              
  Lines         2718     2760      +42     
===========================================
+ Hits           989     1001      +12     
- Misses        1640     1666      +26     
- Partials        89       93       +4     
Impacted Files Coverage Δ
config/config.go 64.46% <0.00%> (-12.76%) ⬇️
stores/flatten.go 87.28% <0.00%> (-4.24%) ⬇️

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 d61906a...66c9248. Read the comment docs.

@autrilla
Copy link
Contributor

autrilla commented Jul 1, 2019

Thanks for the patch! I don't think this will catch the case where a user runs sops file.yaml, and then empties the file

@ajvb
Copy link
Contributor

ajvb commented Jul 1, 2019

Thanks @mr-karan! imo we should also make sure to add a regression test with the patch.

@mr-karan
Copy link
Author

mr-karan commented Jul 2, 2019

@autrilla Um I am not sure if I got you. When the user runs sops -e file.yaml the file contents are loaded in memory, so it won't really matter what the user does to the file after running this command.
Did you mean something else?

@ajvb On it 👍

@autrilla
Copy link
Contributor

autrilla commented Jul 2, 2019

@autrilla Um I am not sure if I got you. When the user runs sops -e file.yaml the file contents are loaded in memory, so it won't really matter what the user does to the file after running this command.
Did you mean something else?

It doesn't panic, but creating a new file with sops new_file.yaml, emptying the contents, and saving it, creates an empty file with no sops section. It should probably just error, since it doesn't make sense. And it's the same if you edit an existing file with sops existing_file.yaml and empty the contents. I think we should emit the same error in those cases.

@jvehent
Copy link
Contributor

jvehent commented Oct 11, 2019

@mr-karan Are you still making changes to this PR or should we close it?

@ajvb ajvb closed this Feb 24, 2022
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.

5 participants