Stop accepting repeated, duplicate Authorization headers. (#3186)

The issue that led to us accepting this behavior has been fixed. Given that this behavior is not RFC-compliant, we remove it from Janus.
divviup · Jun 20, 2024 · 99775e1 · 99775e1
1 parent 8f07eb1
commit 99775e1
Show file tree

Hide file tree

Showing 4 changed files with 10 additions and 62 deletions.
diff --git a/aggregator/src/aggregator/aggregate_init_tests.rs b/aggregator/src/aggregator/aggregate_init_tests.rs
@@ -308,33 +308,11 @@ pub(crate) async fn put_aggregation_job<Q: query_type::QueryType>(
     aggregation_job_id: &AggregationJobId,
     aggregation_job: &AggregationJobInitializeReq<Q>,
     handler: &impl Handler,
-) -> TestConn {
-    put_aggregation_job_with_auth_header_count(
-        task,
-        aggregation_job_id,
-        aggregation_job,
-        handler,
-        1,
-    )
-    .await
-}
-
-pub(crate) async fn put_aggregation_job_with_auth_header_count<Q: query_type::QueryType>(
-    task: &Task,
-    aggregation_job_id: &AggregationJobId,
-    aggregation_job: &AggregationJobInitializeReq<Q>,
-    handler: &impl Handler,
-    auth_header_count: usize,
 ) -> TestConn {
     let (header, value) = task.aggregator_auth_token().request_authentication();
 
-    let mut test_conn = put(task.aggregation_job_uri(aggregation_job_id).unwrap().path());
-
-    for _ in 0..auth_header_count {
-        test_conn = test_conn.with_request_header(header, value.clone());
-    }
-
-    test_conn
+    put(task.aggregation_job_uri(aggregation_job_id).unwrap().path())
+        .with_request_header(header, value)
         .with_request_header(
             KnownHeaderName::ContentType,
             AggregationJobInitializeReq::<Q>::MEDIA_TYPE,

diff --git a/aggregator/src/aggregator/http_handlers.rs b/aggregator/src/aggregator/http_handlers.rs
@@ -8,7 +8,7 @@ use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine};
 use janus_aggregator_core::{datastore::Datastore, instrumented};
 use janus_core::{
     auth_tokens::{AuthenticationToken, DAP_AUTH_HEADER},
-    http::{extract_bearer_token, HeadersExt as _},
+    http::extract_bearer_token,
     taskprov::TASKPROV_HEADER,
     time::Clock,
     Runtime,
@@ -686,7 +686,7 @@ fn parse_auth_token(task_id: &TaskId, conn: &Conn) -> Result<Option<Authenticati
     }
 
     conn.request_headers()
-        .get_coalescing_duplicates(DAP_AUTH_HEADER) // TODO(#3163): get_coalescing_duplicates -> get
+        .get(DAP_AUTH_HEADER)
         .map(|value| {
             AuthenticationToken::new_dap_auth_token_from_bytes(value.as_ref())
                 .map_err(|e| Error::BadRequest(format!("bad DAP-Auth-Token header: {e}")))

diff --git a/aggregator/src/aggregator/http_handlers/tests/aggregation_job_init.rs b/aggregator/src/aggregator/http_handlers/tests/aggregation_job_init.rs
@@ -1,7 +1,5 @@
 use crate::aggregator::{
-    aggregate_init_tests::{
-        put_aggregation_job, put_aggregation_job_with_auth_header_count, PrepareInitGenerator,
-    },
+    aggregate_init_tests::{put_aggregation_job, PrepareInitGenerator},
     empty_batch_aggregations,
     http_handlers::{
         aggregator_handler,
@@ -448,15 +446,9 @@ async fn aggregate_init() {
 
     // Send request, parse response. Do this twice to prove that the request is idempotent.
     let aggregation_job_id: AggregationJobId = random();
-    for auth_header_count in 1..=2 {
-        let mut test_conn = put_aggregation_job_with_auth_header_count(
-            &task,
-            &aggregation_job_id,
-            &request,
-            &handler,
-            auth_header_count,
-        )
-        .await;
+    for _ in 0..2 {
+        let mut test_conn =
+            put_aggregation_job(&task, &aggregation_job_id, &request, &handler).await;
         assert_eq!(test_conn.status(), Some(Status::Ok));
         assert_headers!(
             &test_conn,

diff --git a/core/src/http.rs b/core/src/http.rs
@@ -6,7 +6,7 @@ use janus_messages::problem_type::DapProblemType;
 use reqwest::{header::CONTENT_TYPE, Response};
 use std::fmt::{self, Display, Formatter};
 use tracing::warn;
-use trillium::{Conn, HeaderName, HeaderValue, Headers};
+use trillium::{Conn, HeaderValue};
 
 /// This captures an HTTP status code and parsed problem details document from an HTTP response.
 #[derive(Debug)]
@@ -108,7 +108,7 @@ impl Display for HttpErrorResponse {
 pub fn extract_bearer_token(conn: &Conn) -> Result<Option<AuthenticationToken>, anyhow::Error> {
     if let Some(authorization) = conn
         .request_headers()
-        .get_coalescing_duplicates("authorization") // TODO(#3163): get_coalescing_duplicates -> get
+        .get("authorization")
         .map(HeaderValue::to_string)
     {
         let (auth_scheme, token) = authorization
@@ -127,28 +127,6 @@ pub fn extract_bearer_token(conn: &Conn) -> Result<Option<AuthenticationToken>,
     Ok(None)
 }
 
-pub trait HeadersExt {
-    fn get_coalescing_duplicates<'a>(
-        &self,
-        name: impl Into<HeaderName<'a>>,
-    ) -> Option<&HeaderValue>;
-}
-
-impl HeadersExt for Headers {
-    fn get_coalescing_duplicates<'a>(
-        &self,
-        name: impl Into<HeaderName<'a>>,
-    ) -> Option<&HeaderValue> {
-        self.get_values(name).and_then(|header_values| {
-            header_values
-                .iter()
-                .map(Option::Some)
-                .reduce(|l, r| if l == r { l } else { None })
-                .flatten()
-        })
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use assert_matches::assert_matches;