[CFX-7263] Fail on bad env credentials instead of using the stored profile - #751
Conversation
… silently using the stored profile Why: - EnsureAuthenticated verified DATAROBOT_ENDPOINT/DATAROBOT_API_TOKEN but on failure fell back to the stored profile with no output when the stored token was valid. a script passing a project's .env creds got the caller's own instance with exit 0, so dr llm-gateway list returned the wrong catalog. - dr auth check and dr auth export already treat a complete env pair as authoritative. EnsureAuthenticated was the one path that substituted silently. Changes: - a complete env pair that fails verification now fails the command before the stored-profile check. classified reason (timeout, malformed endpoint, invalid token) goes to stderr plus a line naming both the requested endpoint and the stored profile that was not used. - extracted the classification into ReportEnvCredentialsError, shared with dr auth check (drops its duplicated block). - partial or absent env credentials keep the existing fallback and login flow. - regression tests: env-invalid with stored-valid must return false and must not start the login flow; malformed quoted endpoint variant; classifier and refusal-message unit tests.
…rror Why: - a transport failure (connection refused, DNS, TLS) fell through to the "DATAROBOT_API_TOKEN is invalid or expired, unset it" message. harmful advice when the token was never judged because the instance was never reached. - failure messages always named DATAROBOT_ENDPOINT even when the endpoint came from the SDK-style DATAROBOT_API_ENDPOINT fallback var, telling the user to fix a variable that is unset. - the tui styles probe stdout for color support, so styled stderr output carried ANSI codes when stderr was redirected while stdout stayed a TTY. Changes: - classify *url.Error from VerifyToken as "Could not connect to <host>" before the invalid-token fallthrough. a rejected token is a plain non-200 error, so the fallthrough now only fires when the instance actually answered. - EnvCredentials carries EndpointVar (which env var supplied the endpoint); the malformed-endpoint and could-not-connect messages name it. - writerStyles binds BaseTextStyle/InfoStyle to the destination writer's renderer in the three writer-taking message funcs. - tests: unreachable-endpoint classification (unit + end-to-end via a closed port, proving VerifyToken transport errors are *url.Error), DATAROBOT_API_ENDPOINT naming.
|
🎫 Jira: |
…n the precedence step, drop a double space
…ot could-not-connect Cursor flagged the scheme-less case: ValidateEndpoint forgives what SchemeHostOnly can clean up while VerifyToken dials the raw value, so the transport branch printed a normalized https host that was never requested. Fixed the whole class, not just that case: a missing scheme and a raw url.Parse failure (Op parse, e.g. leading whitespace) both classify as an invalid endpoint now, so the could-not-connect branch only ever names a host that was actually dialed. Two classifier tests added.
…rified token is expired
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 118d6cd. Configure here.
Code OwnershipCli Maintainers
Review requested from the teams above. Labels will be removed automatically upon approval. |
| // VerifyToken dials the raw endpoint value, while ValidateEndpoint above | ||
| // forgives what SchemeHostOnly can clean up (a bare host without a | ||
| // scheme, stray whitespace). Catch those as endpoint problems here so the | ||
| // transport branch below only ever names a host that was actually dialed. | ||
| var urlErr *url.Error |
There was a problem hiding this comment.
Sorry I'm a little confused by this comment existing here.
Also what does 'dials' here mean in "dials the raw endpoint value"?
c-h-russell-walker
left a comment
There was a problem hiding this comment.
This looks great 🚀
Made one comment where I didn't really get the context/placement of a comment and some wording - but no blocking merge.
ajalon1
left a comment
There was a problem hiding this comment.
Concerned how this interacts with dr workload, please hold
Manual CLI interaction testing — CFX-7263 (PR #751) with PR #750 locally merged
Tested commit: Summary verdictAll 9 exercised rows PASS. The two changes compose correctly: a stale Per-row results
Manual CLI test output$ # R13 — .env missing core vars, env has them (--skip-auth isolates #750 skip logic)
$ dr dotenv setup --if-needed --skip-auth
EXIT: 1
# stderr: WARN ... --skip-auth ...; INFO Parsing prompts ...; INFO Adding variable INFRA_ENABLE_LLM ...
# stderr: Error: could not open a new TTY: open /dev/tty: device not configured
# stdout: (no "Configuration already exists, skipping setup." message)
# -> did NOT skip; wizard launch attempted (failed only because the harness has no TTY)
$ # R14 — .env has all core vars (dummy)
$ dr dotenv setup --if-needed --skip-auth
EXIT: 0
# stdout: Configuration already exists, skipping setup.
# -> skipped
$ # R15 — cross-PR interaction: complete bad env pair via llm-gateway list
$ DATAROBOT_ENDPOINT=http://127.0.0.1:1/api/v2 DATAROBOT_API_TOKEN=expired dr llm-gateway list
EXIT: 1
# stderr:
# ❌ Could not connect to http://127.0.0.1:1: dial tcp 127.0.0.1:1: connect: connection refused
# Check DATAROBOT_ENDPOINT and your network, then try again.
# Error: Authentication failed.
# -> fails loudly, no silent profile fallback
$ # R16 — dr auth check in repo with .env dummy creds + complete bad env pair
$ DATAROBOT_ENDPOINT=http://127.0.0.1:1/api/v2 DATAROBOT_API_TOKEN=expired dr auth check
EXIT: 1
# stdout:
# ❌ DATAROBOT_API_TOKEN in '.env' is invalid or expired.
# Run dr dotenv update to refresh credentials.
# ❌ Could not connect to http://127.0.0.1:1: dial tcp 127.0.0.1:1: connect: connection refused
# Check DATAROBOT_ENDPOINT and your network, then try again.
# -> .env creds checked first (verifyDotenvToken), then CLI creds (ReportEnvCredentialsError)
$ # R2 — complete bad pair (401 => invalid token) with a stored profile present
$ DATAROBOT_ENDPOINT=http://127.0.0.1:8765/api/v2 DATAROBOT_API_TOKEN=expired dr llm-gateway list
EXIT: 1
# stderr:
# ❌ DATAROBOT_API_TOKEN environment variable is invalid or expired.
# Unset it and try again:
# unset DATAROBOT_API_TOKEN (or Remove-Item Env:\DATAROBOT_API_TOKEN on Windows)
# Environment credentials for http://127.0.0.1:8765 failed to verify; not falling back to the stored profile for https://app.datarobot.com.
# Error: Authentication failed.
$ # R4 — unreachable endpoint
$ DATAROBOT_ENDPOINT=http://127.0.0.1:1/api/v2 DATAROBOT_API_TOKEN=expired dr llm-gateway list
EXIT: 1
# stderr:
# ❌ Could not connect to http://127.0.0.1:1: dial tcp 127.0.0.1:1: connect: connection refused
# Check DATAROBOT_ENDPOINT and your network, then try again.
# Error: Authentication failed.
# (no "unset DATAROBOT_API_TOKEN" advice)
$ # R10 — DATAROBOT_API_ENDPOINT (not DATAROBOT_ENDPOINT) + token, bad
$ DATAROBOT_API_ENDPOINT=http://127.0.0.1:1/api/v2 DATAROBOT_API_TOKEN=expired dr llm-gateway list
EXIT: 1
# stderr:
# ❌ Could not connect to http://127.0.0.1:1: dial tcp 127.0.0.1:1: connect: connection refused
# Check DATAROBOT_API_ENDPOINT and your network, then try again.
# Error: Authentication failed.
$ # R11 — stderr redirected to a file; confirm no ANSI escape codes
$ dr llm-gateway list 2>err.log # (bad env pair)
EXIT: 1
# err.log (cat -v): M-bM-^]M-^L Could not connect to ... (the M-… bytes are the ❌ emoji, not ANSI)
# ANSI escape count in err.log: 0
$ # R12 — --output-format json with a bad env pair
$ dr llm-gateway list --output-format json # (bad env pair)
EXIT: 1
# stdout: 0 bytes (empty)
# stderr:
# ❌ Could not connect to http://127.0.0.1:1: dial tcp 127.0.0.1:1: connect: connection refused
# Check DATAROBOT_ENDPOINT and your network, then try again.
# Error: Authentication failed.Unit testsTargeted packages covering both changes, with $ go test -race -count=1 ./internal/auth/... ./cmd/auth/... ./cmd/dotenv/... ./internal/envbuilder/...
ok github.com/datarobot/cli/internal/auth 1.580s
? github.com/datarobot/cli/cmd/auth [no test files]
ok github.com/datarobot/cli/cmd/auth/check 1.326s
ok github.com/datarobot/cli/cmd/auth/export 1.206s
? github.com/datarobot/cli/cmd/auth/login [no test files]
? github.com/datarobot/cli/cmd/auth/logout [no test files]
? github.com/datarobot/cli/cmd/auth/seturl [no test files]
ok github.com/datarobot/cli/cmd/dotenv 15.373s
ok github.com/datarobot/cli/internal/envbuilder 1.598sAll packages pass (race detector enabled, no failures). LintPer the task brief, Notes / caveats
|
|
What my 🤖 was trying to say is that ... I was concerned that #750 would conflict with #751, but they seem to handle different layers. The interaction-test droid I kicked off checked a bunch of scenarios, so I'm fine with merging both in now. You should merge this one, I'll then merge #750 after dealing w. review comments, and then we'll revisit #616. One nit below, everything from here on out is informational and non-blocking. N2 —
|
|
🚀 Smoke tests triggered! Running on Linux and Windows... |
|
✅ All smoke tests passed! ✅ Linux: success |
…nt in plain language
…per Go error-string convention
…sable ones Both bots landed on the same two gaps. The fixes overlap in auth.go, so they share a commit. Userinfo leak (cursor, copilot): - VerifyToken redacted the URL it stored on the error, but the messages printed the caller's endpoint raw, so http://user:pw@host/api/v2 put the password on screen for a 404, a 5xx, a transport failure, and a timeout. - Every path that prints an endpoint now goes through hostOrEndpoint, which redacts. That also closes the timeout and stored-profile-not-used lines, which leaked the same way before this branch. Unusable endpoints read as transport failures (cursor, copilot): - A bare host in '.env' reported "Could not connect to https://app.example.com", naming the URL SchemeHostOnly invented rather than the one requested. The stored profile called ftp:// a connection failure, which is the exact wrong-blame this ticket removes from the env pair. - ValidateEndpoint now rejects a scheme-less endpoint too, and reportUnjudged runs it before treating a failure as transport. The env leg's own missing-scheme guard is gone, since ValidateEndpoint returns the same text. Verified against a local server: the env leg's wording for a bare host, a quoted URL, whitespace, and ftp:// is unchanged from #751.
EnsureAuthenticated runs in PreRunE, so the stored-profile diagnostic I added last commit was writing to the command's own stdout. dr llm list --output-format json emitted 176 bytes of prose ahead of nothing, breaking the zero-byte-stdout contract the env-credential path has held since #751. Changes: - ReportUnjudged writes to os.Stderr, matching ReportEnvCredentialsError. - A test captures both streams and asserts stdout stays empty. - setupTestEnvironment clears the env pair. Without it the developer's own DATAROBOT_ENDPOINT and DATAROBOT_API_TOKEN reached EnsureAuthenticated and it verified against their real instance, so the new test passed only because another test had leaked a dead endpoint into the process env.
…problem A stored or '.env' endpoint with leading whitespace passes ValidateEndpoint, which trims, then fails VerifyToken's raw url.Parse. ReportUnjudged read that as transport and printed "Could not connect to <host>: first path segment in URL cannot contain colon" with a check-your-network line. The env-var path has named it an invalid endpoint since #751. One rule instead of a second special case: unusableEndpoint reports either a ValidateEndpoint failure or a parse-op url.Error, and ReportUnjudged prints whichever it gets.
Summary
drhonoredDATAROBOT_ENDPOINT/DATAROBOT_API_TOKENonly once they verified. On failure it silently fell back to the stored CLI profile, so a script passing a project's.envcredentials ran against the caller's own instance with exit 0.dr llm-gateway listlisted the wrong catalog. A complete env pair that fails verification now fails the command, with the reason on stderr and both endpoints named.dr auth checkanddr auth exportalready treat env credentials as authoritative.Notes for review
.envin a template checkout used to self-heal:taskloads.envinto the environment, anddr dotenv updatequietly ran on the stored profile. Those runs now fail with the fix named. pulumi and the SDKs already fail on the same stale credentials.Output
Before this change both commands printed the caller's own catalog and exited 0.
Technical Changes
EnsureAuthenticatedfails before the stored-profile check when a complete env pair fails verification. Classification extracted toReportEnvCredentialsErrorwith a new transport-error branch.EnvCredentials.EndpointVarnamesDATAROBOT_API_ENDPOINTwhen that fallback supplied the endpoint. Styles bind to the destination writer so redirected stderr gets no ANSI codes.Note
Medium Risk
Changes global authentication precedence for every command using
EnsureAuthenticated; workflows that relied on silent fallback todrconfig.yamlwill now fail (intentional), but partial env vars still behave as before.Overview
Fixes silent wrong-instance auth: when both
DATAROBOT_ENDPOINT(orDATAROBOT_API_ENDPOINT) andDATAROBOT_API_TOKENare set but verification fails, commands now fail instead of using the stored CLI profile or starting login—so scripts with a stale project.envno longer hit the wrong DataRobot instance with exit 0.Shared env error reporting:
ReportEnvCredentialsErrorcentralizes classification (timeout, malformed endpoint, missing scheme, transport/DNS, invalid token) and is used fromEnsureAuthenticated(stderr) anddr auth check. Transport failures no longer suggest unsetting the token.reportStoredProfileNotUsednames the requested vs stored endpoints when a profile exists.Other:
EnvCredentials.EndpointVardrives error text for the SDK fallback var; lipgloss styles bind to the destination writer for redirected stderr/tests. Docs describe env-first precedence and the unreachable-endpoint example. Regression tests cover env-invalid + stored-valid, unreachable endpoint, and error message branches.Reviewed by Cursor Bugbot for commit 118d6cd. Configure here.