Skip to main content

max / makenotwork

ux: build OAuth redirects with real URL parsing (preserve query + fragment) ultra-fuzz Run 6 --deep (drive UX Wiring A -> A+): - R6-UX-1: redirect_with_error + issue_authorization_code routed through a shared build_oauth_redirect helper using url::Url::parse + query_pairs_mut. A registered redirect URI carrying a #fragment no longer gets ?code= naively appended after the fragment (which corrupted the callback). Falls back to separator-concat only if the URI doesn't parse. Unit tests cover plain / existing-query / fragment / loopback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-24 20:32 UTC
Commit: e2b6070de349310368d19c8c60c89cd3d85f2c5d
Parent: 9e62bf7
1 file changed, +64 insertions, -16 deletions
M server/src/routes/oauth.rs +64 -16
@@ -129,17 +129,34 @@ fn hash_token(token: &str) -> String {
129 129 hex::encode(hasher.finalize())
130 130 }
131 131
132 + /// Append OAuth response params to a redirect URI, preserving any existing query
133 + /// AND fragment. Real URL parsing places the params in the query component, so a
134 + /// registered URI carrying a `#fragment` no longer gets `?code=` naively appended
135 + /// after the fragment (which corrupts the callback — ultra-fuzz Run 6 R6-UX-1).
136 + /// Falls back to separator-concat only if the URI doesn't parse; it is validated
137 + /// and registered before reaching here, so that path is defensive.
138 + fn build_oauth_redirect(redirect_uri: &str, params: &[(&str, &str)]) -> String {
139 + match url::Url::parse(redirect_uri) {
140 + Ok(mut url) => {
141 + url.query_pairs_mut().extend_pairs(params.iter().copied());
142 + url.into()
143 + }
144 + Err(_) => {
145 + let separator = if redirect_uri.contains('?') { "&" } else { "?" };
146 + let query = params
147 + .iter()
148 + .map(|(k, v)| format!("{}={}", urlencoding::encode(k), urlencoding::encode(v)))
149 + .collect::<Vec<_>>()
150 + .join("&");
151 + format!("{redirect_uri}{separator}{query}")
152 + }
153 + }
154 + }
155 +
132 156 /// Build a `redirect_uri?error=...&state=...` response (OIDC error redirect),
133 157 /// used by `prompt=none` when interaction would otherwise be required.
134 158 fn redirect_with_error(redirect_uri: &str, state: &str, error_code: &str) -> Response {
135 - let separator = if redirect_uri.contains('?') { "&" } else { "?" };
136 - let url = format!(
137 - "{}{}error={}&state={}",
138 - redirect_uri,
139 - separator,
140 - urlencoding::encode(error_code),
141 - urlencoding::encode(state),
142 - );
159 + let url = build_oauth_redirect(redirect_uri, &[("error", error_code), ("state", state)]);
143 160 Redirect::to(&url).into_response()
144 161 }
145 162
@@ -177,14 +194,7 @@ async fn issue_authorization_code(
177 194 )
178 195 .await?;
179 196
180 - let separator = if redirect_uri.contains('?') { "&" } else { "?" };
181 - let redirect_url = format!(
182 - "{}{}code={}&state={}",
183 - redirect_uri,
184 - separator,
185 - urlencoding::encode(&code),
186 - urlencoding::encode(state_param),
187 - );
197 + let redirect_url = build_oauth_redirect(redirect_uri, &[("code", &code), ("state", state_param)]);
188 198 Ok(Redirect::to(&redirect_url).into_response())
189 199 }
190 200
@@ -1044,3 +1054,41 @@ pub fn oauth_routes() -> CsrfRouter<AppState> {
1044 1054
1045 1055 authorize_routes.merge(token_routes).merge(read_routes)
1046 1056 }
1057 +
1058 + #[cfg(test)]
1059 + mod tests {
1060 + use super::build_oauth_redirect;
1061 +
1062 + #[test]
1063 + fn appends_query_to_plain_uri() {
1064 + let url = build_oauth_redirect("https://app.example/cb", &[("code", "abc"), ("state", "s1")]);
1065 + assert_eq!(url, "https://app.example/cb?code=abc&state=s1");
1066 + }
1067 +
1068 + #[test]
1069 + fn merges_with_existing_query() {
1070 + let url = build_oauth_redirect("https://app.example/cb?foo=bar", &[("code", "abc")]);
1071 + assert_eq!(url, "https://app.example/cb?foo=bar&code=abc");
1072 + }
1073 +
1074 + #[test]
1075 + fn preserves_fragment_and_keeps_query_before_it() {
1076 + // R6-UX-1: the naive contains('?') builder appended ?code= AFTER the
1077 + // fragment, corrupting the callback. Real URL parsing keeps the query in
1078 + // its own component, ahead of the fragment.
1079 + let url = build_oauth_redirect("https://app.example/cb#frag", &[("code", "abc"), ("state", "s1")]);
1080 + assert_eq!(url, "https://app.example/cb?code=abc&state=s1#frag");
1081 + }
1082 +
1083 + #[test]
1084 + fn percent_encodes_values() {
1085 + let url = build_oauth_redirect("https://app.example/cb", &[("error", "consent required")]);
1086 + assert!(url.contains("error=consent+required") || url.contains("error=consent%20required"));
1087 + }
1088 +
1089 + #[test]
1090 + fn loopback_callback_gets_query() {
1091 + let url = build_oauth_redirect("http://127.0.0.1:9999/callback", &[("code", "xyz"), ("state", "s")]);
1092 + assert_eq!(url, "http://127.0.0.1:9999/callback?code=xyz&state=s");
1093 + }
1094 + }