fix(connections): allow non-URL outputs in internal connection mode

apps/api/src/modules/projects/project-connection.service.test.ts

@@ -2,10 +2,13 @@ import { describe, it, expect } from "vitest";
 import {
   type AppTemplate,
   getOutputService,
+  getOutputPort,
+  getAppTemplate,
+  getAppConnection,
   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 = {
@@ -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");
@@ -113,3 +201,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);
+  });
+});

apps/api/src/modules/projects/project-connection.service.ts

@@ -9,15 +9,23 @@
  * 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 } from "./project-connection.util";
+import { toInternalUrl, isNetworkUrl } from "./project-connection.util";
 
 const ENVIRONMENT = "production";
 
@@ -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<ConnectionEnds> {
   // 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<boolean> {
+  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,34 +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.
-    if (!output.internal) {
-      // 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;
     }
   }