diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 36414f9b9..28066425c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -72,12 +72,12 @@ jobs: env: RUN_MONGO_TESTS: 'true' GIT_PROXY_MONGO_CONNECTION_STRING: mongodb://localhost:27017/git-proxy-test - run: npm run test:integration + run: npm run test:integration -- --coverage --coverage.reporter=lcovonly --coverage.reportsDirectory=coverage/integration - name: Upload test coverage report uses: codecov/codecov-action@303a32d7a59b442fa8d48b6a1cc6825c09c847a5 # v7.1.1 with: - files: ./coverage/lcov.info,./coverage-cli/lcov.info + files: ./coverage/lcov.info,./coverage/integration/lcov.info,./coverage-cli/lcov.info token: ${{ secrets.CODECOV_TOKEN }} - name: Build frontend @@ -140,7 +140,13 @@ jobs: env: RUN_POSTGRES_TESTS: 'true' GIT_PROXY_POSTGRES_CONNECTION_STRING: postgresql://postgres:postgres@localhost:5432/git_proxy_test - run: npm run test:integration:postgres + run: npm run test:integration:postgres -- --coverage --coverage.reporter=lcovonly --coverage.reportsDirectory=coverage/postgres + + - name: Upload PostgreSQL coverage report + uses: codecov/codecov-action@303a32d7a59b442fa8d48b6a1cc6825c09c847a5 # v7.1.1 + with: + files: ./coverage/postgres/lcov.info + token: ${{ secrets.CODECOV_TOKEN }} # Windows build - single combination for development support build-windows: diff --git a/src/db/file/pushes.ts b/src/db/file/pushes.ts index 4b39a5f06..e2b4217ce 100644 --- a/src/db/file/pushes.ts +++ b/src/db/file/pushes.ts @@ -20,6 +20,8 @@ import { activityPrimaryStatusFromFlags } from '../../activity/activityPrimarySt import { canonicalRemoteUrl } from '../../activity/canonicalRemoteUrl'; import { Action } from '../../proxy/actions/Action'; import { toClass } from '../helper'; +import { pushListProjection } from '../pushProjection'; +import { compactPush, restorePush } from '../pushStorage'; import { PushQuery, RepoActivityTabCounts, @@ -150,12 +152,6 @@ const defaultPushQuery: Partial = { type: 'push', }; -// List queries drop `steps` and `diff` from the returned document: both hold the -// full diff (largest part of a push row), and the mongo and postgres list -// projections exclude them as well. The push-detail path (`getPush`) still returns -// the whole document. -const pushListProjection = { steps: 0, diff: 0 } as const; - export const getPushes = (query: Partial): Promise => { if (!query) query = defaultPushQuery; return new Promise((resolve, reject) => { @@ -213,7 +209,7 @@ export const getPush = async (id: string): Promise => { if (!doc) { resolve(null); } else { - resolve(toClass(doc, Action.prototype)); + resolve(restorePush(doc)); } } }); @@ -237,7 +233,7 @@ export const deletePush = async (id: string): Promise => { export const writeAudit = async (action: Action): Promise => { return new Promise((resolve, reject) => { const options = { multi: false, upsert: true }; - db.update({ id: action.id }, action, options, (err) => { + db.update({ id: action.id }, compactPush(action), options, (err) => { // ignore for code coverage as neDB rarely returns errors even for an invalid query /* istanbul ignore if */ if (err) { diff --git a/src/db/mongo/pushes.ts b/src/db/mongo/pushes.ts index a0ab1f067..3dd0d193a 100644 --- a/src/db/mongo/pushes.ts +++ b/src/db/mongo/pushes.ts @@ -18,7 +18,8 @@ import { activityPrimaryStatusFromFlags } from '../../activity/activityPrimarySt import { canonicalRemoteUrl } from '../../activity/canonicalRemoteUrl'; import { connect, findDocuments, findOneDocument } from './helper'; import { Action } from '../../proxy/actions'; -import { toClass } from '../helper'; +import { pushListProjection } from '../pushProjection'; +import { compactPush, restorePush } from '../pushStorage'; import { PushQuery, RepoActivityTabCounts, @@ -120,33 +121,6 @@ const defaultPushQuery: Partial = { type: 'push', }; -/** Fields returned for push list / activity UIs (shared by getPushes and getPushesForUserProfile). */ -const pushListProjection = { - _id: 0, - id: 1, - allowPush: 1, - attestation: 1, - authorised: 1, - blocked: 1, - blockedMessage: 1, - branch: 1, - canceled: 1, - commitData: 1, - commitFrom: 1, - commitTo: 1, - error: 1, - method: 1, - project: 1, - rejected: 1, - rejection: 1, - repo: 1, - repoName: 1, - timestamp: 1, - type: 1, - url: 1, - userEmail: 1, -} as const; - export const getPushes = async ( query: Partial = defaultPushQuery, ): Promise => { @@ -169,7 +143,7 @@ export const getPushesForUserProfile = async ( export const getPush = async (id: string): Promise => { const doc = await findOneDocument(collectionName, { id }); - return doc ? (toClass(doc, Action.prototype) as Action) : null; + return doc ? restorePush(doc) : null; }; export const deletePush = async function (id: string): Promise { @@ -178,14 +152,16 @@ export const deletePush = async function (id: string): Promise { }; export const writeAudit = async (action: Action): Promise => { - const data = JSON.parse(JSON.stringify(action)); + const data = JSON.parse(JSON.stringify(compactPush(action))); const options = { upsert: true }; const collection = await connect(collectionName); delete data._id; if (typeof data.id !== 'string') { throw new Error('Invalid id'); } - await collection.updateOne({ id: data.id }, { $set: data }, options); + const unset: Record = + data._lastStepIndex === undefined ? { _lastStepIndex: '' } : { lastStep: '' }; + await collection.updateOne({ id: data.id }, { $set: data, $unset: unset }, options); }; export const authorise = async ( diff --git a/src/db/postgres/pushes.ts b/src/db/postgres/pushes.ts index 07693386b..029455083 100644 --- a/src/db/postgres/pushes.ts +++ b/src/db/postgres/pushes.ts @@ -18,7 +18,8 @@ import { activityPrimaryStatusFromFlags } from '../../activity/activityPrimarySt import { canonicalRemoteUrl } from '../../activity/canonicalRemoteUrl'; import { Action } from '../../proxy/actions'; import { CompletedAttestation, Rejection } from '../../proxy/processors/types'; -import { toClass } from '../helper'; +import { pushListProjection } from '../pushProjection'; +import { compactPush, restorePush } from '../pushStorage'; import { emptyRepoActivityTabCounts, PushQuery, @@ -47,8 +48,14 @@ const FILTER_COLUMNS: Record = { type: 'type', }; -const rowToAction = (row: { data: unknown }): Action => - toClass(row.data, Action.prototype) as Action; +const rowToAction = (row: { data: unknown }): Action => restorePush(row.data); + +const listFields = Object.entries(pushListProjection) + .filter(([, included]) => included) + .map(([field]) => `'${field}'`) + .join(', '); +const listData = `(SELECT COALESCE(jsonb_object_agg(field, data->field), '{}'::jsonb) + FROM unnest(ARRAY[${listFields}]) AS field WHERE data ? field)`; function bumpCount( m: Map, @@ -163,16 +170,12 @@ export const getPushesForUserProfile = async ( } const result = await query<{ data: unknown }>( - `SELECT data - 'steps' - 'diff' AS data FROM pushes WHERE type = 'push' AND ${predicate} ORDER BY timestamp DESC`, + `SELECT ${listData} AS data FROM pushes WHERE type = 'push' AND ${predicate} ORDER BY timestamp DESC`, values, ); return result.rows.map(rowToAction); }; -// List queries drop `steps` and `diff` from the returned document: both hold the -// full diff (largest part of a push row) and the mongo backend's list projection -// excludes them as well. The push-detail path (`getPush`) still returns the whole -// document. export const getPushes = async (q: Partial = defaultPushQuery): Promise => { const clauses: string[] = []; const values: unknown[] = []; @@ -185,7 +188,7 @@ export const getPushes = async (q: Partial = defaultPushQuery): Promi const where = clauses.length > 0 ? `WHERE ${clauses.join(' AND ')}` : ''; const result = await query<{ data: unknown }>( - `SELECT data - 'steps' - 'diff' AS data FROM pushes ${where} ORDER BY timestamp DESC`, + `SELECT ${listData} AS data FROM pushes ${where} ORDER BY timestamp DESC`, values, ); return result.rows.map(rowToAction); @@ -208,7 +211,7 @@ const buildAuditUpsert = (action: Action): { text: string; values: unknown[] } = // Round-trip through JSON to drop class identity / mongo-specific _id fields // before persisting (mirrors mongo's `JSON.parse(JSON.stringify(action))`). - const data = JSON.parse(JSON.stringify(action)); + const data = JSON.parse(JSON.stringify(compactPush(action))); delete data._id; return { diff --git a/src/db/pushProjection.ts b/src/db/pushProjection.ts new file mode 100644 index 000000000..976e2dd9a --- /dev/null +++ b/src/db/pushProjection.ts @@ -0,0 +1,56 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import type { Action } from '../proxy/actions'; + +/** + * List and profile responses contain metadata only. Keep every diff representation + * (steps, lastStep, and the top-level diff) out of this allowlist. + * Detail reads and audit writes retain the complete record. + */ +export const pushListProjection = { + _id: 0, + actionType: 1, + allowPush: 1, + attestation: 1, + authorised: 1, + autoApproved: 1, + autoRejected: 1, + blocked: 1, + blockedMessage: 1, + branch: 1, + canceled: 1, + commitData: 1, + commitFrom: 1, + commitTo: 1, + error: 1, + id: 1, + legacyId: 1, + method: 1, + project: 1, + pusherVerified: 1, + rejected: 1, + rejection: 1, + repo: 1, + repoName: 1, + tagData: 1, + tags: 1, + timestamp: 1, + type: 1, + url: 1, + user: 1, + userEmail: 1, +} as const satisfies Partial>; diff --git a/src/db/pushStorage.ts b/src/db/pushStorage.ts new file mode 100644 index 000000000..16dfc1b89 --- /dev/null +++ b/src/db/pushStorage.ts @@ -0,0 +1,60 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { isEqual } from 'lodash'; +import { Action } from '../proxy/actions'; +import { toClass } from './helper'; + +// Internal storage markers that may exist on persisted records but must never +// reach callers of the database layer. +type StoredPush = Action & { _lastStepIndex?: number; _activity?: unknown }; + +/** + * Prepares a push for storage. Works on a shallow copy, so the caller's Action + * is not modified. Any internal markers already on the input are dropped + * first. When `lastStep` is identical to the final entry in `steps`, the + * duplicate copy is replaced with `_lastStepIndex`, a reference to that entry. + * A `lastStep` that differs from the final step is stored as-is. + */ +export const compactPush = (action: Action) => { + const stored: Partial = { ...action }; + delete stored._lastStepIndex; + delete stored._activity; + const index = action.steps?.length - 1; + if (action.lastStep && index >= 0 && isEqual(action.lastStep, action.steps[index])) { + stored._lastStepIndex = index; + delete stored.lastStep; + } + return stored; +}; + +/** + * Reverses compactPush when reading a stored record back into an Action. If + * the record holds a valid `_lastStepIndex` and no `lastStep`, `lastStep` is + * rebuilt from that entry in `steps`. Records that store `lastStep` directly + * (a distinct last step, or historical records) keep it as stored. Internal + * markers are always removed from the returned Action. + */ +export const restorePush = (doc: unknown): Action => { + const action: StoredPush = toClass(doc, Action.prototype); + const index = action._lastStepIndex; + if (index !== undefined && Number.isInteger(index) && index >= 0 && !action.lastStep) { + action.lastStep = action.steps?.[index]; + } + delete action._lastStepIndex; + delete action._activity; + return action; +}; diff --git a/src/proxy/processors/push-action/getDiff.ts b/src/proxy/processors/push-action/getDiff.ts index 39b40d4b7..763390744 100644 --- a/src/proxy/processors/push-action/getDiff.ts +++ b/src/proxy/processors/push-action/getDiff.ts @@ -50,7 +50,7 @@ const exec = async (_req: Request, action: Action): Promise => { const revisionRange = `${commitFrom}..${action.commitTo}`; const diff = await git.diff([revisionRange]); action.setDiff(diff); - step.log(diff); + step.log(`Generated diff (${Buffer.byteLength(diff, 'utf8')} bytes)`); } catch (error: unknown) { const msg = getErrorMessage(error); step.setError(msg); diff --git a/src/proxy/processors/push-action/scanDiff.ts b/src/proxy/processors/push-action/scanDiff.ts index 9b9b0492f..0cf2e3053 100644 --- a/src/proxy/processors/push-action/scanDiff.ts +++ b/src/proxy/processors/push-action/scanDiff.ts @@ -181,7 +181,6 @@ const exec = async (_req: Request, action: Action): Promise => { step.log(`Scanning diff: ${commitFrom}:${commitTo}`); const diff = action.diff ?? steps.find((s) => s.stepName === 'diff')?.content; - step.log(diff); const diffViolations = getDiffViolations(diff, action.project, step); if (diffViolations) { diff --git a/test/db/file/pushStorage.test.ts b/test/db/file/pushStorage.test.ts new file mode 100644 index 000000000..2716e9b2c --- /dev/null +++ b/test/db/file/pushStorage.test.ts @@ -0,0 +1,20 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import * as pushes from '../../../src/db/file/pushes'; +import { definePushStorageContract } from '../pushStorage.contract'; + +definePushStorageContract('NeDB push storage', pushes, (id) => pushes.db.findOneAsync({ id })); diff --git a/test/db/file/pushes.test.ts b/test/db/file/pushes.test.ts index 583eb9181..30d951782 100644 --- a/test/db/file/pushes.test.ts +++ b/test/db/file/pushes.test.ts @@ -16,6 +16,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import * as pushesModule from '../../../src/db/file/pushes'; +import { pushListProjection } from '../../../src/db/pushProjection'; describe('File DB - Pushes', () => { beforeEach(() => { @@ -116,8 +117,8 @@ describe('File DB - Pushes', () => { // row, so neither may reach a list response. The detail view still returns // the whole document. expect(projection).toHaveBeenCalledTimes(2); - expect(projection).toHaveBeenNthCalledWith(1, { steps: 0, diff: 0 }); - expect(projection).toHaveBeenNthCalledWith(2, { steps: 0, diff: 0 }); + expect(projection).toHaveBeenNthCalledWith(1, pushListProjection); + expect(projection).toHaveBeenNthCalledWith(2, pushListProjection); expect(pushesModule.db.findOne).toHaveBeenCalledWith({ id: 'p1' }, expect.any(Function)); }); }); diff --git a/test/db/mongo/push.test.ts b/test/db/mongo/push.test.ts index 97f2f183e..231db5405 100644 --- a/test/db/mongo/push.test.ts +++ b/test/db/mongo/push.test.ts @@ -16,6 +16,7 @@ import { describe, it, expect, afterEach, vi, beforeEach } from 'vitest'; import { Action } from '../../../src/proxy/actions'; +import { pushListProjection } from '../../../src/db/pushProjection'; const mockFindOne = vi.fn(); const mockDeleteOne = vi.fn(); @@ -103,31 +104,7 @@ describe('MongoDB Push Handler', async () => { type: 'push', }, { - projection: { - _id: 0, - id: 1, - allowPush: 1, - attestation: 1, - authorised: 1, - blocked: 1, - blockedMessage: 1, - branch: 1, - canceled: 1, - commitData: 1, - commitFrom: 1, - commitTo: 1, - error: 1, - method: 1, - project: 1, - rejected: 1, - rejection: 1, - repo: 1, - repoName: 1, - timestamp: 1, - type: 1, - url: 1, - userEmail: 1, - }, + projection: pushListProjection, sort: { timestamp: -1, }, @@ -234,6 +211,7 @@ describe('MongoDB Push Handler', async () => { allowPush: TEST_PUSH.allowPush, authorised: TEST_PUSH.authorised, }), + $unset: { _lastStepIndex: '' }, }, { upsert: true }, ); @@ -269,6 +247,7 @@ describe('MongoDB Push Handler', async () => { rejected: false, attestation: attestation, }), + $unset: { _lastStepIndex: '' }, }, { upsert: true }, ); @@ -304,6 +283,7 @@ describe('MongoDB Push Handler', async () => { rejected: true, rejection: rejection, }), + $unset: { _lastStepIndex: '' }, }, { upsert: true }, ); @@ -337,6 +317,7 @@ describe('MongoDB Push Handler', async () => { canceled: true, rejected: false, }), + $unset: { _lastStepIndex: '' }, }, { upsert: true }, ); diff --git a/test/db/mongo/pushStorage.integration.test.ts b/test/db/mongo/pushStorage.integration.test.ts new file mode 100644 index 000000000..cb03165bb --- /dev/null +++ b/test/db/mongo/pushStorage.integration.test.ts @@ -0,0 +1,26 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { describe } from 'vitest'; +import * as pushes from '../../../src/db/mongo/pushes'; +import { definePushStorageContract } from '../pushStorage.contract'; +import { connect } from '../../../src/db/mongo/helper'; + +describe.runIf(process.env.RUN_MONGO_TESTS === 'true')('MongoDB storage contract', () => { + definePushStorageContract('MongoDB push storage', pushes, async (id) => + (await connect('pushes')).findOne({ id }), + ); +}); diff --git a/test/db/postgres/pushStorage.integration.test.ts b/test/db/postgres/pushStorage.integration.test.ts new file mode 100644 index 000000000..3f9305f73 --- /dev/null +++ b/test/db/postgres/pushStorage.integration.test.ts @@ -0,0 +1,57 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { describe, expect, it } from 'vitest'; +import * as pushes from '../../../src/db/postgres/pushes'; +import { definePushStorageContract } from '../pushStorage.contract'; +import { query } from '../../../src/db/postgres/helper'; +import { Action, RequestType, Step } from '../../../src/proxy/actions'; + +describe.runIf(process.env.RUN_POSTGRES_TESTS === 'true')('PostgreSQL storage contract', () => { + definePushStorageContract('PostgreSQL push storage', pushes, async (id) => { + const result = await query<{ data: unknown }>('SELECT data FROM pushes WHERE id = $1', [id]); + return result.rows[0]?.data; + }); + + it('reads historical final steps without rewriting the stored audit', async () => { + const action = new Action('legacy', RequestType.PUSH, 'POST', 123, 'https://example.com/a/b'); + action.addStep(new Step('diff', false, null, false, null, 'legacy diff')); + const data = JSON.parse(JSON.stringify(action)); + await query('INSERT INTO pushes (id, timestamp, type, data) VALUES ($1, $2, $3, $4)', [ + action.id, + action.timestamp, + action.type, + data, + ]); + expect(await pushes.getPush(action.id)).toMatchObject(data); + expect( + (await query('SELECT data FROM pushes WHERE id = $1', [action.id])).rows[0].data, + ).toEqual(data); + const [summary] = await pushes.getPushes({}); + expect(summary).not.toHaveProperty('lastStep'); + expect(summary).not.toHaveProperty('steps'); + }); + + it('preserves explicit null metadata while omitting absent fields', async () => { + const action = new Action('metadata', RequestType.PUSH, 'POST', 123, 'https://example.com/a/b'); + Object.assign(action, { blockedMessage: null }); + delete action.rejection; + await pushes.writeAudit(action); + const [summary] = await pushes.getPushes({}); + expect(summary).toHaveProperty('blockedMessage', null); + expect(summary).not.toHaveProperty('rejection'); + }); +}); diff --git a/test/db/postgres/pushes.test.ts b/test/db/postgres/pushes.test.ts index e00731d5f..1780557b5 100644 --- a/test/db/postgres/pushes.test.ts +++ b/test/db/postgres/pushes.test.ts @@ -15,6 +15,7 @@ */ import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { Action, RequestType, Step } from '../../../src/proxy/actions'; const mockQuery = vi.fn(); @@ -70,7 +71,7 @@ describe('PostgreSQL - Pushes', async () => { await getPushes({ id: 'x' } as never); const [sql, params] = mockQuery.mock.calls[0]; - expect(sql).not.toContain('WHERE'); + expect(sql).toMatch(/FROM pushes\s+ORDER BY timestamp DESC$/); expect(params).toEqual([]); }); }); @@ -83,6 +84,27 @@ describe('PostgreSQL - Pushes', async () => { }); describe('writeAudit', () => { + it('compacts the final step and restores it on detail reads', async () => { + const action = new Action( + 'compact', + RequestType.PUSH, + 'POST', + 123, + 'https://example.com/a/b', + ); + action.addStep(new Step('diff', false, null, false, null, 'reviewable diff')); + mockQuery.mockResolvedValue({ rowCount: 1, rows: [] }); + await writeAudit(action); + const stored = JSON.parse(mockQuery.mock.calls[0][1][9]); + expect(stored).not.toHaveProperty('lastStep'); + expect(stored._lastStepIndex).toBe(0); + mockQuery.mockResolvedValue({ rowCount: 1, rows: [{ data: stored }] }); + const detail = await getPush(action.id); + expect(detail?.lastStep).toEqual(action.lastStep); + expect(detail).not.toHaveProperty('_lastStepIndex'); + expect(action.lastStep?.content).toBe('reviewable diff'); + }); + it('throws Invalid id when id is not a string', async () => { const action = { id: 42, timestamp: 1 } as unknown as Parameters[0]; await expect(writeAudit(action)).rejects.toThrow('Invalid id'); @@ -222,7 +244,7 @@ describe('PostgreSQL - Pushes', async () => { }); describe('list projection', () => { - it('drops steps and diff from list results but not from the detail view', async () => { + it('selects metadata fields in SQL while detail reads retain the entire document', async () => { mockQuery.mockResolvedValue({ rowCount: 0, rows: [] }); await getPushes({}); @@ -232,11 +254,15 @@ describe('PostgreSQL - Pushes', async () => { const [listSql] = mockQuery.mock.calls[0]; const [profileSql] = mockQuery.mock.calls[1]; const [detailSql] = mockQuery.mock.calls[2]; - // Both hold the full diff, so neither may reach a list response. - expect(listSql).toContain("data - 'steps' - 'diff'"); - expect(profileSql).toContain("data - 'steps' - 'diff'"); - expect(detailSql).not.toContain("data - 'steps'"); - expect(detailSql).not.toContain("- 'diff'"); + for (const sql of [listSql, profileSql]) { + expect(sql).toContain('jsonb_object_agg'); + expect(sql).toContain("'tagData'"); + expect(sql).toContain("'rejection'"); + expect(sql).not.toContain("'steps'"); + expect(sql).not.toContain("'lastStep'"); + expect(sql).not.toContain("'diff'"); + } + expect(detailSql).toBe('SELECT data FROM pushes WHERE id = $1'); }); }); @@ -249,7 +275,7 @@ describe('PostgreSQL - Pushes', async () => { const [sql, params] = mockQuery.mock.calls[0]; expect(sql).toContain("data->'attestation'->'reviewer'->>'username'"); expect(sql).toMatch(/ORDER BY timestamp DESC/); - expect(sql).not.toContain('userEmail'); + expect(sql).not.toContain("(data->>'userEmail') = ANY"); expect(params).toEqual(['Alice']); }); diff --git a/test/db/pushStorage.contract.ts b/test/db/pushStorage.contract.ts new file mode 100644 index 000000000..ef0f6837b --- /dev/null +++ b/test/db/pushStorage.contract.ts @@ -0,0 +1,325 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { Action, PushType, RequestType, Step } from '../../src/proxy/actions'; +import { canonicalRemoteUrl } from '../../src/activity/canonicalRemoteUrl'; +import type { Sink } from '../../src/db/types'; + +type PushStorage = Pick< + Sink, + | 'writeAudit' + | 'getPush' + | 'getPushes' + | 'getPushesForUserProfile' + | 'deletePush' + | 'authorise' + | 'reject' + | 'cancel' + | 'getRepoPushRollupsByCanonicalUrl' +>; + +const ids = ['storage-contract-1', 'storage-contract-2', 'storage-contract-3']; +const url = 'https://example.com/storage-contract/repo.git'; +const diff = 'diff --git a/private.txt b/private.txt\n+private payload\n'; +const attestation = { + answers: [{ label: 'Reviewed', checked: true }], + timestamp: new Date('2026-01-01T00:00:00Z'), + reviewer: { username: 'Reviewer', email: 'reviewer@example.com' }, +}; +const rejection = { + reason: 'Needs changes', + timestamp: new Date('2026-01-02T00:00:00Z'), + reviewer: attestation.reviewer, +}; + +const makePush = (id = ids[0], content = diff): Action => { + const action = new Action(id, RequestType.PUSH, 'POST', 100, url); + action.blocked = true; + action.user = 'Contributor'; + action.userEmail = 'author@example.com'; + action.branch = 'refs/heads/main'; + action.commitFrom = 'a'.repeat(40); + action.commitTo = 'b'.repeat(40); + const step = new Step('diff', false, null, false, null, content); + // Historical pushes include the raw diff in logs and sometimes in lastStep too. + step.logs = [`diff - ${content}`]; + action.addStep(step); + return action; +}; + +/** Exercise real sinks with one shared contract; mocks cannot validate database projections. */ +export const definePushStorageContract = ( + name: string, + sink: PushStorage, + readStored: (id: string) => Promise, +): void => { + describe(name, () => { + const cleanup = async () => { + for (const id of ids) await sink.deletePush(id); + }; + beforeEach(cleanup); + afterEach(cleanup); + + it.each(['step', 'action'])( + 'stores a diff on the %s once without changing the detail response', + async (location) => { + const action = makePush(); + action.steps[0].logs = ['diff - Generated diff']; + if (location === 'action') { + action.setDiff(diff); + action.steps[0].content = null; + } + await sink.writeAudit(action); + const stored = await readStored(action.id); + expect(stored).not.toHaveProperty('lastStep'); + expect(JSON.stringify(stored).match(/private payload/g)).toHaveLength(1); + const detail = await sink.getPush(action.id); + expect(detail?.diff).toBe(action.diff); + expect(detail?.steps).toEqual(action.steps); + expect(detail?.lastStep).toEqual(action.lastStep); + expect(detail).not.toHaveProperty('_lastStepIndex'); + + action.lastStep = new Step('failedAfterDiff'); + action.lastStep.setError('Preserve a separate diagnostic'); + await sink.writeAudit(action); + expect(await readStored(action.id)).not.toHaveProperty('_lastStepIndex'); + expect((await sink.getPush(action.id))?.lastStep).toEqual(action.lastStep); + }, + ); + + it('keeps legacy audit content on detail reads while excluding every diff copy from lists', async () => { + const action = makePush(); + // Top-level diff records must also remain outside list responses. + const detailOnly = { + author: 'audit author', + message: diff, + errorMessage: diff, + protocol: 'http', + }; + Object.assign(action, { diff, pluginPrivateData: diff, ...detailOnly }); + await sink.writeAudit(action); + + const summaries = await sink.getPushes({ type: 'push' }); + const summary = summaries.find((push) => push.id === action.id)!; + expect(summary).toMatchObject({ + id: action.id, + url, + user: 'Contributor', + userEmail: 'author@example.com', + commitFrom: action.commitFrom, + commitTo: action.commitTo, + blocked: true, + }); + for (const field of [ + 'steps', + 'lastStep', + 'diff', + '_id', + 'pluginPrivateData', + ...Object.keys(detailOnly), + ]) { + expect(summary).not.toHaveProperty(field); + } + const profile = await sink.getPushesForUserProfile(['author@example.com'], 'reviewer'); + for (const field of Object.keys(detailOnly)) { + expect(profile.find((push) => push.id === action.id)).not.toHaveProperty(field); + } + expect(JSON.stringify(summary)).not.toContain('private payload'); + + const detail = await sink.getPush(action.id); + expect(detail).toMatchObject(detailOnly); + expect(detail?.steps[0].content).toBe(diff); + expect(detail?.steps[0].logs).toEqual([`diff - ${diff}`]); + expect(detail?.lastStep?.content).toBe(diff); + }); + + it('returns equally small summaries for small and multi-megabyte diffs', async () => { + await sink.writeAudit(makePush(ids[0], 'small diff')); + await sink.writeAudit(makePush(ids[1], 'private payload\n'.repeat(131072))); + + const summaries = (await sink.getPushes({ type: 'push' })).filter((push) => + ids.includes(push.id), + ); + expect(summaries).toHaveLength(2); + const sizes = summaries.map((push) => Buffer.byteLength(JSON.stringify(push))); + expect(sizes[0]).toBe(sizes[1]); + expect(sizes[0]).toBeLessThan(2000); + expect((await sink.getPush(ids[1]))?.steps[0].content.length).toBeGreaterThan(2_000_000); + }); + + it('preserves tag metadata used by the activity table', async () => { + const action = makePush(); + action.actionType = PushType.TAG; + action.tags = ['refs/tags/v1']; + action.tagData = [ + { + type: 'tag', + tagName: 'v1', + tagger: 'Tagger', + taggerEmail: 'tagger@example.com', + message: 'Release', + }, + ]; + action.steps = []; + delete action.lastStep; + await sink.writeAudit(action); + + expect( + (await sink.getPushes({ type: 'push' })).find((push) => push.id === action.id), + ).toMatchObject({ actionType: PushType.TAG, tags: action.tags, tagData: action.tagData }); + expect((await sink.getPush(action.id))?.steps).toEqual([]); + }); + + it('preserves filtering and newest-first ordering', async () => { + for (let i = 0; i < ids.length; i++) { + const action = makePush(ids[i]); + action.timestamp = 100 + i; + action.authorised = i === 1; + await sink.writeAudit(action); + } + expect( + (await sink.getPushes({ authorised: false })) + .filter((push) => ids.includes(push.id)) + .map((push) => push.id), + ).toEqual([ids[2], ids[0]]); + expect( + (await sink.getPushes({ authorised: true })) + .filter((push) => ids.includes(push.id)) + .map((push) => push.id), + ).toEqual([ids[1]]); + }); + + it('projects profile activity for both authors and reviewers', async () => { + const authored = makePush(ids[0]); + const reviewed = makePush(ids[1]); + reviewed.userEmail = 'someone-else@example.com'; + reviewed.attestation = attestation; + reviewed.timestamp = 200; + const unrelated = makePush(ids[2]); + unrelated.userEmail = 'unrelated@example.com'; + for (const action of [authored, reviewed, unrelated]) await sink.writeAudit(action); + + const summaries = await sink.getPushesForUserProfile(['author@example.com'], 'reviewer'); + expect(summaries.filter((push) => ids.includes(push.id)).map((push) => push.id)).toEqual([ + ids[1], + ids[0], + ]); + expect(JSON.stringify(summaries)).not.toContain('private payload'); + expect(summaries.find((push) => push.id === reviewed.id)?.attestation?.reviewer).toEqual( + attestation.reviewer, + ); + const reviewOnly = await sink.getPushesForUserProfile([], 'REVIEWER'); + expect(reviewOnly.filter((push) => ids.includes(push.id)).map((push) => push.id)).toEqual([ + ids[1], + ]); + }); + + it('preserves full legacy records through review decisions and cancellation', async () => { + await sink.writeAudit(makePush()); + await sink.authorise(ids[0], attestation); + expect(await sink.getPush(ids[0])).toMatchObject({ + authorised: true, + canceled: false, + rejected: false, + }); + await sink.reject(ids[0], rejection); + expect(await sink.getPush(ids[0])).toMatchObject({ + authorised: false, + canceled: false, + rejected: true, + }); + await sink.cancel(ids[0]); + + const detail = await sink.getPush(ids[0]); + expect(detail).toMatchObject({ authorised: false, canceled: true, rejected: false }); + expect(detail?.steps[0].content).toBe(diff); + expect(detail?.steps[0].logs).toEqual([`diff - ${diff}`]); + expect(detail?.attestation?.reviewer).toEqual(attestation.reviewer); + expect(detail?.rejection?.reason).toBe(rejection.reason); + }); + + it('keeps activity counts and timestamps correct after updates and deletion', async () => { + const action = makePush(); + await sink.writeAudit(action); + const key = canonicalRemoteUrl(url); + const pending = await sink.getRepoPushRollupsByCanonicalUrl(); + expect(pending.tabCounts.get(key)?.pending).toBe(1); + expect(pending.latestPendingReviewAtMs.get(key)).toBe(action.timestamp); + + await sink.authorise(action.id, attestation); + const approved = await sink.getRepoPushRollupsByCanonicalUrl(); + expect(approved.tabCounts.get(key)?.pending).toBe(0); + expect(approved.tabCounts.get(key)?.approved).toBe(1); + expect(approved.latestPendingReviewAtMs.has(key)).toBe(false); + expect(approved.latestPushAtMs.get(key)).toBe(action.timestamp); + + await sink.deletePush(action.id); + expect((await sink.getRepoPushRollupsByCanonicalUrl()).tabCounts.has(key)).toBe(false); + expect(await sink.getPush(action.id)).toBeNull(); + }); + + it('handles retries, concurrent writes, and removal of the latest pending push', async () => { + const key = canonicalRemoteUrl(url); + const actions = ids.map((id, index) => { + const action = makePush(id); + action.timestamp = 100 + index; + return action; + }); + await Promise.all(actions.map((action) => sink.writeAudit(action))); + await Promise.all(actions.map((action) => sink.writeAudit(action))); + expect((await sink.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.pending).toBe(3); + await Promise.all([sink.authorise(ids[2], attestation), sink.reject(ids[0], rejection)]); + const reviewed = await sink.getRepoPushRollupsByCanonicalUrl(); + expect(reviewed.tabCounts.get(key)).toEqual({ + pending: 1, + approved: 1, + rejected: 1, + canceled: 0, + error: 0, + }); + expect(reviewed.latestPushAtMs.get(key)).toBe(102); + expect(reviewed.latestPendingReviewAtMs.get(key)).toBe(101); + await sink.deletePush(ids[2]); + await sink.deletePush(ids[2]); + expect((await sink.getRepoPushRollupsByCanonicalUrl()).latestPushAtMs.get(key)).toBe(101); + }); + + it('moves activity when a stored push changes its canonical repository or request type', async () => { + const action = makePush(); + await sink.writeAudit(action); + await sink.getRepoPushRollupsByCanonicalUrl(); + action.url = 'git@EXAMPLE.com:storage-contract/REPO.git'; + await sink.writeAudit(action); + expect( + (await sink.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(canonicalRemoteUrl(url)) + ?.pending, + ).toBe(1); + action.url = 'https://example.com/storage-contract/moved.git'; + await sink.writeAudit(action); + const moved = await sink.getRepoPushRollupsByCanonicalUrl(); + expect(moved.tabCounts.has(canonicalRemoteUrl(url))).toBe(false); + expect(moved.tabCounts.get(canonicalRemoteUrl(action.url))?.pending).toBe(1); + action.type = RequestType.PULL; + await sink.writeAudit(action); + expect( + (await sink.getRepoPushRollupsByCanonicalUrl()).tabCounts.has( + canonicalRemoteUrl(action.url), + ), + ).toBe(false); + }); + }); +}; diff --git a/test/db/pushStorage.test.ts b/test/db/pushStorage.test.ts new file mode 100644 index 000000000..93692ca75 --- /dev/null +++ b/test/db/pushStorage.test.ts @@ -0,0 +1,77 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { describe, expect, it } from 'vitest'; +import { Action, RequestType, Step } from '../../src/proxy/actions'; +import { compactPush, restorePush } from '../../src/db/pushStorage'; + +const makePush = () => + new Action('compact', RequestType.PUSH, 'POST', 1, 'https://example.com/a/b'); + +describe('push storage representation', () => { + it('stores a repeated last step once and restores the detail API shape', () => { + const action = makePush(); + const step = new Step('diff'); + step.content = 'unique diff payload'; + step.logs = ['Generated diff']; + action.addStep(step); + + const stored = compactPush(action); + expect(JSON.stringify(stored).match(/unique diff payload/g)).toHaveLength(1); + expect(stored).not.toHaveProperty('lastStep'); + expect(action.lastStep).toBe(step); + expect(action).not.toHaveProperty('_lastStepIndex'); + + const restored = restorePush(Object.assign(makePush(), JSON.parse(JSON.stringify(stored)))); + expect(restored).toEqual(action); + expect(restored.getLastStep()).toBe(restored.steps[0]); + expect(restored).not.toHaveProperty('_lastStepIndex'); + expect(compactPush(restored)).toEqual(stored); + }); + + it('compacts structurally equal steps read back from a legacy document', () => { + const action = makePush(); + action.addStep(new Step('diff')); + const legacy = JSON.parse(JSON.stringify(action)); + expect(compactPush(legacy)).not.toHaveProperty('lastStep'); + }); + + it('preserves a last step containing information absent from the steps array', () => { + const action = makePush(); + action.addStep(new Step('diff')); + action.lastStep = Object.assign(new Step('diff'), action.steps[0], { + logs: ['additional diagnostic'], + }); + expect(compactPush(action).lastStep).toEqual(action.lastStep); + expect(restorePush(action)).toEqual(action); + }); + + it('preserves a legacy missing lastStep and empty steps', () => { + const action = makePush(); + expect(restorePush(action)).not.toHaveProperty('lastStep'); + action.addStep(new Step('diff')); + delete action.lastStep; + expect(compactPush(action)).not.toHaveProperty('_lastStepIndex'); + expect(restorePush(action)).not.toHaveProperty('lastStep'); + }); + + it.each([-1, 0.5, 999])('does not fabricate a step for an invalid index %s', (index) => { + const action = makePush(); + const restored = restorePush(Object.assign(action, { _lastStepIndex: index })); + expect(restored.lastStep).toBeUndefined(); + expect(restored).not.toHaveProperty('_lastStepIndex'); + }); +}); diff --git a/test/e2e/push.test.ts b/test/e2e/push.test.ts index 4da4ed974..ed1255ba0 100644 --- a/test/e2e/push.test.ts +++ b/test/e2e/push.test.ts @@ -20,6 +20,7 @@ import { mintUpstreamToken, testConfig } from './setup'; import fs from 'fs'; import path from 'path'; import os from 'os'; +import type { Action } from '../../src/proxy/actions'; describe('Git Proxy E2E - Repository Push Tests', () => { const tempDir: string = path.join(os.tmpdir(), 'git-proxy-push-e2e-tests', Date.now().toString()); @@ -730,9 +731,39 @@ describe('Git Proxy E2E - Repository Push Tests', () => { console.log('[TEST] Step 7: Approving push as authorized approver...'); const approverCookie = await login(approverUser.username, approverUser.password); + // Reviewing a push needs the full diff; the dashboard list only needs metadata. + const detailUrl = `${testConfig.gitProxyUiUrl}/api/v1/push/${pushId}`; + const reviewHeaders = { Cookie: approverCookie }; + const detailResponse = await fetch(detailUrl, { headers: reviewHeaders }); + expect(detailResponse.status).toBe(200); + const pendingPush: Action = await detailResponse.json(); + const diff = pendingPush.diff; + expect(diff).toEqual(expect.stringContaining('Approved Workflow Test')); + expect(diff).toContain('approved-workflow-test.txt'); + const encodedDiff = JSON.stringify(diff).slice(1, -1); + expect(JSON.stringify(pendingPush).split(encodedDiff)).toHaveLength(2); + expect(pendingPush.authorised).toBe(false); + + const listResponse = await fetch(`${testConfig.gitProxyUiUrl}/api/v1/push`, { + headers: reviewHeaders, + }); + expect(listResponse.status).toBe(200); + const pushes: Action[] = await listResponse.json(); + const summary = pushes.find((push) => push.id === pushId); + expect(summary).toMatchObject({ id: pushId, authorised: false }); + for (const field of ['steps', 'lastStep', 'diff']) { + expect(summary).not.toHaveProperty(field); + } + await approvePush(approverCookie, pushId!, defaultQuestions); console.log(`[TEST] SUCCESS: Push ${pushId} approved by ${approverUser.username}`); + const approvedResponse = await fetch(detailUrl, { headers: reviewHeaders }); + expect(approvedResponse.status).toBe(200); + const approvedPush: Action = await approvedResponse.json(); + expect(approvedPush.authorised).toBe(true); + expect(approvedPush.steps).toEqual(pendingPush.steps); + // Step 8: Re-push after approval (should succeed) console.log('[TEST] Step 8: Re-pushing after approval...'); let finalPushOutput = ''; diff --git a/test/processors/diffStorage.test.ts b/test/processors/diffStorage.test.ts new file mode 100644 index 000000000..ee2d3bfd6 --- /dev/null +++ b/test/processors/diffStorage.test.ts @@ -0,0 +1,129 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type { Request } from 'express'; +import { Action, RequestType } from '../../src/proxy/actions'; +import { SAMPLE_COMMIT } from '../../src/proxy/constants'; +import { exec as getDiff } from '../../src/proxy/processors/push-action/getDiff'; +import { exec as scanDiff } from '../../src/proxy/processors/push-action/scanDiff'; + +const { gitDiff } = vi.hoisted(() => ({ gitDiff: vi.fn() })); + +vi.mock('simple-git', () => ({ simpleGit: () => ({ diff: gitDiff }) })); +vi.mock('../../src/config', () => ({ + getCommitConfig: () => ({ diff: { block: { literals: ['BLOCKED_MARKER'] } } }), + getPrivateOrganizations: () => [], +})); + +const request = {} as Request; +const makeDiff = (lines: string[]) => + `diff --git a/example.txt b/example.txt +--- a/example.txt ++++ b/example.txt +@@ -0,0 +1,${lines.length} @@ +${lines.map((line) => `+${line}`).join('\n')} +`; + +const makeAction = () => { + const action = new Action( + 'diff-storage', + RequestType.PUSH, + 'POST', + 1, + 'https://example.com/org/repo.git', + ); + action.proxyGitPath = '/unused'; + action.commitFrom = 'a'.repeat(40); + action.commitTo = 'b'.repeat(40); + action.commitData = [SAMPLE_COMMIT]; + return action; +}; + +describe('diff storage and audit logging', () => { + beforeEach(() => { + gitDiff.mockReset(); + vi.spyOn(console, 'info').mockImplementation(() => {}); + }); + + afterEach(() => vi.restoreAllMocks()); + + it.each([ + ['ordinary', ['reviewable change']], + ['Unicode', ['Ćorić — 日本語 🚀']], + ['large', Array.from({ length: 16384 }, () => 'x'.repeat(64))], + ])('retains a %s diff once for review without logging its contents', async (_name, lines) => { + const diff = makeDiff(lines as string[]); + gitDiff.mockResolvedValue(diff); + const action = await getDiff(request, makeAction()); + await scanDiff(request, action); + + expect(action.error).toBe(false); + expect(action.diff).toBe(diff); + expect(action.steps.find((step) => step.stepName === 'diff')?.content).toBeNull(); + const encodedDiff = JSON.stringify(diff).slice(1, -1); + expect(JSON.stringify(action).split(encodedDiff)).toHaveLength(2); + expect(action.steps.flatMap((step) => step.logs).join('\n')).not.toContain(diff); + expect(console.info).toHaveBeenCalledWith( + `diff - Generated diff (${Buffer.byteLength(diff, 'utf8')} bytes)`, + ); + expect(vi.mocked(console.info).mock.calls.flat().join('\n')).not.toContain(diff); + }); + + it('still rejects prohibited content and records the finding without duplicating the diff', async () => { + const diff = makeDiff(['ordinary line', 'BLOCKED_MARKER']); + gitDiff.mockResolvedValue(diff); + const action = await getDiff(request, makeAction()); + await scanDiff(request, action); + + expect(action.error).toBe(true); + expect(action.continue()).toBe(false); + expect(action.errorMessage).toContain('BLOCKED_MARKER'); + expect(action.errorMessage).toContain('example.txt'); + expect(action.diff).toBe(diff); + expect(action.steps[0].content).toBeNull(); + expect(action.steps[1].logs).toContain( + 'scanDiff - Diff is blocked via configured literals/patterns/providers.', + ); + expect(JSON.stringify(action).split(JSON.stringify(diff).slice(1, -1))).toHaveLength(2); + expect(vi.mocked(console.info).mock.calls.flat().join('\n')).not.toContain(diff); + }); + + it('preserves a legitimate empty diff and its audit messages', async () => { + gitDiff.mockResolvedValue(''); + const action = await getDiff(request, makeAction()); + await scanDiff(request, action); + + expect(action.error).toBe(false); + expect(action.diff).toBe(''); + expect(action.steps[0].content).toBeNull(); + expect(action.steps[0].logs).toContain('diff - Generated diff (0 bytes)'); + expect(action.steps[1].logs).toContain( + 'scanDiff - No commit diff found, but this may be legitimate (empty diff).', + ); + }); + + it('records generation errors without claiming to have generated a diff', async () => { + gitDiff.mockRejectedValue(new Error('Unable to read Git objects')); + const action = await getDiff(request, makeAction()); + + expect(action.error).toBe(true); + expect(action.continue()).toBe(false); + expect(action.steps[0].content).toBeNull(); + expect(action.steps[0].logs).toContain('diff - Unable to read Git objects'); + expect(action.steps[0].logs.join('\n')).not.toContain('Generated diff'); + }); +}); diff --git a/test/testDb.test.ts b/test/testDb.test.ts index 0d9f7fc2c..be5c4a9d3 100644 --- a/test/testDb.test.ts +++ b/test/testDb.test.ts @@ -583,11 +583,16 @@ describe('Database clients', () => { it('should be able to create a push', async () => { await db.writeAudit(TEST_PUSH); const pushes = await db.getPushes({}); - // List responses omit `steps` (and `diff`): both carry the full diff, and - // every backend's list projection excludes them. `getPush` still returns them. - const { steps, ...expected } = TEST_PUSH; - const cleanPushes = cleanResponseData(expected, pushes); - expect(cleanPushes).toContainEqual(expected); + expect(pushes).toContainEqual( + expect.objectContaining({ + id: TEST_PUSH.id, + user: TEST_PUSH.user, + userEmail: TEST_PUSH.userEmail, + }), + ); + expect(pushes.find((push) => push.id === TEST_PUSH.id)).not.toHaveProperty('steps'); + const detail = await db.getPush(TEST_PUSH.id); + expect(cleanResponseData(TEST_PUSH, detail!)).toEqual(TEST_PUSH); }, 20000); it('should be able to delete a push', async () => { diff --git a/test/testPush.test.ts b/test/testPush.test.ts index 8dab34bc8..dbf7aec5c 100644 --- a/test/testPush.test.ts +++ b/test/testPush.test.ts @@ -498,10 +498,15 @@ describe('Push API', () => { const push = res.body.find((p: Action) => p.id === TEST_PUSH.id); expect(push).toBeDefined(); - // Check that all values in push are in TEST_PUSH, except for _id and `steps`: - // list responses omit `steps` and `diff`, both of which carry the full diff. - const { steps, ...expectedPush } = TEST_PUSH; - expect(push).toMatchObject(expectedPush); + expect(push).toMatchObject({ + id: TEST_PUSH.id, + url: TEST_PUSH.url, + user: TEST_PUSH.user, + userEmail: TEST_PUSH.userEmail, + pusherVerified: true, + }); + expect(push).not.toHaveProperty('steps'); + expect(push).not.toHaveProperty('lastStep'); expect(push.canceled).toBe(false); }); diff --git a/website/docs/architecture/processors.md b/website/docs/architecture/processors.md index 4e1874a8d..0ad95ead4 100644 --- a/website/docs/architecture/processors.md +++ b/website/docs/architecture/processors.md @@ -173,7 +173,7 @@ Source: [/src/proxy/processors/push-action/preReceive.ts](https://github.com/fin Executes `git diff` to obtain the diff for the given revision range. If there are no commits (possibly due to a malformed push), the push is blocked. -The data extracted in this step is later used in [`scanDiff`](#scandiff). +The full diff is stored once in `action.diff` for review and later used in [`scanDiff`](#scandiff). The processors log the diff's byte count and scan findings without logging another copy of the full diff. Historical records with the diff in the step's `content` remain readable. Source: [/src/proxy/processors/push-action/getDiff.ts](https://github.com/finos/git-proxy/blob/main/src/proxy/processors/push-action/getDiff.ts) @@ -244,6 +244,8 @@ Source: [/src/proxy/processors/push-action/blockForAuth.ts](https://github.com/f This action runs after a chain has been executed. It stores in the database the entire `Action` object along with the list of `steps` that the action has gone through and their associated logs or error messages that occurred during processing of the chain. +Push list and profile activity responses return metadata without processor steps, `lastStep`, or diff content. Use `GET /api/v1/push/:id` to retrieve the full audit record and diff for review. When `lastStep` exactly matches the final entry in `steps`, the database stores a reference and restores `lastStep` on detail reads. A distinct `lastStep` is preserved. Historical diff content and logs remain readable and are not truncated. + Note: **`audit` writes all actions** (push, pull, default/unclassified) to the DB. An action object (or entry in the pushes table) might look like this: