From 5176e3e8c7b310b216ff5221c14bb894841a60ce Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Mon, 20 May 2024 20:41:01 +0100 Subject: [PATCH] fix(server): invalid webhook authorisations Parameters had been passed in wrong order. Added strong types to prevent a repeat. --- Cargo.toml | 2 +- crates/server/src/actors/repo/webhook.rs | 28 +++--- crates/server/src/actors/webhook/message.rs | 30 ++++--- crates/server/src/actors/webhook/router.rs | 4 +- crates/server/src/actors/webhook/server.rs | 99 ++++++++++++--------- 5 files changed, 91 insertions(+), 72 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index c541f20..ee1bcbc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,7 +3,7 @@ resolver = "2" members = ["crates/cli", "crates/server", "crates/config", "crates/git"] [workspace.package] -version = "0.5.0" +version = "0.5.1" edition = "2021" [workspace.lints.clippy] diff --git a/crates/server/src/actors/repo/webhook.rs b/crates/server/src/actors/repo/webhook.rs index 3dc1e4c..659ad06 100644 --- a/crates/server/src/actors/repo/webhook.rs +++ b/crates/server/src/actors/repo/webhook.rs @@ -24,8 +24,9 @@ pub struct WebhookId(String); pub struct WebhookAuth(ulid::Ulid); impl WebhookAuth { pub fn from_str(authorisation: &str) -> Result { - let id = ulid::Ulid::from_str(authorisation); - Ok(Self(id?)) + let id = ulid::Ulid::from_str(authorisation)?; + info!("Parse auth token: {}", id); + Ok(Self(id)) } fn generate() -> Self { @@ -186,23 +187,16 @@ impl Handler for RepoActor { warn!("Don't know what authorization to expect"); return; }; - let Some(received_authorization) = &msg.authorisation() else { - warn!("Missing authorization token"); - return; - }; - if received_authorization != expected_authorization { + if msg.authorisation() != expected_authorization { warn!( "Invalid authorization - expected {}", expected_authorization ); return; } - let id = msg.id(); - let span = tracing::info_span!("handle", %id); - let _guard = span.enter(); let body = msg.body(); - match serde_json::from_str::(body) { - Err(err) => warn!(?err, %body, "Not a 'push'"), + match serde_json::from_str::(body.as_str()) { + Err(err) => warn!(?err, ?body, "Not a 'push'"), Ok(push) => { if let Some(config) = &self.details.repo_config { match push.branch(config.branches()) { @@ -262,6 +256,10 @@ impl Handler for RepoActor { } } +pub fn split_ref(reference: &str) -> (&str, &str) { + reference.split_at(11) +} + #[derive(Debug, serde::Deserialize)] struct Push { #[serde(rename = "ref")] @@ -298,12 +296,8 @@ impl Push { } } -pub fn split_ref(reference: &str) -> (&str, &str) { - reference.split_at(11) -} - #[derive(Debug)] -enum Branch { +pub enum Branch { Main, Next, Dev, diff --git a/crates/server/src/actors/webhook/message.rs b/crates/server/src/actors/webhook/message.rs index 92551a9..8ab3709 100644 --- a/crates/server/src/actors/webhook/message.rs +++ b/crates/server/src/actors/webhook/message.rs @@ -1,27 +1,33 @@ // use actix::prelude::*; +use git_next_config::RepoAlias; use crate::actors::repo::webhook::WebhookAuth; #[derive(Message, Debug, Clone, derive_more::Constructor)] #[rtype(result = "()")] pub struct WebhookMessage { - id: String, - path: String, - authorisation: String, - body: String, + // forge // TODO: differentiate between multiple forges + repo_alias: RepoAlias, + authorisation: WebhookAuth, + body: Body, } impl WebhookMessage { - pub const fn id(&self) -> &String { - &self.id + pub const fn repo_alias(&self) -> &RepoAlias { + &self.repo_alias } - pub const fn path(&self) -> &String { - &self.path - } - pub const fn body(&self) -> &String { + pub const fn body(&self) -> &Body { &self.body } - pub fn authorisation(&self) -> Option { - WebhookAuth::from_str(&self.authorisation).ok() + pub const fn authorisation(&self) -> &WebhookAuth { + &self.authorisation + } +} + +#[derive(Clone, Debug, derive_more::Constructor)] +pub struct Body(String); +impl Body { + pub fn as_str(&self) -> &str { + self.0.as_str() } } diff --git a/crates/server/src/actors/webhook/router.rs b/crates/server/src/actors/webhook/router.rs index 381d23f..e5a9210 100644 --- a/crates/server/src/actors/webhook/router.rs +++ b/crates/server/src/actors/webhook/router.rs @@ -28,9 +28,9 @@ impl Handler for WebhookRouter { fn handle(&mut self, msg: WebhookMessage, _ctx: &mut Self::Context) -> Self::Result { let _gaurd = self.span.enter(); - let repo_alias = RepoAlias::new(msg.path()); + let repo_alias = msg.repo_alias(); debug!(repo = %repo_alias, "Router..."); - if let Some(recipient) = self.repos.get(&repo_alias) { + if let Some(recipient) = self.repos.get(repo_alias) { info!(repo = %repo_alias, "Sending to Recipient"); recipient.do_send(msg); } diff --git a/crates/server/src/actors/webhook/server.rs b/crates/server/src/actors/webhook/server.rs index 78dad89..4463ae5 100644 --- a/crates/server/src/actors/webhook/server.rs +++ b/crates/server/src/actors/webhook/server.rs @@ -2,13 +2,17 @@ use std::net::SocketAddr; use actix::prelude::*; +use git_next_config::RepoAlias; use tracing::{info, warn}; +use warp::reject::Rejection; -use crate::actors::webhook::message::WebhookMessage; +use crate::actors::{repo::webhook::WebhookAuth, webhook::message::WebhookMessage}; + +use super::message; pub async fn start( socket_addr: SocketAddr, - address: actix::prelude::Recipient, + address: actix::prelude::Recipient, ) { // start webhook server use warp::Filter; @@ -21,51 +25,42 @@ pub async fn start( .and(warp::body::bytes()) .and_then( |recipient: Recipient, - path, + path: String, // query: String, headers: warp::http::HeaderMap, body: bytes::Bytes| async move { info!("POST received"); + let repo_alias = RepoAlias::new(path); let bytes = body.to_vec(); - let request_data = String::from_utf8_lossy(&bytes).to_string(); - let id = ulid::Ulid::new().to_string(); - match headers.get("Authorization") { - Some(auhorisation) => { - info!(id, path, "Received webhook"); - let authorisation = auhorisation - .to_str() - .map_err(|e| { - warn!("Invalid value in authorization: {:?}", e); - warp::reject() - })? // valid characters - .strip_prefix("Basic ") - .ok_or_else(|| { - warn!("Authorization must be 'Basic'"); - warp::reject() - })? // must start with "Basic " - .to_string(); - let message = WebhookMessage::new( - id, - path, - /* query, headers, */ request_data, - authorisation, - ); - recipient - .try_send(message) - .map(|_| { - info!("Message sent ok"); - warp::reply::with_status("OK", warp::http::StatusCode::OK) - }) - .map_err(|e| { - warn!("Unknown error: {:?}", e); - warp::reject() - }) - } - _ => { + let body = message::Body::new(String::from_utf8_lossy(&bytes).to_string()); + headers.get("Authorization").map_or_else( + || { warn!("No Authorization header"); Err(warp::reject()) - } - } + }, + |authorisation_header| { + info!(?repo_alias, ?authorisation_header, "Received webhook",); + match parse_auth(authorisation_header) { + Ok(authorisation) => { + let message = WebhookMessage::new(repo_alias, authorisation, body); + recipient + .try_send(message) + .map(|_| { + info!("Message sent ok"); + warp::reply::with_status("OK", warp::http::StatusCode::OK) + }) + .map_err(|e| { + warn!("Unknown error: {:?}", e); + warp::reject() + }) + } + Err(e) => { + warn!(?e, "Failed to decode authorization header"); + Err(warp::reject()) + } + } + }, + ) }, ); @@ -73,3 +68,27 @@ pub async fn start( info!("Starting webhook server: {}", socket_addr); warp::serve(route).run(socket_addr).await; } + +fn parse_auth(authorization_header: &warp::http::HeaderValue) -> Result { + WebhookAuth::from_str( + authorization_header + .to_str() + .map_err(|e| { + warn!("Invalid non-ascii value in authorization: {:?}", e); + warp::reject() + }) // valid characters + .map(|v| { + info!("raw auth header: {}", v); + v + })? + .strip_prefix("Basic ") + .ok_or_else(|| { + warn!("Authorization must be 'Basic'"); + warp::reject() + })?, // must start with "Basic " + ) + .map_err(|e| { + warn!(?e, "decode error"); + warp::reject() + }) +}