fix(release): forward sim/ queries verbatim through the proxy, budget favicon lookups per caller, refresh oversized cached icons

Author: waleedlatif1

apps/extension/src/omnibox.ts

@@ -39,18 +39,22 @@ function readTyped(text: string): { name: string; rest: string } {
     : { name: typed.slice(0, nameEnd), rest: typed.slice(nameEnd) }
 }
 
+const LONE_SURROGATE = /[\uD800-\uDBFF](?![\uDC00-\uDFFF])|(?<![\uD800-\uDBFF])[\uDC00-\uDFFF]/g
+
+/** Code points XML forbids even as references, besides lone surrogates. */
+const NON_XML_CHARACTER = /[\u0000-\u0008\u000B\u000C\u000E-\u001F\uFFFE\uFFFF]/g
+
 /**
- * Suggestion text is XML, where these five characters are markup and control characters are not
- * allowed even as references, so one bad description can't make Chrome drop every suggestion.
+ * Suggestion text is XML, where these five characters are markup and some code points aren't
+ * allowed at all, so one bad description can't make Chrome drop every suggestion.
  */
 function escapeXml(text: string): string {
   return text
-    .replace(/[\u0000-\u0008\u000B\u000C\u000E-\u001F]/g, '')
+    .replace(NON_XML_CHARACTER, '')
+    .replace(LONE_SURROGATE, '')
     .replace(/[&<>"']/g, (character) => `&#${character.charCodeAt(0)};`)
 }
 
-const LONE_SURROGATE = /[\uD800-\uDBFF](?![\uDC00-\uDFFF])|(?<![\uD800-\uDBFF])[\uDC00-\uDFFF]/g
-
 /** `encodeURIComponent`, except a lone surrogate (which it refuses) is dropped. */
 function encodeSegment(segment: string): string {
   return encodeURIComponent(segment.replace(LONE_SURROGATE, ''))

apps/sim/app/sim/[[...path]]/page.tsx

@@ -1,11 +1,12 @@
 import { ChipLink, StatusPageContent } from '@sim/emcn'
 import type { Metadata } from 'next'
+import { headers } from 'next/headers'
 import { notFound, redirect } from 'next/navigation'
 import { getSession } from '@/lib/auth'
 import { organizationRoutes, simLinkPath } from '@/lib/navigation/paths'
 import { resolveSimLinkOrganization } from '@/lib/sim-links/application/organization'
 import { resolveSimLink, type SimLinkResolution } from '@/lib/sim-links/application/use-cases'
-import { typedQueryString } from '@/lib/sim-links/destination'
+import { SIM_LINK_QUERY_HEADER } from '@/lib/sim-links/typed-query'
 import { parseTypedSimLink, simLinkNameSchema } from '@/lib/sim-links/validation'
 import { buildAuthCrossLink } from '@/app/(auth)/auth-redirect'
 import { LogoShell } from '@/app/(landing)/components/logo-shell'
@@ -18,7 +19,6 @@ export const metadata: Metadata = {
 
 interface SimLinkPageProps {
   params: Promise<{ path?: string[] }>
-  searchParams: Promise<Record<string, string | string[] | undefined>>
 }
 
 /**
@@ -50,8 +50,9 @@ function describeUnresolved(result: SimLinkResolution, validName: boolean, name:
  * Where `sim/<name>/<values…>?<query>` lands: the viewer's organization link's destination,
  * or a page that offers to create the link when nobody has yet.
  */
-export default async function SimLinkPage({ params, searchParams }: SimLinkPageProps) {
-  const [{ path = [] }, query] = await Promise.all([params, searchParams.then(typedQueryString)])
+export default async function SimLinkPage({ params }: SimLinkPageProps) {
+  const [{ path = [] }, requestHeaders] = await Promise.all([params, headers()])
+  const query = requestHeaders.get(SIM_LINK_QUERY_HEADER) ?? ''
   const segments = decodeSimLinkPath(path)
   const session = await getSession()
   if (!session?.user)

apps/sim/lib/api/server/routes/definition.test.ts

@@ -6,6 +6,7 @@ import {
   requireBinaryRouteDefinition,
   requireJsonRouteDefinition,
 } from '@/lib/api/server/routes/definition'
+import { defineOperation } from '@/lib/core/application'
 
 const renameOperation = {
   id: 'files.rename',
@@ -35,12 +36,16 @@ describe('declarative route definition invariants', () => {
         path: '/icon',
         response: { mode: 'binary', status },
       })
-    expect(() =>
-      requireBinaryRouteDefinition(contract([204, 200]), renameOperation, renameOperation)
-    ).toThrow('carries a body first')
+    const operation = defineOperation({
+      id: 'icons.get',
+      principalKinds: ['session'],
+      capability: 'none',
+    })
+    expect(() => requireBinaryRouteDefinition(contract([204, 200]), operation, operation)).toThrow(
+      'carries a body first'
+    )
     expect(
-      requireBinaryRouteDefinition(contract([200, 204]), renameOperation, renameOperation)
-        .successStatus
+      requireBinaryRouteDefinition(contract([200, 204]), operation, operation).successStatus
     ).toBe(200)
   })
 })

apps/sim/lib/favicon/resolve.server.test.ts

@@ -5,6 +5,7 @@ import {
 import { createMockRedis } from '@sim/testing/mocks/redis.mock'
 import { redisConfigMockFns } from '@sim/testing/mocks/redis-config.mock'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
+import { MAX_ICON_BYTES } from '@/lib/favicon/probe'
 import { resolveFavicon } from '@/lib/favicon/resolve.server'
 
 vi.mock('@/lib/core/security/input-validation.server', () => inputValidationMock)
@@ -86,4 +87,47 @@ describe('resolveFavicon', () => {
     expect((await resolveFavicon('acme.example.com', admit)).kind).toBe('hit')
     expect(admit).not.toHaveBeenCalled()
   })
+
+  it('looks up an icon cached under an earlier, larger cap again instead of serving it', async () => {
+    answers = { 'acme.example.com': 'icon' }
+    const oversized = Buffer.concat([Buffer.from('h'), PNG, Buffer.alloc(MAX_ICON_BYTES)])
+    stored.set('favicon:v3:acme.example.com', { value: oversized, ttl: 30 * DAY })
+    const result = await resolveFavicon('acme.example.com')
+    expect(result.kind === 'hit' && result.bytes.length).toBe(PNG.length)
+    expect(inputValidationMockFns.mockSecureFetchWithValidation).toHaveBeenCalled()
+  })
+
+  it("never lets one caller's refused budget answer another caller's lookup", async () => {
+    answers = { 'acme.example.com': 'icon' }
+    const [refused, admitted] = await Promise.all([
+      resolveFavicon('acme.example.com', async () => false),
+      resolveFavicon('acme.example.com', async () => true),
+    ])
+    expect(refused.kind).toBe('unavailable')
+    expect(admitted.kind).toBe('hit')
+  })
+
+  it('does not charge a caller who joins a lookup already asking the providers', async () => {
+    answers = { 'acme.example.com': 'icon' }
+    const fetchProvider = inputValidationMockFns.mockSecureFetchWithValidation
+    const answerProvider = fetchProvider.getMockImplementation()
+    let release = () => {}
+    const released = new Promise<void>((resolve) => {
+      release = resolve
+    })
+    fetchProvider.mockImplementation(
+      async (...args: Parameters<NonNullable<typeof answerProvider>>) => {
+        await released
+        return answerProvider?.(...args)
+      }
+    )
+    const first = resolveFavicon('acme.example.com')
+    await vi.waitFor(() => expect(fetchProvider).toHaveBeenCalled())
+    const joinerAdmit = vi.fn(async () => true)
+    const joined = resolveFavicon('acme.example.com', joinerAdmit)
+    release()
+    expect((await joined).kind).toBe('hit')
+    expect((await first).kind).toBe('hit')
+    expect(joinerAdmit).not.toHaveBeenCalled()
+  })
 })

apps/sim/lib/favicon/resolve.server.ts

@@ -53,6 +53,8 @@ async function readCache(host: string): Promise<FaviconResolution | null> {
     if (cached.toString() === UNAVAILABLE_MARKER) return { kind: 'unavailable' }
     const prefix = cached.subarray(0, 1).toString()
     const bytes = cached.subarray(1)
+    // Cached under an earlier, larger cap: look it up again rather than serve it or call it a miss.
+    if (bytes.length > MAX_ICON_BYTES) return null
     const result = classifyProviderResponse(200, bytes)
     if (result.kind !== 'hit') return { kind: 'miss' }
     return prefix === PROVISIONAL_HIT_PREFIX
@@ -156,8 +158,9 @@ async function walk(candidates: string[], signal: AbortSignal): Promise<FaviconR
  * login still gets its platform's icon. Results are cached per host (30 days for an icon, a day
  * for none or for a parent's icon chosen after a timeout, minutes after an outage), and
  * concurrent lookups of one host in this process share a single resolution. `admitLookup` is
- * asked only when nothing is cached, before any provider is: refused, the host is reported
- * unavailable and nothing is cached, so cached icons stay free while new lookups are budgeted.
+ * the caller's own budget, asked only when nothing is cached and no lookup of the host is already
+ * running: refused, the host is reported unavailable to this caller alone and nothing is cached,
+ * so cached icons and lookups shared with another caller stay free.
  */
 export async function resolveFavicon(
   host: string,
@@ -166,13 +169,16 @@ export async function resolveFavicon(
   const candidates = faviconHostCandidates(host)
   const requested = candidates[0]
   if (!requested) return { kind: 'miss' }
+  const joined = inFlight.get(requested)
+  if (joined) return joined
+  const cached = await readCache(requested)
+  if (cached) return cached
+  if (!(await admitLookup())) return { kind: 'unavailable' }
+  // Another caller may have started the same lookup while this one read the cache and its budget.
   const pending = inFlight.get(requested)
   if (pending) return pending
 
   const resolution = (async (): Promise<FaviconResolution> => {
-    const cached = await readCache(requested)
-    if (cached) return cached
-    if (!(await admitLookup())) return { kind: 'unavailable' }
     const resolved = await runWithOutboundOrganization(null, () =>
       walk(candidates, AbortSignal.timeout(RESOLVE_DEADLINE_MS))
     ).catch((): FaviconResolution => ({ kind: 'unavailable' }))

apps/sim/lib/navigation/paths.ts

@@ -59,9 +59,12 @@ export function organizationRoutes(organizationId: string) {
   } as const
 }
 
+/** The path the `sim/<name>` resolver serves. */
+export const SIM_LINK_ROOT = '/sim'
+
 /** Where `sim/<name>/<values…>` resolves in this app: the address a member types, kept verbatim. */
 export function simLinkPath(...segments: string[]): string {
-  return ['/sim', ...segments.map(encodeURIComponent)].join('/')
+  return [SIM_LINK_ROOT, ...segments.map(encodeURIComponent)].join('/')
 }
 
 /** Whether `pathname` is `root` itself or a route beneath it. */

apps/sim/lib/sim-links/destination.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, it } from 'vitest'
-import { resolveSimLinkTarget, typedQueryString } from '@/lib/sim-links/destination'
+import { resolveSimLinkTarget } from '@/lib/sim-links/destination'
 import { parseTypedSimLink, simLinkFieldsSchema } from '@/lib/sim-links/validation'
 
 const fields = {
@@ -145,6 +145,7 @@ describe('Sim link destinations', () => {
       'https://ex.com/p?a=1&b=two%20words&edit',
     ],
     ['https://ex.com/p', [], null, 'q=a b', 'https://ex.com/p?q=a%20b'],
+    ['https://ex.com/p', [], null, 'q=&flag', 'https://ex.com/p?q=&flag'],
     ['https://ex.com/p?x=1#top', ['y'], null, 'x=2', 'https://ex.com/p/y?x=1&x=2#top'],
     [
       'https://ex.com/search?q={*}',
@@ -162,22 +163,6 @@ describe('Sim link destinations', () => {
   })
 })
 
-describe('typedQueryString', () => {
-  it('keeps the spelling a destination reads: spaces as %20 and bare flags bare', () => {
-    expect(typedQueryString({ q: 'hello world', edit: '', tag: ['a', 'b&c'] })).toBe(
-      'q=hello%20world&edit&tag=a&tag=b%26c'
-    )
-  })
-
-  it('carries a typed # as data, never a fragment, once on a destination', () => {
-    const query = typedQueryString({ q: 'a#b' })
-    expect(resolveSimLinkTarget({ url: 'https://ex.com/p', defaultUrl: null }, [], query)).toEqual({
-      kind: 'redirect',
-      url: 'https://ex.com/p?q=a%23b',
-    })
-  })
-})
-
 describe('parseTypedSimLink', () => {
   it.each([
     [' onboarding', 'onboarding'],

apps/sim/lib/sim-links/destination.ts

@@ -15,25 +15,6 @@ export type SimLinkTarget =
   | { kind: 'missing_values'; required: number }
   | { kind: 'invalid_values' }
 
-/**
- * A typed query rebuilt from parsed parameters with its spelling kept: values percent-encoded as
- * a URL path would be (a space stays `%20`, not `+`), and a key typed without a value stays bare
- * (`?edit`, not `?edit=`). Parameters with the same key come out together, after the first one.
- */
-export function typedQueryString(
-  params: Readonly<Record<string, string | readonly string[] | undefined>>
-): string {
-  return Object.entries(params)
-    .flatMap(([key, value]) =>
-      [value ?? []]
-        .flat()
-        .map((item) =>
-          item ? `${encodeURIComponent(key)}=${encodeURIComponent(item)}` : encodeURIComponent(key)
-        )
-    )
-    .join('&')
-}
-
 /** URL parsers resolve `.` and `..` even when encoded, so a value could climb out of the link's path. */
 function isDotSegment(value: string): boolean {
   return value === '.' || value === '..'

apps/sim/lib/sim-links/typed-query.test.ts

@@ -0,0 +1,16 @@
+import { describe, expect, it } from 'vitest'
+import { typedQuery } from '@/lib/sim-links/typed-query'
+
+describe('typedQuery', () => {
+  it.each([
+    ['', ''],
+    ['?', ''],
+    ['?q=&flag', 'q=&flag'],
+    ['?a=hello%20world&a=b+c', 'a=hello%20world&a=b+c'],
+    ['?_rsc=abc&q=1', 'q=1'],
+    ['?q=1&_rsc', 'q=1'],
+    ['?_rscx=1', '_rscx=1'],
+  ])('keeps %j as typed, apart from Next’s own marker', (search, query) => {
+    expect(typedQuery(search)).toBe(query)
+  })
+})

apps/sim/lib/sim-links/typed-query.ts

@@ -0,0 +1,16 @@
+/**
+ * Request header the proxy sets on every `sim/` request to the query exactly as typed, which a
+ * page can't recover from its parsed `searchParams` (`?q=` and `?q` both parse to an empty value,
+ * and spaces may have been typed as `%20` or `+`). The proxy always overwrites it, so a client
+ * can't supply its own.
+ */
+export const SIM_LINK_QUERY_HEADER = 'x-sim-link-query'
+
+/** A request's `search` as typed, without its `?` or the `_rsc` marker Next adds to its own fetches. */
+export function typedQuery(search: string): string {
+  return search
+    .replace(/^\?/, '')
+    .split('&')
+    .filter((part) => part && part !== '_rsc' && !part.startsWith('_rsc='))
+    .join('&')
+}