test(sip): M6 review hardening — fail-closed 403 assert, cross-endpoint credential fence, registrar refresh test; drop dead unscoped builders - #156
Merged
Conversation
…ndpoint credential fence, registrar refresh test; drop dead unscoped builders Non-blocking items from the M6 independent reviews: - AUTH MINOR-1: the dangling-auth fail-closed scenario in e2e_per_endpoint_auth accepted 403 OR 401; a 401 challenge inviting a retry is not fail-closed. Tightened to a hard 403 only, matching the sibling phantom-endpoint assertion (the shipped code already emits a genuine 403 — test-intent tightening only). - AUTH MINOR-2: new e2e_cross_endpoint_auth proves a VALID credential for endpoint X presented from endpoint Y's source IP is REJECTED (per-endpoint selection, not a credential union), with a positive control proving the digest itself is valid. Goes RED if selection is reverted to the pre-CP4 all-credentials union. Also a named registrar test that a refresh-before-expiry REGISTER replaces the binding in place with a fresh registered_at (live, TTL restored, no duplicate). - ROUTING MINOR-2: deleted session_ext.rs free fn build_reinvite and build_update_connected_line (+ param-only types ReinviteReason / ConnectedLineInfo). Both built Via/Contact/From from the raw local_addr without external-signaling scoping and have zero workspace callers — the live re-INVITE path is SipSession::build_reinvite, which is scoped and covered.
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
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.
Fast-track test-hardening + dead-code cleanup from the M6 independent reviews (non-blocking items). No production behavior change; the only non-test diff is deletion of uncalled code.
1. AUTH MINOR-1 — fail-closed assert tightened to 403-only
e2e_per_endpoint_auth.rsdangling-auth scenario assertedcodes.contains(&403) || codes.contains(&401). A 401 challenge invites a credential retry — that is not fail-closed. Now asserts a hard 403 only, matching the sibling phantom-endpoint assertion. The shipped code already emits a genuine 403 (event_handler.rs"auth section is unresolvable" arm), so this is test-intent tightening.2. AUTH MINOR-2 — cross-endpoint credential rejection fence + registrar refresh test
e2e_cross_endpoint_auth.rs(new test binary — avoids the process-global pjsip-config race documented in the sibling file): TWO authed endpoints (alpha@ 127.0.0.6,beta@ 127.0.0.7, distinct TEST-only credentials) on one transport.None, wire shows a fresh 401 and never a 200.Holds by construction today via per-endpoint auth selection (M6 CP4) but was unguarded against future refactors.
RED captured — with selection temporarily reverted to the pre-CP4 all-credentials union (
match matched_endpoint_name.as_deref()→match None::<&str>inevent_handler.rs), the cross-endpoint INVITE authenticates and the test fails exactly at the intended assertion (control (a) still passing):registrar.rs::test_refresh_before_expiry_keeps_contact_live: seeds a binding 10s from expiry, refreshes via the realhandle_registerpath, asserts the binding is replaced in place with a freshregistered_at(live, TTL restored, exactly one binding, still routable viabest_contact).RED captured — with
register()temporarily changed to alwayspushinstead of replace-in-place:Both RED patches were reverted; the shipped diff is green.
3. ROUTING MINOR-2 — dead code DELETED (not scoped)
session_ext.rsfree fnbuild_reinviteandbuild_update_connected_linebuilt Via/Contact/From from the rawlocal_addrwith noadvertised_signaling_hostportscoping. Grep/callgraph over the workspace: zero callers — the only references were their own definitions and the param-only typesReinviteReason/ConnectedLineInfo(also deleted); the crate re-export surface (lib.rs) only exportsSessionSupplement/SupplementRegistryfrom this module. The live re-INVITE path isSipSession::build_reinvite(session/mod.rs, called fromevent_handler.rs), which IS external-signaling-scoped and covered. Deleting removes the hazard of someone wiring up an unscoped builder later; module doc updated to point at the scoped path. Unused imports pruned (clippy-D warningsclean).Verification
cargo test --workspace --exclude pjsip-shim→ 4467 passed, 1 ignored (68 suites)cargo clippy --workspace --exclude pjsip-shim -- -D warnings→ cleancargo fmt --allrun