Repository navigation
Conversation
RepositoryMetadata and BranchMetadata have dedicated write APIs (RepositoryMetadataSet, BranchMetadataSet) that validate read-only fields before writing. The generic mutable-store write accepts any KeyType, including those two, and writes the underlying key directly -- so that validation is skipped for a caller who addresses the store instead of the API. The path is reachable from gRPC v1, QUIC v4 and the legacy QUIC storage protocol. Refuse both key types there, directing callers to the dedicated APIs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Johan Mjönes <johan@playgoals.com>
nollbit
force-pushed
the
fix/mutable-store-metadata-keys
branch
from
October 5, 2026 08:15
524b507 to
76449a6
Compare
epic-lore-bot
Bot
added
imported
|
Imported as Lore CR-799. |
epic-lore-bot Bot
pushed a commit
that referenced
this pull request
Oct 7, 2026
`Connect` verifies the supplied token and populates the connection context, and only then checks whether the connection is already bound to a different repository. When that check fails the request is refused — but the context has already been mutated, so a refused `Connect` has replaced an established connection's identity with its own. On current `main` it also replaces the cached `PartitionGrants` and `RawToken`, both of which later per-action and copy-source checks read. The fix moves the repository check above all verification and all context mutation, so a refused `Connect` leaves the connection untouched. It is a reorder; no logic changes. ## Scope Present in v0.8.6, v0.9.0, v0.10.0 and current `main`. ## Test `a_rejected_reconnect_leaves_the_existing_token_alone` — establishes a connection bound to one repository with a known token, issues a `Connect` naming a different repository, and asserts both that it is refused and that the original token and repository survive. It fails before this change and passes after. Build, `cargo test -p lore-server`, clippy and `cargo +nightly fmt --check` clean. ## Provenance Reported through HackerOne first, since it touches authorization state. Epic's security team closed it as Informative — reasonably, since the report was source analysis with no runtime PoC, and exploiting it requires getting a request onto an established connection, which is not generally possible. Opening it here as an ordinary correctness fix on that basis. The practical argument for it is small and cheap: an operation that reports failure should not have changed authorization state on the way out. ## Related #232 covers the other issue from the same report — the generic `MutableStore` path writing metadata key types. The two are independent and neither should block the other; that one carries a compatibility question this one does not. ``` Imported-PR: #231 Imported-From: fdfb2db Imported-Base: 05bc767 Imported-Merge: 9f9a1f8 Imported-Merge-Strategy: verbatim Imported-Merged-Paths: 0 Imported-Author: Johan Mjönes (nollbit) Signed-off-by: Johan Mjönes <johan@playgoals.com> GH-URL: #231 ``` Lore-RevId: 1520 Lore-Signature: d34dbd29270f7ed7c72a11f7be7317fa045ca68407980c786fbbd9fc89fffbff
epic-lore-bot Bot
pushed a commit
that referenced
this pull request
Oct 9, 2026
…nd MutableCompareAndSwap paths ## Motivation Three mutable key types are written only through requests that validate the write. `RepositoryMetadataSet` and `BranchMetadataSet` check that read-only metadata fields are unchanged and that referenced blobs exist. `BranchPush` checks branch protection and that the pushed revision's fragments are present. On `main`, the generic `MutableStore` and `MutableCompareAndSwap` storage requests accept any `KeyType`, including these three, and write the key directly. A client with write access can therefore overwrite repository or branch metadata, or move a protected branch head, without any of those checks. This works over gRPC v1, legacy gRPC, QUIC v4 and legacy QUIC. A `MutableLoad` is not even needed: a CAS with a zero expected value returns the current value to retry with. ## Summary - A shared check, `check_generic_write_key_type`, runs first in both `handle_mutable_store` and `handle_mutable_cas`. Every transport dispatches through these two functions. It refuses `RepositoryMetadata`, `BranchMetadata` and `BranchLatestPointer`. - The refusal is a new `MessageHandleError::InvalidArgument`. gRPC answers `InvalidArgument`, which is not counted as a server error. QUIC has no code for a rejected argument, so it answers the generic `Failed`, like other request-validation errors. - Behaviour change: the C API's `lore_storage` mutable store and CAS calls made with `remote` set, and a server running with `mutable_store.mode = "remote"`, send these same generic requests, so neither can write these key types to a server any longer. Calls on a handle's local store are unchanged. Remote mutable store mode is not in use. The release note documents the change. - `storage_remote_test` used `BranchLatestPointer` as an arbitrary key type for its remote round trips, and now uses `Untyped`. ## Test Plan - Added `rejects_key_types_with_a_dedicated_write_request` for both store and CAS, covering all three key types. Each unwraps the `InvalidArgument` and checks that the reason names the key type. The CAS test also checks that the stored value is unchanged. With the guard removed from `handle_mutable_cas`, the CAS test failed: the CAS returned `Ok`. - Added transport mapping tests: gRPC `InvalidArgument` answers `Code::InvalidArgument` with the reason and is not a server error, and QUIC v4 answers `Failed`, is labelled `InvalidArgument` and is not an internal error. - `cargo test -p lore-integration-tests --features integration_tests --test integration storage_`: 211 passed. With `storage_remote_test` temporarily set back to `BranchLatestPointer`, the remote store and CAS round trips failed against the in-process server, confirming the C API remote path reaches the guard. - After merging `main`: `cargo test -p lore-server` (1402 passed), `cargo clippy -p lore-server -p lore-integration-tests --all-targets -- -D warnings --no-deps` (with and without `integration_tests`) clean, `cargo +nightly fmt --all`. - Confirmed that `lore-revision` writes remote metadata only through `metadata_set` and `branch_metadata_set`, and that no URC code matches on `MessageHandleError`. - Not run: smoke tests, and codespell, which is not installed locally. ## Provenance Reported through HackerOne first, together with a separate `Connect` ordering issue. Epic's security team closed it as Informative — the report was source analysis with no runtime PoC. Opening it here as an ordinary hardening change on that basis. Deliberately separate from the `Connect` fix (#231), despite the two being filed together: they are independent, and this one carries a compatibility question the other does not. Neither should block the other. The original change refused the two metadata key types on the generic `MutableStore` path. Review extended it to `MutableCompareAndSwap` and to `BranchLatestPointer`, and changed the refusal from an authorization failure to `InvalidArgument`. ``` Imported-PR: #232 Imported-From: 76449a6 Imported-Base: 05bc767 Imported-Merge: c46a7b1 Imported-Merge-Strategy: verbatim Imported-Merged-Paths: 0 Imported-Author: Johan Mjönes (nollbit) Signed-off-by: Johan Mjönes <johan@playgoals.com> GH-URL: #232 ``` Co-authored-by: Peter Lockhart <peter.lockhart@epicgames.com> Lore-RevId: 1565 Lore-Signature: 87d00b2cad61f9fc0b48921babe32026077a3d43e92ca04806d01c600269594f
|
Closed by mirrored commit 942a4a1. |
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
2 participants
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.
RepositoryMetadataandBranchMetadatahave dedicated write APIs —RepositoryMetadataSetandBranchMetadataSet— that validate read-only fields before writing. The generic mutable-store write accepts anyKeyType, including those two, and writes the underlying key directly, so that validation is skipped for a caller who addresses the store instead of the API.The path is reachable from gRPC v1, QUIC v4 and the legacy QUIC storage protocol.
This refuses both key types there, directing callers to the dedicated APIs.
Scope
Present in v0.8.6, v0.9.0, v0.10.0 and current
main.Test
rejects_repository_and_branch_metadata_key_types.Build,
cargo test -p lore-server, clippy andcargo +nightly fmt --checkclean.The judgement call, which is yours
Refusing outright is the conservative reading and it is what we run in production, but it is a behaviour change on a public path, and if any legitimate caller writes those key types through the generic store, this breaks it. I looked and did not find one, but you know the codebase far better than I do.
The gentler variant is to route those writes through the same validation the dedicated APIs apply, rather than refusing them. Happy to rework it that way, or to drop this entirely if the generic path is load-bearing for something I have missed.
Provenance
Reported through HackerOne first, together with a separate
Connectordering issue. Epic's security team closed it as Informative — the report was source analysis with no runtime PoC. Opening it here as an ordinary hardening change on that basis.Deliberately separate from the
Connectfix (#231), despite the two being filed together: they are independent, and this one carries a compatibility question the other does not. Neither should block the other.