Skip to content

fix(auth): tolerate full hostnames in --account input#15

Merged
facundofarias merged 1 commit into
mainfrom
fix/auth-login-account-normalization
May 29, 2026
Merged

fix(auth): tolerate full hostnames in --account input#15
facundofarias merged 1 commit into
mainfrom
fix/auth-login-account-normalization

Conversation

@facundofarias
Copy link
Copy Markdown
Contributor

@facundofarias facundofarias commented May 29, 2026

Summary

Telemetry showed 9 distinct users got a TLS x509: certificate is valid for *.deployhq.com error during dhq auth login because they typed their full account hostname (e.g. uniportal.deployhq.com) at the Account subdomain: prompt. The CLI naively appended .deployhq.com and built a request to https://uniportal.deployhq.com.deployhq.com/projects, which the wildcard cert doesn't cover.

Examples from telemetry (Mixpanel, last 30d):

  • uniportal.deployhq.com.deployhq.com
  • fatchip-gmbh.deployhq.com.deployhq.com
  • recovery-guide.deployhq.com.deployhq.com
  • abdelkarim.deployhq.com.deployhq.com (×2)
  • florianmatthias.deployhq.com.deployhq.com
  • undigital.deployhq.com.deployhq.com
  • jmdentand.deployhq.com.deployhq.com
  • wooltown.deployhq.com.deployhq.com

Changes

  • New normalizeAccount(input, host) helper that strips scheme, paths, and a trailing .<host> suffix.
  • Applied in runAuthLogin after collecting --account (from either flag or prompt) so the stored value is canonical.
  • Defensively strip .deployhq.com in sdk.New so existing users with already-polluted stored accounts get healed on next run without needing to re-login.
  • Mirrored the strip in config.BaseURL for staging/self-hosted hosts.
  • Clarified the interactive prompt text with the same example shown in the flag help.

Test plan

  • go build ./..., go vet ./..., go test ./..., golangci-lint run
  • New table-driven test TestNormalizeAccount covers: bare subdomain, full hostname, https/http schemes, trailing slash, path, query string, surrounding whitespace, hyphenated subdomains, custom staging host (host-suffix only stripped when it matches the configured host).
  • Smoke: dhq auth login --account uniportal.deployhq.com --email ... --api-key ... succeeds.
  • Smoke: dhq auth login --account https://uniportal.deployhq.com/ --email ... --api-key ... succeeds.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved account input handling to automatically trim whitespace, remove URL schemes, paths, and normalize host suffixes during authentication.
  • Updates

    • Account configuration now flexibly accepts multiple input formats including subdomains, full hostnames, and complete URLs.
  • Tests

    • Added comprehensive test coverage for account input normalization across various URL formats and edge cases.

Review Change Stack

Telemetry showed 9 distinct users got a TLS x509 error during
"dhq auth login" because they typed their full account hostname
(e.g. "uniportal.deployhq.com") at the "Account subdomain:" prompt.
The CLI naively appended ".deployhq.com" and built a request to
https://uniportal.deployhq.com.deployhq.com/projects, which is
covered by the *.deployhq.com cert only at depth 1, hence the
verification failure.

- Add a normalizeAccount helper that strips scheme, paths, and a
  trailing ".<host>" suffix, then apply it in runAuthLogin after
  collecting --account (from either flag or prompt) so the stored
  value is canonical.
- Defensively strip ".deployhq.com" in sdk.New so existing users
  who already stored a polluted account get healed on next run
  without needing to re-login.
- Mirror the strip in config.BaseURL for staging/self-hosted hosts.
- Clarify the interactive prompt with the example from the flag
  help so users know to enter the bare subdomain.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 29, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2cf5c62a-642f-4251-aae6-65017f46a35d

📥 Commits

Reviewing files that changed from the base of the PR and between 0387838 and 269f5a8.

📒 Files selected for processing (4)
  • internal/commands/auth.go
  • internal/commands/auth_test.go
  • internal/config/config.go
  • pkg/sdk/client.go

Walkthrough

The PR adds account input normalization across the CLI, config, and SDK layers. A new normalizeAccount helper sanitizes user input by handling URLs, schemes, paths, and host suffixes, converting them to bare subdomains. The interactive auth login applies this normalization, the config URL builder ensures no duplicate host segments, and the SDK client tolerates full hostnames.

Changes

Account input normalization across CLI and SDK

Layer / File(s) Summary
Interactive account normalization and auth login
internal/commands/auth.go, internal/commands/auth_test.go
normalizeAccount helper converts URLs, schemes, paths, whitespace, and full hostnames into subdomains. runAuthLogin applies this to user input. Table-driven tests cover subdomain, URL, custom host, hyphenated, and edge-case scenarios.
Config base URL normalization
internal/config/config.go
BaseURL trims any existing host suffix from the account argument before constructing the API URL, preventing duplicate host segments.
SDK client account normalization
pkg/sdk/client.go
SDK New function trims trailing .deployhq.com from account input, allowing callers to pass full hostnames instead of subdomains only.

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding tolerance for full hostnames in the --account input, which directly addresses the core issue described in the PR objectives.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/auth-login-account-normalization

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

@facundofarias facundofarias merged commit f7b2ba6 into main May 29, 2026
3 checks passed
@facundofarias facundofarias deleted the fix/auth-login-account-normalization branch May 29, 2026 06:46
facundofarias added a commit that referenced this pull request May 29, 2026
…#18)

Telemetry for "dhq hello" (interactive setup) showed 15 internal
errors over 30d that were really auth/user failures dressed up as
CLI bugs:

- 8 were ".deployhq.com.deployhq.com" TLS errors (users typed their
  full hostname at the "Account subdomain:" prompt).
- 4 were 403 (AccessDenied x2 + api_access_restricted x2).
- 3 were 404 Not found on the validate-credentials API call.

The URL-doubling case is already covered defensively in sdk.New
(PR #15). What's left is helloLogin wrapping the validation error in
*output.InternalError unconditionally, which bypasses ClassifyError's
status-code fallback (PR #16) and gives users a generic "internal
error" message instead of an actionable hint.

Changes:
- Clarify the interactive prompt to "Account subdomain (e.g.
  'mycompany' for mycompany.deployhq.com):" — same example shown in
  the --account flag help. Lifts the hint into the moment it's needed.
- Extract classifyHelloValidateErr that maps the validation API error
  to AuthError (401, 403) or UserError (404) or NetworkError, and
  returns nil for everything else so the raw error passes through.
  ClassifyError can then handle 5xx via its status-code fallback once
  PR #16 lands; if it doesn't, behavior matches main today.
- Uses errors.As so the change is self-contained and doesn't depend
  on sdk.IsForbidden (added in PR #16).

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

1 participant