From d526e1deb86f32ba455c0b56970f9d119b6725a1 Mon Sep 17 00:00:00 2001 From: Joe Dahlquist Date: Sat, 12 Feb 2022 13:01:07 -0800 Subject: [PATCH 1/4] tonic: preserve HTTP method in interceptor --- tonic/src/client/grpc.rs | 7 ++++++- tonic/src/request.rs | 13 ++++++++++--- tonic/src/service/interceptor.rs | 26 +++++++++++++++++++++++++- 3 files changed, 41 insertions(+), 5 deletions(-) diff --git a/tonic/src/client/grpc.rs b/tonic/src/client/grpc.rs index 210d44432..19c994011 100644 --- a/tonic/src/client/grpc.rs +++ b/tonic/src/client/grpc.rs @@ -248,7 +248,12 @@ impl Grpc { }) .map(BoxBody::new); - let mut request = request.into_http(uri, SanitizeHeaders::Yes); + let mut request = request.into_http( + uri, + http::Method::POST, + http::Version::HTTP_2, + SanitizeHeaders::Yes, + ); // Add the gRPC related HTTP headers request diff --git a/tonic/src/request.rs b/tonic/src/request.rs index 64dd042cf..98e8bedb4 100644 --- a/tonic/src/request.rs +++ b/tonic/src/request.rs @@ -169,12 +169,14 @@ impl Request { pub(crate) fn into_http( self, uri: http::Uri, + method: http::Method, + version: http::Version, sanitize_headers: SanitizeHeaders, ) -> http::Request { let mut request = http::Request::new(self.message); - *request.version_mut() = http::Version::HTTP_2; - *request.method_mut() = http::Method::POST; + *request.version_mut() = version; + *request.method_mut() = method; *request.uri_mut() = uri; *request.headers_mut() = match sanitize_headers { SanitizeHeaders::Yes => self.metadata.into_sanitized_headers(), @@ -441,7 +443,12 @@ mod tests { .insert(*header, MetadataValue::from_static("invalid")); } - let http_request = r.into_http(Uri::default(), SanitizeHeaders::Yes); + let http_request = r.into_http( + Uri::default(), + http::Method::POST, + http::Version::HTTP_2, + SanitizeHeaders::Yes, + ); assert!(http_request.headers().is_empty()); } diff --git a/tonic/src/service/interceptor.rs b/tonic/src/service/interceptor.rs index 441eb9163..9dec1fb28 100644 --- a/tonic/src/service/interceptor.rs +++ b/tonic/src/service/interceptor.rs @@ -157,7 +157,14 @@ where } fn call(&mut self, req: http::Request) -> Self::Future { + // It is bad practice to modify the body (i.e. Message) of the request via an interceptor. + // To avoid exposing the body of the request to the interceptor function, we first remove it + // here, allow the interceptor to modify the metadata and extensions, and then recreate the + // HTTP request with the body. Tonic requests do not preserve the URI, HTTP version, and + // HTTP method of the HTTP request, so we extract them here and then add them back in below. let uri = req.uri().clone(); + let method = req.method().clone(); + let version = req.version().clone(); let req = crate::Request::from_http(req); let (metadata, extensions, msg) = req.into_parts(); @@ -168,7 +175,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, SanitizeHeaders::No); + let req = req.into_http(uri, method, version, SanitizeHeaders::No); ResponseFuture::future(self.inner.call(req)) } Err(status) => ResponseFuture::error(status), @@ -322,4 +329,21 @@ mod tests { assert_eq!(expected.version(), response.version()); assert_eq!(expected.headers(), response.headers()); } + + async fn doesnt_change_http_method() { + let svc = tower::service_fn(|request: http::Request| async move { + assert_eq!(request.method(), http::Method::OPTIONS); + + Ok::<_, hyper::Error>(hyper::Response::new(hyper::Body::empty())) + }); + + let svc = InterceptedService::new(svc, |request: crate::Request<()>| Ok(request)); + + let request = http::Request::builder() + .method(http::Method::OPTIONS) + .body(hyper::Body::empty()) + .unwrap(); + + svc.oneshot(request).await.unwrap(); + } } From b971c545a0f0529a802ca64ca6747b6ffc9e0a4c Mon Sep 17 00:00:00 2001 From: Joe Dahlquist Date: Fri, 18 Feb 2022 07:39:15 -0800 Subject: [PATCH 2/4] tonic: fix fmt issue --- tonic/src/service/interceptor.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tonic/src/service/interceptor.rs b/tonic/src/service/interceptor.rs index 9dec1fb28..e57b7b391 100644 --- a/tonic/src/service/interceptor.rs +++ b/tonic/src/service/interceptor.rs @@ -169,7 +169,7 @@ where let (metadata, extensions, msg) = req.into_parts(); match self - .f + .fg .call(crate::Request::from_parts(metadata, extensions, ())) { Ok(req) => { From bd3cf45c5ed33322d5885adf82cef52594c684bd Mon Sep 17 00:00:00 2001 From: Joe Dahlquist Date: Sat, 19 Feb 2022 08:11:33 -0800 Subject: [PATCH 3/4] Revert "tonic: fix fmt issue" This reverts commit b971c545a0f0529a802ca64ca6747b6ffc9e0a4c. --- tonic/src/service/interceptor.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tonic/src/service/interceptor.rs b/tonic/src/service/interceptor.rs index e57b7b391..9dec1fb28 100644 --- a/tonic/src/service/interceptor.rs +++ b/tonic/src/service/interceptor.rs @@ -169,7 +169,7 @@ where let (metadata, extensions, msg) = req.into_parts(); match self - .fg + .f .call(crate::Request::from_parts(metadata, extensions, ())) { Ok(req) => { From 4954763414a5914a24a04030adaf1f0319d9c545 Mon Sep 17 00:00:00 2001 From: Joe Dahlquist Date: Sat, 19 Feb 2022 08:14:32 -0800 Subject: [PATCH 4/4] add test annotation to actually run test --- tonic/src/service/interceptor.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tonic/src/service/interceptor.rs b/tonic/src/service/interceptor.rs index 9dec1fb28..be6925365 100644 --- a/tonic/src/service/interceptor.rs +++ b/tonic/src/service/interceptor.rs @@ -330,6 +330,7 @@ mod tests { assert_eq!(expected.headers(), response.headers()); } + #[tokio::test] async fn doesnt_change_http_method() { let svc = tower::service_fn(|request: http::Request| async move { assert_eq!(request.method(), http::Method::OPTIONS);