fix: data-integrity and security hygiene follow-up (post-GF4) - #4
Merged
Merged
Conversation
setCanonicalSources did a full clear-and-rewrite of the Yjs sources map on every command, from a snapshot that materializeSources had already silently filtered (any source whose sourceJson failed to parse was dropped). The next command, even one unrelated to sources, would permanently destroy that entry. Now preserved instead of deleted when it can't be round-tripped through the typed snapshot. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194R12WjWHUbh7EQbjB3i1h
loadRuntimeSession silently replaced an in-progress runtime session
with a fresh one whenever the guide's stepIds changed, discarding real
recorded progress with no signal to the caller. It now returns
{ runtime, supersededSession }, and run.$guideId.tsx shows a
dismissible banner when real progress was set aside. The old session
was never deleted from storage (a new sessionId is created) — this
just makes the fact visible instead of silent.
Also wires in isRuntimeSession, an already-exported semantic guard
(completions/currentStepIndex/status cross-field consistency) that the
"resume an existing session" path never actually called, alongside the
Ajv shape check. Without it, a schema-valid but internally inconsistent
stored record (e.g. status: 'completed' with no matching completions)
would be trusted and rendered as complete by the UI.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0194R12WjWHUbh7EQbjB3i1h
POST /api/session minted the organization-owner role for any caller whenever GUIDEFORGE_OWNER_ID was unset, documented as "loopback/dev mode" but never actually enforced as loopback-only in code. Added assertSafeBindConfig (apps/api/src/bind-guard.ts), called at startup in server.ts: refuses to bind to a non-loopback host unless GUIDEFORGE_OWNER_ID is also set, turning that comment into an enforced invariant. Also: infra/docker/docker-compose.yml published 8080:8080 for the api service without ever setting GUIDEFORGE_HOST, so the app bound to 127.0.0.1 inside the container and the published port was actually unreachable. Documented this and wired GUIDEFORGE_HOST/ GUIDEFORGE_OWNER_ID through as configurable env passthroughs, with the new guard as the backstop if network mode is enabled without an owner. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194R12WjWHUbh7EQbjB3i1h
verifyReleasePackage verified the embedded signature against the
public key embedded in the same package — proves internal
self-consistency only, not authenticity. Anyone can sign a forged
package with their own key and embed it alongside; keyId was a
free-form label with no cryptographic binding to the key that signed
it. TrustedKeyStore (signing.ts) already existed, fully unit-tested,
but was never wired into verification anywhere.
Added an optional { trustedKeys: TrustedKeyStore } option: when
provided, the embedded keyId must resolve to a currently-active pinned
entry AND its public key must match what's embedded, or verification
fails. Omitting the option preserves prior behavior exactly.
apps/xr-web/src/main.tsx (the one consumer that opens .gforge files
from outside the local session) still doesn't pass a trust store —
documented why in a code comment rather than inventing a
key-distribution mechanism unprompted.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0194R12WjWHUbh7EQbjB3i1h
Tracks the four GF4 residual-risk fixes in this branch, the newly discovered (not yet fixed) claims/citations/generationRuns provenance-drop pattern, and a corrected assumption: PLAN_TONIGHT.md's Phase 0.3 (remove committed planning-pack zips) doesn't apply — those files are already .gitignored, deliberately, across three prior commits. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194R12WjWHUbh7EQbjB3i1h
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Root-level 'pnpm format:check' (prettier --check .) covers the whole repo including docs/, unlike the per-package turbo check tasks I'd verified locally, which only run prettier scoped to each package's src/. CI caught this; verified pnpm format:check, lint, typecheck, build, boundary, dep-check, and security:policy-test all pass at the root level before repushing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194R12WjWHUbh7EQbjB3i1h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Scoped follow-up pass fixing four residual risks called out honestly in
docs/progress/GF4_RELEASE_REPORT.mdafter the0.14.0-rc.1merge — all four verified against current code (not assumed from prior docs) before fixing.packages/collaboration/src/index.ts):setCanonicalSourcesdid a full clear-and-rewrite of the Yjs sources map on every command, from a snapshot that had already silently filtered out any unparseable source. The next command — even one unrelated to sources — permanently destroyed that entry. Now preserved instead of deleted when it can't round-trip through the typed snapshot.apps/web/src/services/guideStore.ts,run.$guideId.tsx):loadRuntimeSessionsilently replaced an in-progress session with a fresh one whenever the guide's steps changed, discarding real recorded progress with zero signal. Now returns{ runtime, supersededSession }and the run page shows a dismissible banner when that happens. Also wires inisRuntimeSession— an already-exported semantic consistency guard that existed but was never called on the load path — alongside the Ajv shape check, so a schema-valid-but-inconsistent stored record can no longer render as falsely complete.apps/apiorganization-owner default (apps/api/src/bind-guard.ts,server.ts,infra/docker/docker-compose.yml): the server minted theorganization-ownerrole for any caller wheneverGUIDEFORGE_OWNER_IDwas unset, documented as "loopback/dev mode" but never actually enforced as loopback-only. Now refuses to boot on a non-loopback bind without an explicit owner. Also documented + wired up why the compose file's published8080port was previously dead (bound to127.0.0.1inside the container with no way to configure otherwise).packages/package-gforge/src/release.ts):verifyReleasePackageverified a release's signature against the public key embedded in the same package — proves internal self-consistency, not authenticity.TrustedKeyStorealready existed fully built and unit-tested but was never wired into verification anywhere. Added an optional{ trustedKeys }parameter; omitting it preserves prior behavior exactly.One item from
PLAN_TONIGHT.md(Phase 0.3, "remove committed planning-pack zips") turned out not to apply — those files are already.gitignored, deliberately, across three separate prior commits with explicit rationale comments. Documented as a corrected assumption rather than silently dropped. Full details and one newly-discovered-but-not-yet-fixed issue (the same provenance-drop pattern inclaimsJson/citationsJson/generationRunsJson) are indocs/progress/CLAUDE_SESSION_WORKLOG.md.Test plan
pnpm check(format/lint/typecheck/test/build across all 25 packages): 98/99 tasks pass. The only failure is@guideforge/api#test's Postgres-dependent cases (6 of 22) — confirmed viagit stashto fail identically on unmodifiedmain(no local Postgres in this sandbox,ECONNREFUSED :15432), not a regression from this branch.Security / privacy impact
Two of the four items are security-relevant: the
apps/apiowner-role default now fails closed instead of open, and release verification now supports authenticity pinning instead of only self-consistency. Neither changes default behavior for existing local-only/loopback usage.Known limitations
apps/xr-web(the one consumer that opens.gforgefiles from outside the local session) still doesn't pass a trust store to verification — documented in a code comment rather than inventing a key-distribution mechanism unprompted.claimsJson/citationsJson/generationRunsJson(same root cause as the sources fix, different write shape) is flagged but not fixed this pass — needs its own dedicated treatment.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
https://claude.ai/code/session_0194R12WjWHUbh7EQbjB3i1h