feat(torii-server): add MCP service to torii ✨ #2778
Conversation
WalkthroughOhayo, sensei! This pull request updates the Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/torii/server/Cargo.toml(1 hunks)crates/torii/server/src/handlers/mcp.rs(1 hunks)crates/torii/server/src/handlers/mod.rs(1 hunks)
🔇 Additional comments (2)
crates/torii/server/src/handlers/mcp.rs (1)
387-387: Ohayo, sensei! Consider sanitizing user-provided SQL queries
Executing user-provided SQL queries can be risky. Ensure that appropriate safeguards are in place to prevent SQL injection or unintended data access.
I recommend verifying that the queries are safe and possibly implementing a whitelist of allowed queries or parameters.
crates/torii/server/src/handlers/mod.rs (1)
5-5: Ohayo, sensei! The addition of the mcp module looks good
Integrating the new mcp module enhances the handler's capabilities. Nice work!
| .send(Message::Text(serde_json::to_string(&response).unwrap())) | ||
| .await | ||
| { | ||
| eprintln!("Error sending message: {}", e); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Use structured logging instead of eprintln!
Using eprintln! for logging can be inconsistent and less informative. Consider utilizing a logging library like tracing for better log management.
Apply this diff to improve logging:
- eprintln!("Error sending message: {}", e);
+ tracing::error!("Error sending message: {}", e);📝 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.
| eprintln!("Error sending message: {}", e); | |
| tracing::error!("Error sending message: {}", e); |
| let (response, websocket) = hyper_tungstenite::upgrade(req, None) | ||
| .expect("Failed to upgrade WebSocket connection"); |
There was a problem hiding this comment.
Ohayo, sensei! Handle WebSocket upgrade errors without panicking
Using expect here will cause a panic if the WebSocket upgrade fails. It's better to handle this error gracefully to maintain server stability.
Apply this diff to handle the error:
- let (response, websocket) = hyper_tungstenite::upgrade(req, None)
- .expect("Failed to upgrade WebSocket connection");
+ let (response, websocket) = match hyper_tungstenite::upgrade(req, None) {
+ Ok(upgrade) => upgrade,
+ Err(e) => {
+ eprintln!("WebSocket upgrade error: {}", e);
+ return Response::builder()
+ .status(StatusCode::BAD_REQUEST)
+ .body(Body::from("Failed to upgrade WebSocket connection"))
+ .unwrap();
+ }
+ };📝 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.
| let (response, websocket) = hyper_tungstenite::upgrade(req, None) | |
| .expect("Failed to upgrade WebSocket connection"); | |
| let (response, websocket) = match hyper_tungstenite::upgrade(req, None) { | |
| Ok(upgrade) => upgrade, | |
| Err(e) => { | |
| eprintln!("WebSocket upgrade error: {}", e); | |
| return Response::builder() | |
| .status(StatusCode::BAD_REQUEST) | |
| .body(Body::from("Failed to upgrade WebSocket connection")) | |
| .unwrap(); | |
| } | |
| }; |
| let table_name: String = row.try_get("table_name").unwrap(); | ||
| let column_name: String = row.try_get("name").unwrap(); | ||
| let column_type: String = row.try_get("type").unwrap(); | ||
| let not_null: bool = row.try_get::<bool, _>("notnull").unwrap(); | ||
| let pk: bool = row.try_get::<bool, _>("pk").unwrap(); | ||
| let default_value: Option<String> = row.try_get("dflt_value").unwrap(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Avoid unwrapping results—handle potential errors gracefully
Using unwrap() can cause the application to panic if an error occurs. It's safer to handle possible errors using ? or match statements.
Apply this diff to handle errors properly:
- let table_name: String = row.try_get("table_name").unwrap();
- let column_name: String = row.try_get("name").unwrap();
- let column_type: String = row.try_get("type").unwrap();
- let not_null: bool = row.try_get::<bool, _>("notnull").unwrap();
- let pk: bool = row.try_get::<bool, _>("pk").unwrap();
- let default_value: Option<String> = row.try_get("dflt_value").unwrap();
+ let table_name: String = row.try_get("table_name")?;
+ let column_name: String = row.try_get("name")?;
+ let column_type: String = row.try_get("type")?;
+ let not_null: bool = row.try_get::<bool, _>("notnull")?;
+ let pk: bool = row.try_get::<bool, _>("pk")?;
+ let default_value: Option<String> = row.try_get("dflt_value")?;You'll need to adjust the function signature to return Result<JsonRpcResponse, sqlx::Error> and handle the error accordingly.
Committable suggestion skipped: line range outside the PR's diff.
crates/torii/server/Cargo.toml
Outdated
| tokio-tungstenite = "0.22.0" | ||
| hyper-tungstenite = "0.22.0" | ||
| futures-util.workspace = true |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo, sensei! Update dependencies to latest compatible versions
The dependencies need updates:
tokio-tungstenite: Update from 0.22.0 to 0.24.0hyper-tungstenite: Current version 0.22.0 is higher than the latest stable (0.15.0), suggesting a potential version mismatch or compatibility issue
Please update the dependencies in crates/torii/server/Cargo.toml to:
tokio-tungstenite = "0.24.0"
hyper-tungstenite = "0.15.0"🔗 Analysis chain
Ohayo, sensei! Verify dependency versions for security and compatibility
Ensure that the added dependencies are up-to-date and free from known vulnerabilities.
Run the following script to check for the latest versions:
Ohayo, sensei! Dependencies need version updates for better security
The current dependencies are not using the latest stable versions:
tokio-tungstenite: Using 0.22.0, but 0.24.0 is availablehyper-tungstenite: Using 0.22.0, but 0.15.0 is available (Note: current version is higher than latest, suggesting a potential issue)
Let's verify if there are any compatibility constraints in the workspace:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for latest versions of new dependencies
echo "Latest version of tokio-tungstenite:"
curl -s https://crates.io/api/v1/crates/tokio-tungstenite | jq '.crate.max_stable_version'
echo "Latest version of hyper-tungstenite:"
curl -s https://crates.io/api/v1/crates/hyper-tungstenite | jq '.crate.max_stable_version'
Length of output: 365
Script:
#!/bin/bash
# Check workspace dependencies and other related crates
cat Cargo.toml
rg "tokio-tungstenite|hyper-tungstenite" -A 2 -B 2
Length of output: 10280
Larkooo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/torii/server/Cargo.toml(1 hunks)crates/torii/server/src/handlers/mcp.rs(1 hunks)crates/torii/server/src/handlers/mod.rs(1 hunks)crates/torii/server/src/proxy.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/server/src/handlers/mod.rs
- crates/torii/server/Cargo.toml
🔇 Additional comments (4)
crates/torii/server/src/proxy.rs (1)
22-22: Ohayo, sensei! The McpHandler integration looks good! ✨
The handler is properly imported and initialized with the shared connection pool, maintaining consistency with other handlers.
Also applies to: 180-180
crates/torii/server/src/handlers/mcp.rs (3)
524-525: Ohayo, sensei! Handle WebSocket upgrade errors without panicking
Using expect here will cause a panic if the WebSocket upgrade fails. It's better to handle this error gracefully to maintain server stability.
296-296: Ohayo, sensei! Use structured logging instead of eprintln!
Using eprintln! for logging can be inconsistent and less informative. Consider utilizing a logging library like tracing for better log management.
343-348: Ohayo, sensei! Avoid unwrapping results—handle potential errors gracefully
Using unwrap() can cause the application to panic if an error occurs. It's safer to handle possible errors using ? or match statements.
| const JSONRPC_VERSION: &str = "2.0"; | ||
| const MCP_VERSION: &str = "2024-11-05"; |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo, sensei! The MCP version date requires attention
The MCP version "2024-11-05" appears to be a future date that's only referenced in mcp.rs. Since this is used as a protocol version and exposed through the API (protocolVersion), it should reflect a current or past version date to avoid potential compatibility issues.
- In
crates/torii/server/src/handlers/mcp.rs: UpdateMCP_VERSIONto use a current or past date instead of "2024-11-05"
🔗 Analysis chain
Ohayo, sensei! Review the MCP version date
The MCP_VERSION is set to a future date (November 2024). This seems incorrect and might cause confusion. Consider using the current version date or following semantic versioning.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for version consistency across the codebase
# Test: Search for MCP version references. Expect: Consistent version strings.
rg -g '!target' '"2024-11-05"'
Length of output: 112
Script:
#!/bin/bash
# Let's check for any other version patterns or dates in MCP-related files
rg -g '!target' 'MCP.*VERSION|VERSION.*MCP' -A 2 -B 2
# Also check for any documentation or comments about versioning
fd -e md -e rs | xargs rg -l 'MCP.*version|version.*MCP' --ignore-case
Length of output: 907
| if let Some(query) = params.get("query").and_then(Value::as_str) { | ||
| match sqlx::query(query).fetch_all(&*self.pool).await { |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo, sensei! The SQL injection vulnerability is confirmed! 🚨
The code in handle_query_tool directly executes raw SQL queries from user input without any validation or sanitization. This is a critical security issue that allows arbitrary SQL execution through the MCP RPC endpoint at tools/call with name: "query".
Key issues:
- Raw query string from user input is passed directly to
sqlx::query() - No input validation or sanitization
- No query whitelisting
- No parameterization of queries
This endpoint should either be:
- Removed if not needed for production use
- Protected with strong authentication and authorization
- Rewritten to use parameterized queries with a strict whitelist of allowed operations
🔗 Analysis chain
Ohayo, sensei! Critical security vulnerability: SQL injection risk! 🚨
The code executes raw SQL queries from user input without any validation or sanitization. This poses a significant security risk as it allows arbitrary SQL execution.
Consider:
- Implementing a query allowlist
- Using parameterized queries
- Adding input validation
- Restricting database permissions
Let me analyze the SQL injection concern further by checking for any validation or sanitization mechanisms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other instances of raw SQL execution
# Test: Search for direct SQL query execution. Expect: Only safe, parameterized queries.
rg -A 2 'query\(' | grep -v 'bind'
Length of output: 37039
Script:
#!/bin/bash
# Check for SQL query validation or sanitization mechanisms
ast-grep --pattern 'fn handle_query_tool($$$)'
# Check the full context of the SQL query execution
rg -B 10 -A 10 'handle_query_tool'
Length of output: 3670
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2778 +/- ##
==========================================
- Coverage 56.00% 55.59% -0.42%
==========================================
Files 434 435 +1
Lines 55090 55502 +412
==========================================
+ Hits 30854 30855 +1
- Misses 24236 24647 +411 ☔ View full report in Codecov by Sentry. |
| use super::Handler; | ||
|
|
||
| const JSONRPC_VERSION: &str = "2.0"; | ||
| const MCP_VERSION: &str = "2024-11-05"; |
There was a problem hiding this comment.
Do you have a link to the MCP releases to know how to bump this in case it becomes necessary?
There was a problem hiding this comment.
Yes, it uses the latest date of the release. I took it from here: https://github.com/modelcontextprotocol/typescript-sdk/blob/main/src/types.ts#L3
| _jsonrpc: String, | ||
| _method: String, | ||
| _params: Option<Value>, |
There was a problem hiding this comment.
Not used, hence prefixed? Or it's conventional?
There was a problem hiding this comment.
Yes not used yet, didn't work on that yet.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
.github/workflows/ci.yml (1)
Line range hint
34-182: Consider adding a changelog commentFor better maintainability and tracking, consider adding a comment in the workflow files documenting the changes introduced in the new image version (v1.0.5).
name: ci on: push: branches: - main pull_request: env: CARGO_TERM_COLOR: always RUST_VERSION: 1.80.0 + # Changelog: + # v1.0.5 - <Add changes/improvements introduced in this version>🧰 Tools
🪛 actionlint (1.7.4)
61-61: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
crates/torii/grpc/src/server/mod.rs (1)
Line range hint
220-229: Ohayo, sensei! Potential SQL Injection Risk withorder_byParameterThe
order_byparameter is constructed from user input and directly interpolated into SQL queries without proper sanitization. This could lead to SQL injection vulnerabilities. Please ensure to validate and sanitize theorder_byinput or use parameterized queries.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
.devcontainer/devcontainer.json(1 hunks).github/workflows/bench.yml(2 hunks).github/workflows/ci.yml(7 hunks).github/workflows/release-dispatch.yml(1 hunks)crates/torii/core/src/model.rs(3 hunks)crates/torii/grpc/proto/types.proto(2 hunks)crates/torii/grpc/src/server/mod.rs(25 hunks)crates/torii/grpc/src/server/tests/entities_test.rs(1 hunks)crates/torii/grpc/src/types/mod.rs(2 hunks)crates/torii/libp2p/src/server/mod.rs(2 hunks)crates/torii/libp2p/src/tests.rs(2 hunks)crates/torii/libp2p/src/types.rs(1 hunks)crates/torii/server/static/sql-playground.html(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- .devcontainer/devcontainer.json
- .github/workflows/release-dispatch.yml
🔇 Additional comments (19)
.github/workflows/bench.yml (1)
17-17: Ohayo sensei! Verify Docker image version compatibility
The Docker image version has been updated from v1.0.4 to v1.0.5 for both benchmark jobs. While this change aligns with updates in other workflow files, we should ensure:
- This is indeed the latest stable version
- The benchmarking tools and configurations are compatible with the new image
Also applies to: 39-39
✅ Verification successful
Ohayo sensei! The Docker image version is correctly aligned
The version update to v1.0.5 is verified to be correct as:
- It matches the latest release tag from the dojoengine/dojo repository
- The same version is consistently used across all workflow files including CI, release, and benchmarking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Docker image version and availability
# Check if the new image version exists and is accessible
docker pull ghcr.io/dojoengine/dojo-dev:v1.0.5 2>/dev/null
if [ $? -eq 0 ]; then
echo "✅ Image v1.0.5 is available"
else
echo "❌ Image v1.0.5 is not accessible"
fi
# Check if this is the latest version
LATEST_TAG=$(curl -s "https://api.github.com/users/dojoengine/packages/container/dojo-dev/versions" | jq -r '.[0].metadata.container.tags[0]')
echo "Latest available tag: $LATEST_TAG"
Length of output: 401
Script:
#!/bin/bash
# Let's try a different approach to verify the image version
# Check GitHub repository releases for version information
gh api \
-H "Accept: application/vnd.github+json" \
/repos/dojoengine/dojo/releases/latest --jq '.tag_name'
# Search workflows for any other Dojo image versions being used
rg "ghcr.io/dojoengine/dojo-dev:" -A 1 -B 1 .github/workflows/
Length of output: 2417
.github/workflows/ci.yml (1)
34-34: Ohayo sensei! Comprehensive Docker image version update
The Docker image version has been consistently updated from v1.0.4 to v1.0.5 across all CI jobs. This systematic update is good practice, but we should:
- Monitor the first few CI runs to ensure all jobs function correctly with the new image
- Verify that all job-specific tools and dependencies are available in the new image version
Also applies to: 59-59, 126-126, 142-142, 157-157, 166-166, 175-175
✅ Verification successful
The previous attempt failed to access Docker, but we can still analyze the workflow file and related configurations to verify if the image version update is properly aligned with all job requirements. This will help us understand if there are any job-specific configurations that might need attention with the new image version.
Ohayo sensei! The Docker image update looks safe to proceed
After analyzing the workflow file, I can confirm that the image version update is consistent and well-integrated:
- All jobs using the Docker image have compatible configurations
- No job-specific dependencies that would be affected by the version change
- The workflow maintains proper job sequencing with
needsdeclarations - The image is used consistently across test, core tests, examples, clippy, fmt, and docs jobs
The recent workflow runs show some builds in progress and a mix of success/failure, but these appear unrelated to the image version update as the failures predate this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify job-specific tools in the new Docker image
# Check recent workflow runs to detect any failures after the image update
gh api -X GET /repos/dojoengine/dojo/actions/workflows/ci.yml/runs \
--jq '.workflow_runs[0:5] | .[] | {id: .id, status: .status, conclusion: .conclusion, created_at: .created_at}'
# List available tools in the new image
docker run --rm ghcr.io/dojoengine/dojo-dev:v1.0.5 bash -c '
echo "Rust version: $(rustc --version)"
echo "Cargo version: $(cargo --version)"
echo "Scarb version: $(scarb --version)"
echo "Checking essential tools..."
command -v protoc >/dev/null && echo "✅ protoc" || echo "❌ protoc"
command -v sozo >/dev/null && echo "✅ sozo" || echo "❌ sozo"
command -v katana >/dev/null && echo "✅ katana" || echo "❌ katana"
'
Length of output: 1197
Script:
#!/bin/bash
# Let's try a different approach to verify the CI workflow changes
# Check the workflow file for any job-specific dependencies or configurations
# First, let's see the full workflow file
cat .github/workflows/ci.yml
# Then check for any recent changes to the workflow
git log -n 1 --patch -- .github/workflows/ci.yml
# Look for any related configuration files that might need updates
fd -t f "config|settings|env" .github/
Length of output: 6109
crates/torii/grpc/src/server/mod.rs (3)
829-836: Ohayo, sensei! Correct Use of Parameterized Queries
The construction of the SQL query in retrieve_tokens uses placeholders and properly binds parameters, enhancing security against SQL injection attacks.
851-874: Ohayo, sensei! Ensure Proper Parameter Binding in Dynamic Queries
In retrieve_token_balances, while constructing dynamic SQL queries, all user inputs are correctly bound using placeholders. This practice helps prevent SQL injection.
Line range hint 1053-1055: Ohayo, sensei! Consistent Use of Asynchronous Patterns
The use of async fn signatures and await expressions is consistent and follows best practices for asynchronous Rust code.
crates/torii/grpc/proto/types.proto (2)
77-77: Ohayo, sensei! Enhancement with order_by Field in Query Message
The addition of the order_by field in the Query message enhances query flexibility. This allows clients to specify the order of results effectively.
170-178: Ohayo, sensei! Well-Defined OrderBy Message and OrderDirection Enum
The new OrderBy message and OrderDirection enum are properly defined and will facilitate ordering in queries. This aligns with the extended functionality in the server code.
crates/torii/grpc/src/server/tests/entities_test.rs (1)
144-144: Ohayo, sensei! Updated Test with New order_by Parameter
The test function test_entities_queries correctly includes the None value for the new order_by parameter, ensuring compatibility with the updated method signature.
crates/torii/grpc/src/types/mod.rs (2)
82-100: Ohayo, sensei! Addition of OrderBy Struct and OrderDirection Enum
The new OrderBy struct and OrderDirection enum are well-defined. They integrate seamlessly into the existing codebase, enhancing the ability to specify ordering in queries.
271-271: Ohayo, sensei! Correct Implementation of From Trait for Query
The From trait implementation for Query correctly maps the order_by field to its proto representation. This ensures that ordering information is accurately transmitted.
crates/torii/core/src/model.rs (1)
Line range hint 124-205: Ohayo sensei! The ordering implementation looks good! ✨
The addition of the order_by parameter enhances query flexibility while maintaining backward compatibility with the default event_id DESC ordering. The implementation is clean and well-tested.
Let's verify the usage of this new parameter across the codebase:
✅ Verification successful
Ohayo sensei! The order_by parameter implementation is properly handled! 🌟
All calls to build_sql_query have been updated to include the new order_by parameter:
- In
grpc/src/server/mod.rs, both calls properly pass theorder_byparameter - The test case in
model.rspassesNonefor the order_by parameter, which correctly tests the default ordering behavior
The implementation maintains backward compatibility and all calling code has been properly updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for all calls to build_sql_query to ensure they handle the new order_by parameter
# Search for function calls
rg -A 2 "build_sql_query\("
Length of output: 816
Script:
#!/bin/bash
# Let's check the actual implementation of these calls to see if they handle the new parameter correctly
# Check the implementation in grpc/src/server/mod.rs
rg -B 5 -A 10 "build_sql_query\(" "crates/torii/grpc/src/server/mod.rs"
# Check the test implementation
rg -B 5 -A 10 "build_sql_query\(" "crates/torii/core/src/model.rs"
Length of output: 2076
crates/torii/server/static/sql-playground.html (2)
377-377: Ohayo sensei! The dynamic URL construction looks good! 🌟
Using window.location.href makes the application more flexible for different deployment contexts while maintaining security.
511-511: The query execution endpoint update is consistent! 🎯
The URL construction follows the same pattern as the schema endpoint, maintaining consistency throughout the application.
crates/torii/libp2p/src/server/mod.rs (3)
40-41: Ohayo sensei! The updated imports look good.
The necessary types are correctly imported from crate::typed_data and crate::types.
421-434: Ohayo sensei! The construction of calldata appears correct.
The calldata is properly constructed with message_hash, the length of the signature, and the signature elements. This aligns with the expected input for is_valid_signature.
417-434: Ohayo sensei! Verify all usages of validate_signature are updated.
The function validate_signature now accepts a slice of Felt. Please ensure that all calls to this function have been updated to pass &[Felt] accordingly throughout the codebase.
To automate this verification, you can run the following script:
✅ Verification successful
Ohayo sensei! All usages of validate_signature are properly updated!
The verification shows that there is only one usage of validate_signature in the codebase, and it correctly passes a slice of Felt (&data.signature) as the signature parameter, which matches the function's signature that accepts signature: &[Felt]. No issues were found!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for all calls to `validate_signature` and check their arguments.
# Using `ast-grep` to find function calls to `validate_signature` with their arguments.
ast-grep --pattern $'validate_signature($_, $_, $_, $_)' --lang rust
Length of output: 602
Script:
#!/bin/bash
# Let's check the signature of the validate_signature function to ensure it matches the usage
rg "fn validate_signature" -A 5 --type rust
Length of output: 443
crates/torii/libp2p/src/types.rs (1)
9-9: Ohayo sensei! Confirm that all uses of Message are updated to the new signature type.
The signature field in Message has been changed to Vec<Felt>. Please verify that all instances where Message is used have been updated to reflect this change throughout the codebase.
You can run the following script to find all usages of the Message struct:
✅ Verification successful
The previous pattern might have been too specific. Let's try a broader search to understand the usage of Message struct and its signature field throughout the codebase.
Ohayo sensei! The Message struct's signature field change is safe.
Based on the search results, the Message struct with the signature: Vec<Felt> field is only used in the torii/libp2p module. The only usages are in:
torii/libp2p/src/tests.rs: Creating test messagestorii/libp2p/src/server/mod.rs: Handling messages in the servertorii/client/src/client/mod.rs: Publishing messagestorii/libp2p/src/client/mod.rs: Client-side message handling
All other Message structs found in the codebase are different types with different purposes (e.g., in examples, contracts, and other modules). The change is localized to the libp2p module's message type and all its usages are properly contained within the related modules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of `Message` struct and check for `signature` field usage.
ast-grep --pattern $'Message { $_* }' --lang rust
Length of output: 50
Script:
#!/bin/bash
# Let's try a different approach to find Message struct usage
# First, find the struct definition
rg "struct Message" -A 5
# Then find all usages of Message type
rg "\bMessage\b" --type rust
# Also search for signature field access
rg "\.signature" --type rust
Length of output: 14717
crates/torii/libp2p/src/tests.rs (2)
549-549: Ohayo sensei! The import of Message in the test is appropriate.
The Message struct is correctly imported for use in the tests.
696-696: Ohayo sensei! Ensure all tests reflect the changes to Message's signature.
In the test_client_messaging test, the Message struct now uses signature: vec![signature.r, signature.s]. Please verify that all other tests involving Message are updated accordingly.
To automate this verification, you can run the following script:
✅ Verification successful
Ohayo sensei! The Message struct usage is consistent throughout the tests.
After checking the codebase, I found that:
- There is only one instance of
Messagestruct instantiation in the tests - The
Messagestruct definition intypes.rsshowssignature: Vec<Felt>field - The test correctly uses
vec![signature.r, signature.s]which creates aVec<Felt>from the signature components
No other tests need to be updated as this is the only usage of the Message struct in the test file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all test functions that use `Message` and check for `signature` field usage.
ast-grep --pattern $'fn $_() { $$$ \n Message { $_* } }' --lang rust
Length of output: 68
Script:
#!/bin/bash
# Let's try a different approach to find Message struct usage in tests
rg -U "Message\s*\{[^}]*signature[^}]*\}" crates/torii/libp2p/src/tests.rs -B 2 -A 2
Length of output: 248
Script:
#!/bin/bash
# Also search for Message struct instantiation pattern without specific fields
rg "Message\s*\{" crates/torii/libp2p/src/tests.rs
Length of output: 149
Script:
#!/bin/bash
# Let's also check the Message struct definition to understand its structure
rg "struct Message" crates/torii/libp2p/src/ -A 5
Length of output: 264
crates/torii/grpc/src/server/mod.rs
Outdated
| let where_clause = | ||
| format!("[{}].[{}] {comparison_operator} ?", member_clause.model, member_clause.member); | ||
|
|
There was a problem hiding this comment.
Ohayo, sensei! Unsafe SQL Query Construction in where_clause
The where_clause is built using user-provided model and member values without sanitization. This poses a SQL injection risk. Consider validating or sanitizing these inputs to ensure they are safe and conform to expected patterns.
crates/torii/grpc/src/server/mod.rs
Outdated
| let order_by = query | ||
| .order_by | ||
| .iter() | ||
| .map(|order_by| { | ||
| format!( | ||
| "[{}].[{}] {}", | ||
| order_by.model, | ||
| order_by.member, | ||
| match order_by.direction { | ||
| 0 => "ASC", | ||
| 1 => "DESC", | ||
| _ => unreachable!(), | ||
| } | ||
| ) | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| .join(", "); | ||
|
|
||
| let order_by = if order_by.is_empty() { None } else { Some(order_by.as_str()) }; | ||
|
|
There was a problem hiding this comment.
Ohayo, sensei! Potential SQL Injection in order_by Clause
The order_by clause is constructed from user input without validation. This can lead to SQL injection vulnerabilities if malicious input is provided. Please validate or sanitize order_by.model and order_by.member to ensure they are valid identifiers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/torii/server/src/handlers/mcp.rs (1)
394-505: Ohayo, sensei! Consider adding rate limiting and authenticationThe query interface could benefit from additional security measures:
- Rate limiting to prevent abuse
- Authentication/authorization to control access
- Query timeout limits to prevent long-running queries
- Resource usage limits (memory, CPU) to prevent DoS attacks
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/torii/server/src/handlers/mcp.rs(1 hunks)
🔇 Additional comments (5)
crates/torii/server/src/handlers/mcp.rs (5)
14-15: Ohayo, sensei! The MCP version date requires attention
The MCP version "2024-11-05" appears to be a future date. While this matches the typescript-sdk repository, using a future date could cause compatibility issues.
296-296: Ohayo, sensei! Use structured logging instead of eprintln!
Using eprintln! for logging can be inconsistent and less informative.
341-346: Ohayo, sensei! Avoid unwrapping results—handle potential errors gracefully
Using unwrap() can cause the application to panic if an error occurs.
396-397: Ohayo, sensei! The SQL injection vulnerability is confirmed! 🚨
The code directly executes raw SQL queries from user input without any validation or sanitization.
522-523: Ohayo, sensei! Handle WebSocket upgrade errors without panicking
Using expect here will cause a panic if the WebSocket upgrade fails.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crates/torii/server/src/handlers/mcp.rs (2)
34-36: Ohayo, sensei! Consider removing unused fieldsThe fields
_jsonrpc,_method, and_paramsinJsonRpcNotificationare prefixed with underscores indicating they're unused. Since notifications aren't currently handled (as seen in thehandle_websocket_connectionmethod), consider removing this struct until it's needed.
404-453: Ohayo, sensei! Extract value conversion logic into a separate functionThe value conversion logic in
handle_query_toolis complex and could benefit from being extracted into a separate function for better maintainability and reusability.+ fn convert_column_value(row: &sqlx::sqlite::SqliteRow, column: &sqlx::sqlite::SqliteColumn, index: usize) -> serde_json::Value { + match column.type_info().name() { + "TEXT" => row.get::<Option<String>, _>(index) + .map_or(serde_json::Value::Null, serde_json::Value::String), + "INTEGER" => row.get::<Option<i64>, _>(index) + .map_or(serde_json::Value::Null, |n| serde_json::Value::Number(n.into())), + "REAL" => row.get::<Option<f64>, _>(index) + .map_or(serde_json::Value::Null, |f| + serde_json::Number::from_f64(f) + .map_or(serde_json::Value::Null, serde_json::Value::Number)), + "BLOB" => row.get::<Option<Vec<u8>>, _>(index) + .map_or(serde_json::Value::Null, |bytes| + serde_json::Value::String(STANDARD.encode(bytes))), + _ => Self::convert_unknown_type(row, index) + } + } + + fn convert_unknown_type(row: &sqlx::sqlite::SqliteRow, index: usize) -> serde_json::Value { + if let Ok(val) = row.try_get::<i64, _>(index) { + serde_json::Value::Number(val.into()) + } else if let Ok(val) = row.try_get::<f64, _>(index) { + serde_json::json!(val) + } else if let Ok(val) = row.try_get::<bool, _>(index) { + serde_json::Value::Bool(val) + } else if let Ok(val) = row.try_get::<String, _>(index) { + serde_json::Value::String(val) + } else { + row.get::<Option<Vec<u8>>, _>(index) + .map_or(serde_json::Value::Null, |bytes| + serde_json::Value::String(STANDARD.encode(bytes))) + } + }Then update the value conversion in
handle_query_tool:- let value: serde_json::Value = match column.type_info().name() { - // ... existing match arms ... - }; + let value = Self::convert_column_value(row, column, i);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/torii/server/Cargo.toml(1 hunks)crates/torii/server/src/handlers/mcp.rs(1 hunks)crates/torii/server/src/handlers/mod.rs(1 hunks)crates/torii/server/src/proxy.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/server/src/handlers/mod.rs
- crates/torii/server/Cargo.toml
🔇 Additional comments (6)
crates/torii/server/src/proxy.rs (1)
22-22: Ohayo, sensei! The MCP handler integration looks good! ✨
The changes correctly integrate the MCP handler into the proxy's handler list.
Also applies to: 180-180
crates/torii/server/src/handlers/mcp.rs (5)
14-15: Ohayo, sensei! The MCP version date requires attention
The MCP version "2024-11-05" appears to be a future date that's only referenced in mcp.rs. Since this is used as a protocol version and exposed through the API (protocolVersion), it should reflect a current or past version date to avoid potential compatibility issues.
294-296: Ohayo, sensei! Use structured logging instead of eprintln!
Using eprintln! for logging can be inconsistent and less informative. Consider utilizing a logging library like tracing for better log management.
341-346: Ohayo, sensei! Avoid unwrapping results—handle potential errors gracefully
Using unwrap() can cause the application to panic if an error occurs. It's safer to handle possible errors using ? or match statements.
396-397: Ohayo, sensei! The SQL injection vulnerability is confirmed! 🚨
The code in handle_query_tool directly executes raw SQL queries from user input without any validation or sanitization. This is a critical security issue that allows arbitrary SQL execution through the MCP RPC endpoint at tools/call with name: "query".
522-523: Ohayo, sensei! Handle WebSocket upgrade errors without panicking
Using expect here will cause a panic if the WebSocket upgrade fails. It's better to handle this error gracefully to maintain server stability.
Summary by CodeRabbit
New Features
mcp, increasing the overall functionality of the server.Bug Fixes