Initial security fixes #758
Conversation
This fixes 33 unhandled errors and brings down the ammount of reported issues in gosec to 36. This means that no unhandled error are present in the codebase.
This fixes four permission related issues noted by gosec. We are now down to 32 issues reported.
app/settings_desktop.go
Outdated
| err := watcher.Remove(path) | ||
| if err != nil { | ||
| fyne.LogError("Failed to stop watching settings file", err) | ||
| return |
There was a problem hiding this comment.
This is the reason that the app test is failing. Removing the return call seems to fix things, but I have a feeling that something might be wrong with the underlying code because it looks like we always get an error saying Fyne error: Failed to stop watching settings file2020/03/22 09:20:41 Cause: can't remove non-existent inotify watch.
There was a problem hiding this comment.
I think you are right this return is not correct. If we can't stop watching a deleted file (entirely probable) we should return to trying again.
Not sure why this is only an issue on this PR though - the others are working as expected. Are you certain nothing has changed?
There was a problem hiding this comment.
I think we got a bit of a misunderstanding regarding what I meant with the comment above. I added that return statement in my PR to handle the error and the reason that it doesn't cause errors in other Pull Requests is because of that. What I tried saying was that it throws an error here even though I don't think it should and we can see it now because we check the error. 😉
There was a problem hiding this comment.
Oh, OK. How do you propose fixing this then? We can't merge with a new break ;)
There was a problem hiding this comment.
I honestly have no idea that might be causing this. Do you want me to just log the error and not return or just not handle the error at all and leave it for another day?
There was a problem hiding this comment.
Let me know what you think after reading fsnotify/fsnotify#40 and fsnotify/fsnotify#238
There was a problem hiding this comment.
The issue seems to be specific to Windows and Linux (would be amazed if this wasn’t the same for BSD too). I suggest that we make sure to only throw errors on Darwin platforms.
There was a problem hiding this comment.
Pushed new commits with the fix. Might need testing on MacOS, but tests are not failing for me anymore at least :)
There was a problem hiding this comment.
Looks like MacOS tests on travis still fails. Looks like we always get an error no matter the case. I say that we ignore this for now and get back to it when it is fixed upstream.
This comment has been minimized.
This comment has been minimized.
My initial guess is that this might be an issue on BSD too, let's only care about the error if we are on darwin platforms.
Fsnotify seems to always throw out false positives.
|
Just restarting the second windows build to see what's going on. It may be that there's a legitimate failure here - can you see if it relates to the discussion above? |
Are you possibly commenting on the wrong PR? All the tests seem to be completing now :) |
|
I have done testing with the 2020/03/27 20:00:57 Fyne error: Preferences load error:
2020/03/27 20:00:57 Cause: mkdir /data: permission denied
2020/03/27 20:00:57 At: /home/jacob/fyne/app/preferences.go:67
2020/03/27 20:01:00 Fyne error: Failed on saving preferences
2020/03/27 20:01:00 Cause: mkdir /data: permission denied
2020/03/27 20:01:00 At: /home/jacob/fyne/app/preferences.go:96Will probably need to look at this more, but it might not be entirely relevant because the preferences code does not work on mobile right? |
|
Yeah you are probably right. If you could put that error log in a “preferences api not working on mobile” ticket we can reference that here and move on :). |
|
Here you go: #779 :) |
Description:
This is a big PR aimed at fixing a lot of potential issues that are being reported by gosec analysing. the issues fixed here are mostly related to error handling, but there are also four fixes for file permissions.
We are now down from 69 to 33 issues on develop.
Partial fix for #742
Checklist:
Note to anyone reviewing this:
Please look at the error handling and say if we need to return or not. There might be places where we don't need to return when an error occurs (would not be possible to exploit) or the other way around where it might be strictly necessary to actually return.