Skip to content

fix(gateway): don't penalise approve_code when all pending entries are legacy format - #30409

Open
AhmetArif0 wants to merge 1 commit into
NousResearch:mainfrom
AhmetArif0:fix/pairing-legacy-lockout
Open

AhmetArif0 wants to merge 1 commit into
NousResearch:mainfrom
AhmetArif0:fix/pairing-legacy-lockout

Conversation

@AhmetArif0

Copy link
Copy Markdown
Contributor

Problem

PR #30383 migrated gateway pairing codes from plaintext storage to salted SHA-256
hashes. The new approve_code() loop correctly skips legacy pending entries
(entries without salt/hash fields) to avoid crashing on in-place upgrades.

However, when matched_key is None at the end of the loop, _record_failed_attempt()
is unconditionally called — even when the only reason for no match is that every
entry in pending is a legacy pre-upgrade entry.

Impact: An admin who calls approve_code during the post-upgrade window (before
the 1-hour TTL prunes old entries) silently accumulates failure counts for calls that
are not real brute-force attempts. After MAX_FAILED_ATTEMPTS (5) such calls the
platform is locked out, blocking all new pairing requests.

Root cause

# gateway/pairing.py — approve_code(), post #30383
if matched_key is None:
    self._record_failed_attempt(platform)  # fires even for legacy-only pending
    return None

Fix

Track whether at least one hash-format entry was examined. Skip
_record_failed_attempt only when pending is non-empty but every entry is
legacy format. Empty pending and real hash-format mismatches still count as
genuine failures, preserving brute-force protection.

any_new_format_checked = False
for entry_id, entry in pending.items():
    if "salt" not in entry or "hash" not in entry:
        continue                        # legacy entry — do not set flag
    any_new_format_checked = True       # real hash entry examined
    ...

if matched_key is None:
    if not pending or any_new_format_checked:
        self._record_failed_attempt(platform)
    return None

Behaviour matrix

Scenario Before After
Legacy-only pending, admin approving ❌ failure recorded → lockout ✅ no failure
Empty pending failure recorded failure recorded (unchanged)
New-format entry, wrong code failure recorded failure recorded (unchanged)
New-format entry, correct code approved approved (unchanged)
Mixed legacy + new-format, match approved approved (unchanged)
Malformed hex salt (new-format) failure recorded failure recorded (unchanged)

Test

Adds test_legacy_only_pending_does_not_trigger_lockout to
TestLegacyPendingFileCompat: calls approve_code MAX_FAILED_ATTEMPTS + 1
times against a legacy-only pending file and asserts no lockout fires, then
confirms a legitimately generated new-format code still approves correctly.

All 41 existing tests pass.

Checklist

  • Root cause identified at exact file/function/line
  • Fix is symmetric with the comment in the existing code ("legacy entries get pruned at TTL")
  • No behavior change for post-upgrade installs with only new-format entries
  • No behavior change for empty pending (brute-force protection intact)
  • Regression test added
  • Full test suite passes

…e legacy

After the hash-storage migration (PR NousResearch#30383), approve_code() skips
legacy pending entries (no salt/hash fields) but still called
_record_failed_attempt() when matched_key was None — even when the
ONLY reason for no match was that every entry in pending was legacy
format from before the upgrade.

An admin who calls approve_code during the post-upgrade window (before
the 1-hour TTL prunes old entries) would accumulate failure counts for
calls that were never real brute-force attempts, triggering a lockout
after MAX_FAILED_ATTEMPTS such calls.

Fix: track any_new_format_checked. Skip failure recording when pending
is non-empty but every entry is legacy (any_new_format_checked is
False). Empty pending and real hash-format mismatches still count as
genuine failures, preserving brute-force protection.

Adds a regression test: MAX_FAILED_ATTEMPTS+1 approve_code calls with
a legacy-only pending file must not trigger lockout.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery area/auth Authentication, OAuth, credential pools labels May 22, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused regression fix. The premise remains valid on current main: gateway/pairing.py:508-512 skips pre-hash entries, while gateway/pairing.py:523-524 still records a failed attempt for every no-match. The proposed condition preserves the lockout path for empty and hash-format pending data.

The legacy fixture on current main has the expected persisted shape at tests/gateway/test_pairing.py:229-242, and the PR adds the missing repeated-approval/lockout assertion. gh pr view 30409 reports no discussion comments and a mergeable PR.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants