diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f00a47..724fa7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ content repos pin the tag they run (see the end of this file). ## Unreleased +- `/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: ` 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. - A public view need not publish the whole bucket. `states: [...]` on a Kv resource shows only those, for one record as well as a listing - a register that is public by law is public about what was decided, not about what was withdrawn. The desk over the same bucket is a different spec and still sees all of it. diff --git a/src/app.rs b/src/app.rs index 5d5b5ab..6e23cb8 100644 --- a/src/app.rs +++ b/src/app.rs @@ -1042,7 +1042,7 @@ fn AlternativeCard( }; let Some(files) = input.files() else { continue }; let Some(file) = files.get(0) else { continue }; - match upload_file(&file).await { + match upload_file(&file, &question_id_for_submit, &alt_name_for_submit).await { Ok(key) => { map.insert(name.clone(), serde_json::Value::String(key)); } @@ -2541,12 +2541,21 @@ pub async fn submit_answer( /// server side is a raw Axum multipart handler (`src/upload.rs`) for /// the same reason. #[cfg(feature = "hydrate")] -async fn upload_file(file: &web_sys::File) -> Result { +async fn upload_file( + file: &web_sys::File, + question_id: &str, + alternative: &str, +) -> Result { use wasm_bindgen::{JsCast, JsValue}; use wasm_bindgen_futures::JsFuture; use web_sys::{FormData, Request, RequestInit, Response}; let form = FormData::new()?; + // Which page and which choice this file is being attached to. The + // server refuses an upload that does not name a page it may + // submit, so these travel with every file. + form.append_with_str("question", question_id)?; + form.append_with_str("alternative", alternative)?; form.append_with_blob("file", file)?; let opts = RequestInit::new(); diff --git a/src/upload.rs b/src/upload.rs index fa3c73c..bc89d1e 100644 --- a/src/upload.rs +++ b/src/upload.rs @@ -57,13 +57,23 @@ pub struct UploadResponse { key: String, } -/// POST /upload - streams the first `file` field straight through to -/// Garage and hands back its object key. Server-side hygiene beyond the -/// HTML `accept` hint (a UX-only signal, not a boundary): a hard size -/// cap, and the upload is rejected outright if Garage isn't configured -/// rather than silently succeeding nowhere. +/// POST /upload - takes the `file` field, plus the `question` and +/// `alternative` it belongs to, and hands back its object key. +/// +/// The two names are not decoration: they are what makes this an +/// authorized route rather than an open one. An upload is half a +/// submission, so it asks the policy exactly what the submission will +/// ask - `Submit` on that page - and it only accepts a page that +/// actually declares a `type: file` field. Without that, anybody who +/// could reach the host could put twenty megabytes in the bucket as +/// often as they liked, on a site whose forms are public. +/// +/// Server-side hygiene beyond the HTML `accept` hint (a UX signal, not +/// a boundary): a hard size cap, and a refusal rather than a silent +/// success when Garage isn't configured. pub async fn upload( State(state): State, + session: tower_sessions::Session, mut multipart: Multipart, ) -> Result, (StatusCode, String)> { let garage = state.garage.as_ref().ok_or(( @@ -71,49 +81,172 @@ pub async fn upload( "uploads are not configured".to_string(), ))?; - while let Some(field) = multipart + let mut question_id = String::new(); + let mut alternative = String::new(); + let mut file: Option<(String, String, Vec)> = None; + + while let Some(mut field) = multipart .next_field() .await .map_err(|e| (StatusCode::BAD_REQUEST, e.to_string()))? { - if field.name() != Some("file") { - continue; - } - let content_type = field - .content_type() - .unwrap_or("application/octet-stream") - .to_string(); - let filename = field.file_name().unwrap_or("upload").to_string(); - // Chunk by chunk, refused the moment the cap is crossed: an - // oversized body is never buffered whole before being turned - // away. The route's body limit (main.rs) is the outer fence. - let mut bytes: Vec = Vec::new(); - let mut field = field; - while let Some(chunk) = field - .chunk() - .await - .map_err(|e| (StatusCode::BAD_REQUEST, e.to_string()))? - { - if bytes.len() + chunk.len() > MAX_UPLOAD_BYTES { - return Err((StatusCode::PAYLOAD_TOO_LARGE, "file too large".to_string())); + match field.name() { + Some("question") | Some("alternative") => { + let name = field.name().unwrap_or_default().to_string(); + let text = field + .text() + .await + .map_err(|e| (StatusCode::BAD_REQUEST, e.to_string()))?; + if name == "question" { + question_id = text; + } else { + alternative = text; + } } - bytes.extend_from_slice(&chunk); + Some("file") => { + let content_type = field + .content_type() + .unwrap_or("application/octet-stream") + .to_string(); + let filename = field.file_name().unwrap_or("upload").to_string(); + // Chunk by chunk, refused the moment the cap is + // crossed: an oversized body is never buffered whole + // before being turned away. The route's body limit + // (main.rs) is the outer fence. + let mut bytes: Vec = Vec::new(); + while let Some(chunk) = field + .chunk() + .await + .map_err(|e| (StatusCode::BAD_REQUEST, e.to_string()))? + { + if bytes.len() + chunk.len() > MAX_UPLOAD_BYTES { + return Err((StatusCode::PAYLOAD_TOO_LARGE, "file too large".to_string())); + } + bytes.extend_from_slice(&chunk); + } + file = Some((filename, content_type, bytes)); + } + _ => continue, } - - let key = format!("{}-{filename}", uuid::Uuid::new_v4()); - garage - .client - .put_object() - .bucket(&garage.bucket) - .key(&key) - .body(ByteStream::from(bytes)) - .content_type(content_type) - .send() - .await - .map_err(|e| (StatusCode::BAD_GATEWAY, format!("upload failed: {e}")))?; - - return Ok(Json(UploadResponse { key })); } - Err((StatusCode::BAD_REQUEST, "no file field in request".to_string())) + let (filename, content_type, bytes) = + file.ok_or((StatusCode::BAD_REQUEST, "no file field in request".to_string()))?; + + // Nothing has been written yet. Everything below decides whether + // anything will be. + if !may_upload(&state, &session, &question_id, &alternative).await { + return Err((StatusCode::FORBIDDEN, "not authorized to upload here".to_string())); + } + + let key = format!("{}-{filename}", uuid::Uuid::new_v4()); + garage + .client + .put_object() + .bucket(&garage.bucket) + .key(&key) + .body(ByteStream::from(bytes)) + .content_type(content_type) + .send() + .await + .map_err(|e| (StatusCode::BAD_GATEWAY, format!("upload failed: {e}")))?; + + Ok(Json(UploadResponse { key })) +} + +/// Whether this caller may attach a file to that page's alternative: +/// the alternative has to exist, has to declare a `type: file` field, +/// and the policy has to let this principal submit that page. The same +/// question `submit_answer` asks, asked before the bytes land rather +/// than after. +async fn may_upload( + state: &AppState, + session: &tower_sessions::Session, + question_id: &str, + alternative: &str, +) -> bool { + use crate::auth::{User, SESSION_USER_KEY}; + + let questions = state.questions.load(); + let Some(page) = page_taking_files(&questions, question_id, alternative) else { + return false; + }; + let user = session.get::(SESSION_USER_KEY).await.ok().flatten(); + let principal = crate::access::Principal::from_user(user.as_ref()); + crate::access::decide_and_publish( + &state.nats, + &state.access.load(), + &principal, + crate::access::Action::Submit, + &crate::access::Resource::Page { id: page }, + serde_json::json!({ "alternative": alternative }), + ) + .allow +} + +/// The resolved page id, when that page has an alternative of this +/// name that is open and actually asks for a file. `None` for a page +/// that does not exist, an alternative that does not, one that is +/// disabled, or one that asks for no file at all - an upload that +/// names none of the site's own file fields is not an upload this site +/// asked for. +fn page_taking_files( + questions: &std::collections::HashMap, + question_id: &str, + alternative: &str, +) -> Option { + let question = crate::content::resolve_question(questions, question_id)?; + question + .alternatives + .iter() + .find(|a| a.name == alternative && !a.disabled)? + .features + .iter() + .any(|f| f.requirements.iter().any(|r| r.kind == "file")) + .then(|| question.id.clone()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn questions(yaml: &str) -> std::collections::HashMap { + crate::content::build_questions(&[("index.yaml".to_string(), yaml.to_string())]).unwrap() + } + + const PAGE: &str = "name: Home +alternatives: + - name: Send drawings + record_as: enquiries + features: + - name: Files + requirements: + - { name: drawings, type: file, optional: true } + - name: Just ask + features: + - name: Words + requirements: + - { name: question, type: textarea } +"; + + // The bytes are only ever written for a page of this site that + // asked for a file. Everything else is somebody using the host as + // a disk. + #[test] + fn an_upload_must_name_a_page_that_asked_for_a_file() { + let qs = questions(PAGE); + assert_eq!(page_taking_files(&qs, "/", "Send drawings"), Some("/".to_string())); + // An alternative on the same page that takes no file. + assert_eq!(page_taking_files(&qs, "/", "Just ask"), None); + // Names that are not there at all. + assert_eq!(page_taking_files(&qs, "/", "Nonexistent"), None); + assert_eq!(page_taking_files(&qs, "/nowhere", "Send drawings"), None); + assert_eq!(page_taking_files(&qs, "", ""), None); + } + + #[test] + fn a_closed_alternative_takes_nothing() { + let qs = questions(&PAGE.replace(" record_as: enquiries\n", " record_as: enquiries\n disabled: true\n")); + assert_eq!(page_taking_files(&qs, "/", "Send drawings"), None); + } }