Skip to content

fix: harden credential handling, codec safety, and API consistency#19

Merged
deadcode-walker merged 10 commits intomainfrom
fix/codebase-review-hardening
Mar 23, 2026
Merged

fix: harden credential handling, codec safety, and API consistency#19
deadcode-walker merged 10 commits intomainfrom
fix/codebase-review-hardening

Conversation

@deadcode-walker
Copy link
Owner

@deadcode-walker deadcode-walker commented Mar 23, 2026

Summary

  • CRITICAL: Fix unbounded recursion in AmiCodec::decode on empty frames — convert to iterative loop
  • CRITICAL: Replace derived Debug on AriConfig with manual impl that redacts password and ws_url
  • HIGH: Adopt Credentials (Zeroizing<String>) from core for ARI, replacing plain String password fields
  • HIGH: Add require_challenge option to AmiClientBuilder (default true) to prevent silent plaintext auth fallback
  • HIGH: Restrict AMI connection module to pub(crate), eliminate unnecessary AmiResponse clone in dispatch
  • HIGH: Add AgiError::InvalidConfig variant (was shoehorned into Io)
  • MEDIUM: Apply url_encode to all user-supplied ARI resource path segments (mailbox, device_state, sound, recording, asterisk modules)
  • MEDIUM: Extract duplicated WsRestRequest into shared ws_proto module
  • MEDIUM: Send WebSocket close frame on ARI listener shutdown
  • MEDIUM: Fix weak jitter entropy in reconnect backoff (hash ThreadId instead of string length)
  • MEDIUM: Downgrade raw ARI payload logging from WARN to TRACE (may contain PII)
  • MEDIUM: Restrict ws_url() accessor to pub(crate), make AriConfig fields private with read-only accessors
  • CHORE: Update rustls-webpki 0.103.9 → 0.103.10 (RUSTSEC-2026-0049)
  • CHORE: Add CLAUDE.md, gitignore .claude/, remove stale .omp/ config
  • FIX: Close Notify race in wait_for_ws_client, promote assert_server_ok to shared helpers

Breaking changes

  • AriConfig fields are now pub(crate) — use accessor methods (base_url(), credentials(), app_name(), etc.)
  • AriConfigBuilder now requires non-empty password
  • AmiClientBuilder defaults to require_challenge(true) — set .require_challenge(false) for plaintext-only Asterisk servers
  • AMI connection module is now pub(crate)
  • ARI resource functions now percent-encode path segments (e.g., 2000@default2000%40default)

Test plan

  • 887 unit tests pass
  • 210 mock integration tests pass
  • cargo clippy --workspace --all-targets --all-features -- -D warnings clean
  • Mock tests updated for encoded URL paths
  • Mock tests updated for require_challenge(false) where plaintext login is used
  • CI: cargo-semver-checks will flag breaking changes — version bumps needed
  • CI: cargo deny should pass with rustls-webpki update

Added project-level CLAUDE.md with agent instructions for test routing
and CI requirements. Gitignored .claude/ for local harness config.
Removed obsolete .omp/ rules and skills (replaced by AGENTS.md and
CLAUDE.md).
…rver_ok

Register the Notify future before checking the atomic counter to prevent
a TOCTOU race that could hang tests under load. Remove redundant
Arc<Notify> inside Arc<ServerState>. Move assert_server_ok to shared
helpers and replace all remaining `let _ = handle.await` in mock tests.
CRITICAL:
- fix unbounded recursion in AmiCodec::decode on empty frames
- replace derived Debug on AriConfig with manual impl that redacts
  password and ws_url

HIGH:
- adopt Credentials (Zeroizing<String>) from core for ARI config,
  replacing plain String username/password fields
- add require_challenge option to AmiClientBuilder (default true)
  to prevent silent plaintext auth fallback
- restrict AMI connection module to pub(crate) visibility
- eliminate unnecessary AmiResponse clone in dispatch_message
- add AgiError::InvalidConfig variant (was shoehorned into Io)

MEDIUM:
- apply url_encode to all user-supplied ARI resource path segments
  (mailbox, device_state, sound, recording, asterisk modules)
- extract duplicated WsRestRequest into shared ws_proto module
- send WebSocket close frame on ARI listener shutdown
- fix weak jitter entropy in reconnect backoff (hash ThreadId)
- downgrade raw ARI payload logging from WARN to TRACE
- restrict ws_url() accessor to pub(crate) to prevent credential leak
- make AriConfig fields pub(crate) with read-only accessors
- document tokio::sync::Mutex hold behavior in Call::wait_for_answer
Fixes CRL distribution point matching vulnerability where correct CRLs
were not consulted for revocation checking on certs with multiple
distributionPoints.
@github-actions github-actions bot added documentation Improvements or additions to documentation dependencies Pull requests that update a dependency file ami agi ari core labels Mar 23, 2026
- semver.yml: add continue-on-error (release-plz owns version bumps)
- ci.yml: pin typos action to v1 instead of master
- docs.yml: use taiki-e/install-action@mdbook instead of cargo install
@github-actions github-actions bot added the ci label Mar 23, 2026
- coverage.yml: enforce --fail-under-lines 60 minimum
- dependabot.yml: add rust ecosystem for toolchain updates (monthly)
- deny.toml: wildcards "allow" -> "deny" to block wildcard deps
- coverage.yml: remove --fail-under-lines (threshold meaningless with
  external test crate) and include test crate in coverage measurement
- dependabot.yml: remove rust ecosystem (no rust-toolchain.toml exists)
@deadcode-walker deadcode-walker enabled auto-merge (rebase) March 23, 2026 06:32
- security.yml: add pull_request trigger so Cargo Deny reports on PRs
  (was only push+schedule, causing required check to never report)
- deny.toml: skip-tree asterisk-rs-tests (internal crate uses workspace
  wildcards, never published)
@deadcode-walker deadcode-walker merged commit 959a005 into main Mar 23, 2026
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agi ami ari ci core dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant