From 849b6119a48b02e3e1ab4aba5dc739af4a86c358 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 7 Oct 2026 18:43:22 +0300 Subject: [PATCH 01/20] test(transcode): add regression tests for custom verbs after a variable A google.api.http template whose last segment is a variable followed by a custom verb panics the router at registration. The tests cover a verb after a variable, after a multi-segment field template and after a literal, each with requests that must match and must not, two bindings that differ only by verb, and a template the router cannot serve. Part of #138 --- tests/custom_verbs.rs | 252 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 252 insertions(+) create mode 100644 tests/custom_verbs.rs diff --git a/tests/custom_verbs.rs b/tests/custom_verbs.rs new file mode 100644 index 0000000..5d738b0 --- /dev/null +++ b/tests/custom_verbs.rs @@ -0,0 +1,252 @@ +//! Custom verbs (`Template = "/" Segments [ Verb ]`, google/api/http.proto): +//! a binding answers only the requests whose last segment ends in its verb, +//! after a variable, a multi-segment field template or a literal alike. +//! +//! The upstream answers with the request it received and the RPC it was +//! called as, so each test sees which binding a request reached and with which +//! path variables. Every case runs against a remote and an in-process +//! upstream. + +#[macro_use] +mod common; + +use std::convert::Infallible; +use std::future::{ready, Ready}; +use std::pin::Pin; +use std::task::{Context, Poll}; + +use axum::body::Body; +use http::{Method, StatusCode}; +use prost_reflect::{DescriptorPool, DynamicMessage, MessageDescriptor, Value as PbValue}; +use serde_json::{json, Value}; +use structured_proxy::transcode::codec::DynamicCodec; +use tower::ServiceExt; + +const OPS_PROTO: &str = r#" +syntax = "proto3"; +package test.v1; +import "google/api/annotations.proto"; + +message Msg { + string name = 1; + string operation = 2; + string rpc = 3; +} + +service Ops { + rpc Get(Msg) returns (Msg) { + option (google.api.http) = { get: "/v2/ops/{operation}" }; + } + rpc Cancel(Msg) returns (Msg) { + option (google.api.http) = { post: "/v2/ops/{operation}:cancel" }; + } + rpc Archive(Msg) returns (Msg) { + option (google.api.http) = { post: "/v2/ops/{operation}:archive" }; + } + rpc Pause(Msg) returns (Msg) { + option (google.api.http) = { post: "/v2/ops/{name}:pause" }; + } + rpc Restore(Msg) returns (Msg) { + option (google.api.http) = { post: "/v1/{name=publishers/*/books/*}:restore" }; + } + rpc Batch(Msg) returns (Msg) { + option (google.api.http) = { post: "/v1/nodes:batch" }; + } + rpc Broken(Msg) returns (Msg) { + option (google.api.http) = { get: "/v1/broken/a{name}b" }; + } +} +"#; + +/// Answers with the request, `rpc` set to the method it was called as. +#[derive(Clone)] +struct Echo { + rpc: String, +} + +impl tonic::server::UnaryService for Echo { + type Response = DynamicMessage; + type Future = Ready, tonic::Status>>; + + fn call(&mut self, request: tonic::Request) -> Self::Future { + let mut msg = request.into_inner(); + msg.set_field_by_name("rpc", PbValue::String(self.rpc.clone())); + ready(Ok(tonic::Response::new(msg))) + } +} + +#[derive(Clone)] +struct Ops { + msg: MessageDescriptor, +} + +impl tonic::server::NamedService for Ops { + const NAME: &'static str = "test.v1.Ops"; +} + +impl tower::Service> for Ops { + type Response = http::Response; + type Error = Infallible; + type Future = + Pin> + Send>>; + + fn poll_ready(&mut self, _cx: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + + fn call(&mut self, req: http::Request) -> Self::Future { + let msg = self.msg.clone(); + Box::pin(async move { + let rpc = req + .uri() + .path() + .strip_prefix("/test.v1.Ops/") + .expect("an Ops method") + .to_owned(); + let mut grpc = tonic::server::Grpc::new(DynamicCodec::new(msg)); + Ok(grpc.unary(Echo { rpc }, req).await) + }) + } +} + +async fn proxy(upstream: common::Upstream) -> common::App { + let pool: DescriptorPool = common::compile("test/v1/ops.proto", OPS_PROTO); + let msg = pool.get_message_by_name("test.v1.Msg").unwrap(); + common::proxy(upstream, Ops { msg }, pool, Default::default()).await +} + +/// Send `method uri`; returns the status, the `Allow` header and the body as +/// JSON (`null` when empty). +async fn call(app: &common::App, method: Method, uri: &str) -> (StatusCode, String, Value) { + let request = http::Request::builder() + .method(method) + .uri(uri) + .body(Body::empty()) + .unwrap(); + let resp = app.clone().oneshot(request).await.unwrap(); + let status = resp.status(); + let allow = resp + .headers() + .get(http::header::ALLOW) + .map(|v| v.to_str().unwrap().to_owned()) + .unwrap_or_default(); + let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .unwrap(); + let body = if bytes.is_empty() { + Value::Null + } else { + serde_json::from_slice(&bytes).unwrap() + }; + (status, allow, body) +} + +upstream_tests! { +async fn verb_after_a_variable_routes_with_the_variable_bound() { + // `{operation}:cancel`: the variable takes the segment without the verb. + let app = proxy(UPSTREAM).await; + let (status, _, body) = call(&app, Method::POST, "/v2/ops/op-1:cancel").await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body, json!({"name": "", "operation": "op-1", "rpc": "Cancel"})); +} + +async fn bindings_differing_only_by_verb_reach_their_own_methods() { + let app = proxy(UPSTREAM).await; + for (verb, rpc) in [("cancel", "Cancel"), ("archive", "Archive")] { + let (status, _, body) = + call(&app, Method::POST, &format!("/v2/ops/op-7:{verb}")).await; + assert_eq!(status, StatusCode::OK, "{verb}: {body}"); + assert_eq!(body["rpc"], rpc, "{verb}"); + assert_eq!(body["operation"], "op-7", "{verb}"); + } + // The same shape with another variable name binds that name. + let (status, _, body) = call(&app, Method::POST, "/v2/ops/op-7:pause").await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body, json!({"name": "op-7", "operation": "", "rpc": "Pause"})); +} + +async fn a_request_without_the_verb_does_not_reach_a_verb_binding() { + // Only the GET binding answers `/v2/ops/{operation}` without a verb. + let app = proxy(UPSTREAM).await; + let (status, allow, _) = call(&app, Method::POST, "/v2/ops/op-1").await; + assert_eq!(status, StatusCode::METHOD_NOT_ALLOWED); + assert_eq!(allow, "GET, HEAD"); + let (status, _, body) = call(&app, Method::GET, "/v2/ops/op-1").await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body["rpc"], "Get"); + assert_eq!(body["operation"], "op-1"); +} + +async fn another_verb_does_not_reach_a_verb_binding() { + let app = proxy(UPSTREAM).await; + let (status, allow, _) = call(&app, Method::POST, "/v2/ops/op-1:delete").await; + assert_eq!(status, StatusCode::METHOD_NOT_ALLOWED); + assert_eq!(allow, "GET, HEAD"); + // A binding without a verb takes the whole segment, colon included. + let (status, _, body) = call(&app, Method::GET, "/v2/ops/op-1:delete").await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body["rpc"], "Get"); + assert_eq!(body["operation"], "op-1:delete"); +} + +async fn an_encoded_colon_is_data_not_a_verb() { + // `%3A` is a colon in the value, not the verb delimiter (RFC 3986 §2.2). + let app = proxy(UPSTREAM).await; + let (status, _, _) = call(&app, Method::POST, "/v2/ops/op-1%3Acancel").await; + assert_eq!(status, StatusCode::METHOD_NOT_ALLOWED); + let (status, _, body) = call(&app, Method::GET, "/v2/ops/op-1%3Acancel").await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body["operation"], "op-1:cancel"); +} + +async fn a_verb_with_an_empty_variable_does_not_match() { + // `Variable` matches a non-empty segment: `/:cancel` binds nothing. + let app = proxy(UPSTREAM).await; + let (status, _, _) = call(&app, Method::POST, "/v2/ops/:cancel").await; + assert_eq!(status, StatusCode::METHOD_NOT_ALLOWED); +} + +async fn a_method_no_binding_of_the_url_answers_lists_those_that_do() { + // DELETE is bound nowhere; GET (without a verb) and POST (`:cancel`) both + // answer this URL. + let app = proxy(UPSTREAM).await; + let (status, allow, _) = call(&app, Method::DELETE, "/v2/ops/op-1:cancel").await; + assert_eq!(status, StatusCode::METHOD_NOT_ALLOWED); + assert_eq!(allow, "GET, POST, HEAD"); +} + +async fn verb_after_a_multi_segment_field_template_routes() { + // AIP-136: `POST /v1/{name=publishers/*/books/*}:restore`. + let app = proxy(UPSTREAM).await; + let (status, _, body) = + call(&app, Method::POST, "/v1/publishers/p1/books/b2:restore").await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body["rpc"], "Restore"); + assert_eq!(body["name"], "publishers/p1/books/b2"); + for uri in ["/v1/publishers/p1/books/b2", "/v1/publishers/p1/books/b2:archive"] { + let (status, _, body) = call(&app, Method::POST, uri).await; + assert_eq!(status, StatusCode::NOT_FOUND, "{uri}: {body}"); + } +} + +async fn verb_after_a_literal_routes_as_before() { + let app = proxy(UPSTREAM).await; + let (status, _, body) = call(&app, Method::POST, "/v1/nodes:batch").await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body["rpc"], "Batch"); + for uri in ["/v1/nodes", "/v1/nodes:other"] { + let (status, _, body) = call(&app, Method::POST, uri).await; + assert_eq!(status, StatusCode::NOT_FOUND, "{uri}: {body}"); + } +} + +async fn a_template_the_router_cannot_serve_is_skipped_not_fatal() { + // `a{name}b` puts text around a variable, which the router cannot + // match; that binding is left out and every other one still serves. + let app = proxy(UPSTREAM).await; + let (status, _, _) = call(&app, Method::GET, "/v1/broken/axb").await; + assert_eq!(status, StatusCode::NOT_FOUND); + let (status, _, body) = call(&app, Method::GET, "/v2/ops/op-1").await; + assert_eq!(status, StatusCode::OK, "{body}"); +} +} From 6ddf0dddb272104f8f04f1dd508da9e7b9708fbf Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 7 Oct 2026 19:08:42 +0300 Subject: [PATCH 02/20] fix(transcode): route custom verbs after a path variable A google.api.http template whose last segment is a variable followed by a custom verb (`/v2/ops/{operation}:cancel`, AIP-136 custom methods) panicked the router at registration: axum's matchit cannot match text after a capture, so the whole REST router failed to start. - The verb after a variable or wildcard is split off the template and matched by the transcoded router on the received (still encoded) path, so `%3A` stays data. A verb after a literal stays part of that literal. - Bindings are grouped by path shape into one route each, whatever their capture names; every binding gets its variables under its own names. This also fixes the same panic for `/v1/{a}` and `/v1/{b}` bound to different methods. - One dispatcher per route chooses the binding by method and verb before the body is read: the longest matching verb wins over a binding without one, a URL no verb matches is 404, a matching URL with another method is 405 with the answering methods in Allow. - A template the router cannot match is skipped with an error in the log, checked with the router axum uses, instead of panicking. - route_paths lists bindings that differ only by verb once, and a repeated method, path and verb as a duplicate. Closes #138 --- Cargo.toml | 4 + README.md | 20 +- src/client_address.rs | 5 +- src/openapi/tests.rs | 18 ++ src/service.rs | 8 +- src/transcode/mod.rs | 455 +++++++++++++++++------------------------ src/transcode/path.rs | 250 ++++++++++++++++++++++ src/transcode/table.rs | 194 ++++++++++++++++++ src/transcode/tests.rs | 247 ++++++++++++++++++++++ tests/custom_verbs.rs | 61 +++++- 10 files changed, 976 insertions(+), 286 deletions(-) create mode 100644 src/transcode/path.rs create mode 100644 src/transcode/table.rs diff --git a/Cargo.toml b/Cargo.toml index dac873b..6108dbf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -60,6 +60,10 @@ hyper-util = { version = "0.1", features = ["server-auto", "server-graceful", "s # in the tree through hyper-util and tonic. hyper = "1" tokio-util = { version = "0.7.16", features = ["rt"] } +# The router axum matches paths with: a transcoded template is checked with it +# before axum registers it, where a path it rejects would panic. Already in the +# tree through axum, which pins the version this resolves to. +matchit = "0.8" # gRPC client (to upstream service). `tls-connect-info` gives a rustls server # stream the connection record (client certificates) a tonic handler reads, so diff --git a/README.md b/README.md index f4baff5..5ad1d42 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,8 @@ tonic services. - REST routes from the `google.api.http` annotations in your protos: path, query and JSON or form body mapped onto the request message ([Request mapping](#request-mapping)), `response_body`, - `additional_bindings`, and `custom` rules for any HTTP method + `additional_bindings`, custom verbs (`/v1/{name}:cancel`), and `custom` + rules for any HTTP method - Server streaming as NDJSON, or Server-Sent Events when the client asks for `text/event-stream` - Errors as `google.rpc.Status` JSON with the standard HTTP status mapping and @@ -380,6 +381,23 @@ body, as the route's `google.api.http` rule says: A value that is not valid for its field, or two members of one `oneof`, is answered with `INVALID_ARGUMENT` (400) before the upstream is called. +**Custom verbs.** A path template may end in a verb, as AIP-136 custom methods +do (`post: "/v1/{name=operations/*}:cancel"`): + +- A binding with a verb answers only a request whose path ends in it, the + variable taking the rest of the segment. A percent-encoded colon (`%3A`) is + part of the value, never the verb's delimiter. +- Bindings of one path that differ only by verb reach their own RPCs. A + binding without a verb takes the whole last segment, colon included, when + no verb of the path ends the request. +- A request that matches the path but none of its verbs is `404`; one whose + verb matches but whose method does not is `405`, with the methods that + answer it in `Allow`. + +A template the router cannot match, such as text around a variable within one +segment (`/v1/a{name}b`), is left out with an error in the log; the other +routes still serve. + ## Client address The proxy works out who the client is once per request, before any guard diff --git a/src/client_address.rs b/src/client_address.rs index 270c22f..a8cbc4a 100644 --- a/src/client_address.rs +++ b/src/client_address.rs @@ -358,8 +358,9 @@ impl Resolver { self.resolve_noting(peer, headers, None) } - /// [`resolve`](Self::resolve), noting in `hops` the trusted proxies - /// between the client and this proxy, nearest first. + /// The client address of a request from `peer` carrying `headers` (what + /// [`apply`](Self::apply) puts on it), noting in `hops` the trusted + /// proxies between the client and this proxy, nearest first. fn resolve_noting( &self, peer: Option, diff --git a/src/openapi/tests.rs b/src/openapi/tests.rs index 0f0eb74..4c433bb 100644 --- a/src/openapi/tests.rs +++ b/src/openapi/tests.rs @@ -184,6 +184,9 @@ service Api { rpc Raw(google.api.HttpBody) returns (google.api.HttpBody) { option (google.api.http) = { post: "/v1/raw" body: "*" }; } + rpc Archive(Item) returns (Item) { + option (google.api.http) = { post: "/v1/items/{name}:archive" }; + } } "#; @@ -224,6 +227,21 @@ fn custom_head_rule_is_a_head_operation() { assert_eq!(item["head"]["parameters"][0]["in"], "path"); } +#[test] +fn custom_verb_after_a_variable_keeps_its_path_and_parameter() { + // OpenAPI path templating allows a template expression inside a segment, + // so the verb stays in the path and the variable is a path parameter. + let spec = spec(&[]); + let archive = &spec["paths"]["/v1/items/{name}:archive"]["post"]; + assert_eq!(archive["operationId"], "Api.Archive"); + let params = archive["parameters"].as_array().unwrap(); + let path_params: Vec<&Value> = params.iter().filter(|p| p["in"] == "path").collect(); + assert_eq!(path_params.len(), 1); + assert_eq!(path_params[0]["name"], "name"); + // No path item without the verb comes from this binding. + assert!(spec["paths"]["/v1/items/{name}"].get("post").is_none()); +} + #[test] fn star_rule_is_listed_under_every_method_with_distinct_ids() { let spec = spec(&[]); diff --git a/src/service.rs b/src/service.rs index 3690311..fc45365 100644 --- a/src/service.rs +++ b/src/service.rs @@ -48,7 +48,7 @@ use crate::upstream::{ /// only the guards whose scope names them (`grpc`, `fallback`); a guard's /// rejection of a gRPC call is a gRPC status. /// -/// Serve it with [`serve`](crate::serve) or +/// Serve it with [`serve`](crate::serve()) or /// [`serve_with`](crate::serve_with) (TLS, a connection limit), or hand it to /// any server that takes a tower service of `http` types: a Unix socket, an /// existing hyper or axum server. Native gRPC needs HTTP/2 on that server @@ -339,8 +339,10 @@ impl ProxyService { /// them; CORS and tracing are the fallback's own. /// /// A request whose path a route answers but not with its method stays with - /// the proxy (`405`), as does every gRPC request and every browser - /// preflight for a gRPC-Web call, which follows the call it announces. + /// the proxy (`405`), as does one whose path matches a transcoded template + /// but none of its custom verbs (`404`), every gRPC request and every + /// browser preflight for a gRPC-Web call, which follows the call it + /// announces. #[must_use] pub fn with_fallback(mut self, fallback: F) -> Self where diff --git a/src/transcode/mod.rs b/src/transcode/mod.rs index c6d2f1f..daab579 100644 --- a/src/transcode/mod.rs +++ b/src/transcode/mod.rs @@ -14,21 +14,22 @@ pub mod codec; pub mod error; pub(crate) mod httpbody; pub mod metadata; +mod path; pub mod request; pub(crate) mod response; pub(crate) mod rule; mod select; +mod table; +pub use path::proto_path_to_axum; pub use select::RpcSelection; use axum::body::{Body, Bytes}; -use axum::extract::{OriginalUri, Path, RawQuery, State}; +use axum::extract::{FromRequest, FromRequestParts, OriginalUri, Path, Request, State}; use axum::http::header::{ALLOW, CONTENT_TYPE}; use axum::http::{HeaderMap, HeaderName, HeaderValue, Method, StatusCode, Uri}; use axum::response::sse::{Event, KeepAlive, Sse}; use axum::response::{IntoResponse, Response}; -use axum::routing::{MethodFilter, MethodRouter}; -use axum::Extension; use axum::Router; use futures::{StreamExt, TryStreamExt}; use prost_reflect::{ @@ -47,8 +48,10 @@ use crate::received::ReceivedRequest; use crate::service::ConnectionInfo; use crate::upstream::Upstream; use error::{ErrorDetailsPolicy, StatusDetails}; +use path::MountedPath; use response::UpstreamHeaders; use rule::RouteMethod; +use table::{Choice, PathTable}; /// How long the upstream may take to answer a call with its response headers /// when the client's `grpc-timeout` asks for no shorter deadline. @@ -235,34 +238,69 @@ pub fn routes(pool: &DescriptorPool, aliases: &[AliasConfig]) /// [`routes`], built as `options` describe. /// -/// A second binding for a method and path already taken (or any binding on a -/// path a `custom` `*` rule takes, which answers every method) is skipped with -/// an error in the log. +/// A binding answers the requests whose path matches its template, custom verb +/// included (`/v1/{name}:cancel`, google/api/http.proto `Verb`), and whose +/// method is its own; a GET binding answers HEAD too. Among the bindings whose +/// verb a request's path ends in, the one with the longest verb serves it. A +/// path that bindings answer, but none with the request's method, is `405` +/// with their methods in `Allow`; a path that matches a template but no verb +/// of it is `404`. +/// +/// A second binding for a method, path and verb already taken (or any binding +/// on a path and verb a `custom` `*` rule takes, which answers every method), +/// and a template the router cannot match (text around a variable within one +/// segment, a path that does not start with `/`), are skipped with an error in +/// the log. pub fn routes_with_options( pool: &DescriptorPool, aliases: &[AliasConfig], options: &TranscodeOptions, ) -> Router { - let bindings = route_bindings(pool, aliases, &options.selection); - if bindings.is_empty() { + let tables = path_tables(pool, aliases, options); + if tables.is_empty() { tracing::warn!("No HTTP-annotated RPCs found in proto descriptors"); return Router::new(); } + let mut router: Router = Router::new(); + for table in tables { + let path = table.path.clone(); + let table = Arc::new(table); + // Every method goes to the table, which answers 405 itself: the URL, + // not the path alone, decides which methods a path answers. + router = router.route( + &path, + axum::routing::any(move |State(state): State, request: Request| { + dispatch(table, state, request) + }), + ); + } + router +} + +/// The tables [`routes_with_options`] mounts, one per router path, each +/// holding every binding of that path in binding order. +fn path_tables( + pool: &DescriptorPool, + aliases: &[AliasConfig], + options: &TranscodeOptions, +) -> Vec { + let bindings = route_bindings(pool, aliases, &options.selection); tracing::info!("Registering {} transcoded REST→gRPC routes", bindings.len()); // At most two renderers, without and with the opaque-detail extension, // each shared by every route that uses it and built only when one does. // The second is a copy of the first: the descriptor pool inside is shared. let mut status_details: [Option>; 2] = [None, None]; - // Every binding of one path goes into the same method router, in binding - // order. - let mut paths: Vec = Vec::new(); - let mut path_index: HashMap = HashMap::new(); + // The router tells paths apart by shape only, so every binding of one + // shape goes into one table, whatever its capture names. + let mut tables: Vec = Vec::new(); + let mut by_shape: HashMap = HashMap::new(); for mut binding in bindings { let policy = &options.error_details; - if policy.enabled_for(&binding.axum_path) { - let opaque = policy.opaque_for(&binding.axum_path); + let route = binding.mount.display(); + if policy.enabled_for(&route) { + let opaque = policy.opaque_for(&route); let slot = usize::from(opaque); if status_details[slot].is_none() { let base = status_details @@ -277,163 +315,136 @@ pub fn routes_with_options( } binding.entry.ndjson_envelope = options.ndjson_envelope; binding.entry.denied_headers = options.denied_response_headers.clone(); - let index = match path_index.get(&binding.axum_path) { - Some(&index) => index, + match by_shape.get(&binding.mount.shape) { + Some(&index) => tables[index].add(binding.mount, binding.entry), None => { - path_index.insert(binding.axum_path.clone(), paths.len()); - paths.push(PathRoutes { - path: binding.axum_path, - methods: Vec::new(), - }); - paths.len() - 1 + by_shape.insert(binding.mount.shape.clone(), tables.len()); + tables.push(PathTable::new(binding.mount, binding.entry)); } - }; - paths[index].add(Arc::new(binding.entry)); + } } - let mut router: Router = Router::new(); - for path in &paths { - router = router.route(&path.path, path.method_router()); - } - router + // A path the router refuses next to the ones before it is left out, as + // axum would panic on it. matchit is the router axum matches with. + let mut router = matchit::Router::new(); + tables.retain(|table| match router.insert(table.path.as_str(), ()) { + Ok(()) => true, + Err(error) => { + tracing::error!( + path = %table.path, + %error, + "transcoded route conflicts with another; skipping its bindings" + ); + false + } + }); + tables } -/// The axum handler serving the route entry `$entry`, for the state type `S` -/// in scope. A macro because the closure's handler type cannot be named. -macro_rules! endpoint { - ($entry:expr) => {{ - let entry: Arc = $entry; - move |state: State, - method: Method, - OriginalUri(uri): OriginalUri, - headers: HeaderMap, - path_params: Path, - raw_query: RawQuery, - connection: Option>, - address: Option>, - body: Bytes| { - let client = Client { - headers, - origin: Origin { - connection: connection.map(|Extension(connection)| connection), - address: address.map(|Extension(address)| address), - }, - line: RequestLine { method, uri }, - }; - handle(state, client, path_params, raw_query, body, entry) +/// Serve `request` on `table`'s path: choose the binding before anything is +/// extracted, so a request no binding answers gets its 404 or 405 without its +/// body being read. +async fn dispatch( + table: Arc, + state: S, + request: Request, +) -> Response { + let (mut parts, body) = request.into_parts(); + let index = match table.choose(&parts.method, parts.uri.path()) { + Choice::Route(index) => index, + Choice::MethodNotAllowed(allow) => { + return (StatusCode::METHOD_NOT_ALLOWED, [(ALLOW, allow)]).into_response() } - }}; + Choice::NotFound => return StatusCode::NOT_FOUND.into_response(), + }; + let mut path_params = match Path::::from_request_parts(&mut parts, &state).await { + Ok(Path(params)) => params, + Err(rejection) => return rejection.into_response(), + }; + table.bind_params(index, &mut path_params); + // The target as received, before a router the proxy is nested in strips + // its prefix; the router records it on every request it routes. + let uri = match parts.extensions.remove::() { + Some(OriginalUri(uri)) => uri, + None => parts.uri.clone(), + }; + // Taken rather than copied: reading the body needs only its limit, which + // stays in the extensions. + let client = Client { + headers: std::mem::take(&mut parts.headers), + origin: Origin { + connection: parts.extensions.remove::(), + address: parts.extensions.remove::(), + }, + line: RequestLine { + method: std::mem::replace(&mut parts.method, Method::GET), + uri, + }, + }; + let body = match Bytes::from_request(Request::from_parts(parts, body), &state).await { + Ok(body) => body, + Err(rejection) => return rejection.into_response(), + }; + handle(state, client, path_params, body, RouteRef { table, index }).await } -/// The bindings mounted at one axum path. -struct PathRoutes { - path: String, - methods: Vec>, +/// The binding serving a request, held through its path's table: the table +/// is already shared with the request, so naming the binding takes no +/// refcount of its own. +#[derive(Clone)] +struct RouteRef { + table: Arc, + index: usize, } -impl PathRoutes { - /// Add `entry` unless its method is already answered on this path. - fn add(&mut self, entry: Arc) { - let taken = self.methods.iter().any(|existing| { - existing.http_method == entry.http_method - || existing.http_method == RouteMethod::Any - || entry.http_method == RouteMethod::Any - }); - if taken { - tracing::error!( - method = entry.http_method.as_str(), - path = %self.path, - rpc = %entry.grpc_path, - "HTTP method and path already bound to another RPC; skipping this binding" - ); - return; - } - self.methods.push(entry); - } +impl std::ops::Deref for RouteRef { + type Target = RouteEntry; - /// One method router for every binding of the path. Methods axum routes by - /// itself are registered directly; any other token (a `custom` rule such - /// as `PROPFIND`) is dispatched by a fallback that answers `405` with the - /// full `Allow` list (RFC 9110 §15.5.6) for a method nobody binds. - fn method_router(&self) -> MethodRouter { - let mut router = MethodRouter::new(); - let mut extension: Vec<(Method, Arc)> = Vec::new(); - let mut allow: Vec<&str> = Vec::new(); - for entry in &self.methods { - match &entry.http_method { - // `add` keeps a `*` binding alone on its path. - RouteMethod::Any => return axum::routing::any(endpoint!(entry.clone())), - RouteMethod::One(method) => { - allow.push(method.as_str()); - match MethodFilter::try_from(method.clone()) { - Ok(filter) => router = router.on(filter, endpoint!(entry.clone())), - Err(_) => extension.push((method.clone(), entry.clone())), - } - } - } - } - if extension.is_empty() { - return router; - } - // A GET route answers HEAD too. - if allow.contains(&"GET") && !allow.contains(&"HEAD") { - allow.push("HEAD"); - } - let allow = HeaderValue::from_str(&allow.join(", ")) - .expect("method tokens are valid header value characters"); - let extension: Arc<[(Method, Arc)]> = extension.into(); - // The method decides before anything is extracted, so a request no - // binding answers gets its 405 without its body being read. - router.fallback( - move |State(state): State, request: axum::extract::Request| async move { - match extension - .iter() - .find(|(bound, _)| bound == request.method()) - { - Some((_, entry)) => { - axum::handler::Handler::call(endpoint!(entry.clone()), request, state).await - } - None => (StatusCode::METHOD_NOT_ALLOWED, [(ALLOW, allow)]).into_response(), - } - }, - ) + fn deref(&self) -> &RouteEntry { + self.table.entry(self.index) } } -/// One transcode route to mount: the RPC entry that serves it and the axum path -/// to register it at. +/// One transcode route to mount: the RPC entry that serves it and where it is +/// mounted. struct RouteBinding { entry: RouteEntry, - axum_path: String, + mount: MountedPath, } /// The single source of truth for what [`routes`] mounts: every binding of -/// every unary and server-streaming RPC, plus its config aliases. Both -/// [`routes`] (to build handlers) and [`route_paths`] (to enumerate paths for -/// collision checks) consume this, so the mounted set and the enumerated set -/// cannot drift apart. +/// every unary and server-streaming RPC, plus its config aliases, less the +/// templates the router cannot match. Both [`routes`] (to build handlers) and +/// [`route_paths`] (to enumerate paths for collision checks) consume this, so +/// the mounted set and the enumerated set cannot drift apart. fn route_bindings( pool: &DescriptorPool, aliases: &[AliasConfig], selection: &RpcSelection, ) -> Vec { let mut bindings = Vec::new(); + let mut push = |mount: MountedPath, entry: RouteEntry| match path::mountable(&mount.axum) { + Ok(()) => bindings.push(RouteBinding { entry, mount }), + Err(error) => tracing::error!( + path = %mount.display(), + rpc = %entry.grpc_path, + %error, + "google.api.http path template cannot be routed; skipping this binding" + ), + }; for entry in extract_routes(pool, selection) { for alias in aliases { if let Some(suffix) = entry.http_path.strip_prefix(&alias.to) { if alias.from.ends_with("/{path}") { let prefix = alias.from.trim_end_matches("/{path}"); - bindings.push(RouteBinding { - axum_path: proto_path_to_axum(&format!("{prefix}{suffix}")), - entry: entry.clone(), - }); + push( + MountedPath::new(&format!("{prefix}{suffix}")), + entry.clone(), + ); } } } - bindings.push(RouteBinding { - axum_path: proto_path_to_axum(&entry.http_path), - entry, - }); + push(MountedPath::new(&entry.http_path), entry); } bindings } @@ -448,16 +459,28 @@ fn route_bindings( /// /// Each entry is `(method, path)` where `method` is the uppercase HTTP token, /// or `*` for a `custom` rule that answers every method, so callers can -/// distinguish same-path/different-method routes from real conflicts. +/// distinguish same-path/different-method routes from real conflicts. A +/// custom verb after a variable is matched by the transcoded routes, not by +/// the path: bindings of one method that differ only by such a verb share one +/// route and are listed once, while a repeated method, path and verb is listed +/// as often as it is bound. pub fn route_paths( pool: &DescriptorPool, aliases: &[AliasConfig], selection: &RpcSelection, ) -> Vec<(String, String)> { - route_bindings(pool, aliases, selection) - .into_iter() - .map(|b| (b.entry.http_method.as_str().to_string(), b.axum_path)) - .collect() + // The verbs bound so far under each (method, route shape). + let mut verbs: HashMap<(String, String), Vec>> = HashMap::new(); + let mut paths = Vec::new(); + for RouteBinding { entry, mount } in route_bindings(pool, aliases, selection) { + let method = entry.http_method.as_str().to_owned(); + let bound = verbs.entry((method.clone(), mount.shape)).or_default(); + if bound.is_empty() || bound.contains(&mount.verb) { + paths.push((method, mount.axum)); + } + bound.push(mount.verb); + } + paths } /// JSON serialization options shared by the unary and streaming response paths, @@ -603,12 +626,11 @@ struct Origin { /// Serve one request on a transcoded route. async fn handle( - State(proxy_state): State, + proxy_state: S, client: Client, - Path(path_params): Path, - RawQuery(raw_query): RawQuery, + path_params: PathParams, body: Bytes, - entry: Arc, + entry: RouteRef, ) -> Response { let keep_alive_secs = proxy_state.sse_keep_alive_secs(); let Client { @@ -616,18 +638,20 @@ async fn handle( origin, line, } = client; - let prepared = line.received(&entry).and_then(|received| { - let mut call = prepare( - proxy_state, - &headers, - origin, - &path_params, - raw_query.as_deref(), - body, - &entry, - )?; + // The query is read where it was received, before the request line is + // handed on. + let prepared = prepare( + proxy_state, + &headers, + origin, + &path_params, + line.uri.query(), + body, + &entry, + ) + .and_then(|mut call| { // Next to the extensions `prepare` sets, for an upstream in process. - call.request.extensions_mut().insert(received); + call.request.extensions_mut().insert(line.received(&entry)?); Ok(call) }); let call = match prepared { @@ -905,7 +929,7 @@ fn upstream_error(mut status: tonic::Status, entry: &RouteEntry) -> Response { /// sent and are not forwarded. async fn streaming_call( call: Call, - entry: Arc, + entry: RouteRef, use_sse: bool, keep_alive_secs: u64, ) -> Response { @@ -946,7 +970,7 @@ async fn streaming_call( /// and the client sees a truncated transfer instead of a clean end. async fn http_body_stream( mut stream: tonic::Streaming, - entry: Arc, + entry: RouteRef, initial: MetadataMap, ) -> Response { let first = match stream.message().await { @@ -1271,118 +1295,5 @@ fn response_shape(output: &MessageDescriptor, response_body: Option) -> } } -/// Convert a `google.api.http` path template to axum 0.8 path syntax. -/// -/// The proto `{param}` form IS axum 0.8's native capture syntax, so plain -/// single-segment params pass through verbatim. Only field-path templates and -/// bare wildcards need rewriting (axum 0.7 used `:param`; 0.8 uses `{param}` -/// and rejects any segment starting with `:`): -/// - `{name=*}` (single segment) -> `{name}` -/// - `{name=**}` (multi-segment) -> `{*name}` (axum catch-all) -/// - bare `*` segment -> `{wildcardN}` -/// - bare `**` segment -> `{*wildcardN}` (axum catch-all) -pub fn proto_path_to_axum(path: &str) -> String { - let mut out = String::with_capacity(path.len()); - - let segments = split_top_level(path); - let last = segments.len().saturating_sub(1); - for (idx, segment) in segments.iter().enumerate() { - if idx > 0 { - out.push('/'); - } - out.push_str(&convert_segment(segment, idx, idx == last)); - } - - out -} - -/// Split a path on `/` boundaries that are NOT inside a `{...}` brace span. -/// -/// google.api.http field templates can embed slashes inside a single capture -/// (e.g. the AIP-127 resource name `{name=shelves/*/books/*}`), so a naive -/// `str::split('/')` would fracture the brace span into invalid fragments. -/// Tracking brace depth keeps each capture intact. -fn split_top_level(path: &str) -> Vec<&str> { - let mut segments = Vec::new(); - let mut depth = 0usize; - let mut start = 0usize; - - for (i, ch) in path.char_indices() { - match ch { - '{' => depth += 1, - // Decrement only on a matched brace; a stray `}` (malformed input) - // is treated as a literal rather than driving depth negative. - '}' if depth > 0 => depth -= 1, - '/' if depth == 0 => { - segments.push(&path[start..i]); - start = i + 1; - } - _ => {} - } - } - segments.push(&path[start..]); - segments -} - -/// Convert a single top-level path segment from proto template to axum 0.8 form. -/// -/// `is_last` indicates the terminal segment: axum permits a catch-all capture -/// (`{*name}`) only there, so catch-alls in any other position must degrade. -fn convert_segment(segment: &str, idx: usize, is_last: bool) -> String { - if let Some(inner) = segment.strip_prefix('{').and_then(|s| s.strip_suffix('}')) { - // Brace capture, possibly with a `name=template` field path. - if let Some((name, template)) = inner.split_once('=') { - return match template { - // Single-segment field path collapses to a plain capture. - "*" => format!("{{{name}}}"), - // Multi-segment catch-all maps to axum's `{*name}` (terminal only). - "**" => catch_all(name, is_last), - // Templates with interspersed literals (`{name=shelves/*/books/*}`) - // have no faithful axum form: axum cannot bind literal segments - // into one capture. Collapse to a catch-all so routing stays - // deterministic and the field still binds to the matched tail, - // and warn so the limitation surfaces instead of mis-routing. - _ => { - tracing::warn!( - template = %inner, - "google.api.http multi-segment field template is not fully \ - supported; routing it as a catch-all capture" - ); - catch_all(name, is_last) - } - }; - } - // Plain `{name}` is already valid axum 0.8 syntax. - return format!("{{{inner}}}"); - } - - // Bare wildcards: name them by position so multiple wildcards never collide. - match segment { - "**" => catch_all(&format!("wildcard{idx}"), is_last), - "*" => format!("{{wildcard{idx}}}"), - literal => literal.to_string(), - } -} - -/// Emit an axum catch-all `{*name}` when `is_last`, else degrade to a -/// single-segment `{name}` capture. -/// -/// axum accepts a catch-all only in the final path segment; a mid-path -/// `{*name}` is rejected at `Router::route()`. A non-terminal catch-all comes -/// from a malformed or unsupported google.api.http template, so we degrade -/// (capturing one segment) and warn rather than panic the whole router. -fn catch_all(name: &str, is_last: bool) -> String { - if is_last { - format!("{{*{name}}}") - } else { - tracing::warn!( - capture = %name, - "catch-all in a non-terminal path segment is unrepresentable in axum; \ - degrading to a single-segment capture" - ); - format!("{{{name}}}") - } -} - #[cfg(test)] mod tests; diff --git a/src/transcode/path.rs b/src/transcode/path.rs new file mode 100644 index 0000000..00461e6 --- /dev/null +++ b/src/transcode/path.rs @@ -0,0 +1,250 @@ +//! `google.api.http` path templates in the form the router mounts them. +//! +//! The template grammar (google/api/http.proto) is +//! `Template = "/" Segments [ Verb ]` with `Verb = ":" LITERAL`. axum (matchit) +//! matches a literal after a variable in no segment, so a verb that follows a +//! variable or a wildcard is split off here and matched by the transcoded +//! router itself; a verb after a literal stays part of that literal. + +use std::borrow::Cow; + +/// Where one binding is mounted. +#[derive(Debug, Clone)] +pub(crate) struct MountedPath { + /// The axum path (`/v1/{name}`), without a verb that follows a variable. + pub(crate) axum: String, + /// `axum` with every capture unnamed: the bindings that share it share one + /// route, since the router tells their paths apart by shape only. + pub(crate) shape: String, + /// The capture names of `axum`, in path order. + pub(crate) captures: Vec, + /// The custom verb after the last segment's variable, `:` included. + pub(crate) verb: Option, +} + +impl MountedPath { + /// Mount `template`, a `google.api.http` path template. + pub(crate) fn new(template: &str) -> Self { + let (base, verb) = split_verb(template); + let segments = split_top_level(base); + let last = segments.len() - 1; + let mut axum = String::with_capacity(base.len()); + let mut shape = String::with_capacity(base.len()); + let mut captures = Vec::new(); + for (idx, segment) in segments.iter().enumerate() { + if idx > 0 { + axum.push('/'); + shape.push('/'); + } + match convert_segment(segment, idx, idx == last) { + Segment::Literal(literal) => { + axum.push_str(literal); + shape.push_str(literal); + } + Segment::Capture { name, catch_all } => { + let open = if catch_all { "{*" } else { "{" }; + axum.push_str(open); + axum.push_str(&name); + axum.push('}'); + shape.push_str(open); + shape.push('}'); + captures.push(name); + } + } + } + Self { + axum, + shape, + captures, + verb: verb.map(str::to_owned), + } + } + + /// The route as written, verb included: what route policies match and + /// what the log names. + pub(crate) fn display(&self) -> Cow<'_, str> { + match &self.verb { + None => Cow::Borrowed(&self.axum), + Some(verb) => Cow::Owned(format!("{}{verb}", self.axum)), + } + } +} + +/// Convert a `google.api.http` path template to axum 0.8 path syntax. +/// +/// The proto `{param}` form IS axum 0.8's native capture syntax, so plain +/// single-segment params pass through verbatim. Only field-path templates and +/// bare wildcards need rewriting (axum 0.7 used `:param`; 0.8 uses `{param}` +/// and rejects any segment starting with `:`): +/// - `{name=*}` (single segment) -> `{name}` +/// - `{name=**}` (multi-segment) -> `{*name}` (axum catch-all) +/// - bare `*` segment -> `{wildcardN}` +/// - bare `**` segment -> `{*wildcardN}` (axum catch-all) +/// +/// A custom verb after a variable or wildcard (`{name}:cancel`) is not part of +/// the result: axum cannot match it, so the transcoded router checks it +/// itself. A verb after a literal (`/v1/nodes:batch`) stays in that literal. +pub fn proto_path_to_axum(path: &str) -> String { + MountedPath::new(path).axum +} + +/// Whether axum can register `axum_path` on its own: what `Router::route` +/// checks before it inserts, then the insertion into an empty matchit router, +/// the router axum matches with. A path that fails would panic at +/// `Router::route`. Conflicts between paths are a separate check. +pub(crate) fn mountable(axum_path: &str) -> Result<(), String> { + if !axum_path.starts_with('/') { + return Err("a path template must start with '/'".to_string()); + } + if axum_path + .split('/') + .any(|segment| segment.starts_with(':') || segment.starts_with('*')) + { + return Err("a path segment must not start with ':' or '*'".to_string()); + } + matchit::Router::new() + .insert(axum_path, ()) + .map_err(|e| e.to_string()) +} + +/// Split the custom verb off `template` when its last segment is a variable +/// or a wildcard: `/v1/{name}:cancel` is `/v1/{name}` and `:cancel`. An empty +/// verb is not one (`Verb = ":" LITERAL`), and anything else after a variable +/// stays in the template for the router to reject. +fn split_verb(template: &str) -> (&str, Option<&str>) { + let segments = split_top_level(template); + let last = segments[segments.len() - 1]; + let start = template.len() - last.len(); + let colon = if last.starts_with('{') { + closing_brace(last).map(|close| close + 1) + } else if last.starts_with("**") { + Some(2) + } else if last.starts_with('*') { + Some(1) + } else { + None + }; + match colon { + Some(colon) if last[colon..].starts_with(':') && last.len() > colon + 1 => { + let at = start + colon; + (&template[..at], Some(&template[at..])) + } + _ => (template, None), + } +} + +/// The index of the `}` closing the `{` that opens `segment`. +fn closing_brace(segment: &str) -> Option { + let mut depth = 0usize; + for (i, ch) in segment.char_indices() { + match ch { + '{' => depth += 1, + '}' => { + depth = depth.checked_sub(1)?; + if depth == 0 { + return Some(i); + } + } + _ => {} + } + } + None +} + +/// Split a path on `/` boundaries that are NOT inside a `{...}` brace span. +/// +/// google.api.http field templates can embed slashes inside a single capture +/// (e.g. the AIP-127 resource name `{name=shelves/*/books/*}`), so a naive +/// `str::split('/')` would fracture the brace span into invalid fragments. +/// Tracking brace depth keeps each capture intact. Never empty. +fn split_top_level(path: &str) -> Vec<&str> { + let mut segments = Vec::new(); + let mut depth = 0usize; + let mut start = 0usize; + + for (i, ch) in path.char_indices() { + match ch { + '{' => depth += 1, + // Decrement only on a matched brace; a stray `}` (malformed input) + // is treated as a literal rather than driving depth negative. + '}' if depth > 0 => depth -= 1, + '/' if depth == 0 => { + segments.push(&path[start..i]); + start = i + 1; + } + _ => {} + } + } + segments.push(&path[start..]); + segments +} + +/// One top-level path segment in axum form. +enum Segment<'a> { + Literal(&'a str), + Capture { name: String, catch_all: bool }, +} + +/// Convert a single top-level path segment from proto template to axum 0.8 form. +/// +/// `is_last` indicates the terminal segment: axum permits a catch-all capture +/// (`{*name}`) only there, so catch-alls in any other position must degrade. +fn convert_segment(segment: &str, idx: usize, is_last: bool) -> Segment<'_> { + if let Some(inner) = segment.strip_prefix('{').and_then(|s| s.strip_suffix('}')) { + // Brace capture, possibly with a `name=template` field path. + if let Some((name, template)) = inner.split_once('=') { + return match template { + // Single-segment field path collapses to a plain capture. + "*" => capture(name, false), + // Multi-segment catch-all maps to axum's `{*name}` (terminal only). + "**" => catch_all(name, is_last), + // Templates with interspersed literals (`{name=shelves/*/books/*}`) + // have no faithful axum form: axum cannot bind literal segments + // into one capture. Collapse to a catch-all so routing stays + // deterministic and the field still binds to the matched tail, + // and warn so the limitation surfaces instead of mis-routing. + _ => { + tracing::warn!( + template = %inner, + "google.api.http multi-segment field template is not fully \ + supported; routing it as a catch-all capture" + ); + catch_all(name, is_last) + } + }; + } + // Plain `{name}` is already valid axum 0.8 syntax. + return capture(inner, false); + } + + // Bare wildcards: name them by position so multiple wildcards never collide. + match segment { + "**" => catch_all(&format!("wildcard{idx}"), is_last), + "*" => capture(&format!("wildcard{idx}"), false), + literal => Segment::Literal(literal), + } +} + +fn capture(name: &str, catch_all: bool) -> Segment<'static> { + Segment::Capture { + name: name.to_owned(), + catch_all, + } +} + +/// A catch-all capture when `is_last`, else a single-segment one. +/// +/// axum accepts a catch-all only in the final path segment; a mid-path +/// `{*name}` is rejected at `Router::route()`. A non-terminal catch-all comes +/// from a malformed or unsupported google.api.http template, so we degrade +/// (capturing one segment) and warn rather than panic the whole router. +fn catch_all(name: &str, is_last: bool) -> Segment<'static> { + if !is_last { + tracing::warn!( + capture = %name, + "catch-all in a non-terminal path segment is unrepresentable in axum; \ + degrading to a single-segment capture" + ); + } + capture(name, is_last) +} diff --git a/src/transcode/table.rs b/src/transcode/table.rs new file mode 100644 index 0000000..941681e --- /dev/null +++ b/src/transcode/table.rs @@ -0,0 +1,194 @@ +//! The bindings mounted at one router path, and the choice among them for a +//! request: by its method and by the custom verb its path ends in. + +use axum::http::{HeaderValue, Method}; + +use super::path::MountedPath; +use super::rule::RouteMethod; +use super::{PathParams, RouteEntry}; + +/// Every binding the router serves at one path. +pub(super) struct PathTable { + /// The router path, written with the first binding's capture names. + pub(super) path: String, + /// The capture names of `path`, in path order. + captures: Vec, + bindings: Vec, +} + +/// One binding of a [`PathTable`]. +struct Binding { + entry: RouteEntry, + /// This binding's names for the captures of the table's path, when they + /// differ from the path's own. + names: Option>, + /// The custom verb after the last capture, `:` included. + verb: Option, +} + +/// What answers a request on a table's path. +pub(super) enum Choice { + /// The binding at this index. + Route(usize), + /// Bindings answer the URL, none with the request's method: the value of + /// `Allow` lists their methods (RFC 9110 §15.5.6). + MethodNotAllowed(HeaderValue), + /// No binding answers the URL: its verb is none of theirs. + NotFound, +} + +impl Binding { + /// Whether this binding answers a request for `path` (as received, still + /// percent-encoded): with a verb, only a path whose last segment ends in + /// it after at least one character of the variable. An encoded colon + /// (`%3A`) is data, never the verb's delimiter (RFC 3986 §2.2). + fn answers(&self, path: &str) -> bool { + match &self.verb { + None => true, + Some(verb) => path + .strip_suffix(verb.as_str()) + .is_some_and(|rest| !rest.is_empty() && !rest.ends_with('/')), + } + } + + fn verb_len(&self) -> usize { + self.verb.as_ref().map_or(0, String::len) + } +} + +impl PathTable { + /// A table for `mount`, served first by `entry`. + pub(super) fn new(mount: MountedPath, entry: RouteEntry) -> Self { + let MountedPath { + axum, + captures, + verb, + .. + } = mount; + Self { + path: axum, + captures, + bindings: vec![Binding { + entry, + names: None, + verb, + }], + } + } + + /// Add `entry` mounted at `mount`, a path of this table's shape, unless a + /// binding with its verb already answers its method (a `custom` `*` rule + /// answers every one). + pub(super) fn add(&mut self, mount: MountedPath, entry: RouteEntry) { + let taken = self.bindings.iter().any(|existing| { + existing.verb == mount.verb + && (existing.entry.http_method == entry.http_method + || existing.entry.http_method == RouteMethod::Any + || entry.http_method == RouteMethod::Any) + }); + if taken { + tracing::error!( + method = entry.http_method.as_str(), + path = %mount.display(), + rpc = %entry.grpc_path, + "HTTP method and path already bound to another RPC; skipping this binding" + ); + return; + } + let names = (mount.captures != self.captures).then_some(mount.captures); + self.bindings.push(Binding { + entry, + names, + verb: mount.verb, + }); + } + + /// The route entry of the binding at `index`. + pub(super) fn entry(&self, index: usize) -> &RouteEntry { + &self.bindings[index].entry + } + + /// Which binding answers `method` on `path`. Among the bindings whose + /// verb the path ends in, the longest verb wins, so a binding without one + /// takes only what no verb claims; within that, the request's own method + /// (or a `*` rule) before a GET binding answering HEAD. + pub(super) fn choose(&self, method: &Method, path: &str) -> Choice { + // (index, verb length, own method) of the best binding so far. + let mut best: Option<(usize, usize, bool)> = None; + let mut answered = false; + for (index, binding) in self.bindings.iter().enumerate() { + if !binding.answers(path) { + continue; + } + answered = true; + let own = match &binding.entry.http_method { + RouteMethod::Any => true, + RouteMethod::One(bound) if bound == method => true, + // A GET binding answers HEAD too. + RouteMethod::One(bound) if *bound == Method::GET && *method == Method::HEAD => { + false + } + RouteMethod::One(_) => continue, + }; + let rank = (binding.verb_len(), own); + if best.is_none_or(|(_, len, best_own)| (len, best_own) < rank) { + best = Some((index, rank.0, rank.1)); + } + } + match best { + Some((index, ..)) => Choice::Route(index), + None if answered => Choice::MethodNotAllowed(self.allow(path)), + None => Choice::NotFound, + } + } + + /// The methods of the bindings answering `path`, in binding order, with + /// HEAD after them when a GET binding answers it. + fn allow(&self, path: &str) -> HeaderValue { + let mut methods: Vec<&str> = Vec::new(); + for binding in self.bindings.iter().filter(|b| b.answers(path)) { + let method = binding.entry.http_method.as_str(); + if !methods.contains(&method) { + methods.push(method); + } + } + if methods.contains(&"GET") && !methods.contains(&"HEAD") { + methods.push("HEAD"); + } + HeaderValue::from_str(&methods.join(", ")) + .expect("method tokens are valid header value characters") + } + + /// Turn the path parameters the router matched on this table's path into + /// those of the binding at `index`: its verb taken off the last capture, + /// and the captures under its own names. A parameter the table's path does + /// not capture (a prefix the router is nested under) is left as it is. + pub(super) fn bind_params(&self, index: usize, params: &mut PathParams) { + let binding = &self.bindings[index]; + if let (Some(verb), Some(last)) = (&binding.verb, self.captures.last()) { + if let Some(value) = params.get_mut(last) { + // The verb ends the raw path and holds no escape, so the + // decoded value ends in it too. + if let Some(len) = value.strip_suffix(verb.as_str()).map(str::len) { + value.truncate(len); + } + } + } + if let Some(names) = &binding.names { + // Taken out before any goes back in: one binding's name may be + // another capture's name on the table's path. + let renamed: Vec<(&String, Option)> = self + .captures + .iter() + .zip(names) + .filter(|(path_name, own)| path_name != own) + .map(|(path_name, own)| (own, params.remove(path_name))) + .collect(); + for (own, value) in renamed { + if let Some(value) = value { + params.insert(own.clone(), value); + } + } + } + } +} diff --git a/src/transcode/tests.rs b/src/transcode/tests.rs index 4595e66..a072bb2 100644 --- a/src/transcode/tests.rs +++ b/src/transcode/tests.rs @@ -158,6 +158,253 @@ fn multi_segment_field_template_does_not_fracture() { let _router: Router<()> = Router::new().route(&path, get(|| async { "ok" })); } +#[test] +fn custom_verb_after_a_variable_or_wildcard_is_split_off() { + // `Template = "/" Segments [ Verb ]`: axum cannot match text after a + // capture, so the verb leaves the mounted path and is kept to be matched + // by the transcoded router. + let cases = [ + // (template, axum path, shape, captures, verb) + ( + "/v2/ops/{operation}:cancel", + "/v2/ops/{operation}", + "/v2/ops/{}", + &["operation"][..], + Some(":cancel"), + ), + ( + "/v1/{name=publishers/*/books/*}:archive", + "/v1/{*name}", + "/v1/{*}", + &["name"][..], + Some(":archive"), + ), + ( + "/v1/{name=*}:x", + "/v1/{name}", + "/v1/{}", + &["name"][..], + Some(":x"), + ), + ( + "/v1/files/{path=**}:x", + "/v1/files/{*path}", + "/v1/files/{*}", + &["path"][..], + Some(":x"), + ), + ( + "/v1/*:x", + "/v1/{wildcard2}", + "/v1/{}", + &["wildcard2"][..], + Some(":x"), + ), + ( + "/v1/**:x", + "/v1/{*wildcard2}", + "/v1/{*}", + &["wildcard2"][..], + Some(":x"), + ), + // A verb whose LITERAL holds a colon is the whole rest of the segment. + ("/v1/{a}:b:c", "/v1/{a}", "/v1/{}", &["a"][..], Some(":b:c")), + ]; + for (template, axum, shape, captures, verb) in cases { + let mount = path::MountedPath::new(template); + assert_eq!(mount.axum, axum, "{template}"); + assert_eq!(mount.shape, shape, "{template}"); + assert_eq!(mount.captures, captures, "{template}"); + assert_eq!(mount.verb.as_deref(), verb, "{template}"); + // Policies and the log see the template's own path, verb included. + assert_eq!( + mount.display(), + axum.to_owned() + verb.unwrap(), + "{template}" + ); + assert!(path::mountable(&mount.axum).is_ok(), "{template}"); + } + assert_eq!(proto_path_to_axum("/v2/ops/{op}:cancel"), "/v2/ops/{op}"); +} + +#[test] +fn verb_after_a_literal_or_anything_else_stays_in_the_path() { + // After a literal axum matches the verb as part of it. + let mount = path::MountedPath::new("/v1/nodes:batch"); + assert_eq!(mount.axum, "/v1/nodes:batch"); + assert_eq!(mount.verb, None); + assert!(mount.captures.is_empty()); + // A colon before the last segment is no verb. + let mount = path::MountedPath::new("/v1/{a}:x/items"); + assert_eq!(mount.verb, None); + assert_eq!(mount.axum, "/v1/{a}:x/items"); + // An empty verb, and text after a variable that is not a verb, stay in the + // path, which the router cannot match. + for template in ["/v1/{a}:", "/v1/{a}x:y", "/v1/{a"] { + let mount = path::MountedPath::new(template); + assert_eq!(mount.verb, None, "{template}"); + assert!(path::mountable(&mount.axum).is_err(), "{template}"); + } +} + +#[test] +fn mountable_refuses_what_axum_would_panic_on() { + for (path, reason) in [ + ("/v1/a{x}b", "text around a capture"), + ("/v1/{x}{y}", "two captures in one segment"), + ("/v1/{}", "a capture without a name"), + ("v1/items", "no leading slash"), + ("/v1/:items", "a segment starting with ':'"), + ("/v1/*items", "a segment starting with '*'"), + ] { + assert!(path::mountable(path).is_err(), "{path}: {reason}"); + // The same path does panic axum. + let registered = std::panic::catch_unwind(|| { + let _router: Router<()> = Router::new().route(path, get(|| async { "ok" })); + }); + assert!(registered.is_err(), "{path}: axum took it"); + } + for path in ["/v1/{x}", "/v1/{*rest}", "/v1/nodes:batch", "/"] { + assert!(path::mountable(path).is_ok(), "{path}"); + } +} + +#[test] +fn route_paths_list_one_route_for_bindings_that_differ_only_by_verb() { + // A verb after a variable is matched past the router, so the bindings of + // one method that differ only by it share a route; a repeated method, + // shape and verb is a real duplicate and stays visible to the caller. + let pool = api_pool( + r#"syntax = "proto3"; +package t; +import "google/api/annotations.proto"; +message Req { string name = 1; string id = 2; } +service S { + rpc Get(Req) returns (Req) { option (google.api.http) = { get: "/v1/ops/{name}" }; } + rpc Cancel(Req) returns (Req) { option (google.api.http) = { post: "/v1/ops/{name}:cancel" }; } + rpc Pause(Req) returns (Req) { option (google.api.http) = { post: "/v1/ops/{id}:pause" }; } + rpc Batch(Req) returns (Req) { option (google.api.http) = { post: "/v1/ops:batch" }; } + rpc Again(Req) returns (Req) { option (google.api.http) = { post: "/v1/ops/{id}:cancel" }; } +} +"#, + ); + let paths = route_paths(&pool, &[], &RpcSelection::default()); + let expected = [ + ("GET", "/v1/ops/{name}"), + ("POST", "/v1/ops/{name}"), + ("POST", "/v1/ops:batch"), + ("POST", "/v1/ops/{id}"), + ]; + let expected: Vec<(String, String)> = expected + .iter() + .map(|(m, p)| ((*m).to_owned(), (*p).to_owned())) + .collect(); + assert_eq!(paths, expected); +} + +#[test] +fn bindings_of_one_shape_share_one_table_whatever_their_names() { + // The router tells `/v1/ops/{name}` and `/v1/ops/{id}` apart by shape + // only; registering both panicked axum. One table takes them. + let pool = api_pool( + r#"syntax = "proto3"; +package t; +import "google/api/annotations.proto"; +message Req { string name = 1; string id = 2; } +service S { + rpc Get(Req) returns (Req) { option (google.api.http) = { get: "/v1/ops/{name}" }; } + rpc Post(Req) returns (Req) { option (google.api.http) = { post: "/v1/ops/{id}" }; } + rpc Broken(Req) returns (Req) { option (google.api.http) = { get: "/v1/broken/a{name}b" }; } +} +"#, + ); + let tables = path_tables(&pool, &[], &TranscodeOptions::default()); + let paths: Vec<&str> = tables.iter().map(|t| t.path.as_str()).collect(); + // The template the router cannot match is left out. + assert_eq!(paths, ["/v1/ops/{name}"]); + let table = &tables[0]; + let post = match table.choose(&Method::POST, "/v1/ops/x") { + table::Choice::Route(index) => index, + _ => panic!("POST must be answered"), + }; + assert_eq!(table.entry(post).grpc_path.path(), "/t.S/Post"); + // The router itself builds from them. + let _router: Router> = routes(&pool, &[]); +} + +#[test] +fn swapped_capture_names_bind_each_binding_its_own_values() { + // The table's path names its captures `a`, `b`; the POST binding names + // them `b`, `a`. Renaming one by one would let one value overwrite the + // other. + let pool = api_pool( + r#"syntax = "proto3"; +package t; +import "google/api/annotations.proto"; +message Req { string a = 1; string b = 2; } +service S { + rpc Get(Req) returns (Req) { option (google.api.http) = { get: "/x/{a}/{b}" }; } + rpc Post(Req) returns (Req) { option (google.api.http) = { post: "/x/{b}/{a}:go" }; } +} +"#, + ); + let tables = path_tables(&pool, &[], &TranscodeOptions::default()); + assert_eq!(tables.len(), 1); + let table = &tables[0]; + let matched = || -> PathParams { + [("a", "first"), ("b", "second:go")] + .into_iter() + .map(|(k, v)| (k.to_owned(), v.to_owned())) + .collect() + }; + let post = match table.choose(&Method::POST, "/x/first/second:go") { + table::Choice::Route(index) => index, + _ => panic!("POST must be answered"), + }; + let mut params = matched(); + table.bind_params(post, &mut params); + assert_eq!(params.len(), 2); + assert_eq!(params["b"], "first"); + assert_eq!(params["a"], "second"); + // The binding with the path's own names and no verb is left as matched. + let get = match table.choose(&Method::GET, "/x/first/second:go") { + table::Choice::Route(index) => index, + _ => panic!("GET must be answered"), + }; + let mut params = matched(); + table.bind_params(get, &mut params); + assert_eq!(params, matched()); +} + +#[test] +fn head_falls_back_to_get_and_the_longest_verb_wins() { + let pool = api_pool( + r#"syntax = "proto3"; +package t; +import "google/api/annotations.proto"; +message Req { string name = 1; } +service S { + rpc Get(Req) returns (Req) { option (google.api.http) = { get: "/v1/{name}" }; } + rpc Short(Req) returns (Req) { option (google.api.http) = { post: "/v1/{name}:c" }; } + rpc Long(Req) returns (Req) { option (google.api.http) = { post: "/v1/{name}:b:c" }; } +} +"#, + ); + let tables = path_tables(&pool, &[], &TranscodeOptions::default()); + let table = &tables[0]; + let rpc = |method: Method, path: &str| match table.choose(&method, path) { + table::Choice::Route(index) => table.entry(index).grpc_path.path().to_owned(), + table::Choice::MethodNotAllowed(allow) => format!("405 {}", allow.to_str().unwrap()), + table::Choice::NotFound => "404".to_owned(), + }; + assert_eq!(rpc(Method::HEAD, "/v1/x"), "/t.S/Get"); + // Both verbs end the path; the longer one is the binding's. + assert_eq!(rpc(Method::POST, "/v1/x:b:c"), "/t.S/Long"); + assert_eq!(rpc(Method::POST, "/v1/x:c"), "/t.S/Short"); + assert_eq!(rpc(Method::POST, "/v1/x"), "405 GET, HEAD"); + assert_eq!(rpc(Method::PUT, "/v1/x:c"), "405 GET, POST, HEAD"); +} + /// Regression for the axum 0.7→0.8 migration bug: `proto_path_to_axum` /// emitted `:id` syntax, which axum 0.8 rejects at `Router::route()` with /// a startup panic ("Path segments must not start with `:`"). Building the diff --git a/tests/custom_verbs.rs b/tests/custom_verbs.rs index 5d738b0..7e12378 100644 --- a/tests/custom_verbs.rs +++ b/tests/custom_verbs.rs @@ -46,6 +46,9 @@ service Ops { rpc Pause(Msg) returns (Msg) { option (google.api.http) = { post: "/v2/ops/{name}:pause" }; } + rpc Drop(Msg) returns (Msg) { + option (google.api.http) = { delete: "/v2/ops/{name}" }; + } rpc Restore(Msg) returns (Msg) { option (google.api.http) = { post: "/v1/{name=publishers/*/books/*}:restore" }; } @@ -166,22 +169,38 @@ async fn bindings_differing_only_by_verb_reach_their_own_methods() { } async fn a_request_without_the_verb_does_not_reach_a_verb_binding() { - // Only the GET binding answers `/v2/ops/{operation}` without a verb. + // Only the GET and DELETE bindings answer `/v2/ops/{..}` without a verb. let app = proxy(UPSTREAM).await; let (status, allow, _) = call(&app, Method::POST, "/v2/ops/op-1").await; assert_eq!(status, StatusCode::METHOD_NOT_ALLOWED); - assert_eq!(allow, "GET, HEAD"); + assert_eq!(allow, "GET, DELETE, HEAD"); let (status, _, body) = call(&app, Method::GET, "/v2/ops/op-1").await; assert_eq!(status, StatusCode::OK, "{body}"); assert_eq!(body["rpc"], "Get"); assert_eq!(body["operation"], "op-1"); } +async fn bindings_of_one_path_bind_their_own_variable_names() { + // `/v2/ops/{operation}` (GET) and `/v2/ops/{name}` (DELETE) are one path + // to the router; each binding still fills its own field. + let app = proxy(UPSTREAM).await; + let (status, _, body) = call(&app, Method::DELETE, "/v2/ops/op-3").await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body, json!({"name": "op-3", "operation": "", "rpc": "Drop"})); +} + +async fn head_is_answered_by_the_get_binding_without_a_body() { + let app = proxy(UPSTREAM).await; + let (status, _, body) = call(&app, Method::HEAD, "/v2/ops/op-1").await; + assert_eq!(status, StatusCode::OK); + assert_eq!(body, Value::Null); +} + async fn another_verb_does_not_reach_a_verb_binding() { let app = proxy(UPSTREAM).await; let (status, allow, _) = call(&app, Method::POST, "/v2/ops/op-1:delete").await; assert_eq!(status, StatusCode::METHOD_NOT_ALLOWED); - assert_eq!(allow, "GET, HEAD"); + assert_eq!(allow, "GET, DELETE, HEAD"); // A binding without a verb takes the whole segment, colon included. let (status, _, body) = call(&app, Method::GET, "/v2/ops/op-1:delete").await; assert_eq!(status, StatusCode::OK, "{body}"); @@ -207,12 +226,12 @@ async fn a_verb_with_an_empty_variable_does_not_match() { } async fn a_method_no_binding_of_the_url_answers_lists_those_that_do() { - // DELETE is bound nowhere; GET (without a verb) and POST (`:cancel`) both - // answer this URL. + // PUT is bound nowhere; GET and DELETE (without a verb) and POST + // (`:cancel`) answer this URL, POST `:archive` and `:pause` do not. let app = proxy(UPSTREAM).await; - let (status, allow, _) = call(&app, Method::DELETE, "/v2/ops/op-1:cancel").await; + let (status, allow, _) = call(&app, Method::PUT, "/v2/ops/op-1:cancel").await; assert_eq!(status, StatusCode::METHOD_NOT_ALLOWED); - assert_eq!(allow, "GET, POST, HEAD"); + assert_eq!(allow, "GET, POST, DELETE, HEAD"); } async fn verb_after_a_multi_segment_field_template_routes() { @@ -244,9 +263,35 @@ async fn a_template_the_router_cannot_serve_is_skipped_not_fatal() { // `a{name}b` puts text around a variable, which the router cannot // match; that binding is left out and every other one still serves. let app = proxy(UPSTREAM).await; - let (status, _, _) = call(&app, Method::GET, "/v1/broken/axb").await; + let request = http::Request::get("/v1/broken/axb").body(Body::empty()).unwrap(); + let (status, _) = common::send(&app, request).await; assert_eq!(status, StatusCode::NOT_FOUND); let (status, _, body) = call(&app, Method::GET, "/v2/ops/op-1").await; assert_eq!(status, StatusCode::OK, "{body}"); } } + +#[tokio::test] +async fn a_verb_bound_twice_for_one_method_is_refused_at_startup() { + // Two POST `:cancel` bindings on one path cannot both serve; the proxy + // says so instead of serving one of them. + const TWICE_PROTO: &str = r#" +syntax = "proto3"; +package test.v1; +import "google/api/annotations.proto"; +message Msg { string name = 1; string id = 2; } +service Twice { + rpc A(Msg) returns (Msg) { option (google.api.http) = { post: "/v1/x/{name}:cancel" }; } + rpc B(Msg) returns (Msg) { option (google.api.http) = { post: "/v1/x/{id}:cancel" }; } +} +"#; + let pool = common::compile("test/v1/twice.proto", TWICE_PROTO); + let err = structured_proxy::ProxyServer::from_yaml_str( + "upstream:\n default: \"http://127.0.0.1:1\"\n", + ) + .unwrap() + .with_descriptors(pool) + .router() + .expect_err("a repeated method, path and verb must be rejected"); + assert!(err.to_string().contains("more than one endpoint"), "{err}"); +} From bfa7640a4be68d7a77b19d9c021bb68038b0eaeb Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 7 Oct 2026 19:12:26 +0300 Subject: [PATCH 03/20] docs: add contributing guide, security policy and CLA note The repository had no contributing guide or security policy. CONTRIBUTING lists the checks CI runs for every crypto backend and the organisation CLA; SECURITY points to private vulnerability reporting; the README license section links both. --- CONTRIBUTING.md | 62 +++++++++++++++++++++++++++++++++++++++++++++++++ README.md | 2 ++ SECURITY.md | 20 ++++++++++++++++ 3 files changed, 84 insertions(+) create mode 100644 CONTRIBUTING.md create mode 100644 SECURITY.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 0000000..9e5edb0 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,62 @@ +# Contributing to structured-proxy + +Bug reports, fixes, documentation and new capabilities are welcome. + +## Development setup + +```bash +git clone https://github.com/structured-world/structured-proxy.git +cd structured-proxy +cargo build --features cli +``` + +Tests run under [cargo-nextest](https://nexte.st/), which is not part of the +Rust toolchain: + +```bash +cargo install cargo-nextest --locked +``` + +Before opening a pull request, run what CI runs. The crate builds with each +crypto backend, without one and with both, and the code differs between them: + +```bash +cargo fmt --all -- --check +cargo clippy --all-targets --features cli -- -D warnings +cargo clippy --all-targets --no-default-features --features cli -- -D warnings +cargo clippy --all-targets --all-features -- -D warnings +cargo nextest run --features cli +cargo nextest run --no-default-features --features cli +cargo nextest run --all-features +cargo test --doc --all-features +``` + +The rate-limit reconciliation test runs against Redis when +`SHIELD_REDIS_TEST_URL` names one (`redis://127.0.0.1:6379/`). + +## Pull requests + +1. Create a branch from `main`. +2. Make the change, with tests. A bug fix comes with a test that fails without + it. +3. Write commit messages and the pull request title in the + [Conventional Commits](https://www.conventionalcommits.org/) form; pull + requests are squash-merged, so the title becomes the commit on `main`. +4. A change to what the proxy serves or how it is configured updates the + README in the same pull request. + +## Contributor License Agreement (CLA) + +Before a first pull request can be merged, you sign the Structured World +Contributor License Agreement once, at . It covers +every repository of the organisation and takes a minute: sign in with GitHub, +confirm your e-mail address, sign. The `CLA` status on your pull request then +turns green by itself. + +You keep the copyright in your contribution. If you contribute as part of your +job, your employer may also need to sign the corporate agreement; the page +above explains when. + +## Security + +Do not report vulnerabilities in public issues; see [SECURITY.md](SECURITY.md). diff --git a/README.md b/README.md index 5ad1d42..ef6b3f3 100644 --- a/README.md +++ b/README.md @@ -1485,3 +1485,5 @@ USDT (TRC-20): `TFDsezHa1cBkoeZT5q2T49Wp66K8t2DmdA` ## License Apache-2.0 + +Contributions are accepted under the [Structured World Contributor License Agreement](https://sw.foundation/cla); see [CONTRIBUTING.md](CONTRIBUTING.md). diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 0000000..875988a --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,20 @@ +# Security policy + +structured-proxy faces untrusted clients: a way past a guard (auth, rate +limits, maintenance), a request that reaches the upstream in a form it should +not, a crash, a hang or unbounded memory on any request is a security issue. + +## Reporting a vulnerability + +Report it privately through +[GitHub private vulnerability reporting](https://github.com/structured-world/structured-proxy/security/advisories/new), +not in a public issue. Include the request or configuration that triggers it, +the version or commit, the features enabled, and what you observed. + +You will get an answer within a few days. A confirmed issue is fixed in a new +release, and the advisory is published with credit to you unless you prefer +otherwise. + +## Supported versions + +Fixes go into the latest release only. From 3d9f5f9761db05522e07fc6eedffe17a86010f6e Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 7 Oct 2026 20:40:48 +0300 Subject: [PATCH 04/20] fix(transcode): split the verb at the first colon and index it by path A request's custom verb is its last segment from the first unencoded `:`: a client percent-encodes the reserved characters of a variable's value and a template writes the reserved characters of a verb encoded too (google/api/http.proto), so neither holds a raw colon. Verbs are compared whole instead of as path suffixes, so `x:b:c` is the verb `:b:c`, not the variable `x:b` with `:c`. - A verb some binding of the request's path binds owns that URL: only its bindings answer, 405 with their methods otherwise. A verb none binds stays part of the last variable, as Envoy and grpc-gateway treat it. - Verbs are indexed across route shapes, so a verb bound on a path the router ranks below another match (a static prefix) is still found, with the variables of its own path. - A verb written percent-encoded in the template is taken off the decoded variable in decoded form. - `**` before a verb may match no segment. - route_paths lists each route as it is mounted: a `*` rule with a verb no longer collides with another verb on its path. Regression tests cover each case and failed on the previous routing. Closes #138 --- Cargo.toml | 3 + README.md | 22 ++-- src/transcode/mod.rs | 151 +++++++++++++++++++------- src/transcode/path.rs | 15 +++ src/transcode/table.rs | 233 ++++++++++++++++++++++++++++++----------- src/transcode/tests.rs | 113 ++++++++++++++------ tests/custom_verbs.rs | 142 ++++++++++++++++++++++++- 7 files changed, 533 insertions(+), 146 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 6108dbf..f9c93ea 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -64,6 +64,9 @@ tokio-util = { version = "0.7.16", features = ["rt"] } # before axum registers it, where a path it rejects would panic. Already in the # tree through axum, which pins the version this resolves to. matchit = "0.8" +# Decodes the path parameters of a custom verb bound on another route the +# request matches, as axum decodes its own; already in the tree through axum. +percent-encoding = "2" # gRPC client (to upstream service). `tls-connect-info` gives a rustls server # stream the connection record (client certificates) a tonic handler reads, so diff --git a/README.md b/README.md index ef6b3f3..ebbcae4 100644 --- a/README.md +++ b/README.md @@ -384,15 +384,19 @@ answered with `INVALID_ARGUMENT` (400) before the upstream is called. **Custom verbs.** A path template may end in a verb, as AIP-136 custom methods do (`post: "/v1/{name=operations/*}:cancel"`): -- A binding with a verb answers only a request whose path ends in it, the - variable taking the rest of the segment. A percent-encoded colon (`%3A`) is - part of the value, never the verb's delimiter. -- Bindings of one path that differ only by verb reach their own RPCs. A - binding without a verb takes the whole last segment, colon included, when - no verb of the path ends the request. -- A request that matches the path but none of its verbs is `404`; one whose - verb matches but whose method does not is `405`, with the methods that - answer it in `Allow`. +- A request's verb is its last segment from the first unencoded colon: a + client percent-encodes the reserved characters of a variable's value, and a + verb writes its own reserved characters encoded too + (`:run%3Anow`), so neither holds a raw `:`. A percent-encoded colon (`%3A`) + in a request is part of a value, never the verb's delimiter. +- A verb some binding of the request's path binds owns that URL: only the + bindings of that verb answer it, each reaching its own RPC. A verb no + binding binds stays part of the last variable (`/v1/items/a:b` is the item + `a:b`), as Envoy's transcoder and grpc-gateway treat an unbound verb. +- `**` before a verb may match no segment: `/v1/{name=**}:purge` answers + `/v1/:purge` with an empty `name`. +- A URL that bindings answer, but none with the request's method, is `405`, + with their methods in `Allow`; a path that no binding answers is `404`. A template the router cannot match, such as text around a variable within one segment (`/v1/a{name}b`), is left out with an error in the log; the other diff --git a/src/transcode/mod.rs b/src/transcode/mod.rs index daab579..3a377c0 100644 --- a/src/transcode/mod.rs +++ b/src/transcode/mod.rs @@ -51,7 +51,7 @@ use error::{ErrorDetailsPolicy, StatusDetails}; use path::MountedPath; use response::UpstreamHeaders; use rule::RouteMethod; -use table::{Choice, PathTable}; +use table::{Choice, PathTable, Routes}; /// How long the upstream may take to answer a call with its response headers /// when the client's `grpc-timeout` asks for no shorter deadline. @@ -240,11 +240,12 @@ pub fn routes(pool: &DescriptorPool, aliases: &[AliasConfig]) /// /// A binding answers the requests whose path matches its template, custom verb /// included (`/v1/{name}:cancel`, google/api/http.proto `Verb`), and whose -/// method is its own; a GET binding answers HEAD too. Among the bindings whose -/// verb a request's path ends in, the one with the longest verb serves it. A -/// path that bindings answer, but none with the request's method, is `405` -/// with their methods in `Allow`; a path that matches a template but no verb -/// of it is `404`. +/// method is its own; a GET binding answers HEAD too. A request's verb is its +/// last segment from the first unencoded `:`: a verb some binding of its path +/// binds owns the URL, answered by that verb's bindings alone; a verb none +/// binds stays part of the last variable. A URL that bindings answer, but none +/// with the request's method, is `405` with their methods in `Allow`; a path +/// that matches a template but that no binding answers is `404`. /// /// A second binding for a method, path and verb already taken (or any binding /// on a path and verb a `custom` `*` rule takes, which answers every method), @@ -262,16 +263,16 @@ pub fn routes_with_options( return Router::new(); } + let routes = Arc::new(Routes::new(tables)); let mut router: Router = Router::new(); - for table in tables { - let path = table.path.clone(); - let table = Arc::new(table); - // Every method goes to the table, which answers 405 itself: the URL, - // not the path alone, decides which methods a path answers. + for (table, mounted) in routes.tables.iter().enumerate() { + let routes = routes.clone(); + // Every method goes to the routes, which answer 405 themselves: the + // URL, not the path alone, decides which methods a path answers. router = router.route( - &path, + &mounted.path, axum::routing::any(move |State(state): State, request: Request| { - dispatch(table, state, request) + dispatch(routes, table, state, request) }), ); } @@ -341,17 +342,18 @@ fn path_tables( tables } -/// Serve `request` on `table`'s path: choose the binding before anything is -/// extracted, so a request no binding answers gets its 404 or 405 without its -/// body being read. +/// Serve `request`, which the router matched to the table at `matched`: +/// choose the binding before anything is extracted, so a request no binding +/// answers gets its 404 or 405 without its body being read. async fn dispatch( - table: Arc, + routes: Arc, + matched: usize, state: S, request: Request, ) -> Response { let (mut parts, body) = request.into_parts(); - let index = match table.choose(&parts.method, parts.uri.path()) { - Choice::Route(index) => index, + let (table, index) = match routes.choose(matched, &parts.method, parts.uri.path()) { + Choice::Route { table, index } => (table, index), Choice::MethodNotAllowed(allow) => { return (StatusCode::METHOD_NOT_ALLOWED, [(ALLOW, allow)]).into_response() } @@ -361,7 +363,14 @@ async fn dispatch( Ok(Path(params)) => params, Err(rejection) => return rejection.into_response(), }; - table.bind_params(index, &mut path_params); + if table != matched { + // A verb bound on another path the request matches: its captures come + // from that path, the router's prefix parameters stay. + if let Err(rejection) = rebind(&routes, matched, parts.uri.path(), &mut path_params) { + return rejection.into_response(routes.tables[table].entry(index)); + } + } + routes.tables[table].bind_params(index, &mut path_params); // The target as received, before a router the proxy is nested in strips // its prefix; the router records it on every request it routes. let uri = match parts.extensions.remove::() { @@ -385,15 +394,45 @@ async fn dispatch( Ok(body) => body, Err(rejection) => return rejection.into_response(), }; - handle(state, client, path_params, body, RouteRef { table, index }).await + let entry = RouteRef { + routes, + table, + index, + }; + handle(state, client, path_params, body, entry).await } -/// The binding serving a request, held through its path's table: the table -/// is already shared with the request, so naming the binding takes no -/// refcount of its own. +/// Replace the captures of the table at `matched` in `params` with those of +/// the table binding the verb `path` ends in, percent-decoded as the router +/// decodes its own. +fn rebind( + routes: &Routes, + matched: usize, + path: &str, + params: &mut PathParams, +) -> Result<(), Unmappable> { + for name in &routes.tables[matched].captures { + params.remove(name); + } + let found = routes + .verb_match(path) + .expect("the verb's table was chosen from this match"); + for (name, raw) in found.params.iter() { + let value = percent_encoding::percent_decode_str(raw) + .decode_utf8() + .map_err(|_| Unmappable(format!("path parameter {name} is not valid UTF-8")))?; + params.insert(name.to_owned(), value.into_owned()); + } + Ok(()) +} + +/// The binding serving a request, held through the routes: they are already +/// shared with the request, so naming the binding takes no refcount of its +/// own. #[derive(Clone)] struct RouteRef { - table: Arc, + routes: Arc, + table: usize, index: usize, } @@ -401,7 +440,7 @@ impl std::ops::Deref for RouteRef { type Target = RouteEntry; fn deref(&self) -> &RouteEntry { - self.table.entry(self.index) + self.routes.tables[self.table].entry(self.index) } } @@ -461,24 +500,62 @@ fn route_bindings( /// or `*` for a `custom` rule that answers every method, so callers can /// distinguish same-path/different-method routes from real conflicts. A /// custom verb after a variable is matched by the transcoded routes, not by -/// the path: bindings of one method that differ only by such a verb share one -/// route and are listed once, while a repeated method, path and verb is listed -/// as often as it is bound. +/// the path, so the bindings of one path are listed as the route they share: +/// `*` once when a `custom` `*` rule is among them, else each method once. +/// Two bindings that cannot both serve (one method or a `*` rule, one verb) +/// are listed both, so the caller sees the collision. pub fn route_paths( pool: &DescriptorPool, aliases: &[AliasConfig], selection: &RpcSelection, ) -> Vec<(String, String)> { - // The verbs bound so far under each (method, route shape). - let mut verbs: HashMap<(String, String), Vec>> = HashMap::new(); + // The bindings of each route shape, in first-binding order. + let mut shapes: Vec> = Vec::new(); + let mut by_shape: HashMap = HashMap::new(); + for binding in route_bindings(pool, aliases, selection) { + match by_shape.get(&binding.mount.shape) { + Some(&index) => shapes[index].push(binding), + None => { + by_shape.insert(binding.mount.shape.clone(), shapes.len()); + shapes.push(vec![binding]); + } + } + } let mut paths = Vec::new(); - for RouteBinding { entry, mount } in route_bindings(pool, aliases, selection) { - let method = entry.http_method.as_str().to_owned(); - let bound = verbs.entry((method.clone(), mount.shape)).or_default(); - if bound.is_empty() || bound.contains(&mount.verb) { - paths.push((method, mount.axum)); + for bindings in &shapes { + let listed = |method: &str, path: &str| (method.to_owned(), path.to_owned()); + match bindings + .iter() + .find(|b| b.entry.http_method == RouteMethod::Any) + { + Some(star) => paths.push(listed("*", &star.mount.axum)), + None => { + let mut methods: Vec<&str> = Vec::new(); + for binding in bindings { + let method = binding.entry.http_method.as_str(); + if !methods.contains(&method) { + methods.push(method); + paths.push(listed(method, &binding.mount.axum)); + } + } + } + } + for (at, binding) in bindings.iter().enumerate() { + let clashes = bindings[..at].iter().any(|earlier| { + table::clash( + &earlier.entry.http_method, + earlier.mount.verb.as_deref(), + &binding.entry.http_method, + binding.mount.verb.as_deref(), + ) + }); + if clashes { + paths.push(listed( + binding.entry.http_method.as_str(), + &binding.mount.axum, + )); + } } - bound.push(mount.verb); } paths } diff --git a/src/transcode/path.rs b/src/transcode/path.rs index 00461e6..d9dab1d 100644 --- a/src/transcode/path.rs +++ b/src/transcode/path.rs @@ -20,6 +20,9 @@ pub(crate) struct MountedPath { pub(crate) captures: Vec, /// The custom verb after the last segment's variable, `:` included. pub(crate) verb: Option, + /// Whether the last capture may be empty: a `**` matches zero or more + /// segments (google/api/http.proto), any other variable at least one. + pub(crate) empty_last: bool, } impl MountedPath { @@ -31,6 +34,7 @@ impl MountedPath { let mut axum = String::with_capacity(base.len()); let mut shape = String::with_capacity(base.len()); let mut captures = Vec::new(); + let empty_last = is_double_wildcard(segments[last]); for (idx, segment) in segments.iter().enumerate() { if idx > 0 { axum.push('/'); @@ -57,6 +61,7 @@ impl MountedPath { shape, captures, verb: verb.map(str::to_owned), + empty_last, } } @@ -133,6 +138,16 @@ fn split_verb(template: &str) -> (&str, Option<&str>) { } } +/// Whether `segment` is `**` or a variable bound to it (`{name=**}`). +fn is_double_wildcard(segment: &str) -> bool { + segment == "**" + || segment + .strip_prefix('{') + .and_then(|s| s.strip_suffix('}')) + .and_then(|inner| inner.split_once('=')) + .is_some_and(|(_, template)| template == "**") +} + /// The index of the `}` closing the `{` that opens `segment`. fn closing_brace(segment: &str) -> Option { let mut depth = 0usize; diff --git a/src/transcode/table.rs b/src/transcode/table.rs index 941681e..816293e 100644 --- a/src/transcode/table.rs +++ b/src/transcode/table.rs @@ -1,5 +1,16 @@ -//! The bindings mounted at one router path, and the choice among them for a -//! request: by its method and by the custom verb its path ends in. +//! The transcoded routes: the bindings mounted at each router path, and the +//! choice among them for a request, by the custom verb its path ends in and +//! by its method. +//! +//! A request's verb is its last segment from the first unencoded `:`. Neither +//! side of the verb holds one: a client percent-encodes every reserved +//! character of a variable's value, and a `LITERAL` writes its reserved +//! characters percent-encoded too (google/api/http.proto). A verb some binding +//! of the request's path binds owns that URL; a verb none binds stays part of +//! the last variable, as Envoy's transcoder and grpc-gateway leave an unbound +//! verb. + +use std::collections::HashMap; use axum::http::{HeaderValue, Method}; @@ -7,12 +18,21 @@ use super::path::MountedPath; use super::rule::RouteMethod; use super::{PathParams, RouteEntry}; +/// Every transcoded route, and where each custom verb is bound. +pub(super) struct Routes { + pub(super) tables: Vec, + /// For each verb, the paths of the tables binding it. The router matches a + /// request to one path whatever its verb, while the verb may be bound on + /// a path of another shape the request also matches. + verbs: HashMap>, +} + /// Every binding the router serves at one path. pub(super) struct PathTable { /// The router path, written with the first binding's capture names. pub(super) path: String, /// The capture names of `path`, in path order. - captures: Vec, + pub(super) captures: Vec, bindings: Vec, } @@ -22,37 +42,110 @@ struct Binding { /// This binding's names for the captures of the table's path, when they /// differ from the path's own. names: Option>, - /// The custom verb after the last capture, `:` included. - verb: Option, + verb: Option, } -/// What answers a request on a table's path. +/// A custom verb after the last capture. +struct Verb { + /// As written in the template, `:` included: compared with the request's + /// path as received. + raw: String, + /// `raw` percent-decoded: what ends the decoded value of the capture. + decoded: String, + /// Whether the capture before it may be empty (`**`). + empty_ok: bool, +} + +/// What answers a request. pub(super) enum Choice { - /// The binding at this index. - Route(usize), + /// The binding at `index` of the table at `table`. + Route { table: usize, index: usize }, /// Bindings answer the URL, none with the request's method: the value of /// `Allow` lists their methods (RFC 9110 §15.5.6). MethodNotAllowed(HeaderValue), - /// No binding answers the URL: its verb is none of theirs. + /// No binding answers the URL. NotFound, } -impl Binding { - /// Whether this binding answers a request for `path` (as received, still - /// percent-encoded): with a verb, only a path whose last segment ends in - /// it after at least one character of the variable. An encoded colon - /// (`%3A`) is data, never the verb's delimiter (RFC 3986 §2.2). - fn answers(&self, path: &str) -> bool { - match &self.verb { - None => true, - Some(verb) => path - .strip_suffix(verb.as_str()) - .is_some_and(|rest| !rest.is_empty() && !rest.ends_with('/')), +/// The request path without its custom verb, and the verb (`:` included): +/// the last segment from its first unencoded colon. +pub(super) fn split_verb(path: &str) -> Option<(&str, &str)> { + let start = path.rfind('/').map_or(0, |slash| slash + 1); + let colon = start + path[start..].find(':')?; + Some(path.split_at(colon)) +} + +/// Whether two bindings of one path cannot both serve: the same verb and the +/// same method, or the same verb and a `custom` `*` rule, which answers every +/// method. +pub(super) fn clash( + method: &RouteMethod, + verb: Option<&str>, + other_method: &RouteMethod, + other_verb: Option<&str>, +) -> bool { + verb == other_verb + && (method == other_method + || *method == RouteMethod::Any + || *other_method == RouteMethod::Any) +} + +impl Routes { + /// The routes of `tables`, with their verbs indexed. Every path was + /// already accepted by a router holding them all, so a router holding + /// some of them accepts it too. + pub(super) fn new(tables: Vec) -> Self { + let mut verbs: HashMap> = HashMap::new(); + for (index, table) in tables.iter().enumerate() { + let mut seen: Vec<&str> = Vec::new(); + for verb in table.bindings.iter().filter_map(|b| b.verb.as_ref()) { + if seen.contains(&verb.raw.as_str()) { + continue; + } + seen.push(&verb.raw); + let inserted = verbs + .entry(verb.raw.clone()) + .or_default() + .insert(table.path.as_str(), index); + debug_assert!(inserted.is_ok(), "{}: {inserted:?}", table.path); + } } + Self { tables, verbs } } - fn verb_len(&self) -> usize { - self.verb.as_ref().map_or(0, String::len) + /// The table binding the verb `path` ends in, when one of the request's + /// path does: its index and the router's match, whose captures are that + /// table's. + pub(super) fn verb_match<'r, 'p>( + &'r self, + path: &'p str, + ) -> Option> { + let (_, verb) = split_verb(path)?; + self.verbs.get(verb)?.at(path).ok() + } + + /// Which binding answers `method` on `path`, a request the router matched + /// to the table at `table`. A verb some table binds for this path owns the + /// URL: only the bindings of that verb answer it. Otherwise the bindings + /// without a verb of `table` do, the verb text being part of the last + /// variable. + pub(super) fn choose(&self, table: usize, method: &Method, path: &str) -> Choice { + if let Some((rest, verb)) = split_verb(path) { + if let Some(found) = self.verb_match(path) { + let index = *found.value; + let bound = |binding: &Binding| { + binding.verb.as_ref().is_some_and(|own| { + own.raw == verb && (own.empty_ok || !rest.ends_with('/')) + }) + }; + if let Some(choice) = self.tables[index].choose(index, method, bound) { + return choice; + } + } + } + self.tables[table] + .choose(table, method, |binding| binding.verb.is_none()) + .unwrap_or(Choice::NotFound) } } @@ -63,6 +156,7 @@ impl PathTable { axum, captures, verb, + empty_last, .. } = mount; Self { @@ -71,20 +165,21 @@ impl PathTable { bindings: vec![Binding { entry, names: None, - verb, + verb: verb.map(|raw| Verb::new(raw, empty_last)), }], } } - /// Add `entry` mounted at `mount`, a path of this table's shape, unless a - /// binding with its verb already answers its method (a `custom` `*` rule - /// answers every one). + /// Add `entry` mounted at `mount`, a path of this table's shape, unless it + /// [`clash`]es with a binding already there. pub(super) fn add(&mut self, mount: MountedPath, entry: RouteEntry) { let taken = self.bindings.iter().any(|existing| { - existing.verb == mount.verb - && (existing.entry.http_method == entry.http_method - || existing.entry.http_method == RouteMethod::Any - || entry.http_method == RouteMethod::Any) + clash( + &existing.entry.http_method, + existing.verb.as_ref().map(|v| v.raw.as_str()), + &entry.http_method, + mount.verb.as_deref(), + ) }); if taken { tracing::error!( @@ -99,7 +194,7 @@ impl PathTable { self.bindings.push(Binding { entry, names, - verb: mount.verb, + verb: mount.verb.map(|raw| Verb::new(raw, mount.empty_last)), }); } @@ -108,45 +203,46 @@ impl PathTable { &self.bindings[index].entry } - /// Which binding answers `method` on `path`. Among the bindings whose - /// verb the path ends in, the longest verb wins, so a binding without one - /// takes only what no verb claims; within that, the request's own method - /// (or a `*` rule) before a GET binding answering HEAD. - pub(super) fn choose(&self, method: &Method, path: &str) -> Choice { - // (index, verb length, own method) of the best binding so far. - let mut best: Option<(usize, usize, bool)> = None; + /// Among the bindings `answers` admits, the one serving `method`: its own + /// (or a `custom` `*` rule) before a GET binding answering HEAD. `None` + /// when `answers` admits none. + fn choose( + &self, + table: usize, + method: &Method, + answers: impl Fn(&Binding) -> bool, + ) -> Option { + let mut head: Option = None; let mut answered = false; for (index, binding) in self.bindings.iter().enumerate() { - if !binding.answers(path) { + if !answers(binding) { continue; } answered = true; - let own = match &binding.entry.http_method { - RouteMethod::Any => true, - RouteMethod::One(bound) if bound == method => true, - // A GET binding answers HEAD too. + match &binding.entry.http_method { + RouteMethod::Any => return Some(Choice::Route { table, index }), + RouteMethod::One(bound) if bound == method => { + return Some(Choice::Route { table, index }) + } + // A GET binding answers HEAD too, after one bound to HEAD. RouteMethod::One(bound) if *bound == Method::GET && *method == Method::HEAD => { - false + head.get_or_insert(index); } - RouteMethod::One(_) => continue, - }; - let rank = (binding.verb_len(), own); - if best.is_none_or(|(_, len, best_own)| (len, best_own) < rank) { - best = Some((index, rank.0, rank.1)); + RouteMethod::One(_) => {} } } - match best { - Some((index, ..)) => Choice::Route(index), - None if answered => Choice::MethodNotAllowed(self.allow(path)), - None => Choice::NotFound, + match head { + Some(index) => Some(Choice::Route { table, index }), + None if answered => Some(Choice::MethodNotAllowed(self.allow(&answers))), + None => None, } } - /// The methods of the bindings answering `path`, in binding order, with + /// The methods of the bindings `answers` admits, in binding order, with /// HEAD after them when a GET binding answers it. - fn allow(&self, path: &str) -> HeaderValue { + fn allow(&self, answers: &impl Fn(&Binding) -> bool) -> HeaderValue { let mut methods: Vec<&str> = Vec::new(); - for binding in self.bindings.iter().filter(|b| b.answers(path)) { + for binding in self.bindings.iter().filter(|b| answers(b)) { let method = binding.entry.http_method.as_str(); if !methods.contains(&method) { methods.push(method); @@ -159,17 +255,15 @@ impl PathTable { .expect("method tokens are valid header value characters") } - /// Turn the path parameters the router matched on this table's path into - /// those of the binding at `index`: its verb taken off the last capture, - /// and the captures under its own names. A parameter the table's path does - /// not capture (a prefix the router is nested under) is left as it is. + /// Turn the path parameters matched on this table's path into those of + /// the binding at `index`: its verb taken off the last capture, and the + /// captures under its own names. A parameter the table's path does not + /// capture (a prefix the router is nested under) is left as it is. pub(super) fn bind_params(&self, index: usize, params: &mut PathParams) { let binding = &self.bindings[index]; if let (Some(verb), Some(last)) = (&binding.verb, self.captures.last()) { if let Some(value) = params.get_mut(last) { - // The verb ends the raw path and holds no escape, so the - // decoded value ends in it too. - if let Some(len) = value.strip_suffix(verb.as_str()).map(str::len) { + if let Some(len) = value.strip_suffix(verb.decoded.as_str()).map(str::len) { value.truncate(len); } } @@ -192,3 +286,16 @@ impl PathTable { } } } + +impl Verb { + fn new(raw: String, empty_ok: bool) -> Self { + let decoded = percent_encoding::percent_decode_str(&raw) + .decode_utf8_lossy() + .into_owned(); + Self { + raw, + decoded, + empty_ok, + } + } +} diff --git a/src/transcode/tests.rs b/src/transcode/tests.rs index a072bb2..81248e6 100644 --- a/src/transcode/tests.rs +++ b/src/transcode/tests.rs @@ -292,8 +292,8 @@ service S { let expected = [ ("GET", "/v1/ops/{name}"), ("POST", "/v1/ops/{name}"), - ("POST", "/v1/ops:batch"), ("POST", "/v1/ops/{id}"), + ("POST", "/v1/ops:batch"), ]; let expected: Vec<(String, String)> = expected .iter() @@ -322,12 +322,8 @@ service S { let paths: Vec<&str> = tables.iter().map(|t| t.path.as_str()).collect(); // The template the router cannot match is left out. assert_eq!(paths, ["/v1/ops/{name}"]); - let table = &tables[0]; - let post = match table.choose(&Method::POST, "/v1/ops/x") { - table::Choice::Route(index) => index, - _ => panic!("POST must be answered"), - }; - assert_eq!(table.entry(post).grpc_path.path(), "/t.S/Post"); + let built = table::Routes::new(tables); + assert_eq!(answer(&built, Method::POST, "/v1/ops/x"), "/t.S/Post"); // The router itself builds from them. let _router: Router> = routes(&pool, &[]); } @@ -350,35 +346,91 @@ service S { ); let tables = path_tables(&pool, &[], &TranscodeOptions::default()); assert_eq!(tables.len(), 1); - let table = &tables[0]; - let matched = || -> PathParams { - [("a", "first"), ("b", "second:go")] + let routes = table::Routes::new(tables); + let table = &routes.tables[0]; + let matched = |b: &str| -> PathParams { + [("a", "first"), ("b", b)] .into_iter() .map(|(k, v)| (k.to_owned(), v.to_owned())) .collect() }; - let post = match table.choose(&Method::POST, "/x/first/second:go") { - table::Choice::Route(index) => index, - _ => panic!("POST must be answered"), - }; - let mut params = matched(); + let post = bound(&routes, Method::POST, "/x/first/second:go"); + let mut params = matched("second:go"); table.bind_params(post, &mut params); assert_eq!(params.len(), 2); assert_eq!(params["b"], "first"); assert_eq!(params["a"], "second"); // The binding with the path's own names and no verb is left as matched. - let get = match table.choose(&Method::GET, "/x/first/second:go") { - table::Choice::Route(index) => index, - _ => panic!("GET must be answered"), - }; - let mut params = matched(); + let get = bound(&routes, Method::GET, "/x/first/second"); + let mut params = matched("second"); table.bind_params(get, &mut params); - assert_eq!(params, matched()); + assert_eq!(params, matched("second")); +} + +/// The index of the binding answering `method path`, in the routes' only table. +fn bound(routes: &table::Routes, method: Method, path: &str) -> usize { + match routes.choose(0, &method, path) { + table::Choice::Route { table: 0, index } => index, + _ => panic!("{method} {path} must be answered by the only table"), + } +} + +/// What answers `method path`: the gRPC path of the binding, or the status, +/// with the table the router itself would match it to. +fn answer(routes: &table::Routes, method: Method, path: &str) -> String { + let mut router = matchit::Router::new(); + for (index, table) in routes.tables.iter().enumerate() { + router.insert(table.path.as_str(), index).unwrap(); + } + let Ok(matched) = router.at(path) else { + return "no route".to_owned(); + }; + match routes.choose(*matched.value, &method, path) { + table::Choice::Route { table, index } => routes.tables[table] + .entry(index) + .grpc_path + .path() + .to_owned(), + table::Choice::MethodNotAllowed(allow) => format!("405 {}", allow.to_str().unwrap()), + table::Choice::NotFound => "404".to_owned(), + } +} + +/// The routes of an annotated `.proto` source. +fn routes_of(source: &'static str) -> table::Routes { + table::Routes::new(path_tables( + &api_pool(source), + &[], + &TranscodeOptions::default(), + )) } #[test] -fn head_falls_back_to_get_and_the_longest_verb_wins() { - let pool = api_pool( +fn the_verb_is_the_last_segment_from_its_first_colon() { + // Neither a variable's value nor a verb holds an unencoded colon + // (google/api/http.proto), so `x:b:c` is the variable `x` and the verb + // `:b:c`: bound for GET only, a POST is 405, not the `:c` binding. + let routes = routes_of( + r#"syntax = "proto3"; +package t; +import "google/api/annotations.proto"; +message Req { string name = 1; } +service S { + rpc Short(Req) returns (Req) { option (google.api.http) = { post: "/v1/{name}:c" }; } + rpc Long(Req) returns (Req) { option (google.api.http) = { get: "/v1/{name}:b:c" }; } +} +"#, + ); + assert_eq!(answer(&routes, Method::POST, "/v1/x:b:c"), "405 GET, HEAD"); + assert_eq!(answer(&routes, Method::GET, "/v1/x:b:c"), "/t.S/Long"); + assert_eq!(answer(&routes, Method::POST, "/v1/x:c"), "/t.S/Short"); + // No binding without a verb: an unbound verb has nothing to fall back on. + assert_eq!(answer(&routes, Method::POST, "/v1/x:d"), "404"); +} + +#[test] +fn head_falls_back_to_get_and_a_bound_verb_owns_its_url() { + let routes = routes_of( r#"syntax = "proto3"; package t; import "google/api/annotations.proto"; @@ -390,19 +442,16 @@ service S { } "#, ); - let tables = path_tables(&pool, &[], &TranscodeOptions::default()); - let table = &tables[0]; - let rpc = |method: Method, path: &str| match table.choose(&method, path) { - table::Choice::Route(index) => table.entry(index).grpc_path.path().to_owned(), - table::Choice::MethodNotAllowed(allow) => format!("405 {}", allow.to_str().unwrap()), - table::Choice::NotFound => "404".to_owned(), - }; + let rpc = |method: Method, path: &str| answer(&routes, method, path); assert_eq!(rpc(Method::HEAD, "/v1/x"), "/t.S/Get"); - // Both verbs end the path; the longer one is the binding's. assert_eq!(rpc(Method::POST, "/v1/x:b:c"), "/t.S/Long"); assert_eq!(rpc(Method::POST, "/v1/x:c"), "/t.S/Short"); assert_eq!(rpc(Method::POST, "/v1/x"), "405 GET, HEAD"); - assert_eq!(rpc(Method::PUT, "/v1/x:c"), "405 GET, POST, HEAD"); + // `:c` is bound: the binding without a verb does not take `x:c`. + assert_eq!(rpc(Method::PUT, "/v1/x:c"), "405 POST"); + assert_eq!(rpc(Method::GET, "/v1/x:c"), "405 POST"); + // `:z` is bound nowhere: it stays in the variable. + assert_eq!(rpc(Method::GET, "/v1/x:z"), "/t.S/Get"); } /// Regression for the axum 0.7→0.8 migration bug: `proto_path_to_axum` diff --git a/tests/custom_verbs.rs b/tests/custom_verbs.rs index 7e12378..69ed2e1 100644 --- a/tests/custom_verbs.rs +++ b/tests/custom_verbs.rs @@ -13,12 +13,14 @@ mod common; use std::convert::Infallible; use std::future::{ready, Ready}; use std::pin::Pin; +use std::sync::Arc; use std::task::{Context, Poll}; use axum::body::Body; use http::{Method, StatusCode}; use prost_reflect::{DescriptorPool, DynamicMessage, MessageDescriptor, Value as PbValue}; use serde_json::{json, Value}; +use structured_proxy::hooks::{ExtraRoute, ExtraRouteHandler, RouteRequest, RouteResponse}; use structured_proxy::transcode::codec::DynamicCodec; use tower::ServiceExt; @@ -58,6 +60,18 @@ service Ops { rpc Broken(Msg) returns (Msg) { option (google.api.http) = { get: "/v1/broken/a{name}b" }; } + rpc Make(Msg) returns (Msg) { + option (google.api.http) = { post: "/v3/x/{name}" }; + } + rpc Sweep(Msg) returns (Msg) { + option (google.api.http) = { post: "/v3/{operation}/{name}:sweep" }; + } + rpc Purge(Msg) returns (Msg) { + option (google.api.http) = { post: "/v4/{name=**}:purge" }; + } + rpc RunNow(Msg) returns (Msg) { + option (google.api.http) = { post: "/v2/jobs/{name}:run%3Anow" }; + } } "#; @@ -112,6 +126,16 @@ impl tower::Service> for Ops { } } +/// An embedder's extra route answering `pong`. +struct Ping; + +#[async_trait::async_trait] +impl ExtraRouteHandler for Ping { + async fn handle(&self, _req: RouteRequest) -> RouteResponse { + RouteResponse::new(StatusCode::OK, bytes::Bytes::from_static(b"pong")) + } +} + async fn proxy(upstream: common::Upstream) -> common::App { let pool: DescriptorPool = common::compile("test/v1/ops.proto", OPS_PROTO); let msg = pool.get_message_by_name("test.v1.Msg").unwrap(); @@ -225,13 +249,67 @@ async fn a_verb_with_an_empty_variable_does_not_match() { assert_eq!(status, StatusCode::METHOD_NOT_ALLOWED); } -async fn a_method_no_binding_of_the_url_answers_lists_those_that_do() { - // PUT is bound nowhere; GET and DELETE (without a verb) and POST - // (`:cancel`) answer this URL, POST `:archive` and `:pause` do not. +async fn a_bound_verb_owns_its_url_for_every_method() { + // `:cancel` is bound (POST), so `/v2/ops/op-1:cancel` names the verb, not + // an operation called `op-1:cancel`: the bindings without a verb do not + // answer it, whatever the method. + let app = proxy(UPSTREAM).await; + for method in [Method::GET, Method::PUT, Method::DELETE] { + let (status, allow, _) = call(&app, method.clone(), "/v2/ops/op-1:cancel").await; + assert_eq!(status, StatusCode::METHOD_NOT_ALLOWED, "{method}"); + assert_eq!(allow, "POST", "{method}"); + } +} + +async fn the_verb_starts_at_the_first_colon_of_the_last_segment() { + // A variable holds no unencoded colon (google/api/http.proto: a client + // percent-encodes it), so `op-1:b:cancel` is the verb `:b:cancel`, which + // no binding has; as an unbound verb it stays in the variable. let app = proxy(UPSTREAM).await; - let (status, allow, _) = call(&app, Method::PUT, "/v2/ops/op-1:cancel").await; + let (status, allow, _) = call(&app, Method::POST, "/v2/ops/op-1:b:cancel").await; assert_eq!(status, StatusCode::METHOD_NOT_ALLOWED); - assert_eq!(allow, "GET, POST, DELETE, HEAD"); + assert_eq!(allow, "GET, DELETE, HEAD"); + let (status, _, body) = call(&app, Method::GET, "/v2/ops/op-1:b:cancel").await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body["operation"], "op-1:b:cancel"); +} + +async fn a_verb_of_another_path_shape_is_found() { + // `/v3/x/{name}` and `/v3/{operation}/{name}:sweep`: the router prefers + // the static `x` for `/v3/x/y:sweep`, yet the bound verb belongs to the + // other path, which binds its own variables. + let app = proxy(UPSTREAM).await; + let (status, _, body) = call(&app, Method::POST, "/v3/x/y:sweep").await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body, json!({"name": "y", "operation": "x", "rpc": "Sweep"})); + let (status, _, body) = call(&app, Method::POST, "/v3/a/b:sweep").await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body, json!({"name": "b", "operation": "a", "rpc": "Sweep"})); + let (status, _, body) = call(&app, Method::POST, "/v3/x/y").await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body, json!({"name": "y", "operation": "", "rpc": "Make"})); +} + +async fn a_double_wildcard_before_a_verb_may_match_no_segment() { + // `**` matches zero or more segments (google/api/http.proto). + let app = proxy(UPSTREAM).await; + let (status, _, body) = call(&app, Method::POST, "/v4/:purge").await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body["rpc"], "Purge"); + assert_eq!(body["name"], ""); + let (status, _, body) = call(&app, Method::POST, "/v4/a/b:purge").await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body["name"], "a/b"); +} + +async fn an_encoded_character_in_the_verb_is_taken_off_decoded() { + // `:run%3Anow` keeps its reserved character encoded, as the template + // grammar asks; the variable gets the value without the decoded verb. + let app = proxy(UPSTREAM).await; + let (status, _, body) = call(&app, Method::POST, "/v2/jobs/j1:run%3Anow").await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body["rpc"], "RunNow"); + assert_eq!(body["name"], "j1"); } async fn verb_after_a_multi_segment_field_template_routes() { @@ -259,6 +337,31 @@ async fn verb_after_a_literal_routes_as_before() { } } +async fn an_extra_route_shares_a_transcoded_path_on_another_method() { + // The embedder's GET and the transcoded POST on one path both serve, as + // they did when each method had its own route. + let pool: DescriptorPool = common::compile("test/v1/ops.proto", OPS_PROTO); + let msg = pool.get_message_by_name("test.v1.Msg").unwrap(); + let app = common::app(UPSTREAM, Ops { msg }, |yaml| { + structured_proxy::ProxyServer::from_yaml_str(yaml) + .unwrap() + .with_descriptors(pool) + .with_extra_routes([ExtraRoute::new( + Method::GET, + "/v1/nodes:batch", + Arc::new(Ping), + )]) + }) + .await; + let request = http::Request::get("/v1/nodes:batch").body(Body::empty()).unwrap(); + let (status, body) = common::send(&app, request).await; + assert_eq!(status, StatusCode::OK); + assert_eq!(body, "pong"); + let (status, _, body) = call(&app, Method::POST, "/v1/nodes:batch").await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body["rpc"], "Batch"); +} + async fn a_template_the_router_cannot_serve_is_skipped_not_fatal() { // `a{name}b` puts text around a variable, which the router cannot // match; that binding is left out and every other one still serves. @@ -295,3 +398,32 @@ service Twice { .expect_err("a repeated method, path and verb must be rejected"); assert!(err.to_string().contains("more than one endpoint"), "{err}"); } + +#[tokio::test] +async fn a_star_rule_with_a_verb_starts_beside_another_verb() { + // A `custom` `*` rule answers every method, but only for its own verb, so + // it does not clash with a POST binding of another verb on its path. + const STAR_PROTO: &str = r#" +syntax = "proto3"; +package test.v1; +import "google/api/annotations.proto"; +message Msg { string name = 1; } +service Star { + rpc Inspect(Msg) returns (Msg) { + option (google.api.http) = { custom: { kind: "*" path: "/v1/x/{name}:inspect" } }; + } + rpc Cancel(Msg) returns (Msg) { option (google.api.http) = { post: "/v1/x/{name}:cancel" }; } +} +"#; + let pool = common::compile("test/v1/star.proto", STAR_PROTO); + let router = structured_proxy::ProxyServer::from_yaml_str( + "upstream:\n default: \"http://127.0.0.1:1\"\n", + ) + .unwrap() + .with_descriptors(pool) + .router(); + assert!( + router.is_ok(), + "bindings of different verbs share their path: {router:?}" + ); +} From acbbb1f4a1103e1377732adff6dd7cdb0967955d Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 7 Oct 2026 21:09:48 +0300 Subject: [PATCH 05/20] fix(transcode): keep exact and plain routes reachable beside verbs - Percent-escapes in a verb are compared case-insensitively on their hex digits (RFC 3986 6.2.2.1): `:run%3anow` matches a binding written `:run%3Anow`. - A path ending in a literal that the router matched answers its URL, so `/v1/jobs/special:cancel` is not taken over by `{name}:cancel`. - Bindings without a verb are found on the best path that has some: a path with only verb bindings no longer hides a less specific plain route (`GET /v1/jobs/x` reaches `/v1/{path=**}` beside `/v1/jobs/{id}:cancel`). - route_paths lists a path with a verb after its variable as `*`: it answers every method, so an extra route on that path is a startup error instead of taking a bound verb's URL past it (or panicking the merge when its variable is named differently). Regression tests cover each case and failed before the fix. --- README.md | 5 ++ src/transcode/mod.rs | 36 +++++++------ src/transcode/table.rs | 116 +++++++++++++++++++++++++++++++++++------ src/transcode/tests.rs | 76 ++++++++++++++++++++++++++- tests/custom_verbs.rs | 26 +++++++++ 5 files changed, 226 insertions(+), 33 deletions(-) diff --git a/README.md b/README.md index ebbcae4..166c9b4 100644 --- a/README.md +++ b/README.md @@ -393,8 +393,13 @@ do (`post: "/v1/{name=operations/*}:cancel"`): bindings of that verb answer it, each reaching its own RPC. A verb no binding binds stays part of the last variable (`/v1/items/a:b` is the item `a:b`), as Envoy's transcoder and grpc-gateway treat an unbound verb. +- A template ending in a literal matches its URL exactly and wins over a + variable with the same verb: `get: "/v1/jobs/special:cancel"` answers that + URL ahead of `post: "/v1/jobs/{name}:cancel"`, as a static route does. - `**` before a verb may match no segment: `/v1/{name=**}:purge` answers `/v1/:purge` with an empty `name`. +- A path with a verb after its variable answers every method, so an extra + route of yours on the same path stops the proxy at startup. - A URL that bindings answer, but none with the request's method, is `405`, with their methods in `Allow`; a path that no binding answers is `404`. diff --git a/src/transcode/mod.rs b/src/transcode/mod.rs index 3a377c0..a9c5394 100644 --- a/src/transcode/mod.rs +++ b/src/transcode/mod.rs @@ -364,9 +364,11 @@ async fn dispatch( Err(rejection) => return rejection.into_response(), }; if table != matched { - // A verb bound on another path the request matches: its captures come - // from that path, the router's prefix parameters stay. - if let Err(rejection) = rebind(&routes, matched, parts.uri.path(), &mut path_params) { + // Another path the request matches answers it (a verb bound there, or + // the only bindings without a verb): its captures come from that path, + // the router's prefix parameters stay. + if let Err(rejection) = rebind(&routes, matched, table, parts.uri.path(), &mut path_params) + { return rejection.into_response(routes.tables[table].entry(index)); } } @@ -403,11 +405,11 @@ async fn dispatch( } /// Replace the captures of the table at `matched` in `params` with those of -/// the table binding the verb `path` ends in, percent-decoded as the router -/// decodes its own. +/// the table at `chosen`, percent-decoded as the router decodes its own. fn rebind( routes: &Routes, matched: usize, + chosen: usize, path: &str, params: &mut PathParams, ) -> Result<(), Unmappable> { @@ -415,9 +417,9 @@ fn rebind( params.remove(name); } let found = routes - .verb_match(path) - .expect("the verb's table was chosen from this match"); - for (name, raw) in found.params.iter() { + .params_of(chosen, path) + .expect("the chosen table was found by matching this path"); + for (name, raw) in found.iter() { let value = percent_encoding::percent_decode_str(raw) .decode_utf8() .map_err(|_| Unmappable(format!("path parameter {name} is not valid UTF-8")))?; @@ -497,13 +499,13 @@ fn route_bindings( /// (e.g. a forward-auth endpoint). /// /// Each entry is `(method, path)` where `method` is the uppercase HTTP token, -/// or `*` for a `custom` rule that answers every method, so callers can -/// distinguish same-path/different-method routes from real conflicts. A -/// custom verb after a variable is matched by the transcoded routes, not by -/// the path, so the bindings of one path are listed as the route they share: -/// `*` once when a `custom` `*` rule is among them, else each method once. -/// Two bindings that cannot both serve (one method or a `*` rule, one verb) -/// are listed both, so the caller sees the collision. +/// or `*` for a route that answers every method, so callers can distinguish +/// same-path/different-method routes from real conflicts. The bindings of one +/// path are listed as the route they share: `*` once when a `custom` `*` rule +/// or a custom verb after a variable is among them (the transcoded routes +/// match the verb, and answer a bound verb's other methods with 405), else +/// each method once. Two bindings that cannot both serve (one method or a `*` +/// rule, one verb) are listed both, so the caller sees the collision. pub fn route_paths( pool: &DescriptorPool, aliases: &[AliasConfig], @@ -524,9 +526,11 @@ pub fn route_paths( let mut paths = Vec::new(); for bindings in &shapes { let listed = |method: &str, path: &str| (method.to_owned(), path.to_owned()); + // A path with a verb after its variable answers every method: one + // whose verb is bound only for others is told so with 405. match bindings .iter() - .find(|b| b.entry.http_method == RouteMethod::Any) + .find(|b| b.entry.http_method == RouteMethod::Any || b.mount.verb.is_some()) { Some(star) => paths.push(listed("*", &star.mount.axum)), None => { diff --git a/src/transcode/table.rs b/src/transcode/table.rs index 816293e..32e3444 100644 --- a/src/transcode/table.rs +++ b/src/transcode/table.rs @@ -10,6 +10,7 @@ //! the last variable, as Envoy's transcoder and grpc-gateway leave an unbound //! verb. +use std::borrow::Cow; use std::collections::HashMap; use axum::http::{HeaderValue, Method}; @@ -25,6 +26,10 @@ pub(super) struct Routes { /// request to one path whatever its verb, while the verb may be bound on /// a path of another shape the request also matches. verbs: HashMap>, + /// The paths of the tables with a binding without a verb: a path whose + /// bindings all carry verbs must not hide one of these from a request + /// that names no bound verb. + plain: matchit::Router, } /// Every binding the router serves at one path. @@ -47,8 +52,8 @@ struct Binding { /// A custom verb after the last capture. struct Verb { - /// As written in the template, `:` included: compared with the request's - /// path as received. + /// As written in the template, `:` included, escapes in upper case: + /// compared with the request's path as received, likewise normalized. raw: String, /// `raw` percent-decoded: what ends the decoded value of the capture. decoded: String, @@ -75,6 +80,35 @@ pub(super) fn split_verb(path: &str) -> Option<(&str, &str)> { Some(path.split_at(colon)) } +/// `text` with the hex digits of its percent-escapes in upper case: `%3a` and +/// `%3A` are one octet (RFC 3986 §6.2.2.1). Borrowed when nothing changes. +pub(super) fn normalize_escapes(text: &str) -> Cow<'_, str> { + let lower_hex = |b: &u8| b.is_ascii_hexdigit() && b.is_ascii_lowercase(); + let bytes = text.as_bytes(); + let lower_escape = + |at: usize| bytes[at] == b'%' && bytes[at + 1..].iter().take(2).any(lower_hex); + if !(0..bytes.len()).any(lower_escape) { + return Cow::Borrowed(text); + } + let mut normalized = String::with_capacity(text.len()); + let mut chars = text.chars(); + while let Some(ch) = chars.next() { + normalized.push(ch); + if ch == '%' { + for digit in chars.by_ref().take(2) { + // Only a hex digit is part of the escape; anything else is + // kept as it is, so a malformed escape is not rewritten. + normalized.push(if digit.is_ascii_hexdigit() { + digit.to_ascii_uppercase() + } else { + digit + }); + } + } + } + Cow::Owned(normalized) +} + /// Whether two bindings of one path cannot both serve: the same verb and the /// same method, or the same verb and a `custom` `*` rule, which answers every /// method. @@ -84,7 +118,7 @@ pub(super) fn clash( other_method: &RouteMethod, other_verb: Option<&str>, ) -> bool { - verb == other_verb + verb.map(normalize_escapes) == other_verb.map(normalize_escapes) && (method == other_method || *method == RouteMethod::Any || *other_method == RouteMethod::Any) @@ -96,7 +130,12 @@ impl Routes { /// some of them accepts it too. pub(super) fn new(tables: Vec) -> Self { let mut verbs: HashMap> = HashMap::new(); + let mut plain = matchit::Router::new(); for (index, table) in tables.iter().enumerate() { + if table.bindings.iter().any(|b| b.verb.is_none()) { + let inserted = plain.insert(table.path.as_str(), index); + debug_assert!(inserted.is_ok(), "{}: {inserted:?}", table.path); + } let mut seen: Vec<&str> = Vec::new(); for verb in table.bindings.iter().filter_map(|b| b.verb.as_ref()) { if seen.contains(&verb.raw.as_str()) { @@ -110,29 +149,60 @@ impl Routes { debug_assert!(inserted.is_ok(), "{}: {inserted:?}", table.path); } } - Self { tables, verbs } + Self { + tables, + verbs, + plain, + } } /// The table binding the verb `path` ends in, when one of the request's /// path does: its index and the router's match, whose captures are that /// table's. - pub(super) fn verb_match<'r, 'p>( + fn verb_match<'r, 'p>(&'r self, path: &'p str) -> Option> { + let (_, verb) = split_verb(path)?; + self.verbs + .get(normalize_escapes(verb).as_ref())? + .at(path) + .ok() + } + + /// The router's match of `path` on the table at `table`, which [`choose`] + /// picked for it: its captures, still percent-encoded. + /// + /// [`choose`]: Self::choose + pub(super) fn params_of<'r, 'p>( &'r self, + table: usize, path: &'p str, - ) -> Option> { - let (_, verb) = split_verb(path)?; - self.verbs.get(verb)?.at(path).ok() + ) -> Option> { + [self.verb_match(path), self.plain.at(path).ok()] + .into_iter() + .flatten() + .find(|found| *found.value == table) + .map(|found| found.params) } /// Which binding answers `method` on `path`, a request the router matched - /// to the table at `table`. A verb some table binds for this path owns the - /// URL: only the bindings of that verb answer it. Otherwise the bindings - /// without a verb of `table` do, the verb text being part of the last - /// variable. + /// to the table at `table`. + /// + /// - A path ending in a literal matched exactly: its bindings answer, as a + /// static route wins over a variable everywhere. + /// - A verb some table binds for this path owns the URL: only the + /// bindings of that verb answer it. + /// - Otherwise the bindings without a verb answer, of the best path that + /// has some, the verb text being part of the last variable. pub(super) fn choose(&self, table: usize, method: &Method, path: &str) -> Choice { + let plain = |binding: &Binding| binding.verb.is_none(); + if self.tables[table].literal_end() { + return self.tables[table] + .choose(table, method, plain) + .unwrap_or(Choice::NotFound); + } if let Some((rest, verb)) = split_verb(path) { if let Some(found) = self.verb_match(path) { let index = *found.value; + let verb = normalize_escapes(verb); let bound = |binding: &Binding| { binding.verb.as_ref().is_some_and(|own| { own.raw == verb && (own.empty_ok || !rest.ends_with('/')) @@ -143,8 +213,18 @@ impl Routes { } } } - self.tables[table] - .choose(table, method, |binding| binding.verb.is_none()) + // The router's own table, when it has such bindings, is also the best + // of those that have: no second lookup for the common request. + let index = if self.tables[table].bindings.iter().any(plain) { + table + } else { + match self.plain.at(path) { + Ok(found) => *found.value, + Err(_) => return Choice::NotFound, + } + }; + self.tables[index] + .choose(index, method, plain) .unwrap_or(Choice::NotFound) } } @@ -198,6 +278,12 @@ impl PathTable { }); } + /// Whether the path ends in a literal (a verb after a literal is part of + /// it), so the router matched the request's last segment exactly. + fn literal_end(&self) -> bool { + !self.path.ends_with('}') + } + /// The route entry of the binding at `index`. pub(super) fn entry(&self, index: usize) -> &RouteEntry { &self.bindings[index].entry @@ -293,7 +379,7 @@ impl Verb { .decode_utf8_lossy() .into_owned(); Self { - raw, + raw: normalize_escapes(&raw).into_owned(), decoded, empty_ok, } diff --git a/src/transcode/tests.rs b/src/transcode/tests.rs index 81248e6..f935f4e 100644 --- a/src/transcode/tests.rs +++ b/src/transcode/tests.rs @@ -289,9 +289,10 @@ service S { "#, ); let paths = route_paths(&pool, &[], &RpcSelection::default()); + // A path with verbs answers every method (405 for those its verb is not + // bound to), so it is listed as `*`. let expected = [ - ("GET", "/v1/ops/{name}"), - ("POST", "/v1/ops/{name}"), + ("*", "/v1/ops/{name}"), ("POST", "/v1/ops/{id}"), ("POST", "/v1/ops:batch"), ]; @@ -428,6 +429,77 @@ service S { assert_eq!(answer(&routes, Method::POST, "/v1/x:d"), "404"); } +#[test] +fn a_verb_matches_whatever_the_case_of_its_escapes() { + // `%3A` and `%3a` are the same octet (RFC 3986 §6.2.2.1). + let routes = routes_of( + r#"syntax = "proto3"; +package t; +import "google/api/annotations.proto"; +message Req { string name = 1; } +service S { + rpc Get(Req) returns (Req) { option (google.api.http) = { get: "/v1/{name}" }; } + rpc RunNow(Req) returns (Req) { option (google.api.http) = { post: "/v1/{name}:run%3anow" }; } +} +"#, + ); + assert_eq!( + answer(&routes, Method::POST, "/v1/job:run%3Anow"), + "/t.S/RunNow" + ); + assert_eq!( + answer(&routes, Method::POST, "/v1/job:run%3anow"), + "/t.S/RunNow" + ); +} + +#[test] +fn a_literal_route_keeps_its_url_over_a_verb_after_a_variable() { + // The router picks the exact literal for `/v1/jobs/special:cancel`; that + // route answers it, as a static route wins over a variable everywhere. + let routes = routes_of( + r#"syntax = "proto3"; +package t; +import "google/api/annotations.proto"; +message Req { string name = 1; } +service S { + rpc Special(Req) returns (Req) { option (google.api.http) = { get: "/v1/jobs/special:cancel" }; } + rpc Cancel(Req) returns (Req) { option (google.api.http) = { post: "/v1/jobs/{name}:cancel" }; } +} +"#, + ); + assert_eq!( + answer(&routes, Method::GET, "/v1/jobs/special:cancel"), + "/t.S/Special" + ); + assert_eq!( + answer(&routes, Method::POST, "/v1/jobs/other:cancel"), + "/t.S/Cancel" + ); +} + +#[test] +fn a_path_with_only_verb_bindings_does_not_hide_a_plain_route() { + // `/v1/jobs/{id}` binds `:cancel` only; `GET /v1/jobs/x` has no verb, so + // the catch-all GET, which the router ranks lower, answers it. + let routes = routes_of( + r#"syntax = "proto3"; +package t; +import "google/api/annotations.proto"; +message Req { string id = 1; string path = 2; } +service S { + rpc Cancel(Req) returns (Req) { option (google.api.http) = { post: "/v1/jobs/{id}:cancel" }; } + rpc Files(Req) returns (Req) { option (google.api.http) = { get: "/v1/{path=**}" }; } +} +"#, + ); + assert_eq!(answer(&routes, Method::GET, "/v1/jobs/x"), "/t.S/Files"); + assert_eq!( + answer(&routes, Method::POST, "/v1/jobs/x:cancel"), + "/t.S/Cancel" + ); +} + #[test] fn head_falls_back_to_get_and_a_bound_verb_owns_its_url() { let routes = routes_of( diff --git a/tests/custom_verbs.rs b/tests/custom_verbs.rs index 69ed2e1..04d1f1f 100644 --- a/tests/custom_verbs.rs +++ b/tests/custom_verbs.rs @@ -399,6 +399,32 @@ service Twice { assert!(err.to_string().contains("more than one endpoint"), "{err}"); } +#[tokio::test] +async fn an_extra_route_on_a_path_with_verbs_is_refused_at_startup() { + // The transcoded route of `/v1/x/{name}` answers every method of a URL + // ending in a bound verb; an extra GET there would take `GET + // /v1/x/a:cancel` past it. + const VERB_PROTO: &str = r#" +syntax = "proto3"; +package test.v1; +import "google/api/annotations.proto"; +message Msg { string name = 1; } +service Verb { + rpc Cancel(Msg) returns (Msg) { option (google.api.http) = { post: "/v1/x/{name}:cancel" }; } +} +"#; + let pool = common::compile("test/v1/verb.proto", VERB_PROTO); + let err = structured_proxy::ProxyServer::from_yaml_str( + "upstream:\n default: \"http://127.0.0.1:1\"\n", + ) + .unwrap() + .with_descriptors(pool) + .with_extra_routes([ExtraRoute::new(Method::GET, "/v1/x/{id}", Arc::new(Ping))]) + .router() + .expect_err("an extra route on a path with verbs must be rejected"); + assert!(err.to_string().contains("more than one endpoint"), "{err}"); +} + #[tokio::test] async fn a_star_rule_with_a_verb_starts_beside_another_verb() { // A `custom` `*` rule answers every method, but only for its own verb, so From e1fff556d450b65fbc79fa69cc389fd82a1f3d97 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 7 Oct 2026 21:41:05 +0300 Subject: [PATCH 06/20] fix(transcode): enforce multi-segment field templates - A variable bound to a multi-segment template at the end of a path (`{name=publishers/*/books/*}`) is mounted as a catch-all; its value is now checked against the template (literals, one segment per `*`, any number for `**`), so `/v1/anything/else:restore` no longer reaches the RPC. Bindings of one path with different templates both mount. - Guards learn the methods of the transcoded bindings directly: a path with verbs is reserved as `*`, which hid an extension method such as PROPFIND from scope validation. - OpenAPI leaves out a template the router cannot mount, as the router does, instead of documenting a URL that is always 404. Regression tests cover each case and failed before the fix. --- README.md | 4 ++ src/lib.rs | 27 +++++---- src/openapi.rs | 6 ++ src/openapi/tests.rs | 16 +++++ src/transcode/mod.rs | 42 ++++++++++--- src/transcode/path.rs | 51 ++++++++++------ src/transcode/table.rs | 133 ++++++++++++++++++++++++++++++++++------- src/transcode/tests.rs | 33 ++++++++++ tests/custom_verbs.rs | 36 +++++++++++ 9 files changed, 290 insertions(+), 58 deletions(-) diff --git a/README.md b/README.md index 166c9b4..9b98054 100644 --- a/README.md +++ b/README.md @@ -381,6 +381,10 @@ body, as the route's `google.api.http` rule says: A value that is not valid for its field, or two members of one `oneof`, is answered with `INVALID_ARGUMENT` (400) before the upstream is called. +A variable bound to a multi-segment template at the end of a path +(`{name=shelves/*/books/*}`, AIP-127) takes only a value of that shape: its +literals, one segment per `*`, any number for `**`. + **Custom verbs.** A path template may end in a verb, as AIP-136 custom methods do (`post: "/v1/{name=operations/*}:cancel"`): diff --git a/src/lib.rs b/src/lib.rs index 1c8c351..00fc1fe 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -841,7 +841,20 @@ impl ProxyServer { let forward_auth = auth.as_ref().and_then(|built| { auth::forward::ForwardAuth::build(self.config.auth.as_ref()?, built.clone()) }); - let guards = Arc::new(self.guards(resolver, auth, maintenance_exempt, &mounted)?); + // The methods a route answers: `*` marks a route that answers every + // method, not a method, and a transcoded path with verbs is listed as + // `*` while its bindings still name theirs. + let mut routed: Vec = mounted + .iter() + .filter(|(method, _)| method != "*") + .filter_map(|(method, _)| http::Method::from_bytes(method.as_bytes()).ok()) + .collect(); + routed.extend(transcode::bound_methods( + &pool, + &self.config.aliases, + &selection, + )); + let guards = Arc::new(self.guards(resolver, auth, maintenance_exempt, &routed)?); // Health routes. Paths are configurable; the whole group is skippable. let health_routes = if self.config.health.enabled { @@ -984,7 +997,7 @@ impl ProxyServer { /// The guards the configuration and the hooks turn on, each with its /// scope, after the client-address resolution `resolver`; /// `maintenance_exempt` lists the paths maintenance mode leaves - /// reachable, `mounted` the `(method, path)` of every route. + /// reachable, `routed` the methods the routes answer. /// /// # Errors /// @@ -996,17 +1009,11 @@ impl ProxyServer { resolver: Arc, auth: Option>, maintenance_exempt: Vec, - mounted: &[(String, String)], + routed: &[http::Method], ) -> anyhow::Result { use config::Traffic::{Endpoints, Grpc, Transcoded}; - // `*` marks a route that answers every method, not a method. - let routed: Vec = mounted - .iter() - .filter(|(method, _)| method != "*") - .filter_map(|(method, _)| http::Method::from_bytes(method.as_bytes()).ok()) - .collect(); let scope = |config: Option<&ScopeConfig>, default: &[config::Traffic], what: &str| { - guard::Scope::compile(config, default, what, &routed).map_err(anyhow::Error::msg) + guard::Scope::compile(config, default, what, routed).map_err(anyhow::Error::msg) }; // A guard never sets what the client-address forwarding writes. let reserved: Arc<[http::HeaderName]> = resolver.configured_headers().cloned().collect(); diff --git a/src/openapi.rs b/src/openapi.rs index a3722a8..8f14e8c 100644 --- a/src/openapi.rs +++ b/src/openapi.rs @@ -12,6 +12,7 @@ use serde_json::{json, Map, Value}; use crate::config::{AliasConfig, OpenApiConfig}; use crate::transcode::httpbody; +use crate::transcode::path; use crate::transcode::request::BodyMapping; use crate::transcode::rule::{self, HttpBinding, RouteMethod}; use crate::transcode::RpcSelection; @@ -86,6 +87,11 @@ pub fn generate( } } for path in targets { + // The router leaves out a template it cannot match; + // documenting it would promise a URL that is 404. + if path::mountable(&path::MountedPath::new(&path).axum).is_err() { + continue; + } let mut operation = operation.clone(); if http_method == "head" { strip_response_content(&mut operation); diff --git a/src/openapi/tests.rs b/src/openapi/tests.rs index 4c433bb..46c1ab1 100644 --- a/src/openapi/tests.rs +++ b/src/openapi/tests.rs @@ -187,6 +187,9 @@ service Api { rpc Archive(Item) returns (Item) { option (google.api.http) = { post: "/v1/items/{name}:archive" }; } + rpc Broken(Item) returns (Item) { + option (google.api.http) = { get: "/v1/broken/a{name}b" }; + } } "#; @@ -242,6 +245,19 @@ fn custom_verb_after_a_variable_keeps_its_path_and_parameter() { assert!(spec["paths"]["/v1/items/{name}"].get("post").is_none()); } +#[test] +fn a_template_the_router_cannot_serve_is_not_documented() { + // The router skips `a{name}b`; the spec must not promise it either. + let spec = spec(&[]); + let paths = spec["paths"].as_object().unwrap(); + assert!(!paths.contains_key("/v1/broken/a{name}b"), "{paths:?}"); + assert!(!paths.values().any(|item| item + .as_object() + .unwrap() + .values() + .any(|op| op["operationId"] == "Api.Broken"))); +} + #[test] fn star_rule_is_listed_under_every_method_with_distinct_ids() { let spec = spec(&[]); diff --git a/src/transcode/mod.rs b/src/transcode/mod.rs index a9c5394..28c4230 100644 --- a/src/transcode/mod.rs +++ b/src/transcode/mod.rs @@ -14,7 +14,7 @@ pub mod codec; pub mod error; pub(crate) mod httpbody; pub mod metadata; -mod path; +pub(crate) mod path; pub mod request; pub(crate) mod response; pub(crate) mod rule; @@ -453,6 +453,16 @@ struct RouteBinding { mount: MountedPath, } +impl RouteBinding { + fn claim(&self) -> table::Claim<'_> { + table::Claim { + method: &self.entry.http_method, + verb: self.mount.verb.as_deref(), + template: self.mount.last_template.as_deref(), + } + } +} + /// The single source of truth for what [`routes`] mounts: every binding of /// every unary and server-streaming RPC, plus its config aliases, less the /// templates the router cannot match. Both [`routes`] (to build handlers) and @@ -545,14 +555,9 @@ pub fn route_paths( } } for (at, binding) in bindings.iter().enumerate() { - let clashes = bindings[..at].iter().any(|earlier| { - table::clash( - &earlier.entry.http_method, - earlier.mount.verb.as_deref(), - &binding.entry.http_method, - binding.mount.verb.as_deref(), - ) - }); + let clashes = bindings[..at] + .iter() + .any(|earlier| table::clash(&earlier.claim(), &binding.claim())); if clashes { paths.push(listed( binding.entry.http_method.as_str(), @@ -564,6 +569,25 @@ pub fn route_paths( paths } +/// The methods the transcoded bindings for this pool, aliases and selection +/// answer, each once: what [`route_paths`] lists under `*` for a path with +/// verbs still names a method a route answers. +pub(crate) fn bound_methods( + pool: &DescriptorPool, + aliases: &[AliasConfig], + selection: &RpcSelection, +) -> Vec { + let mut methods: Vec = Vec::new(); + for binding in route_bindings(pool, aliases, selection) { + if let RouteMethod::One(method) = binding.entry.http_method { + if !methods.contains(&method) { + methods.push(method); + } + } + } + methods +} + /// JSON serialization options shared by the unary and streaming response paths, /// so a given message serializes identically regardless of RPC kind. fn response_serialize_options() -> SerializeOptions { diff --git a/src/transcode/path.rs b/src/transcode/path.rs index d9dab1d..a2c99c5 100644 --- a/src/transcode/path.rs +++ b/src/transcode/path.rs @@ -23,6 +23,12 @@ pub(crate) struct MountedPath { /// Whether the last capture may be empty: a `**` matches zero or more /// segments (google/api/http.proto), any other variable at least one. pub(crate) empty_last: bool, + /// The field template of the last capture when the router cannot hold it + /// (`{name=shelves/*/books/*}` mounted as a catch-all): its segments, which + /// the matched value must follow. + pub(crate) last_template: Option>, + /// The index of the last segment, the one the last capture starts at. + pub(crate) last_segment: usize, } impl MountedPath { @@ -35,6 +41,14 @@ impl MountedPath { let mut shape = String::with_capacity(base.len()); let mut captures = Vec::new(); let empty_last = is_double_wildcard(segments[last]); + let last_template = field_template(segments[last]) + .filter(|template| *template != "*" && *template != "**") + .map(|template| { + split_top_level(template) + .into_iter() + .map(str::to_owned) + .collect() + }); for (idx, segment) in segments.iter().enumerate() { if idx > 0 { axum.push('/'); @@ -62,6 +76,8 @@ impl MountedPath { captures, verb: verb.map(str::to_owned), empty_last, + last_template, + last_segment: last, } } @@ -140,12 +156,17 @@ fn split_verb(template: &str) -> (&str, Option<&str>) { /// Whether `segment` is `**` or a variable bound to it (`{name=**}`). fn is_double_wildcard(segment: &str) -> bool { - segment == "**" - || segment - .strip_prefix('{') - .and_then(|s| s.strip_suffix('}')) - .and_then(|inner| inner.split_once('=')) - .is_some_and(|(_, template)| template == "**") + segment == "**" || field_template(segment) == Some("**") +} + +/// The template a variable segment binds its field to: `shelves/*` of +/// `{name=shelves/*}`, none for a plain `{name}` or a literal. +fn field_template(segment: &str) -> Option<&str> { + segment + .strip_prefix('{') + .and_then(|s| s.strip_suffix('}')) + .and_then(|inner| inner.split_once('=')) + .map(|(_, template)| template) } /// The index of the `}` closing the `{` that opens `segment`. @@ -214,18 +235,12 @@ fn convert_segment(segment: &str, idx: usize, is_last: bool) -> Segment<'_> { // Multi-segment catch-all maps to axum's `{*name}` (terminal only). "**" => catch_all(name, is_last), // Templates with interspersed literals (`{name=shelves/*/books/*}`) - // have no faithful axum form: axum cannot bind literal segments - // into one capture. Collapse to a catch-all so routing stays - // deterministic and the field still binds to the matched tail, - // and warn so the limitation surfaces instead of mis-routing. - _ => { - tracing::warn!( - template = %inner, - "google.api.http multi-segment field template is not fully \ - supported; routing it as a catch-all capture" - ); - catch_all(name, is_last) - } + // have no axum form: axum cannot bind literal segments into one + // capture. The last segment is mounted as a catch-all and the + // transcoded routes check the value against the template; in + // any other position it degrades to one segment, which the + // template cannot match, so that is warned about. + _ => catch_all(name, is_last), }; } // Plain `{name}` is already valid axum 0.8 syntax. diff --git a/src/transcode/table.rs b/src/transcode/table.rs index 32e3444..bf139a9 100644 --- a/src/transcode/table.rs +++ b/src/transcode/table.rs @@ -48,6 +48,64 @@ struct Binding { /// differ from the path's own. names: Option>, verb: Option, + template: Option