carverauto/serviceradar

Fork 0

fixing gitignore issue #2243

Merged

mfreeman451 merged 1 commit from refs/pull/2243/head into main

2025-09-26 17:37:20 +00:00

mfreeman451 commented

2025-09-26 17:29:26 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #1663
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1663
Original created: 2025-09-26T17:29:26Z
Original updated: 2025-09-26T17:37:23Z
Original head: carverauto/serviceradar:fix/srql_rpm_build
Original base: main
Original merged: 2025-09-26T17:37:20Z by @mfreeman451

PR Type

Enhancement

Description

Add three new CLI executables for SRQL functionality
Implement WebSocket streaming endpoint for real-time queries
Add comprehensive query execution API with pagination support
Integrate environment-based configuration for Proton connections

Diagram Walkthrough

flowchart LR
  CLI["CLI Tools"] --> Parser["Query Parser"]
  Parser --> Translator["SRQL Translator"]
  Translator --> Server["Dream Server"]
  Server --> WebSocket["WebSocket Stream"]
  Server --> API["REST API"]
  API --> Proton["Proton Client"]
  WebSocket --> Proton

File Walkthrough

Relevant files

Enhancement

cli.ml `Basic SRQL translator CLI` ocaml/srql/bin/cli.ml Simple command-line interface for SRQL translation Parses query from command line arguments Translates SRQL to SQL with parameter substitution Outputs translated SQL and parameters to stdout	+38/-0
main.ml `Complete SRQL web server implementation` ocaml/srql/bin/main.ml Full-featured Dream web server with multiple endpoints WebSocket streaming endpoint with real-time query execution REST API for query execution with pagination support Environment-based configuration for Proton connections Cursor-based pagination with base64 encoding CORS and authentication support for WebSocket connections	+900/-0
srql_cli.ml `Interactive SRQL CLI client` ocaml/srql/bin/srql_cli.ml Interactive CLI client for SRQL queries Command-line argument parsing with help system Translation-only mode and full execution mode TLS connection support with configurable ports	+115/-0

Configuration changes

dune

Dune build configuration for CLI tools

ocaml/srql/bin/dune

Build configuration for three executables
srql-server for web server functionality
srql-translator for basic translation
srql-cli for interactive client usage

+21/-0

Imported from GitHub pull request. Original GitHub pull request: #1663 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1663 Original created: 2025-09-26T17:29:26Z Original updated: 2025-09-26T17:37:23Z Original head: carverauto/serviceradar:fix/srql_rpm_build Original base: main Original merged: 2025-09-26T17:37:20Z by @mfreeman451 --- ### **PR Type** Enhancement ___ ### **Description** - Add three new CLI executables for SRQL functionality - Implement WebSocket streaming endpoint for real-time queries - Add comprehensive query execution API with pagination support - Integrate environment-based configuration for Proton connections ___ ### Diagram Walkthrough ```mermaid flowchart LR CLI["CLI Tools"] --> Parser["Query Parser"] Parser --> Translator["SRQL Translator"] Translator --> Server["Dream Server"] Server --> WebSocket["WebSocket Stream"] Server --> API["REST API"] API --> Proton["Proton Client"] WebSocket --> Proton ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><table> <tr> <td> <details> <summary><strong>cli.ml</strong><dd><code>Basic SRQL translator CLI</code>                                                                </dd></summary> <hr> ocaml/srql/bin/cli.ml <ul><li>Simple command-line interface for SRQL translation<br> <li> Parses query from command line arguments<br> <li> Translates SRQL to SQL with parameter substitution<br> <li> Outputs translated SQL and parameters to stdout</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1663/files#diff-0d36e61b4b9bcd258865917bf3a837854aed9268e4598ef2875d2335d5dd01fb">+38/-0</a>    </td> </tr> <tr> <td> <details> <summary><strong>main.ml</strong><dd><code>Complete SRQL web server implementation</code>                                    </dd></summary> <hr> ocaml/srql/bin/main.ml <ul><li>Full-featured Dream web server with multiple endpoints<br> <li> WebSocket streaming endpoint with real-time query execution<br> <li> REST API for query execution with pagination support<br> <li> Environment-based configuration for Proton connections<br> <li> Cursor-based pagination with base64 encoding<br> <li> CORS and authentication support for WebSocket connections</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1663/files#diff-eef5ff32dece7828774801b08bbee80cdb527bf0a21c0cb4ab91318f31f865d7">+900/-0</a>  </td> </tr> <tr> <td> <details> <summary><strong>srql_cli.ml</strong><dd><code>Interactive SRQL CLI client</code>                                                            </dd></summary> <hr> ocaml/srql/bin/srql_cli.ml <ul><li>Interactive CLI client for SRQL queries<br> <li> Command-line argument parsing with help system<br> <li> Translation-only mode and full execution mode<br> <li> TLS connection support with configurable ports</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1663/files#diff-5d777ffd86cd672e8f72baf427408ead9e28cfce715966b27227eed869b0619c">+115/-0</a>  </td> </tr> </table></td></tr><tr><td><strong>Configuration changes</strong></td><td><table> <tr> <td> <details> <summary><strong>dune</strong><dd><code>Dune build configuration for CLI tools</code>                                      </dd></summary> <hr> ocaml/srql/bin/dune <ul><li>Build configuration for three executables<br> <li> <code>srql-server</code> for web server functionality<br> <li> <code>srql-translator</code> for basic translation<br> <li> <code>srql-cli</code> for interactive client usage</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1663/files#diff-8d74319b07e5cb27694e55ae1484ea992387ee5e534ebfd322710c5c44304e4f">+21/-0</a>    </td> </tr> </table></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-09-26 17:30:11 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1663#issuecomment-3339774386
Original created: 2025-09-26T17:30:11Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 5 🔵🔵🔵🔵🔵

🧪 No relevant tests

🔒 Security concerns

Authentication/Authorization:
The WebSocket stream defaults to allowing any Origin and no API key/bearer unless env vars are set. In production this can expose real-time data. Consider default-deny: require SRQL_API_KEY and/or bearer by default, and restrict origins explicitly.

SQL injection: Several places build SQL by string concatenation:

splice_where_predicate and build_boundary_predicate construct predicates using field names and values from cursor/order structures.
wrap_from_with_table rewrites FROM clause by substring operations.
While values often come from typed Proton columns and some escaping is done for strings, field names and table names are interpolated without quoting. If any of these can be influenced by user input (e.g., via query syntax mapping), this could allow injection. Ensure that:
Order-by field names and entity/table come from a validated whitelist/schema.
Use prepared statements or properly quote identifiers.
Avoid inline substitution for params when executing; prefer prepared execution with parameters.

Information disclosure: Error responses return Printexc.to_string exn to clients, which may leak internal details. Replace with sanitized messages while logging full errors server-side.

⚡ Recommended focus areas for review

Security Config

The WebSocket endpoint allows any Origin by default and optional API key/bearer checks via env; defaults may expose streaming data unintentionally in production. Verify secure defaults and document required envs.

Dream.get "/api/stream" (fun request ->
    (* Origin check parity: SRQL_ALLOWED_ORIGINS=csv or allow all by default *)
    let origin_allowed =
      match Dream.header request "Origin" with
      | None -> true
      | Some origin -> (
          match Sys.getenv_opt "SRQL_ALLOWED_ORIGINS" with
          | None -> true
          | Some csv ->
              let allowed =
                csv |> String.split_on_char ',' |> List.map String.trim
                |> List.filter (( <> ) "")
              in
              List.exists (fun a -> a = origin || a = "*") allowed)
    in
    if not origin_allowed then
      Dream.json ~status:`Forbidden
        (Yojson.Safe.to_string
           (`Assoc [ ("error", `String "WebSocket CORS: Origin not allowed") ]))
    else
      (* API key / bearer parity: require API key if SRQL_API_KEY set; optionally require Bearer if SRQL_REQUIRE_BEARER=true *)
      let api_key_ok =
        match Sys.getenv_opt "SRQL_API_KEY" with
        | None -> true
        | Some needed_key -> (
            let provided =
              match Dream.header request "X-API-Key" with
              | Some v -> Some v
              | None -> (
                  match Dream.cookie request "api_key" with
                  | Some v -> Some v
                  | None -> None)
            in
            match provided with Some v -> v = needed_key | None -> false)
      in
      let bearer_required =
        match Sys.getenv_opt "SRQL_REQUIRE_BEARER" with
        | Some v ->
            let v = String.lowercase_ascii (String.trim v) in
            v = "1" || v = "true" || v = "yes"
        | None -> false
      in
      let bearer_ok =
        match Dream.header request "Authorization" with
        | Some s when String.length s > 7 && String.sub s 0 7 = "Bearer " -> true
        | _ -> not bearer_required
      in
      if (not api_key_ok) || not bearer_ok then
        Dream.json ~status:`Unauthorized
          (Yojson.Safe.to_string (`Assoc [ ("error", `String "Unauthorized WebSocket") ]))
      else

SQL Injection Risk

Cursor boundary building and FROM table wrapping concatenate SQL fragments directly; ensure identifiers and literals derived from cursor/order_by are sanitized and cannot be user-controlled unsafe values.

(* Build SQL literal from a typed cursor value *)
let sql_literal_of_typed (typ : string) value : string =
  match value with
  | Proton.Column.String s ->
      let lt = String.lowercase_ascii typ in
      let has needle = match index_of_sub lt needle with Some _ -> true | None -> false in
      if has "string" || has "uuid" then "'" ^ escape_sql_string s ^ "'" else s
  | Proton.Column.DateTime (ts, _tz) -> Printf.sprintf "toDateTime(%Ld)" ts
  | Proton.Column.DateTime64 (v, p, _tz) -> Printf.sprintf "toDateTime64(%Ld, %d)" v p
  | _ -> Proton.Column.value_to_string value

(* Build a lexicographic boundary predicate for multi-column ORDER BY. 
   order: (field, dir) list
   vals: Each is (name, typ, value) coming from the row/columns
   direction: Next|Prev
*)
let build_boundary_predicate ~(order : (string * Srql_translator.Sql_ir.order_dir) list)
    ~(vals : (string * string * _) list) ~(direction : dir) : string option =
  if order = [] then None
  else
    (* Map value lookup by field name *)
    let lookup name =
      try Some (List.find (fun (n, _, _) -> String.equal n name) vals) with Not_found -> None
    in
    (* ensure all keys present *)
    if List.exists (fun (f, _) -> lookup f = None) order then None
    else
      let rec build_terms idx acc_eq =
        match List.nth order idx with
        | exception _ -> []
        | field, odir ->
            let _, typ, v = Option.get (lookup field) in
            let lit = sql_literal_of_typed typ v in
            let cmp =
              match (odir, direction) with
              | Srql_translator.Sql_ir.Asc, Next | Srql_translator.Sql_ir.Desc, Prev -> ">"
              | Srql_translator.Sql_ir.Desc, Next | Srql_translator.Sql_ir.Asc, Prev -> "<"
            in
            let this_term =
              let lhs = field and rhs = lit in
              let cmp_expr = Printf.sprintf "%s %s %s" lhs cmp rhs in
              match acc_eq with
              | [] -> cmp_expr
              | eqs -> "(" ^ String.concat " AND " eqs ^ ") AND " ^ cmp_expr
            in
            this_term :: build_terms (idx + 1) (acc_eq @ [ Printf.sprintf "%s = %s" field lit ])
      in
      let terms = build_terms 0 [] in
      if terms = [] then None
      else Some ("(" ^ String.concat " OR " (List.map (fun t -> "(" ^ t ^ ")") terms) ^ ")")

let splice_where_predicate (sql : string) (predicate : string) : string =
  let lsql = String.lowercase_ascii sql in
  let pos_order =
    match index_of_sub lsql " order by " with Some i -> i | None -> String.length sql
  in
  let head = String.sub sql 0 pos_order in
  let tail = String.sub sql pos_order (String.length sql - pos_order) in
  let lhead = String.lowercase_ascii head in
  let new_head =
    match index_of_sub lhead " where " with
    | Some _ -> head ^ " AND (" ^ predicate ^ ")"
    | None -> head ^ " WHERE " ^ predicate
  in
  new_head ^ tail

Error Handling

CLI swallows translation/execute errors by printing and returning unit with success exit code; consider proper exit codes to signal failure in scripts/CI.


let main () =
  let host, port, use_tls, tls_port, translate_only, query = parse_args () in

  Printf.printf "SRQL Query: %s\n" query;

  if translate_only then (
    (* Just translate, don't execute *)
    try
      let translation = Srql_translator.Proton_client.SRQL.translate query in
      Printf.printf "Translated SQL: %s\n" translation.sql;
      (match translation.params with
      | [] -> ()
      | params ->
          print_endline "Parameters:";
          List.iter
            (fun (name, value) ->
              Printf.printf "  %s -> %s\n" name (Proton.Column.value_to_string value))
            params);
      Lwt.return_unit
    with e ->
      Printf.printf "Translation error: %s\n" (Printexc.to_string e);
      Lwt.return_unit)
  else
    (* Connect and execute *)
    let config =
      if use_tls then
        { Srql_translator.Proton_client.Config.local_docker_tls with host; port = tls_port }
      else { Srql_translator.Proton_client.Config.local_docker_no_tls with host; port }
    in

    Printf.printf "Connecting to %s:%d (TLS: %b)...\n" host
      (if use_tls then tls_port else port)
      use_tls;

    Srql_translator.Proton_client.Client.with_connection config (fun client ->
        let* result = Srql_translator.Proton_client.SRQL.translate_and_execute client query in
        format_result result;
        Lwt.return_unit)

let () =
  Lwt_main.run
    (try main ()
     with e ->
       Printf.printf "Error: %s\n" (Printexc.to_string e);

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1663#issuecomment-3339774386 Original created: 2025-09-26T17:30:11Z --- ## PR Reviewer Guide 🔍 Here are some key observations to aid the review process: <table> <tr><td>⏱️ <strong>Estimated effort to review</strong>: 5 🔵🔵🔵🔵🔵</td></tr> <tr><td>🧪 <strong>No relevant tests</strong></td></tr> <tr><td>🔒 <strong>Security concerns</strong><br><br> <strong>Authentication/Authorization:</strong><br> The WebSocket stream defaults to allowing any Origin and no API key/bearer unless env vars are set. In production this can expose real-time data. Consider default-deny: require SRQL_API_KEY and/or bearer by default, and restrict origins explicitly. SQL injection: Several places build SQL by string concatenation: - splice_where_predicate and build_boundary_predicate construct predicates using field names and values from cursor/order structures. - wrap_from_with_table rewrites FROM clause by substring operations. While values often come from typed Proton columns and some escaping is done for strings, field names and table names are interpolated without quoting. If any of these can be influenced by user input (e.g., via query syntax mapping), this could allow injection. Ensure that: - Order-by field names and entity/table come from a validated whitelist/schema. - Use prepared statements or properly quote identifiers. - Avoid inline substitution for params when executing; prefer prepared execution with parameters. Information disclosure: Error responses return Printexc.to_string exn to clients, which may leak internal details. Replace with sanitized messages while logging full errors server-side.</td></tr> <tr><td>⚡ <strong>Recommended focus areas for review</strong><br><br> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1663/files#diff-eef5ff32dece7828774801b08bbee80cdb527bf0a21c0cb4ab91318f31f865d7R220-R270'><strong>Security Config</strong></a> The WebSocket endpoint allows any Origin by default and optional API key/bearer checks via env; defaults may expose streaming data unintentionally in production. Verify secure defaults and document required envs. </summary> ```ocaml Dream.get "/api/stream" (fun request -> (* Origin check parity: SRQL_ALLOWED_ORIGINS=csv or allow all by default *) let origin_allowed = match Dream.header request "Origin" with | None -> true | Some origin -> ( match Sys.getenv_opt "SRQL_ALLOWED_ORIGINS" with | None -> true | Some csv -> let allowed = csv |> String.split_on_char ',' |> List.map String.trim |> List.filter (( <> ) "") in List.exists (fun a -> a = origin || a = "*") allowed) in if not origin_allowed then Dream.json ~status:`Forbidden (Yojson.Safe.to_string (`Assoc [ ("error", `String "WebSocket CORS: Origin not allowed") ])) else (* API key / bearer parity: require API key if SRQL_API_KEY set; optionally require Bearer if SRQL_REQUIRE_BEARER=true *) let api_key_ok = match Sys.getenv_opt "SRQL_API_KEY" with | None -> true | Some needed_key -> ( let provided = match Dream.header request "X-API-Key" with | Some v -> Some v | None -> ( match Dream.cookie request "api_key" with | Some v -> Some v | None -> None) in match provided with Some v -> v = needed_key | None -> false) in let bearer_required = match Sys.getenv_opt "SRQL_REQUIRE_BEARER" with | Some v -> let v = String.lowercase_ascii (String.trim v) in v = "1" || v = "true" || v = "yes" | None -> false in let bearer_ok = match Dream.header request "Authorization" with | Some s when String.length s > 7 && String.sub s 0 7 = "Bearer " -> true | _ -> not bearer_required in if (not api_key_ok) || not bearer_ok then Dream.json ~status:`Unauthorized (Yojson.Safe.to_string (`Assoc [ ("error", `String "Unauthorized WebSocket") ])) else ``` </details> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1663/files#diff-eef5ff32dece7828774801b08bbee80cdb527bf0a21c0cb4ab91318f31f865d7R89-R153'><strong>SQL Injection Risk</strong></a> Cursor boundary building and FROM table wrapping concatenate SQL fragments directly; ensure identifiers and literals derived from cursor/order_by are sanitized and cannot be user-controlled unsafe values. </summary> ```ocaml (* Build SQL literal from a typed cursor value *) let sql_literal_of_typed (typ : string) value : string = match value with | Proton.Column.String s -> let lt = String.lowercase_ascii typ in let has needle = match index_of_sub lt needle with Some _ -> true | None -> false in if has "string" || has "uuid" then "'" ^ escape_sql_string s ^ "'" else s | Proton.Column.DateTime (ts, _tz) -> Printf.sprintf "toDateTime(%Ld)" ts | Proton.Column.DateTime64 (v, p, _tz) -> Printf.sprintf "toDateTime64(%Ld, %d)" v p | _ -> Proton.Column.value_to_string value (* Build a lexicographic boundary predicate for multi-column ORDER BY. order: (field, dir) list vals: Each is (name, typ, value) coming from the row/columns direction: Next|Prev *) let build_boundary_predicate ~(order : (string * Srql_translator.Sql_ir.order_dir) list) ~(vals : (string * string * _) list) ~(direction : dir) : string option = if order = [] then None else (* Map value lookup by field name *) let lookup name = try Some (List.find (fun (n, _, _) -> String.equal n name) vals) with Not_found -> None in (* ensure all keys present *) if List.exists (fun (f, _) -> lookup f = None) order then None else let rec build_terms idx acc_eq = match List.nth order idx with | exception _ -> [] | field, odir -> let _, typ, v = Option.get (lookup field) in let lit = sql_literal_of_typed typ v in let cmp = match (odir, direction) with | Srql_translator.Sql_ir.Asc, Next | Srql_translator.Sql_ir.Desc, Prev -> ">" | Srql_translator.Sql_ir.Desc, Next | Srql_translator.Sql_ir.Asc, Prev -> "<" in let this_term = let lhs = field and rhs = lit in let cmp_expr = Printf.sprintf "%s %s %s" lhs cmp rhs in match acc_eq with | [] -> cmp_expr | eqs -> "(" ^ String.concat " AND " eqs ^ ") AND " ^ cmp_expr in this_term :: build_terms (idx + 1) (acc_eq @ [ Printf.sprintf "%s = %s" field lit ]) in let terms = build_terms 0 [] in if terms = [] then None else Some ("(" ^ String.concat " OR " (List.map (fun t -> "(" ^ t ^ ")") terms) ^ ")") let splice_where_predicate (sql : string) (predicate : string) : string = let lsql = String.lowercase_ascii sql in let pos_order = match index_of_sub lsql " order by " with Some i -> i | None -> String.length sql in let head = String.sub sql 0 pos_order in let tail = String.sub sql pos_order (String.length sql - pos_order) in let lhead = String.lowercase_ascii head in let new_head = match index_of_sub lhead " where " with | Some _ -> head ^ " AND (" ^ predicate ^ ")" | None -> head ^ " WHERE " ^ predicate in new_head ^ tail ``` </details> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1663/files#diff-5d777ffd86cd672e8f72baf427408ead9e28cfce715966b27227eed869b0619cR70-R114'><strong>Error Handling</strong></a> CLI swallows translation/execute errors by printing and returning unit with success exit code; consider proper exit codes to signal failure in scripts/CI. </summary> ```ocaml let main () = let host, port, use_tls, tls_port, translate_only, query = parse_args () in Printf.printf "SRQL Query: %s\n" query; if translate_only then ( (* Just translate, don't execute *) try let translation = Srql_translator.Proton_client.SRQL.translate query in Printf.printf "Translated SQL: %s\n" translation.sql; (match translation.params with | [] -> () | params -> print_endline "Parameters:"; List.iter (fun (name, value) -> Printf.printf " %s -> %s\n" name (Proton.Column.value_to_string value)) params); Lwt.return_unit with e -> Printf.printf "Translation error: %s\n" (Printexc.to_string e); Lwt.return_unit) else (* Connect and execute *) let config = if use_tls then { Srql_translator.Proton_client.Config.local_docker_tls with host; port = tls_port } else { Srql_translator.Proton_client.Config.local_docker_no_tls with host; port } in Printf.printf "Connecting to %s:%d (TLS: %b)...\n" host (if use_tls then tls_port else port) use_tls; Srql_translator.Proton_client.Client.with_connection config (fun client -> let* result = Srql_translator.Proton_client.SRQL.translate_and_execute client query in format_result result; Lwt.return_unit) let () = Lwt_main.run (try main () with e -> Printf.printf "Error: %s\n" (Printexc.to_string e); ``` </details> </td></tr> </table>

qodo-code-review[bot] commented

2025-09-26 17:31:41 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1663#issuecomment-3339782275
Original created: 2025-09-26T17:31:41Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Security	Fix SQL injection vulnerability Fix a SQL injection vulnerability in the `sql_literal_of_typed` function by always quoting and escaping `Proton.Column.String` values, regardless of the associated type string. ocaml/srql/bin/main.ml [90-98] let sql_literal_of_typed (typ : string) value : string = match value with \| Proton.Column.String s -> - let lt = String.lowercase_ascii typ in - let has needle = match index_of_sub lt needle with Some _ -> true \| None -> false in - if has "string" \|\| has "uuid" then "'" ^ escape_sql_string s ^ "'" else s + "'" ^ escape_sql_string s ^ "'" \| Proton.Column.DateTime (ts, _tz) -> Printf.sprintf "toDateTime(%Ld)" ts \| Proton.Column.DateTime64 (v, p, _tz) -> Printf.sprintf "toDateTime64(%Ld, %d)" v p \| _ -> Proton.Column.value_to_string value Apply / Chat Suggestion importance[1-10]: 10 __ Why: The suggestion correctly identifies a critical SQL injection vulnerability in the cursor handling logic and provides an appropriate fix.	High
High-level	Re-implementing standard library features is risky The PR adds custom Base64 encoding and decoding functions. It is recommended to replace these with a standard, well-tested library to enhance reliability and security while reducing code maintenance. Examples: ocaml/srql/bin/main.ml [609-643] `let decode_cursor s = try Yojson.Safe.from_string s with _ -> let rev_tbl = Array.make 256 (-1) in let chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/" in String.iteri (fun i c -> rev_tbl.(int_of_char c) <- i) chars; let len = String.length s in let buf = Buffer.create len in ... (clipped 25 lines)` ocaml/srql/bin/main.ml [803-828] `let b64_encode s = (* Simple base64 implementation for small strings ) let tbl = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/" in let len = String.length s in let buf = Buffer.create ((len + 2) / 3 4) in let get i = int_of_char s.[i] in let rec loop i = if i >= len then () else let b0 = get i in ... (clipped 16 lines)` Solution Walkthrough: Before: `let decode_cursor s = try Yojson.Safe.from_string s with _ -> // ... manual base64 decoding implementation ... let rev_tbl = Array.make 256 (-1) in let chars = "..." in String.iteri (fun i c -> rev_tbl.(int_of_char c) <- i) chars; // ... complex bitwise logic ... Yojson.Safe.from_string (Buffer.contents buf) let b64_encode s = // ... manual base64 encoding implementation ... let tbl = "..." in let buf = Buffer.create ... in // ... complex bitwise logic ... Buffer.contents buf` After: (* Add `base64` library to dependencies ) open Base64 let decode_cursor s = try Yojson.Safe.from_string s with _ -> match decode s with \| Ok decoded_str -> Yojson.Safe.from_string decoded_str \| Error _ -> ( handle decoding error *) `Null let b64_encode s = encode_string ~pad:true s let encode_cursor_b64 = function \| Some (`String s) -> `String (b64_encode s) \| ... Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies custom Base64 implementations which introduces unnecessary security risks and maintenance overhead; replacing this with a standard library is a critical improvement.	High
Possible issue	Use standard library for Base64 Replace the custom Base64 decoding logic in `decode_cursor` with a standard library like `base64` to improve robustness and maintainability. ocaml/srql/bin/main.ml [609-642] let decode_cursor s = try Yojson.Safe.from_string s with _ -> - let rev_tbl = Array.make 256 (-1) in - let chars = - "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/" - in - String.iteri (fun i c -> rev_tbl.(int_of_char c) <- i) chars; - let len = String.length s in - let buf = Buffer.create len in - let rec sextet i acc bits = - if i >= len then (acc, bits, i) - else - let c = s.[i] in - if c = '=' then (acc, bits, i + 1) - else - let v = rev_tbl.(int_of_char c) in - if v < 0 then (acc, bits, i + 1) - else - let acc' = (acc lsl 6) lor v in - sextet (i + 1) acc' (bits + 6) - in - let rec loop i acc bits = - if i >= len then () - else - let acc', bits', j = sextet i acc bits in - if bits' >= 8 then ( - let b = (acc' lsr (bits' - 8)) land 0xFF in - Buffer.add_char buf (char_of_int b); - loop j acc' (bits' - 8)) - else loop j acc' bits' - in - loop 0 0 0; - Yojson.Safe.from_string (Buffer.contents buf) + match Base64.decode s with + \| Ok decoded -> Yojson.Safe.from_string decoded + \| Error (`Msg msg) -> failwith ("Base64 decoding failed: " ^ msg) Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that using a standard, well-tested library for Base64 decoding is preferable to a custom, potentially buggy implementation, improving code robustness and maintainability.	Medium
General	Improve command-line argument parsing Improve the argument parsing in `srql_cli` to correctly handle queries containing spaces by collecting all non-flag arguments and joining them, rather than only taking the first one. ocaml/srql/bin/srql_cli.ml [24-64] let parse_args () = let host = ref "serviceradar-proton" in let port = ref 8463 in let use_tls = ref false in let tls_port = ref 9440 in let translate_only = ref false in - let query = ref "" in + let query_parts = ref [] in let args = Array.to_list Sys.argv in let rec parse = function \| [] -> () \| "--help" :: _ \| "-h" :: _ -> print_usage () \| "--host" :: h :: rest -> host := h; parse rest \| "--port" :: p :: rest -> port := int_of_string p; parse rest \| "--tls" :: rest -> use_tls := true; parse rest \| "--tls-port" :: p :: rest -> tls_port := int_of_string p; parse rest \| "--translate-only" :: rest -> translate_only := true; parse rest \| q :: rest when String.length q > 0 && q.[0] <> '-' -> - query := q; + query_parts := q :: !query_parts; parse rest \| _ :: rest -> parse rest in parse (List.tl args); - if !query = "" then ( + let query = String.concat " " (List.rev !query_parts) in + + if query = "" then ( print_endline "Error: No query provided"; print_endline ""; print_usage ()); - (!host, !port, !use_tls, !tls_port, !translate_only, !query) + (!host, !port, !use_tls, !tls_port, !translate_only, query) Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a limitation in the command-line argument parsing and proposes a more robust implementation that correctly handles multi-word queries.	Low
More

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1663#issuecomment-3339782275 Original created: 2025-09-26T17:31:41Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>Security</td> <td> <details><summary>Fix SQL injection vulnerability</summary> ___ **Fix a SQL injection vulnerability in the <code>sql_literal_of_typed</code> function by always <br>quoting and escaping <code>Proton.Column.String</code> values, regardless of the associated <br>type string.** [ocaml/srql/bin/main.ml [90-98]](https://github.com/carverauto/serviceradar/pull/1663/files#diff-eef5ff32dece7828774801b08bbee80cdb527bf0a21c0cb4ab91318f31f865d7R90-R98) ```diff let sql_literal_of_typed (typ : string) value : string = match value with | Proton.Column.String s -> - let lt = String.lowercase_ascii typ in - let has needle = match index_of_sub lt needle with Some _ -> true | None -> false in - if has "string" || has "uuid" then "'" ^ escape_sql_string s ^ "'" else s + "'" ^ escape_sql_string s ^ "'" | Proton.Column.DateTime (ts, _tz) -> Printf.sprintf "toDateTime(%Ld)" ts | Proton.Column.DateTime64 (v, p, _tz) -> Printf.sprintf "toDateTime64(%Ld, %d)" v p | _ -> Proton.Column.value_to_string value ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 10</summary> __ Why: The suggestion correctly identifies a critical SQL injection vulnerability in the cursor handling logic and provides an appropriate fix. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>High-level</td> <td> <details><summary>Re-implementing standard library features is risky</summary> ___ **The PR adds custom Base64 encoding and decoding functions. It is recommended to <br>replace these with a standard, well-tested library to enhance reliability and <br>security while reducing code maintenance.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1663/files#diff-eef5ff32dece7828774801b08bbee80cdb527bf0a21c0cb4ab91318f31f865d7R609-R643">ocaml/srql/bin/main.ml [609-643]</a> </summary> ```ocaml let decode_cursor s = try Yojson.Safe.from_string s with _ -> let rev_tbl = Array.make 256 (-1) in let chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/" in String.iteri (fun i c -> rev_tbl.(int_of_char c) <- i) chars; let len = String.length s in let buf = Buffer.create len in ... (clipped 25 lines) ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1663/files#diff-eef5ff32dece7828774801b08bbee80cdb527bf0a21c0cb4ab91318f31f865d7R803-R828">ocaml/srql/bin/main.ml [803-828]</a> </summary> ```ocaml let b64_encode s = (* Simple base64 implementation for small strings *) let tbl = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/" in let len = String.length s in let buf = Buffer.create ((len + 2) / 3 * 4) in let get i = int_of_char s.[i] in let rec loop i = if i >= len then () else let b0 = get i in ... (clipped 16 lines) ``` </details> ### Solution Walkthrough: #### Before: ```ocaml let decode_cursor s = try Yojson.Safe.from_string s with _ -> // ... manual base64 decoding implementation ... let rev_tbl = Array.make 256 (-1) in let chars = "..." in String.iteri (fun i c -> rev_tbl.(int_of_char c) <- i) chars; // ... complex bitwise logic ... Yojson.Safe.from_string (Buffer.contents buf) let b64_encode s = // ... manual base64 encoding implementation ... let tbl = "..." in let buf = Buffer.create ... in // ... complex bitwise logic ... Buffer.contents buf ``` #### After: ```ocaml (* Add `base64` library to dependencies *) open Base64 let decode_cursor s = try Yojson.Safe.from_string s with _ -> match decode s with | Ok decoded_str -> Yojson.Safe.from_string decoded_str | Error _ -> (* handle decoding error *) `Null let b64_encode s = encode_string ~pad:true s let encode_cursor_b64 = function | Some (`String s) -> `String (b64_encode s) | ... ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies custom Base64 implementations which introduces unnecessary security risks and maintenance overhead; replacing this with a standard library is a critical improvement. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Use standard library for Base64</summary> ___ **Replace the custom Base64 decoding logic in <code>decode_cursor</code> with a standard <br>library like <code>base64</code> to improve robustness and maintainability.** [ocaml/srql/bin/main.ml [609-642]](https://github.com/carverauto/serviceradar/pull/1663/files#diff-eef5ff32dece7828774801b08bbee80cdb527bf0a21c0cb4ab91318f31f865d7R609-R642) ```diff let decode_cursor s = try Yojson.Safe.from_string s with _ -> - let rev_tbl = Array.make 256 (-1) in - let chars = - "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/" - in - String.iteri (fun i c -> rev_tbl.(int_of_char c) <- i) chars; - let len = String.length s in - let buf = Buffer.create len in - let rec sextet i acc bits = - if i >= len then (acc, bits, i) - else - let c = s.[i] in - if c = '=' then (acc, bits, i + 1) - else - let v = rev_tbl.(int_of_char c) in - if v < 0 then (acc, bits, i + 1) - else - let acc' = (acc lsl 6) lor v in - sextet (i + 1) acc' (bits + 6) - in - let rec loop i acc bits = - if i >= len then () - else - let acc', bits', j = sextet i acc bits in - if bits' >= 8 then ( - let b = (acc' lsr (bits' - 8)) land 0xFF in - Buffer.add_char buf (char_of_int b); - loop j acc' (bits' - 8)) - else loop j acc' bits' - in - loop 0 0 0; - Yojson.Safe.from_string (Buffer.contents buf) + match Base64.decode s with + | Ok decoded -> Yojson.Safe.from_string decoded + | Error (`Msg msg) -> failwith ("Base64 decoding failed: " ^ msg) ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies that using a standard, well-tested library for Base64 decoding is preferable to a custom, potentially buggy implementation, improving code robustness and maintainability. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>Improve command-line argument parsing</summary> ___ **Improve the argument parsing in <code>srql_cli</code> to correctly handle queries containing <br>spaces by collecting all non-flag arguments and joining them, rather than only <br>taking the first one.** [ocaml/srql/bin/srql_cli.ml [24-64]](https://github.com/carverauto/serviceradar/pull/1663/files#diff-5d777ffd86cd672e8f72baf427408ead9e28cfce715966b27227eed869b0619cR24-R64) ```diff let parse_args () = let host = ref "serviceradar-proton" in let port = ref 8463 in let use_tls = ref false in let tls_port = ref 9440 in let translate_only = ref false in - let query = ref "" in + let query_parts = ref [] in let args = Array.to_list Sys.argv in let rec parse = function | [] -> () | "--help" :: _ | "-h" :: _ -> print_usage () | "--host" :: h :: rest -> host := h; parse rest | "--port" :: p :: rest -> port := int_of_string p; parse rest | "--tls" :: rest -> use_tls := true; parse rest | "--tls-port" :: p :: rest -> tls_port := int_of_string p; parse rest | "--translate-only" :: rest -> translate_only := true; parse rest | q :: rest when String.length q > 0 && q.[0] <> '-' -> - query := q; + query_parts := q :: !query_parts; parse rest | _ :: rest -> parse rest in parse (List.tl args); - if !query = "" then ( + let query = String.concat " " (List.rev !query_parts) in + + if query = "" then ( print_endline "Error: No query provided"; print_endline ""; print_usage ()); - (!host, !port, !use_tls, !tls_port, !translate_only, !query) + (!host, !port, !use_tls, !tls_port, !translate_only, query) ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies a limitation in the command-line argument parsing and proposes a more robust implementation that correctly handles multi-word queries. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table>

qodo-code-review[bot] commented

2025-09-26 17:36:04 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1663#issuecomment-3339800705
Original created: 2025-09-26T17:36:04Z

CI Feedback 🧐

A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

Action: build-test

Failed stage: Install dependencies (with tests) [❌]

Failure summary:

The action failed due to an OPAM package resolution conflict:
- deps-of-srql-translator →
mirage-crypto-rng-lwt >= 1.2.0 → mirage-crypto-rng = 1.2.0 → mirage-crypto = 1.2.0
-
deps-of-srql-translator → proton >= 1.0.16 → mirage-crypto >= 2.0.2
These constraints are
incompatible (require mirage-crypto = 1.2.0 vs mirage-crypto >= 2.0.2), so OPAM found no solution
and exited with code 20.

Relevant error logs:

1:  ##[group]Runner Image Provisioner
2:  Hosted Compute Agent
...

193:  Setting up musl-dev:amd64 (1.2.4-2) ...
194:  Setting up musl-tools (1.2.4-2) ...
195:  Processing triggers for man-db (2.12.0-4build2) ...
196:  Not building database; man-db/auto-update is not 'true'.
197:  Running kernel seems to be up-to-date.
198:  Restarting services...
199:  Service restarts being deferred:
200:  systemctl restart hosted-compute-agent.service
201:  No containers need to be restarted.
202:  No user sessions are running outdated binaries.
203:  No VM guests are running outdated hypervisor (qemu) binaries on this host.
204:  [command]/opt/hostedtoolcache/opam/2.4.1/x86_64/opam init --auto-setup --bare --enable-shell-hook
205:  No configuration file found, using built-in defaults.
206:  Checking for available remotes: rsync and local, git, mercurial.
207:  - you won't be able to use darcs repositories unless you install the ^[[01mdarcs^[[0m command on your system.
208:  ^[[31m[ERROR]^[[0m Sandboxing is not working on your platform ubuntu:
209:  "~/.opam/opam-init/hooks/sandbox.sh build sh -c echo SUCCESS | tee check-write" exited with code 1 "bwrap: loopback: Failed RTM_NEWADDR: Operation not permitted"
210:  Do you want to disable it?  Note that this will result in less secure package builds, so please ensure that you have some other isolation mechanisms in place (such as running within a container or virtual machine). [^[[1;34my^[[0m/^[[1;34mN^[[0m] y
...

673:  shell: /usr/bin/bash -e {0}
674:  env:
675:  OPAMCOLOR: always
676:  OPAMCONFIRMLEVEL: unsafe-yes
677:  OPAMDOWNLOADJOBS: 4
678:  OPAMERRLOGLEN: 0
679:  OPAMEXTERNALSOLVER: builtin-0install
680:  OPAMPRECISETRACKING: 1
681:  OPAMRETRIES: 10
682:  OPAMROOT: /home/runner/.opam
683:  OPAMSOLVERTIMEOUT: 600
684:  OPAMYES: 1
685:  DUNE_CACHE_ROOT: /home/runner/.cache/dune
686:  CLICOLOR_FORCE: 1
687:  ##[endgroup]
688:  ^[[31m[ERROR]^[[0m Package conflict!
689:  ^[[31m  * ^[[0mNo agreement on the version of ^[[01mmirage-crypto^[[0m:
690:  - deps-of-srql-translator^[[33m → ^[[0mmirage-crypto-rng-lwt >= 1.2.0^[[33m → ^[[0mmirage-crypto-rng = 1.2.0^[[33m → ^[[0m^[[31;01mmirage-crypto = 1.2.0^[[0m
691:  - deps-of-srql-translator^[[33m → ^[[0mproton >= 1.0.16^[[33m → ^[[0m^[[31;01mmirage-crypto >= 2.0.2^[[0m
692:  No solution found, exiting
693:  ##[error]Process completed with exit code 20.
694:  Post job cleanup.

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1663#issuecomment-3339800705 Original created: 2025-09-26T17:36:04Z --- ## CI Feedback 🧐 A test triggered by this PR failed. Here is an AI-generated analysis of the failure: <table><tr><td> **Action:** build-test</td></tr> <tr><td> **Failed stage:** [Install dependencies (with tests)](https://github.com/carverauto/serviceradar/actions/runs/18044876721/job/51353043128) [❌] </td></tr> <tr><td> **Failure summary:** The action failed due to an OPAM package resolution conflict:<br> - <code>deps-of-srql-translator → </code><br><code>mirage-crypto-rng-lwt >= 1.2.0 → mirage-crypto-rng = 1.2.0 → mirage-crypto = 1.2.0</code><br> - <br><code>deps-of-srql-translator → proton >= 1.0.16 → mirage-crypto >= 2.0.2</code><br> These constraints are <br>incompatible (require <code>mirage-crypto = 1.2.0</code> vs <code>mirage-crypto >= 2.0.2</code>), so OPAM found no solution <br>and exited with code 20.<br> </td></tr> <tr><td> <details><summary>Relevant error logs:</summary> ```yaml 1: ##[group]Runner Image Provisioner 2: Hosted Compute Agent ... 193: Setting up musl-dev:amd64 (1.2.4-2) ... 194: Setting up musl-tools (1.2.4-2) ... 195: Processing triggers for man-db (2.12.0-4build2) ... 196: Not building database; man-db/auto-update is not 'true'. 197: Running kernel seems to be up-to-date. 198: Restarting services... 199: Service restarts being deferred: 200: systemctl restart hosted-compute-agent.service 201: No containers need to be restarted. 202: No user sessions are running outdated binaries. 203: No VM guests are running outdated hypervisor (qemu) binaries on this host. 204: [command]/opt/hostedtoolcache/opam/2.4.1/x86_64/opam init --auto-setup --bare --enable-shell-hook 205: No configuration file found, using built-in defaults. 206: Checking for available remotes: rsync and local, git, mercurial. 207: - you won't be able to use darcs repositories unless you install the ^[[01mdarcs^[[0m command on your system. 208: ^[[31m[ERROR]^[[0m Sandboxing is not working on your platform ubuntu: 209: "~/.opam/opam-init/hooks/sandbox.sh build sh -c echo SUCCESS | tee check-write" exited with code 1 "bwrap: loopback: Failed RTM_NEWADDR: Operation not permitted" 210: Do you want to disable it? Note that this will result in less secure package builds, so please ensure that you have some other isolation mechanisms in place (such as running within a container or virtual machine). [^[[1;34my^[[0m/^[[1;34mN^[[0m] y ... 673: shell: /usr/bin/bash -e {0} 674: env: 675: OPAMCOLOR: always 676: OPAMCONFIRMLEVEL: unsafe-yes 677: OPAMDOWNLOADJOBS: 4 678: OPAMERRLOGLEN: 0 679: OPAMEXTERNALSOLVER: builtin-0install 680: OPAMPRECISETRACKING: 1 681: OPAMRETRIES: 10 682: OPAMROOT: /home/runner/.opam 683: OPAMSOLVERTIMEOUT: 600 684: OPAMYES: 1 685: DUNE_CACHE_ROOT: /home/runner/.cache/dune 686: CLICOLOR_FORCE: 1 687: ##[endgroup] 688: ^[[31m[ERROR]^[[0m Package conflict! 689: ^[[31m * ^[[0mNo agreement on the version of ^[[01mmirage-crypto^[[0m: 690: - deps-of-srql-translator^[[33m → ^[[0mmirage-crypto-rng-lwt >= 1.2.0^[[33m → ^[[0mmirage-crypto-rng = 1.2.0^[[33m → ^[[0m^[[31;01mmirage-crypto = 1.2.0^[[0m 691: - deps-of-srql-translator^[[33m → ^[[0mproton >= 1.0.16^[[33m → ^[[0m^[[31;01mmirage-crypto >= 2.0.2^[[0m 692: No solution found, exiting 693: ##[error]Process completed with exit code 20. 694: Post job cleanup. ``` </details></td></tr></table>