Skip to main content

max / audiofiles

Fix percent_decode swallowing the next escape on an invalid sequence The OAuth callback URL decoder consumed two bytes after every '%' even when the escape was invalid, so a '%' that began the *following* escape got eaten ("%%41" produced the literal "%%41" instead of "%A"). Rewritten with a peekable iterator that only consumes bytes it has validated; invalid sequences emit just the proven-bad bytes and leave the rest for the next iteration. Fail-closed and localhost-OAuth-only, but now correct. Added the first tests for the function. (Sync minor) Tests: audiofiles-sync 52 green (3 new). clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-15 23:34 UTC
Commit: 9964a5abc5a1c80eed8c86e56610c95071eb9868
Parent: f057fbd
1 file changed, +58 insertions, -22 deletions
@@ -168,32 +168,39 @@ pub fn start_auth(client: &SyncKitClient) -> Result<AuthSession> {
168 168
169 169 /// Decode percent-encoded UTF-8 strings (RFC 3986).
170 170 /// Passes through invalid sequences unchanged.
171 + ///
172 + /// On an invalid escape, only the bytes actually proven invalid are emitted
173 + /// literally — a peekable iterator leaves the rest unconsumed, so a `%` that
174 + /// begins the *next* escape isn't swallowed (`"%%41"` decodes to `"%A"`, not
175 + /// the literal `"%%41"`).
171 176 fn percent_decode(input: &str) -> String {
172 177 let mut bytes = Vec::with_capacity(input.len());
173 - let mut chars = input.bytes();
178 + let mut chars = input.bytes().peekable();
174 179 while let Some(b) = chars.next() {
175 - if b == b'%' {
176 - let hi = chars.next();
177 - let lo = chars.next();
178 - if let (Some(hi), Some(lo)) = (hi, lo) {
179 - if let (Some(h), Some(l)) = (hex_val(hi), hex_val(lo)) {
180 - bytes.push(h << 4 | l);
181 - continue;
182 - }
183 - // Invalid hex pair — emit literally
184 - bytes.push(b'%');
185 - bytes.push(hi);
186 - bytes.push(lo);
187 - } else {
188 - bytes.push(b'%');
189 - if let Some(hi) = hi {
190 - bytes.push(hi);
180 + match b {
181 + b'%' => match chars.peek().copied() {
182 + Some(hi_byte) if hex_val(hi_byte).is_some() => {
183 + chars.next(); // consume the high nibble
184 + match chars.peek().copied() {
185 + Some(lo_byte) if hex_val(lo_byte).is_some() => {
186 + chars.next(); // consume the low nibble
187 + // Both nibbles validated above.
188 + bytes.push((hex_val(hi_byte).unwrap() << 4) | hex_val(lo_byte).unwrap());
189 + }
190 + // Valid high nibble but no valid low nibble: emit '%' and
191 + // the high byte literally, leaving the rest unconsumed.
192 + _ => {
193 + bytes.push(b'%');
194 + bytes.push(hi_byte);
195 + }
196 + }
191 197 }
192 - }
193 - } else if b == b'+' {
194 - bytes.push(b' ');
195 - } else {
196 - bytes.push(b);
198 + // '%' not followed by a hex digit: emit it literally and leave the
199 + // next byte for the next iteration (it may start a real escape).
200 + _ => bytes.push(b'%'),
201 + },
202 + b'+' => bytes.push(b' '),
203 + other => bytes.push(other),
197 204 }
198 205 }
199 206 String::from_utf8(bytes).unwrap_or_else(|e| String::from_utf8_lossy(e.as_bytes()).into_owned())
@@ -207,3 +214,32 @@ fn hex_val(b: u8) -> Option<u8> {
207 214 _ => None,
208 215 }
209 216 }
217 +
218 + #[cfg(test)]
219 + mod tests {
220 + use super::*;
221 +
222 + #[test]
223 + fn percent_decode_valid_escapes() {
224 + assert_eq!(percent_decode("a%20b"), "a b");
225 + assert_eq!(percent_decode("%41%42%43"), "ABC");
226 + assert_eq!(percent_decode("a+b"), "a b");
227 + assert_eq!(percent_decode("plain"), "plain");
228 + }
229 +
230 + #[test]
231 + fn percent_decode_invalid_escape_does_not_eat_next_escape() {
232 + // A bare '%' (invalid) must not swallow the '%' that starts the real
233 + // escape: "%%41" is a literal '%' followed by "%41" ('A').
234 + assert_eq!(percent_decode("%%41"), "%A");
235 + // Valid high nibble, no valid low nibble: emit '%' + high byte literally,
236 + // and still decode the following escape.
237 + assert_eq!(percent_decode("%4%41"), "%4A");
238 + }
239 +
240 + #[test]
241 + fn percent_decode_trailing_percent_is_literal() {
242 + assert_eq!(percent_decode("abc%"), "abc%");
243 + assert_eq!(percent_decode("abc%4"), "abc%4");
244 + }
245 + }