Skip to content

fix(sip): reject realm names with hyphens/uppercase; add DNS and cache waits to test workflow - #31

Open
michaela-band wants to merge 2 commits into
mainfrom
feat/sip-provisioning
Open

fix(sip): reject realm names with hyphens/uppercase; add DNS and cache waits to test workflow#31
michaela-band wants to merge 2 commits into
mainfrom
feat/sip-provisioning

Conversation

@michaela-band

Copy link
Copy Markdown
Contributor

Summary

  • ValidateRealmName regex fix — the previous regex allowed hyphens and uppercase letters (DNS label rules), but the Bandwidth API rejects both with error 33013 ([a-z0-9] only). Validation now matches the API constraint, so invalid names fail fast with a clear message before any HTTP request is made.
  • Test update — removed TestRealmCreate_IfNotExistsMatchesNameCaseInsensitively (the uppercase-input scenario it covered is no longer reachable) and replaced it with TestRealmCreate_IfNotExistsRejectsUppercaseName, which verifies validation fires before any API call.
  • AGENTS.md workflow fixes — two gaps discovered during live end-to-end testing of the ephemeral-realm SIP call workflow:
    • New realm FQDNs can take 1–2 minutes to propagate in public DNS after the realm reaches ACTIVE status. Added a dig poll loop after realm creation to block until the FQDN resolves.
    • Bandwidth's SIP proxy caches credential hashes; a freshly created credential fails 407 auth for ~15 s even when the digest response is mathematically correct. Added a sleep 15 after credential creation with an explanation.
  • Fixed the example realm name sip-testsiptest (the hyphen would have failed the corrected validation).
  • Updated the baresip output table to distinguish a repeated-407 credential cache miss from a genuine 403 mismatch.

Test plan

  • make test passes (all packages green)
  • go vet ./... clean (golangci-lint not installed locally; CI will cover it)
  • Manually verified the end-to-end call workflow with the fixes applied — call connected with 183 Session Progress after the 15 s credential cache wait

Note: this PR was substantially assisted by Claude Code. Changes have been reviewed and the test behavior was verified against the live Bandwidth API.

🤖 Generated with Claude Code

…e waits

ValidateRealmName previously allowed uppercase letters and internal hyphens
(DNS label rules), but the Bandwidth API rejects both with error 33013 —
supported chars are [a-z0-9] only. The mismatch caused a confusing runtime
failure after CLI validation passed. Updated the regex and error message to
match the actual API constraint.

The AGENTS.md ephemeral-realm SIP test workflow had two related gaps
discovered during live testing:
- New realm FQDNs can take 1–2 minutes to appear in public DNS after the
  realm reaches ACTIVE status. Added a dig poll loop after realm creation.
- Bandwidth's SIP proxy caches credential hashes; a freshly created or
  rotated credential fails authentication for ~15 s. Added a sleep 15 after
  credential creation with an explanation.

Also fixed the example realm name (sip-test → siptest) which would have
failed the now-correct validation, and updated the baresip output table to
distinguish a repeated-407 cache miss from a genuine 403 credential error.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@michaela-band
michaela-band requested review from a team as code owners August 14, 2026 18:22
@bwappsec

bwappsec commented Aug 14, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@kshahbw

kshahbw commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Went through this one carefully and also verified the central claim against the live API rather than taking it on faith. The regex change is correct — I'd land it after a few small fixes.

Verified against prod

Built a binary from main (this branch's own validation blocks the test client-side) and probed a SIP-enabled account:

--name hyphenprobe-x  → Realm hyphenprobe-x is invalid. Supported chars: [a-z0-9] (error 33013)
--name HYPHENPROBEUC  → Realm HYPHENPROBEUC is invalid. Supported chars: [a-z0-9] (error 33013)
--name probe9x        → created, ACTIVE  (control — deleted right after)

The control create is the important one: it proves the 33013s are specific to the name and not a blanket create failure. Both halves of the claim hold, and the new error message matches the API's own wording. Also worth noting the change makes shortName (internal/sip/service.go:142) unambiguous — it splits the FQDN label on the last hyphen, which is only well-defined if names can't contain hyphens. Nice side effect.

Suite is green locally too: go test ./..., go build ./..., go vet ./... all clean, and golangci-lint run ./... reports 0 issues — so the "lint not installed locally" checkbox is genuinely fine.

Three things I'd fix

1. Stale rationale + orphaned EqualFold in cmd/sip/realm_create.go:66-70. The comment still says "ValidateRealmName accepts uppercase", which this PR makes false, and it's justifying a branch whose only test (TestRealmCreate_IfNotExistsMatchesNameCaseInsensitively) this PR deletes. I mutated the EqualFold to a plain != and the whole suite still passes — and doing so made the "strings" import unused, i.e. that comparison is the only use of the package in the file. Suggest dropping to != and deleting the paragraph.

2. The dig poll loop in AGENTS.md never gives up. dig +short on an unresolvable name returns empty with exit 0, so grep -q '.' can never succeed and the until loop spins forever — I ran it verbatim and had to kill it. For a file whose audience is agents, that's a rough failure mode. Worth capping it (~18 tries) and exiting loudly with the FQDN in the message.

3. sip-test-agentsiptestagent isn't needed. The [a-z0-9] rule is realm-name-only; ValidateUsername returns nil for vapi-agent and sip-test-agent, internal/sip/sip_test.go:69 asserts exactly that, and cmd/sip/credential_create.go:43 uses --username vapi-agent in its own help text. The rename implies a constraint that doesn't exist and now contradicts that example.

Two things the live run turned up

The "1–2 minutes to propagate" premise didn't reproduce for me. The new realm's FQDN resolved at t+0, the instant --wait returned ACTIVE, and stayed resolved at 15/30/45s. I checked whether that's a wildcard record making the gate meaningless, and it isn't — bogus labels under the same account suffix return empty while all real realms resolve, so the dig check is a real readiness signal. My guess is negative DNS caching on the resolver you hit, which would produce exactly what you saw and would make the loop genuinely necessary. So keep the loop, but maybe soften the wording to "may take up to a minute or two on some resolvers" — your n=1 and my n=1 disagree, and the doc currently states it as a flat fact.

The workflow silently requires a SIP-enabled account. Running step 2 on my default account fails with exit 4 and "this account isn't enabled for SIP credentials — contact Bandwidth support to enable SipCredentialSettings"; only one of the accounts on my token has SIP. The workflow never mentions this, so an agent following it on the wrong account dies at step 2 with an error that reads like a support ticket rather than "pass --account-id." Since this PR is specifically about that workflow's live-testing gaps, one line about it would fit right in.

Not approving yet just so the fixes get a look, but the substance here is verified correct rather than merely plausible — good catch on the validation mismatch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants