-
Notifications
You must be signed in to change notification settings - Fork 324
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
uncaught exceptions in audio captcha generation #238
Comments
Hello, thanks for opening this issue.
I suspect this is due to #196, i.e. sox producing wav files at different sample rates and then being unable to merge them. If you'd like to invest some time into investigating the real source of this and submitting a pull request, I'd really appreciate this.
I mean, I agree, but in the end I'm more concerned by the user experience, so I'd rather fix the underlying problem. In the end the user will get a 404 instead of a 500 when requesting the audio, and both suck in pretty much the same way. |
awesome! we're going to deploy this for testing soon, thanks, you can follow our progress in https://gitlab.torproject.org/tpo/web/donate-neo/-/issues/142 but i'll try to report back here specifically soon! part of the challenge is this is intermittent, so it's hard to tell whether it's fixed... but i'll do my best! |
we're currently running 0613733 and we're still getting FileNotFound exceptions, here's an example:
update: fix copy-paste, previous version was from before the fix, but the error is mostly unchanged. |
Right, I suppose that what is happening is that for some reason the URL to the audio file is being requested multiple times at a very short interval. The way audio files are handled is
Now, your errors all happen at steps 4, 5 and 6, which probably means that a second process is serving another request to the same url. The second process generates the files (steps 1, 2, 3) but the first process deletes them (steps 4, 5, 6) before they can be deleted / renamed / read again. The underlying issue is that each process should have its own temporary directory to avoid one process deleting the other's files, but that's not the case today. As a workaround, 1c92409 adds a random postfix to the three filenames generated in the first three steps, so that each process has their own set of files. Could you please test this commit? |
Hi!
I don't want to be that guy, but (now, i know, you'd need like half a billion parallel requests to hit that collision, but i always feel seeing
will do!
note that we're using multithreading here, so having "processes" in their own directories wouldn't fix this. in general though, it seems we're fixing this the wrong way: if the files already exist, why not just reuse them or wait for another (async?) process to complete the generation, instead of rebuilding the files? |
No that's a good point, fixed in b7abe2e
Yes I was thinking of using tempfile.TemporaryDirectory which ought to be threadsafe and also handles automatic deletion when used as a context manager. This way every thread should get their own copy.
Because repetition attacks. This is precisely why we're injecting noise into the generated wav file, otherwise one cleartext will generate the same output sound file when requested multiple times, which will give an attacker agency to create a rainbow table of file hashes. And also, it makes the architecture of the audio generation view utterly complicated for no good reason at all IMO. 🤷 |
In our environment, we're getting spurious exceptions like this:
it's unclear why this is happening, we suspect it might be related with multithreading issues in the codebase and lack of locking. we also have issues with old captcha files not being cleaned up for example.
but exactly why this is happening is somewhat besides the point: such failures should at least be handled better than a 500 error with a backtrace. we should add exception handling in (e.g.) this code:
django-simple-captcha/captcha/views.py
Lines 165 to 199 in 153cfd9
so that we can somewhat recover more gracefully when that happens. if at least this was a custom exception from this module (instead of a fairly generic OSError/FileNotFoundError, we could handle it (or ignore it, at least for now) differently than other exceptions...
i'd be happy to propose a PR to at least do the latter if there's an opening here.
Note that this issue is tracked at https://gitlab.torproject.org/tpo/web/donate-neo/-/issues/142 where you can also see the entire source code for this project.
The text was updated successfully, but these errors were encountered: