feat(api): polish a description before the mission exists, and download an artifact
Two endpoints the wizard redesign needs.
`POST /api/missions/refine-draft` — the polish button fires while the user is
still typing, before anything is created, so it has no id to route on.
`refine` deliberately requires a saved draft because its Accept writes back;
this one has nothing to write back to and returns the text. Same system prompt,
same model chain. The phase list comes from the workflow recipe rather than the
caller, for the same reason `phases_for_create` prefers it: a client that
guessed would have the model write acceptance criteria for phases the mission
will not run.
`GET /api/missions/{id}/artifacts/{artifact_id}/download` — the file itself.
`artifact_content` caps at 2 MiB and reads as UTF-8, so a large or binary
artifact is unreachable by any means today; this streams the bytes with a
filename attached and no ceiling.
Both artifact routes now resolve through ONE containment check. Two copies of
"is this path under _outputs" is two chances for one of them to be the lenient
one, and the lenient one is an arbitrary read of the gateway's filesystem — so a
test asserts there is a single resolver and that both routes call it.
The download filename was chosen by an AGENT and lands in a header every browser
parses, so quotes, backslashes and control characters are stripped rather than
escaped; the test covers a header-injection attempt.
Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
Omar Sobh
co-authored by
Claude Opus 5
Claude Opus 5
parent
f27d2605eb
commit
25f075a8be
@@ -344,6 +344,88 @@ pub async fn get(
|
||||
///
|
||||
/// Text only, and capped: these are markdown documents, and streaming an
|
||||
/// arbitrary captured file into a JSON body is not what this is for.
|
||||
/// Turn a stored artifact path into an absolute one, refusing anything outside
|
||||
/// `_outputs`.
|
||||
///
|
||||
/// Shared by the read and download routes deliberately: two copies of a
|
||||
/// containment check is two chances for one of them to be the lenient one, and
|
||||
/// the lenient one is a path-traversal read of the gateway's filesystem.
|
||||
fn resolve_artifact_path(stored: &str) -> Result<std::path::PathBuf, ApiError> {
|
||||
let root = crate::mission_outputs::outputs_root_dir();
|
||||
let abs = crate::mission_outputs::missions_root_dir().join(stored);
|
||||
// `canonicalize` on BOTH sides, so a symlink out of the tree resolves to
|
||||
// its target before the comparison rather than after.
|
||||
let resolved = std::fs::canonicalize(&abs).map_err(|_| ApiError::NotFound)?;
|
||||
let root = std::fs::canonicalize(&root).map_err(|_| ApiError::NotFound)?;
|
||||
if !resolved.starts_with(&root) {
|
||||
eprintln!(
|
||||
"missions: refused artifact {} — outside {}",
|
||||
resolved.display(),
|
||||
root.display()
|
||||
);
|
||||
return Err(ApiError::NotFound);
|
||||
}
|
||||
Ok(resolved)
|
||||
}
|
||||
|
||||
/// `GET /api/missions/{id}/artifacts/{artifact_id}/download` — the file itself.
|
||||
///
|
||||
/// Separate from `artifact_content` because that route cannot serve the two
|
||||
/// cases a download exists for: it caps at 2 MiB and reads as UTF-8, so a large
|
||||
/// or binary artifact is unreachable by any means today. This one streams the
|
||||
/// bytes with a filename attached and no ceiling.
|
||||
pub async fn artifact_download(
|
||||
State(state): State<AppState>,
|
||||
Authed(user): Authed,
|
||||
Path((id, artifact_id)): Path<(Uuid, Uuid)>,
|
||||
) -> Result<axum::response::Response, ApiError> {
|
||||
use axum::response::IntoResponse;
|
||||
|
||||
cm_db::repo::missions::get(&state.pool, id, user.workspace_id.as_uuid())
|
||||
.await?
|
||||
.ok_or(ApiError::NotFound)?;
|
||||
let artifacts = cm_db::repo::missions::artifacts_for(&state.pool, id).await?;
|
||||
let artifact = artifacts
|
||||
.into_iter()
|
||||
.find(|a| a.id == artifact_id)
|
||||
.ok_or(ApiError::NotFound)?;
|
||||
let resolved = resolve_artifact_path(&artifact.path)?;
|
||||
|
||||
let bytes = tokio::fs::read(&resolved)
|
||||
.await
|
||||
.map_err(|_| ApiError::NotFound)?;
|
||||
|
||||
// The basename, never the stored path: `_outputs/<mission>/<phase>/repo/x.md`
|
||||
// as a filename would arrive as a browser-mangled string, and the path is
|
||||
// internal layout the user has no reason to see.
|
||||
let name = resolved
|
||||
.file_name()
|
||||
.and_then(|n| n.to_str())
|
||||
.filter(|n| !n.is_empty())
|
||||
.unwrap_or("artifact");
|
||||
// Quoted and stripped of quotes/newlines: a filename is attacker-influenced
|
||||
// input (an agent chose it) and this header is parsed by every browser.
|
||||
let safe: String = name
|
||||
.chars()
|
||||
.filter(|c| *c != '"' && *c != '\\' && !c.is_control())
|
||||
.collect();
|
||||
|
||||
Ok((
|
||||
[
|
||||
(
|
||||
axum::http::header::CONTENT_TYPE,
|
||||
artifact.mime.unwrap_or_else(|| "application/octet-stream".into()),
|
||||
),
|
||||
(
|
||||
axum::http::header::CONTENT_DISPOSITION,
|
||||
format!("attachment; filename=\"{safe}\""),
|
||||
),
|
||||
],
|
||||
bytes,
|
||||
)
|
||||
.into_response())
|
||||
}
|
||||
|
||||
pub async fn artifact_content(
|
||||
State(state): State<AppState>,
|
||||
Authed(user): Authed,
|
||||
@@ -363,18 +445,7 @@ pub async fn artifact_content(
|
||||
.find(|a| a.id == artifact_id)
|
||||
.ok_or(ApiError::NotFound)?;
|
||||
|
||||
let root = crate::mission_outputs::outputs_root_dir();
|
||||
let abs = crate::mission_outputs::missions_root_dir().join(&artifact.path);
|
||||
let resolved = std::fs::canonicalize(&abs).map_err(|_| ApiError::NotFound)?;
|
||||
let root = std::fs::canonicalize(&root).map_err(|_| ApiError::NotFound)?;
|
||||
if !resolved.starts_with(&root) {
|
||||
eprintln!(
|
||||
"missions::artifact_content: refused {} — outside {}",
|
||||
resolved.display(),
|
||||
root.display()
|
||||
);
|
||||
return Err(ApiError::NotFound);
|
||||
}
|
||||
let resolved = resolve_artifact_path(&artifact.path)?;
|
||||
|
||||
let meta = std::fs::metadata(&resolved).map_err(|_| ApiError::NotFound)?;
|
||||
if meta.len() > MAX_BYTES {
|
||||
@@ -610,6 +681,61 @@ pub struct RefineResponse {
|
||||
pub refined: String,
|
||||
}
|
||||
|
||||
#[derive(Debug, Deserialize)]
|
||||
pub struct RefineDraftRequest {
|
||||
#[serde(default)]
|
||||
pub title: String,
|
||||
pub description: String,
|
||||
#[serde(default)]
|
||||
pub template_kind: Option<String>,
|
||||
}
|
||||
|
||||
/// `POST /api/missions/refine-draft` — polish a description with no mission
|
||||
/// behind it yet.
|
||||
///
|
||||
/// The wizard's polish button fires while the user is still typing, before
|
||||
/// anything is created. `refine` deliberately requires a saved draft so its
|
||||
/// Accept can write back; this one has nothing to write back to and returns the
|
||||
/// text for the caller to put in the box.
|
||||
///
|
||||
/// The phase list comes from the workflow recipe rather than the caller, for
|
||||
/// the same reason `phases_for_create` prefers it: the recipe is the
|
||||
/// authoritative composition, and a client that guessed would have the model
|
||||
/// write acceptance criteria for phases the mission will not run.
|
||||
pub async fn refine_draft(
|
||||
State(state): State<AppState>,
|
||||
Authed(_user): Authed,
|
||||
Json(req): Json<RefineDraftRequest>,
|
||||
) -> Result<Json<RefineResponse>, ApiError> {
|
||||
let phase_kinds: Vec<String> = req
|
||||
.template_kind
|
||||
.as_deref()
|
||||
.and_then(crate::workflow_registry::get)
|
||||
.map(|r| r.phases.iter().map(|p| p.kind.clone()).collect())
|
||||
.unwrap_or_default();
|
||||
|
||||
let result = crate::mission_refiner::refine_draft(
|
||||
&state.runtime,
|
||||
req.title.trim(),
|
||||
req.template_kind.as_deref().unwrap_or("custom"),
|
||||
&phase_kinds,
|
||||
&req.description,
|
||||
)
|
||||
.await
|
||||
.map_err(|e| {
|
||||
eprintln!("refine-draft failed: {e}");
|
||||
if e.contains("empty") {
|
||||
ApiError::BadRequest
|
||||
} else {
|
||||
crate::subscription::as_api_error(&e)
|
||||
}
|
||||
})?;
|
||||
Ok(Json(RefineResponse {
|
||||
original: result.original,
|
||||
refined: result.refined,
|
||||
}))
|
||||
}
|
||||
|
||||
/// POST /api/missions/{id}/refine — generate a coherent, sectioned
|
||||
/// Markdown rewrite of the current description WITHOUT persisting.
|
||||
/// Frontend renders a before/after diff; user hits Accept (PATCH
|
||||
@@ -1531,3 +1657,43 @@ mod tests {
|
||||
assert!(outputs_of(Some(&serde_json::json!({}))).is_empty());
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod artifact_tests {
|
||||
/// A filename reaches `Content-Disposition` after an AGENT chose it.
|
||||
///
|
||||
/// The value is attacker-influenced and parsed by every browser, so the
|
||||
/// quote and control characters that would end the header early — or inject
|
||||
/// a second one — are removed rather than escaped.
|
||||
#[test]
|
||||
fn a_downloaded_filename_cannot_break_out_of_its_header() {
|
||||
let clean = |name: &str| -> String {
|
||||
name.chars()
|
||||
.filter(|c| *c != '"' && *c != '\\' && !c.is_control())
|
||||
.collect()
|
||||
};
|
||||
assert_eq!(clean("findings.md"), "findings.md");
|
||||
assert_eq!(clean("re\"port.md"), "report.md");
|
||||
assert_eq!(clean("a\r\nX-Evil: 1.md"), "aX-Evil: 1.md");
|
||||
assert_eq!(clean("back\\slash.md"), "backslash.md");
|
||||
}
|
||||
|
||||
/// Both artifact routes resolve through ONE containment check.
|
||||
///
|
||||
/// Two copies is two chances for one of them to be the lenient one, and the
|
||||
/// lenient one is an arbitrary read of the gateway's filesystem.
|
||||
#[test]
|
||||
fn one_containment_check_serves_both_routes() {
|
||||
let src = include_str!("missions.rs");
|
||||
assert_eq!(
|
||||
src.matches(concat!("fn resolve_", "artifact_path")).count(),
|
||||
1,
|
||||
"one resolver"
|
||||
);
|
||||
assert_eq!(
|
||||
src.matches(concat!("resolve_", "artifact_path(&artifact.path)")).count(),
|
||||
2,
|
||||
"and both routes must go through it"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user