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

.github/workflows/ci.yml

@@ -20,8 +20,10 @@ jobs:
     runs-on: ubuntu-latest
     services:
       # Shared store for the rate-limit cross-instance reconciliation test.
+      # Pulled through Google's Docker Hub mirror: anonymous pulls from
+      # Docker Hub on shared runner addresses hit its rate limit.
       redis:
-        image: redis:7
+        image: mirror.gcr.io/library/redis:7
         ports:
           - 6379:6379
         options: >-
@@ -199,8 +201,11 @@ jobs:
         with:
           persist-credentials: false
       # Scans Cargo.lock for RustSec advisories; honors the ignore list in
-      # deny.toml. Third-party action: pinned to a commit SHA (dependabot
-      # keeps it fresh).
-      - uses: EmbarkStudios/cargo-deny-action@3c6349835b2b7b196a839186cb8b78e02f7b5f25 # v2.1.1
+      # deny.toml. The released binary rather than the Docker action, whose
+      # image build pulls from Docker Hub and hits its anonymous rate limit
+      # on shared runners.
+      - uses: taiki-e/install-action@e407f7bafb71fd004bc5c2da3032e5470cbb6ef0 # v2
         with:
-          command: check advisories
+          tool: cargo-deny
+      - name: Check advisories
+        run: cargo deny check advisories

CONTRIBUTING.md

@@ -0,0 +1,80 @@
+# 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
+```
+
+CI runs these on every pull request, and three more checks that need a
+particular toolchain or host. To run them yourself:
+
+```bash
+# The minimum supported Rust version, `rust-version` in Cargo.toml.
+rustup toolchain install 1.99
+cargo +1.99 check --all-targets --all-features
+cargo +1.99 check --all-targets --no-default-features --features cli
+
+# The static musl build the release ships (Linux, with musl-tools installed).
+rustup target add x86_64-unknown-linux-musl
+cargo nextest run --target x86_64-unknown-linux-musl --features cli
+
+# The advisory check of the dependencies.
+cargo install cargo-deny --locked
+cargo deny check advisories
+```
+
+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

@@ -39,7 +39,7 @@ doc = false
 axum = { version = "0.8", features = ["macros"] }
 # `util`: the guarded gRPC and fallback paths are boxed services.
 tower = { version = "0.5", features = ["util"] }
-tower-http = { version = "0.7", features = ["cors", "trace"] }
+tower-http = { version = "0.7", features = ["trace"] }
 # Foundational HTTP types, used directly by the framework-agnostic embedding
 # hooks (src/hooks.rs) so an embedder never names `axum`. Already in the tree
 # transitively via both axum and tonic; pinned here to make the public API deps
@@ -60,6 +60,18 @@ 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"
+# The hash of the routing maps a request is looked up in. Their keys are the
+# paths and segments the proxy binds, fixed at startup, so a request can only
+# look up, never fill a map with colliding keys: no randomized hash is needed,
+# and FxHash looks up short strings faster than SipHash.
+rustc-hash = "2"
+# 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
@@ -186,6 +198,8 @@ cli = ["dep:clap", "dep:tracing-subscriber", "redis"]
 # `test-util`: deadline tests run on a paused clock instead of waiting.
 tokio = { version = "1", features = ["macros", "rt-multi-thread", "test-util"] }
 tower = { version = "0.5", features = ["util", "limit"] }
+# An embedder's own CORS around the proxy (tests/edge.rs).
+tower-http = { version = "0.7", features = ["cors"] }
 http-body-util = "0.1"
 # `hyper::upgrade::on`: a fallback that upgrades its connection (src/serve/tests.rs).
 hyper = "1"

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
@@ -143,7 +144,9 @@ service:
   name: "my-service"
 
 cors:
-  # Empty list = permissive CORS (dev mode, reflects any Origin).
+  # Empty list = permissive CORS (dev mode): every origin (`*`), without
+  # credentials; a preflight gets back the headers it asks for, since `*`
+  # does not cover `Authorization`.
   # A non-empty list allows those exact origins, with credentials; the
   # preflight echoes the methods and headers the browser asks for. There is
   # no "*" wildcard (browsers never send `Origin: *`, so listing "*" would
@@ -380,6 +383,48 @@ 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 transcoded request: another route of
+  the proxy (an extra route of yours) takes it, else it is `404`, or your
+  fallback's, preflight included.
+
+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 +1226,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 +1513,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

@@ -97,9 +97,7 @@ use std::net::{IpAddr, Ipv6Addr, SocketAddr};
 use std::sync::Arc;
 use std::task::{Context, Poll};
 
-use axum::extract::{ConnectInfo, Request};
-use axum::middleware::Next;
-use axum::response::Response;
+use axum::extract::ConnectInfo;
 use http::header::{HeaderName, FORWARDED};
 use http::{HeaderMap, HeaderValue};
 use ipnet::IpNet;
@@ -358,8 +356,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>,
@@ -748,26 +747,5 @@ where
     }
 }
 
-/// The guard of `client_address.required`: a request whose address did not
-/// resolve is refused, `INVALID_ARGUMENT` when a trusted proxy forwarded
-/// something unreadable, `INTERNAL` when the server recorded no connection.
-pub(crate) async fn require(request: Request, next: Next) -> Response {
-    let resolution = request
-        .extensions()
-        .get::<ClientAddress>()
-        .map_or(Resolution::Unavailable, ClientAddress::resolution);
-    match resolution {
-        Resolution::Peer(_) | Resolution::Forwarded(_) => next.run(request).await,
-        Resolution::Invalid(invalid) => crate::guard::reject(
-            tonic::Code::InvalidArgument,
-            format!("the client address a trusted proxy forwarded is {invalid}"),
-        ),
-        Resolution::Unavailable => crate::guard::reject(
-            tonic::Code::Internal,
-            "no connection information to resolve the client address from",
-        ),
-    }
-}
-
 #[cfg(test)]
 mod tests;