Refine granularity of 400 and 502 error diagnostics

Signed-off-by: Eloi DEMOLIS <[email protected]>
sozu-proxy · Nov 6, 2024 · 0be817b · 0be817b
1 parent 61cbf21
commit 0be817b
Show file tree

Hide file tree

Showing 3 changed files with 179 additions and 64 deletions.
diff --git a/lib/src/protocol/kawa_h1/answers.rs b/lib/src/protocol/kawa_h1/answers.rs
@@ -369,12 +369,38 @@ Sozu-Id: %REQUEST_ID\r
 {
     \"route\": \"%ROUTE\",
     \"request_id\": \"%REQUEST_ID\",
+    \"parsing_phase\": \"%PHASE\",
+    \"successfully_parsed\": %SUCCESSFULLY_PARSED,
+    \"partially_parsed\": %PARTIALLY_PARSED,
+    \"invalid\": %INVALID,
+
 }
 </pre>
-<p>Request could not be parsed. Parser stopped at phase: %PHASE.</p>
-<p>Diagnostic: %MESSAGE</p>
-<p>Further details:</p>
-<pre>%DETAILS</pre>
+<p>Request could not be parsed.</p>
+<p>%MESSAGE</p>
+<div id=details hidden=true>
+<p>While parsing %PHASE, this was reported as invalid:</p>
+<pre>
+<span id=p1 style='background:lime'></span><span id=p2 style='background:yellow'></span><span id=p3 style='background:red'></span>
+</pre>
+</div>
+<script>
+function display(id, chunks) {
+    let [start, end] = chunks.split(' ... ');
+    let dec = new TextDecoder('utf8');
+    let decode = chunk => dec.decode(new Uint8Array(chunk.split(' ').filter(e => e).map(e => parseInt(e, 16))));
+    document.getElementById(id).innerHTML = JSON.stringify(end ? `${decode(start)} ... ${decode(end)}` : `${decode(start)}`).slice(1,-1);
+}
+let p1 = %SUCCESSFULLY_PARSED;
+let p2 = %PARTIALLY_PARSED;
+let p3 = %INVALID;
+if (p1 !== null) {
+    document.getElementById('details').hidden=false;
+    display('p1', p1);
+    display('p2', p2);
+    display('p3', p3);
+}
+</script>
 <footer>This is an automatic answer by Sozu.</footer>",
     )
 }
@@ -480,12 +506,38 @@ Sozu-Id: %REQUEST_ID\r
     \"request_id\": \"%REQUEST_ID\",
     \"cluster_id\": \"%CLUSTER_ID\",
     \"backend_id\": \"%BACKEND_ID\",
+    \"parsing_phase\": \"%PHASE\",
+    \"successfully_parsed\": \"%SUCCESSFULLY_PARSED\",
+    \"partially_parsed\": \"%PARTIALLY_PARSED\",
+    \"invalid\": \"%INVALID\",
+
 }
 </pre>
-<p>Response could not be parsed. Parser stopped at phase: %PHASE.</p>
-<p>Diagnostic: %MESSAGE</p>
-<p>Further details:</p>
-<pre>%DETAILS</pre>
+<p>Response could not be parsed.</p>
+<p>%MESSAGE</p>
+<div id=details hidden=true>
+<p>While parsing %PHASE, this was reported as invalid:</p>
+<pre>
+<span id=p1 style='background:lime'></span><span id=p2 style='background:yellow'></span><span id=p3 style='background:red'></span>
+</pre>
+</div>
+<script>
+function display(id, chunks) {
+    let [start, end] = chunks.split(' ... ');
+    let dec = new TextDecoder('utf8');
+    let decode = chunk => dec.decode(new Uint8Array(chunk.split(' ').filter(e => e).map(e => parseInt(e, 16))));
+    document.getElementById(id).innerHTML = JSON.stringify(end ? `${decode(start)} ... ${decode(end)}` : `${decode(start)}`).slice(1,-1);
+}
+let p1 = %SUCCESSFULLY_PARSED;
+let p2 = %PARTIALLY_PARSED;
+let p3 = %INVALID;
+if (p1 !== null) {
+    document.getElementById('details').hidden=false;
+    display('p1', p1);
+    display('p2', p2);
+    display('p3', p3);
+}
+</script>
 <footer>This is an automatic answer by Sozu.</footer>",
     )
 }
@@ -641,11 +693,23 @@ impl HttpAnswers {
             valid_in_header: false,
             typ: ReplacementType::VariableOnce(0),
         };
-        let details = TemplateVariable {
-            name: "DETAILS",
+        let successfully_parsed = TemplateVariable {
+            name: "SUCCESSFULLY_PARSED",
             valid_in_body: true,
             valid_in_header: false,
-            typ: ReplacementType::VariableOnce(0),
+            typ: ReplacementType::Variable(0),
+        };
+        let partially_parsed = TemplateVariable {
+            name: "PARTIALLY_PARSED",
+            valid_in_body: true,
+            valid_in_header: false,
+            typ: ReplacementType::Variable(0),
+        };
+        let invalid = TemplateVariable {
+            name: "INVALID",
+            valid_in_body: true,
+            valid_in_header: false,
+            typ: ReplacementType::Variable(0),
         };
 
         match status {
@@ -657,7 +721,7 @@ impl HttpAnswers {
             400 => Template::new(
                 400,
                 answer,
-                &[length, route, request_id, message, phase, details],
+                &[length, route, request_id, message, phase, successfully_parsed, partially_parsed, invalid],
             ),
             401 => Template::new(
                 401,
@@ -682,7 +746,7 @@ impl HttpAnswers {
             502 => Template::new(
                 502,
                 answer,
-                &[length, route, request_id, cluster_id, backend_id, message, phase, details],
+                &[length, route, request_id, cluster_id, backend_id, message, phase, successfully_parsed, partially_parsed, invalid],
             ),
             503 => Template::new(
                 503,
@@ -806,10 +870,19 @@ impl HttpAnswers {
             DefaultAnswer::Answer400 {
                 message,
                 phase,
-                details,
+                successfully_parsed,
+                partially_parsed,
+                invalid,
             } => {
-                variables = vec![route.into(), request_id.into(), phase_to_vec(phase)];
-                variables_once = vec![message.into(), details.into()];
+                variables = vec![
+                    route.into(),
+                    request_id.into(),
+                    phase_to_vec(phase),
+                    successfully_parsed.into(),
+                    partially_parsed.into(),
+                    invalid.into(),
+                ];
+                variables_once = vec![message.into()];
                 &self.listener_answers.answer_400
             }
             DefaultAnswer::Answer401 {} => {
@@ -844,16 +917,21 @@ impl HttpAnswers {
             DefaultAnswer::Answer502 {
                 message,
                 phase,
-                details,
+                successfully_parsed,
+                partially_parsed,
+                invalid,
             } => {
                 variables = vec![
                     route.into(),
                     request_id.into(),
                     cluster_id.unwrap_or_default().into(),
                     backend_id.unwrap_or_default().into(),
                     phase_to_vec(phase),
+                    successfully_parsed.into(),
+                    partially_parsed.into(),
+                    invalid.into(),
                 ];
-                variables_once = vec![message.into(), details.into()];
+                variables_once = vec![message.into()];
                 &self.listener_answers.answer_502
             }
             DefaultAnswer::Answer503 { message } => {

diff --git a/lib/src/protocol/kawa_h1/diagnostics.rs b/lib/src/protocol/kawa_h1/diagnostics.rs
@@ -9,7 +9,7 @@ const CHARSET: &str = "all characters are LATIN-1 (no UTF-8 allowed)";
 #[cfg(not(feature = "tolerant-http1-parser"))]
 const CHARSET: &str = "all characters are UASCII (no UTF-8 allowed)";
 
-fn hex_dump(buffer: &[u8], window: usize, start: usize, end: usize) -> String {
+fn hex_and_ascii_dump(buffer: &[u8], window: usize, start: usize, end: usize) -> String {
     let mut result = String::with_capacity(window * 4 * 2 + 10);
     if end - start <= window {
         let slice = &buffer[start..end];
@@ -36,11 +36,41 @@ fn hex_dump(buffer: &[u8], window: usize, start: usize, end: usize) -> String {
     result
 }
 
+fn hex_dump(buffer: &[u8], window: usize, start: usize, end: usize) -> String {
+    let mut result = String::with_capacity(window * 3 + 10);
+    result.push('\"');
+    if end - start <= window {
+        let slice = &buffer[start..end];
+        for (i, c) in slice.iter().enumerate() {
+            let _ = write!(result, "{c:02x}");
+            if i < slice.len() - 1 {
+                result.push(' ');
+            }
+        }
+    } else {
+        let half = window / 2;
+        let slice1 = &buffer[start..start + half - 1];
+        let slice2 = &buffer[end - half + 1..end];
+        for c in slice1 {
+            let _ = write!(result, "{c:02x} ");
+        }
+        result.push_str("... ");
+        for (i, c) in slice2.iter().enumerate() {
+            let _ = write!(result, "{c:02x}");
+            if i < slice2.len() - 1 {
+                result.push(' ');
+            }
+        }
+    }
+    result.push('\"');
+    result
+}
+
 pub fn diagnostic_400_502(
     marker: ParsingPhaseMarker,
     kind: ParsingErrorKind,
     kawa: &GenericHttpStream,
-) -> (String, String) {
+) -> (String, String, String, String) {
     match kind {
         ParsingErrorKind::Consuming { index } => {
             let message = match marker {
@@ -55,52 +85,47 @@ pub fn diagnostic_400_502(
                     } else {
                         "trailer"
                     };
-                    if let Some(Block::Header(Pair { key, .. })) = kawa.blocks.back() {
-                        format!(
-                            "A {marker} is invalid, make sure {CHARSET}. Last valid {marker} is: {:?}.",
-                            String::from_utf8_lossy(key.data(kawa.storage.buffer())),
-                        )
-                    } else {
-                        format!("The first {marker} is invalid, make sure {CHARSET}.")
-                    }
+                    format!("A {marker} is invalid, make sure {CHARSET}.")
+                    // if let Some(Block::Header(Pair { key, .. })) = kawa.blocks.back() {
+                    //     format!(
+                    //         "A {marker} is invalid, make sure {CHARSET}. Last valid {marker} is: {:?}.",
+                    //         String::from_utf8_lossy(key.data(kawa.storage.buffer())),
+                    //     )
+                    // } else {
+                    //     format!("The first {marker} is invalid, make sure {CHARSET}.")
+                    // }
                 }
                 ParsingPhaseMarker::Cookies => {
-                    if kawa.detached.jar.len() > 1 {
-                        let Pair { key, .. } = &kawa.detached.jar[kawa.detached.jar.len() - 2];
-                        format!(
-                            "A cookie is invalid, make sure {CHARSET}. Last valid cookie is: {:?}.",
-                            String::from_utf8_lossy(key.data(kawa.storage.buffer())),
-                        )
-                    } else {
-                        format!("The first cookie is invalid, make sure {CHARSET}.")
-                    }
+                    // if kawa.detached.jar.len() > 1 {
+                    //     let Pair { key, .. } = &kawa.detached.jar[kawa.detached.jar.len() - 2];
+                    //     format!(
+                    //         "A cookie is invalid, make sure {CHARSET}. Last valid cookie is: {:?}.",
+                    //         String::from_utf8_lossy(key.data(kawa.storage.buffer())),
+                    //     )
+                    // } else {
+                    //     format!("The first cookie is invalid, make sure {CHARSET}.")
+                    // }
+                    format!("A cookie is invalid, make sure {CHARSET}.")
                 }
                 ParsingPhaseMarker::Body
                 | ParsingPhaseMarker::Chunks
                 | ParsingPhaseMarker::Terminated
                 | ParsingPhaseMarker::Error => {
-                    format!("Parsing phase {marker:?} should not produce 400 error.")
+                    format!("The parser stopped in an unexpected phase.")
+                    // format!("Parsing phase {marker:?} should not produce 400 error.")
                 }
             };
             let buffer = kawa.storage.buffer();
-            let details = format!(
-                "\
-Parsed successfully:
-{}
-Partially parsed (valid):
-{}
-Invalid:
-{}",
-                hex_dump(buffer, 32, kawa.storage.start, kawa.storage.head),
-                hex_dump(buffer, 32, kawa.storage.head, index as usize),
-                hex_dump(buffer, 32, index as usize, kawa.storage.end),
-            );
-            (message, details)
+            let successfully_parsed = hex_dump(buffer, 32, kawa.storage.start, kawa.storage.head);
+            let partially_parsed = hex_dump(buffer, 32, kawa.storage.head, index as usize);
+            let invalid = hex_dump(buffer, 32, index as usize, kawa.storage.end);
+            (message, successfully_parsed, partially_parsed, invalid)
         }
         ParsingErrorKind::Processing { message } => (
-            "The request was successfully parsed but presents inconsistent or invalid values."
-                .into(),
-            message.to_owned(),
+            format!("The request was successfully parsed but presents inconsistent or invalid values: {message}"),
+            String::from("null"),
+            String::from("null"),
+            String::from("null"),
         ),
     }
 }

diff --git a/lib/src/protocol/kawa_h1/mod.rs b/lib/src/protocol/kawa_h1/mod.rs
@@ -81,7 +81,9 @@ pub enum DefaultAnswer {
     Answer400 {
         message: String,
         phase: kawa::ParsingPhaseMarker,
-        details: String,
+        successfully_parsed: String,
+        partially_parsed: String,
+        invalid: String,
     },
     Answer401 {},
     Answer404 {},
@@ -96,7 +98,9 @@ pub enum DefaultAnswer {
     Answer502 {
         message: String,
         phase: kawa::ParsingPhaseMarker,
-        details: String,
+        successfully_parsed: String,
+        partially_parsed: String,
+        invalid: String,
     },
     Answer503 {
         message: String,
@@ -445,11 +449,14 @@ impl<Front: SocketHandler, L: ListenerHandler + L7ListenerHandler> Http<Front, L
                 self.log_request_error(metrics, "Parsing error on the request");
                 return StateResult::CloseSession;
             } else {
-                let (message, details) = diagnostic_400_502(marker, kind, &self.request_stream);
+                let (message, successfully_parsed, partially_parsed, invalid) =
+                    diagnostic_400_502(marker, kind, &self.request_stream);
                 self.set_answer(DefaultAnswer::Answer400 {
-                    phase: marker,
-                    details,
                     message,
+                    phase: marker,
+                    successfully_parsed,
+                    partially_parsed,
+                    invalid,
                 });
                 return StateResult::Continue;
             }
@@ -837,11 +844,14 @@ impl<Front: SocketHandler, L: ListenerHandler + L7ListenerHandler> Http<Front, L
             if response_stream.consumed {
                 return SessionResult::Close;
             } else {
-                let (message, details) = diagnostic_400_502(marker, kind, response_stream);
+                let (message, successfully_parsed, partially_parsed, invalid) =
+                    diagnostic_400_502(marker, kind, response_stream);
                 self.set_answer(DefaultAnswer::Answer502 {
-                    phase: marker,
-                    details,
                     message,
+                    phase: marker,
+                    successfully_parsed,
+                    partially_parsed,
+                    invalid,
                 });
                 return SessionResult::Continue;
             }
@@ -1243,9 +1253,11 @@ impl<Front: SocketHandler, L: ListenerHandler + L7ListenerHandler> Http<Front, L
             Ok(tuple) => tuple,
             Err(cluster_error) => {
                 self.set_answer(DefaultAnswer::Answer400 {
-                    phase: self.request_stream.parsing_phase.marker(),
-                    details: cluster_error.to_string(),
                     message: "Could not extract the route after connection started, this should not happen.".into(),
+                    phase: self.request_stream.parsing_phase.marker(),
+                    successfully_parsed: String::from("null"),
+                    partially_parsed: String::from("null"),
+                    invalid: String::from("null"),
                 });
                 return Err(cluster_error);
             }