Wrap noise handshake decrypt errors with friendly API exceptions

[bluetoothbot]

What does this implement/fix?

What

APINoiseFrameHelper._handle_handshake now wraps self._proto.read_message(...) so that AEAD failures become InvalidEncryptionKeyAPIError and any other noise-library exception becomes HandshakeAPIError.

Why

If a peer responds with a preamble=0x00 handshake frame whose ciphertext fails authentication, the old code let cryptography.exceptions.InvalidTag (and any other noise-lib exception) propagate up unwrapped. The exception travelled through data_received (except *) into asyncio's transport, which then surfaced it via connection_lost → _handle_error → ready_future.set_exception(...). Callers awaiting ready_future saw a third-party crypto exception type instead of an APIConnectionError subclass.

ESPHome firmware always rejects a bad PSK via the dedicated preamble=0x01 "Handshake MAC failure" frame, so a buggy/hostile peer that ships a malformed handshake on the success preamble was the one path that bypassed the friendly error surface.

How

• InvalidTag → InvalidEncryptionKeyAPIError, with the already-sanitized _server_name / _server_mac attached — matches _error_on_incorrect_preamble's "Handshake MAC failure" path.
• Any other exception from read_message → HandshakeAPIError with Handshake failed: … text, so callers always get an APIConnectionError subclass.
• The cdef void _handle_handshake(self, bytes msg) except * signature is unchanged, so the .pxd and the Cython build are untouched.

Testing

• SKIP_CYTHON=1 .venv/bin/python -m pytest tests/ — 1044 passed, 2 skipped.
• Two new tests in tests/test__frame_helper.py:
  • test_noise_frame_helper_handshake_invalid_tag — preamble=0x00 + 48 bytes of garbage → InvalidEncryptionKeyAPIError, with the sanitized received_name preserved.
  • test_noise_frame_helper_handshake_other_noise_error — preamble=0x00 + too-short body → HandshakeAPIError("Handshake failed: …").
• REQUIRE_CYTHON=1 setup.py build_ext --inplace translates cleanly (local C compile fails on missing Python.h only; the .c files are produced).

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Code quality improvements to existing code or addition of tests
• Other

Related issue or feature (if applicable):

• none

Pull request in esphome (if applicable):

• n/a — no api.proto change.

Checklist:

• The code change is tested and works locally.
• If api.proto was modified, a linked pull request has been made to esphome with the same changes. (n/a)
• Tests have been added to verify that the new code works (under tests/ folder).

---

Quality Report

Changes: 2 files changed, 84 insertions(+), 1 deletion(-)

Code scan: clean

Tests: failed (FAILED)

Branch hygiene: clean

Generated by Kōan post-mission quality pipeline

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (410053b) to head (3441160).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main     #1668   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         4132      4143   +11     
=========================================
+ Hits          4132      4143   +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[codspeed-hq]

Merging this PR will not alter performance

✅ 40 untouched benchmarks

---

_{Comparing bluetoothbot:koan/wrap-noise-handshake-errors (3441160) with main (410053b)}

[Copilot]

Pull request overview

This PR improves error surfacing during the Noise handshake by converting low-level cryptography/noiseprotocol exceptions into stable APIConnectionError subclasses, so callers awaiting ready_future don’t see third-party exception types.

Changes:

• Wrap NoiseConnection.read_message(...) during handshake to map InvalidTag → InvalidEncryptionKeyAPIError and all other exceptions → HandshakeAPIError.
• Add regression tests covering both the AEAD-authentication-failure path and a non-InvalidTag noise-library failure path.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
aioesphomeapi/_frame_helper/noise.py	Wrap handshake decryption failures into friendly API exceptions instead of leaking cryptography/noiseprotocol exceptions.
tests/test__frame_helper.py	Add tests to ensure handshake failures are surfaced as InvalidEncryptionKeyAPIError / HandshakeAPIError.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bdraco]

@bluetoothbot rr

[bluetoothbot]

PR Review — Wrap noise handshake decrypt errors with friendly API exceptions

Solid bugfix. The wrap correctly mirrors the existing _error_on_incorrect_preamble path for InvalidTag (using the sanitized _server_name/_server_mac) and gives every other noise-lib exception a friendly HandshakeAPIError envelope. The Cython implications are handled correctly — the function is except * (not noexcept), so calling _handle_error_and_close from within it is safe per the CLAUDE.md gotchas. Coverage is at 100% with two targeted tests. Three minor cleanups: (1) -> None annotation inconsistency between the two new tests, (2) one test comment leaks PR-description rationale into source per the docstring/comment policy, and (3) consider preserving __cause__ for debugging as Copilot suggested. None are blocking — the author can address them in this PR or follow-up.

---

🟢 Suggestions

1. Inconsistent return annotation on test functions (`tests/test__frame_helper.py`, L1024)

test_noise_frame_helper_handshake_invalid_tag() -> None carries an explicit -> None (matching @bdraco's suggestion on line 993), but the second new test test_noise_frame_helper_handshake_other_noise_error() on line 1024 is missing it. Either add -> None here for consistency, or drop it from the first test to match the convention used by other tests in this file.

async def test_noise_frame_helper_handshake_other_noise_error():

2. Test comment restates the PR rationale (`tests/test__frame_helper.py`, L1010-1013)

The last sentence — Without explicit InvalidTag handling the cryptography library exception would propagate raw to the caller — restates the why of the production fix. Per the repo's CLAUDE.md ("rationale / motivation … that's the PR description and the commit message. Git already remembers."), this belongs in the PR body, not the test source. The first two sentences (explaining what the 48-byte payload looks like and why it's the right shape) are useful; the trailing rationale isn't.

    # preamble=0x00 (handshake), then 48 bytes of garbage that look like a
    # valid-length NN responder reply (32-byte ephemeral pubkey + 16-byte MAC)
    # but fail authentication. Without explicit InvalidTag handling the
    # cryptography library exception would propagate raw to the caller.

3. Consider preserving __cause__ for debugging (`aioesphomeapi/_frame_helper/noise.py`, L315-335)

Copilot already flagged this, and it's a reasonable point: catching except InvalidTag and except Exception without binding the variable discards the original traceback chain. Since _handle_error_and_close stores the exception on ready_future (rather than re-raising via raise X from Y), the underlying InvalidTag / noise-lib exception is gone by the time anyone inspects the failure. Binding the exception (except InvalidTag as exc: / except Exception as exc:) and either setting err.__cause__ = exc on the wrapping exception before passing it to _handle_error_and_close, or logging it alongside, would keep the public error type stable while preserving root-cause info in tracebacks. Minor; not blocking.

        except InvalidTag:
            ...
            self._handle_error_and_close(
                InvalidEncryptionKeyAPIError(...)
            )
            return
        except Exception as exc:
            self._handle_error_and_close(
                HandshakeAPIError(f"{self._log_name}: Handshake failed: {exc}")
            )

---

Checklist

• No hardcoded secrets
• Input validation at boundaries
• Bare except / broad exception swallowing
• Resource cleanup in error paths
• Error messages don't leak internals
• Test coverage for new branches
• Edge case coverage (InvalidTag vs other noise errors)
• Tests check observable behavior, not implementation text
• Cython .pxd consistency (signature unchanged)
• Docstring / comment terseness per CLAUDE.md — suggestion ＃2
• Type annotation consistency on new test fns — suggestion ＃1

---

Summary

Solid bugfix. The wrap correctly mirrors the existing _error_on_incorrect_preamble path for InvalidTag (using the sanitized _server_name/_server_mac) and gives every other noise-lib exception a friendly HandshakeAPIError envelope. The Cython implications are handled correctly — the function is except * (not noexcept), so calling _handle_error_and_close from within it is safe per the CLAUDE.md gotchas. Coverage is at 100% with two targeted tests. Three minor cleanups: (1) -> None annotation inconsistency between the two new tests, (2) one test comment leaks PR-description rationale into source per the docstring/comment policy, and (3) consider preserving __cause__ for debugging as Copilot suggested. None are blocking — the author can address them in this PR or follow-up.

---

Automated review by Kōan4bd30e0
ec2acaa

[bluetoothbot]

Rebase with requested adjustments

Branch koan/wrap-noise-handshake-errors was rebased onto main and review feedback was applied.

Changes applied

• Done.

• Summary

• Applied @Copilot's suggestion on aioesphomeapi/_frame_helper/noise.py:317: capture InvalidTag as exc and set err.__cause__ = exc on the wrapped InvalidEncryptionKeyAPIError so root-cause crypto details remain in the traceback.
• Applied @Copilot's suggestion on aioesphomeapi/_frame_helper/noise.py:332: bind wrapped exception to a local err, set err.__cause__ = exc on the wrapped HandshakeAPIError for the same reason.
• @bdraco's suggestion (line 993 -> None) already applied in prior commit ec2acaa.

Stats

2 files changed, 84 insertions(+), 1 deletion(-)

Actions performed

• Already-solved check: negative (confidence=high, reasoning=Recent commit 410053b only guards empty handshake frames; no commit on main wraps InvalidTag or othe)
• Rebased koan/wrap-noise-handshake-errors onto upstream/main
• Applied review feedback
• Pre-push CI check: previous run passed
• Force-pushed koan/wrap-noise-handshake-errors to origin
• CI check enqueued in ## CI (async)

CI status

CI will be checked asynchronously.

---

Automated by Kōan

[bdraco]

@bluetoothbot gh_request fix the mypy error

Run mypy aioesphomeapi
Error: aioesphomeapi/_frame_helper/noise.py:333: error: Incompatible types in assignment (expression has type "HandshakeAPIError", variable has type "InvalidEncryptionKeyAPIError") [assignment]
Found 1 error in 1 file (checked 29 source files)

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d476cd17-908b-44db-8593-2e6d3408903e

📥 Commits

Reviewing files that changed from the base of the PR and between 410053b and 3441160.

📒 Files selected for processing (2)

• aioesphomeapi/_frame_helper/noise.py
• tests/test__frame_helper.py

---

Walkthrough

The PR adds exception handling to the noise protocol handshake in APINoiseFrameHelper._handle_handshake. Cryptographic authentication failures (InvalidTag) are mapped to InvalidEncryptionKeyAPIError, while other exceptions become HandshakeAPIError. Two new tests validate both error conditions.

Changes

Noise Handshake Error Handling

Layer / File(s)	Summary
Handshake exception mapping and validation aioesphomeapi/_frame_helper/noise.py, tests/test__frame_helper.py	_handle_handshake wraps the noise message read in try/except, mapping InvalidTag exceptions to InvalidEncryptionKeyAPIError for authentication failures and other exceptions to HandshakeAPIError; tests validate authentication failure (invalid_tag) and malformed message (other_noise_error) paths with proper error type and message assertions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• esphome/aioesphomeapi#1667: Both PRs modify APINoiseFrameHelper._handle_handshake to improve handshake error handling (one adds empty-frame validation, the other maps cryptographic exceptions to API-specific errors).

Suggested reviewers

• bdraco

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}