feat(gatekeeper-kit): replayable runs with account-adjudicated expiry - #441
feat(gatekeeper-kit): replayable runs with account-adjudicated expiry#441ndisidore wants to merge 2 commits into
Conversation
Preview:
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 1 actionable inline finding. Gatekeeper-kit tests pass: 343 tests. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 1 actionable inline finding. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 2 actionable inline findings. |
|
No new findings. Previously published inline findings remain applicable to the current head. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
|
No new findings. Previously published findings remain applicable to |
Not worth actioning. The window is the notify RPC only; fresh()'s #overtaken already covers the wide mint-await window. The delta is error flavor for one caller — it sees expired instead of the reconnect that landed mid-notify — and its re-entry refetches the live credentials. No false notify is created (the grant was confirmed dead when notify fired; the latch handles dedup).
The scenario needs a triple coincidence: 401 + a concurrent fetch adopting a successor inside the ask window + the report reply lost (fail-closed "expired"). Net damage: the one reporting caller gets CredentialsExpiredError instead of CredentialsChangedError. Consider this accepted |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 1 actionable inline finding. Tests were not run because |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 1 actionable inline finding. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 1 actionable inline finding. Current GitHub build, test, and lint checks pass; local tests were unavailable because |
This comment was marked as outdated.
This comment was marked as outdated.
8d3db43 to
2c1c7a4
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
|
No new findings. Previously published inline findings remain applicable to the current head. |
|
No new findings. Previously published findings remain applicable. |
|
LGTM! |
c288f8b to
6754a17
Compare
|
LGTM! |
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 new potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ Empty vendor IDs lose log attribution
An empty vendor ID makes defineActions discard the configured attribution. Both credential helpers and observer tracking repeat this check, leaving their logs unattributed.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
|
LGTM! |
| } | ||
| this.#logger.error("malformed credential rejection verdict", { | ||
| event: "credentials.rejection.verdict.malformed", | ||
| error: new Error(`unexpected verdict: ${String(verdict)}`), |
There was a problem hiding this comment.
P2 Do not log the malformed verdict value
This fail-closed branch copies an arbitrary out-of-contract RPC result into the logged error message. If a hand-written account implementation accidentally returns a provider response or bearer string instead of a verdict, that credential is persisted in server logs, contrary to the logging rule forbidding tokens and response bodies. Log only static/type metadata, and add a test that a malformed secret-like value is absent from the logged event.
|
Posted 1 actionable inline finding. Tests were not run because |
| // Dual guard, neither subsumes the other: the fence blocks fetches started before an expiry | ||
| // "" is reserved for a never-connected read: adopting live credentials under it would wedge | ||
| // every rejection as retryable, since "" always adjudicates superseded. | ||
| if (current.identity === "") { |
There was a problem hiding this comment.
P1 Clear the previous cache authority before rejecting this read
After a successful id-a/gen-a fetch, this source may hold cache entries partitioned by gen-a. If the account reconnects as B and a hand-written stub accidentally serves B under the reserved empty identity, this branch throws but leaves #generation === "gen-a"; a later cache-first request can therefore return A’s data to B without another credential fetch. Since this response cannot establish which principal is current, clear/fence the authority before throwing, and extend this test to start from an established partition.
|
Posted 1 actionable inline finding. |
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
|
d521cdd to
afcf9ed
Compare
|
Posted 1 actionable inline finding. |
CredentialSource.run gains a replayable option: a credential rejection is retried once with credentials minted through a refreshCredentials channel, and only a rejection of those reports expiry — so derived-bearer ports stop reporting routine stale bearers as grant death. The flag without a channel throws at the call. The refresh is observed, never adopted: plain reads stay the snapshot's only writer, which removes the second concurrent writer earlier revisions fenced around. noteCredentialsExpired now returns the account's verdict on the reported identity — an explicit false resolves as the fixed retry message with the cache authority dropped; anything else (lost answers included) fails closed as accepted. The verdict is asked first, then clear and fence land as one synchronous transition, and a reconnect crossing the refresh fences any authority not adopted past it. Replays coalesce per rejected read via SingleFlight's new object keys. Plan §4.6/§4.13/§5.6/§5.8/§6 reconciled with the reduced model.
afcf9ed to
3feafe3
Compare
| [Symbol.dispose](): void { | ||
| if (this.#disposed) return; | ||
| this.#disposed = true; | ||
| this.#dispose?.(); |
There was a problem hiding this comment.
P2 Defer owned-resource disposal until submitted cursor calls settle
A client can issue its final next() and immediately dispose the cursor stub while retaining the returned RPC promise. Cap’n Web releases the target when the stub is dropped, so this hook can dispose the callback-owned RPC stub while that fetchPage() is still using it, making the already-submitted page fail. Queue the cleanup behind #queue (which synchronously claims each submitted call), and add a gated test covering disposal with running and queued next() calls.
|
Posted 1 actionable inline finding. |
1 participant
CredentialSource.rungains areplayableoption: when the provider rejects credentials, the account decides whether they areexpired,superseded, orunavailable, and a replayable operation may retry once with a same-connection successor without crossing a reconnect.This is an improved version of
fetchWithAuthRetrythat many gatekeepers hand rollDerived-bearer ports can heal a rejected current credential inside
reportCredentialsRejected, with identity-keyed single-flight coalescing concurrent mints; grant-death ports instead notify the Workshop and return expired.Each attempt receives its own
{ identity, generation }fence, while namedCredentialsChangedErrorandCredentialsExpiredErrorresults survive RPC boundaries and stale reads, reports, and refetches cannot re-establish cache authority.