From bbd9ab17843fc8de489ec887ee64c536f36c1f12 Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 20 Aug 2026 18:26:30 -0500 Subject: [PATCH] Add detail fields to invalid message errors --- aggregator/src/aggregator.rs | 99 ++++++++++++---------- aggregator/src/aggregator/http_handlers.rs | 12 ++- 2 files changed, 61 insertions(+), 50 deletions(-) diff --git a/aggregator/src/aggregator.rs b/aggregator/src/aggregator.rs index 0d991ec6e..07eea0e13 100644 --- a/aggregator/src/aggregator.rs +++ b/aggregator/src/aggregator.rs @@ -391,58 +391,63 @@ impl Aggregator { task_id_base64: Option<&[u8]>, ) -> Result<(Vec, Option), Error> { // Retrieve the appropriate HPKE config list. - let hpke_config_list = - if self.cfg.taskprov_config.enabled || self.cfg.require_global_hpke_keys { - // If we're running in taskprov mode or requiring global keys, unconditionally - // provide the global keys and ignore the task_id parameter. - let configs = self.global_hpke_keypairs.configs(); - if configs.is_empty() { - return Err(Error::Internal( - "this server is missing its global HPKE config".into(), - )); - } else { - HpkeConfigList::new(configs.to_vec()) - } + let hpke_config_list = if self.cfg.taskprov_config.enabled + || self.cfg.require_global_hpke_keys + { + // If we're running in taskprov mode or requiring global keys, unconditionally + // provide the global keys and ignore the task_id parameter. + let configs = self.global_hpke_keypairs.configs(); + if configs.is_empty() { + return Err(Error::Internal( + "this server is missing its global HPKE config".into(), + )); } else { - // Otherwise, try to get the task-specific key. - match task_id_base64 { - Some(task_id_base64) => { - let task_id_bytes = URL_SAFE_NO_PAD - .decode(task_id_base64) - .map_err(|_| Error::InvalidMessage(None, "task_id"))?; - let task_id = TaskId::get_decoded(&task_id_bytes) - .map_err(|_| Error::InvalidMessage(None, "task_id"))?; - let task_aggregator = self - .task_aggregators - .get(&task_id) - .await? - .ok_or(Error::UnrecognizedTask(task_id))?; - - match task_aggregator.handle_hpke_config() { - Some(hpke_config_list) => hpke_config_list, - // Assuming something hasn't gone horribly wrong with the database, this - // should only happen in the case where the system has been moved from taskprov - // mode to non-taskprov mode. Thus there's still taskprov tasks in the database. - // This isn't a supported use case, so the operator needs to delete these tasks - // or move the system back into taskprov mode. - None => { - return Err(Error::Internal("task has no HPKE configs".to_string())) - } + HpkeConfigList::new(configs.to_vec()) + } + } else { + // Otherwise, try to get the task-specific key. + match task_id_base64 { + Some(task_id_base64) => { + let task_id_bytes = URL_SAFE_NO_PAD.decode(task_id_base64).map_err(|_| { + Error::InvalidMessage(None, "task_id parameter is not valid base64url") + })?; + let task_id = TaskId::get_decoded(&task_id_bytes).map_err(|_| { + Error::InvalidMessage( + None, + "task_id parameter is not of the correct length", + ) + })?; + let task_aggregator = self + .task_aggregators + .get(&task_id) + .await? + .ok_or(Error::UnrecognizedTask(task_id))?; + + match task_aggregator.handle_hpke_config() { + Some(hpke_config_list) => hpke_config_list, + // Assuming something hasn't gone horribly wrong with the database, this + // should only happen in the case where the system has been moved from taskprov + // mode to non-taskprov mode. Thus there's still taskprov tasks in the database. + // This isn't a supported use case, so the operator needs to delete these tasks + // or move the system back into taskprov mode. + None => { + return Err(Error::Internal("task has no HPKE configs".to_string())) } } - // No task ID present, try to fall back to a global config. - None => { - let configs = self.global_hpke_keypairs.configs(); - if configs.is_empty() { - // This server isn't configured to provide global HPKE keys, the client - // should have given us a task ID. - return Err(Error::MissingTaskId); - } else { - HpkeConfigList::new(configs.to_vec()) - } + } + // No task ID present, try to fall back to a global config. + None => { + let configs = self.global_hpke_keypairs.configs(); + if configs.is_empty() { + // This server isn't configured to provide global HPKE keys, the client + // should have given us a task ID. + return Err(Error::MissingTaskId); + } else { + HpkeConfigList::new(configs.to_vec()) } } - }; + } + }; // Encode & (if configured to do so) sign the HPKE config list. let encoded_hpke_config_list = hpke_config_list diff --git a/aggregator/src/aggregator/http_handlers.rs b/aggregator/src/aggregator/http_handlers.rs index 0128af572..eb9f7f1ab 100644 --- a/aggregator/src/aggregator/http_handlers.rs +++ b/aggregator/src/aggregator/http_handlers.rs @@ -51,7 +51,10 @@ async fn run_error_handler(error: &Error, mut conn: Conn) -> Conn { let conn = match error { Error::InvalidConfiguration(_) => conn.with_status(Status::InternalServerError), Error::MessageDecode(_) => { - conn.with_problem_document(&ProblemDocument::new_dap(DapProblemType::InvalidMessage)) + conn.with_problem_document( + &ProblemDocument::new_dap(DapProblemType::InvalidMessage) + .with_detail("Could not decode a message"), + ) } Error::MessageEncode(_) => conn.with_status(Status::InternalServerError), Error::ReportRejected(rejection) => match rejection.reason() { @@ -69,8 +72,9 @@ async fn run_error_handler(error: &Error, mut conn: Conn) -> Conn { .with_detail(rejection.reason().detail()), ), }, - Error::InvalidMessage(task_id, _) => { + Error::InvalidMessage(task_id, detail) => { let mut doc = ProblemDocument::new_dap(DapProblemType::InvalidMessage); + doc = doc.with_detail(detail); if let Some(task_id) = task_id { doc = doc.with_task_id(task_id); } @@ -148,7 +152,9 @@ async fn run_error_handler(error: &Error, mut conn: Conn) -> Conn { | Error::TaskParameters(_) => conn.with_status(Status::InternalServerError), Error::AggregateShareRequestRejected(_, _) => conn.with_status(Status::BadRequest), Error::EmptyAggregation(task_id) => conn.with_problem_document( - &ProblemDocument::new_dap(DapProblemType::InvalidMessage).with_task_id(task_id), + &ProblemDocument::new_dap(DapProblemType::InvalidMessage) + .with_task_id(task_id) + .with_detail("An empty aggregation with no report shares was attempted"), ), Error::ForbiddenMutation { .. } => conn.with_status(Status::Conflict), Error::BadRequest(_) => conn.with_status(Status::BadRequest),