A file can come back out, and a script cannot come back forever

Two halves of the same evening. Uploads were one-way. A `type: file` field put an object in the bucket and wrote its key into the record, and nothing ever read it back - a desk showed `0aba5734-...-drawing.pdf` as a string with no link, so a builder could receive drawings and never open one. `GET /attachment/{bucket}/{record}/{field}` streams it. The key is not the credential and is not taken from the caller: it is read out of the record named, so a file is reachable only through a record somebody is allowed to see, and a key that leaks off a desk is a link to nothing. And there was no rate limiting anywhere. A burst of 30 per caller IP, refilled one every two seconds, outside the session layer so anyone over it is turned away before a handler is entered. Well above a person filling in a form - a page load fetches several resources at once - and well below being a nuisance. PORTAL_RATE_BURST and PORTAL_RATE_SECONDS retune it; either 0 turns it off for an instance behind something that already does this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-28 16:28:43 +02:00
parent 8d6f29011c
commit d5de25e8c6
6 changed files with 415 additions and 3 deletions
@@ -6,6 +6,8 @@ content repos pin the tag they run (see the end of this file).

 ## Unreleased

+- A file can come back out. `GET /attachment/{bucket}/{record}/{field}` streams what a `type: file` field stored, reading the object key out of the record rather than taking one from the caller - so a file is only reachable through a record somebody may see, and a key that leaks off a desk is a link to nothing. Before this, uploads were one-way: a desk showed the key as a string and there was no way to open it.
+- Rate limiting, per caller IP, outside everything else: a burst of 30 refilled one every two seconds (`PORTAL_RATE_BURST`, `PORTAL_RATE_SECONDS`; either 0 turns it off). A form-filling visitor never reaches it; a script does. There was none at all before.
 - `/upload` asks the policy, like every other way in. It asked nothing: any caller who could reach the host could put 20 MB in the bucket, as often as they liked, and a site whose forms are public is a site whose upload route is public. It now takes the `question` and `alternative` the file is being attached to, refuses a page that declares no `type: file` field or an alternative that is closed, and asks `Submit` on that page - the same question the submission itself will ask - before a byte is written. Latent until an instance had storage configured; the first one now does.
 - A submitter stays in dialogue with their case for as long as it runs. `self_transition` takes `from: [state, ...]`, and the access policy compiles one rule per state named; it defaulted to the initial state alone and was written that way in the compiler, so an applicant written to for a missing drawing could not send it, and nobody could withdraw once anything had happened. Omitted, it still means the initial state alone.
 - A record can belong to another record. `type: record` with `of: <bucket>` stores the parent's id, which arrives on the link rather than being typed; a submission naming a record that is not there is refused. The other half is `where: { field, equals }` on a Kv resource, so an application's page can list the objections that name it.
@@ -794,7 +794,7 @@ dependencies = [
 "linked-hash-map",
 "linked_hash_set",
 "miette",
- "nonempty",
+ "nonempty 0.12.0",
 "ref-cast",
 "regex",
 "rustc-literal-escaper",
@@ -1060,6 +1060,12 @@ dependencies = [
 "cfg-if",
 ]

+[[package]]
+name = "crossbeam-utils"
+version = "0.8.23"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "a31eee39dddec8330830986fcd7625edb5a24ec90ea038215273bbc3adb08ac6"
+
 [[package]]
 name = "crypto-bigint"
 version = "0.5.5"
@@ -1161,6 +1167,20 @@ dependencies = [
 "syn 2.0.119",
 ]

+[[package]]
+name = "dashmap"
+version = "6.2.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "e6361d5c062261c78a176addb82d4c821ae42bed6089de0e12603cd25de2059c"
+dependencies = [
+ "cfg-if",
+ "crossbeam-utils",
+ "hashbrown 0.14.5",
+ "lock_api",
+ "once_cell",
+ "parking_lot_core",
+]
+
 [[package]]
 name = "data-encoding"
 version = "2.11.0"
@@ -1517,6 +1537,16 @@ dependencies = [
 "percent-encoding",
 ]

+[[package]]
+name = "forwarded-header-value"
+version = "0.1.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "8835f84f38484cc86f110a805655697908257fb9a7af005234060891557198e9"
+dependencies = [
+ "nonempty 0.7.0",
+ "thiserror 1.0.69",
+]
+
 [[package]]
 name = "fs_extra"
 version = "1.3.0"
@@ -1594,6 +1624,12 @@ version = "0.3.33"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "b231ed28831efb4a61a08580c4bc233ec56bc009f4cd8f52da2c3cb97df0c109"

+[[package]]
+name = "futures-timer"
+version = "3.0.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "af43fadb8a98512d547e37b4e92e0ced13e205c061b87b4623eff01d918d6968"
+
 [[package]]
 name = "futures-util"
 version = "0.3.33"
@@ -1642,9 +1678,11 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "899def5c37c4fd7b2664648c28120ecec138e4d395b459e5ca34f9cce2dd77fd"
 dependencies = [
 "cfg-if",
+ "js-sys",
 "libc",
 "r-efi 5.3.0",
 "wasip2",
+ "wasm-bindgen",
 ]

 [[package]]
@@ -1695,6 +1733,29 @@ dependencies = [
 "web-sys",
 ]

+[[package]]
+name = "governor"
+version = "0.10.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "9efcab3c1958580ff1f25a2a41be1668f7603d849bb63af523b208a3cc1223b8"
+dependencies = [
+ "cfg-if",
+ "dashmap",
+ "futures-sink",
+ "futures-timer",
+ "futures-util",
+ "getrandom 0.3.4",
+ "hashbrown 0.16.1",
+ "nonzero_ext",
+ "parking_lot",
+ "portable-atomic",
+ "quanta",
+ "rand 0.9.5",
+ "smallvec",
+ "spinning_top",
+ "web-time",
+]
+
 [[package]]
 name = "group"
 version = "0.13.0"
@@ -1756,6 +1817,12 @@ version = "0.12.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "8a9ee70c43aaf417c914396645a0fa852624801b24ebb7ae78fe8272889ac888"

+[[package]]
+name = "hashbrown"
+version = "0.14.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "e5274423e17b7c9fc20b6e7e208532f9b19825d82dfd615708b70edd83df41f1"
+
 [[package]]
 name = "hashbrown"
 version = "0.16.1"
@@ -1993,6 +2060,19 @@ dependencies = [
 "webpki-roots",
 ]

+[[package]]
+name = "hyper-timeout"
+version = "0.5.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "2b90d566bffbce6a75bd8b09a05aa8c2cb1fabb6cb348f8840c9e4c90a0d83b0"
+dependencies = [
+ "hyper 1.11.0",
+ "hyper-util",
+ "pin-project-lite",
+ "tokio",
+ "tower-service",
+]
+
 [[package]]
 name = "hyper-util"
 version = "0.1.20"
@@ -2887,6 +2967,12 @@ dependencies = [
 "signatory",
 ]

+[[package]]
+name = "nonempty"
+version = "0.7.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "e9e591e719385e6ebaeb5ce5d3887f7d5676fceca6411d1925ccc95745f3d6f7"
+
 [[package]]
 name = "nonempty"
 version = "0.12.0"
@@ -2896,6 +2982,12 @@ dependencies = [
 "serde",
 ]

+[[package]]
+name = "nonzero_ext"
+version = "0.3.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "38bf9645c8b145698bb0b18a4637dcacbc421ea49bef2317e4fd8065a387cf21"
+
 [[package]]
 name = "nu-ansi-term"
 version = "0.50.3"
@@ -3297,6 +3389,7 @@ dependencies = [
 "tower",
 "tower-http",
 "tower-sessions",
+ "tower_governor",
 "tracing",
 "tracing-subscriber",
 "url",
@@ -3468,6 +3561,21 @@ version = "0.11.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "007d8adb5ddab6f8e3f491ac63566a7d5002cc7ed73901f72057943fa71ae1ae"

+[[package]]
+name = "quanta"
+version = "0.12.6"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "f3ab5a9d756f0d97bdc89019bd2e4ea098cf9cde50ee7564dde6b81ccc8f06c7"
+dependencies = [
+ "crossbeam-utils",
+ "libc",
+ "once_cell",
+ "raw-cpuid",
+ "wasi",
+ "web-sys",
+ "winapi",
+]
+
 [[package]]
 name = "quick-error"
 version = "1.2.3"
@@ -3667,6 +3775,15 @@ dependencies = [
 "rand_core 0.9.5",
 ]

+[[package]]
+name = "raw-cpuid"
+version = "11.6.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "498cd0dc59d73224351ee52a95fee0f1a617a2eae0e7d9d720cc622c73a54186"
+dependencies = [
+ "bitflags 2.13.1",
+]
+
 [[package]]
 name = "reactive_graph"
 version = "0.2.14"
@@ -4585,6 +4702,15 @@ version = "0.10.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "023a211cb3138dbc438680b32560ad89f699977624c9f8dbb95a47d5b4c07dd3"

+[[package]]
+name = "spinning_top"
+version = "0.3.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "d96d2d1d716fb500937168cc09353ffdc7a012be8475ac7308e1bdf0e3923300"
+dependencies = [
+ "lock_api",
+]
+
 [[package]]
 name = "spki"
 version = "0.7.3"
@@ -4906,6 +5032,17 @@ dependencies = [
 "tokio",
 ]

+[[package]]
+name = "tokio-stream"
+version = "0.1.19"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "a3d06f0b082ba57c26b79407372e57cf2a1e28124f78e9479fe80322cf53420b"
+dependencies = [
+ "futures-core",
+ "pin-project-lite",
+ "tokio",
+]
+
 [[package]]
 name = "tokio-tungstenite"
 version = "0.29.0"
@@ -4984,6 +5121,35 @@ dependencies = [
 "winnow",
 ]

+[[package]]
+name = "tonic"
+version = "0.14.6"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "ac2a5518c70fa84342385732db33fb3f44bc4cc748936eb5833d2df34d6445ef"
+dependencies = [
+ "async-trait",
+ "axum",
+ "base64 0.22.1",
+ "bytes",
+ "h2 0.4.15",
+ "http 1.4.2",
+ "http-body 1.1.0",
+ "http-body-util",
+ "hyper 1.11.0",
+ "hyper-timeout",
+ "hyper-util",
+ "percent-encoding",
+ "pin-project",
+ "socket2 0.6.5",
+ "sync_wrapper",
+ "tokio",
+ "tokio-stream",
+ "tower",
+ "tower-layer",
+ "tower-service",
+ "tracing",
+]
+
 [[package]]
 name = "tower"
 version = "0.5.3"
@@ -4992,9 +5158,12 @@ checksum = "ebe5ef63511595f1344e2d5cfa636d973292adc0eec1f0ad45fae9f0851ab1d4"
 dependencies = [
 "futures-core",
 "futures-util",
+ "indexmap 2.14.0",
 "pin-project-lite",
+ "slab",
 "sync_wrapper",
 "tokio",
+ "tokio-util",
 "tower-layer",
 "tower-service",
 "tracing",
@@ -5106,6 +5275,23 @@ dependencies = [
 "tower-sessions-core",
 ]

+[[package]]
+name = "tower_governor"
+version = "0.8.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "44de9b94d849d3c46e06a883d72d408c2de6403367b39df2b1c9d9e7b6736fe6"
+dependencies = [
+ "axum",
+ "forwarded-header-value",
+ "governor",
+ "http 1.4.2",
+ "pin-project",
+ "thiserror 2.0.19",
+ "tonic",
+ "tower",
+ "tracing",
+]
+
 [[package]]
 name = "tracing"
 version = "0.1.44"
@@ -5528,6 +5714,22 @@ dependencies = [
 "rustls-pki-types",
 ]

+[[package]]
+name = "winapi"
+version = "0.3.9"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "5c839a674fcd7a98952e593242ea400abe93992746761e38641405d28b00f419"
+dependencies = [
+ "winapi-i686-pc-windows-gnu",
+ "winapi-x86_64-pc-windows-gnu",
+]
+
+[[package]]
+name = "winapi-i686-pc-windows-gnu"
+version = "0.4.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "ac3b87c63620426dd9b991e5ce0329eff545bccbbb34f3be09ff6fb6ab51b7b6"
+
 [[package]]
 name = "winapi-util"
 version = "0.1.11"
@@ -5537,6 +5739,12 @@ dependencies = [
 "windows-sys 0.61.2",
 ]

+[[package]]
+name = "winapi-x86_64-pc-windows-gnu"
+version = "0.4.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f"
+
 [[package]]
 name = "windows-core"
 version = "0.62.2"
@@ -63,6 +63,7 @@ web-sys = { version = "0.3", features = [
    "Response",
    "Window",
 ], optional = true }
+tower_governor = { version = "0.8.0", optional = true }

 [features]
 default = []
@@ -81,6 +82,7 @@ ssr = [
    "dep:leptos_axum",
    "dep:axum",
    "dep:aws-sdk-s3",
+    "dep:tower_governor",
    "dep:tokio",
    "dep:tower",
    "dep:tower-http",
@@ -75,6 +75,52 @@ values.
 | `AUTOMATION_READ_TOKEN` | the automation KV read endpoint |
 | `GARAGE_*` | uploads |
 | `MAPBOX_TOKEN` | map tiles, proxied; unset, maps are off |
+| `GARAGE_*` (4) | attachments; see below. Unset, upload fields fail closed |
+| `PORTAL_RATE_BURST`, `PORTAL_RATE_SECONDS` | the rate limit; see below |

 Mail is delivered by [gdo](https://project.uhhm.no/uhhm/gdo), a
 separate unit on the same host reading the same NATS.
+
+## Rate limiting
+
+Every request passes a per-caller-IP limit before a session is loaded
+or a handler is entered: a burst of 30, refilled one every two
+seconds. A visitor filling in a form never reaches it (a page load
+fetches several resources at once); a script does.
+
+| | |
+|---|---|
+| `PORTAL_RATE_BURST` | how many requests at once, default 30 |
+| `PORTAL_RATE_SECONDS` | seconds per refilled request, default 2 |
+
+Either set to `0` turns it off, for an instance behind something that
+already does this. The caller is read from `X-Forwarded-For` or
+`X-Real-IP`, falling back to the socket — all of these instances are
+behind Caddy.
+
+This is a fence, not an accounting system. What keeps uploads honest
+is the policy check on the route itself.
+
+## Attachments
+
+A `type: file` field stores an object key in the record. The file
+comes back out at:
+
+    GET /attachment/{bucket}/{record}/{field}
+
+which reads the key out of that record rather than taking one from the
+caller, so a file can only be fetched through a record somebody is
+allowed to see, and a leaked key is a link to nothing. Reading it
+needs the same policy permission as reading the bucket on a desk.
+
+Uploads need a bucket and a key in the Garage on the host:
+
+```sh
+garage bucket create <site>-attachments
+garage key create <site>-portal-uploads
+garage bucket allow --read --write <site>-attachments --key <site>-portal-uploads
+```
+
+then `GARAGE_S3_ENDPOINT`, `GARAGE_ACCESS_KEY`, `GARAGE_SECRET_KEY` and
+`GARAGE_UPLOADS_BUCKET` in that instance's env. Unset, an upload field
+fails closed rather than appearing to work and storing nowhere.
@@ -132,6 +132,42 @@ async fn main() -> anyhow::Result<()> {
        .with_same_site(tower_sessions::cookie::SameSite::Lax)
        .with_name("portal_session");

+    // What a person does costs a little; what a script does costs a
+    // lot. A burst of 30 with one refilled every two seconds is well
+    // above anything a visitor filling in a form will reach - a page
+    // load fetches several resources at once - and well below what it
+    // takes to be a nuisance. Per caller IP, in memory, no store: this
+    // is a fence, not an accounting system, and the fence that matters
+    // for uploads is the policy check on the route itself.
+    //
+    // PORTAL_RATE_PER_SECOND / PORTAL_RATE_BURST retune it per site;
+    // 0 for either turns it off, for an instance behind something that
+    // already does this.
+    let rate_seconds: u64 = std::env::var("PORTAL_RATE_SECONDS")
+        .ok()
+        .and_then(|v| v.parse().ok())
+        .unwrap_or(2);
+    let rate_burst: u32 = std::env::var("PORTAL_RATE_BURST")
+        .ok()
+        .and_then(|v| v.parse().ok())
+        .unwrap_or(30);
+    let governor = (rate_seconds > 0 && rate_burst > 0).then(|| {
+        // SmartIpKeyExtractor reads X-Forwarded-For/X-Real-IP, which is
+        // what every one of these instances is behind; it falls back to
+        // the socket address when neither is set.
+        let conf = tower_governor::governor::GovernorConfigBuilder::default()
+            .per_second(rate_seconds)
+            .burst_size(rate_burst)
+            .key_extractor(tower_governor::key_extractor::SmartIpKeyExtractor)
+            .finish()
+            .expect("a rate limit of at least one per second and a burst of at least one");
+        std::sync::Arc::new(conf)
+    });
+    match &governor {
+        Some(_) => tracing::info!(rate_seconds, rate_burst, "rate limiting per caller IP"),
+        None => tracing::warn!("rate limiting is off (PORTAL_RATE_SECONDS or PORTAL_RATE_BURST is 0)"),
+    }
+
    async fn server_fn_handler(
        State(state): State<AppState>,
        request: Request<Body>,
@@ -175,6 +211,10 @@ async fn main() -> anyhow::Result<()> {
            "/upload",
            post(upload::upload).layer(axum::extract::DefaultBodyLimit::max(upload::MAX_UPLOAD_BYTES + 64 * 1024)),
        )
+        .route(
+            "/attachment/{bucket}/{record}/{field}",
+            get(upload::attachment),
+        )
        .route("/gitea-repo", get(content::gitea_repo_handler))
        // Content-shipped assets (hero module etc.), same-origin.
        .route("/site/{*path}", get(content::site_asset_handler))
@@ -224,7 +264,13 @@ async fn main() -> anyhow::Result<()> {
            },
        )
        .fallback(leptos_axum::file_and_error_handler::<AppState, _>(shell))
-        .layer(session_layer)
+        .layer(session_layer);
+    // Outermost, so a caller who is over the limit is turned away
+    // before a session is loaded or a handler is entered.
+    let app = match governor {
+        Some(conf) => app.layer(tower_governor::GovernorLayer::new(conf)),
+        None => app,
+    }
    .with_state(state);

    tracing::info!("listening on http://{addr}");
@@ -250,3 +250,111 @@ alternatives:
        assert_eq!(page_taking_files(&qs, "/", "Send drawings"), None);
    }
 }
+
+/// GET /attachment/{bucket}/{record}/{field} - the other half of an
+/// upload. A `type: file` field stores an object key, which on its own
+/// is a string a desk can read and nobody can open; this streams the
+/// object back.
+///
+/// The key is never the credential. It is not even taken from the
+/// caller: the caller names a record, and the key is read out of that
+/// record's own field, so a file can only be fetched through a record
+/// somebody is allowed to see. Guessing keys gets you nowhere, and a
+/// key that leaks out of a desk is not a link to anything.
+pub async fn attachment(
+    State(state): State<AppState>,
+    session: tower_sessions::Session,
+    axum::extract::Path((bucket, record, field)): axum::extract::Path<(String, String, String)>,
+) -> Result<axum::response::Response, (StatusCode, String)> {
+    use axum::response::IntoResponse;
+
+    let garage = state.garage.as_ref().ok_or((
+        StatusCode::SERVICE_UNAVAILABLE,
+        "uploads are not configured".to_string(),
+    ))?;
+
+    // Refusals read the same as a miss all the way down, so this
+    // cannot be used to learn which buckets, records or fields exist.
+    let missing = || (StatusCode::NOT_FOUND, "not found".to_string());
+
+    if !may_read_bucket(&state, &session, &bucket).await {
+        return Err(missing());
+    }
+
+    let store = state.jetstream.get_key_value(&bucket).await.map_err(|_| missing())?;
+    let bytes = store.get(&record).await.map_err(|_| missing())?.ok_or_else(missing)?;
+    let answer: crate::answers::Answer = serde_json::from_slice(&bytes).map_err(|_| missing())?;
+    let key = answer
+        .responses
+        .get(&field)
+        .and_then(|v| v.as_str())
+        .filter(|k| !k.is_empty())
+        .ok_or_else(missing)?;
+
+    let object = garage
+        .client
+        .get_object()
+        .bucket(&garage.bucket)
+        .key(key)
+        .send()
+        .await
+        .map_err(|_| missing())?;
+
+    let content_type = object
+        .content_type()
+        .unwrap_or("application/octet-stream")
+        .to_string();
+    let body = object
+        .body
+        .collect()
+        .await
+        .map_err(|e| (StatusCode::BAD_GATEWAY, format!("reading the file failed: {e}")))?
+        .into_bytes();
+
+    // The stored key is `<uuid>-<original name>`; the person who gets
+    // the file should see the name they sent, not the uuid.
+    let filename = key.split_once('-').map(|(_, rest)| rest).unwrap_or(key);
+    let filename = filename.rsplit('/').next().unwrap_or(filename).replace(['"', '\\'], "");
+    Ok((
+        [
+            (axum::http::header::CONTENT_TYPE, content_type),
+            (
+                axum::http::header::CONTENT_DISPOSITION,
+                format!("attachment; filename=\"{filename}\""),
+            ),
+        ],
+        body,
+    )
+        .into_response())
+}
+
+/// Whether this caller may see this bucket at all: whether any feature
+/// in the loaded content lists it and the policy lets them read that
+/// feature. The desk that shows the record is what grants the file.
+async fn may_read_bucket(
+    state: &AppState,
+    session: &tower_sessions::Session,
+    bucket: &str,
+) -> bool {
+    use crate::auth::{User, SESSION_USER_KEY};
+
+    let user = session.get::<User>(SESSION_USER_KEY).await.ok().flatten();
+    let principal = crate::access::Principal::from_user(user.as_ref());
+    let access = state.access.load();
+    state.questions.load().values().any(|question| {
+        question.alternatives.iter().any(|alternative| {
+            alternative.features.iter().any(|feature| {
+                feature.resource.as_ref().and_then(|r| r.bucket()) == Some(bucket)
+                    && access.allows(
+                        &principal,
+                        crate::access::Action::Read,
+                        &crate::access::Resource::Feature {
+                            page: question.id.clone(),
+                            alternative: alternative.name.clone(),
+                            feature: feature.name.clone(),
+                        },
+                    )
+            })
+        })
+    })
+}