diff --git a/.cargo/config.toml b/.cargo/config.toml index ce2708a..7a37a32 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -1,6 +1,7 @@ -[build] -rustc-wrapper = "/home/vheins/.cargo/bin/sccache" - +# sccache is opt-in: export `RUSTC_WRAPPER=sccache` (or set +# `build.rustc-wrapper` in your own `~/.cargo/config.toml`) to route rustc +# through it. A machine-specific wrapper path here would break every other +# checkout; CI sets `RUSTC_WRAPPER` only in the jobs that install sccache. [env] SCCACHE_CACHE_SIZE = "5G" CARGO_INCREMENTAL = "0" @@ -8,4 +9,4 @@ CARGO_INCREMENTAL = "0" # Convenience alias so the documented `cargo xtask ` surface works without # a separate `cargo-xtask` binary (ADOPT-030). Expands to `cargo run -p xtask --`. [alias] -xtask = "run -p xtask --" \ No newline at end of file +xtask = "run -p xtask --" diff --git a/.github/workflows/auto-assign-reviewer.yml b/.github/workflows/auto-assign-reviewer.yml index 8abc9c7..9dbc1b3 100644 --- a/.github/workflows/auto-assign-reviewer.yml +++ b/.github/workflows/auto-assign-reviewer.yml @@ -1,8 +1,14 @@ name: Auto Assign Reviewer # Requests a review from vheins on every PR that targets master. +# +# `pull_request_target` runs in the base repository's context, so the token can +# request reviewers on PRs opened from forks; a `pull_request` run from a fork +# only gets a read-only token and fails with "Resource not accessible by +# integration". This is safe here because the job never checks out or runs the +# PR's code: it only calls the REST API. on: - pull_request: + pull_request_target: types: [opened, ready_for_review, reopened, synchronize] branches: [master] diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e1c9291..1ca3011 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -22,11 +22,12 @@ concurrency: env: CARGO_TERM_COLOR: always - # Route rustc through sccache and store the cache in the GitHub Actions - # cache. Set workflow-wide: the compile-free `deny`/`audit` jobs never - # invoke rustc, so the wrapper is inert there (no sccache binary installed). - RUSTC_WRAPPER: sccache - SCCACHE_GHA_ENABLED: "true" + +# `RUSTC_WRAPPER: sccache` routes rustc through sccache and stores the cache in +# the GitHub Actions cache. It is set per job, only where +# `mozilla-actions/sccache-action` installs the binary: `cargo deny` runs +# `cargo metadata`, which invokes `rustc -vV`, so a wrapper without the binary +# fails the `deny` job. jobs: # fmt + clippy (-D warnings) + workspace DAG cycle check. @@ -34,6 +35,9 @@ jobs: name: Format, lint & cycles runs-on: ubuntu-latest timeout-minutes: 15 + env: + RUSTC_WRAPPER: sccache + SCCACHE_GHA_ENABLED: "true" steps: - uses: actions/checkout@v4 - name: Install stable toolchain (rustfmt + clippy) @@ -53,6 +57,9 @@ jobs: name: Test (workspace) runs-on: ubuntu-latest timeout-minutes: 30 + env: + RUSTC_WRAPPER: sccache + SCCACHE_GHA_ENABLED: "true" steps: - uses: actions/checkout@v4 - name: Install stable toolchain @@ -60,8 +67,16 @@ jobs: - name: Run sccache-cache uses: mozilla-actions/sccache-action@v0.0.11 - uses: Swatinem/rust-cache@v2 + # The scaffold compile gate (`crates/rustasea-scaffold/tests/generated_compiles.rs`) + # runs `cargo check --offline` on every generated starter kit, and the + # WASM variants need crates that the host build never downloads. Fetch + # every locked crate, for every target, before the tests run. + - name: cargo fetch + run: cargo fetch + # `--no-fail-fast` reports every failing test in one run instead of + # stopping at the first failing test binary. - name: cargo test - run: cargo test --workspace + run: cargo test --workspace --no-fail-fast # The `s3` and `sftp` disks sit behind `rustasea-storage`'s opt-in `aws` # and `sftp` features, so the default workspace run never compiles them. # Their Docker-backed suites (RustFS, SFTP) stay `#[ignore]`d. @@ -105,6 +120,9 @@ jobs: name: MSRV (1.88.0) runs-on: ubuntu-latest timeout-minutes: 15 + env: + RUSTC_WRAPPER: sccache + SCCACHE_GHA_ENABLED: "true" steps: - uses: actions/checkout@v4 - name: Install Rust 1.88.0 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9ee87dd..4e685c8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -134,14 +134,14 @@ cargo test -p rustasea-storage --features sftp --test sftp_container -- --ignore | Job | What it runs | |---|---| | `quality` | `cargo xtask ci` (fmt, clippy with `-D warnings` on the workspace and on `rustasea-storage --features aws,sftp`, `deps:check`, `lines:check`, cycle check) | - | `test` | `cargo test --workspace`, then `cargo test -p rustasea-storage --features aws,sftp` | + | `test` | `cargo fetch`, then `cargo test --workspace --no-fail-fast` and `cargo test -p rustasea-storage --features aws,sftp` | | `deny` | `cargo deny check` | | `audit` | `cargo audit` | | `msrv` | `cargo check --workspace` on Rust 1.88.0 | Formatting violations fail the build: run `cargo fmt --all` before pushing. - CI uses [sccache](https://github.com/mozilla/sccache) (`mozilla-actions/sccache-action`) to cache Rust compilation across the `quality`, `test`, and `msrv` jobs. + CI uses [sccache](https://github.com/mozilla/sccache) (`mozilla-actions/sccache-action`) to cache Rust compilation across the `quality`, `test`, and `msrv` jobs. Locally it is opt-in: install sccache and export `RUSTC_WRAPPER=sccache` (or set `build.rustc-wrapper` in your own `~/.cargo/config.toml`). 4. Address review feedback with new commits; avoid rewriting shared history. ## Coding standards diff --git a/README.md b/README.md index 61b836c..656d2d3 100644 --- a/README.md +++ b/README.md @@ -760,8 +760,8 @@ cargo xtask migrate # run migrations ``` CI runs the same gate — see [`.github/workflows/ci.yml`](.github/workflows/ci.yml): -`quality` (`cargo xtask ci`), `test` (`cargo test --workspace`, plus -`rustasea-storage` with its opt-in `aws`/`sftp` drivers), `deny` +`quality` (`cargo xtask ci`), `test` (`cargo fetch`, then `cargo test --workspace +--no-fail-fast`, plus `rustasea-storage` with its opt-in `aws`/`sftp` drivers), `deny` (`cargo deny check`), `audit` (`cargo audit`), and an `msrv` job that checks the workspace builds on the 1.88 floor (ADR-0001). **Formatting violations fail the build**: run `cargo fmt --all` before pushing, or `cargo xtask fmt` to check. diff --git a/crates/rustasea-app/src/bootstrap/tinker.rs b/crates/rustasea-app/src/bootstrap/tinker.rs index 314cb47..f01ddbb 100644 --- a/crates/rustasea-app/src/bootstrap/tinker.rs +++ b/crates/rustasea-app/src/bootstrap/tinker.rs @@ -121,8 +121,14 @@ mod tests { use rustasea::foundation::CONFIG_LOADER_KEY; /// Build a container with a config loader + environment binding. + /// + /// Tests run with the crate root as their working directory, so the + /// process-relative `config/app` the app loads at runtime is never found + /// here. The workspace `config/app.toml` is located from + /// `CARGO_MANIFEST_DIR` instead (the crate lives two levels below it). fn container() -> (Arc, Container) { - let loader = Arc::new(ConfigLoader::load_from(&["config/app"]).expect("load config")); + let app_config = concat!(env!("CARGO_MANIFEST_DIR"), "/../../config/app"); + let loader = Arc::new(ConfigLoader::load_from(&[app_config]).expect("load config")); let mut container = Container::new(); container.instance(CONFIG_LOADER_KEY, Arc::clone(&loader)); container.instance("app.environment", "local".to_string()); @@ -134,8 +140,8 @@ mod tests { fn config_renders_json() { let (loader, container) = container(); let source = AppTinkerSource::from_booted(loader, "local".to_string(), &container); - // `app_env` is always resolvable (defaulted) — it must render as a JSON - // string, never panic. + // `app_env` comes from `config/app.toml` (an `APP_ENV` set by a + // concurrent test only overrides it) and must render as a JSON string. let rendered = source.config("app_env").expect("app_env resolves"); assert!( rendered.starts_with('"'), diff --git a/crates/rustasea-app/src/main.rs b/crates/rustasea-app/src/main.rs index 1039cf5..cecee77 100644 --- a/crates/rustasea-app/src/main.rs +++ b/crates/rustasea-app/src/main.rs @@ -53,12 +53,29 @@ async fn main() -> anyhow::Result<()> { let listener = tokio::net::TcpListener::bind(addr).await?; println!("RustaSea dev server listening on http://{addr}"); - axum::serve(listener, router) - .with_graceful_shutdown(app.shutdown()) - .await?; + serve(listener, router, app.shutdown()).await?; Ok(()) } +/// Serve `router` on `listener` until `shutdown` resolves. +/// +/// Every request carries the connection's peer address as +/// `ConnectInfo`: the login throttle and the authentication log key +/// on the client IP, and without it every client falls back to `0.0.0.0` and +/// shares one throttle bucket. +pub(crate) async fn serve( + listener: tokio::net::TcpListener, + router: axum::Router, + shutdown: impl std::future::Future + Send + 'static, +) -> std::io::Result<()> { + axum::serve( + listener, + router.into_make_service_with_connect_info::(), + ) + .with_graceful_shutdown(shutdown) + .await +} + /// Resolve the bind address from `APP_URL` (host:port) or the default. fn bind_address() -> SocketAddr { std::env::var("APP_URL") diff --git a/crates/rustasea-app/src/routes/tests/auth_log.rs b/crates/rustasea-app/src/routes/tests/auth_log.rs index 3df0c89..f9bfbd3 100644 --- a/crates/rustasea-app/src/routes/tests/auth_log.rs +++ b/crates/rustasea-app/src/routes/tests/auth_log.rs @@ -21,9 +21,11 @@ //! [`AuthenticationLogLogger`]: rustasea_authlog::AuthenticationLogLogger //! [`PROVIDER_LOCK`]: super::settings_flows::PROVIDER_LOCK +use std::net::SocketAddr; use std::sync::Arc; use axum::body::Body; +use axum::extract::ConnectInfo; use axum::http::{header, HeaderValue, Request, StatusCode}; use axum::Router; use rustasea::auth::users::AuthUserRecord; @@ -131,6 +133,13 @@ fn login_post(email: &str, password: &str) -> Request { request } +/// Attach a client peer address, as the production server does for every request. +fn from_peer(mut request: Request, ip: &str) -> Request { + let addr = SocketAddr::new(ip.parse().expect("peer ip"), 54321); + request.extensions_mut().insert(ConnectInfo(addr)); + request +} + /// Extract the `rustasea-session` cookie value from a response's `Set-Cookie`. fn session_cookie(headers: &axum::http::HeaderMap) -> Option { let raw = headers.get(header::SET_COOKIE)?.to_str().ok()?; @@ -148,7 +157,7 @@ async fn successful_login_records_row() { let logger = Arc::new(AuthenticationLogLogger::new(pool.clone())); let _guard = LoggerGuard::install(logger.clone()); - let mut request = login_post(EMAIL, PASSWORD); + let mut request = from_peer(login_post(EMAIL, PASSWORD), "203.0.113.10"); request .headers_mut() .insert(header::USER_AGENT, HeaderValue::from_static("test-agent")); @@ -160,7 +169,7 @@ async fn successful_login_records_row() { let row = &rows[0]; assert_eq!(row.event, "login_succeeded"); assert_eq!(row.user_id.as_deref(), Some("user-1")); - assert_eq!(row.ip_address.as_deref(), Some("0.0.0.0")); + assert_eq!(row.ip_address.as_deref(), Some("203.0.113.10")); assert_eq!(row.user_agent.as_deref(), Some("test-agent")); assert!(row.successful); assert!(row.login_at.is_some()); @@ -261,3 +270,97 @@ async fn logout_fills_open_row() { ); pool.close().await; } + +/// Send a `POST /login` over a real TCP connection and return the status code. +/// +/// Drives the served socket rather than `Router::oneshot`, so the request goes +/// through the same connection handling as production. +async fn login_over_tcp(addr: SocketAddr, email: &str, password: &str) -> u16 { + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + + let body = format!("email={email}&password={password}"); + let request = format!( + "POST /login HTTP/1.1\r\nhost: {addr}\r\n\ + content-type: application/x-www-form-urlencoded\r\ncontent-length: {}\r\n\ + x-csrf-token: {}\r\nsec-fetch-site: same-origin\r\nconnection: close\r\n\r\n{body}", + body.len(), + crate::routes::helpers::csrf_token(), + ); + let mut stream = tokio::net::TcpStream::connect(addr).await.expect("connect"); + stream + .write_all(request.as_bytes()) + .await + .expect("write request"); + let mut response = Vec::new(); + stream + .read_to_end(&mut response) + .await + .expect("read response"); + String::from_utf8_lossy(&response) + .split_whitespace() + .nth(1) + .and_then(|code| code.parse().ok()) + .expect("status line") +} + +/// The production serve path attaches the connection's peer address, so a +/// login over real TCP is logged with the client IP. Without `ConnectInfo` +/// every request fell back to `0.0.0.0` and shared one throttle bucket. +#[tokio::test] +async fn served_app_logs_the_connection_peer() { + let _lock = super::settings_flows::PROVIDER_LOCK.lock().await; + install_provider(); + let pool = pool_with_table().await; + let logger = Arc::new(AuthenticationLogLogger::new(pool.clone())); + let _guard = LoggerGuard::install(logger.clone()); + + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind"); + let addr = listener.local_addr().expect("local addr"); + let (stop, stopped) = tokio::sync::oneshot::channel::<()>(); + let server = tokio::spawn(crate::serve(listener, app_with_guard(), async { + let _ = stopped.await; + })); + + let status = login_over_tcp(addr, EMAIL, PASSWORD).await; + let _ = stop.send(()); + server.await.expect("server task").expect("serve"); + + assert_eq!(status, StatusCode::SEE_OTHER.as_u16(), "login must succeed"); + let rows = logger.latest(10).await.expect("query"); + assert_eq!(rows.len(), 1, "exactly one row"); + assert_eq!(rows[0].ip_address.as_deref(), Some("127.0.0.1")); + pool.close().await; +} + +/// The `login` throttle keys on the client peer: five failed attempts lock that +/// client out, while a different client can still log in as the same user. +/// With every request on `0.0.0.0`, any client could lock any username out. +#[tokio::test] +async fn lockout_is_scoped_to_the_client_peer() { + let _lock = super::settings_flows::PROVIDER_LOCK.lock().await; + install_provider(); + + let attacker = "203.0.113.21"; + for _ in 0..5 { + let request = from_peer(login_post(EMAIL, "wrong-password"), attacker); + let (status, _, _) = call(app_with_guard(), request).await; + assert_eq!(status, StatusCode::UNPROCESSABLE_ENTITY); + } + let request = from_peer(login_post(EMAIL, "wrong-password"), attacker); + let (status, _, _) = call(app_with_guard(), request).await; + assert_eq!( + status, + StatusCode::TOO_MANY_REQUESTS, + "the attacking client is throttled" + ); + + let request = from_peer(login_post(EMAIL, PASSWORD), "203.0.113.22"); + let (status, _, _) = call(app_with_guard(), request).await; + assert_eq!( + status, + StatusCode::SEE_OTHER, + "another client still logs in as the same user" + ); +} diff --git a/crates/rustasea-app/src/routes/tests/two_factor.rs b/crates/rustasea-app/src/routes/tests/two_factor.rs index a75310c..df25a8a 100644 --- a/crates/rustasea-app/src/routes/tests/two_factor.rs +++ b/crates/rustasea-app/src/routes/tests/two_factor.rs @@ -10,11 +10,13 @@ //! [`MemoryUserProvider`]: rustasea::auth::MemoryUserProvider //! [`PROVIDER_LOCK`]: super::settings_flows::PROVIDER_LOCK +use std::net::SocketAddr; +use std::sync::atomic::{AtomicU8, Ordering}; use std::sync::Arc; use std::time::{SystemTime, UNIX_EPOCH}; use axum::body::Body; -use axum::extract::Extension; +use axum::extract::{ConnectInfo, Extension}; use axum::http::{header, HeaderMap, HeaderValue, Request, StatusCode}; use axum::Router; use rustasea::auth::users::{AuthUserRecord, MemoryUserRegistry}; @@ -227,11 +229,27 @@ async fn confirm(secret: &str) { assert_eq!(status, StatusCode::OK, "confirm: {body}"); } +/// Last octet of the peer address handed to the next login (see [`unique_peer`]). +static NEXT_PEER_OCTET: AtomicU8 = AtomicU8::new(1); + +/// A distinct loopback peer for one login request. +/// +/// The `login` limiter allows five attempts per minute per `username|ip`, and +/// its registry is process-wide. Without `ConnectInfo` every test login +/// resolves to the same `0.0.0.0` peer, so the lifecycle test's five logins as +/// `ada@example.com`, on top of other modules' logins as the same user, hit +/// `429`. A fresh peer per login keeps each request in its own bucket. +fn unique_peer() -> ConnectInfo { + let octet = NEXT_PEER_OCTET.fetch_add(1, Ordering::Relaxed); + ConnectInfo(SocketAddr::from(([127, 0, 2, octet], 54321))) +} + /// Start a challenge by logging in; returns the pending session id. async fn start_challenge(guard: &Arc) -> String { let body = format!("email={USER_A_EMAIL}&password={USER_A_PASSWORD}"); - let (status, headers, response) = - call(app_with_guard(guard.clone()), post("/login", &body)).await; + let mut request = post("/login", &body); + request.extensions_mut().insert(unique_peer()); + let (status, headers, response) = call(app_with_guard(guard.clone()), request).await; assert_eq!(status, StatusCode::FOUND, "login: {response}"); assert_eq!(location(&headers).as_deref(), Some("/two-factor-challenge")); cookie_value(&headers, SESSION_COOKIE_NAME).expect("pending session cookie") diff --git a/crates/rustasea-orm/src/profile.rs b/crates/rustasea-orm/src/profile.rs index a6309fa..d7ba1fa 100644 --- a/crates/rustasea-orm/src/profile.rs +++ b/crates/rustasea-orm/src/profile.rs @@ -116,6 +116,12 @@ pub async fn track( mod tests { use super::*; + /// Serializes the tests that install, clear, or rely on the process-wide + /// recorder slot: run in parallel, one test's `clear_query_recorder` or + /// `register_query_recorder` lands in the middle of another's `track` call. + /// A tokio mutex, so the async tests can hold it across `.await`. + static RECORDER_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); + /// Verifies the registry round-trips an installed recorder. #[test] fn recorder_registry_round_trips() { @@ -123,6 +129,7 @@ mod tests { impl QueryRecorder for Noop { fn record(&self, _event: SqlQueryEvent) {} } + let _serial = RECORDER_LOCK.blocking_lock(); clear_query_recorder(); assert!(query_recorder().is_none()); register_query_recorder(Arc::new(Noop)); @@ -146,9 +153,14 @@ mod tests { } } + // Unique SQL, so a query tracked by a concurrent DB test while this + // recorder is installed cannot be mistaken for this test's event. + const SQL: &str = "SELECT $1 -- profile::track_records_success_with_original_sql"; + + let _serial = RECORDER_LOCK.lock().await; let recorder = Arc::new(Capturing::default()); register_query_recorder(recorder.clone()); - let value = track("fetch_json", "SELECT $1", async { + let value = track("fetch_json", SQL, async { Ok::<_, crate::error::OrmError>(7) }) .await @@ -157,15 +169,16 @@ mod tests { assert_eq!(value, 7); let events = recorder.events.lock().unwrap(); - assert_eq!(events.len(), 1); - assert_eq!(events[0].kind, "fetch_json"); - assert_eq!(events[0].sql, "SELECT $1"); - assert!(events[0].error.is_none()); + let ours: Vec<&SqlQueryEvent> = events.iter().filter(|event| event.sql == SQL).collect(); + assert_eq!(ours.len(), 1); + assert_eq!(ours[0].kind, "fetch_json"); + assert!(ours[0].error.is_none()); } /// Verifies `track` runs untouched (and records nothing) with no recorder. #[tokio::test] async fn track_without_recorder_passes_through() { + let _serial = RECORDER_LOCK.lock().await; clear_query_recorder(); let value = track("execute_bind", "DELETE FROM t", async { Ok::<_, crate::error::OrmError>(3) diff --git a/crates/rustasea-queue-dashboard/src/lib.rs b/crates/rustasea-queue-dashboard/src/lib.rs index 05e56c8..ec773ed 100644 --- a/crates/rustasea-queue-dashboard/src/lib.rs +++ b/crates/rustasea-queue-dashboard/src/lib.rs @@ -387,9 +387,17 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + /// Serializes the tests that reset and read the process-wide heartbeat + /// registry: run in parallel, one test's `clear`/`stamp_at` lands in the + /// middle of the other's snapshot. + static HEARTBEATS_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + /// `workers` renders the heartbeat registry as ordered, active rows. #[test] fn workers_renders_heartbeats() { + let _serial = HEARTBEATS_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); heartbeat::clear(); let at = Utc .timestamp_opt(1_700_000_000, 0) @@ -406,6 +414,9 @@ mod tests { /// A stale heartbeat is reported but flagged `active == false`. #[test] fn workers_flags_stale_heartbeats_inactive() { + let _serial = HEARTBEATS_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); heartbeat::clear(); let now = Utc::now(); heartbeat::stamp_at("qd-fresh", now); diff --git a/crates/rustasea-scaffold/src/templates/core.rs b/crates/rustasea-scaffold/src/templates/core.rs index 4a2182c..be408b5 100644 --- a/crates/rustasea-scaffold/src/templates/core.rs +++ b/crates/rustasea-scaffold/src/templates/core.rs @@ -459,7 +459,8 @@ async fn main() -> Result<(), Box> { let listener = tokio::net::TcpListener::bind(addr).await?; println!("@@app_pascal@@ listening on http://{addr}"); - axum::serve(listener, router) + // `ConnectInfo` hands every request its client IP (throttle keys, audit logs). + axum::serve(listener, router.into_make_service_with_connect_info::()) .with_graceful_shutdown(app.shutdown()) .await?; Ok(()) diff --git a/crates/rustasea/tests/cli.rs b/crates/rustasea/tests/cli.rs index b1d8164..010ab4d 100644 --- a/crates/rustasea/tests/cli.rs +++ b/crates/rustasea/tests/cli.rs @@ -143,7 +143,7 @@ fn artisan_alias_generates_livewire_app() { let app = dir.join("demo-app"); assert!(app.join("resources/views/partials/counter.html").exists()); let manifest = std::fs::read_to_string(app.join("Cargo.toml")).expect("read manifest"); - assert!(manifest.contains("features = [\"view\", \"broadcast\"]")); + assert!(manifest.contains("features = [\"view\", \"action\"]")); std::fs::remove_dir_all(&dir).ok(); }