Conversation
Signed-off-by: Effi-S <effi.szt@gmail.com>
Signed-off-by: Effi-S <effi.szt@gmail.com>
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
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.
Fixes #2066
Summary
Provider.areMe(backing bothAreMeandIsMe) returns whatever partial result it has accumulatedwhen the storage lookup for signer existence errors, rather than propagating the error. This means a
transient storage failure makes
IsMereportfalse("not mine") for an identity that actually isours, which is on the token-ownership decision path.
Where
token/services/identity/provider.go:270-279:On error,
resultcontains only whatever was already resolved from the in-memory cache(
provider.go:257-264) before the storage call — everything not in cache is silently reported as"not mine," logged only at
Errorfwith no error propagated to the caller.Related:
token/services/identity/wallet/service.go:138-149Wallet()similarly discards two errorsfrom
OwnerWallet/IssuerWalletand returnsnil(a caller cannot distinguish "no wallet" from"lookup failed"):
Impact
IsMeis used to decide ownership-related behavior (e.g. whether to react to a token as an ownedtoken). A transient storage error causing a false negative means an owned token can be treated as
not-owned for that call, with no visible error — the caller has no signal that the answer is
unreliable rather than authoritative.
Reproduction
Not yet committed. Mock
Storage.GetExistingSignerInfoto return an error for a set of identitiesthat includes at least one genuinely-owned identity not already warm in the in-memory cache; assert
IsMe/AreMeeither propagates the error or is documented as best-effort at the call sites that relyon it for correctness-sensitive decisions.
Suggested fix
AreMe/IsMe's current signatures ([]string/bool, no error return) make it structurally unableto distinguish "confirmed not mine" from "couldn't check." Changing the signature ripples through the
driver.IdentityProviderinterface and every caller, so the minimal fix here is: keep the signatures,but do not silently swallow storage errors into a negative result for the specific identities the
storage call was about — instead of returning early, consider retrying once, or making the
error-swallowing explicit and loud enough (metric increment, not just a log line) that an operator can
detect ownership answers were degraded. If callers need a hard guarantee, a follow-up to thread an
error return through
AreMeis worth considering separately.Severity
MEDIUM — requires a transient storage failure to trigger, and produces an unsignaled false negative
on an ownership decision.