feat(sozo-walnut): remove sensitive data from dojo configuration file

[marijamijailovic]

Description

This PR add prompt message to prevent user from exspose their private key during verification process

Related issue

Tests

• Yes
• No, because they aren't needed
• No, because I need help

Added to documentation?

• README.md
• Dojo Book
• No documentation needed

Checklist

• I've formatted my code (scripts/prettier.sh, scripts/rust_fmt.sh, scripts/cairo_fmt.sh)
• I've linted my code (scripts/clippy.sh, scripts/docs.sh)
• I've commented my code
• I've requested a review after addressing the comments

Summary by CodeRabbit

• New Features

  • Enhanced error handling for TOML serialization with a new error variant.
  • Updated logic for processing TOML files with specific naming conventions.

• Chores

  • Reorganized dependencies in the project configuration.

[coderabbitai]

Ohayo, sensei! Let me break down these changes for you with precision and clarity.

Walkthrough

The changes introduce modifications to the TOML file handling in the collect_files function of the verification module. Specifically, TOML files prefixed with "dojo_" are now parsed for an "env" table, which is removed if present. Additionally, a new error variant for TOML serialization issues is added to the error handling structure, enhancing the robustness of the module. The Cargo.toml file also sees a reorganization of dependencies, reflecting updates to the workspace configuration.

Changes

File	Change Summary
crates/sozo/walnut/src/verification.rs	Added conditional check for TOML files starting with "dojo_"; removes "env" table if present; handles TOML serialization errors.
crates/sozo/walnut/Cargo.toml	Removed clap.workspace and dojo-utils.workspace; added clap.workspace, dojo-utils.workspace, and toml.workspace for workspace usage.
crates/sozo/walnut/src/lib.rs	Added new error variant TomlSerializationError to handle TOML serialization errors.

Sequence Diagram

sequenceDiagram
    participant User
    participant Verification
    Verification->>User: Display warning message
    Verification->>User: Prompt for confirmation
    alt User confirms (Enter)
        Verification->>Verification: Continue verification process
    else User aborts (Any input)
        Verification->>User: Print abortion message
        Verification-->>User: Terminate verification
    end

Loading

The sequence diagram illustrates the new control flow with the user interaction step, showing how the verification process can be conditionally executed based on user input.

Tip

🌐 Web search-backed reviews and chat

• We have enabled web search-based reviews and chat for all users. This feature allows CodeRabbit to access the latest documentation and information on the web.
• You can disable this feature by setting web_search: false in the knowledge_base settings.
• Please share any feedback in the Discord discussion.

---

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9dbc626 and 9ccd1de.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (3)

• crates/sozo/walnut/Cargo.toml (1 hunks)
• crates/sozo/walnut/src/lib.rs (1 hunks)
• crates/sozo/walnut/src/verification.rs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• crates/sozo/walnut/src/verification.rs

🔇 Additional comments (4)

crates/sozo/walnut/src/lib.rs (1)

48-49: Ohayo! The error handling implementation looks solid, sensei!

The new TomlSerializationError variant is well-integrated with proper error message formatting and automatic conversion from toml::ser::Error. This addition will help handle TOML serialization failures gracefully when processing configuration files.

Let's verify that this error variant is properly used in the verification module. Run this script to check the usage:

✅ Verification successful

🏁 Script executed:

#!/bin/bash
# Search for TomlSerializationError usage in the verification module
rg "TomlSerializationError" crates/sozo/walnut/src/verification.rs

Length of output: 148

---

Ohayo, Sensei! The TomlSerializationError variant is correctly implemented and is being used in the verification module as expected (with the .map_err(Error::TomlSerializationError)? call). This confirms that the new error variant properly converts the underlying toml::ser::Error, aligning with our safe error handling practices.

crates/sozo/walnut/Cargo.toml (3)

10-10: Ohayo, sensei! The updated clap.workspace = true entry on line 10 now properly reflects our workspace configuration. This change appears consistent with our dependency management practices.

---

12-12: Ohayo, sensei! The dojo-utils.workspace = true declaration on line 12 is now correctly aligned as a workspace dependency. Please double-check that any duplicate or obsolete entries have been fully removed from the manifest.

---

20-20: Ohayo, sensei! The inclusion of toml.workspace = true on line 20 is a solid update, especially given our new TOML file handling and error management enhancements. This extra dependency will support safer TOML serialization and align with our overall privacy safeguards.

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

crates/sozo/walnut/src/verification.rs (1)

32-47: Ohayo sensei! Great addition of the security prompt! 🎯

The implementation effectively prevents accidental exposure of sensitive data. The warning message and user interaction flow are well-implemented.

Consider enhancing the warning message with specific examples.

The warning could be more specific about common sensitive files. Here's a suggested enhancement:

-        "⚠️  Warning: You are about to submit all workspace files to walnut.
-        Ensure your project does not contain private keys. Config files like dojo_*.toml will be \
-         uploaded.\n
+        "⚠️  Warning: You are about to submit all workspace files to walnut.
+        Ensure your project does not contain sensitive data such as:
+        - Private keys or wallet seeds
+        - API keys or tokens
+        - Environment files (.env)
+        Config files like dojo_*.toml will be uploaded.\n

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f4b733d and 9dbc626.

📒 Files selected for processing (1)

• crates/sozo/walnut/src/verification.rs (1 hunks)

[glihm]

That's a good addition, thank you for taking care of this.
Since the build doesn't require any sensitive information, wondering if we shouldn't back the user by updating the content of the file before upload.

[env] section is the only one with sensitive info (or maybe IPFS one too, but less important for now). Examining the file before could be a good thing to add, wdyt?

[marijamijailovic]

Hi Glihm, thanks for your comment! I like the idea of checking the file content, specifically the [env] section, but only to see if it contains the private_key field. (From my understanding, this is the only field that exposes a private key.) Then, we can notify the user with a message that potentially includes a list of all dojo_*.toml files containing the private_key field.

However, I am of the opinion that we should not modify the dojo_*.toml file or the private_key field without the user's knowledge.

[glihm]

Hi Glihm, thanks for your comment! I like the idea of checking the file content, specifically the [env] section, but only to see if it contains the private_key field. (From my understanding, this is the only field that exposes a private key.) Then, we can notify the user with a message that potentially includes a list of all dojo_*.toml files containing the private_key field.

However, I am of the opinion that we should not modify the dojo_*.toml file or the private_key field without the user's knowledge.

I agree on the fact that modifying a file without user consent is not good, definitely.
But in you case, you're actually uploading them, no? So when the upload happens, and you know that the configuration file will still work without those keys (since you're only building), it could be stripped out before the upload.

Doing so, you don't modify locally the files, and you ensure user's privacy by not uploading something that could be critical for the user.

[marijamijailovic]

Hi Glihm, thanks for your comment! I like the idea of checking the file content, specifically the [env] section, but only to see if it contains the private_key field. (From my understanding, this is the only field that exposes a private key.) Then, we can notify the user with a message that potentially includes a list of all dojo_.toml files containing the private_key field.
However, I am of the opinion that we should not modify the dojo_.toml file or the private_key field without the user's knowledge.

I agree on the fact that modifying a file without user consent is not good, definitely. But in you case, you're actually uploading them, no? So when the upload happens, and you know that the configuration file will still work without those keys (since you're only building), it could be stripped out before the upload.

Doing so, you don't modify locally the files, and you ensure user's privacy by not uploading something that could be critical for the user.

To clarify, before we upload, we can remove the whole [env] section from the dojo_*.toml files, and proceed with the upload and build process without that section. This way, we avoid uploading anything potentially sensitive, and we can remove this new warning prompt message to the user?

I just wanted to make sure we're aligned on this approach before moving forward with these changes.

[glihm]

Hi Glihm, thanks for your comment! I like the idea of checking the file content, specifically the [env] section, but only to see if it contains the private_key field. (From my understanding, this is the only field that exposes a private key.) Then, we can notify the user with a message that potentially includes a list of all dojo__.toml files containing the private_key field.
However, I am of the opinion that we should not modify the dojo__.toml file or the private_key field without the user's knowledge.

I agree on the fact that modifying a file without user consent is not good, definitely. But in you case, you're actually uploading them, no? So when the upload happens, and you know that the configuration file will still work without those keys (since you're only building), it could be stripped out before the upload.
Doing so, you don't modify locally the files, and you ensure user's privacy by not uploading something that could be critical for the user.

To clarify, before we upload, we can remove the whole [env] section from the dojo_*.toml files, and proceed with the upload and build process without that section. This way, we avoid uploading anything potentially sensitive, and we can remove this new warning prompt message to the user?

I just wanted to make sure we're aligned on this approach before moving forward with these changes.

Yes exactly @marijamijailovic, thank you for confirming, you can go ahead with those changes. The environment is the only one containing private key that can be harmful. IPFS is not much important in the current context.

dojo/crates/dojo/world/src/config/environment.rs

Lines 6 to 12 in bf0c5ee

	pub struct Environment {
	pub rpc_url: Option<String>,
	pub account_address: Option<String>,
	pub private_key: Option<String>,
	pub keystore_path: Option<String>,
	pub keystore_password: Option<String>,
	pub world_address: Option<String>,

[marijamijailovic]

Hi Glihm, thanks for your comment! I like the idea of checking the file content, specifically the [env] section, but only to see if it contains the private_key field. (From my understanding, this is the only field that exposes a private key.) Then, we can notify the user with a message that potentially includes a list of all dojo__.toml files containing the private_key field.
However, I am of the opinion that we should not modify the dojo__.toml file or the private_key field without the user's knowledge.

I agree on the fact that modifying a file without user consent is not good, definitely. But in you case, you're actually uploading them, no? So when the upload happens, and you know that the configuration file will still work without those keys (since you're only building), it could be stripped out before the upload.
Doing so, you don't modify locally the files, and you ensure user's privacy by not uploading something that could be critical for the user.

To clarify, before we upload, we can remove the whole [env] section from the dojo_*.toml files, and proceed with the upload and build process without that section. This way, we avoid uploading anything potentially sensitive, and we can remove this new warning prompt message to the user?
I just wanted to make sure we're aligned on this approach before moving forward with these changes.

Yes exactly @marijamijailovic, thank you for confirming, you can go ahead with those changes. The environment is the only one containing private key that can be harmful. IPFS is not much important in the current context.

dojo/crates/dojo/world/src/config/environment.rs

Lines 6 to 12 in bf0c5ee

	pub struct Environment {
	pub rpc_url: Option<String>,
	pub account_address: Option<String>,
	pub private_key: Option<String>,
	pub keystore_path: Option<String>,
	pub keystore_password: Option<String>,
	pub world_address: Option<String>,

Hi @glihm, I have implemented those changes.

[glihm]

Thank you @marijamijailovic for your iteration on that. 👍

Small duplication of the file_data.insert which could have taken the file_content based on your two cases (dojo config file or not) and then used only once.

Let's move forward with that. :) Thank you very much!

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 25 lines in your changes missing coverage. Please review.

Project coverage is 57.03%. Comparing base (d875ec8) to head (5b5614c).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/sozo/walnut/src/verification.rs	0.00%	25 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2978      +/-   ##
==========================================
- Coverage   57.04%   57.03%   -0.02%     
==========================================
  Files         427      427              
  Lines       56536    56556      +20     
==========================================
+ Hits        32251    32256       +5     
- Misses      24285    24300      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.