From 51e7d1805276715b856f52a280f9715347040872 Mon Sep 17 00:00:00 2001 From: Armin Fauland Date: Tue, 4 Aug 2026 12:57:18 +0000 Subject: [PATCH 1/2] fix(api): redact secrets in run debug data instead of storing them in clear text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The debug data collected for a run contains the resolved variables, and those include secrets. It was serialised and written to scrape_data verbatim, so a login password ended up in the database in clear text: sqlite> SELECT value FROM scrape_data WHERE key='__debugData'; {"var_email":"user@example.com","var_password":"", …} GET /api/runs/:runId/debug then hands that row out unchanged. Anything else reading it — a backup, a database dump, the artifacts endpoint — does the same. The project already has SecretRedactionService, and ScrapeExecutionService already has it injected; the debug path just never used it. Redaction is applied at the source, before the row is written, rather than on the way out. Redacting only on read would leave the plaintext at rest, where backups and dumps still pick it up. Both read paths (debug and artifacts) redact as well, as a safety net for rows written before this change. That only works while the secrets are still registered in the running process, so it is explicitly not a substitute for the fix at the source — existing rows should be cleared: DELETE FROM scrape_data WHERE key = '__debugData'; Tests: one at the source proving the plaintext never reaches storeData, one on the endpoint proving it is not handed out. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LixHBPkhb8h5oDdMqSG4se --- .../src/scrape/scrape-ui.controller.spec.ts | 41 +++++++++++++++++++ apps/api/src/scrape/scrape-ui.controller.ts | 16 +++++++- .../services/scrape-execution.service.spec.ts | 25 ++++++++++- .../services/scrape-execution.service.ts | 9 +++- 4 files changed, 87 insertions(+), 4 deletions(-) diff --git a/apps/api/src/scrape/scrape-ui.controller.spec.ts b/apps/api/src/scrape/scrape-ui.controller.spec.ts index bba2941..bca2cc6 100644 --- a/apps/api/src/scrape/scrape-ui.controller.spec.ts +++ b/apps/api/src/scrape/scrape-ui.controller.spec.ts @@ -8,6 +8,7 @@ import { SseTicketService } from './sse-ticket.service'; import { SchedulerService } from './scheduler.service'; import { DatabaseService } from '../database/database.service'; import { AuthorResolverService } from './author-resolver.service'; +import { SecretRedactionService } from '../_logger/secret-redaction.service'; import { of } from 'rxjs'; describe('ScrapeUIController', () => { @@ -18,6 +19,7 @@ describe('ScrapeUIController', () => { let mockSchedulerService: any; let mockDatabaseService: any; let mockAuthorResolverService: any; + let mockSecretRedaction: any; const createMockResponse = () => { const res: any = { @@ -96,6 +98,14 @@ describe('ScrapeUIController', () => { .mockImplementation((meta) => Promise.resolve(meta || {})), }; + // Gibt das Objekt unveraendert zurueck, ersetzt aber einen bekannten + // Geheimwert — so laesst sich pruefen, DASS geschwaerzt wird. + mockSecretRedaction = { + redactObject: vi.fn((o) => + JSON.parse(JSON.stringify(o).replaceAll('geheim123', '***')), + ), + }; + const module: TestingModule = await Test.createTestingModule({ controllers: [ScrapeUIController], providers: [ @@ -105,6 +115,10 @@ describe('ScrapeUIController', () => { { provide: SchedulerService, useValue: mockSchedulerService }, { provide: DatabaseService, useValue: mockDatabaseService }, { provide: AuthorResolverService, useValue: mockAuthorResolverService }, + { + provide: SecretRedactionService, + useValue: mockSecretRedaction, + }, ], }).compile(); @@ -803,6 +817,33 @@ describe('ScrapeUIController', () => { // ============ GET /runs/:runId/debug ============ describe('getRunDebugData', () => { + it('does not hand out credentials stored in the debug data', async () => { + mockDatabaseService.getRun = vi + .fn() + .mockResolvedValue({ id: 'run-1', scrapeId: 'amazon' }); + mockDatabaseService.dataSource = { + getRepository: vi.fn().mockReturnValue({ + findOne: vi.fn().mockResolvedValue({ + value: JSON.stringify({ + var_email: 'user@example.com', + var_password: 'geheim123', + }), + }), + }), + }; + + const res: any = { + status: vi.fn().mockReturnThis(), + json: vi.fn().mockReturnThis(), + }; + + await controller.getRunDebugData('run-1', res); + + expect(mockSecretRedaction.redactObject).toHaveBeenCalled(); + const ausgeliefert = JSON.stringify(res.json.mock.calls[0][0]); + expect(ausgeliefert).not.toContain('geheim123'); + expect(ausgeliefert).toContain('***'); + }); it('should return 404 if run not found', async () => { const res = createMockResponse(); mockDatabaseService.getRun.mockResolvedValue(null); diff --git a/apps/api/src/scrape/scrape-ui.controller.ts b/apps/api/src/scrape/scrape-ui.controller.ts index cb86e5f..be6771b 100644 --- a/apps/api/src/scrape/scrape-ui.controller.ts +++ b/apps/api/src/scrape/scrape-ui.controller.ts @@ -20,6 +20,7 @@ import { SseTicketService } from './sse-ticket.service'; import { SchedulerService } from './scheduler.service'; import { DatabaseService } from '../database/database.service'; import { AuthorResolverService } from './author-resolver.service'; +import { SecretRedactionService } from '../_logger/secret-redaction.service'; import { Public } from '../auth/decorators/public.decorator'; import { Observable, map } from 'rxjs'; import * as jsonata from 'jsonata'; @@ -56,6 +57,7 @@ export class ScrapeUIController { private schedulerService: SchedulerService, private databaseService: DatabaseService, private authorResolverService: AuthorResolverService, + private secretRedaction: SecretRedactionService, ) {} @Get('scrapes') @@ -453,7 +455,12 @@ export class ScrapeUIController { return; } - const debugData = JSON.parse(debugDataEntry.value); + // Auch beim Ausliefern geschwaerzt — fuer Zeilen, die vor dem Fix + // an der Quelle geschrieben wurden. Wirkt nur, solange die Secrets + // in diesem Prozess registriert sind: Sicherheitsnetz, kein Ersatz. + const debugData = this.secretRedaction.redactObject( + JSON.parse(debugDataEntry.value), + ); res.status(HttpStatus.OK).json(debugData); } catch (error) { res.status(HttpStatus.INTERNAL_SERVER_ERROR).json({ @@ -494,7 +501,12 @@ export class ScrapeUIController { return; } - const debugData = JSON.parse(debugDataEntry.value); + // Auch beim Ausliefern geschwaerzt — fuer Zeilen, die vor dem Fix + // an der Quelle geschrieben wurden. Wirkt nur, solange die Secrets + // in diesem Prozess registriert sind: Sicherheitsnetz, kein Ersatz. + const debugData = this.secretRedaction.redactObject( + JSON.parse(debugDataEntry.value), + ); // Extract artifacts from debug data // Strategy: Only extract artifacts from the deepest loop level to avoid duplicates diff --git a/apps/api/src/scrape/services/scrape-execution.service.spec.ts b/apps/api/src/scrape/services/scrape-execution.service.spec.ts index 01a86f4..feb2643 100644 --- a/apps/api/src/scrape/services/scrape-execution.service.spec.ts +++ b/apps/api/src/scrape/services/scrape-execution.service.spec.ts @@ -19,6 +19,7 @@ vi.mock('../../_logger/scrape-logger.service', () => { describe('ScrapeExecutionService', () => { let service: ScrapeExecutionService; + let mockSecretRedaction: any; let mockPuppeteerService: any; let mockActionHandlerService: any; let mockScrapeEventsService: any; @@ -76,7 +77,7 @@ describe('ScrapeExecutionService', () => { publish: vi.fn().mockResolvedValue(undefined), }; - const mockSecretRedaction = { + mockSecretRedaction = { registerSecret: vi.fn(), redact: vi.fn((msg: string) => msg), redactObject: vi.fn((obj: any) => obj), @@ -577,6 +578,28 @@ describe('ScrapeExecutionService', () => { ); }); + it('redacts the debug data before it reaches the database', async () => { + // Kern der Sache: der Klartext darf gar nicht erst gespeichert werden. + // Nur beim Ausliefern zu schwaerzen liesse ihn in der Datenbank, in + // jedem Backup und in jedem Dump stehen. + mockSecretRedaction.redactObject.mockImplementation((o: any) => + JSON.parse(JSON.stringify(o).replaceAll('geheim123', '***')), + ); + + const previousData = new Map(); + previousData.set('var_password', 'geheim123'); + + mockActionHandlerService.handleAction.mockResolvedValue(undefined); + await service.executeScrape(createScrape(), 'run-1', previousData, {}); + + expect(mockSecretRedaction.redactObject).toHaveBeenCalled(); + const gespeichert = mockDatabaseService.storeData.mock.calls.find( + (c: any[]) => c[1] === '__debugData', + ); + expect(gespeichert[2]).not.toContain('geheim123'); + expect(gespeichert[2]).toContain('***'); + }); + it('should build debug data summarizing loop results', async () => { const previousData = new Map(); previousData.set('loopResult', { iterations: [1, 2], total: 5 }); diff --git a/apps/api/src/scrape/services/scrape-execution.service.ts b/apps/api/src/scrape/services/scrape-execution.service.ts index 8f6799c..7e2de92 100644 --- a/apps/api/src/scrape/services/scrape-execution.service.ts +++ b/apps/api/src/scrape/services/scrape-execution.service.ts @@ -84,7 +84,14 @@ export class ScrapeExecutionService { ); // Build and save debug data - const debugData = this.buildDebugData(previousData); + // Redacted BEFORE persisting, not on the way out: the run's variables + // include resolved secrets, so an unredacted debugData writes login + // credentials to the database in plain text. Anything that later reads + // that row — the debug endpoint, a backup, a database dump — hands them + // out. Redacting only on read would leave the plaintext at rest. + const debugData = this.secretRedaction.redactObject( + this.buildDebugData(previousData), + ); await this.databaseService.storeData( scrape.id, '__debugData', From 71eff1c395cf32e081d584195e4ae55bd7a26fa4 Mon Sep 17 00:00:00 2001 From: Armin Fauland Date: Tue, 4 Aug 2026 13:46:15 +0000 Subject: [PATCH 2/2] test(api): use an obvious placeholder instead of a password-like fixture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitGuardian flagged the previous test value as a "Generic Password" on PR #153. It was never a real credential — an invented string — but sitting next to a `var_password` key it looks like one to a scanner, and a false positive on a security PR costs the reviewer attention. Replaced with `<>`, which reads as a placeholder to humans and scanners alike. Both tests still assert the same thing: the value must not appear in what is stored, and must not appear in what the endpoint returns. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LixHBPkhb8h5oDdMqSG4se --- apps/api/src/scrape/scrape-ui.controller.spec.ts | 8 +++++--- .../scrape/services/scrape-execution.service.spec.ts | 11 ++++++++--- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/apps/api/src/scrape/scrape-ui.controller.spec.ts b/apps/api/src/scrape/scrape-ui.controller.spec.ts index bca2cc6..e1b05fe 100644 --- a/apps/api/src/scrape/scrape-ui.controller.spec.ts +++ b/apps/api/src/scrape/scrape-ui.controller.spec.ts @@ -102,7 +102,9 @@ describe('ScrapeUIController', () => { // Geheimwert — so laesst sich pruefen, DASS geschwaerzt wird. mockSecretRedaction = { redactObject: vi.fn((o) => - JSON.parse(JSON.stringify(o).replaceAll('geheim123', '***')), + JSON.parse( + JSON.stringify(o).replaceAll('<>', '***'), + ), ), }; @@ -826,7 +828,7 @@ describe('ScrapeUIController', () => { findOne: vi.fn().mockResolvedValue({ value: JSON.stringify({ var_email: 'user@example.com', - var_password: 'geheim123', + var_password: '<>', }), }), }), @@ -841,7 +843,7 @@ describe('ScrapeUIController', () => { expect(mockSecretRedaction.redactObject).toHaveBeenCalled(); const ausgeliefert = JSON.stringify(res.json.mock.calls[0][0]); - expect(ausgeliefert).not.toContain('geheim123'); + expect(ausgeliefert).not.toContain('<>'); expect(ausgeliefert).toContain('***'); }); it('should return 404 if run not found', async () => { diff --git a/apps/api/src/scrape/services/scrape-execution.service.spec.ts b/apps/api/src/scrape/services/scrape-execution.service.spec.ts index feb2643..62bfb72 100644 --- a/apps/api/src/scrape/services/scrape-execution.service.spec.ts +++ b/apps/api/src/scrape/services/scrape-execution.service.spec.ts @@ -583,11 +583,16 @@ describe('ScrapeExecutionService', () => { // Nur beim Ausliefern zu schwaerzen liesse ihn in der Datenbank, in // jedem Backup und in jedem Dump stehen. mockSecretRedaction.redactObject.mockImplementation((o: any) => - JSON.parse(JSON.stringify(o).replaceAll('geheim123', '***')), + JSON.parse( + JSON.stringify(o).replaceAll('<>', '***'), + ), ); const previousData = new Map(); - previousData.set('var_password', 'geheim123'); + // Bewusst ein offensichtlicher Platzhalter: ein realistisch aussehender + // Wert neben dem Feldnamen var_password laesst Secret-Scanner anschlagen + // (GitGuardian hat genau das an PR #153 gemeldet). + previousData.set('var_password', '<>'); mockActionHandlerService.handleAction.mockResolvedValue(undefined); await service.executeScrape(createScrape(), 'run-1', previousData, {}); @@ -596,7 +601,7 @@ describe('ScrapeExecutionService', () => { const gespeichert = mockDatabaseService.storeData.mock.calls.find( (c: any[]) => c[1] === '__debugData', ); - expect(gespeichert[2]).not.toContain('geheim123'); + expect(gespeichert[2]).not.toContain('<>'); expect(gespeichert[2]).toContain('***'); });