Merge pull request #3111 from trycompai/main

[comp] Production Deploy

Author: tofikwest

apps/api/src/security-penetration-tests/dto/create-penetration-test.dto.ts

@@ -4,7 +4,9 @@ import {
   IsBoolean,
   IsEnum,
   IsOptional,
+  IsString,
   IsUrl,
+  MaxLength,
 } from 'class-validator';
 import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger';
 
@@ -107,4 +109,15 @@ export class CreatePenetrationTestDto {
   @ArrayUnique()
   @IsEnum(pentestCheckValues, { each: true })
   checks?: PentestCheck[];
+
+  @ApiPropertyOptional({
+    description:
+      'Free-text context shared with the testing agent, e.g. remediation notes or accepted-by-design explanations from a previous run. Saved per-finding context notes for the same target are appended automatically. Max 4000 characters.',
+    required: false,
+    maxLength: 4000,
+  })
+  @IsOptional()
+  @IsString()
+  @MaxLength(4000)
+  additionalContext?: string;
 }

apps/api/src/security-penetration-tests/dto/upsert-finding-context.dto.ts

@@ -0,0 +1,24 @@
+import { ApiProperty } from '@nestjs/swagger';
+import { IsNotEmpty, IsString, MaxLength } from 'class-validator';
+
+export class UpsertFindingContextDto {
+  @ApiProperty({
+    description: 'Penetration test run ID the finding belongs to',
+    example: 'pentest-abc123',
+  })
+  @IsString()
+  @IsNotEmpty()
+  runId!: string;
+
+  @ApiProperty({
+    description:
+      'Context for the finding, e.g. an accepted-by-design rationale or remediation details. Shared with the testing agent on future scans of the same target. Max 2000 characters.',
+    example:
+      'Read access to appConfiguration is accepted by design: the collection only holds non-secret bootstrap configuration and write access is restricted to privileged users.',
+    maxLength: 2000,
+  })
+  @IsString()
+  @IsNotEmpty()
+  @MaxLength(2000)
+  context!: string;
+}

apps/api/src/security-penetration-tests/finding-context.util.spec.ts

@@ -0,0 +1,130 @@
+import {
+  buildAdditionalContext,
+  MAX_ADDITIONAL_CONTEXT_LENGTH,
+  normalizeTargetUrl,
+} from './finding-context.util';
+
+describe('normalizeTargetUrl', () => {
+  it('lowercases the host and strips trailing slashes', () => {
+    expect(normalizeTargetUrl('https://App.Example.com/')).toBe(
+      'https://app.example.com',
+    );
+  });
+
+  it('strips trailing slashes from paths but keeps the path itself', () => {
+    expect(normalizeTargetUrl('https://app.example.com/portal/')).toBe(
+      'https://app.example.com/portal',
+    );
+  });
+
+  it('drops URL fragments', () => {
+    expect(normalizeTargetUrl('https://app.example.com/#section')).toBe(
+      'https://app.example.com',
+    );
+  });
+
+  it('keeps query strings', () => {
+    expect(normalizeTargetUrl('https://app.example.com/?env=staging')).toBe(
+      'https://app.example.com/?env=staging',
+    );
+  });
+
+  it('preserves a trailing slash that belongs to a query value', () => {
+    expect(
+      normalizeTargetUrl('https://app.example.com/?next=/portal/'),
+    ).toBe('https://app.example.com/?next=/portal/');
+  });
+
+  it('strips path trailing slashes while keeping the query intact', () => {
+    expect(normalizeTargetUrl('https://app.example.com/app/?v=2')).toBe(
+      'https://app.example.com/app?v=2',
+    );
+  });
+
+  it('returns non-URL input trimmed', () => {
+    expect(normalizeTargetUrl('  not a url  ')).toBe('not a url');
+  });
+});
+
+describe('buildAdditionalContext', () => {
+  it('returns undefined when there is nothing to send', () => {
+    expect(
+      buildAdditionalContext({ findingContexts: [] }),
+    ).toBeUndefined();
+    expect(
+      buildAdditionalContext({
+        userProvidedContext: '   ',
+        findingContexts: [],
+      }),
+    ).toBeUndefined();
+  });
+
+  it('returns only the user-provided context when there are no notes', () => {
+    expect(
+      buildAdditionalContext({
+        userProvidedContext: '  Focus on auth flows.  ',
+        findingContexts: [],
+      }),
+    ).toBe('Focus on auth flows.');
+  });
+
+  it('formats stored notes as a numbered list with titles', () => {
+    const result = buildAdditionalContext({
+      findingContexts: [
+        { issueTitle: 'appConfiguration read access', context: 'By design.' },
+        { issueTitle: 'Storage attachment access', context: 'Hardened.' },
+      ],
+    });
+
+    expect(result).toContain('1. "appConfiguration read access": By design.');
+    expect(result).toContain('2. "Storage attachment access": Hardened.');
+    expect(result).toContain('Customer-provided context');
+  });
+
+  it('places the user context before the stored notes', () => {
+    const result = buildAdditionalContext({
+      userProvidedContext: 'Retest of the May findings.',
+      findingContexts: [{ issueTitle: 'Issue A', context: 'Fixed.' }],
+    });
+
+    expect(result).toBeDefined();
+    const userIndex = (result as string).indexOf('Retest of the May findings.');
+    const notesIndex = (result as string).indexOf('1. "Issue A": Fixed.');
+    expect(userIndex).toBeGreaterThanOrEqual(0);
+    expect(notesIndex).toBeGreaterThan(userIndex);
+  });
+
+  it('does not add an omission marker when everything fits', () => {
+    const result = buildAdditionalContext({
+      userProvidedContext: 'User intent.',
+      findingContexts: [{ issueTitle: 'Issue A', context: 'Fixed.' }],
+    });
+
+    expect(result).not.toContain('omitted for length');
+  });
+
+  it('caps the composed briefing, keeping user context and marking omitted notes', () => {
+    const findingContexts = Array.from({ length: 30 }, (_, i) => ({
+      issueTitle: `Finding ${i + 1}`,
+      context: 'x'.repeat(1900),
+    }));
+
+    const result = buildAdditionalContext({
+      userProvidedContext: 'User intent.',
+      findingContexts,
+    });
+
+    expect(result).toBeDefined();
+    expect((result as string).length).toBeLessThanOrEqual(
+      MAX_ADDITIONAL_CONTEXT_LENGTH,
+    );
+    expect(result).toContain('User intent.');
+    expect(result).toContain('1. "Finding 1"');
+    expect(result).toMatch(/\d+ more notes omitted for length/);
+    // Whole notes are dropped, never cut: every included note line ends
+    // with its full 1900-char body.
+    const includedBodies = (result as string).match(/x{1900}/g) ?? [];
+    expect(includedBodies.length).toBeGreaterThan(0);
+    expect(result).not.toMatch(/x{1901,}/);
+  });
+});

apps/api/src/security-penetration-tests/finding-context.util.ts

@@ -0,0 +1,98 @@
+/**
+ * Pure helpers for pentest finding-context notes. Kept free of Nest/DB
+ * imports so both the storage service and the run-creation path can use
+ * them, and so they unit-test without mocks.
+ */
+
+export interface FindingContextNote {
+  issueTitle: string;
+  context: string;
+}
+
+/**
+ * Canonical form of a scan target so notes written on a run match future
+ * runs of the same target regardless of casing or a trailing slash
+ * (`https://App.example.com/` ≡ `https://app.example.com`). Only the
+ * path's trailing slashes are stripped — a `/` at the end of a query
+ * value belongs to that value and must survive. Non-URL input is
+ * returned trimmed — the create DTO already enforces a valid URL.
+ */
+export function normalizeTargetUrl(value: string): string {
+  const trimmed = value.trim();
+  try {
+    const url = new URL(trimmed);
+    url.hash = '';
+    while (url.pathname.length > 1 && url.pathname.endsWith('/')) {
+      url.pathname = url.pathname.slice(0, -1);
+    }
+    const normalized = url.toString();
+    // URL always renders the bare root path with a trailing slash
+    // (`https://x.com/`); drop it for origin-only targets so keys stay
+    // in the `https://x.com` form.
+    return url.pathname === '/' && !url.search
+      ? normalized.replace(/\/$/, '')
+      : normalized;
+  } catch {
+    return trimmed;
+  }
+}
+
+/**
+ * Budget for the composed briefing. The provider has no documented limit
+ * (verified against its OpenAPI spec and prompt-injection source), but an
+ * unbounded string composed from arbitrarily many notes shouldn't be sent
+ * to an external API or an agent prompt. When over budget, whole notes
+ * are dropped — never cut mid-sentence — and an explicit omission marker
+ * tells the agent the list is incomplete.
+ */
+export const MAX_ADDITIONAL_CONTEXT_LENGTH = 20_000;
+
+const NOTES_HEADER =
+  'Customer-provided context on findings reported in previous scans of this target. ' +
+  'Take it into account when validating and reporting findings (e.g. behavior that is ' +
+  'accepted by design, or issues the customer has since remediated):';
+
+/**
+ * Composes the `additionalContext` string sent to the pentest provider on
+ * run creation: the user's free-text context for this run (if any) followed
+ * by the stored per-finding notes from previous scans of the same target.
+ * Returns undefined when there is nothing to send.
+ */
+export function buildAdditionalContext(params: {
+  userProvidedContext?: string;
+  findingContexts: FindingContextNote[];
+}): string | undefined {
+  const userSection = params.userProvidedContext?.trim();
+
+  if (params.findingContexts.length === 0) {
+    return userSection || undefined;
+  }
+
+  const noteLines = params.findingContexts.map(
+    (note, index) =>
+      `${index + 1}. "${note.issueTitle.trim()}": ${note.context.trim()}`,
+  );
+
+  const compose = (includedCount: number): string => {
+    const omitted = noteLines.length - includedCount;
+    const lines = noteLines.slice(0, includedCount);
+    if (omitted > 0) {
+      lines.push(
+        `(${omitted} more note${omitted === 1 ? '' : 's'} omitted for length — see the finding context notes in Comp AI)`,
+      );
+    }
+    const sections = userSection ? [userSection] : [];
+    sections.push([NOTES_HEADER, ...lines].join('\n'));
+    return sections.join('\n\n');
+  };
+
+  let included = noteLines.length;
+  while (
+    included > 0 &&
+    compose(included).length > MAX_ADDITIONAL_CONTEXT_LENGTH
+  ) {
+    included -= 1;
+  }
+
+  return compose(included);
+}

apps/api/src/security-penetration-tests/pentest-finding-contexts.controller.spec.ts

@@ -0,0 +1,109 @@
+// The controller's runtime import chain reaches `@db`, which instantiates
+// a Prisma client at module load. These tests never touch the database.
+jest.mock('@db', () => ({ db: {} }));
+
+jest.mock('../auth/hybrid-auth.guard', () => ({
+  HybridAuthGuard: class {
+    canActivate() {
+      return true;
+    }
+  },
+}));
+
+// The real PermissionGuard transitively imports better-auth's ESM bundle,
+// which Jest cannot parse. The mock must keep PERMISSIONS_KEY's real value
+// ('required_permissions') because @RequirePermission writes its metadata
+// under it at class-definition time.
+jest.mock('../auth/permission.guard', () => ({
+  PermissionGuard: class {
+    canActivate() {
+      return true;
+    }
+  },
+  PERMISSIONS_KEY: 'required_permissions',
+}));
+
+import { PERMISSIONS_KEY } from '../auth/permission.guard';
+import type { RequiredPermission } from '../auth/permission.guard';
+import { PentestFindingContextsController } from './pentest-finding-contexts.controller';
+import type { PentestFindingContextsService } from './pentest-finding-contexts.service';
+
+describe('PentestFindingContextsController', () => {
+  const listForTargetMock = jest.fn();
+  const upsertMock = jest.fn();
+  const removeMock = jest.fn();
+
+  const serviceMock = {
+    listForTarget: listForTargetMock,
+    upsert: upsertMock,
+    remove: removeMock,
+  } as unknown as jest.Mocked<PentestFindingContextsService>;
+
+  const controller = new PentestFindingContextsController(serviceMock);
+
+  beforeEach(() => {
+    jest.clearAllMocks();
+  });
+
+  it('lists context notes for a target', async () => {
+    const expected = [{ id: 'ptfc_1', providerIssueId: 'issue_1' }];
+    listForTargetMock.mockResolvedValueOnce(expected);
+
+    const response = await controller.list(
+      'org_123',
+      'https://app.example.com',
+    );
+
+    expect(listForTargetMock).toHaveBeenCalledWith(
+      'org_123',
+      'https://app.example.com',
+    );
+    expect(response).toEqual(expected);
+  });
+
+  it('upserts a context note for a finding', async () => {
+    const expected = { id: 'ptfc_1', providerIssueId: 'issue_1' };
+    upsertMock.mockResolvedValueOnce(expected);
+
+    const response = await controller.upsert('org_123', 'issue_1', {
+      runId: 'run_123',
+      context: 'Accepted by design.',
+    });
+
+    expect(upsertMock).toHaveBeenCalledWith({
+      organizationId: 'org_123',
+      issueId: 'issue_1',
+      runId: 'run_123',
+      context: 'Accepted by design.',
+    });
+    expect(response).toEqual(expected);
+  });
+
+  it('removes a context note for a finding', async () => {
+    removeMock.mockResolvedValueOnce({ deleted: true });
+
+    const response = await controller.remove('org_123', 'issue_1');
+
+    expect(removeMock).toHaveBeenCalledWith({
+      organizationId: 'org_123',
+      issueId: 'issue_1',
+    });
+    expect(response).toEqual({ deleted: true });
+  });
+
+  it('requires pentest permissions on every endpoint', () => {
+    const requirements = (['list', 'upsert', 'remove'] as const).map(
+      (method) =>
+        Reflect.getMetadata(
+          PERMISSIONS_KEY,
+          PentestFindingContextsController.prototype[method],
+        ) as RequiredPermission[] | undefined,
+    );
+
+    expect(requirements).toEqual([
+      [{ resource: 'pentest', actions: ['read'] }],
+      [{ resource: 'pentest', actions: ['update'] }],
+      [{ resource: 'pentest', actions: ['update'] }],
+    ]);
+  });
+});