1072 lines
33 KiB
Markdown
1072 lines
33 KiB
Markdown
# Security Fixes Implementation Plan
|
|
|
|
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
|
|
|
**Goal:** Fix all Critical, Important, and Minor security/robustness issues identified in the June 2026 code review.
|
|
|
|
**Architecture:** Fixes are applied in dependency order — shared utilities first, then middleware, then handlers, then startup. No new crates required except `rpassword` is intentionally avoided (plain stdin for first-user prompt is acceptable).
|
|
|
|
**Tech Stack:** Rust, Axum 0.7, SQLx 0.7, jsonwebtoken 9, bcrypt 0.15, DashMap 5, tower-http 0.5
|
|
|
|
---
|
|
|
|
## File Map
|
|
|
|
| File | Changes |
|
|
|------|---------|
|
|
| `src/state.rs` | Fix `unix_now()` panic |
|
|
| `src/routes/mod.rs` | Add shared `is_valid_identifier()` |
|
|
| `src/routes/admin/tables.rs` | Use shared validator, add `PROTECTED_TABLES` denylist |
|
|
| `src/routes/admin/queries.rs` | Sort params longest-first before substitution |
|
|
| `src/routes/crud.rs` | Fix cache key, drop NULL_SENTINEL, enforce R/W/D bits, use shared validator |
|
|
| `src/auth/middleware.rs` | Stash Claims in extensions in `blacklist_layer`; add `authenticate()` helper |
|
|
| `src/config.rs` | Add `cors_origins: Vec<String>` field |
|
|
| `src/main.rs` | First-user interactive prompt, body size limit, configured CORS |
|
|
|
|
---
|
|
|
|
## Task 1: Fix `unix_now()` panic on sub-epoch clock
|
|
|
|
**Files:**
|
|
- Modify: `src/state.rs:16-19`
|
|
|
|
- [ ] **Step 1: Apply the fix**
|
|
|
|
Change `src/state.rs`:
|
|
```rust
|
|
pub fn unix_now() -> u64 {
|
|
SystemTime::now()
|
|
.duration_since(UNIX_EPOCH)
|
|
.unwrap_or_default()
|
|
.as_secs()
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 2: Run tests**
|
|
|
|
```bash
|
|
cargo test
|
|
```
|
|
Expected: all tests pass.
|
|
|
|
- [ ] **Step 3: Commit**
|
|
|
|
```bash
|
|
git add src/state.rs
|
|
git commit -m "fix: don't panic in unix_now() when clock is before epoch"
|
|
```
|
|
|
|
---
|
|
|
|
## Task 2: Fix stored query prefix-substitution bug
|
|
|
|
**Files:**
|
|
- Modify: `src/routes/admin/queries.rs:29-30`
|
|
|
|
**The bug:** When sorted alphabetically, `:user` is replaced before `:username`, turning `:username` into `$1name` — invalid SQL.
|
|
|
|
- [ ] **Step 1: Write a failing test**
|
|
|
|
Add to `src/routes/admin/queries.rs` (at the bottom, inside a `#[cfg(test)]` block — create the block if it doesn't exist):
|
|
|
|
```rust
|
|
#[cfg(test)]
|
|
mod tests {
|
|
#[test]
|
|
fn test_prefix_param_substitution_order() {
|
|
// Simulate the substitution logic with a prefix-conflicting pair.
|
|
let template = "SELECT * FROM t WHERE user_id = :user_id AND username = :username";
|
|
let mut params: Vec<(String, String)> = vec![
|
|
("user_id".into(), "42".into()),
|
|
("username".into(), "alice".into()),
|
|
];
|
|
// Sort longest-first (the fix)
|
|
params.sort_by(|a, b| b.0.len().cmp(&a.0.len()));
|
|
let mut sql = template.to_string();
|
|
for (i, (name, _)) in params.iter().enumerate() {
|
|
sql = sql.replace(&format!(":{}", name), &format!("${}", i + 1));
|
|
}
|
|
assert!(sql.contains("$1") && sql.contains("$2"), "sql: {}", sql);
|
|
assert!(!sql.contains(":username"), "placeholder not replaced: {}", sql);
|
|
assert!(!sql.contains(":user_id"), "placeholder not replaced: {}", sql);
|
|
// username (len 8) should be $1, user_id (len 7) should be $2
|
|
assert_eq!(sql, "SELECT * FROM t WHERE user_id = $2 AND username = $1");
|
|
}
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 2: Run test to confirm it fails before the fix**
|
|
|
|
```bash
|
|
cargo test test_prefix_param_substitution_order
|
|
```
|
|
Expected: FAIL (currently sorts alphabetically, not by length).
|
|
|
|
- [ ] **Step 3: Apply the fix in `execute_query`**
|
|
|
|
In `src/routes/admin/queries.rs`, change line 30 from:
|
|
```rust
|
|
sorted_params.sort_by(|a, b| a.0.cmp(&b.0));
|
|
```
|
|
to:
|
|
```rust
|
|
sorted_params.sort_by(|a, b| b.0.len().cmp(&a.0.len()).then(a.0.cmp(&b.0)));
|
|
```
|
|
|
|
- [ ] **Step 4: Run test to confirm it passes**
|
|
|
|
```bash
|
|
cargo test test_prefix_param_substitution_order
|
|
```
|
|
Expected: PASS.
|
|
|
|
- [ ] **Step 5: Commit**
|
|
|
|
```bash
|
|
git add src/routes/admin/queries.rs
|
|
git commit -m "fix: sort stored query params longest-first to prevent prefix substitution corruption"
|
|
```
|
|
|
|
---
|
|
|
|
## Task 3: Fix cache key collision between by-id lookup and column filter
|
|
|
|
**Files:**
|
|
- Modify: `src/routes/crud.rs:48`
|
|
|
|
**The bug:** `GET /api/users/by_id` and `GET /api/users?by_id=foo` both produce key `"GET:users:by_id"`. Tilde (`~`) is not a valid identifier character so `"~id"` can never collide with a column name.
|
|
|
|
- [ ] **Step 1: Update the existing test to expect the new key**
|
|
|
|
In `src/routes/crud.rs` tests, change `test_build_select_by_id`:
|
|
```rust
|
|
#[test]
|
|
fn test_build_select_by_id() {
|
|
let (sql, params, key) = build_query("GET", "orders", Some("42"), &[], &[]).unwrap();
|
|
assert_eq!(sql, "SELECT * FROM orders WHERE id = $1");
|
|
assert_eq!(params, vec![Value::String("42".into())]);
|
|
assert_eq!(key, "GET:orders:~id");
|
|
}
|
|
```
|
|
|
|
> Note: `params` type changes to `Vec<Value>` in Task 6. For now, keep as `Vec<String>` and update `"by_id"` → `"~id"` only.
|
|
>
|
|
> If doing this task before Task 6, the test assertion for params stays `vec!["42".to_string()]`. Just change the key assertion to `"GET:orders:~id"`.
|
|
|
|
- [ ] **Step 2: Apply the fix**
|
|
|
|
In `src/routes/crud.rs:48`, change:
|
|
```rust
|
|
let key = format!("GET:{}:by_id", table);
|
|
```
|
|
to:
|
|
```rust
|
|
let key = format!("GET:{}:~id", table);
|
|
```
|
|
|
|
- [ ] **Step 3: Run tests**
|
|
|
|
```bash
|
|
cargo test
|
|
```
|
|
Expected: all tests pass.
|
|
|
|
- [ ] **Step 4: Commit**
|
|
|
|
```bash
|
|
git add src/routes/crud.rs
|
|
git commit -m "fix: change by-id cache key to ~id to prevent column name collision"
|
|
```
|
|
|
|
---
|
|
|
|
## Task 4: Extract shared `is_valid_identifier` to eliminate duplication
|
|
|
|
**Files:**
|
|
- Modify: `src/routes/mod.rs`
|
|
- Modify: `src/routes/crud.rs:16-22`
|
|
- Modify: `src/routes/admin/tables.rs:17-23`
|
|
|
|
- [ ] **Step 1: Add the shared function to `src/routes/mod.rs`**
|
|
|
|
Replace the current content of `src/routes/mod.rs`:
|
|
```rust
|
|
pub mod admin;
|
|
pub mod auth;
|
|
pub mod crud;
|
|
|
|
/// Returns true if `name` is a safe SQL identifier (non-empty, alphanumeric + underscore only).
|
|
pub fn is_valid_identifier(name: &str) -> bool {
|
|
!name.is_empty() && name.chars().all(|c| c.is_alphanumeric() || c == '_')
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 2: Update `src/routes/crud.rs` to use the shared function**
|
|
|
|
Remove the local `validate_identifier` (lines 16-22):
|
|
```rust
|
|
// DELETE this function entirely:
|
|
fn validate_identifier(name: &str) -> Result<()> {
|
|
if name.chars().all(|c| c.is_alphanumeric() || c == '_') {
|
|
Ok(())
|
|
} else {
|
|
Err(anyhow!("invalid identifier: {}", name))
|
|
}
|
|
}
|
|
```
|
|
|
|
Replace the two call sites in `build_query`:
|
|
```rust
|
|
// Line 34 — was: validate_identifier(table)?;
|
|
if !crate::routes::is_valid_identifier(table) {
|
|
return Err(anyhow!("invalid identifier: {}", table));
|
|
}
|
|
// Line 35-37 — was: for (col, _) in ... { validate_identifier(col)?; }
|
|
for (col, _) in body_cols.iter().chain(filter_cols.iter()) {
|
|
if !crate::routes::is_valid_identifier(col) {
|
|
return Err(anyhow!("invalid identifier: {}", col));
|
|
}
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 3: Update `src/routes/admin/tables.rs` to use the shared function**
|
|
|
|
Remove the local `validate_identifier` (lines 17-23):
|
|
```rust
|
|
// DELETE this function entirely:
|
|
fn validate_identifier(name: &str) -> Result<(), StatusCode> {
|
|
if !name.is_empty() && name.chars().all(|c| c.is_alphanumeric() || c == '_') {
|
|
Ok(())
|
|
} else {
|
|
Err(StatusCode::BAD_REQUEST)
|
|
}
|
|
}
|
|
```
|
|
|
|
Replace all three call sites:
|
|
```rust
|
|
// In get_table_preview, create_table, drop_table — replace validate_identifier(&name)? with:
|
|
if !crate::routes::is_valid_identifier(&name) {
|
|
return Err(StatusCode::BAD_REQUEST);
|
|
}
|
|
// In create_table column loop — replace validate_identifier(&col.name)? with:
|
|
if !crate::routes::is_valid_identifier(&col.name) {
|
|
return Err(StatusCode::BAD_REQUEST);
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 4: Run tests**
|
|
|
|
```bash
|
|
cargo test
|
|
```
|
|
Expected: all tests pass.
|
|
|
|
- [ ] **Step 5: Commit**
|
|
|
|
```bash
|
|
git add src/routes/mod.rs src/routes/crud.rs src/routes/admin/tables.rs
|
|
git commit -m "refactor: extract shared is_valid_identifier to eliminate duplication"
|
|
```
|
|
|
|
---
|
|
|
|
## Task 5: Add protected-table denylist to DDL operations
|
|
|
|
**Files:**
|
|
- Modify: `src/routes/admin/tables.rs`
|
|
|
|
- [ ] **Step 1: Write failing tests**
|
|
|
|
Add to `src/routes/admin/tables.rs` at the bottom:
|
|
|
|
```rust
|
|
#[cfg(test)]
|
|
mod tests {
|
|
use super::PROTECTED_TABLES;
|
|
|
|
#[test]
|
|
fn test_protected_tables_list() {
|
|
assert!(PROTECTED_TABLES.contains(&"users"));
|
|
assert!(PROTECTED_TABLES.contains(&"blacklist"));
|
|
assert!(PROTECTED_TABLES.contains(&"api_keys"));
|
|
assert!(PROTECTED_TABLES.contains(&"queries"));
|
|
assert!(PROTECTED_TABLES.contains(&"permissions"));
|
|
}
|
|
|
|
#[test]
|
|
fn test_is_protected() {
|
|
assert!(is_protected("users"));
|
|
assert!(is_protected("USERS")); // case-insensitive
|
|
assert!(!is_protected("orders"));
|
|
}
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 2: Run test to confirm it fails**
|
|
|
|
```bash
|
|
cargo test test_protected_tables_list test_is_protected
|
|
```
|
|
Expected: FAIL — `PROTECTED_TABLES` and `is_protected` not defined yet.
|
|
|
|
- [ ] **Step 3: Add the denylist and helper**
|
|
|
|
At the top of `src/routes/admin/tables.rs`, after the `use` imports, add:
|
|
|
|
```rust
|
|
const PROTECTED_TABLES: &[&str] = &["users", "blacklist", "api_keys", "queries", "permissions"];
|
|
|
|
fn is_protected(name: &str) -> bool {
|
|
let lower = name.to_lowercase();
|
|
PROTECTED_TABLES.iter().any(|&t| t == lower)
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 4: Guard `drop_table`**
|
|
|
|
In `drop_table`, after `validate_identifier(&name)?;`, add:
|
|
```rust
|
|
if is_protected(&name) {
|
|
return Err(StatusCode::FORBIDDEN);
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 5: Guard `create_table`**
|
|
|
|
In `create_table`, after `validate_identifier(&body.name)?;`, add:
|
|
```rust
|
|
if is_protected(&body.name) {
|
|
return Err(StatusCode::FORBIDDEN);
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 6: Run tests**
|
|
|
|
```bash
|
|
cargo test
|
|
```
|
|
Expected: all tests pass.
|
|
|
|
- [ ] **Step 7: Commit**
|
|
|
|
```bash
|
|
git add src/routes/admin/tables.rs
|
|
git commit -m "fix: block DDL operations on protected system tables"
|
|
```
|
|
|
|
---
|
|
|
|
## Task 6: Eliminate NULL_SENTINEL — thread `serde_json::Value` through `build_query`
|
|
|
|
**Files:**
|
|
- Modify: `src/routes/crud.rs` (signature of `build_query`, binding loop, password hashing)
|
|
|
|
**The bug:** `"\x00NULL"` is an in-band sentinel — a real string field containing those bytes would be silently written as SQL NULL.
|
|
|
|
- [ ] **Step 1: Update `build_query` signature and return type**
|
|
|
|
Change the function signature and all `Vec<String>` params returns to `Vec<Value>`. Full new function (replace lines 27-114):
|
|
|
|
```rust
|
|
/// Returns (sql, ordered_param_values, cache_key)
|
|
pub fn build_query(
|
|
method: &str,
|
|
table: &str,
|
|
id: Option<&str>,
|
|
body_cols: &[(String, Value)],
|
|
filter_cols: &[(String, String)],
|
|
) -> Result<(String, Vec<Value>, String)> {
|
|
if !crate::routes::is_valid_identifier(table) {
|
|
return Err(anyhow!("invalid identifier: {}", table));
|
|
}
|
|
for (col, _) in body_cols.iter() {
|
|
if !crate::routes::is_valid_identifier(col) {
|
|
return Err(anyhow!("invalid identifier: {}", col));
|
|
}
|
|
}
|
|
for (col, _) in filter_cols.iter() {
|
|
if !crate::routes::is_valid_identifier(col) {
|
|
return Err(anyhow!("invalid identifier: {}", col));
|
|
}
|
|
}
|
|
|
|
let mut sorted_body = body_cols.to_vec();
|
|
sorted_body.sort_by(|a, b| a.0.cmp(&b.0));
|
|
let mut sorted_filters = filter_cols.to_vec();
|
|
sorted_filters.sort_by(|a, b| a.0.cmp(&b.0));
|
|
|
|
match method.to_uppercase().as_str() {
|
|
"GET" => {
|
|
if let Some(id_val) = id {
|
|
let sql = format!("SELECT * FROM {} WHERE id = $1", table);
|
|
let key = format!("GET:{}:~id", table);
|
|
Ok((sql, vec![Value::String(id_val.to_string())], key))
|
|
} else if sorted_filters.is_empty() {
|
|
let sql = format!("SELECT * FROM {}", table);
|
|
let key = format!("GET:{}:", table);
|
|
Ok((sql, vec![], key))
|
|
} else {
|
|
let col_names: Vec<String> = sorted_filters.iter().map(|(c, _)| c.clone()).collect();
|
|
let where_clause: Vec<String> = col_names
|
|
.iter()
|
|
.enumerate()
|
|
.map(|(i, c)| format!("{} = ${}", c, i + 1))
|
|
.collect();
|
|
let sql = format!("SELECT * FROM {} WHERE {}", table, where_clause.join(" AND "));
|
|
let params: Vec<Value> = sorted_filters.iter().map(|(_, v)| Value::String(v.clone())).collect();
|
|
let key = format!("GET:{}:{}", table, col_names.join(","));
|
|
Ok((sql, params, key))
|
|
}
|
|
}
|
|
"POST" => {
|
|
if sorted_body.is_empty() {
|
|
return Err(anyhow!("POST requires a body with at least one field"));
|
|
}
|
|
let cols: Vec<String> = sorted_body.iter().map(|(c, _)| c.clone()).collect();
|
|
let placeholders: Vec<String> = (1..=cols.len()).map(|i| format!("${}", i)).collect();
|
|
let sql = format!(
|
|
"INSERT INTO {} ({}) VALUES ({}) RETURNING *",
|
|
table,
|
|
cols.join(", "),
|
|
placeholders.join(", ")
|
|
);
|
|
let params: Vec<Value> = sorted_body.iter().map(|(_, v)| v.clone()).collect();
|
|
let key = format!("POST:{}:{}", table, cols.join(","));
|
|
Ok((sql, params, key))
|
|
}
|
|
"PUT" => {
|
|
let id_val = id.ok_or_else(|| anyhow!("PUT requires an id"))?;
|
|
if sorted_body.is_empty() {
|
|
return Err(anyhow!("PUT requires a body with at least one field"));
|
|
}
|
|
let cols: Vec<String> = sorted_body.iter().map(|(c, _)| c.clone()).collect();
|
|
let set_clause: Vec<String> = cols
|
|
.iter()
|
|
.enumerate()
|
|
.map(|(i, c)| format!("{} = ${}", c, i + 1))
|
|
.collect();
|
|
let id_placeholder = cols.len() + 1;
|
|
let sql = format!(
|
|
"UPDATE {} SET {} WHERE id = ${} RETURNING *",
|
|
table,
|
|
set_clause.join(", "),
|
|
id_placeholder
|
|
);
|
|
let mut params: Vec<Value> = sorted_body.iter().map(|(_, v)| v.clone()).collect();
|
|
params.push(Value::String(id_val.to_string()));
|
|
let key = format!("PUT:{}:{}:by_id", table, cols.join(","));
|
|
Ok((sql, params, key))
|
|
}
|
|
"DELETE" => {
|
|
let id_val = id.ok_or_else(|| anyhow!("DELETE requires an id"))?;
|
|
let sql = format!("DELETE FROM {} WHERE id = $1", table);
|
|
let key = format!("DELETE:{}:by_id", table);
|
|
Ok((sql, vec![Value::String(id_val.to_string())], key))
|
|
}
|
|
m => Err(anyhow!("unsupported method: {}", m)),
|
|
}
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 2: Update `handle_crud` — simplify body collection and remove sentinel**
|
|
|
|
Replace the body collection block (lines 206-234) in `handle_crud`:
|
|
|
|
```rust
|
|
// Body params: collect as typed Values directly
|
|
let mut body_cols: Vec<(String, Value)> = body
|
|
.map(|Json(b)| b.into_iter().collect())
|
|
.unwrap_or_default();
|
|
|
|
// Hash the password field for the users table before building the query.
|
|
if table == "users" && matches!(method_str.to_uppercase().as_str(), "POST" | "PUT") {
|
|
if let Some(pos) = body_cols.iter().position(|(k, _)| k == "password") {
|
|
let (_, val) = body_cols.remove(pos);
|
|
if let Value::String(plaintext) = val {
|
|
let hash = bcrypt::hash(&plaintext, bcrypt::DEFAULT_COST)
|
|
.map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
|
|
body_cols.push(("password_hash".to_string(), Value::String(hash)));
|
|
}
|
|
}
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 3: Update the binding loop in `handle_crud`**
|
|
|
|
Replace the binding loop (after `let mut q = sqlx::query(&sql);`):
|
|
|
|
```rust
|
|
let mut q = sqlx::query(&sql);
|
|
for val in ¶ms_vals {
|
|
match val {
|
|
Value::Null => q = q.bind(Option::<String>::None),
|
|
Value::Bool(b) => q = q.bind(*b),
|
|
Value::Number(n) => {
|
|
if let Some(i) = n.as_i64() {
|
|
q = q.bind(i);
|
|
} else if let Some(f) = n.as_f64() {
|
|
q = q.bind(f);
|
|
} else {
|
|
q = q.bind(n.to_string());
|
|
}
|
|
}
|
|
Value::String(s) => q = q.bind(s.as_str()),
|
|
other => q = q.bind(other.to_string()),
|
|
}
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 4: Update unit tests for `build_query` to use `Value`**
|
|
|
|
In the `#[cfg(test)]` block at the bottom of `src/routes/crud.rs`, update all `build_query` calls (the signature now takes `&[(String, Value)]` for body_cols and returns `Vec<Value>`).
|
|
|
|
Update `test_build_insert`:
|
|
```rust
|
|
#[test]
|
|
fn test_build_insert() {
|
|
let cols = vec![
|
|
("email".into(), Value::String("a@b.com".into())),
|
|
("name".into(), Value::String("Alice".into())),
|
|
];
|
|
let (sql, params, key) = build_query("POST", "users", None, &cols, &[]).unwrap();
|
|
assert_eq!(sql, "INSERT INTO users (email, name) VALUES ($1, $2) RETURNING *");
|
|
assert_eq!(params, vec![Value::String("a@b.com".into()), Value::String("Alice".into())]);
|
|
assert_eq!(key, "POST:users:email,name");
|
|
}
|
|
```
|
|
|
|
Update `test_build_update`:
|
|
```rust
|
|
#[test]
|
|
fn test_build_update() {
|
|
let cols = vec![("name".into(), Value::String("Bob".into()))];
|
|
let (sql, params, key) = build_query("PUT", "users", Some("7"), &cols, &[]).unwrap();
|
|
assert_eq!(sql, "UPDATE users SET name = $1 WHERE id = $2 RETURNING *");
|
|
assert_eq!(params, vec![Value::String("Bob".into()), Value::String("7".into())]);
|
|
assert_eq!(key, "PUT:users:name:by_id");
|
|
}
|
|
```
|
|
|
|
Update `test_build_select_by_id` (already changed key in Task 3):
|
|
```rust
|
|
#[test]
|
|
fn test_build_select_by_id() {
|
|
let (sql, params, key) = build_query("GET", "orders", Some("42"), &[], &[]).unwrap();
|
|
assert_eq!(sql, "SELECT * FROM orders WHERE id = $1");
|
|
assert_eq!(params, vec![Value::String("42".into())]);
|
|
assert_eq!(key, "GET:orders:~id");
|
|
}
|
|
```
|
|
|
|
Update `test_build_delete`:
|
|
```rust
|
|
#[test]
|
|
fn test_build_delete() {
|
|
let (sql, params, key) = build_query("DELETE", "users", Some("3"), &[], &[]).unwrap();
|
|
assert_eq!(sql, "DELETE FROM users WHERE id = $1");
|
|
assert_eq!(params, vec![Value::String("3".into())]);
|
|
assert_eq!(key, "DELETE:users:by_id");
|
|
}
|
|
```
|
|
|
|
Update `test_build_select_all` (no body/filter params, just check it compiles):
|
|
```rust
|
|
#[test]
|
|
fn test_build_select_all() {
|
|
let (sql, params, key) = build_query("GET", "orders", None, &[], &[]).unwrap();
|
|
assert_eq!(sql, "SELECT * FROM orders");
|
|
assert!(params.is_empty());
|
|
assert_eq!(key, "GET:orders:");
|
|
}
|
|
```
|
|
|
|
Update `test_build_select_with_filters`:
|
|
```rust
|
|
#[test]
|
|
fn test_build_select_with_filters() {
|
|
let filters = vec![("status".into(), "active".into()), ("role".into(), "admin".into())];
|
|
let (sql, params, key) = build_query("GET", "users", None, &[], &filters).unwrap();
|
|
assert_eq!(sql, "SELECT * FROM users WHERE role = $1 AND status = $2");
|
|
assert_eq!(params, vec![Value::String("admin".into()), Value::String("active".into())]);
|
|
assert_eq!(key, "GET:users:role,status");
|
|
}
|
|
```
|
|
|
|
`test_rejects_invalid_table_name` needs no change.
|
|
|
|
- [ ] **Step 5: Run tests**
|
|
|
|
```bash
|
|
cargo test
|
|
```
|
|
Expected: all tests pass.
|
|
|
|
- [ ] **Step 6: Commit**
|
|
|
|
```bash
|
|
git add src/routes/crud.rs
|
|
git commit -m "fix: eliminate NULL_SENTINEL by threading serde_json::Value through build_query"
|
|
```
|
|
|
|
---
|
|
|
|
## Task 7: Stash Claims in `blacklist_layer` to eliminate double DB hit
|
|
|
|
**Files:**
|
|
- Modify: `src/auth/middleware.rs`
|
|
|
|
**The fix:** Add an `authenticate()` helper. `blacklist_layer` stashes resolved `Claims` in request extensions. All `require_*` functions check extensions first, skipping the DB call if already authenticated.
|
|
|
|
- [ ] **Step 1: Rewrite `src/auth/middleware.rs`**
|
|
|
|
Replace the entire file:
|
|
|
|
```rust
|
|
use axum::{
|
|
extract::{Request, State},
|
|
http::StatusCode,
|
|
middleware::Next,
|
|
response::Response,
|
|
};
|
|
|
|
use crate::{auth::{decode_jwt, resolve_api_key, Claims}, state::AppState};
|
|
|
|
/// Resolves a Bearer token to Claims, trying JWT then API key.
|
|
/// On success, inserts Claims into request extensions so downstream middleware
|
|
/// can reuse them without hitting the database again.
|
|
async fn authenticate(
|
|
token: &str,
|
|
state: &AppState,
|
|
req: &mut Request,
|
|
) -> Option<Claims> {
|
|
if let Some(existing) = req.extensions().get::<Claims>().cloned() {
|
|
return Some(existing);
|
|
}
|
|
let claims = if let Ok(c) = decode_jwt(token, &state.config.jwt_secret) {
|
|
c
|
|
} else {
|
|
resolve_api_key(token, &state.pool).await?
|
|
};
|
|
req.extensions_mut().insert(claims.clone());
|
|
Some(claims)
|
|
}
|
|
|
|
pub async fn blacklist_layer(
|
|
State(state): State<AppState>,
|
|
mut req: Request,
|
|
next: Next,
|
|
) -> Result<Response, StatusCode> {
|
|
let method = req.method().as_str().to_uppercase();
|
|
let path = req.uri().path().to_string();
|
|
let caller_mask = if let Some(token) = extract_bearer(&req) {
|
|
authenticate(&token, &state, &mut req)
|
|
.await
|
|
.map(|c| c.permissions_mask())
|
|
.unwrap_or(0)
|
|
} else {
|
|
0
|
|
};
|
|
if state.blacklist_cache.is_blocked(&method, &path, caller_mask).await {
|
|
return Err(StatusCode::FORBIDDEN);
|
|
}
|
|
Ok(next.run(req).await)
|
|
}
|
|
|
|
pub fn extract_bearer(req: &Request) -> Option<String> {
|
|
req.headers()
|
|
.get("authorization")
|
|
.and_then(|v| v.to_str().ok())
|
|
.and_then(|v| v.strip_prefix("Bearer "))
|
|
.map(|s| s.to_string())
|
|
}
|
|
|
|
pub async fn require_auth(
|
|
State(state): State<AppState>,
|
|
mut req: Request,
|
|
next: Next,
|
|
) -> Result<Response, StatusCode> {
|
|
let token = extract_bearer(&req).ok_or(StatusCode::UNAUTHORIZED)?;
|
|
authenticate(&token, &state, &mut req).await.ok_or(StatusCode::UNAUTHORIZED)?;
|
|
Ok(next.run(req).await)
|
|
}
|
|
|
|
pub async fn require_super_admin(
|
|
State(state): State<AppState>,
|
|
mut req: Request,
|
|
next: Next,
|
|
) -> Result<Response, StatusCode> {
|
|
let token = extract_bearer(&req).ok_or(StatusCode::UNAUTHORIZED)?;
|
|
let claims = authenticate(&token, &state, &mut req).await.ok_or(StatusCode::UNAUTHORIZED)?;
|
|
if !claims.has_permission(crate::auth::permissions::SUPER_ADMIN) {
|
|
return Err(StatusCode::FORBIDDEN);
|
|
}
|
|
Ok(next.run(req).await)
|
|
}
|
|
|
|
pub async fn require_admin_query(
|
|
State(state): State<AppState>,
|
|
mut req: Request,
|
|
next: Next,
|
|
) -> Result<Response, StatusCode> {
|
|
let token = extract_bearer(&req).ok_or(StatusCode::UNAUTHORIZED)?;
|
|
let claims = authenticate(&token, &state, &mut req).await.ok_or(StatusCode::UNAUTHORIZED)?;
|
|
if !claims.has_permission(crate::auth::permissions::ADMIN_QUERY) {
|
|
return Err(StatusCode::FORBIDDEN);
|
|
}
|
|
Ok(next.run(req).await)
|
|
}
|
|
|
|
pub async fn require_admin_cache(
|
|
State(state): State<AppState>,
|
|
mut req: Request,
|
|
next: Next,
|
|
) -> Result<Response, StatusCode> {
|
|
let token = extract_bearer(&req).ok_or(StatusCode::UNAUTHORIZED)?;
|
|
let claims = authenticate(&token, &state, &mut req).await.ok_or(StatusCode::UNAUTHORIZED)?;
|
|
if !claims.has_permission(crate::auth::permissions::ADMIN_CACHE) {
|
|
return Err(StatusCode::FORBIDDEN);
|
|
}
|
|
Ok(next.run(req).await)
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 2: Run tests**
|
|
|
|
```bash
|
|
cargo test
|
|
```
|
|
Expected: all tests pass.
|
|
|
|
- [ ] **Step 3: Commit**
|
|
|
|
```bash
|
|
git add src/auth/middleware.rs
|
|
git commit -m "fix: stash Claims in request extensions to eliminate double DB hit per API key request"
|
|
```
|
|
|
|
---
|
|
|
|
## Task 8: Enforce READ/WRITE/DELETE permission bits in CRUD handler
|
|
|
|
**Files:**
|
|
- Modify: `src/routes/crud.rs`
|
|
|
|
- [ ] **Step 1: Add `Extension(claims)` to `handle_crud` and check permissions**
|
|
|
|
Change the `handle_crud` signature to extract `Claims`:
|
|
|
|
```rust
|
|
pub async fn handle_crud(
|
|
State(state): State<AppState>,
|
|
method: Method,
|
|
Extension(claims): Extension<Claims>,
|
|
Path(params): Path<HashMap<String, String>>,
|
|
Query(query_params): Query<HashMap<String, String>>,
|
|
body: Option<Json<HashMap<String, Value>>>,
|
|
) -> Result<Json<Value>, StatusCode> {
|
|
```
|
|
|
|
Then add permission check immediately after extracting `method_str`:
|
|
|
|
```rust
|
|
let method_str = method.as_str();
|
|
|
|
// Enforce permission bits before doing any work.
|
|
let required_bit = match method_str.to_uppercase().as_str() {
|
|
"GET" => crate::auth::permissions::READ,
|
|
"POST" | "PUT" => crate::auth::permissions::WRITE,
|
|
"DELETE" => crate::auth::permissions::DELETE,
|
|
_ => return Err(StatusCode::METHOD_NOT_ALLOWED),
|
|
};
|
|
if !claims.has_permission(required_bit) {
|
|
return Err(StatusCode::FORBIDDEN);
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 2: Run tests**
|
|
|
|
```bash
|
|
cargo test
|
|
```
|
|
Expected: all tests pass. (`build_query` unit tests don't call `handle_crud`, so they're unaffected.)
|
|
|
|
- [ ] **Step 3: Commit**
|
|
|
|
```bash
|
|
git add src/routes/crud.rs
|
|
git commit -m "fix: enforce READ/WRITE/DELETE permission bits in CRUD handler"
|
|
```
|
|
|
|
---
|
|
|
|
## Task 9: Add configurable CORS and body size limit
|
|
|
|
**Files:**
|
|
- Modify: `src/config.rs`
|
|
- Modify: `src/main.rs`
|
|
|
|
- [ ] **Step 1: Add `cors_origins` to `Config`**
|
|
|
|
In `src/config.rs`, add the field and parsing:
|
|
|
|
```rust
|
|
use anyhow::Result;
|
|
|
|
#[derive(Clone, Debug)]
|
|
pub struct Config {
|
|
pub database_url: String,
|
|
pub jwt_secret: String,
|
|
pub jwt_expiry_secs: u64,
|
|
pub cache_max_capacity: usize,
|
|
pub cache_idle_timeout_secs: u64,
|
|
pub cache_sweep_interval_secs: u64,
|
|
/// Comma-separated list of allowed CORS origins, or "*" for permissive.
|
|
/// If empty, no CORS headers are added.
|
|
pub cors_origins: Vec<String>,
|
|
}
|
|
|
|
impl Config {
|
|
pub fn from_env() -> Result<Self> {
|
|
Ok(Self {
|
|
database_url: std::env::var("DATABASE_URL")?,
|
|
jwt_secret: std::env::var("JWT_SECRET")?,
|
|
jwt_expiry_secs: std::env::var("JWT_EXPIRY_SECS")
|
|
.unwrap_or_else(|_| "3600".into())
|
|
.parse()?,
|
|
cache_max_capacity: std::env::var("CACHE_MAX_CAPACITY")
|
|
.unwrap_or_else(|_| "10000".into())
|
|
.parse()?,
|
|
cache_idle_timeout_secs: std::env::var("CACHE_IDLE_TIMEOUT_SECS")
|
|
.unwrap_or_else(|_| "300".into())
|
|
.parse()?,
|
|
cache_sweep_interval_secs: std::env::var("CACHE_SWEEP_INTERVAL_SECS")
|
|
.unwrap_or_else(|_| "60".into())
|
|
.parse()?,
|
|
cors_origins: std::env::var("CORS_ORIGINS")
|
|
.unwrap_or_default()
|
|
.split(',')
|
|
.map(|s| s.trim().to_string())
|
|
.filter(|s| !s.is_empty())
|
|
.collect(),
|
|
})
|
|
}
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 2: Update `.env.example`**
|
|
|
|
Add `CORS_ORIGINS=` (empty, no CORS by default) to `.env.example`.
|
|
|
|
- [ ] **Step 3: Update `src/main.rs` imports and app builder**
|
|
|
|
Add imports at the top of `src/main.rs`:
|
|
```rust
|
|
use axum::extract::DefaultBodyLimit;
|
|
use axum::http::{header, HeaderValue, Method};
|
|
use tower_http::cors::{Any, CorsLayer};
|
|
```
|
|
|
|
Replace `CorsLayer::permissive()` in the app builder with a configured layer. The CRUD routes also get a body size limit. Replace the `crud_routes` and `app` blocks:
|
|
|
|
```rust
|
|
let crud_routes = Router::new()
|
|
.route("/api/:table", get(handle_crud).post(handle_crud))
|
|
.route("/api/:table/", get(handle_crud).post(handle_crud))
|
|
.route("/api/:table/:id", get(handle_crud).put(handle_crud).delete(handle_crud))
|
|
.route("/api/:table/:id/", get(handle_crud).put(handle_crud).delete(handle_crud))
|
|
.layer(DefaultBodyLimit::max(1 * 1024 * 1024)) // 1 MB
|
|
.route_layer(middleware::from_fn_with_state(state.clone(), require_auth))
|
|
.route_layer(middleware::from_fn_with_state(state.clone(), blacklist_layer));
|
|
|
|
let cors_layer = build_cors(&config.cors_origins);
|
|
|
|
let app = Router::new()
|
|
.route("/auth/login", post(login))
|
|
.merge(crud_routes)
|
|
.nest("/api/admin", admin_router(state.clone()))
|
|
.nest_service("/", ServeDir::new("ui/dist").fallback(ServeFile::new("ui/dist/index.html")))
|
|
.layer(cors_layer)
|
|
.with_state(state);
|
|
```
|
|
|
|
Add the `build_cors` helper function (before `main`):
|
|
|
|
```rust
|
|
fn build_cors(origins: &[String]) -> CorsLayer {
|
|
if origins.is_empty() {
|
|
return CorsLayer::new();
|
|
}
|
|
if origins.iter().any(|o| o == "*") {
|
|
return CorsLayer::permissive();
|
|
}
|
|
let parsed: Vec<HeaderValue> = origins
|
|
.iter()
|
|
.filter_map(|o| o.parse().ok())
|
|
.collect();
|
|
CorsLayer::new()
|
|
.allow_origin(parsed)
|
|
.allow_methods([Method::GET, Method::POST, Method::PUT, Method::DELETE, Method::OPTIONS])
|
|
.allow_headers([header::CONTENT_TYPE, header::AUTHORIZATION])
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 4: Run tests**
|
|
|
|
```bash
|
|
cargo test
|
|
```
|
|
Expected: all tests pass.
|
|
|
|
- [ ] **Step 5: Commit**
|
|
|
|
```bash
|
|
git add src/config.rs src/main.rs .env.example
|
|
git commit -m "fix: replace permissive CORS with configured origins; add 1MB body size limit"
|
|
```
|
|
|
|
---
|
|
|
|
## Task 10: First-user interactive setup (replace hardcoded admin/admin)
|
|
|
|
**Files:**
|
|
- Modify: `src/main.rs`
|
|
|
|
**Behavior:** On startup, if no users exist in the database, the server prompts on stdin for a username and password, creates that user with full permissions (mask=63), then starts serving. Routes are locked down by auth from the first request — there is no magic credential.
|
|
|
|
- [ ] **Step 1: Add `use std::io::Write;` import to `src/main.rs`**
|
|
|
|
In `src/main.rs` imports, add:
|
|
```rust
|
|
use std::io::{self, Write};
|
|
```
|
|
|
|
- [ ] **Step 2: Replace the admin seed block**
|
|
|
|
Remove lines 42-56 in `src/main.rs` (the current admin seed):
|
|
```rust
|
|
// DELETE all of this:
|
|
let count: i64 = sqlx::query_scalar::<_, Option<i64>>("SELECT COUNT(*) FROM users WHERE username = 'admin'")
|
|
.fetch_one(&pool)
|
|
.await?
|
|
.unwrap_or(0);
|
|
if count == 0 {
|
|
let hash = bcrypt::hash("admin", bcrypt::DEFAULT_COST)?;
|
|
sqlx::query(
|
|
"INSERT INTO users (username, password_hash, permissions_mask) VALUES ('admin', $1, '63')"
|
|
)
|
|
.bind(hash)
|
|
.execute(&pool)
|
|
.await?;
|
|
tracing::info!("seeded admin user");
|
|
}
|
|
```
|
|
|
|
Replace with:
|
|
```rust
|
|
// If no users exist, prompt to create the first admin.
|
|
let user_count: i64 = sqlx::query_scalar::<_, Option<i64>>("SELECT COUNT(*) FROM users")
|
|
.fetch_one(&pool)
|
|
.await?
|
|
.unwrap_or(0);
|
|
if user_count == 0 {
|
|
println!("\nNo users found. Create the first admin account.");
|
|
print!("Username: ");
|
|
io::stdout().flush()?;
|
|
let mut username = String::new();
|
|
io::stdin().read_line(&mut username)?;
|
|
let username = username.trim().to_string();
|
|
if username.is_empty() {
|
|
anyhow::bail!("username cannot be empty");
|
|
}
|
|
|
|
print!("Password: ");
|
|
io::stdout().flush()?;
|
|
let mut password = String::new();
|
|
io::stdin().read_line(&mut password)?;
|
|
let password = password.trim().to_string();
|
|
if password.is_empty() {
|
|
anyhow::bail!("password cannot be empty");
|
|
}
|
|
|
|
let hash = bcrypt::hash(&password, bcrypt::DEFAULT_COST)?;
|
|
sqlx::query(
|
|
"INSERT INTO users (username, password_hash, permissions_mask) VALUES ($1, $2, '63')",
|
|
)
|
|
.bind(&username)
|
|
.bind(hash)
|
|
.execute(&pool)
|
|
.await?;
|
|
tracing::info!("created first admin user: {}", username);
|
|
println!("Admin user '{}' created. Starting server...\n", username);
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 3: Run tests**
|
|
|
|
```bash
|
|
cargo test
|
|
```
|
|
Expected: all tests pass (this is startup logic, not unit-testable without a DB).
|
|
|
|
- [ ] **Step 4: Commit**
|
|
|
|
```bash
|
|
git add src/main.rs
|
|
git commit -m "fix: replace hardcoded admin/admin seed with interactive first-user setup on empty DB"
|
|
```
|
|
|
|
---
|
|
|
|
## Task 11: Remove spurious `mut` on `body_cols_typed`
|
|
|
|
> **Note:** After Task 6, `body_cols_typed` no longer exists — this was eliminated when the body collection was simplified. Verify the `mut` warning is gone.
|
|
|
|
- [ ] **Step 1: Confirm no `mut` warning**
|
|
|
|
```bash
|
|
cargo build 2>&1 | grep "unused_mut\|body_cols_typed"
|
|
```
|
|
Expected: no output (variable was removed in Task 6).
|
|
|
|
If the warning still appears for another variable, find and remove the `mut`:
|
|
```bash
|
|
cargo build 2>&1 | grep "warning.*mut"
|
|
```
|
|
Fix any remaining spurious `mut` annotations.
|
|
|
|
- [ ] **Step 2: Commit if any change was needed**
|
|
|
|
```bash
|
|
git add src/routes/crud.rs
|
|
git commit -m "fix: remove spurious mut annotations"
|
|
```
|
|
|
|
---
|
|
|
|
## Self-Review
|
|
|
|
**Spec coverage check:**
|
|
- C1 (CRUD permissions) → Task 8 ✓
|
|
- C2 (admin/admin seed) → Task 10 ✓
|
|
- C3 (protected tables) → Task 5 ✓
|
|
- C4 (prefix substitution) → Task 2 ✓
|
|
- I1 (cache key collision) → Task 3 ✓
|
|
- I2 (double DB hit) → Task 7 ✓
|
|
- I3 (NULL_SENTINEL) → Task 6 ✓
|
|
- I4 (unix_now panic) → Task 1 ✓
|
|
- M1 (duplicate validator) → Task 4 ✓
|
|
- M3 (permissive CORS) → Task 9 ✓
|
|
- M5 (body size limit) → Task 9 ✓
|
|
- M6 (spurious mut) → Task 11 ✓
|
|
|
|
**Dependency order:**
|
|
- Task 4 (shared validator) must run before Task 5 (it uses `is_valid_identifier`)
|
|
- Task 6 (Value params) must run before Task 8 (handle_crud signature stabilizes)
|
|
- Task 7 (stash Claims) must run before Task 8 (handle_crud reads claims from extensions)
|
|
- All other tasks are independent
|
|
|
|
**Type consistency check:**
|
|
- `build_query` returns `Vec<Value>` after Task 6; all callers updated in the same task ✓
|
|
- `is_valid_identifier` returns `bool` (added Task 4); callers in Tasks 4 and 5 check `!is_valid_identifier(...)` ✓
|
|
- `authenticate()` returns `Option<Claims>` (Task 7); all require_* callers use `.ok_or(UNAUTHORIZED)?` ✓
|