Skip to content

Commit 31be8ee

Browse files
security: reject dryRun:false with empty syncedGoalIds (GLA-1077 MED-3)
Guard against accidental full-company mirror: POST /integrations/github now returns 422 when dryRun is disabled but syncedGoalIds is empty. Matches the safe-default requirement from the GLA-38 threat model. Co-Authored-By: Paperclip <noreply@paperclip.ing>
1 parent 5f2737d commit 31be8ee

2 files changed

Lines changed: 52 additions & 0 deletions

File tree

server/src/__tests__/github-integration-handshake.test.ts

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,52 @@ describe("POST /integrations/github — token-to-repo handshake", () => {
190190
expect(mockRegistry.upsertCompanySettings).toHaveBeenCalledOnce();
191191
});
192192

193+
it("accepts dryRun:true with empty syncedGoalIds (onboarding path)", async () => {
194+
mockFetch.mockResolvedValue(makeFetchResponse(200));
195+
196+
const app = await createApp();
197+
const res = await post(app, {
198+
repo: "owner/new-repo",
199+
secretRef: SECRET_ID,
200+
syncedGoalIds: [],
201+
dryRun: true,
202+
});
203+
204+
expect(res.status).toBe(200);
205+
expect(mockRegistry.upsertCompanySettings).toHaveBeenCalledOnce();
206+
});
207+
208+
it("accepts dryRun:false when syncedGoalIds is non-empty", async () => {
209+
const goalId = "cccccccc-cccc-cccc-cccc-cccccccccccc";
210+
mockGoals.list.mockResolvedValue([{ id: goalId }]);
211+
mockFetch.mockResolvedValue(makeFetchResponse(200));
212+
213+
const app = await createApp();
214+
const res = await post(app, {
215+
repo: "owner/new-repo",
216+
secretRef: SECRET_ID,
217+
syncedGoalIds: [goalId],
218+
dryRun: false,
219+
});
220+
221+
expect(res.status).toBe(200);
222+
expect(mockRegistry.upsertCompanySettings).toHaveBeenCalledOnce();
223+
});
224+
225+
it("returns 422 and does not persist when dryRun:false and syncedGoalIds is empty", async () => {
226+
const app = await createApp();
227+
const res = await post(app, {
228+
repo: "owner/new-repo",
229+
secretRef: SECRET_ID,
230+
syncedGoalIds: [],
231+
dryRun: false,
232+
});
233+
234+
expect(res.status).toBe(422);
235+
expect(res.body.error).toMatch(/syncedGoalIds must list at least one goal/i);
236+
expect(mockRegistry.upsertCompanySettings).not.toHaveBeenCalled();
237+
});
238+
193239
it("redacts token from 401 error body before surfacing", async () => {
194240
const leakyBody = `{"message":"Bad credentials for token ${FAKE_TOKEN}"}`;
195241
mockFetch.mockResolvedValue(makeFetchResponse(401, leakyBody));

server/src/routes/github-integration.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,12 @@ export function githubIntegrationRoutes(db: Db) {
166166
dryRun: body["dryRun"] !== false,
167167
};
168168

169+
if (configToSave.dryRun === false && configToSave.syncedGoalIds.length === 0) {
170+
throw unprocessable(
171+
"syncedGoalIds must list at least one goal when dryRun is disabled — leaving it empty would mirror every issue in the company. Either pick the goal subtrees you want to sync, or keep dryRun enabled.",
172+
);
173+
}
174+
169175
// Validate that the configured token is authorised on the target repo whenever
170176
// the repo value changes. Skipped when repo is unchanged to avoid an extra
171177
// GitHub round-trip on every config save.

0 commit comments

Comments
 (0)