From 453e294883264b7cc06405c066879519813d61f5 Mon Sep 17 00:00:00 2001
From: Justin Ma <hustcer@outlook.com>
Date: Fri, 14 Feb 2025 12:23:59 +0800
Subject: [PATCH] Refactor `kv` commands: replace inline params in SQL queries
 (#15108)

# Description

Update some comments and fix potential security issue:

SQL Injection in DELETE statements: The code constructs SQL queries by
interpolating the $key variable directly into the string. If a key
contains malicious input could lead to SQL injection. Need to use
parameterized queries or escaping.
---
 crates/nu-std/std-rfc/kv/mod.nu | 41 +++++++++++++++------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/crates/nu-std/std-rfc/kv/mod.nu b/crates/nu-std/std-rfc/kv/mod.nu
index 30088dba4d..95d90282e1 100644
--- a/crates/nu-std/std-rfc/kv/mod.nu
+++ b/crates/nu-std/std-rfc/kv/mod.nu
@@ -14,8 +14,7 @@
 
 # Stores the pipeline value for later use
 #
-# If the key already exists, it is updated
-# to the new value provided.
+# If the key already exists, it is updated to the new value provided.
 @example "Store the list of files in the home directory" {
   ls ~ | kv set "home snapshot"
 }
@@ -29,7 +28,7 @@ export def "kv set" [
   key: string
   value_or_closure?: any
   --return (-r): string   # Whether and what to return to the pipeline output
-  --universal (-u)
+  --universal (-u)        # Store the key-value pair in a universal database
 ] {
   # Pipeline input is preferred, but prioritize
   # parameter if present. This allows $in to be
@@ -53,7 +52,7 @@ export def "kv set" [
   let db_open = (db_setup --universal=$universal)
   try {
     # Delete the existing key if it does exist
-    do $db_open | query db $"DELETE FROM std_kv_store WHERE key = '($key)'"
+    do $db_open | query db "DELETE FROM std_kv_store WHERE key = :key" --params { key: $key }
   }
 
   match $universal {
@@ -97,36 +96,34 @@ export def "kv set" [
 
 # Retrieves a stored value by key
 #
-# Counterpart of "kv set". Returns null
-# if the key is not found.
+# Counterpart of "kv set". Returns null if the key is not found.
 @example "Retrieve a stored value" {
   kv get foo
 }
 export def "kv get" [
-  key: string # Key of the kv-pair to retrieve
-  --universal (-u)
+  key: string       # Key of the kv-pair to retrieve
+  --universal (-u)  # Whether to use the universal db
 ] {
   let db_open = (db_setup --universal=$universal)
   do $db_open
     # Hack to turn a SQLiteDatabase into a table
-  | $in.std_kv_store | wrap temp | get temp
-  | where key == $key
+    | $in.std_kv_store | wrap temp | get temp
+    | where key == $key
     # Should only be one occurrence of each key in the stor
-  | get -i value.0
-  | match $in {
+    | get -i value.0
+    | match $in {
       # Key not found
       null => null
       # Key found
       _ => { from nuon }
-  }
+    }
 }
 
 # List the currently stored key-value pairs
 #
-# Returns results as the Nushell value rather
-# than the stored nuon.
+# Returns results as the Nushell value rather than the stored nuon.
 export def "kv list" [
-  --universal (-u)
+  --universal (-u)  # Whether to use the universal db
 ] {
   let db_open = (db_setup --universal=$universal)
 
@@ -140,8 +137,8 @@ export def "kv list" [
 
 # Returns and removes a key-value pair
 export def --env "kv drop" [
-  key: string   # Key of the kv-pair to drop
-  --universal (-u)
+  key: string       # Key of the kv-pair to drop
+  --universal (-u)  # Whether to use the universal db
 ] {
   let db_open = (db_setup --universal=$universal)
 
@@ -150,7 +147,7 @@ export def --env "kv drop" [
   try {
     do $db_open
       # Hack to turn a SQLiteDatabase into a table
-    | query db $"DELETE FROM std_kv_store WHERE key = '($key)'"
+      | query db "DELETE FROM std_kv_store WHERE key = :key" --params { key: $key }
   }
 
   if $universal and ($env.NU_KV_UNIVERSALS? | default false) {
@@ -163,12 +160,12 @@ export def --env "kv drop" [
 def universal_db_path [] {
   $env.NU_UNIVERSAL_KV_PATH?
   | default (
-      $nu.data-dir | path join "std_kv_variables.sqlite3"
+    $nu.data-dir | path join "std_kv_variables.sqlite3"
   )
 }
 
 def db_setup [
-  --universal
+  --universal   # Whether to use the universal db
 ] : nothing -> closure {
   try {
     match $universal {
@@ -181,7 +178,7 @@ def db_setup [
           value: ''
         }
         $dummy_record | into sqlite (universal_db_path) -t std_kv_store
-        open (universal_db_path) | query db $"DELETE FROM std_kv_store WHERE key = '($uuid)'"
+        open (universal_db_path) | query db "DELETE FROM std_kv_store WHERE key = :key" --params { key: $uuid }
       }
       false => {
         # Create the stor table if it doesn't exist