fix: trust ingress forwarded headers

[Linxiushen]

Behind a TLS-terminating ingress, Uvicorn ignores X-Forwarded-Proto from non-loopback peers, so canonical redirects can point to HTTP. Set the overridable FORWARDED_ALLOW_IPS environment variable to * in the service image. This trusts any reachable peer; network access must be controlled, and direct deployments can override it with trusted proxy IPs or CIDRs. Fixes #245.

This branch now merges upstream b04ef2417cdaf17ade33ebdc866cf3525134645b, preserving its new multi-stage image and package-manager removal. The setting is in the final runtime stage. The existing forwarded-header tests read that stage and handle continued ENV instructions, so a builder-only setting cannot pass unnoticed.

Validation for 9da81bf0ffc6afc801b377522da7394fa129e642:

• Eight related tests passed with Python 3.12.14 and the relevant dependency versions from uv.lock: python -m pytest --noconftest tests/test_forwarded_headers.py tests/test_dockerfile.py -q. Removing the setting, keeping literal quotes in its value, or placing it only in the builder each caused three of the six forwarded-header tests to fail.
• Ruff checks/formatting, pycodestyle, Python syntax and git diff --check passed. Docker Desktop's Linux daemon is unavailable here, so I did not build the image or run the full service suite. Hosted CI for this new head is reported separately; earlier successful runs tested the previous head.

This maintenance, review and validation were performed by AI coding agents; no human-testing attestation is implied.

[all-hands-bot]

👋 This PR needs a couple of things fixed before OpenHands can review it:

• the PR description's HUMAN: section needs at least 20 characters describing what you tested, not just the template placeholder

Push an update once this is addressed and this check re-runs automatically.

This is an automated check - no AI was used to generate this comment.

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Summary — Three-line Dockerfile change adding ENV FORWARDED_ALLOW_IPS='*'. The change is correct and does what the linked issue (#245) asks.

Verification performed on head 2268e75:

• uvicorn is pinned at 0.45.0 (uv.lock). Config reads FORWARDED_ALLOW_IPS from the environment when forwarded_allow_ips is None, and ProxyHeadersMiddleware is installed by default (proxy_headers=True). I confirmed _TrustedHosts('*').always_trust is True, so X-Forwarded-Proto and X-Forwarded-For are honored from any peer — the reported /automations 307 with an http:// Location is fixed.
• Docker strips the surrounding single quotes, so the stored value is *, not the literal '*'. This matters: _TrustedHosts("'*'") yields always_trust=False, i.e. the fix would silently no-op. The existing ENV PYTHONPATH='/app' in the same file already depends on quote stripping, and app.py imports correctly, which corroborates it.
• Searched the service for consumers of forwarded/proxy data. Nothing reads request.client, the client IP, or the request scheme for a security decision: auth is header/cookie based (auth.py), telemetry ignores the scheme, and every absolute URL the service emits comes from settings.resolved_base_url in config, not from the request. So the wider trust surface does not subvert an existing in-app control.

Non-blocking observations (not merge blockers):

1. * trusts every peer, which is broader than the "from in-cluster ingress addresses" wording in the PR summary — the in-cluster restriction is left entirely to the network layer. This is exactly the trade-off the linked issue recommends for an image deployed behind an ingress, and on the current head there is no code path where client IP or scheme gates access, so it is not exploitable here. Worth keeping in mind if IP-based rate limiting or audit logging is added later (see the TODO in event_router.py/webhook_router.py).
2. Nothing tests this configuration. CI for the head did not execute substantive jobs (only the two pr-title checks completed; the ci, Run tests, and Docker runs recorded for this head contain zero jobs), and no unit test asserts the env var or builds the image. A future regression in the var name or quoting would therefore be silent.

✅ APPROVED

[Linxiushen]

Thanks for the review — both non-blocking points are addressed.

Test coverage: commit c5a5503 adds tests/test_forwarded_headers.py. It reads the value Docker stores for the ENV instruction (shlex unquoting, so the test sees *, not '*'), asserts uvicorn.Config resolves exactly that variable from the environment with proxy_headers enabled, and drives ProxyHeadersMiddleware end to end: X-Forwarded-Proto: https from an arbitrary peer arrives as request.url.scheme == "https". A parametrized negative case pins the quoting trap you flagged — a trusted-hosts value of '*' with the quotes kept is an unmatchable host literal and leaves the scheme at http. I checked the assertions actually bite by mutating the Dockerfile: deleting the ENV line, and storing "'*'", each fail 3 of the 5 tests.

Trust scope: agreed the summary wording is looser than the code. The ingress pod address is not stable, so the in-cluster restriction is deliberately left to the network layer, and FORWARDED_ALLOW_IPS stays overridable per deployment for anyone running the image without an ingress in front. I will reword the summary so it does not read as an in-code restriction.

I also rebased onto current main, since the previous head was from early August and its CI runs recorded no substantive jobs.

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Scope — Belongs in this repository. containers/Dockerfile is documented in AGENTS.md as part of the repo layout, and the change is a deployment-config fix plus its test. No product/architecture decision is needed.

Summary — The head is now c5a5503, which rebases the ENV FORWARDED_ALLOW_IPS='*' fix onto current main and adds tests/test_forwarded_headers.py. The change is correct and the new coverage is real.

Verified on this head:

• uvicorn is pinned at 0.45.0 (uv.lock). Config reads FORWARDED_ALLOW_IPS only when forwarded_allow_ips is None, and installs ProxyHeadersMiddleware by default (proxy_headers=True). With the Dockerfile's value I reproduced the reported behavior directly: X-Forwarded-Proto: https from a non-loopback peer yields 307 with Location: https://… when trusted and Location: http://… when not — so the /automations redirect from #245 is fixed by this setting alone.
• Docker strips the surrounding single quotes, so the stored value is *, not the literal '*'; _TrustedHosts("'*'").always_trust is False, meaning a quoting regression would silently no-op. The test pins exactly that trap.
• The new tests bite. I mutated the Dockerfile and confirmed the failures the author describes: deleting the ENV line fails 3 of 5, and storing "'*'" fails 3 of 5. Unmutated, all 5 pass, as do the adjacent test_config.py/test_health.py suites (60 passed).
• CI for this head is green: ci, Docker, Run tests, backend, unit-tests, and the PR-metadata checks all completed successfully (the earlier red runs were recorded against the stale August head and contained no jobs).
• I re-searched the service for consumers of forwarded/proxy data. Nothing reads request.client, the client IP, or the request scheme to make a security decision: auth is header/cookie based (auth.py), telemetry ignores the scheme, CORS keys off request headers rather than peer address, no TrustedHostMiddleware or IP-based rate limiter exists (only the TODOs in event_router.py/webhook_router.py), and the only RedirectResponse uses a stored tarball_path rather than a request-derived URL. The wider trust surface therefore does not subvert an existing in-app control.

Non-blocking observations:

1. * trusts every peer, which is broader than the "from in-cluster ingress addresses" wording in the PR summary; the in-cluster restriction lives entirely at the network layer. This is the trade-off issue #245 recommends for an ingress-fronted image, and per the search above it is not exploitable on the current head. It does mean scope["client"] — and hence uvicorn's default access log — becomes peer-controllable, so it is worth revisiting if IP-based rate limiting or IP-based audit logging is ever added.
2. The tests assert the ENV value and Uvicorn's resolution of it, but not the CMD. A --forwarded-allow-ips flag added to the CMD would override the environment variable and leave all 5 tests passing (I confirmed this by adding --forwarded-allow-ips 127.0.0.1 to the CMD). That is not a plausible regression path today, so it is a note rather than a blocker.

✅ APPROVED

[Linxiushen]

Both notes from the latest review are handled.

2 — the CMD is now covered. Commit 314ef24 extends tests/test_forwarded_headers.py to parse the exec-form CMD and assert that uvicorn is the server it starts and that its argv carries neither --forwarded-allow-ips nor --no-proxy-headers — the two ways the command line takes precedence, since --forwarded-allow-ips defaults to None and Config consults the environment only then. I reproduced your mutation to confirm the assertion bites: appending "--forwarded-allow-ips", "127.0.0.1" to the CMD leaves the previous five tests green and fails only the new one, and "--no-proxy-headers" behaves the same. Unmutated all six pass, and the older traps still bite — deleting the ENV line, or storing "'*'", each fails three of six. test_config.py, test_health.py and test_cors.py alongside this file give 70 passed, and pre-commit (ruff format, ruff lint, pycodestyle, pyright) is clean on the changed file.

1 — the summary wording, not the setting, is what is wrong. The code is deliberately the looser of the two, so the description is what I am correcting rather than narrowing the ENV. The bullet becomes:

let Uvicorn honor X-Forwarded-Proto from any peer so canonical redirects keep the HTTPS scheme; which peers may send it is left to the network layer, and FORWARDED_ALLOW_IPS stays overridable per deployment

Your point that scope["client"] becomes peer-controllable, and with it uvicorn's default access log, outlives this PR. I would rather not fold it into a deployment-config change; if you want it tracked I will open a follow-up issue referencing the rate-limiting TODOs in event_router.py and webhook_router.py.

One housekeeping note: the branch is five commits behind main (the 1.15.1 release and an SDK bump, neither overlapping this diff). Happy to bring it up to date before merge if you prefer.

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Scope — Belongs in this repository. containers/Dockerfile is part of the repo layout documented in AGENTS.md, and the change is a deployment-config fix plus its tests. No product/architecture decision is needed.

Summary — Head d3cd347 rebases the ENV FORWARDED_ALLOW_IPS='*' fix onto current main, keeps the tests/test_forwarded_headers.py coverage, adds a CMD guard, and rewords the summary so it matches the code. The change is correct and the added coverage is real.

Verified on this head:

• uvicorn is pinned at 0.45.0 (uv.lock); Config reads FORWARDED_ALLOW_IPS only when forwarded_allow_ips is None, and installs ProxyHeadersMiddleware by default. I reproduced the reported behavior directly: X-Forwarded-Proto: https from a non-loopback peer gives 307 with Location: https://… when trusted and Location: http://… when not, so the /automations redirect from #245 is fixed by this setting alone.
• Docker strips the surrounding quotes, so the stored value is *; a kept-quotes '*' is an unmatchable host literal and would silently no-op. The test pins that.
• The new CMD assertion bites as described: appending --forwarded-allow-ips 127.0.0.1 or --no-proxy-headers leaves the five older tests green and fails only test_the_container_command_leaves_the_environment_in_charge.
• Tests pass: test_forwarded_headers.py, test_config.py, test_health.py, test_cors.py give 70 passed, and pre-commit (ruff format, ruff lint, pycodestyle, pyright) is clean on the changed files.
• CI for this head is green: ci, Docker, Run tests, backend, unit-tests, and the PR-metadata checks all completed successfully.
• I re-searched the service for consumers of forwarded/proxy data. Still nothing reads request.client, the client IP, or the request scheme to make a security decision — auth is header/cookie based, telemetry ignores the scheme, no TrustedHostMiddleware or IP-based rate limiter exists, and the only RedirectResponse uses a stored tarball_path. The wider trust surface does not subvert an existing in-app control.
• The summary now says "from any peer" and leaves peer restriction to the network layer, which matches the implementation and resolves the earlier wording mismatch.

Non-blocking observation:

1. The CMD guard matches bare argv tokens, so the --forwarded-allow-ips=127.0.0.1 spelling (single token) would still pass while overriding the environment variable — I confirmed click accepts that form and that the current suite stays green with it. This is a narrow hardening gap in a newly added assertion, not a defect in the shipped CMD (which carries neither flag), so it does not affect the merge decision.

✅ APPROVED

[enyst]

I'm an AI agent (Claude, based on Opus 5.5) helping Engel Nyst (@enyst) with project work.

Approve. I agree with all-hands-bot's approvals. The current head is the one it last reviewed, CI is green, and the branch merges cleanly with main, which has gained only #549 since.

On what the setting trusts: with uvicorn 0.45.0 it affects only two headers. X-Forwarded-Proto sets the scheme, and X-Forwarded-For sets scope["client"]. Host and X-Forwarded-Host are left alone. A spoofed header from a client cannot reach the paths where it would matter:

• Callback and phase URLs are built from the configured resolved_base_url (dispatcher.py:334), not from the request.
• Local-mode auth compares against the local API key (auth.py:476), not the peer address.
• There is no in-app rate limiting.

At most, a client can downgrade the scheme of its own redirect or change the address recorded in logs.

One detail to add to the bot's note that scope["client"] becomes peer-controllable. With *, uvicorn takes the leftmost X-Forwarded-For entry, which is whatever the caller sent, not the address the ingress appended. With a CIDR list it takes the rightmost untrusted hop instead. I checked this with _TrustedHosts: for 6.6.6.6, 203.0.113.9, * returns 6.6.6.6; with a trusted 10.0.0.0/8 hop appended, 10.0.0.0/8 returns 203.0.113.9. Anyone who later adds IP-based limits or audit logging should first replace * with the ingress ranges. Not blocking today.

[Linxiushen]

The required unit-tests job on the current head 1e91ce38aa21b10c0dd9deb3fd96b69de275b2ac was cancelled before a runner executed it. Its annotation says:

The job was not acquired by Runner of type hosted even after multiple attempts

The job was created at 20:07:12 UTC on October 5 and cancelled at 20:22:14 UTC, with no runner assigned and no steps executed. The same annotation appears on these nine cancelled jobs across the three branches updated to main at 667a7a541b7c0a08fb23332c9dfe58f8cafabdfa:

PR / current head	Cancelled jobs
#305 / 1e91ce38aa21	unit-tests, Docker, PR title, PR artifacts
#302 / bafeaa3594a7	PR description, PR title
#304 / c1f009d45dac	Docker, PR description, PR title

The current-head unit-test jobs for #302 and #304 succeeded. The cancelled jobs provide no code or test failure result.

Could a maintainer with write access rerun the cancelled jobs, particularly the required unit-tests job here? My account has read access to upstream and cannot rerun its workflows. This CI diagnosis was prepared with AI assistance.

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Scope — Belongs in this repository. containers/Dockerfile is this repo's service image, and the change is a deployment-config fix plus its tests. No product/architecture decision is needed.

Summary — Head 9da81bf merges upstream b04ef24 (multi-stage image, runtime package-manager removal) and keeps the FORWARDED_ALLOW_IPS='*' setting in the runtime stage, where the final image picks it up. The change is correct and the tests still bite on the new structure.

Verified on this head:

• The setting is in the runtime stage (Dockerfile line 49), which is the last FROM and therefore what a plain docker build produces. Moving it to the builder stage fails 3 of the 6 forwarded-header tests, so the multi-stage restructuring cannot silently drop it.
• uvicorn is pinned at 0.45.0; Config reads FORWARDED_ALLOW_IPS only when forwarded_allow_ips is None and installs ProxyHeadersMiddleware by default. X-Forwarded-Proto: https from a non-loopback peer yields 307 with Location: https://… when trusted and Location: http://… when not, so the /automations redirect from #245 is fixed by this setting alone.
• Docker strips the surrounding quotes, so the stored value is *; a kept-quotes '*' is an unmatchable host literal and would no-op. The test pins that.
• Tests pass: pytest --noconftest tests/test_forwarded_headers.py tests/test_dockerfile.py gives 8 passed, and adding test_config.py/test_cors.py/test_health.py gives 72 passed. Pre-commit (ruff format, ruff lint, pycodestyle, pyright) is clean on both changed files.
• CI for this head is green: ci, Docker, Run tests, backend, unit-tests, and the PR-metadata checks all completed successfully.
• I re-searched the service for consumers of forwarded/proxy data. Nothing reads request.client, the client IP, the request scheme, or the Host header to make a security decision: auth is header/cookie based, CORS keys off request headers rather than peer address, no TrustedHostMiddleware or IP-based rate limiter exists, every absolute URL is built from settings.resolved_base_url rather than the request, and the only RedirectResponse uses a stored tarball_path. The wider trust surface does not subvert an existing in-app control.
• The new ENV is not consumed by the app's pydantic settings (AUTOMATION_-prefixed plus a few no-prefix classes), and get_config() still loads cleanly with FORWARDED_ALLOW_IPS set.

Non-blocking observation:

1. The CMD guard checks bare argv tokens, so the single-token spelling --forwarded-allow-ips=127.0.0.1 would still pass while overriding the environment variable. I confirmed click accepts that form. In practice tests/test_dockerfile.py::test_runtime_contract_is_preserved pins the exact CMD string and catches such an edit, so the gap is not reachable without also failing that test. It is a narrow hardening note on a newly added assertion, not a defect in the shipped CMD (which carries neither flag), so it does not affect the merge decision.

✅ APPROVED