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

unhandled FileNotFoundError when files have been manually deleted from disk #4787

Closed
redshiftzero opened this issue Sep 10, 2019 · 7 comments · Fixed by #5549 or #5573
Closed

unhandled FileNotFoundError when files have been manually deleted from disk #4787

redshiftzero opened this issue Sep 10, 2019 · 7 comments · Fixed by #5549 or #5573
Labels
app help wanted Issues we would definitely appreciate volunteer help with QA: Release

Comments

@redshiftzero
Copy link
Contributor

Description

In 1.0.0, we introduced logic for an admin to check if there are files that have been deleted on disk but not had their corresponding db rows deleted. Right now in the JI, an unhandled exception will occur in the JI if a user tries to download all submissions and there are any missing files on disk.

Steps to Reproduce

  1. Submit a file as a source
  2. Delete that file on the server at the filesystem level
  3. Now download the file from the journalist interface (e.g. Select All -> Download)

Expected Behavior

All files on the server are downloaded.

Actual Behavior

500 occurs and FileNotFoundError is unhandled

Comments

I don't think this the highest priority since this is an edge case but it might be nice to add exception handling to flash a message instructing the user to get their admin to run the cleanup tool (it's easy for an admin to miss a notification in an OSSEC alert).

@redshiftzero redshiftzero added the help wanted Issues we would definitely appreciate volunteer help with label Nov 7, 2019
@zenmonkeykstop
Copy link
Contributor

There was a case with this on the SI as well (fix in for 1.6.0) - would be cool to look for other potential cases as well.

@emkll
Copy link
Contributor

emkll commented Oct 7, 2020

Looks like this issue is not completely addressed: While #5549 addresses the issue for deletions, it does not address this issue for downloads. Steps to reproduce are as follows:

  1. Go to source interface, submit two messages
  2. SSH into app server, go to /var/lib/securedrop/store and delete one of the messages
  3. Go to Journalist Interface and try to download the deleted message
  4. Observe error 500

We may need to wrap the send_file call with try/except in

@emkll emkll reopened this Oct 7, 2020
@emkll emkll removed the priority/low label Oct 7, 2020
@DrGFreeman
Copy link
Contributor

DrGFreeman commented Oct 8, 2020

This issue is flagged with "Help Wanted". I'd be interested in working on it if no one else is.

From a UX perspective, when the journalist deletes a message, they don't have to know that the corresponding file was missing. However if the file is missing when they try to download it, they will expect some sort of feedback to tell them the message cannot be downloaded.

I see different possible scenarios:

Missing files cases:

A. One of multiple message files is missing
B. All of the message files from a source are missing (individual files or the whole source folder)

Journalist actions:

  1. From /, clicks "Download" unread against the source or selects the source only and clicks Download or Download Unread (zip)
  2. From / selects more than one source and clicks Download or Download Unread (zip)
  3. From /col/<filesystem_id>, clicks on the message with missing file (gpg)
  4. From /col/<filesystem_id>, selects the message with missing file and clicks Download Selected (zip)
  5. From /col/<filesystem_id>, selects multiple/all messages and clicks Download Selected (zip)

In cases A1, A2, B2 and A5, the zip file can be produced with the remaining available message files.
In cases A3 and B3 the gpg file cannot be downloaded.
In cases A4, B1, B4 and B5, the zip file cannot be produced/downloaded.

Opinions on the way to handle these scenarios from a UX prespective would be helpful.

Thx.

@zenmonkeykstop
Copy link
Contributor

Hey @DrGFreeman thanks for the analysis and offer of help! Would be very happy to have your involvement on this.

For the deletion case addressed in 1.6.0, it originally summarized any non-fatal errors in an additional flash message, after deletions were complete. That flash message was dropped to avoid reopening translations late in the release cycle, but will probably be added again for next release. A similar mechanism would probably work OK for submissions IMO but it would be cool to get @ninavizz' or others' opinions. (In A3 and B3, there would just be the error flash message.)

One other thing to bear in mind is that these errors would occur in the context of an inconsistent file store on the server, so there should be:

  • logging on the JI to reflect this, which will trigger OSSEC alerts for the admins
  • error messaging that directs journalists to get their admins involved (in case they miss the OSSEC alerts)

@eloquence
Copy link
Member

eloquence commented Oct 8, 2020

Thanks from my end as well @DrGFreeman for the awesome breakdown :)

It's important to note that this issue is not expected to arise on production systems, and we have no report of journalists or admins seeing these 500 errors. If it does arise, it indicates a serious issue with the file store, and possible tampering. It really does require admin involvement.

We discussed this a bit at today's tech meeting and agreed that we can start with the simple possible improvement for now upon any cases where users currently see a 500 error, and that would be a catch-all error message. It would be great to get Nina's input on the wording here. @ninavizz, let's discuss a bit, I can get you up to speed. For now, DrG, you can just put in a placeholder error.

What is the current behavior in collections where only some files in a collection are available? Do some of those cases you listed currently work, or do they all throw a 500?

@DrGFreeman
Copy link
Contributor

What is the current behavior in collections where only some files in a collection are available? Do some of those cases you listed currently work, or do they all throw a 500?

All cases above throw a 500.

I will start working on a solution with a generic flash message for now + logging of the errors.

@eloquence
Copy link
Member

That sounds great, thank you @DrGFreeman! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app help wanted Issues we would definitely appreciate volunteer help with QA: Release
Projects
None yet
5 participants