fix(transcode): route custom verbs after a path variable

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 <https://sw.foundation/cla>. 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).

Cargo.toml

@@ -60,6 +60,13 @@ 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"
+# 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

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,47 @@ 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 (`{name=shelves/*/books/*}`,
+AIP-127) takes only a value of that shape: its literals, one segment per `*`,
+any number for `**`. `**` must be the last part of the path: a template that
+uses it before the last segment, or puts another segment after it, is left
+out with an error in the log. Of the bindings of one router path, those such
+a template or an escaped literal constrains are tried before the open ones. The value of such a variable, and of `{name=**}`, is
+percent-decoded except for `%2F`, which reaches the field as received, so an
+encoded slash stays distinct from a segment boundary; a single-segment
+variable decodes it. A literal of such a template that holds a percent-escape
+matches whatever the case of its hex digits, so to the router it is a
+variable: like any variable, it cannot share its position with a `**` of
+another binding, and the later of the two is left out with an error.
+
+**Custom verbs.** A path template may end in a verb, as AIP-136 custom methods
+do (`post: "/v1/{name=operations/*}:cancel"`):
+
+- 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.
+- 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 URL that no binding answers, its field
+  template or verb not fitting, is no route of the proxy: `404`, or your
+  fallback's.
+
+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
@@ -1181,7 +1223,8 @@ A plain TCP connection passes `stream.connect_info()` directly
 ### What passes through
 
 A request no route matches gets `404`, or goes to your own service with
-`ProxyService::with_fallback(my_axum_app)`. Native gRPC calls and the fallback
+`ProxyService::with_fallback(my_axum_app)`; so does a URL whose path a
+transcoded route matches but no binding answers. Native gRPC calls and the fallback
 pass only the guards whose scope names `grpc` or `fallback` (see
 [Guards and scopes](#guards-and-scopes)); by default none does. The fallback
 keeps its own CORS and tracing.
@@ -1467,3 +1510,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).

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.

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<SocketAddr>,

src/guard.rs

@@ -140,7 +140,7 @@ pub(crate) struct Scope {
 impl Scope {
     /// Compile `config` for the guard named `what`, with `default` traffic
     /// when the config names none; `routed` are the methods the proxy's
-    /// routes answer beyond the standard ones (`custom` rules, extra routes).
+    /// routes answer beyond the standard ones.
     ///
     /// # Errors
     /// A scope that covers no traffic, a path glob that is relative or does
@@ -150,7 +150,7 @@ impl Scope {
         config: Option<&ScopeConfig>,
         default: &[Traffic],
         what: &str,
-        routed: &[Method],
+        routed: Routed<'_>,
     ) -> Result<Arc<Self>, String> {
         let traffic = config.and_then(|c| c.traffic.as_deref()).unwrap_or(default);
         let classes = traffic.iter().fold(0, |acc, t| acc | traffic_bits(*t));
@@ -232,6 +232,16 @@ const STANDARD_METHODS: [Method; 9] = [
     Method::PATCH,
 ];
 
+/// The methods the proxy's routes answer beyond the standard ones.
+#[derive(Clone, Copy)]
+pub(crate) enum Routed<'a> {
+    /// These: `custom` rules, extra routes.
+    Methods(&'a [Method]),
+    /// Every method: a route answers them all (a `custom` `*` rule, the
+    /// verify endpoint).
+    Every,
+}
+
 /// One `scope.methods` entry of the guard `what`: a method some request can
 /// carry, or an error. A method no route answers would match no request and
 /// leave the guard covering nothing, unless the scope covers the fallback,
@@ -240,7 +250,7 @@ fn scope_method(
     name: &str,
     what: &str,
     covers_fallback: bool,
-    routed: &[Method],
+    routed: Routed<'_>,
 ) -> Result<Method, String> {
     if name == "*" {
         return Err(format!(
@@ -249,7 +259,11 @@ fn scope_method(
     }
     let method = Method::from_bytes(name.to_ascii_uppercase().as_bytes())
         .map_err(|_| format!("{what}.scope.methods entry {name:?} is not a method"))?;
-    if covers_fallback || STANDARD_METHODS.contains(&method) || routed.contains(&method) {
+    let answered = match routed {
+        Routed::Methods(methods) => methods.contains(&method),
+        Routed::Every => true,
+    };
+    if covers_fallback || STANDARD_METHODS.contains(&method) || answered {
         Ok(method)
     } else {
         Err(format!(

src/guard/tests.rs

@@ -1,6 +1,9 @@
 use super::*;
 use crate::upstream::GrpcProtocol;
 
+/// No route answering a method beyond the standard ones.
+const STANDARD: Routed<'static> = Routed::Methods(&[]);
+
 fn maintenance() -> Maintenance {
     Maintenance {
         exempt: vec![
@@ -45,7 +48,13 @@ fn request(method: &str, path: &str) -> http::Request<()> {
 
 #[test]
 fn a_scope_without_traffic_takes_the_default() {
-    let scope = Scope::compile(None, &[Traffic::Transcoded, Traffic::Grpc], "shield", &[]).unwrap();
+    let scope = Scope::compile(
+        None,
+        &[Traffic::Transcoded, Traffic::Grpc],
+        "shield",
+        STANDARD,
+    )
+    .unwrap();
     assert!(scope.covers(Class::Transcoded));
     assert!(scope.covers(Class::Grpc));
     assert!(!scope.covers(Class::Endpoints));
@@ -56,7 +65,7 @@ fn a_scope_without_traffic_takes_the_default() {
 #[test]
 fn all_traffic_covers_every_class() {
     let config = ScopeConfig::traffic([Traffic::All]);
-    let scope = Scope::compile(Some(&config), &[], "shield", &[]).unwrap();
+    let scope = Scope::compile(Some(&config), &[], "shield", STANDARD).unwrap();
     for class in [
         Class::Transcoded,
         Class::Endpoints,
@@ -72,7 +81,7 @@ fn all_traffic_covers_every_class() {
 fn a_scope_that_covers_no_traffic_is_an_error() {
     // An empty list would mount the guard nowhere; a typo, not a choice.
     let config = ScopeConfig::traffic([]);
-    let err = Scope::compile(Some(&config), &[Traffic::Transcoded], "auth", &[]).unwrap_err();
+    let err = Scope::compile(Some(&config), &[Traffic::Transcoded], "auth", STANDARD).unwrap_err();
     assert!(err.contains("auth.scope.traffic"), "{err}");
 }
 
@@ -82,14 +91,16 @@ fn a_relative_or_invalid_path_glob_is_an_error() {
         paths: vec!["v1/**".into()],
         ..ScopeConfig::default()
     };
-    let err = Scope::compile(Some(&relative), &[Traffic::Transcoded], "shield", &[]).unwrap_err();
+    let err =
+        Scope::compile(Some(&relative), &[Traffic::Transcoded], "shield", STANDARD).unwrap_err();
     assert!(err.contains("must start with '/'"), "{err}");
 
     let invalid = ScopeConfig {
         paths: vec!["/v1/[".into()],
         ..ScopeConfig::default()
     };
-    let err = Scope::compile(Some(&invalid), &[Traffic::Transcoded], "shield", &[]).unwrap_err();
+    let err =
+        Scope::compile(Some(&invalid), &[Traffic::Transcoded], "shield", STANDARD).unwrap_err();
     assert!(err.contains("is invalid"), "{err}");
 }
 
@@ -99,7 +110,8 @@ fn an_invalid_method_is_an_error() {
         methods: vec!["GE T".into()],
         ..ScopeConfig::default()
     };
-    let err = Scope::compile(Some(&config), &[Traffic::Transcoded], "shield", &[]).unwrap_err();
+    let err =
+        Scope::compile(Some(&config), &[Traffic::Transcoded], "shield", STANDARD).unwrap_err();
     assert!(err.contains("is not a method"), "{err}");
 }
 
@@ -115,23 +127,26 @@ fn a_catch_all_method_is_an_error() {
     // `*` means every method in route policies; here it would match no
     // request and leave the guard covering nothing.
     let config = methods(&["*"], Traffic::Transcoded);
-    let err = Scope::compile(Some(&config), &[], "auth", &[]).unwrap_err();
+    let err = Scope::compile(Some(&config), &[], "auth", STANDARD).unwrap_err();
     assert!(err.contains("leave methods out"), "{err}");
 }
 
 #[test]
 fn a_method_no_route_answers_is_an_error() {
     // A typo matches no request, and the guard would silently cover nothing.
     let config = methods(&["PSOT"], Traffic::Transcoded);
-    let err = Scope::compile(Some(&config), &[], "shield", &[]).unwrap_err();
+    let err = Scope::compile(Some(&config), &[], "shield", STANDARD).unwrap_err();
     assert!(err.contains("\"PSOT\""), "{err}");
 }
 
 #[test]
 fn an_extension_method_a_route_answers_is_accepted() {
     let config = methods(&["PROPFIND"], Traffic::Transcoded);
     let routed = [Method::from_bytes(b"PROPFIND").unwrap()];
-    let scope = Scope::compile(Some(&config), &[], "shield", &routed).unwrap();
+    let scope = Scope::compile(Some(&config), &[], "shield", Routed::Methods(&routed)).unwrap();
+    assert!(scope.matches(&request("PROPFIND", "/dav")));
+    // A route answering every method answers it too.
+    let scope = Scope::compile(Some(&config), &[], "shield", Routed::Every).unwrap();
     assert!(scope.matches(&request("PROPFIND", "/dav")));
 }
 
@@ -140,7 +155,7 @@ fn any_method_token_is_accepted_for_the_fallback() {
     // The fallback is the embedder's own service: which methods it answers
     // is not the proxy's to know.
     let config = methods(&["PROPFIND"], Traffic::Fallback);
-    assert!(Scope::compile(Some(&config), &[], "shield", &[]).is_ok());
+    assert!(Scope::compile(Some(&config), &[], "shield", STANDARD).is_ok());
 }
 
 #[test]
@@ -150,7 +165,7 @@ fn paths_and_methods_narrow_the_requests_a_scope_matches() {
         methods: vec!["post".into()],
         ..ScopeConfig::default()
     };
-    let scope = Scope::compile(Some(&config), &[Traffic::Transcoded], "shield", &[]).unwrap();
+    let scope = Scope::compile(Some(&config), &[Traffic::Transcoded], "shield", STANDARD).unwrap();
     assert!(scope.narrows());
     assert!(scope.matches(&request("POST", "/v1/orders/42")));
     // Methods compare as tokens, case aside.
@@ -179,7 +194,7 @@ async fn a_narrowed_guard_readies_the_branch_it_calls() {
     };
     let scoped = Scoped {
         layer: tower::layer::layer_fn(|inner| inner),
-        scope: Scope::compile(Some(&config), &[], "maintenance", &[]).unwrap(),
+        scope: Scope::compile(Some(&config), &[], "maintenance", STANDARD).unwrap(),
     }
     .layer(fallback);
     for path in ["/other", "/guarded", "/other"] {