Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(torii): sql playground #2714

Merged
merged 14 commits into from
Nov 22, 2024
Merged

feat(torii): sql playground #2714

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
831a649
feat(torii): sql playground
Larkooo Nov 22, 2024
eee8cdc
fix: sql playground
Larkooo Nov 22, 2024
1f6c270
fmt
Larkooo Nov 22, 2024
9b7b380
fix: dynamic types
Larkooo Nov 22, 2024
8121039
auto completion & schema
Larkooo Nov 22, 2024
f8cc181
fix playground
Larkooo Nov 22, 2024
ceb5db2
chore
Larkooo Nov 22, 2024
148a9e1
c
Larkooo Nov 22, 2024
25e0a6f
fix
Larkooo Nov 22, 2024
9e7a872
chore: show fetch time in query result
Larkooo Nov 22, 2024
50f7c56
fmt
Larkooo Nov 22, 2024
57499e9
better error hamdling
Larkooo Nov 22, 2024
a9eb280
add history and favoriting queries
Larkooo Nov 22, 2024
d3dbc5b
dedup
Larkooo Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 44 additions & 8 deletions crates/torii/server/src/handlers/sql.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
use base64::Engine;
use http::header::CONTENT_TYPE;
use hyper::{Body, Method, Request, Response, StatusCode};
use include_str;
Larkooo marked this conversation as resolved.
Show resolved Hide resolved
use sqlx::{Column, Row, SqlitePool, TypeInfo};

use super::Handler;
Expand All @@ -29,7 +30,7 @@
"TEXT" => row
.get::<Option<String>, _>(i)
.map_or(serde_json::Value::Null, serde_json::Value::String),
"INTEGER" | "NULL" => row
"INTEGER" => row

Check warning on line 33 in crates/torii/server/src/handlers/sql.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/server/src/handlers/sql.rs#L33 

Added line #L33 was not covered by tests
.get::<Option<i64>, _>(i)
.map_or(serde_json::Value::Null, |n| {
serde_json::Value::Number(n.into())
Expand All @@ -48,9 +49,25 @@
.map_or(serde_json::Value::Null, |bytes| {
serde_json::Value::String(STANDARD.encode(bytes))
}),
_ => row
.get::<Option<String>, _>(i)
.map_or(serde_json::Value::Null, serde_json::Value::String),
_ => {
// Try different types in order
if let Ok(val) = row.try_get::<i64, _>(i) {
serde_json::Value::Number(val.into())
} else if let Ok(val) = row.try_get::<f64, _>(i) {

Check warning on line 56 in crates/torii/server/src/handlers/sql.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/server/src/handlers/sql.rs#L54-L56 

Added lines #L54 - L56 were not covered by tests
// Handle floating point numbers
serde_json::json!(val)
} else if let Ok(val) = row.try_get::<bool, _>(i) {
serde_json::Value::Bool(val)
} else if let Ok(val) = row.try_get::<String, _>(i) {
serde_json::Value::String(val)

Check warning on line 62 in crates/torii/server/src/handlers/sql.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/server/src/handlers/sql.rs#L58-L62 

Added lines #L58 - L62 were not covered by tests
} else {
// Handle or fallback to BLOB as base64
let val = row.get::<Option<Vec<u8>>, _>(i);
val.map_or(serde_json::Value::Null, |bytes| {
serde_json::Value::String(STANDARD.encode(bytes))
})

Check warning on line 68 in crates/torii/server/src/handlers/sql.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/server/src/handlers/sql.rs#L65-L68 

Added lines #L65 - L68 were not covered by tests
}
},
};
obj.insert(column.name().to_string(), value);
}
Expand Down Expand Up @@ -117,6 +134,28 @@
.unwrap()),
}
}

async fn serve_playground(&self) -> Response<Body> {
let html = include_str!("../../static/sql-playground.html");

Response::builder()
.status(StatusCode::OK)
.header(CONTENT_TYPE, "text/html")
.header("Access-Control-Allow-Origin", "*")
.body(Body::from(html))
.unwrap()
}

Check warning on line 147 in crates/torii/server/src/handlers/sql.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/server/src/handlers/sql.rs#L138-L147 

Added lines #L138 - L147 were not covered by tests
Comment on lines +138 to +147
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ohayo sensei! Let's enhance the playground security!

The playground serving implementation needs some security improvements:

  1. Using unwrap() could cause panics
  2. The Access-Control-Allow-Origin: * header is too permissive
  3. Missing important security headers (CSP, X-Content-Type-Options)

Consider this safer implementation:

 async fn serve_playground(&self) -> Response<Body> {
     let html = include_str!("../../static/sql-playground.html");
 
-    Response::builder()
-        .status(StatusCode::OK)
-        .header(CONTENT_TYPE, "text/html")
-        .header("Access-Control-Allow-Origin", "*")
-        .body(Body::from(html))
-        .unwrap()
+    Response::builder()
+        .status(StatusCode::OK)
+        .header(CONTENT_TYPE, "text/html")
+        .header("Content-Security-Policy", "default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline';")
+        .header("X-Content-Type-Options", "nosniff")
+        .body(Body::from(html))
+        .unwrap_or_else(|e| {
+            Response::builder()
+                .status(StatusCode::INTERNAL_SERVER_ERROR)
+                .body(Body::from(format!("Failed to serve playground: {}", e)))
+                .unwrap_or_default()
+        })

Committable suggestion skipped: line range outside the PR's diff.


async fn handle_request(&self, req: Request<Body>) -> Response<Body> {
if req.method() == Method::GET && req.uri().query().unwrap_or_default().is_empty() {
self.serve_playground().await

Check warning on line 151 in crates/torii/server/src/handlers/sql.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/server/src/handlers/sql.rs#L149-L151 

Added lines #L149 - L151 were not covered by tests
} else {
match self.extract_query(req).await {
Ok(query) => self.execute_query(query).await,
Err(_) => self.serve_playground().await,

Check warning on line 155 in crates/torii/server/src/handlers/sql.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/server/src/handlers/sql.rs#L153-L155 

Added lines #L153 - L155 were not covered by tests
}
}
}

Check warning on line 158 in crates/torii/server/src/handlers/sql.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/server/src/handlers/sql.rs#L158 

Added line #L158 was not covered by tests
Comment on lines +149 to +158
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ohayo! Let's enhance the request validation, sensei!

The current implementation has two areas for improvement:

  1. Falling back to playground on query extraction error masks potential issues
  2. Missing input validation before query execution

Consider this enhanced implementation:

 async fn handle_request(&self, req: Request<Body>) -> Response<Body> {
     if req.method() == Method::GET && req.uri().query().unwrap_or_default().is_empty() {
         self.serve_playground().await
     } else {
         match self.extract_query(req).await {
             Ok(query) => {
+                // Validate query before execution
+                if query.trim().is_empty() {
+                    return Response::builder()
+                        .status(StatusCode::BAD_REQUEST)
+                        .body(Body::from("Empty query"))
+                        .unwrap_or_default();
+                }
                 self.execute_query(query).await
             }
-            Err(_) => self.serve_playground().await,
+            Err(e) => e,  // Return the actual error for better visibility
         }
     }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async fn handle_request(&self, req: Request<Body>) -> Response<Body> {
if req.method() == Method::GET && req.uri().query().unwrap_or_default().is_empty() {
self.serve_playground().await
} else {
match self.extract_query(req).await {
Ok(query) => self.execute_query(query).await,
Err(_) => self.serve_playground().await,
}
}
}
async fn handle_request(&self, req: Request<Body>) -> Response<Body> {
if req.method() == Method::GET && req.uri().query().unwrap_or_default().is_empty() {
self.serve_playground().await
} else {
match self.extract_query(req).await {
Ok(query) => {
// Validate query before execution
if query.trim().is_empty() {
return Response::builder()
.status(StatusCode::BAD_REQUEST)
.body(Body::from("Empty query"))
.unwrap_or_default();
}
self.execute_query(query).await
}
Err(e) => e, // Return the actual error for better visibility
}
}
}

}

#[async_trait::async_trait]
Expand All @@ -126,9 +165,6 @@
}

async fn handle(&self, req: Request<Body>) -> Response<Body> {
match self.extract_query(req).await {
Ok(query) => self.execute_query(query).await,
Err(response) => response,
}
self.handle_request(req).await

Check warning on line 168 in crates/torii/server/src/handlers/sql.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/server/src/handlers/sql.rs#L168 

Added line #L168 was not covered by tests
}
}
Loading
Loading