Address review comments: ref column, Db holds connection, required secret
- Rename the `ref_name` column to `ref` in the migration. - Db now holds an Arc<Mutex<Connection>> instead of a PathBuf; callers use db.lock() to get a MutexGuard rather than opening a fresh connection each time. - webhook_secret is now a required field in GlobalConfig (no Option). QuireCi::new() errors if config.fnl is absent. The "no secret" bypass path in the webhook handler and its test are removed. https://claude.ai/code/session_01YXVu67hVnQSAY8wzmfmRHw
diff --git a/quire-ci/migrations/0001_initial.sql b/quire-ci/migrations/0001_initial.sql
index 771144c..f72449c 100644
--- a/quire-ci/migrations/0001_initial.sql
+++ b/quire-ci/migrations/0001_initial.sql
@@ -1,7 +1,7 @@
CREATE TABLE runs (
id TEXT PRIMARY KEY,
repo TEXT NOT NULL,
- ref_name TEXT NOT NULL,
+ ref TEXT NOT NULL,
sha TEXT NOT NULL,
created_at INTEGER NOT NULL,
dispatched_at INTEGER,
diff --git a/quire-ci/src/db.rs b/quire-ci/src/db.rs
index 4bf8e9c..54fdab3 100644
--- a/quire-ci/src/db.rs
+++ b/quire-ci/src/db.rs
@@ -1,4 +1,5 @@
-use std::path::{Path, PathBuf};
+use std::path::Path;
+use std::sync::{Arc, Mutex, MutexGuard};
use rusqlite::Connection;
use rusqlite_migration::{M, Migrations};
@@ -15,33 +16,28 @@ pub enum Error {
Migration(#[from] rusqlite_migration::Error),
}
-/// An opened, migrated SQLite database. Cheap to clone — holds only the path.
-#[derive(Clone, Debug)]
+/// An opened, migrated SQLite database.
+#[derive(Clone)]
pub struct Db {
- path: PathBuf,
+ conn: Arc<Mutex<Connection>>,
}
impl Db {
- /// Open the database at `path`, run pending migrations, and return a `Db`.
- pub fn open(path: PathBuf) -> Result<Self, Error> {
- let mut conn = connect(&path)?;
+ pub fn open(path: &Path) -> Result<Self, Error> {
+ tracing::debug!(path = %path.display(), "opening database");
+ let mut conn = open_connection(path)?;
MIGRATIONS.to_latest(&mut conn)?;
- let db = Self { path };
- tracing::debug!(path = %db.path().display(), "database opened");
- Ok(db)
+ Ok(Self {
+ conn: Arc::new(Mutex::new(conn)),
+ })
}
- /// Open a new connection to the database.
- pub fn connect(&self) -> Result<Connection, rusqlite::Error> {
- connect(&self.path)
- }
-
- pub fn path(&self) -> &Path {
- &self.path
+ pub fn lock(&self) -> MutexGuard<'_, Connection> {
+ self.conn.lock().unwrap()
}
}
-fn connect(path: &Path) -> Result<Connection, rusqlite::Error> {
+fn open_connection(path: &Path) -> Result<Connection, rusqlite::Error> {
let conn = Connection::open(path)?;
conn.execute_batch(
"PRAGMA journal_mode = WAL;
@@ -52,11 +48,13 @@ fn connect(path: &Path) -> Result<Connection, rusqlite::Error> {
}
#[cfg(test)]
-pub fn open_in_memory() -> Result<Connection, Error> {
+pub fn open_in_memory() -> Result<Db, Error> {
let mut conn = Connection::open_in_memory()?;
conn.execute_batch("PRAGMA foreign_keys = ON;")?;
MIGRATIONS.to_latest(&mut conn)?;
- Ok(conn)
+ Ok(Db {
+ conn: Arc::new(Mutex::new(conn)),
+ })
}
#[cfg(test)]
@@ -70,7 +68,8 @@ mod tests {
#[test]
fn runs_table_has_expected_columns() {
- let conn = open_in_memory().expect("open_in_memory");
+ let db = open_in_memory().expect("open_in_memory");
+ let conn = db.lock();
let mut stmt = conn
.prepare("PRAGMA table_info(runs)")
.expect("prepare pragma");
@@ -83,7 +82,7 @@ mod tests {
for expected in &[
"id",
"repo",
- "ref_name",
+ "ref",
"sha",
"created_at",
"dispatched_at",
diff --git a/quire-ci/src/quire.rs b/quire-ci/src/quire.rs
index 7a6ff9f..360c0a1 100644
--- a/quire-ci/src/quire.rs
+++ b/quire-ci/src/quire.rs
@@ -1,8 +1,9 @@
use std::path::PathBuf;
-use miette::{IntoDiagnostic, Result};
+use miette::{IntoDiagnostic, Result, bail};
use quire_core::fennel::Fennel;
+use quire_core::secret::SecretString;
use crate::db::Db;
@@ -15,8 +16,7 @@ pub struct GlobalConfig {
/// TCP port the HTTP server binds to on all interfaces (`0.0.0.0`).
#[serde(default = "default_port")]
pub port: u16,
- #[serde(default)]
- pub webhook_secret: Option<quire_core::secret::SecretString>,
+ pub webhook_secret: SecretString,
}
fn default_port() -> u16 {
@@ -26,9 +26,6 @@ fn default_port() -> u16 {
pub use quire_core::telemetry::SentryConfig;
/// Application runtime context.
-///
-/// Loads config at construction time so callers don't have to thread
-/// Results around.
#[derive(Clone)]
pub struct QuireCi {
config: GlobalConfig,
@@ -38,13 +35,12 @@ pub struct QuireCi {
impl QuireCi {
pub fn new(base_dir: PathBuf) -> Result<Self> {
let config_path = base_dir.join("config.fnl");
- let config = if config_path.exists() {
- let fennel = Fennel::new().into_diagnostic()?;
- fennel.load_file(&config_path).into_diagnostic()?
- } else {
- GlobalConfig::default()
- };
- let db = Db::open(base_dir.join("quire-ci.db")).into_diagnostic()?;
+ if !config_path.exists() {
+ bail!("config file not found: {}", config_path.display());
+ }
+ let fennel = Fennel::new().into_diagnostic()?;
+ let config: GlobalConfig = fennel.load_file(&config_path).into_diagnostic()?;
+ let db = Db::open(&base_dir.join("quire-ci.db")).into_diagnostic()?;
Ok(Self { config, db })
}
@@ -57,25 +53,22 @@ impl QuireCi {
}
}
-impl Default for GlobalConfig {
- fn default() -> Self {
- Self {
- sentry: None,
- port: default_port(),
- webhook_secret: None,
- }
- }
-}
-
#[cfg(test)]
mod tests {
use super::*;
+ fn write_config(dir: &tempfile::TempDir, content: &str) {
+ fs_err::write(dir.path().join("config.fnl"), content).expect("write config");
+ }
+
+ fn minimal_config(secret: &str) -> String {
+ format!(r#"{{:webhook-secret "{secret}"}}"#)
+ }
+
#[test]
fn global_config_defaults() {
let dir = tempfile::tempdir().expect("tempdir");
- let config_path = dir.path().join("config.fnl");
- fs_err::write(&config_path, "{}").expect("write");
+ write_config(&dir, &minimal_config("s3cret"));
let q = QuireCi::new(dir.path().to_path_buf()).expect("should load");
assert_eq!(q.config().port, 3000);
@@ -84,31 +77,26 @@ mod tests {
#[test]
fn global_config_parses_custom_port() {
let dir = tempfile::tempdir().expect("tempdir");
- let config_path = dir.path().join("config.fnl");
- fs_err::write(&config_path, r#"{:port 4000}"#).expect("write");
+ write_config(&dir, r#"{:webhook-secret "s3cret" :port 4000}"#);
let q = QuireCi::new(dir.path().to_path_buf()).expect("should load");
assert_eq!(q.config().port, 4000);
}
#[test]
- fn global_config_missing_file_uses_defaults() {
+ fn global_config_missing_file_errors() {
let dir = tempfile::tempdir().expect("tempdir");
-
- let q = QuireCi::new(dir.path().to_path_buf()).expect("should load");
- assert_eq!(q.config().port, 3000);
- assert!(q.config().sentry.is_none());
+ let result = QuireCi::new(dir.path().to_path_buf());
+ assert!(result.is_err(), "should error when config file is missing");
}
#[test]
fn global_config_loads_sentry() {
let dir = tempfile::tempdir().expect("tempdir");
- let config_path = dir.path().join("config.fnl");
- fs_err::write(
- &config_path,
- r#"{:sentry {:dsn "https://key@sentry.io/123"}}"#,
- )
- .expect("write");
+ write_config(
+ &dir,
+ r#"{:webhook-secret "s3cret" :sentry {:dsn "https://key@sentry.io/123"}}"#,
+ );
let q = QuireCi::new(dir.path().to_path_buf()).expect("should load");
let sentry = q
@@ -122,8 +110,11 @@ mod tests {
#[test]
fn db_is_created_at_expected_path() {
let dir = tempfile::tempdir().expect("tempdir");
- let q = QuireCi::new(dir.path().to_path_buf()).expect("should load");
- assert_eq!(q.db().path(), dir.path().join("quire-ci.db"));
- assert!(q.db().path().exists(), "db file should exist after new()");
+ write_config(&dir, &minimal_config("s3cret"));
+ QuireCi::new(dir.path().to_path_buf()).expect("should load");
+ assert!(
+ dir.path().join("quire-ci.db").exists(),
+ "db file should exist after new()"
+ );
}
}
diff --git a/quire-ci/src/server.rs b/quire-ci/src/server.rs
index 1d3cc1b..bc046b9 100644
--- a/quire-ci/src/server.rs
+++ b/quire-ci/src/server.rs
@@ -44,31 +44,31 @@ async fn webhook(
headers: HeaderMap,
body: axum::body::Bytes,
) -> std::result::Result<StatusCode, WebhookError> {
- if let Some(secret) = quire.config().webhook_secret.as_ref() {
- let secret_bytes = secret
- .reveal()
- .map_err(|e| {
- tracing::error!(error = %e, "failed to resolve webhook secret");
- WebhookError::Internal
- })?
- .as_bytes()
- .to_vec();
-
- let auth_header = headers
- .get("Authorization")
- .and_then(|v| v.to_str().ok())
- .and_then(|s| s.strip_prefix("HMAC-SHA256 "))
- .ok_or(WebhookError::Unauthorized)?
- .to_string();
-
- let provided_bytes = hex::decode(&auth_header).map_err(|_| WebhookError::Unauthorized)?;
+ let secret_bytes = quire
+ .config()
+ .webhook_secret
+ .reveal()
+ .map_err(|e| {
+ tracing::error!(error = %e, "failed to resolve webhook secret");
+ WebhookError::Internal
+ })?
+ .as_bytes()
+ .to_vec();
- let mut mac =
- Hmac::<Sha256>::new_from_slice(&secret_bytes).expect("HMAC accepts any key length");
- mac.update(&body);
- mac.verify_slice(&provided_bytes)
- .map_err(|_| WebhookError::Unauthorized)?;
- }
+ let auth_header = headers
+ .get("Authorization")
+ .and_then(|v| v.to_str().ok())
+ .and_then(|s| s.strip_prefix("HMAC-SHA256 "))
+ .ok_or(WebhookError::Unauthorized)?
+ .to_string();
+
+ let provided_bytes = hex::decode(&auth_header).map_err(|_| WebhookError::Unauthorized)?;
+
+ let mut mac =
+ Hmac::<Sha256>::new_from_slice(&secret_bytes).expect("HMAC accepts any key length");
+ mac.update(&body);
+ mac.verify_slice(&provided_bytes)
+ .map_err(|_| WebhookError::Unauthorized)?;
let event: PushEvent = serde_json::from_slice(&body).map_err(|_| WebhookError::BadRequest)?;
@@ -77,18 +77,14 @@ async fn webhook(
.and_then(|v| v.to_str().ok())
.map(|s| s.to_string());
- let conn = quire.db().connect().map_err(|e| {
- tracing::error!(error = %e, "failed to open database connection");
- WebhookError::Internal
- })?;
-
+ let conn = quire.db().lock();
let now_ms = jiff::Timestamp::now().as_millisecond();
for push_ref in event.updated_refs() {
let id = uuid::Uuid::now_v7().to_string();
conn.execute(
- "INSERT INTO runs (id, repo, ref_name, sha, created_at, traceparent)
- VALUES (?1, ?2, ?3, ?4, ?5, ?6)",
+ r#"INSERT INTO runs (id, repo, "ref", sha, created_at, traceparent)
+ VALUES (?1, ?2, ?3, ?4, ?5, ?6)"#,
rusqlite::params![
id,
event.repo,
@@ -164,12 +160,6 @@ mod tests {
.with_state(quire)
}
- fn quire_without_secret() -> (tempfile::TempDir, QuireCi) {
- let dir = tempfile::tempdir().expect("tempdir");
- let quire = QuireCi::new(dir.path().to_path_buf()).expect("QuireCi::new");
- (dir, quire)
- }
-
fn quire_with_secret(secret: &str) -> (tempfile::TempDir, QuireCi) {
let dir = tempfile::tempdir().expect("tempdir");
let config_path = dir.path().join("config.fnl");
@@ -224,33 +214,8 @@ mod tests {
let resp = app.oneshot(req).await.unwrap();
assert_eq!(resp.status(), StatusCode::NO_CONTENT);
- let conn = db.connect().expect("connect");
- let count: i64 = conn
- .query_row("SELECT COUNT(*) FROM runs", [], |row| row.get(0))
- .expect("count");
- assert_eq!(count, 1);
- }
-
- #[tokio::test]
- async fn no_secret_configured_allows_unsigned_post() {
- let (_dir, quire) = quire_without_secret();
- let db = quire.db().clone();
- let app = make_app(quire);
-
- let body = push_event_body();
-
- let req = Request::builder()
- .method("POST")
- .uri("/webhook")
- .header("content-type", "application/json")
- .body(Body::from(body))
- .unwrap();
-
- let resp = app.oneshot(req).await.unwrap();
- assert_eq!(resp.status(), StatusCode::NO_CONTENT);
-
- let conn = db.connect().expect("connect");
- let count: i64 = conn
+ let count: i64 = db
+ .lock()
.query_row("SELECT COUNT(*) FROM runs", [], |row| row.get(0))
.expect("count");
assert_eq!(count, 1);