fix(webapp): reject unsafe hosts when deriving an image policy origin

Author: kathiekiwi

apps/webapp/app/entry.server.tsx

@@ -21,7 +21,12 @@ import { env } from "./env.server";
 import { eventLoopMonitor, eventLoopUtilizationMonitor } from "./eventLoopMonitor.server";
 import { logger } from "./services/logger.server";
 import { avatarObjectStoreImageOrigin } from "./services/userAvatar.server";
-import { buildImgSrcDirective, parseCspImageOrigins, withImgSrc } from "./utils/cspImageOrigins";
+import {
+  appendImageOrigin,
+  buildImgSrcDirective,
+  parseCspImageOrigins,
+  withImgSrc,
+} from "./utils/cspImageOrigins";
 import { singleton } from "./utils/singleton";
 import { remoteBuildsEnabled } from "./v3/remoteImageBuilder.server";
 import {
@@ -82,9 +87,7 @@ const IMG_SRC_DIRECTIVE = buildImgSrcDirective(
       );
     }
 
-    const avatarOrigin = avatarObjectStoreImageOrigin();
-
-    return avatarOrigin && !origins.includes(avatarOrigin) ? [...origins, avatarOrigin] : origins;
+    return appendImageOrigin(origins, avatarObjectStoreImageOrigin());
   })
 );
 

apps/webapp/app/utils/cspImageOrigins.test.ts

@@ -1,6 +1,7 @@
 import { describe, expect, it } from "vitest";
 import { faviconUrl } from "./favicon";
 import {
+  appendImageOrigin,
   BASE_IMG_SRC_SOURCES,
   buildImgSrcDirective,
   imageOriginFromUrl,
@@ -207,6 +208,10 @@ describe("imageOriginFromUrl", () => {
     ["empty", ""],
     ["not a URL", "s3.example.com"],
     ["a non-http scheme", "s3://bucket"],
+    ["a wildcard host", "http://*.evil.com"],
+    ["a host carrying a directive separator", "http://evil.com;script-src"],
+    ["a host carrying a source separator", "http://evil.com,https://other.test"],
+    ["a host with whitespace", "http://evil.com script-src"],
   ])("is undefined when the base URL is %s", (_case, value) => {
     expect(imageOriginFromUrl(value)).toBeUndefined();
   });
@@ -226,3 +231,22 @@ describe("imageOriginFromUrl", () => {
     ).toBe(false);
   });
 });
+
+describe("appendImageOrigin", () => {
+  it("leaves the directive unchanged when no origin is configured", () => {
+    expect(buildImgSrcDirective(appendImageOrigin([], undefined))).toBe(buildImgSrcDirective());
+  });
+
+  it("does not list an origin twice", () => {
+    expect(appendImageOrigin(["http://localhost:9005"], "http://localhost:9005")).toEqual([
+      "http://localhost:9005",
+    ]);
+  });
+
+  it("appends a new origin after the configured ones", () => {
+    expect(appendImageOrigin(["https://sso.example.com"], "http://localhost:9005")).toEqual([
+      "https://sso.example.com",
+      "http://localhost:9005",
+    ]);
+  });
+});

apps/webapp/app/utils/cspImageOrigins.ts

@@ -120,6 +120,10 @@ function rejectionReason(value: string, allowHttp: boolean): string | undefined
 export function imageOriginFromUrl(baseUrl: string | undefined | null): string | undefined {
   if (!baseUrl) return undefined;
 
+  // `new URL` keeps these in the host, and a ";" or "," would truncate or inject a
+  // directive once the sources are space-joined.
+  if (/[*;,]|\s/.test(baseUrl)) return undefined;
+
   let url: URL;
   try {
     url = new URL(baseUrl);
@@ -134,6 +138,15 @@ export function imageOriginFromUrl(baseUrl: string | undefined | null): string |
   return `${url.protocol}//${url.host}`;
 }
 
+/** Adds an optional origin to a source list, keeping it free of duplicates. */
+export function appendImageOrigin(
+  origins: readonly string[],
+  origin: string | undefined
+): string[] {
+  if (!origin || origins.includes(origin)) return [...origins];
+  return [...origins, origin];
+}
+
 /** The full directive: the base sources plus any configured extra origins. */
 export function buildImgSrcDirective(extraOrigins: readonly string[] = []): string {
   return ["img-src", ...BASE_IMG_SRC_SOURCES, ...extraOrigins].join(" ");