Fix concatenation of audio captcha wav files #3350
Conversation
|
Thanks! I'll mark this as draft until we get the captcha stuff merged. |
|
Captcha is merged now. |
crates/api/src/lib.rs
Outdated
| let (header, samples) = wav::read(&mut cursor) | ||
| .expect("Unable to parse wav file"); | ||
| any_header = Some(header); | ||
| concat_samples.extend(samples.as_sixteen().expect("Expected 16-bit samples")); |
There was a problem hiding this comment.
You shouldnt use expect as this can crash.
There was a problem hiding this comment.
I can change it to propagate errors instead. Should I have the function return a Result, or just an empty string on failure as before?
There was a problem hiding this comment.
OK, take a look - I restructured it so that it will log errors and return an empty string if something goes wrong
There was a problem hiding this comment.
It should definitely return a result so that the client knows something went wrong. Checking for empty string would be very awkward.
There was a problem hiding this comment.
OK, fixed so that captcha_as_wav_base64 returns a result. I changed GetCaptcha to log the error and populate an empty string for wav, because that way it doesn't crash and break the png captcha too. In practice the client handles that gracefully and doesn't display the audio captcha in that case.
Maybe a follow-up would be to change the CaptchaResponse struct so that it's optional. Would that be worth it?
crates/api/src/lib.rs
Outdated
| @@ -22,18 +24,26 @@ pub trait Perform { | |||
| } | |||
|
|
|||
| /// Converts the captcha to a base64 encoded wav audio file | |||
| pub(crate) fn captcha_as_wav_base64(captcha: &Captcha) -> String { | |||
| pub(crate) fn captcha_as_wav_base64(captcha: &Captcha) -> Result<String, Box<dyn std::error::Error>> { | |||
There was a problem hiding this comment.
Look at the error type for the other functions in this file.
There was a problem hiding this comment.
OK, switched to LemmyError and added a translation: LemmyNet/lemmy-translations#76
| @@ -33,7 +34,10 @@ impl Perform for GetCaptcha { | |||
|
|
|||
| let png = captcha.as_base64().expect("failed to generate captcha"); | |||
|
|
|||
| let wav = captcha_as_wav_base64(&captcha); | |||
| let wav = captcha_as_wav_base64(&captcha).unwrap_or_else(|err| { | |||
There was a problem hiding this comment.
Look at how errors are handled elsewhere in similar files.
Nutomic
force-pushed
the
fix_audio_captcha_append
branch
from
7a5b203 to
322d84b
Compare
|
Confirmed working, thanks! |
crates/api/src/lib.rs
Outdated
| } | ||
| let _ = wav::write(any_header.unwrap(), | ||
| &wav::BitDepth::Sixteen(concat_samples), | ||
| &mut output_buffer); |
There was a problem hiding this comment.
Are you ignoring an error here? Might have to put ? instead.
Also you need to run cargo +nightly fmt --all
There was a problem hiding this comment.
Ah, good catch. Handled that error too, and formatted.
Head branch was pushed to by a user without write access
|
You have an unused import |
Fixed |
|
Thanks! |
This is for after the captcha code is merged again. No rush.
The existing code doesn't work, it tries to concatenate the raw bytes of the wav files, which results in only the first letter being played and the rest ignored.
Please let me know if you'd prefer different patterns or more error checking, or if you'd prefer to do it without introducing a dependency. Happy to adjust the code to your preferences.