Skip to content

fix(security): cross-project trust gate must fail closed in gstack-learnings-search (#1745)#1749

Closed
Pablosinyores wants to merge 1 commit into
garrytan:mainfrom
Pablosinyores:fix/learnings-search-trust-gate-fail-closed
Closed

fix(security): cross-project trust gate must fail closed in gstack-learnings-search (#1745)#1749
Pablosinyores wants to merge 1 commit into
garrytan:mainfrom
Pablosinyores:fix/learnings-search-trust-gate-fail-closed

Conversation

@Pablosinyores

Copy link
Copy Markdown

Summary

Closes #1745.

The inline comment in bin/gstack-learnings-search promises an allowlist: cross-project learnings are only loaded if trusted (user-stated) to prevent prompt injection from one project's AI-generated learnings silently influencing reviews in another. The implementation is a denylist:

if (isCrossProject && e.trusted === false) continue;

Rows where trusted is missing / undefined (not literally false) are admitted because undefined === false is false. The trusted field landed in #988 (2026-04-13); gstack-learnings-search shipped earlier (#622, 2026-03-29). Any learnings.jsonl written in that window — plus legacy hand-edited rows and rows produced by other tools — lacks the field, so those rows leak across projects under --cross-project today.

Fix

Flip the gate to a proper allowlist:

if (isCrossProject && e.trusted !== true) continue;

Now only rows that explicitly set trusted: true are admitted across projects. Current-project rows are untouched (the gate is scoped to isCrossProject).

Test plan

  • New regression test (gstack-learnings-search cross-project trust gate fails closed for rows missing the trusted field (#1745)) inserts a legacy-shape row with no trusted field into the other-project fixture and asserts the search does not surface its insight text under --cross-project --query INJECTED.
  • Pre-existing cross-project mode only imports trusted entries from other projects still passes (foreign-user (trusted=true) admitted, foreign-observed (trusted=false) blocked).
  • bun test test/gstack-learnings-search.test.ts test/learnings.test.ts test/learnings-injection.test.ts — 36/36 green locally.

…arnings-search (garrytan#1745)

The inline comment in bin/gstack-learnings-search promises an
allowlist: cross-project learnings are 'only loaded if trusted
(user-stated)' to prevent prompt injection from one project's
AI-generated learnings silently influencing reviews in another.

The implementation is a denylist:

    if (isCrossProject && e.trusted === false) continue;

Rows where 'trusted' is missing/undefined (not literally false) are
admitted because 'undefined === false' is false. The 'trusted' field
landed in garrytan#988 (2026-04-13); 'gstack-learnings-search' shipped earlier
(garrytan#622, 2026-03-29). Any learnings.jsonl written in that window — plus
legacy hand-edited rows and rows produced by other tools — lacks the
field, so those rows leak across projects under --cross-project today.

Flip the gate to an allowlist:

    if (isCrossProject && e.trusted !== true) continue;

Now only rows that explicitly set trusted: true are admitted across
projects. Current-project rows are untouched (the gate is scoped to
isCrossProject). New regression test pins the legacy-row branch by
inserting an entry with no 'trusted' field into the other-project
fixture and asserting the search does not surface its insight text
under --cross-project --query INJECTED. Full learnings + injection
suites green (36/36).
@jbetala7

Copy link
Copy Markdown
Contributor

Heads up on a collision: this overlaps with #1746, which is already open against the same issue (#1745) with the same fix.

Both PRs:

  • flip if (isCrossProject && e.trusted === false) continue;if (isCrossProject && e.trusted !== true) continue; in bin/gstack-learnings-search
  • add a regression test inserting a legacy source: observed row with no trusted field into the other-project fixture and assert its INJECTED insight is not surfaced under --cross-project

#1746 was opened 2026-05-27T07:55Z (right after #1745 was filed at 07:55Z); this one at 10:24Z, so they landed independently within a couple hours. Flagging it so a maintainer doesn't double-review the identical change — no issue with the approach here, the allowlist flip is correct. Whichever lands, the other can close out.

@Pablosinyores

Copy link
Copy Markdown
Author

@jbetala7 — confirmed, your #1746 lands first and is the same flip on the same two files (bin/gstack-learnings-search + test). No reason to keep this open; closing in favour of #1746.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gstack-learnings-search cross-project trust gate fails open for rows missing the trusted field

2 participants