Repository navigation
Conversation
nollbit
marked this pull request as draft
October 1, 2026 14:27
nollbit
marked this pull request as ready for review
October 1, 2026 15:04
Connect verified the supplied token and populated the connection context, and only then checked whether the connection was already bound to a different repository. When that check failed the request was refused -- but the context had already been changed, so a refused Connect replaced an established connection's identity with its own, along with the cached PartitionGrants and RawToken that later per-action and copy-source checks read. Move the repository check above all verification and all context mutation, so a refused Connect leaves the connection untouched. 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/connect-rejected-reconnect
branch
from
October 5, 2026 08:21
d715252 to
fdfb2db
Compare
epic-lore-bot
Bot
added
imported
|
Imported as Lore CR-800. |
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
|
Closed by mirrored commit 6e87521. |
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
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.
Connectverifies 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 refusedConnecthas replaced an established connection's identity with its own.On current
mainit also replaces the cachedPartitionGrantsandRawToken, 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
Connectleaves 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 aConnectnaming 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 andcargo +nightly fmt --checkclean.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
MutableStorepath writing metadata key types. The two are independent and neither should block the other; that one carries a compatibility question this one does not.