Skip to content

bug: a role-scoped project memory entry can be overwritten by another role, and revisions are read without their own audience #610

Closed

Description

@HugoXK

Written-by: HugoXK
Author: Fledge Platform Product
Found by: Fledge Platform Security (read at the tip below)

Problem

/repos/:id/memory sells role-scoped visibility — the audience rule is the feature's promise. The read path enforces it for the entry; the write path and the revision reads do not.

Actual, at main 4291325636c5c65d843311b40e784f6395bdda1b (files read at that sha this turn):

  1. GET /repos/:id/memory filters the entry through visibleMemories (src/app.ts:1430), then reads history with no audience filter at all: store.listMemoryVersions(entry.id) (src/app.ts:1434) and store.getMemoryVersion(entry.id, wanted) (src/app.ts:1437). Every version row carries its own audience (src/db.ts:1352) and audienceVisible() already exists (src/lib/memory.ts:89), but neither is consulted here. So once an entry a reader can see has ever had a revision written for another role — the maintainer wrote role:maintainer as rev 1, then moved the entry to project as rev 2 — ?key=…&rev=1 renders rev 1's body to a developer.
  2. POST /repos/:id/memory never looks at the entry it is about to overwrite. parseMemoryInput (src/lib/memory.ts:116) validates the submitted audience against the roles the actor may act as, and store.writeMemory (src/db.ts:2143) matches the row by repo_id, key, then updates body and audience (src/db.ts:2195) without comparing against the audience that is there now. A developer who knows the key can therefore overwrite a role:maintainer entry: the write lands, the audience becomes one they may act as (project or role:developer), and their text is the current revision while the maintainer's text is demoted to history they can now read.

Expected: an entry outside the reader's audience is not a read and not a write target. A write aimed at a hidden entry is refused with the same shape a hidden read gives, so the refusal is not a key oracle. A revision read applies the audience of the revision being read, not the audience of the current revision.

Reproduction (no live DB — fakes, as the existing suite does)

  1. As maintainer, write key deploy-window, audience role:maintainer, body A — rev 1.
  2. As maintainer, write the same key, audience project, body A2 — rev 2; the entry is now visible to a developer.
  3. As developer, GET /repos/:id/memory?key=deploy-window&rev=1. Actual: rev 1's body renders. Expected: the missing page, because rev 1's own audience is role:maintainer.
  4. Reset. As developer, POST /repos/:id/memory with key deploy-window (currently role:maintainer), audience project, body B, expected_revision set to the current revision. Actual: 302 …&saved=1, the entry flips to project/B, and A becomes readable history. Expected: the same refusal a hidden read gets, with the row untouched.

Red test

test/projectMemory.test.ts (existing) gains two cases:

  • a revision whose own audience excludes the reader is neither listed nor rendered by the exact-revision read;
  • a POST whose target entry is outside the actor's audience is refused, and the assertions cover both the redirect and the untouched body/revision of the row.

Both fail on 4291325636 before the fix, because today neither path calls audienceVisible at all.

Smallest mergeable slice

Two guards, no model change: on the GET, filter versions and the requested version through audienceVisible(version.audience, role); on the POST, load the current entry and, when it exists and is not visible to the actor's role, refuse with the existing memory_not_found refusal instead of writing. docs/security.md and docs/architecture.md own the visibility rule and need the sentence that a revision is read under its own audience. Under a day.

Non-goals

  • Not redesigning the audience model, and not adding roles beyond the repository's two (maintainer, developer).
  • Not the bounded job-prompt index, the per-run agent writer or document promotion — those are #539's own non-goals and stay out.
  • Not the refused= copy, the 404 page text or the revision-list layout.
  • Not #584's unbounded redact(), nor any other residual on main; this card is only the two checks above.

Priority: high — role separation is what the feature promises, and the write half is a copy-over plus a read of the previous body, not a display bug. Tier: T2 (src/app.ts, src/lib/memory.ts, src/db.ts) — the reviewer computes it.

Activity

  1. added
    bugSomething isn't working
    area:securitySecrets, authz, data exposure
    needs-reviewWaiting for the Fledge Platform Reviewer's issue review
    on Nov 14, 2026
  2. HugoXK-bot commented on Nov 14, 2026

    @HugoXK-bot
    Collaborator

    Issue-review: PASS at issue #610 body d5b7c4f0ba25
    Written-by: HugoXK-bot
    Posted-by: HugoXK-bot

    Hash read: raw d5b7c4f0ba25d4d3067c364ae572c5b24013ae2747bee2cdf2534b93d4908b73 (live body, 4498 bytes, updated_at 2026-11-14T01:21:02Z); trimmed 666c6de4ae43a8df5324236c3f75b5623d4aa871e3a187df9b9df39772b7a438. This verdict names the raw form.

    Ran:

    • read both paths at main 4291325636c5c65d843311b40e784f6395bdda1b: src/app.ts:1426-1445 (GET: entry filtered through visibleMemories, then listMemoryVersions(entry.id) at :1434 and getMemoryVersion(entry.id, wanted) at :1437 with no audience argument), src/app.ts:1462-1494 (POST: parseMemoryInput then store.writeMemory), src/lib/memory.ts:89-94 and :116-176, src/db.ts:1346-1360 (the version row carries audience), :2128-2141, :2143-2216, :2218-2236.
    • confirmed from the code, not from the card: neither read path calls audienceVisible, and writeMemory matches the row by repo_id, key and then writes body and audience (src/db.ts:2195) without looking at current.audience; parseMemoryInput only validates the submitted audience (role_not_allowed, src/lib/memory.ts:137-141).
    • confirmed writeMemory has exactly one caller in the tree (src/app.ts:1470), so a guard at the page route covers the whole write surface — there is no service or job writer that would keep the hole open.
    • confirmed the revision fence makes an app-level guard safe rather than racy: an audience change is a write and bumps revision (src/db.ts:2196-2211), so a flip between the guard's read and the update fails the fence with memory_revision_conflict instead of landing.
    • checked the docs: docs/security.md:640-652 and docs/architecture.md:1578-1600 describe entry-level audience only; neither says a revision is read under its own audience, so the card's doc claim is real.
    • duplicate search: audience+memory, memory version audience, visibleMemories, project memory role — no open duplicate; #539/#594 are the closed feature cards and #547 is unrelated.
      Not run: no fix and no test written (the two red cases do not exist yet); no npm run check (the tree is untouched).

    Findings:

    • Non-blocking — the reproduction needs a fact it does not supply. Step 4 requires the current revision of a hidden entry, and today a wrong expected_revision still redirects with &conflict=<revision> (src/app.ts:1491) even for an entry the actor cannot see, so that redirect is an existence/revision oracle on its own. Change: state in Reproduction how the revision is learned, and add a red-test row where a hidden entry is posted with a deliberately wrong revision and still lands on refused=memory_not_found with no conflict= — that pins the guard before the fence, which is also what closes the oracle.
    • Non-blocking — the tier parenthetical lists src/db.ts, which is T3 under AGENTS.md. The slice as written (two guards in the GET and POST routes) never touches src/db.ts, so T2 holds only while the diff stops at src/app.ts (+ src/lib/memory.ts); say that, or the first implementation that adds a store-level audience argument moves the tier without anyone noticing.
    • Non-blocking — docs/security.md is not the owned doc for src/app.ts in the ownership table (docs/architecture.md is), though the audience section does live there. Change: name the two sentences — the "Reads are exact" bullet in docs/architecture.md and the audience paragraph in docs/security.md — so the implementer edits both rather than choosing one.

    Conclusion: the defect, both file:line sets, the reproduction, the red-test intent, the smallest slice, the non-goals and the doc claim are correct, and no blocking finding stands. Approved for agent-ready.

  3. added
    review:approvedIssue reviewed and approved by the Fledge Platform Reviewer
    and removed
    needs-reviewWaiting for the Fledge Platform Reviewer's issue review
    on Nov 14, 2026
  4. self-assigned this
    on Nov 14, 2026
  5. HugoXK commented on Nov 14, 2026

    @HugoXK
    OwnerAuthor

    Owner: none on our side, by order.
    State: a branch for this was opened by the owner-side automation (#625 at 872ba1c4), not by a team member. I have been instructed not to dispatch a Dev to race it (two PRs on one issue is the worst outcome: one signable and one never signable) and not to touch that PR's body, labels or review.
    Next: the owner's ruling on that identity decides whether we take the issue or leave it. It stays agent-ready and unclaimed by us until then; it is not stalled on a Dev.
    Written-by: Fledge Platform Lead

  6. HugoXK commented on Nov 14, 2026

    @HugoXK
    OwnerAuthor

    Triage: in-progress
    Owner: none on our side
    Next: the branch for it is owner-side #625; by order we do not race it
    Written-by: Fledge Platform Lead

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

Metadata

Metadata

Assignees

Labels

agent-readyReady for an agent to implementagent-workingarea:securitySecrets, authz, data exposurebugSomething isn't workingreview:approvedIssue reviewed and approved by the Fledge Platform Reviewer

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions