Address siGit Code Review
paydii committed
Aug 29, 2026 at 21:19 UTC
cc1e17be090427a5c4a880f3eac50e5703a2cf8c
2 files changed
+43
-19
.github/workflows/ci.yml
+1
-1
@@ -78,7 +78,7 @@ jobs:
78
key: clippy-${{ matrix.target }}
79
80
- name: Clippy
81
- run: cargo clippy --tests --target ${{ matrix.target }} -- -D warnings
81
+ run: cargo clippy --locked --tests --target ${{ matrix.target }} -- -D warnings
82
83
test:
84
name: Test — ${{ matrix.name }}
src/browser_auth.rs
+42
-18
@@ -205,7 +205,7 @@ fn wait_for_code(listener: TcpListener, expected_state: &str) -> Result<String,
205
.set_nonblocking(true)
206
.map_err(|error| format!("could not watch for the browser: {error}"))?;
207
208
- loop {
208
+ 'connections: loop {
209
if std::time::Instant::now() >= deadline {
210
return Err("timed out waiting for the browser".to_string());
211
}
@@ -218,12 +218,37 @@ fn wait_for_code(listener: TcpListener, expected_state: &str) -> Result<String,
218
}
219
Err(error) => return Err(format!("could not accept the browser: {error}")),
220
};
221
- stream.set_nonblocking(false).ok();
222
- stream.set_read_timeout(Some(Duration::from_secs(5))).ok();
223
-
224
- // The request line is all we need, and it arrives in the first packet.
221
+ if std::time::Instant::now() >= deadline {
222
+ return Err("timed out waiting for the browser".to_string());
223
+ }
224
+ stream
225
+ .set_nonblocking(false)
226
+ .map_err(|error| format!("could not read the browser callback: {error}"))?;
227
+ let remaining = deadline.saturating_duration_since(std::time::Instant::now());
228
+ stream
229
+ .set_read_timeout(Some(remaining.min(Duration::from_secs(5))))
230
+ .map_err(|error| format!("could not read the browser callback: {error}"))?;
231
+
232
+ // Read through the end of the request line; a TCP read may return a
233
+ // partial line when the request spans multiple packets.
234
let mut buffer = [0_u8; 4096];
226
- let read = stream.read(&mut buffer).unwrap_or(0);
235
+ let mut read = 0;
236
+ while read < buffer.len() {
237
+ match stream.read(&mut buffer[read..]) {
238
+ Ok(0) => break,
239
+ Ok(count) => {
240
+ read += count;
241
+ if buffer[..read].windows(2).any(|window| window == b"\r\n") {
242
+ break;
243
+ }
244
+ }
245
+ Err(error) => {
246
+ respond(&mut stream, "Could not read the sign-in callback.");
247
+ log::debug!("could not read browser callback: {error}");
248
+ continue 'connections;
249
+ }
250
+ }
251
+ }
252
let request = String::from_utf8_lossy(&buffer[..read]);
253
let Some(target) = request
254
.lines()
@@ -328,21 +353,15 @@ async fn exchange(code: &str, verifier: &str, redirect_uri: &str) -> Result<Stri
353
.filter(|token| !token.trim().is_empty())
354
.ok_or_else(|| "siGit Code Cloud returned no access token".to_string())?;
355
331
- // Store before the profile lookup: the token is the thing worth keeping,
332
- // and the email is only there to label it.
356
+ // The token is the thing worth keeping, and the email is only there to
357
+ // label it. Store once so a profile lookup failure does not leave partial
358
+ // state on disk.
359
+ let email = fetch_email(token).await;
360
credentials::store(&Credentials {
361
access_token: token.to_string(),
335
- email: None,
362
+ email: email.clone(),
363
})?;
364
338
- let email = fetch_email(token).await;
339
- if email.is_some() {
340
- credentials::store(&Credentials {
341
- access_token: token.to_string(),
342
- email: email.clone(),
343
- })?;
344
- }
345
-
365
Ok(email.unwrap_or_else(|| "(unknown)".to_string()))
366
}
367
@@ -415,7 +434,7 @@ fn percent_decode(value: &str) -> String {
434
let mut index = 0;
435
while index < bytes.len() {
436
match bytes[index] {
418
- b'%' if index + 2 < bytes.len() => {
437
+ b'%' if index + 3 <= bytes.len() => {
438
let hex = std::str::from_utf8(&bytes[index + 1..index + 3]).unwrap_or("");
439
match u8::from_str_radix(hex, 16) {
440
Ok(byte) => {
@@ -494,4 +513,9 @@ mod tests {
513
assert_eq!(pairs[0], ("code".to_string(), "abc/def".to_string()));
514
assert_eq!(pairs[1], ("state".to_string(), "x y".to_string()));
515
}
516
+
517
+ #[test]
518
+ fn percent_decode_handles_escape_at_end() {
519
+ assert_eq!(percent_decode("code%2F"), "code/");
520
+ }
521
}