Skip to content

forward: no way to express "forward everything" or "everything except", and the absent-block default silently rewrites the request #182

Description

@OliverWolffGIP

Summary

The forward model can express only one of the three postures a proxy needs, and it has picked the
rarest one as the default. There is no way to say "forward everything", and no way to say "forward
everything except X". The only expressible posture is an exhaustive allowlist — and omitting the
block means nothing crosses.

The result is that a route which does not enumerate every header and every query parameter its
backend uses will silently produce a different upstream request than the client sent. No error, no
log, no metric.

Expected

Three postures, resolved independently for headers and for query parameters:

Declaration Behaviour
no list declared everything is forwarded
allowlist declared only the listed elements are forwarded
denylist declared everything is forwarded except the listed elements

headers_allow / query_allow keep their current meaning. The two gaps are the absent case and the
denylist.

Actual

ForwardPolicyStage.copyAllowedQuery / copyAllowedHeaders iterate the allowlist and look each
name up, so anything unnamed is simply never read. ForwardConfig's canonical constructor
normalises an absent list to an empty one:

headersAllow = headersAllow == null ? List.of() : List.copyOf(headersAllow);
queryAllow   = queryAllow   == null ? List.of() : List.copyOf(queryAllow);

so "not declared" and "declared empty" are already indistinguishable downstream. The schema is a
plain array of string with no reserved value, and the lookup is literal, so "*" would match a
parameter named * rather than acting as a wildcard.

Why the current default is the wrong way round

The failure mode is a silent rewrite, not a refusal. The upstream receives a request the client
did not send, the client receives a plausible answer computed from the wrong input, and nothing
anywhere records that an input was discarded — there is no logging, event or metric in the entire
forward package.

That is also inconsistent with how the rest of the gateway behaves. An unknown configuration key
refuses the boot; an unrecognised security_defaults.profile refuses the boot; overlapping anchors
refuse the boot; an over-cap body is a 413; a path outside the allowlist is a 400. Everywhere
else this project fails loudly and on purpose. The forward stage is the one place that degrades
quietly, and it does so on the input path where a wrong answer is hardest to notice.

Four things the change has to settle

These are not objections — they are the places where a naive "absent means forward everything" would
break something that currently works.

1. Hop-by-hop headers need an unconditional carve-out

ResponseStage strips hop-by-hop headers on the way back, but the request path has no equivalent:
copyAllowedHeaders skips only FORWARDING_HEADERS (x-forwarded-*, forwarded). Today that is
harmless, because nothing crosses unless it is named and nobody names Connection.

Under a forward-everything default, Connection, Keep-Alive, TE, Trailer, Upgrade,
Transfer-Encoding and Proxy-Authorization would start crossing to the upstream. That is a
protocol violation and a request-smuggling surface, and it must not be reachable by omitting
configuration. These belong on an unconditional deny that no configuration can lift — the same way
the regenerated forwarding headers already work.

2. Forwarding-header regeneration must survive

Inbound X-Forwarded-* / Forwarded are deliberately never propagated and are regenerated instead.
A forward-everything default must not become a route around that.

3. Cookie on session routes would break the BFF's own mediation property

This is the one I would flag hardest. The BFF guarantees that the browser's session cookie never
reaches the upstream — the integration test asserts exactly that:

Cookie is deliberately NOT allow-listed, so the browser session cookie is stripped and never
reaches the upstream — the two halves of the mediation proof.

That guarantee currently holds because the default is deny. Flip the default and it holds only for
deployments that remember to write a denylist. A documented security property of the product would
then depend on an operator not forgetting something.

So Cookie (and arguably Authorization, which is already overwritten by the mediated bearer)
needs a carve-out on require: session routes specifically — deny by default there, regardless of
the route's declaration.

4. Allowlist and denylist on the same dimension must be a boot error

If both headers_allow and headers_deny are declared, precedence becomes something a reader has
to guess. Refusing at boot, naming the route and the dimension, is consistent with how every other
contradiction in this configuration surface is handled.

Two smaller specification points:

  • Header names match case-insensitively (HTTP), query-parameter names match exactly. A denylist has
    to state which rule it follows, and they differ per dimension.
  • Whether a denylist entry may be a prefix or pattern (X-Internal-*) or must be an exact name.
    Exact-only is a fine answer; it just needs to be the stated one.

Note on the upgrade

Flipping a default from "nothing crosses" to "everything crosses" widens what reaches every backend
in every existing deployment that omitted the block. The alpha policy permits breaking changes
without a migration path, but this one widens a security posture rather than narrowing it, and it
does so for configurations that do not change. It is worth an explicit release note rather than
being left to the diff.

Suggested shape

forward:
  headers_allow: [...]   # allowlist        — mutually exclusive with headers_deny
  headers_deny:  [...]   # denylist         — mutually exclusive with headers_allow
  # neither declared     → all headers forwarded, minus the unconditional carve-outs
  query_allow:   [...]
  query_deny:    [...]
  # neither declared     → all query parameters forwarded

Resolved independently per dimension, so a route may allowlist headers while forwarding all query
parameters, or the reverse.

Whatever is decided, making a discarded input visible — a counter, or a log line naming what was
dropped — would remove the whole class of "the backend answered plausibly and wrongly" debugging
this currently produces.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions