fix: deduplicate workspace provisioning#34
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR refactors workspace schema provisioning by extracting table-prefix handling into a public ChangesSchema provisioning refactor with table prefix support
🎯 2 (Simple) | ⏱️ ~8 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/common/src/db/workspaces.rs`:
- Around line 104-111: provision_schema currently interpolates schema_name into
SQL before validation, exposing a SQL injection risk; update provision_schema to
validate schema_name at function entry (reusing the same validation
logic/pattern used by create and scoped_connection) and reject or sanitize
invalid names before calling sqlx::query(format!(...)); ensure callers such as
KronosLibraryClient::provision_workspace can only pass validated names and keep
the CREATE SCHEMA statement unchanged aside from using the already-validated
schema_name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b0b892b-9441-4ca2-a92d-ad70bc0aa656
📒 Files selected for processing (2)
crates/common/src/db/workspaces.rscrates/worker/src/client.rs
| pub async fn provision_schema( | ||
| pool: &PgPool, | ||
| schema_name: &str, | ||
| table_prefix: &str, | ||
| ) -> Result<(), sqlx::Error> { | ||
| sqlx::query(&format!("CREATE SCHEMA IF NOT EXISTS \"{}\"", schema_name)) | ||
| .execute(pool) | ||
| .await?; |
There was a problem hiding this comment.
Add input validation before SQL interpolation in the now-public function.
provision_schema is now a public API, but schema_name is interpolated into SQL at line 109 before scoped_connection performs its validation at line 113. Callers like KronosLibraryClient::provision_workspace pass external input directly, creating a SQL injection vector through quoted identifier escapes.
Validate at function entry, consistent with the create function's pattern.
🛡️ Proposed fix to add validation
pub async fn provision_schema(
pool: &PgPool,
schema_name: &str,
table_prefix: &str,
) -> Result<(), sqlx::Error> {
+ use crate::tenant::validate_table_prefix;
+ assert!(
+ validate_schema_name(schema_name),
+ "Invalid schema name: {}",
+ schema_name
+ );
+ assert!(
+ validate_table_prefix(table_prefix),
+ "Invalid table prefix: {}",
+ table_prefix
+ );
+
sqlx::query(&format!("CREATE SCHEMA IF NOT EXISTS \"{}\"", schema_name))
.execute(pool)
.await?;🧰 Tools
🪛 OpenGrep (1.22.0)
[ERROR] 109-109: SQL query built via format!() passed to a database method. Use parameterized queries with bind parameters instead.
(coderabbit.sql-injection.rust-format-query)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/common/src/db/workspaces.rs` around lines 104 - 111, provision_schema
currently interpolates schema_name into SQL before validation, exposing a SQL
injection risk; update provision_schema to validate schema_name at function
entry (reusing the same validation logic/pattern used by create and
scoped_connection) and reject or sanitize invalid names before calling
sqlx::query(format!(...)); ensure callers such as
KronosLibraryClient::provision_workspace can only pass validated names and keep
the CREATE SCHEMA statement unchanged aside from using the already-validated
schema_name.
Source: Linters/SAST tools
mahatoankitkumar
force-pushed
the
fix/library
branch
from
becf669 to
7fdc372
Compare
mahatoankitkumar
force-pushed
the
fix/library
branch
from
7fdc372 to
b812a21
Compare
| schema_name: &str, | ||
| table_prefix: &str, | ||
| ) -> Result<(), sqlx::Error> { | ||
| sqlx::query(&format!("CREATE SCHEMA IF NOT EXISTS \"{}\"", schema_name)) |
There was a problem hiding this comment.
Possible to use bind parameters here @mahatoankitkumar instead of string concatenation ?
Embedders previously needed their own sqlx dependency for two reasons: building the PgPool to pass to KronosLibraryClient::new, and naming sqlx types (PgPool, sqlx::Error) when implementing SchemaProvider. Re-exporting sqlx from kronos-common removes the need for a separate pin (which could also drift to an incompatible version), and from_database_url + pool() let callers skip pool construction entirely while still sharing the pool. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
This pull request refactors how workspace schema provisioning is handled, centralizing the logic and making it more flexible by supporting table prefixes. The main change is to move the schema creation and DDL application logic into a reusable function, and to ensure the table prefix is consistently applied.
Refactoring and centralization:
provision_schemaincrates/common/src/db/workspaces.rs, making it reusable and easier to maintain.createfunction to use the newprovision_schemasignature, passing an empty string as the table prefix for now.KronosLibraryClient::provision_workspaceimplementation to call the newprovision_schemafunction, removing duplicated logic and ensuring table prefixing is handled consistently.Summary by CodeRabbit