Merge remote-tracking branch 'origin/fix/w6-risk' into integrate/v2-w5

Author: waleedlatif1

apps/sim/app/api/v2/logs/route.test.ts

@@ -24,6 +24,7 @@ vi.mock('@/lib/logs/application/list-public-logs', () => ({
   listPublicLogs: { operation: { id: 'logs.list' }, execute: mocks.execute },
 }))
 
+import { UNREADABLE_CURSOR_MESSAGE } from '@/lib/api/cursor-binding'
 import { OrchestrationError } from '@/lib/core/orchestration/types'
 import { cursorFilterScope, encodeScopedCursor } from '@/app/api/v2/lib/response'
 import { GET } from '@/app/api/v2/logs/route'
@@ -216,7 +217,14 @@ describe('GET /api/v2/logs', () => {
     expect(mocks.execute).not.toHaveBeenCalled()
   })
 
-  /** Neither param exists on this operation, so naming them sends the caller nowhere. */
+  /**
+   * An undecodable token says nothing about which param changed, and this
+   * operation declares neither `sortBy` nor `sortOrder` under a `.strict()`
+   * query schema — so the sort-mismatch message would answer one 400 with
+   * advice that earns a second. The message is asserted exactly rather than by
+   * absence: "does not say sortBy" is satisfied by almost any wording, including
+   * one that tells the caller nothing at all.
+   */
   it('names the params a rejected cursor is actually bound to', async () => {
     const response = await GET(
       new NextRequest(
@@ -225,6 +233,8 @@ describe('GET /api/v2/logs', () => {
     )
 
     const body = await response.json()
+    expect(body.error.message).toBe(UNREADABLE_CURSOR_MESSAGE)
+    expect(body.error.message).toContain('Restart pagination without a cursor')
     expect(body.error.message).not.toContain('sortBy')
     expect(body.error.message).not.toContain('sortOrder')
   })

apps/sim/app/api/v2/skills/route.test.ts

@@ -50,6 +50,7 @@ vi.mock('@/lib/skills/application/use-cases', () => ({
   createSkillUseCase: { operation: { id: 'skills.create' }, execute: mocks.create },
 }))
 
+import { PrincipalKindAuthorizationError } from '@/lib/core/application'
 import { cursorFilterScope, cursorSortKey, encodeOffsetCursor } from '@/app/api/v2/lib/response'
 import { GET, POST } from '@/app/api/v2/skills/route'
 
@@ -311,6 +312,33 @@ describe('/api/v2/skills', () => {
     )
   })
 
+  /**
+   * The workspace-key create used to be the case this file pinned analytics
+   * against: it succeeded, and the assertion was that no `skill_created` event
+   * was attributed to a principal with no human subject. `skills.create` now
+   * denies the key outright, so what needs pinning here is the surface's half of
+   * that — the refusal reaches the caller as the operation's own 403, and a
+   * create that never happened emits nothing.
+   */
+  it('refuses a workspace-key create and records no analytics for it', async () => {
+    mocks.create.mockRejectedValueOnce(
+      new PrincipalKindAuthorizationError('workspace_api_key', 'skills.create')
+    )
+
+    const response = await POST(
+      request('POST', '/api/v2/skills', {
+        workspaceId: WORKSPACE_ID,
+        name: skill.name,
+        description: skill.description,
+        content: skill.content,
+      })
+    )
+
+    expect(response.status).toBe(403)
+    expect(mocks.create).toHaveBeenCalledWith(expect.objectContaining({ principal: PRINCIPAL }))
+    expect(mocks.capture).not.toHaveBeenCalled()
+  })
+
   it('authenticates before parsing skill input', async () => {
     mocks.authenticate.mockRejectedValueOnce(new MockV2ApiKeyUnauthenticatedError())
 

apps/sim/lib/api/server/routes/v2-json-route.test.ts

@@ -35,6 +35,7 @@ import {
   defineV2JsonRoute,
   type V2ErrorPolicy,
   v2ApiKeyAuth,
+  v2HeadAuthorizationResponse,
   v2OrchestrationErrorPolicy,
   v2RateLimits,
 } from '@/lib/api/server/routes/v2-json-route'
@@ -623,4 +624,23 @@ describe('defineV2JsonRoute HEAD on a route that is not head-safe', () => {
   it('refuses at definition time to build the route when the use case cannot authorize', () => {
     expect(() => createHeadHandler({ omitAuthorize: true })).toThrow(/authorize/)
   })
+
+  /**
+   * The definition-time guard is what a route hits, and it covers both builders
+   * that answer a `HEAD` this way. This pins the responder's own behaviour if it
+   * is ever reached another way: a missing authorization phase has to fail,
+   * because skipping it hands back the bodiless 200 for a resource nothing
+   * authorized — the leak the guard exists to prevent, restored.
+   */
+  it('refuses to answer 200 when the authorization phase is missing', async () => {
+    await expect(
+      v2HeadAuthorizationResponse({
+        useCase: { authorize: undefined },
+        principal,
+        input: { widgetId: 'widget-1', workspaceId: 'workspace-1' },
+        request: headRequest(),
+        errorPolicy: v2OrchestrationErrorPolicy,
+      })
+    ).rejects.toThrow(/authorize/)
+  })
 })

apps/sim/lib/api/server/routes/v2-json-route.ts

@@ -195,6 +195,14 @@ export function requireHeadAuthorizableUseCase(
  * produced — 400, 401, 403, 404, 429 — and only an authorized caller reaches the
  * 200. What a `HEAD` never reaches is the use case's business phase, so the
  * outbound connection, the row write, and the audit event stay unfired.
+ *
+ * A use case with no `authorize` is refused here rather than skipped. Both
+ * builders that call this already refuse such a route at module load through
+ * {@link requireHeadAuthorizableUseCase}, and they are its only callers, so the
+ * refusal is unreachable through them. It is not written as a comment because
+ * the alternative — an optional call — degrades a missing phase into exactly the
+ * bodiless 200 this function exists to stop, and it does so silently. Failing
+ * closed makes an authorization that actually ran the only route to that 200.
  */
 export async function v2HeadAuthorizationResponse(args: {
   useCase: Pick<OperationUseCase<ApplicationOperation, unknown, unknown>, 'authorize'>
@@ -203,8 +211,14 @@ export async function v2HeadAuthorizationResponse(args: {
   request: NextRequest
   errorPolicy: V2ErrorPolicy
 }): Promise<NextResponse> {
+  const { authorize } = args.useCase
+  if (typeof authorize !== 'function') {
+    throw new Error(
+      'HEAD on a route that is not head-safe reached a use case with no authorize(); answering 200 would leak the existence of a resource the GET never authorized.'
+    )
+  }
   try {
-    await args.useCase.authorize?.({
+    await authorize({
       principal: args.principal,
       input: args.input,
       request: args.request,

apps/sim/lib/folders/queries.test.ts

@@ -17,6 +17,7 @@ import {
   listActiveFolderRows,
   listFoldersForWorkspace,
   loadActiveFolderPathIndex,
+  resolveFolderPathFilter,
   resolveRestoredFolderId,
   toFolderApi,
   wouldCreateFolderCycle,
@@ -318,6 +319,40 @@ describe('folder queries', () => {
     })
   })
 
+  /**
+   * The one place the real helper is exercised. Every list use case that filters
+   * by `folderPath` mocks this module out and stands a reimplementation in for
+   * it, so a defect here — a miss widening to unfiltered, a root path that stops
+   * resolving — would leave all of those suites green while every filtered list
+   * answered with the wrong rows.
+   */
+  describe('resolveFolderPathFilter', () => {
+    const index = {
+      pathById: new Map([['f-1', 'Reports']]),
+      idByPath: new Map([['Reports', 'f-1']]),
+    }
+
+    it('treats an omitted path as no filter at all', () => {
+      expect(resolveFolderPathFilter(index, undefined)).toEqual({ kind: 'unfiltered' })
+    })
+
+    it('resolves the root path to the workspace root rather than to a folder id', () => {
+      expect(resolveFolderPathFilter(index, '/')).toEqual({ kind: 'folder', folderId: null })
+    })
+
+    it('resolves a named path to its folder id', () => {
+      expect(resolveFolderPathFilter(index, 'Reports')).toEqual({ kind: 'folder', folderId: 'f-1' })
+    })
+
+    /**
+     * A path naming no active folder narrows the list to nothing. Widening it to
+     * `unfiltered` would answer a scoped read with every row in the workspace.
+     */
+    it('narrows to nothing for a path that names no active folder', () => {
+      expect(resolveFolderPathFilter(index, 'Archive')).toEqual({ kind: 'noMatch' })
+    })
+  })
+
   describe('toFolderApi', () => {
     it('serializes timestamps to ISO strings and preserves a null deletedAt', () => {
       expect(toFolderApi(ROW)).toMatchObject({

apps/sim/lib/skills/application/operations.test.ts

@@ -27,6 +27,13 @@ describe('skill operation registry', () => {
    * The invariant the create/delete split violated. A principal kind that can
    * create a skill must be able to remove it, or its only possible interaction
    * with the resource is to accumulate rows it can never reach again.
+   *
+   * Symmetry alone is not the property. A lifecycle that uniformly ALLOWED a
+   * workspace key is just as symmetric and reopens the hole, because the edit
+   * paths cannot resolve an acting subject for one. So both halves are pinned:
+   * the writes agree on a policy, and the policy they agree on is the one every
+   * edit path can honour. The principal kinds are compared directly rather than
+   * left to the test's own name.
    */
   it('admits the same principal kinds to every write in the lifecycle', () => {
     const writes = [
@@ -35,9 +42,11 @@ describe('skill operation registry', () => {
       skillOperations.upsert,
       skillOperations.delete,
     ]
-    const policies = writes.map((operation) => operation.workspaceApiKey)
 
-    expect(new Set(policies).size).toBe(1)
+    expect(new Set(writes.map((operation) => operation.workspaceApiKey))).toEqual(new Set(['deny']))
+    for (const operation of writes) {
+      expect(operation.principalKinds).toEqual(skillOperations.delete.principalKinds)
+    }
   })
 
   it('gates every edit path on a human subject rather than workspace role', () => {

apps/sim/lib/table/rows/cursor.test.ts

@@ -218,5 +218,27 @@ describe('tokens minted before the filter stamp', () => {
     expect(() => assertCursorQueryBinding(decoded, { predicate: ACTIVE })).toThrow(
       /Restart paging without the cursor/
     )
+    /**
+     * The code, not just the wording, is what a bumped `CURSOR_VERSION` would
+     * cost: every in-flight token would fail `INVALID_CURSOR` at decode instead,
+     * including the unfiltered ones that resume fine today.
+     */
+    try {
+      assertCursorQueryBinding(decoded, { predicate: ACTIVE })
+      expect.unreachable('a re-filtered replay must be refused')
+    } catch (e) {
+      expect((e as TableQueryValidationError).code).toBe('CURSOR_FILTER_CONFLICT')
+    }
+  })
+
+  /**
+   * The version a token minted today carries. Pinned so a bump is a deliberate
+   * edit here rather than a silent one that strands every cursor a running
+   * deploy already handed out.
+   */
+  it('mints tokens at the version the previous deploy could already read', () => {
+    const token = encodeCursor({ lastRow: ROW, keysetValid: true, nextOffset: 10 })
+
+    expect(JSON.parse(Buffer.from(token, 'base64url').toString('utf8')).v).toBe(1)
   })
 })

apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.test.ts

@@ -3,6 +3,7 @@
  */
 
 import { describe, expect, it } from 'vitest'
+import { LOCAL_UPLOAD_METADATA_SUFFIX } from '@/lib/uploads/core/storage-key'
 import {
   findWorkspaceFileRecord,
   generateWorkspaceFileKey,
@@ -92,12 +93,26 @@ describe('workspace file reference normalization', () => {
   })
 })
 
+/**
+ * The only place the real key builder is measured — the upload-session suites
+ * stand a stub in for it — so the budget is asserted here the way the filesystem
+ * enforces it. Local storage writes a metadata sidecar beside the object under
+ * the object's own name, so `NAME_MAX` bounds the key's last component PLUS that
+ * suffix, not the component alone. Measuring the component alone passes with the
+ * sidecar reservation removed, and the overflow returns as an `ENAMETOOLONG` 500
+ * on a name the contract already admitted.
+ */
 describe('workspace file storage keys', () => {
-  it('keeps the last key component within one path component for the longest admitted name', () => {
+  /** POSIX `NAME_MAX`, in bytes, for one path component. */
+  const NAME_MAX = 255
+
+  it('leaves the longest admitted name room for its local sidecar', () => {
     const key = generateWorkspaceFileKey('ws_123', `${'a'.repeat(251)}.txt`)
     const lastSegment = key.slice(key.lastIndexOf('/') + 1)
 
-    expect(Buffer.byteLength(lastSegment, 'utf-8')).toBeLessThanOrEqual(255)
+    expect(
+      Buffer.byteLength(`${lastSegment}${LOCAL_UPLOAD_METADATA_SUFFIX}`, 'utf-8')
+    ).toBeLessThanOrEqual(NAME_MAX)
     expect(key.startsWith('workspace/ws_123/')).toBe(true)
     expect(lastSegment.endsWith('.txt')).toBe(true)
   })

apps/sim/lib/uploads/upload-session/provider.test.ts

@@ -1,9 +1,16 @@
 /**
  * @vitest-environment node
  */
-import { mkdir, readdir, readFile, rm, stat } from 'node:fs/promises'
+import { link, mkdir, readdir, readFile, rm, stat } from 'node:fs/promises'
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
 
+/**
+ * Spied rather than replaced: every assertion in this file reads the real
+ * filesystem, and the only behaviour worth faking is a single `link` answering
+ * `EXDEV`, which no temporary directory can be made to produce on its own.
+ */
+vi.mock('node:fs/promises', { spy: true })
+
 const { testUploadDirectory, mockS3Presign, mockS3PartUrls } = vi.hoisted(() => ({
   testUploadDirectory: `/tmp/sim-upload-session-provider-${process.pid}`,
   mockS3Presign: vi.fn(),
@@ -117,6 +124,60 @@ describe('local upload-session provider', () => {
     })
   })
 
+  /**
+   * Staging moved out of the destination's own directory into one shared
+   * `.staging` root, which is what makes this reachable: a volume mounted under
+   * part of the uploads tree puts the staged object and its destination on
+   * different devices, and a hard link cannot span them. Publication has to
+   * survive that without giving up the create-or-fail the link provides.
+   */
+  it('publishes across a filesystem boundary a hard link cannot span', async () => {
+    vi.mocked(link).mockRejectedValueOnce(
+      Object.assign(new Error('EXDEV: cross-device link'), { code: 'EXDEV' })
+    )
+
+    await writeLocalPutObject({
+      uploadId: 'upload-1',
+      key: 'workspace/workspace-1/file.bin',
+      body: byteStream('ab', 'cd'),
+      expectedSize: 4,
+      contentType: 'application/octet-stream',
+      metadata: METADATA,
+    })
+
+    await expect(readFile(localPath('workspace/workspace-1/file.bin'), 'utf8')).resolves.toBe(
+      'abcd'
+    )
+    await expect(
+      headProviderObject({
+        provider: 'local',
+        key: 'workspace/workspace-1/file.bin',
+        context: CONTEXT,
+      })
+    ).resolves.toMatchObject({ size: 4, uploadId: 'upload-1' })
+    expect(await temporaryFiles('workspace/workspace-1')).toEqual([])
+    expect(await allEntries('.staging')).toEqual([])
+  })
+
+  it('still refuses to overwrite an existing object when the link cannot span devices', async () => {
+    const params = {
+      uploadId: 'upload-1',
+      key: 'workspace/workspace-1/file.bin',
+      expectedSize: 3,
+      contentType: 'application/octet-stream',
+      metadata: METADATA,
+    }
+    await writeLocalPutObject({ ...params, body: byteStream('one') })
+    vi.mocked(link).mockRejectedValueOnce(
+      Object.assign(new Error('EXDEV: cross-device link'), { code: 'EXDEV' })
+    )
+
+    await expect(writeLocalPutObject({ ...params, body: byteStream('two') })).rejects.toThrow()
+
+    await expect(readFile(localPath(params.key), 'utf8')).resolves.toBe('one')
+    expect(await temporaryFiles('workspace/workspace-1')).toEqual([])
+  })
+
   it('does not let a replayed PUT overwrite the final object', async () => {
     const params = {
       uploadId: 'upload-1',