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-sqlite): primitive upgrades towards bigger types #2931

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+73 −1
Open

feat(torii-sqlite): primitive upgrades towards bigger types #2931

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6fb9f39
refactor(torii): primitive upgrades towards bigger types
Larkooo Jan 20, 2025
0792db5
fmt
Larkooo Jan 20, 2025
f2affb6
fix can upgrade to and column mod
Larkooo Jan 20, 2025
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
74 changes: 73 additions & 1 deletion crates/torii/sqlite/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -847,6 +847,20 @@ fn add_columns_recursive(
));
};

let mut modify_column = |name: &str, sql_type: &str, sql_value: &str| {
// SQLite doesn't support ALTER COLUMN directly, so we need to:
// 1. Create a new temporary column
// 2. Copy the data
// 3. Drop the old column
// 4. Rename the temporary column
alter_table_queries
.push(format!("ALTER TABLE [{table_id}] ADD COLUMN [tmp_{name}] {sql_type}"));
alter_table_queries.push(format!("UPDATE [{table_id}] SET [tmp_{name}] = {sql_value}"));
alter_table_queries.push(format!("ALTER TABLE [{table_id}] DROP COLUMN [{name}]"));
alter_table_queries
.push(format!("ALTER TABLE [{table_id}] RENAME COLUMN [tmp_{name}] TO [{name}]"));
};

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! Address potential SQL injection vulnerability in modify_column.

In the modify_column closure, the sql_value is directly interpolated into the SQL statement without parameterization:

alter_table_queries.push(format!("UPDATE [{table_id}] SET [tmp_{name}] = {sql_value}"));

This approach might expose the code to SQL injection attacks if sql_value contains malicious input. It's essential to use parameterized queries or ensure that sql_value is properly sanitized before use.

Apply this diff to fix the issue by using parameterized queries:

- alter_table_queries.push(format!("UPDATE [{table_id}] SET [tmp_{name}] = {sql_value}"));
+ alter_table_queries.push(format!("UPDATE [{table_id}] SET [tmp_{name}] = ?"));
+ // Ensure to pass `sql_value` as a parameter when executing the query.

match ty {
Ty::Struct(s) => {
for member in &s.children {
Expand Down Expand Up @@ -940,9 +954,67 @@ fn add_columns_recursive(
let column_name =
if column_prefix.is_empty() { "value".to_string() } else { column_prefix };

add_column(&column_name, p.to_sql_type().as_ref());
if let Some(upgrade_diff) = upgrade_diff {
if let Some(old_primitive) = upgrade_diff.as_primitive() {
if old_primitive.can_upgrade_to(p) {
// Modify existing column to new type
modify_column(
&column_name,
p.to_sql_type().as_ref(),
p.to_sql_value().as_ref(),
);
} else {
return Err(anyhow::anyhow!(
"Invalid primitive type upgrade from {:?} to {:?}",
old_primitive,
p
));
}
}
} else {
// New column
add_column(&column_name, p.to_sql_type().as_ref());
}
}
}

Ok(())
}

impl Primitive {
fn can_upgrade_to(&self, other: &Primitive) -> bool {
use Primitive::*;
match (self, other) {
// Boolean (no upgrades)
(Bool(_), Bool(_)) => true,

// Unsigned integer upgrades
(U8(_), U16(_) | U32(_) | USize(_) | U64(_) | U128(_) | Felt252(_)) => true,
(U16(_), U32(_) | USize(_) | U64(_) | U128(_) | Felt252(_)) => true,
(U32(_), USize(_) | U64(_) | U128(_) | Felt252(_)) => true,
(USize(_), U32(_) | U64(_) | U128(_) | Felt252(_)) => true,
(U64(_), U128(_) | Felt252(_)) => true,
(U128(_), Felt252(_)) => true,

// U256 can only upgrade to itself
(U256(_), U256(_)) => true,

// Signed integer upgrades
(I8(_), I16(_) | I32(_) | I64(_) | I128(_) | Felt252(_)) => true,
(I16(_), I32(_) | I64(_) | I128(_) | Felt252(_)) => true,
(I32(_), I64(_) | I128(_) | Felt252(_)) => true,
(I64(_), I128(_) | Felt252(_)) => true,
(I128(_), Felt252(_)) => true,

// Felt252 and address types
(Felt252(_), ClassHash(_) | ContractAddress(_)) => true,
(ClassHash(_), Felt252(_) | ContractAddress(_)) => true,
(ContractAddress(_), Felt252(_) | ClassHash(_)) => true,

// Same type is considered an upgrade (no-op)
(a, b) if std::mem::discriminant(a) == std::mem::discriminant(b) => true,

_ => false,
}
}
}
Loading