[i,l,-r] Adversarial-review fixes: single-use race, dedupe redesign, IAM retry, requester honesty (SECOP-1100/1101/1102/1103)#17
Merged
Conversation
… (SECOP-1100) Fixes three defects from the 2026-07-10 adversarial review (live in prod): - Non-atomic check-then-act: verifyNotConsumed→execute→markConsumed(set upsert) let concurrent approvers all pass. Replace with an atomic Firestore create() claim taken immediately BEFORE execute (claimForApproval); the loser of the create is rejected. Release the claim if execute fails before granting so legitimate retries work. - onProposalApproved runs inside execute() AFTER the grant; an uncaught error there 500'd the approver and skipped audit+consume, leaving the token replayable. It's now a non-throwing wrapper (best-effort notification, ERROR log) around notifyProposalApproved. - Approver DM no longer asserts 'the requester has been notified' (the beneficiary DM is best-effort and may not have landed). GET keeps verifyNotConsumed for the friendly 'already used' banner; the claim is the real gate. Fail-open on Firestore errors is preserved. Tests updated: claim-before-execute ordering, reject-when-claim-lost, claim fail-open, release best-effort.
…er copy honesty (SECOP-1103)
SECOP-1102 (AbstractIamClient):
- On exhausting the member-domain propagation retries, throw
AccessDeniedException naming the iam.allowedPolicyMemberDomains policy
instead of falling through to NotAuthenticatedException('Not
authenticated') — a genuine external-principal denial now points
operators at the org policy, not auth.
- Widen the retry budget 3→4 (2/4/8/16 ≈30s) to cover the full 15–26s
propagation lag the fix documents; only runs on first-ever join of an
org-scoped role.
SECOP-1103 (index.html): the JOIN_PROPOSED list is who we *requested*,
not a delivery receipt (reviewers not in Slack silently get no DM, and
the Slack DM is the delivery-accurate surface) — reword 'Sent to' →
'Reviewers requested' / 'We asked …'. Fix the picker hint that promised
'closest teammates' unconditionally to also cover the small-named-ACL
broadcast. (Naming the full expanded broadcast audience is deferred —
it needs the recipient list threaded back through propose, folded into
SECOP-1101.)
…OP-1101) Replaces the SECOP-1097 duplicate-skip, which ran inside the Slack notification layer AFTER a token was minted, with a non-atomic existence check that (a) treated any registry doc as live so an expired request locked out re-submits for up to the ~24h TTL lag, (b) had a lookup→record TOCTOU under concurrency, and (c) left a second live token dangling on every duplicate. Now: AbstractProposalHandler.propose reserves a pending marker BEFORE minting via a new reserveProposal hook (default no-op=true for mail/debug); a live reservation → AccessDeniedException, no token minted. SlackProposalHandler reserves atomically through Firestore create(), and treats an existing-but-expired marker as free (compares expires_at to now, not TTL deletion) — closing both the lockout and the TOCTOU. Reservation released if propose fails after reserving. Fail-open on Firestore errors. Keys auto-selected requests on (beneficiary, group) so a re-submit whose picked set shifted (affinity/presence) is still the same request; user-picked sets include recipients. The old in-handler skip is gone.
phosphore
added a commit
that referenced
this pull request
Jul 10, 2026
… per-candidate degradation (SECOP-1104) Addresses the four defects the adversarial review found in the SECOP-1098 ranking (all in this unmerged #16): - Latency: enrich the shortlist CONCURRENTLY (async lookup→presence+tz chains, joined once) instead of ~3×15 strictly-sequential blocking calls on the already-slow propose POST. - Per-candidate degradation: each chain .exceptionally → 'unknown' (tier 2); one bad candidate no longer aborts ranking for all (the outer fail-open now only catches systemic errors). - Timezone wrap: compare offsets on the 24h circle (min(d, 86400-d)) so antimeridian teammates with identical local clocks rank as close, not ~24h apart. - Scope-error visibility: SlackClient.isActive/timezoneOffsetSeconds WARN on !isOk (e.g. missing users:read), and the ranking INFO log carries the tier distribution (active/tzNear/other) so a silently inert ranking is detectable. Tests: antimeridian promotion, per-candidate degradation vs wholesale fallback. Suite 1050/1050. NOTE: still to do before #16 merges — rebase onto the SECOP-1100..1103 fixes (PR #17) and reconcile the version bump; and the cross-phase lookupUserByEmail cache (ranking re-looks-up winners the fan-out then looks up again) is deferred as a pure optimization.
phosphore
added a commit
that referenced
this pull request
Jul 10, 2026
… per-candidate degradation (SECOP-1104) Addresses the four defects the adversarial review found in the SECOP-1098 ranking (all in this unmerged #16): - Latency: enrich the shortlist CONCURRENTLY (async lookup→presence+tz chains, joined once) instead of ~3×15 strictly-sequential blocking calls on the already-slow propose POST. - Per-candidate degradation: each chain .exceptionally → 'unknown' (tier 2); one bad candidate no longer aborts ranking for all (the outer fail-open now only catches systemic errors). - Timezone wrap: compare offsets on the 24h circle (min(d, 86400-d)) so antimeridian teammates with identical local clocks rank as close, not ~24h apart. - Scope-error visibility: SlackClient.isActive/timezoneOffsetSeconds WARN on !isOk (e.g. missing users:read), and the ranking INFO log carries the tier distribution (active/tzNear/other) so a silently inert ranking is detectable. Tests: antimeridian promotion, per-candidate degradation vs wholesale fallback. Suite 1050/1050. NOTE: still to do before #16 merges — rebase onto the SECOP-1100..1103 fixes (PR #17) and reconcile the version bump; and the cross-phase lookupUserByEmail cache (ranking re-looks-up winners the fan-out then looks up again) is deferred as a pure optimization.
phosphore
added a commit
that referenced
this pull request
Jul 10, 2026
…SECOP-1098) (#16) * auto-narrow: rank auto-picked reviewers availability-first (SECOP-1098) On an empty picker selection we auto-pick the requester's closest teammates; ranking them purely by team affinity meant an asleep or off-shift teammate ranked the same as one online now. Since the point of picking is to minimise how long the requester waits, reorder the affinity shortlist availability-first before capping at 5: tier 0: currently active on Slack tier 1: in a working-hours timezone within 2h of the requester tier 2: everyone else (stable sort, so affinity breaks ties within a tier) Wiring: ProposalHandler gains a rankReviewersByAvailability default (no-op passthrough — mail/debug keep pure affinity); SlackProposalHandler overrides it using users.getPresence + users.info (tz), reusing the SlackClient it already holds, so GroupsResource needs no new dependency. Guarantees: - availability is a tiebreaker, never a filter — an offline teammate is deprioritised, not dropped, so an off-hours request still lands in the closest teammates' DMs; - fail-open: any Slack enrichment error falls back to affinity order; - bounded: only a 15-teammate shortlist is enriched, behind the picker's existing per-user rate limiter. NOTE: presence/tz behaviour under the bot token wants live validation on a real workspace before this deploys (it's fail-open, so no risk to the base flow if it misbehaves). Version -> 2.3.0-wavemm.12. * availability ranking: parallel enrichment, tz wrap, scope visibility, per-candidate degradation (SECOP-1104) Addresses the four defects the adversarial review found in the SECOP-1098 ranking (all in this unmerged #16): - Latency: enrich the shortlist CONCURRENTLY (async lookup→presence+tz chains, joined once) instead of ~3×15 strictly-sequential blocking calls on the already-slow propose POST. - Per-candidate degradation: each chain .exceptionally → 'unknown' (tier 2); one bad candidate no longer aborts ranking for all (the outer fail-open now only catches systemic errors). - Timezone wrap: compare offsets on the 24h circle (min(d, 86400-d)) so antimeridian teammates with identical local clocks rank as close, not ~24h apart. - Scope-error visibility: SlackClient.isActive/timezoneOffsetSeconds WARN on !isOk (e.g. missing users:read), and the ranking INFO log carries the tier distribution (active/tzNear/other) so a silently inert ranking is detectable. Tests: antimeridian promotion, per-candidate degradation vs wholesale fallback. Suite 1050/1050. NOTE: still to do before #16 merges — rebase onto the SECOP-1100..1103 fixes (PR #17) and reconcile the version bump; and the cross-phase lookupUserByEmail cache (ranking re-looks-up winners the fan-out then looks up again) is deferred as a pure optimization. * build: 2.3.0-wavemm.13 (availability ranking on top of the review fixes)
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.
Fixes from the 2026-07-10 max-effort adversarial review of the #13–#16 range.
2.3.0-wavemm.12; full suite 1046/1046.SECOP-1100 (was live in prod) — single-use approvals made atomic.
verifyNotConsumed → execute → markConsumed(set upsert)was a check-then-act; concurrent approver clicks all passed. Now an atomic Firestorecreate()claim is taken immediately before execute (claimForApproval); the loser is rejected, and the claim is released if execute fails pre-grant. Also:onProposalApproved(which runs insideexecute(), after the grant) is now a non-throwing best-effort wrapper — an error there previously 500'd the approver and skipped consume/audit, leaving the token replayable. Approver DM no longer claims "the requester has been notified" (best-effort DM).SECOP-1101 — duplicate-request dedupe redesigned. The SECOP-1097 skip ran after minting (left a second live token), treated any registry doc as live (≤24h TTL-lag lockout of legit re-requests), and had a lookup/record TOCTOU. Now
AbstractProposalHandler.proposereserves a pending marker atomically before minting; a live reservation throws (no second token). Expiry-aware (comparesexpires_atto now, not TTL deletion) and released on propose failure. Auto-selected requests key on(beneficiary, group)— presence-stable across re-submits; user-picked sets include recipients.SECOP-1102 — IAM member-domain retry. Genuine denials no longer surface as the misleading "Not authenticated" (throw
AccessDeniedExceptionnaming the org policy on exhaustion); budget widened 3→4 (2/4/8/16 ≈30s) to cover the full observed 15–26s propagation.SECOP-1103 — requester-facing honesty. JOIN_PROPOSED list reworded "Sent to" → "Reviewers requested" (it's intent; reviewers not in Slack silently get no DM, and the Slack DM is delivery-accurate); picker hint fixed to also cover the small-named-ACL broadcast.
Not included: SECOP-1104 (availability-ranking fixes) — those are on the unmerged #16 branch, handled separately.
🤖 Generated with Claude Code