Repository navigation
feat(gateway): read one set of env vars, legacy names as aliases - #1980
Conversation
5 new issues
|
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (8)
🤖 Increase coverage with AI coding...🚦 See full report on Qlty Cloud » 🛟 Help
|
|
Run marker for review 5442140350. |
Tonours
left a comment
Tonours
left a comment
There was a problem hiding this comment.
All four security triggers verified independently at 18444f1764: the pins resolve to the workspace packages, one allowlist resolution feeds both services with enforcement at authorization and token exchange, and the warning names the setting without its value. The security-request thread is resolved.
FOREST_GATEWAY_* names win; forest-mcp-server and forest-bff names stay readable with one warning each. Errors name the key the operator set. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
18444f1 to
8b64388
Compare
A blank allowlist resolved to no allowlist, accepting any registered client on both services; forest-mcp-server refused to start on it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
QA in a real environment: 26/26Setup: Gateway built from this branch, local Forest server (dev), the
Fix shipped from the security review: a whitespace-only Not covered: MCP traffic through the agent (needs a full interactive OAuth sign-in), staging boot (done with the hosted API switch). |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| echo "mcp without PORT: HEALTHCHECK healthy on 3931" | ||
| stop_container | ||
|
|
||
| CONTAINER=$(run_probed \ |
There was a problem hiding this comment.
🟠 High docker/smoke-test.sh:163
Both API-only containers omit FOREST_AGENT_URL, so /health returns 503 and wait_health healthy times out; the smoke test then exits before Docker validation can pass. Set FOREST_AGENT_URL in both scenarios, as in the combined-service scenario.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/gateway/docker/smoke-test.sh around line 163:
Both API-only containers omit `FOREST_AGENT_URL`, so `/health` returns 503 and `wait_health healthy` times out; the smoke test then exits before Docker validation can pass. Set `FOREST_AGENT_URL` in both scenarios, as in the combined-service scenario.
| echo "is_latest=$IS_LATEST" >> "$GITHUB_OUTPUT" | ||
|
|
||
| TAGS="-t $IMAGE:$FULL" | ||
| if [ "$IS_LATEST" = "true" ]; then |
There was a problem hiding this comment.
🟠 High workflows/docker-publish-gateway.yml:423
An older release can overwrite :latest, the major/minor tags, and the server deployment with its version after a newer release has already published. IS_LATEST is decided before imagetools create, while different versions run concurrently, so the newer tag can appear and publish between the check and write; serialize the mutable-tag decision and write, and ensure notifications use the same protected decision.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @.github/workflows/docker-publish-gateway.yml around line 423:
An older release can overwrite `:latest`, the major/minor tags, and the server deployment with its version after a newer release has already published. `IS_LATEST` is decided before `imagetools create`, while different versions run concurrently, so the newer tag can appear and publish between the check and write; serialize the mutable-tag decision and write, and ensure notifications use the same protected decision.
605c738
into
feature/prd-1337-create-the-forestadmingateway-package-that-serves-both
* feat(gateway): serve the MCP and the API on one port Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * ci: lint and test the gateway package Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(gateway): build against the workspace agent-bff and mcp-server Exact pins fall behind the workspace on each release, since a private package is skipped by multi-semantic-release, and Yarn then pulls the published versions instead of linking the local ones. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat(mcp-server): export the listener variables parseMcpEnv reads Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(gateway): follow the listener variable names agent-bff and mcp-server own A rename on their side now fails the gateway's typecheck, or carries over through the exported name, instead of silently moving the API's public url and port. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat(agent-bff): export the openapi --output handling for other CLIs Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(gateway): reuse agent-bff's openapi --output handling Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat(gateway): read one set of env vars, legacy names as aliases (#1980) * feat(gateway): warn that forest-mcp-server and forest-bff are deprecated (#1990) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2 participants
Summary
The standalone Gateway reads one set of variables. Legacy
forest-mcp-server/forest-bffnames stay readable as aliases; errors name the key the operator actually set.PORTMCP_SERVER_PORT,HTTP_PORTFOREST_GATEWAY_URL(bare origin)FOREST_MCP_SERVER_URLmcponFOREST_AGENT_URLAGENT_URL(only withapi)FOREST_GATEWAY_ALLOWED_OAUTH_CLIENTSFOREST_MCP_ALLOWED_OAUTH_CLIENTSFOREST_GATEWAY_API_*(incl.PUBLIC_URL, must end withP/api)BFF_*(derived fromCONFIG_KEYS)apionResolution rules (
env-aliases.ts):Also:
forest-gateway openapiuses the same resolution: server isFOREST_GATEWAY_API_PUBLIC_URL, else relativeP/api.normalizeDomainList(domains, label = 'allowedOAuthClients')(forestadmin-client): the Gateway validates the list at boot, naming its env key.CONFIG_KEYS,parsePublicUrl;renderOpenApitakeslabels.AGENT_URLwithmcponly → ignored with a warning;FOREST_GATEWAY_URL/FOREST_MCP_SERVER_URLwithapionly → ignored with a warning.agent-bff1.40.0,mcp-server1.28.0,forestadmin-client1.43.5): before,packages/gateway/node_modulesheld 1.39.0 / 1.27.1 from npm, so its tests didn't run against workspace code.Evidence
BFF_PUBLIC_URL,HTTP_PORT,MCP_SERVER_PORT,FOREST_MCP_SERVER_URLignored with a warning;AGENT_URL/BFF_*passed through raw; errors namedtokenTtl.accessTokenSeconds,allowedOAuthClients,BFF_*.After: gateway 174/174, agent-bff 2250/2250, mcp-server 1544/1544, forestadmin-client 357/357, lint clean.
env-aliases.test.ts: precedence, empty values, shadowed alias not validated, disagreeing aliases, no value in warnings.gateway-env.test.ts: every alias;HTTP_PORT=8080+MCP_SERVER_PORT=3931→ fails naming both;PORT=8080+ bothabc→ 8080, two warnings; rootBFF_PUBLIC_URL→must end with /api; rootBFF_PUBLIC_URLshadowed by a valid new name → boots;FOREST_AGENT_URLwith a query → error without the value; legacy MCP allowlist → both services.run-gateway.test.ts: no encryption key → boots,configured.oauth: false, sign-in-off warning;apiwithout agent url → 503 degraded namingFOREST_AGENT_URL;forest-bffconfig → one warning per alias, key never logged; invalid TTL withoutFOREST_AUTH_SECRET→ fails; invalid CORS entry dropped with warning; uploads off skip the storage module; noFOREST_GATEWAY_URL→ healthy + warning; hosted API config (docker-compose.bff.ymlkeys translated) → healthy, no warning.cli-dispatch.test.ts:openapiannouncesFOREST_GATEWAY_API_PUBLIC_URLunder/forest.Not covered: the runtime
P/api/agent/openapi.jsonwith a public URL (needs a signed token + Forest schema); covered by theopenapicommand. Staging boot happens with the switch of the hosted API service.Merge Danger
Door: two-way
Private, unpublished package; shared-package changes are additive (optional
label/labels, new exports).Blast Radius: Gateway-only
forest-bffandforest-mcp-serverkeep today's names and error messages (default labels unchanged).Stacked on #1978.
Fixes PRD-1338
🤖 Generated with Claude Code
Note
Split gateway env config into MCP and API settings with legacy names as aliases
FOREST_GATEWAY_*variables, keeping oldBFF_*, MCP, and listener names as legacy aliases.EnvAliasesin env-aliases.ts: the primary name wins, aliases fill gaps, disagreeing aliases throw, and legacy or ignored settings produce warnings without exposing secret values.renderOpenApiandnormalizeDomainList.AGENT_URLis ignored with a warning when the API service is disabled; MCP settings are validated before degraded-mode secret checks, so previously-bootable invalid configs may now fail; agent URLs must be absolute HTTP(S) without query or fragment.Changes since #1980 opened
parseGatewayEnvto treat whitespace-only values forFOREST_GATEWAY_ALLOWED_OAUTH_CLIENTSand its aliases as set, causing validation to reject them while empty strings remain treated as unset [9c43d06]EnvAliases.resolveto acceptResolveOptionswith ablankIsSetboolean option that controls whether whitespace-only values are treated as set versus unset [9c43d06]blankIsSetoption inEnvAliases.resolveand for whitespace-only OAuth client allowlist behavior inparseGatewayEnv[9c43d06]@forestadmin/gatewayincluding multi-stage Dockerfile, dependency manifest generation, workspace closure validation, vulnerability scanning gate, and smoke testing [76466fa]@forestadmin/gatewaytriggered by new version tags [76466fa]@forestadmin/gatewaypackage and pinnedzoddependency to exact version in@forestadmin/mcp-server[76466fa]📊 Macroscope summarized 9c43d06. 2 files reviewed, 1 issue evaluated, 1 issue filtered, 0 comments posted
🗂️ Filtered Issues
packages/gateway/src/gateway-env.ts — 0 comments posted, 1 evaluated, 1 filtered
CONFIG_KEYSincludesHTTP_PORT, so this loop resolves it again asFOREST_GATEWAY_API_HTTP_PORTwithHTTP_PORTas an alias and line 183 restores the legacy value intoapiEnv. ConsequentlyPORT=8080, HTTP_PORT=abccorrectly selectsPORTfor the Gateway, butparseConfig(settings.env)later parses the reintroducedHTTP_PORT=abcand aborts startup. This also contradicts the nearby intendedgateway.api.env.HTTP_PORT === undefinedcase. Exclude the listener key from this loop (or clear it after resolving API settings) so a shadowed legacy port is not validated by the API. [ Out of scope (triage) ]