fix(pentest): address cubic review findings on finding-context flow

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

@@ -28,6 +28,18 @@ describe('normalizeTargetUrl', () => {
     );
   });
 
+  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');
   });

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

@@ -12,19 +12,26 @@ export interface FindingContextNote {
 /**
  * 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`). Non-URL input
- * is returned trimmed — the create DTO already enforces a valid URL.
+ * (`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 = '';
-    let normalized = url.toString();
-    while (normalized.endsWith('/')) {
-      normalized = normalized.slice(0, -1);
+    while (url.pathname.length > 1 && url.pathname.endsWith('/')) {
+      url.pathname = url.pathname.slice(0, -1);
     }
-    return normalized;
+    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;
   }

apps/api/src/security-penetration-tests/report-appendix.util.spec.ts

@@ -1,7 +1,8 @@
-import { PDFDocument } from 'pdf-lib';
+import { PDFDocument, StandardFonts } from 'pdf-lib';
 import {
   appendContextNotesToMarkdown,
   appendContextNotesToPdf,
+  wrapText,
   type ReportContextNote,
 } from './report-appendix.util';
 
@@ -108,6 +109,28 @@ describe('appendContextNotesToPdf', () => {
     expect(merged.getPageCount()).toBe(2);
   });
 
+  it('hard-wraps unbroken tokens wider than the line (long URLs/IDs)', async () => {
+    const doc = await PDFDocument.create();
+    const font = await doc.embedFont(StandardFonts.Helvetica);
+    const maxWidth = 483; // appendix body width: 595 - 2 * 56
+    const longUrl = `https://app.example.com/api/v1/resources/${'a'.repeat(300)}`;
+
+    const lines = wrapText({
+      text: `Remediated, see ${longUrl} for the change.`,
+      font,
+      fontSize: 10,
+      maxWidth,
+    });
+
+    expect(lines.length).toBeGreaterThan(1);
+    for (const line of lines) {
+      expect(font.widthOfTextAtSize(line, 10)).toBeLessThanOrEqual(maxWidth);
+    }
+    // Nothing got dropped while splitting.
+    expect(lines.join('')).toContain('aaaaaaaaaa');
+    expect(lines.join(' ')).toContain('Remediated,');
+  });
+
   it('throws on unparseable provider bytes (caller falls back to original)', async () => {
     await expect(
       appendContextNotesToPdf({

apps/api/src/security-penetration-tests/report-appendix.util.ts

@@ -61,7 +61,38 @@ function sanitizePdfText(text: string): string {
     .replace(/[^\n\x20-\x7E¡-ÿ]/g, '?');
 }
 
-function wrapText(params: {
+// Character-level split for a single token wider than the line (long
+// URLs, IDs). Without this the token would be emitted as one overflowing
+// line and run off the page margin.
+function splitLongWord(params: {
+  word: string;
+  font: PDFFont;
+  fontSize: number;
+  maxWidth: number;
+}): string[] {
+  const parts: string[] = [];
+  let current = '';
+  for (const char of params.word) {
+    const candidate = current + char;
+    if (
+      params.font.widthOfTextAtSize(candidate, params.fontSize) >
+        params.maxWidth &&
+      current
+    ) {
+      parts.push(current);
+      current = char;
+    } else {
+      current = candidate;
+    }
+  }
+  if (current) {
+    parts.push(current);
+  }
+  return parts;
+}
+
+/** Exported for tests — every returned line fits within maxWidth. */
+export function wrapText(params: {
   text: string;
   font: PDFFont;
   fontSize: number;
@@ -77,6 +108,18 @@ function wrapText(params: {
 
     let currentLine = '';
     for (const word of paragraph.split(' ')) {
+      const wordWidth = params.font.widthOfTextAtSize(word, params.fontSize);
+      if (wordWidth > params.maxWidth) {
+        if (currentLine) {
+          lines.push(currentLine);
+          currentLine = '';
+        }
+        const parts = splitLongWord({ ...params, word });
+        lines.push(...parts.slice(0, -1));
+        currentLine = parts[parts.length - 1] ?? '';
+        continue;
+      }
+
       const candidate = currentLine ? `${currentLine} ${word}` : word;
       const candidateWidth = params.font.widthOfTextAtSize(
         candidate,

apps/api/src/security-penetration-tests/security-penetration-tests.service.spec.ts

@@ -277,6 +277,28 @@ describe('SecurityPenetrationTestsService', () => {
     );
   });
 
+  it('creates the run even when the notes lookup fails (best-effort context)', async () => {
+    fetchMock.mockResolvedValueOnce(
+      new Response(JSON.stringify({ id: 'run_456', status: 'provisioning' }), {
+        status: 200,
+      }),
+    );
+    // e.g. transient outage, or the table missing mid-deploy before the
+    // migration has run — must never block pentest creation.
+    mockedDb.securityPenetrationTestFindingContext.findMany.mockRejectedValue(
+      new Error('relation does not exist'),
+    );
+
+    const result = await service.createReport('org_123', {
+      targetUrl: 'https://app.example.com',
+      additionalContext: 'User-typed context.',
+    });
+
+    expect(result.id).toBe('run_456');
+    const requestBody = await getRequestBody();
+    expect(requestBody.additionalContext).toBe('User-typed context.');
+  });
+
   it('appends stored finding-context notes for the normalized target to additionalContext', async () => {
     fetchMock.mockResolvedValueOnce(
       new Response(JSON.stringify({ id: 'run_456', status: 'provisioning' }), {

apps/api/src/security-penetration-tests/security-penetration-tests.service.ts

@@ -461,20 +461,20 @@ export class SecurityPenetrationTestsService {
    * context notes customers saved on findings from previous scans of the
    * same target (see PentestFindingContextsService). This is what makes a
    * re-run an informed retest instead of a blind one.
+   *
+   * The notes lookup is best-effort: a DB failure (transient outage, or
+   * the table missing mid-deploy before the migration runs) must never
+   * block creating a pentest — the run proceeds with whatever context
+   * the caller typed, and the miss is logged.
    */
   private async resolveAdditionalContext(
     organizationId: string,
     payload: CreatePenetrationTestDto,
   ): Promise<string | undefined> {
-    const findingContexts =
-      await db.securityPenetrationTestFindingContext.findMany({
-        where: {
-          organizationId,
-          targetUrl: normalizeTargetUrl(payload.targetUrl),
-        },
-        orderBy: { createdAt: 'asc' },
-        select: { issueTitle: true, context: true },
-      });
+    const findingContexts = await this.findContextNotesQuietly(
+      organizationId,
+      payload.targetUrl,
+    );
 
     return buildAdditionalContext({
       userProvidedContext: payload.additionalContext,