From 8d71164fbcf3dd4339ccf96856a7559055446fc9 Mon Sep 17 00:00:00 2001 From: chbndrhnns <7534547+chbndrhnns@users.noreply.github.com> Date: Tue, 18 Aug 2026 21:38:28 +0000 Subject: [PATCH 1/2] fix(connections): allow non-URL outputs in internal connection mode Template outputs like JWT keys (SUPABASE_ANON_KEY, SUPABASE_SERVICE_ROLE_KEY), passwords, and tokens are not network URLs. In internal mode, createConnection was unconditionally running toInternalUrl on every non-internal template output, causing toInternalUrl to fail on non-URLs and rejecting connections with 'Internal mode isn't available for this connection'. Only rewrite values that parse as valid network URLs; inject non-URL credentials verbatim. --- .../project-connection.service.test.ts | 22 +++++++++- .../projects/project-connection.service.ts | 6 ++- .../projects/project-connection.util.ts | 10 +++++ .../connection-internal-synth.test.ts | 43 +++++++++++++++++++ 4 files changed, 78 insertions(+), 3 deletions(-) diff --git a/apps/api/src/modules/projects/project-connection.service.test.ts b/apps/api/src/modules/projects/project-connection.service.test.ts index 2ae0143fd..7e01f49fe 100644 --- a/apps/api/src/modules/projects/project-connection.service.test.ts +++ b/apps/api/src/modules/projects/project-connection.service.test.ts @@ -5,7 +5,7 @@ import { resolveInternalEndpoint, isValidEnvKey, } from "@repo/core"; -import { toInternalUrl } from "./project-connection.util"; +import { toInternalUrl, isNetworkUrl } from "./project-connection.util"; // Minimal templates — getAppEndpoints reads `endpoints` when present. const MONGO = { @@ -113,3 +113,23 @@ describe("isValidEnvKey", () => { expect(isValidEnvKey("")).toBe(false); }); }); + +describe("isNetworkUrl", () => { + it("returns true for valid network URLs", () => { + expect(isNetworkUrl("postgresql://postgres:pw@db:5432/postgres")).toBe(true); + expect(isNetworkUrl("http://203.0.113.5:8000")).toBe(true); + expect(isNetworkUrl("https://studio.opsh.io")).toBe(true); + expect(isNetworkUrl("mongodb://root:s3cr3t@mongo:27017/")).toBe(true); + expect(isNetworkUrl("redis://:secret@redis:6379/0")).toBe(true); + }); + + it("returns false for non-URL strings like tokens, secrets, usernames", () => { + expect(isNetworkUrl("eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoiYW5vbiJ9.xyz")).toBe(false); + expect(isNetworkUrl("0198c246743de5d952712004d56b18a8aab8c5a9a56bca04eb7b9d79cc452811")).toBe(false); + expect(isNetworkUrl("supabase")).toBe(false); + expect(isNetworkUrl("my-bucket")).toBe(false); + expect(isNetworkUrl("not a url")).toBe(false); + expect(isNetworkUrl("")).toBe(false); + expect(isNetworkUrl("a:b")).toBe(false); + }); +}); diff --git a/apps/api/src/modules/projects/project-connection.service.ts b/apps/api/src/modules/projects/project-connection.service.ts index 382e1a64f..5b8efecc2 100644 --- a/apps/api/src/modules/projects/project-connection.service.ts +++ b/apps/api/src/modules/projects/project-connection.service.ts @@ -17,7 +17,7 @@ import { assertResourceInOrg } from "../../lib/controller-helpers"; import { permission } from "../../lib/permission"; import { getAppConnectionView } from "../apps/app-settings.service"; import { mergeEnvVars } from "./project-env.service"; -import { toInternalUrl } from "./project-connection.util"; +import { toInternalUrl, isNetworkUrl } from "./project-connection.util"; const ENVIRONMENT = "production"; @@ -246,7 +246,9 @@ export async function createConnection( // the east-west address as its value — the synthesizer built it from the same // alias+port the container answers to on the shared network. Inject verbatim; // toInternalUrl would need a template and return null. - if (!output.internal) { + // Non-URL values (passwords, tokens, API keys) don't carry a host or port — + // inject verbatim in both internal and public modes. + if (!output.internal && isNetworkUrl(value)) { // Template source: rewrite host → the source app's internal service alias. // The output's declared/derived `service` is authoritative for which // alias+port to target; if it can't resolve, internal isn't viable here. diff --git a/apps/api/src/modules/projects/project-connection.util.ts b/apps/api/src/modules/projects/project-connection.util.ts index 2e1c8165e..1a1b7eafa 100644 --- a/apps/api/src/modules/projects/project-connection.util.ts +++ b/apps/api/src/modules/projects/project-connection.util.ts @@ -47,3 +47,13 @@ export function toInternalUrl( u.port = String(ep.port); return u.href; } + +/** Check if a string parses as a network URL (protocol + hostname). */ +export function isNetworkUrl(value: string): boolean { + try { + const u = new URL(value); + return Boolean(u.protocol && u.hostname); + } catch { + return false; + } +} diff --git a/apps/api/test/modules/projects/connection-internal-synth.test.ts b/apps/api/test/modules/projects/connection-internal-synth.test.ts index 152f8fcdf..8dbd113b4 100644 --- a/apps/api/test/modules/projects/connection-internal-synth.test.ts +++ b/apps/api/test/modules/projects/connection-internal-synth.test.ts @@ -52,6 +52,14 @@ vi.mock("../../../src/modules/projects/project-env.service", () => ({ vi.mock("../../../src/modules/projects/project-connection.util", () => ({ toInternalUrl: h.toInternalUrl, + isNetworkUrl: (val: string) => { + try { + const u = new URL(val); + return Boolean(u.protocol && u.hostname); + } catch { + return false; + } + }, })); import { createConnection } from "../../../src/modules/projects/project-connection.service"; @@ -160,4 +168,39 @@ describe("createConnection — synthesized internal source", () => { }), ); }); + + it("a TEMPLATE non-URL output (token, password, key) is injected verbatim in internal mode", async () => { + h.getAppConnectionView.mockResolvedValue({ + outputs: [ + { + id: "anonKey", + label: "Anon Key", + value: "eyJhbGciOiJIUzI1NiJ9.abc.def", + envKey: "SUPABASE_ANON_KEY", + service: "kong", + internal: false, + secret: false, + width: "full" as const, + }, + ], + }); + h.getTemplateForOrg.mockResolvedValue({ id: "supabase", connection: { outputs: [] } }); + + const res = await createConnection( + ctx, + "app-a", + { sourceProjectId: "db-c", outputId: "anonKey", envKey: "SUPABASE_ANON_KEY", mode: "internal" }, + { defer: true }, + ); + + expect(h.toInternalUrl).not.toHaveBeenCalled(); + expect(h.mergeEnvVars).toHaveBeenCalledWith( + "app-a", + "org1", + expect.objectContaining({ + upserts: [{ key: "SUPABASE_ANON_KEY", value: "eyJhbGciOiJIUzI1NiJ9.abc.def", isSecret: true }], + }), + ); + expect(res.connection.mode).toBe("internal"); + }); }); From 26ddd69ff370560d3a6f75a743bbf71a5b51d41e Mon Sep 17 00:00:00 2001 From: Hydra Date: Wed, 19 Aug 2026 04:34:06 +0300 Subject: [PATCH 2/2] fix(connections): decide internal reach from the endpoint, not from which output threw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Letting non-URL outputs through is right, but three things downstream were reading the rejection it removed as a signal. `writeBinding` chose internal-vs-public by catching the "internal mode isn't available" ValidationError. The first output it links is always a credential (`buildObjectStorageEnv` pushes the access key first), so that throw fired on every non-cloud MinIO bind and every mode-less bind deterministically landed on public — and the dashboard sends no mode, so that is the default path. With credentials no longer rejected, nothing throws and the bind silently flips to internal with the endpoint rewrite and reachability unchecked. It now asks `internalModeAvailable()` about the ENDPOINT output, the only one of the three that carries a host. `toInternalUrl` fell back to the service's FIRST declared endpoint whenever the resolved value had no port, and minio.json declares the console (9001) before the S3 API (9000) — so a routed MinIO handed the consumer `S3_ENDPOINT=https://minio:9001`, TLS against a plaintext console port, while the bind reported success and the Storage tab kept showing the working public URL. Only the output's own source names the container port (`publicUrl:minio:9000`), so `getOutputPort()` supplies it and it outranks the resolved URL's port — which is either absent (a routed `https://`) or the HOST side of a published mapping (`19000` of `19000:9000`). `toInternalUrl` also carried `https://` onto the alias. East-west traffic terminates at the container, which serves plaintext; the certificate lives on the edge. An endpoint declared `kind: "http"` now drops to `http:`, so Kong resolves to `http://kong:8000` too. A DSN keeps its own scheme. Internal was never checked for co-location. `openship-` networks are per-host and `attachLinkedNetworks` only WARNS when the attach fails, so a cross-server internal link injected an alias that resolves nowhere and still deployed green. Both ends must now derive to the same server, and the cloud check reads the durable `cloudWorkspaceId` via `deriveProjectDeployTarget` instead of the per-deployment meta snapshot, which is absent on a project bound to cloud that has not redeployed since. Credentials are exempt: a JWT carries no host, so gating one on network topology would re-break what GH-631 reported. `createConnection` and the predicate now share one resolver, so a caller that must choose a mode before it writes cannot disagree with what the create will accept. A DEFAULTED internal that cannot resolve falls back to public rather than failing a request that never asked for internal. The GH-631 regression test re-implemented `isNetworkUrl` inside its own `vi.mock` factory, so it never ran the shipped predicate; it now spreads the real module and overrides only `toInternalUrl`. A test against the real minio catalog entry pins the port and scheme end to end, because the wrong endpoint is silent when it happens. --- .../project-connection.service.test.ts | 92 +++++++- .../projects/project-connection.service.ts | 202 +++++++++++++----- .../projects/project-connection.util.ts | 62 ++++-- .../projects/project-storage.service.test.ts | 52 ++++- .../projects/project-storage.service.ts | 30 +-- .../connection-internal-synth.test.ts | 139 ++++++++++-- packages/core/src/app-templates.ts | 13 ++ 7 files changed, 488 insertions(+), 102 deletions(-) diff --git a/apps/api/src/modules/projects/project-connection.service.test.ts b/apps/api/src/modules/projects/project-connection.service.test.ts index 7e01f49fe..ff0b3c4da 100644 --- a/apps/api/src/modules/projects/project-connection.service.test.ts +++ b/apps/api/src/modules/projects/project-connection.service.test.ts @@ -2,6 +2,9 @@ import { describe, it, expect } from "vitest"; import { type AppTemplate, getOutputService, + getOutputPort, + getAppTemplate, + getAppConnection, resolveInternalEndpoint, isValidEnvKey, } from "@repo/core"; @@ -24,6 +27,16 @@ const SUPABASE = { ], } as unknown as AppTemplate; +// Mirrors the real catalog, INCLUDING the order: the console comes first, so +// "first endpoint of the service" is the wrong answer for the S3 API output. +const MINIO = { + id: "minio", + endpoints: [ + { service: "minio", port: 9001, label: "Console", kind: "http" }, + { service: "minio", port: 9000, label: "S3 API", kind: "http" }, + ], +} as unknown as AppTemplate; + describe("toInternalUrl — rewrite a public connection URL to the internal service alias", () => { it("rewrites a Mongo host:port to the service alias, keeping creds + port", () => { expect(toInternalUrl("mongodb://root:s3cr3t@88.99.101.216:27017/", MONGO)).toBe( @@ -55,8 +68,9 @@ describe("toInternalUrl — service-aware (declared output.service is authoritat it("rewrites a PORTLESS public URL to the declared service's endpoint (Kong API)", () => { // `publicUrl:kong` resolves to a domain with no :8000; with the declared // service, internal mode still reaches kong:8000 — the old port-match - // returned null for this and forced it to Public. - expect(toInternalUrl("https://abc.opsh.io", SUPABASE, "kong")).toBe("https://kong:8000/"); + // returned null for this and forced it to Public. The scheme drops to http: + // kong terminates plaintext on 8000, the cert lives on the edge. + expect(toInternalUrl("https://abc.opsh.io", SUPABASE, "kong")).toBe("http://kong:8000/"); }); it("uses the DECLARED service even when the URL port matches another service", () => { @@ -72,6 +86,80 @@ describe("toInternalUrl — service-aware (declared output.service is authoritat }); }); +describe("toInternalUrl — the DECLARED port wins (the resolved value never names the container)", () => { + it("picks the declared endpoint, not the service's first, for a routed value", () => { + // GH-631/#632 follow-up: `publicUrl:minio:9000` resolves to a ROUTED domain, + // which is portless — so the service's first endpoint won and a bucket client + // was handed MinIO's web console (9001) while the bind reported success. + expect(toInternalUrl("https://acme-s3.opsh.io", MINIO, "minio", 9000)).toBe( + "http://minio:9000/", + ); + }); + + it("outranks a published HOST port carried by the resolved value", () => { + // A `19000:9000` mapping resolves to http://host:19000. No container answers + // on 19000, so port-matching fell through to the console as well. + expect(toInternalUrl("http://203.0.113.5:19000", MINIO, "minio", 9000)).toBe( + "http://minio:9000/", + ); + }); + + it("without a declared port, still falls back to the service's first endpoint", () => { + expect(toInternalUrl("https://acme-s3.opsh.io", MINIO, "minio")).toBe("http://minio:9001/"); + }); +}); + +describe("toInternalUrl — TLS does not follow the rewrite onto a container port", () => { + it("keeps a DSN's own scheme (a tcp endpoint is not http)", () => { + expect(toInternalUrl("postgresql://u:p@host:5432/postgres", SUPABASE, "db")).toBe( + "postgresql://u:p@db:5432/postgres", + ); + expect(toInternalUrl("mongodb://root:pw@host:27017/", MONGO, "mongo")).toBe( + "mongodb://root:pw@mongo:27017/", + ); + }); + + it("leaves an already-plaintext http value alone", () => { + expect(toInternalUrl("http://203.0.113.5:9000", MINIO, "minio", 9000)).toBe( + "http://minio:9000/", + ); + }); +}); + +describe("the REAL MinIO catalog entry resolves internally to its S3 API", () => { + // Pins catalog + code together: the port fix is inert if the shipped output + // stops declaring its port, and the wrong endpoint is silent when it happens. + const minio = getAppTemplate("minio")!; + const endpointOut = getAppConnection(minio)!.outputs.find((o) => o.id === "endpoint")!; + + it("declares the S3 API port on the endpoint output", () => { + expect(endpointOut.source).toBe("publicUrl:minio:9000"); + expect(getOutputPort(endpointOut)).toBe(9000); + }); + + it("rewrites a ROUTED S3 url to http://minio:9000 — not the console, not https", () => { + expect( + toInternalUrl( + "https://acme-s3.opsh.io", + minio, + getOutputService(endpointOut), + getOutputPort(endpointOut), + ), + ).toBe("http://minio:9000/"); + }); +}); + +describe("getOutputPort", () => { + it("reads the container port a publicUrl source names", () => { + expect(getOutputPort({ source: "publicUrl:minio:9000" })).toBe(9000); + }); + it("returns null when the source names no port", () => { + expect(getOutputPort({ source: "publicUrl:kong" })).toBeNull(); + expect(getOutputPort({ source: "env:kong:ANON_KEY" })).toBeNull(); + expect(getOutputPort({ source: "template:postgres://{{host}}:5432" })).toBeNull(); + }); +}); + describe("getOutputService", () => { it("prefers an explicit service (needed for template: sources)", () => { expect(getOutputService({ service: "db", source: "template:postgres://{{host}}:5432" })).toBe("db"); diff --git a/apps/api/src/modules/projects/project-connection.service.ts b/apps/api/src/modules/projects/project-connection.service.ts index 5b8efecc2..900eec35a 100644 --- a/apps/api/src/modules/projects/project-connection.service.ts +++ b/apps/api/src/modules/projects/project-connection.service.ts @@ -9,13 +9,21 @@ * URL is encrypted at rest (via the project env merge path). */ -import { repos } from "@repo/db"; -import { ValidationError, isValidEnvKey, getAppEndpoints } from "@repo/core"; +import { repos, type Project } from "@repo/db"; +import { + ValidationError, + isValidEnvKey, + getAppEndpoints, + getAppConnection, + getOutputPort, + deriveProjectDeployTarget, + type AppTemplate, +} from "@repo/core"; import { getTemplateForOrg } from "../apps/catalog-source"; import type { RequestContext } from "../../lib/request-context"; import { assertResourceInOrg } from "../../lib/controller-helpers"; import { permission } from "../../lib/permission"; -import { getAppConnectionView } from "../apps/app-settings.service"; +import { getAppConnectionView, type AppConnectionOutput } from "../apps/app-settings.service"; import { mergeEnvVars } from "./project-env.service"; import { toInternalUrl, isNetworkUrl } from "./project-connection.util"; @@ -169,31 +177,39 @@ async function applyConnectionToTarget( } } -export async function createConnection( +/** Both authorized ends of a prospective connection plus the chosen output. */ +interface ConnectionEnds { + source: Project; + target: Project; + output: AppConnectionOutput; + template: AppTemplate | undefined; + /** Container port the output's catalog `source` names — see `getOutputPort`. */ + declaredPort: number | null; +} + +/** + * Load and authorize both ends of a connection and resolve the chosen output. + * Shared by `createConnection` and `internalModeAvailable` so a caller that has to + * pick a mode BEFORE connecting sees exactly what the create will see. + */ +async function loadConnectionEnds( ctx: RequestContext, targetProjectId: string, - input: CreateConnectionInput, - /** `defer` skips the best-effort apply-redeploy so a bundle redeploys ONCE at - * the end instead of per-item. */ - opts?: { defer?: boolean }, -): Promise<{ connection: ConnectionView; requiresRedeploy: true }> { - const envKey = input.envKey.trim(); - if (!isValidEnvKey(envKey)) { - throw new ValidationError("Enter a valid environment variable name (letters, digits, _)."); - } - + sourceProjectId: string, + outputId: string, +): Promise { // Both projects must exist, be in the SAME org (no cross-tenant flow), and the // caller must be able to read the source + write the target. const target = await repos.project.findById(targetProjectId); assertResourceInOrg(target, "Project", ctx.organizationId, targetProjectId); - const source = await repos.project.findById(input.sourceProjectId); - assertResourceInOrg(source, "Project", target.organizationId, input.sourceProjectId); + const source = await repos.project.findById(sourceProjectId); + assertResourceInOrg(source, "Project", target.organizationId, sourceProjectId); if (source.id === target.id) { throw new ValidationError("A project can't connect to itself."); } await permission.assert(ctx, { resourceType: "project", - resourceId: input.sourceProjectId, + resourceId: sourceProjectId, action: "read", }); await permission.assert(ctx, { @@ -203,15 +219,120 @@ export async function createConnection( }); // Resolve the connection value from the source app's already-computed outputs. - const view = await getAppConnectionView(ctx, input.sourceProjectId); - const output = view.outputs.find((o) => o.id === input.outputId); + const view = await getAppConnectionView(ctx, sourceProjectId); + const output = view.outputs.find((o) => o.id === outputId); if (!output || !output.value) { throw new ValidationError("That connection value isn't available yet on the source app."); } - // Resolve the source app template ONCE — used both to default the reach mode - // from the endpoint's declared scope and to rewrite the internal host below. + // The source app template — used to default the reach mode from the endpoint's + // declared scope and to rewrite the internal host. const template = await getTemplateForOrg(source.organizationId, source.appTemplateId ?? ""); + const spec = template + ? getAppConnection(template)?.outputs.find((o) => o.id === outputId) + : undefined; + return { source, target, output, template, declaredPort: spec ? getOutputPort(spec) : null }; +} + +/** The east-west value an internal connection should inject, or why it can't. */ +type InternalResolution = { value: string } | { error: string }; + +/** + * Resolve what INTERNAL mode would inject for this output, or the reason it isn't + * viable. ONE resolver, because a caller that must choose a mode up front used to + * infer availability from WHICH error `createConnection` happened to throw first — + * so the answer depended on the shape of an unrelated output's value. + */ +function resolveInternalValue(ends: ConnectionEnds): InternalResolution { + const { source, target, output, template, declaredPort } = ends; + + // GH-631: a non-URL value (password, token, API key, bucket name) names no host, + // so internal mode has nothing to rewrite and nothing to reach — inject it + // verbatim. Answered BEFORE reachability on purpose: a credential is equally + // valid whichever box the source runs on, and gating it on network topology + // would re-break the case that was reported. + if (!isNetworkUrl(output.value)) return { value: output.value }; + + // Everything below hands the consumer a HOST to reach east-west, which rides on + // joining the source app's docker network at deploy (attachLinkedNetworks). + // Those networks are per-HOST, so both ends must land on the same box: a + // cloud-hosted project has no attachable shared network on this pipe yet, and + // one server cannot see another's — the alias would resolve nowhere, and the + // attach only WARNS, so the deploy goes green around a dead host. `serverId` is + // the durable binding behind `deriveProjectDeployTarget`; the per-deployment + // snapshot is re-derived, and a deploy that failed to would silently pass here. + if ( + deriveProjectDeployTarget(source) === "cloud" || + deriveProjectDeployTarget(target) === "cloud" + ) { + return { error: "Internal mode isn't available for a cloud-hosted app yet — use Public." }; + } + if ((source.serverId ?? null) !== (target.serverId ?? null)) { + return { + error: + "Internal mode needs both projects on the same server — they're on different servers, so use Public.", + }; + } + + // A synthesized output (plain app / raw compose, no template) already carries + // the east-west address as its value — the synthesizer built it from the same + // alias+port the container answers to on the shared network. Inject verbatim; + // toInternalUrl would need a template and return null. + if (output.internal) return { value: output.value }; + + // Template source: rewrite host → the source app's internal service alias. The + // output's declared/derived `service` and `port` are authoritative for which + // alias+port to target; if it can't resolve, internal isn't viable here. + const internal = toInternalUrl(output.value, template, output.service, declaredPort); + return internal + ? { value: internal } + : { + error: + "Internal mode isn't available for this connection — use Public, or pick a database app's URL.", + }; +} + +/** + * Would wiring `outputId` from `sourceProjectId` into `targetProjectId` as an + * INTERNAL connection actually resolve? For a caller that has to commit to one + * mode for a SET of outputs (the object-storage bind) and so must ask before it + * writes. Answered by the same resolver `createConnection` runs, so the two can't + * disagree. An unavailable internal is the answer here, not an error. + */ +export async function internalModeAvailable( + ctx: RequestContext, + targetProjectId: string, + input: { sourceProjectId: string; outputId: string }, +): Promise { + const ends = await loadConnectionEnds( + ctx, + targetProjectId, + input.sourceProjectId, + input.outputId, + ); + return !("error" in resolveInternalValue(ends)); +} + +export async function createConnection( + ctx: RequestContext, + targetProjectId: string, + input: CreateConnectionInput, + /** `defer` skips the best-effort apply-redeploy so a bundle redeploys ONCE at + * the end instead of per-item. */ + opts?: { defer?: boolean }, +): Promise<{ connection: ConnectionView; requiresRedeploy: true }> { + const envKey = input.envKey.trim(); + if (!isValidEnvKey(envKey)) { + throw new ValidationError("Enter a valid environment variable name (letters, digits, _)."); + } + + const ends = await loadConnectionEnds( + ctx, + targetProjectId, + input.sourceProjectId, + input.outputId, + ); + const { source, target, output, template } = ends; // Default the reach mode from the source endpoint's declared `scope` when the // caller didn't choose: a DB endpoint (scope "internal") wires internal, a @@ -229,36 +350,15 @@ export async function createConnection( let value = output.value; if (mode === "internal") { - // Internal reachability rides on joining the source app's docker network at - // deploy (attachLinkedNetworks). A cloud-hosted source (Oblien) has no - // attachable shared network on this pipe yet, so an internal alias would be - // unreachable — steer to Public instead of injecting a dead host. - const srcDep = source.activeDeploymentId - ? await repos.deployment.findById(source.activeDeploymentId).catch(() => null) - : null; - const srcTarget = (srcDep?.meta as { deployTarget?: string } | null)?.deployTarget; - if (srcTarget === "cloud") { - throw new ValidationError( - "Internal mode isn't available for a cloud-hosted app yet — use Public.", - ); - } - // A synthesized output (plain app / raw compose, no template) already carries - // the east-west address as its value — the synthesizer built it from the same - // alias+port the container answers to on the shared network. Inject verbatim; - // toInternalUrl would need a template and return null. - // Non-URL values (passwords, tokens, API keys) don't carry a host or port — - // inject verbatim in both internal and public modes. - if (!output.internal && isNetworkUrl(value)) { - // Template source: rewrite host → the source app's internal service alias. - // The output's declared/derived `service` is authoritative for which - // alias+port to target; if it can't resolve, internal isn't viable here. - const internal = toInternalUrl(value, template, output.service); - if (!internal) { - throw new ValidationError( - "Internal mode isn't available for this connection — use Public, or pick a database app's URL.", - ); - } - value = internal; + const resolved = resolveInternalValue(ends); + if ("error" in resolved) { + // An EXPLICIT internal ask gets the reason. A DEFAULTED one falls back to + // public — failing a request that never asked for internal turns a working + // public wire-up into an error the caller can't act on. + if (input.mode === "internal") throw new ValidationError(resolved.error); + mode = "public"; + } else { + value = resolved.value; } } diff --git a/apps/api/src/modules/projects/project-connection.util.ts b/apps/api/src/modules/projects/project-connection.util.ts index 1a1b7eafa..b9fcbc8f0 100644 --- a/apps/api/src/modules/projects/project-connection.util.ts +++ b/apps/api/src/modules/projects/project-connection.util.ts @@ -6,12 +6,19 @@ import { getAppEndpoints, resolveInternalEndpoint, type AppTemplate } from "@rep * reaches it with no public port. * * SERVICE-AWARE (authoritative): when the output declares its `service`, rewrite - * host→that service's alias and port→the service's declared endpoint port - * (preferring the endpoint whose port matches the URL's own). This works even - * for a PORTLESS url (e.g. a public Kong URL with no `:8000`) — the declared - * endpoint supplies the port — and it's correct when two services share a port. + * host→that service's alias and port→the service's declared endpoint port. This + * works even for a PORTLESS url (e.g. a public Kong URL with no `:8000`) — the + * declared endpoint supplies the port — and it's correct when two services share + * a port. * - * FALLBACK (no declared service): match the URL's port to a declared endpoint and + * `declaredPort` (the container port named by a `publicUrl::` source) + * OUTRANKS the resolved URL's own port, because neither resolved shape names the + * container: a routed value is portless (`https://`), and a published-port + * value carries the HOST port of the mapping (`19000` of `19000:9000`). Falling + * back to the service's FIRST declared endpoint in either case handed out the + * wrong one — MinIO's console (9001) instead of its S3 API (9000). + * + * FALLBACK (no declared service): match the port to a declared endpoint and * rewrite to that service's alias, as before. A portless URL with no service is * not an internal target → null (caller steers to Public). Pure — unit-testable. */ @@ -19,6 +26,7 @@ export function toInternalUrl( value: string, template: AppTemplate | undefined, service?: string | null, + declaredPort?: number | null, ): string | null { if (!template) return null; let u: URL; @@ -27,32 +35,44 @@ export function toInternalUrl( } catch { return null; } - const urlPort = u.port ? Number(u.port) : undefined; + const port = declaredPort ?? (u.port ? Number(u.port) : undefined); if (service) { - const ep = resolveInternalEndpoint(template, service, urlPort); - if (ep) { - u.hostname = ep.service; - u.port = String(ep.port); - return u.href; - } - // Declared service exposes no endpoint → not internally reachable. - return null; + const ep = resolveInternalEndpoint(template, service, port); + if (!ep) return null; // Declared service exposes no endpoint → not internally reachable. + const kind = getAppEndpoints(template).find( + (e) => e.service === ep.service && e.port === ep.port, + )?.kind; + return rewriteToAlias(u, ep.service, ep.port, kind); } - if (urlPort === undefined) return null; - const ep = getAppEndpoints(template).find((e) => e.port === urlPort); + if (port === undefined) return null; + const ep = getAppEndpoints(template).find((e) => e.port === port); if (!ep) return null; - u.hostname = ep.service; - u.port = String(ep.port); + return rewriteToAlias(u, ep.service, ep.port, ep.kind); +} + +/** + * Point `u` at `service:port`, dropping TLS for an `http` endpoint: east-west + * traffic terminates at the container, which serves PLAINTEXT — the edge owns the + * certificate. Carrying a routed value's `https://` onto the alias produced a URL + * that can never connect (`https://minio:9000`), and it looked right. + */ +function rewriteToAlias(u: URL, service: string, port: number, kind?: "http" | "tcp"): string { + if (kind === "http" && u.protocol === "https:") u.protocol = "http:"; + u.hostname = service; + u.port = String(port); return u.href; } -/** Check if a string parses as a network URL (protocol + hostname). */ +/** + * Does `value` name a network location (a parseable URL WITH a host)? Only such a + * value has a host to rewrite for internal mode; a credential (JWT, password, + * token, bucket name) carries no host and must be injected verbatim. + */ export function isNetworkUrl(value: string): boolean { try { - const u = new URL(value); - return Boolean(u.protocol && u.hostname); + return Boolean(new URL(value).hostname); } catch { return false; } diff --git a/apps/api/src/modules/projects/project-storage.service.test.ts b/apps/api/src/modules/projects/project-storage.service.test.ts index eda08eb3c..31e72d879 100644 --- a/apps/api/src/modules/projects/project-storage.service.test.ts +++ b/apps/api/src/modules/projects/project-storage.service.test.ts @@ -19,7 +19,9 @@ const h = vi.hoisted(() => ({ projects: {} as Record | null>, updates: [] as Array<{ id: string; patch: Record }>, merges: [] as Array<{ projectId: string; upserts: { key: string; value: string; isSecret?: boolean }[]; deletes: string[] }>, - connections: [] as Array<{ projectId: string; sourceProjectId: string; outputId: string; envKey: string }>, + connections: [] as Array<{ projectId: string; sourceProjectId: string; outputId: string; envKey: string; mode?: string }>, + internalAvailable: true, + internalAsks: [] as Array<{ projectId: string; sourceProjectId: string; outputId: string }>, links: [] as Array<{ id: string; envKey: string; sourceProjectId: string }>, deletedLinks: [] as string[], probe: vi.fn(async () => ({ ok: true }) as { ok: boolean; message?: string }), @@ -56,10 +58,14 @@ vi.mock("./project-env.service", () => ({ }), })); vi.mock("./project-connection.service", () => ({ - createConnection: vi.fn(async (_ctx: unknown, projectId: string, input: { sourceProjectId: string; outputId: string; envKey: string }) => { + createConnection: vi.fn(async (_ctx: unknown, projectId: string, input: { sourceProjectId: string; outputId: string; envKey: string; mode?: string }) => { h.connections.push({ projectId, ...input }); h.links.push({ id: `link_${input.envKey}`, envKey: input.envKey, sourceProjectId: input.sourceProjectId }); }), + internalModeAvailable: vi.fn(async (_ctx: unknown, projectId: string, input: { sourceProjectId: string; outputId: string }) => { + h.internalAsks.push({ projectId, ...input }); + return h.internalAvailable; + }), deleteConnection: vi.fn(async (_ctx: unknown, _projectId: string, linkId: string) => { h.deletedLinks.push(linkId); h.links = h.links.filter((l) => l.id !== linkId); @@ -83,6 +89,8 @@ beforeEach(() => { h.links = []; h.deletedLinks = []; h.redeploys = []; + h.internalAsks = []; + h.internalAvailable = true; h.probe.mockResolvedValue({ ok: true }); h.outputs = [ { id: "endpoint", value: "https://minio.example.com" }, @@ -192,6 +200,46 @@ describe("bindObjectStorage — an installed storage app", () => { }); }); +/** + * GH-631/#632 follow-up. The dashboard sends NO mode, so this is the default + * path. It used to pick internal-vs-public by catching the "internal mode isn't + * available" ValidationError — which the FIRST linked output always threw, and + * that output is a credential (AWS_ACCESS_KEY_ID is pushed first). Once + * credentials stopped being rejected for not parsing as URLs, every bind flipped + * to internal with nothing checking that the ENDPOINT was internally reachable. + * The mode must come from asking about the endpoint, and from nothing else. + */ +describe("bindObjectStorage — choosing internal vs public with no mode given", () => { + const input = { sourceProjectId: "store", bucket: "uploads" }; + + it("asks the connection layer about the ENDPOINT output, not a credential", async () => { + await bindObjectStorage(ctx, "app", input); + expect(h.internalAsks).toEqual([ + { projectId: "app", sourceProjectId: "store", outputId: "endpoint" }, + ]); + }); + + it("wires every output internal when the endpoint resolves internally", async () => { + await bindObjectStorage(ctx, "app", input); + expect(h.connections.map((c) => c.mode)).toEqual(["internal", "internal", "internal"]); + }); + + it("wires public — not internal — when the endpoint has no internal address", async () => { + // e.g. the storage app runs on another server, or on Oblien cloud: the + // per-host `openship-` network isn't joinable, so an alias would + // resolve nowhere while the bind still reported success. + h.internalAvailable = false; + await bindObjectStorage(ctx, "app", input); + expect(h.connections.map((c) => c.mode)).toEqual(["public", "public", "public"]); + }); + + it("an EXPLICIT mode is still obeyed without asking", async () => { + await bindObjectStorage(ctx, "app", { ...input, mode: "public" as const }); + expect(h.internalAsks).toEqual([]); + expect(h.connections.map((c) => c.mode)).toEqual(["public", "public", "public"]); + }); +}); + describe("unbindObjectStorage", () => { it("removes the connection rows and the plain keys, then clears the marker", async () => { await bindObjectStorage(ctx, "app", { sourceProjectId: "store", bucket: "uploads" }); diff --git a/apps/api/src/modules/projects/project-storage.service.ts b/apps/api/src/modules/projects/project-storage.service.ts index 0ff8257b8..2da575f8b 100644 --- a/apps/api/src/modules/projects/project-storage.service.ts +++ b/apps/api/src/modules/projects/project-storage.service.ts @@ -45,7 +45,11 @@ import { runConnectivityCheck } from "../../lib/connectivity"; import "../../lib/connectivity-checks"; // registers the S3 probe used below import { getAppConnectionView } from "../apps/app-settings.service"; import { mergeEnvVars } from "./project-env.service"; -import { createConnection, deleteConnection } from "./project-connection.service"; +import { + createConnection, + deleteConnection, + internalModeAvailable, +} from "./project-connection.service"; const ENVIRONMENT = "production"; @@ -344,18 +348,18 @@ async function writeBinding( } // Nothing chosen → prefer internal: upload traffic between two containers on - // the same box has no reason to leave through the edge. Internal isn't always - // available (a cloud-hosted source has no attachable shared network) and the - // connection layer is the authority on that, so try it and fall back instead of - // re-deriving the rule here. Its internal check runs before it writes anything, - // so a rejected attempt leaves nothing behind — and only THAT rejection is - // retried; any other failure is the caller's to see. - try { - await link("internal"); - } catch (err) { - if (!/internal mode isn't available/i.test(safeErrorMessage(err))) throw err; - await link("public"); - } + // the same box has no reason to leave through the edge. The connection layer + // stays the authority on whether internal is available, but ASK it about the + // ENDPOINT — the only one of these outputs that carries a host. Inferring the + // answer from a failed `link("internal")` made the mode depend on whichever + // output threw first, which was always a credential: once credentials stopped + // being rejected for not parsing as URLs, every bind silently flipped to + // internal, endpoint rewrite and cross-server reachability unchecked. + const internal = await internalModeAvailable(ctx, projectId, { + sourceProjectId, + outputId: SOURCE_OUTPUT_IDS.endpoint, + }); + await link(internal ? "internal" : "public"); } /** diff --git a/apps/api/test/modules/projects/connection-internal-synth.test.ts b/apps/api/test/modules/projects/connection-internal-synth.test.ts index 8dbd113b4..0d1bf4bd1 100644 --- a/apps/api/test/modules/projects/connection-internal-synth.test.ts +++ b/apps/api/test/modules/projects/connection-internal-synth.test.ts @@ -6,7 +6,8 @@ import { describe, it, expect, beforeEach, vi } from "vitest"; * (`http://:`), tagged `internal: true`. `createConnection` must * inject that value VERBATIM in internal mode — never route it through * `toInternalUrl`, which needs a template and would return null (killing the - * link). The cloud-source guard still fires first, so a cloud-hosted source is + * link). The co-location guard still fires first, so a cloud-hosted source — or + * one on a different server, since `openship-` networks are per-host — is * steered to Public even when it carries a synthesized internal value. */ @@ -50,16 +51,15 @@ vi.mock("../../../src/modules/projects/project-env.service", () => ({ mergeEnvVars: h.mergeEnvVars, })); -vi.mock("../../../src/modules/projects/project-connection.util", () => ({ +// Only `toInternalUrl` is driven by this suite. Everything else — notably +// `isNetworkUrl`, which decides whether a value even HAS a host to rewrite — must +// be the shipped implementation: a hand-copy inside the factory would keep these +// tests green while the production predicate rotted. +vi.mock("../../../src/modules/projects/project-connection.util", async (importOriginal) => ({ + ...(await importOriginal< + typeof import("../../../src/modules/projects/project-connection.util") + >()), toInternalUrl: h.toInternalUrl, - isNetworkUrl: (val: string) => { - try { - const u = new URL(val); - return Boolean(u.protocol && u.hostname); - } catch { - return false; - } - }, })); import { createConnection } from "../../../src/modules/projects/project-connection.service"; @@ -122,12 +122,15 @@ describe("createConnection — synthesized internal source", () => { }); it("still steers a CLOUD-hosted source to Public before injecting anything", async () => { + // Read from the DURABLE binding (`cloudWorkspaceId`, via + // deriveProjectDeployTarget), not the per-deployment meta snapshot: a project + // bound to cloud that hasn't redeployed has no snapshot to read, and used to + // sail through this guard. h.findById.mockImplementation(async (id: string) => id === "app-a" ? { id: "app-a", name: "App A", slug: "app-a", organizationId: "org1", activeDeploymentId: null } - : { id: "db-c", name: "Plain App", slug: "plain-app", organizationId: "org1", appTemplateId: null, activeDeploymentId: "dep1" }, + : { id: "db-c", name: "Plain App", slug: "plain-app", organizationId: "org1", appTemplateId: null, activeDeploymentId: null, cloudWorkspaceId: "ws_1" }, ); - h.deploymentFindById.mockResolvedValue({ id: "dep1", meta: { deployTarget: "cloud" } }); await expect( createConnection( @@ -143,6 +146,72 @@ describe("createConnection — synthesized internal source", () => { expect(h.upsert).not.toHaveBeenCalled(); }); + it("refuses internal when the two projects sit on DIFFERENT servers", async () => { + // `openship-` networks are per-host, and attachLinkedNetworks only + // WARNS when the attach fails — so a cross-server internal link injected an + // alias that resolves nowhere and still deployed green. + h.findById.mockImplementation(async (id: string) => + id === "app-a" + ? { id: "app-a", name: "App A", slug: "app-a", organizationId: "org1", activeDeploymentId: null, serverId: "srv-a" } + : { id: "db-c", name: "Plain App", slug: "plain-app", organizationId: "org1", appTemplateId: null, activeDeploymentId: null, serverId: "srv-b" }, + ); + + await expect( + createConnection( + ctx, + "app-a", + { sourceProjectId: "db-c", outputId: "svc", envKey: "DB_URL", mode: "internal" }, + { defer: true }, + ), + ).rejects.toThrow(/same server/i); + + expect(h.mergeEnvVars).not.toHaveBeenCalled(); + expect(h.upsert).not.toHaveBeenCalled(); + }); + + it("allows internal for two projects on the SAME server", async () => { + h.findById.mockImplementation(async (id: string) => + id === "app-a" + ? { id: "app-a", name: "App A", slug: "app-a", organizationId: "org1", activeDeploymentId: null, serverId: "srv-a" } + : { id: "db-c", name: "Plain App", slug: "plain-app", organizationId: "org1", appTemplateId: null, activeDeploymentId: null, serverId: "srv-a" }, + ); + + const res = await createConnection( + ctx, + "app-a", + { sourceProjectId: "db-c", outputId: "svc", envKey: "DB_URL", mode: "internal" }, + { defer: true }, + ); + + expect(res.connection.mode).toBe("internal"); + }); + + it("a DEFAULTED mode falls back to public instead of failing the request", async () => { + // Only an EXPLICIT `mode: "internal"` gets the error. When the caller never + // chose, refusing would turn a working public wire-up into a dead end. + h.findById.mockImplementation(async (id: string) => + id === "app-a" + ? { id: "app-a", name: "App A", slug: "app-a", organizationId: "org1", activeDeploymentId: null, serverId: "srv-a" } + : { id: "db-c", name: "Plain App", slug: "plain-app", organizationId: "org1", appTemplateId: null, activeDeploymentId: null, serverId: "srv-b" }, + ); + + const res = await createConnection( + ctx, + "app-a", + { sourceProjectId: "db-c", outputId: "svc", envKey: "DB_URL" }, + { defer: true }, + ); + + expect(res.connection.mode).toBe("public"); + expect(h.mergeEnvVars).toHaveBeenCalledWith( + "app-a", + "org1", + expect.objectContaining({ + upserts: [{ key: "DB_URL", value: "http://my-app:8080", isSecret: true }], + }), + ); + }); + it("a TEMPLATE (non-internal) output still routes through toInternalUrl", async () => { // Regression guard: the `if (!output.internal)` branch must not disturb the // existing template path — a template output IS rewritten to the alias URL. @@ -159,7 +228,9 @@ describe("createConnection — synthesized internal source", () => { { defer: true }, ); - expect(h.toInternalUrl).toHaveBeenCalledWith("postgres://host:5432/db", expect.anything(), "my-app"); + // 4th arg = the container port the output's catalog source declares (null + // here — this fixture's `connection.outputs` is empty). + expect(h.toInternalUrl).toHaveBeenCalledWith("postgres://host:5432/db", expect.anything(), "my-app", null); expect(h.mergeEnvVars).toHaveBeenCalledWith( "app-a", "org1", @@ -203,4 +274,46 @@ describe("createConnection — synthesized internal source", () => { ); expect(res.connection.mode).toBe("internal"); }); + + it("a non-URL output is NOT gated on co-location — a key works from any server", async () => { + // The reachability guards must sit behind the verbatim path: a JWT carries no + // host, so refusing it because the two projects sit on different servers would + // re-break exactly what GH-631 reported. + h.findById.mockImplementation(async (id: string) => + id === "app-a" + ? { id: "app-a", name: "App A", slug: "app-a", organizationId: "org1", activeDeploymentId: null, serverId: "srv-a" } + : { id: "db-c", name: "Supabase", slug: "supa", organizationId: "org1", appTemplateId: "supabase", activeDeploymentId: null, serverId: "srv-b" }, + ); + h.getAppConnectionView.mockResolvedValue({ + outputs: [ + { + id: "anonKey", + label: "Anon Key", + value: "eyJhbGciOiJIUzI1NiJ9.abc.def", + envKey: "SUPABASE_ANON_KEY", + service: "kong", + internal: false, + secret: false, + width: "full" as const, + }, + ], + }); + h.getTemplateForOrg.mockResolvedValue({ id: "supabase", connection: { outputs: [] } }); + + const res = await createConnection( + ctx, + "app-a", + { sourceProjectId: "db-c", outputId: "anonKey", envKey: "SUPABASE_ANON_KEY", mode: "internal" }, + { defer: true }, + ); + + expect(res.connection.mode).toBe("internal"); + expect(h.mergeEnvVars).toHaveBeenCalledWith( + "app-a", + "org1", + expect.objectContaining({ + upserts: [{ key: "SUPABASE_ANON_KEY", value: "eyJhbGciOiJIUzI1NiJ9.abc.def", isSecret: true }], + }), + ); + }); }); diff --git a/packages/core/src/app-templates.ts b/packages/core/src/app-templates.ts index 790da9793..b9ae45bcd 100644 --- a/packages/core/src/app-templates.ts +++ b/packages/core/src/app-templates.ts @@ -610,6 +610,19 @@ export function getOutputService(output: Pick): return m?.[1] ?? null; } +/** + * The CONTAINER port an output's source names (`publicUrl::`), or + * null when it names none. Authoritative for internal-mode rewriting alongside + * `getOutputService`: the port that survives into the RESOLVED value is either + * absent (a routed `https://`) or the host side of a published mapping, so + * only this declaration says which of a multi-endpoint service's ports the output + * actually means. + */ +export function getOutputPort(output: Pick): number | null { + const m = /^publicUrl:[^:]+:(\d+)$/.exec(output.source ?? ""); + return m ? Number(m[1]) : null; +} + /** * Resolve the internal docker endpoint (service alias + container port) an * internal connection URL should point at: the declared endpoint for `service`,