Skip to content

Commit

Permalink
fix(tonic): don't remove reserved headers in interceptor (#701)
Browse files Browse the repository at this point in the history
`InterceptedService` would previously use `tonic::Request::into_http`
which removes reserved headers. That mean inner middleware in the stack
wouldn't be able to see those headers, which could result in errors.

Fixes #700
  • Loading branch information
davidpdrsn authored Jul 8, 2021
1 parent 0583cff commit 6711b80
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 10 deletions.
3 changes: 2 additions & 1 deletion tonic/src/client/grpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::{
body::BoxBody,
client::GrpcService,
codec::{encode_client, Codec, Streaming},
request::SanitizeHeaders,
Code, Request, Response, Status,
};
use futures_core::Stream;
Expand Down Expand Up @@ -247,7 +248,7 @@ impl<T> Grpc<T> {
})
.map(BoxBody::new);

let mut request = request.into_http(uri);
let mut request = request.into_http(uri, SanitizeHeaders::Yes);

// Add the gRPC related HTTP headers
request
Expand Down
20 changes: 17 additions & 3 deletions tonic/src/request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,13 +166,20 @@ impl<T> Request<T> {
Request::from_http_parts(parts, message)
}

pub(crate) fn into_http(self, uri: http::Uri) -> http::Request<T> {
pub(crate) fn into_http(
self,
uri: http::Uri,
sanitize_headers: SanitizeHeaders,
) -> http::Request<T> {
let mut request = http::Request::new(self.message);

*request.version_mut() = http::Version::HTTP_2;
*request.method_mut() = http::Method::POST;
*request.uri_mut() = uri;
*request.headers_mut() = self.metadata.into_sanitized_headers();
*request.headers_mut() = match sanitize_headers {
SanitizeHeaders::Yes => self.metadata.into_sanitized_headers(),
SanitizeHeaders::No => self.metadata.into_headers(),
};
*request.extensions_mut() = self.extensions.into_http();

request
Expand Down Expand Up @@ -412,6 +419,13 @@ fn duration_to_grpc_timeout(duration: Duration) -> String {
.expect("duration is unrealistically large")
}

/// When converting a `tonic::Request` into a `http::Request` should reserved
/// headers be removed?
pub(crate) enum SanitizeHeaders {
Yes,
No,
}

#[cfg(test)]
mod tests {
use super::*;
Expand All @@ -427,7 +441,7 @@ mod tests {
.insert(*header, MetadataValue::from_static("invalid"));
}

let http_request = r.into_http(Uri::default());
let http_request = r.into_http(Uri::default(), SanitizeHeaders::Yes);
assert!(http_request.headers().is_empty());
}

Expand Down
49 changes: 43 additions & 6 deletions tonic/src/service/interceptor.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! gRPC interceptors which are a kind of middleware.

use crate::Status;
use crate::{request::SanitizeHeaders, Status};
use pin_project::pin_project;
use std::{
fmt,
Expand Down Expand Up @@ -115,7 +115,7 @@ where
Ok(req) => {
let (metadata, extensions, _) = req.into_parts();
let req = crate::Request::from_parts(metadata, extensions, msg);
let req = req.into_http(uri);
let req = req.into_http(uri, SanitizeHeaders::No);
ResponseFuture::future(self.inner.call(req))
}
Err(status) => ResponseFuture::error(status),
Expand Down Expand Up @@ -170,14 +170,51 @@ where

fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
match self.project().kind.project() {
KindProj::Future(future) => {
let response = futures_core::ready!(future.poll(cx).map_err(Into::into)?);
Poll::Ready(Ok(response))
}
KindProj::Future(future) => future.poll(cx).map_err(Into::into),
KindProj::Error(status) => {
let error = status.take().unwrap().into();
Poll::Ready(Err(error))
}
}
}
}

#[cfg(test)]
mod tests {
#[allow(unused_imports)]
use super::*;
use tower::ServiceExt;

#[tokio::test]
async fn doesnt_remove_headers() {
let svc = tower::service_fn(|request: http::Request<hyper::Body>| async move {
assert_eq!(
request
.headers()
.get("user-agent")
.expect("missing in leaf service"),
"test-tonic"
);

Ok::<_, hyper::Error>(hyper::Response::new(hyper::Body::empty()))
});

let svc = InterceptedService::new(svc, |request: crate::Request<()>| {
assert_eq!(
request
.metadata()
.get("user-agent")
.expect("missing in interceptor"),
"test-tonic"
);
Ok(request)
});

let request = http::Request::builder()
.header("user-agent", "test-tonic")
.body(hyper::Body::empty())
.unwrap();

svc.oneshot(request).await.unwrap();
}
}

0 comments on commit 6711b80

Please sign in to comment.