fix(sfdx): retrieve org credentials after SF CLI 2.136+ redaction @W-22684438 - #3998
Draft
jstvz wants to merge 12 commits into
Draft
fix(sfdx): retrieve org credentials after SF CLI 2.136+ redaction @W-22684438#3998jstvz wants to merge 12 commits into
jstvz wants to merge 12 commits into
Conversation
Adds private helper that calls sf org auth show-access-token for use as a fallback when sf org display redacts the token (SF CLI 2.136+). Refs #3987
Adds private helper that calls sf org auth show-user-password for use as a fallback when sf org display redacts the password (SF CLI 2.136+). Returns None on failure or absent password; never raises. Refs #3987
sfdx_info now uses result.id directly for org_id (no more token-prefix split) and falls back to sf org auth show-access-token / show-user-password when sf org display redacts those fields. Cache shape and 1-hour TTL preserved. ScratchOrgConfig.scratch_info inherits the fix via the existing alias. Fixes #3987
When sf org display does not return accessToken (CLI 2.136+ redaction), fall back to sf org auth show-access-token. Legacy CLI path unchanged. Refs #3987
Pre-commit black hook on the prior task commits was skipped due to a local Python 3.14 / black 22.3.0 incompatibility. Run black manually to keep the test file aligned with the project format. Refs #3987
After sfdx_info requires result.id from `sf org display --json`, the test_org_import fixture must include the id field. The two sibling fixtures (test_org_import__persistent_org, test_org_import__trial_org) already had it.
Code-reviewer pass on the original fix found two BLOCKING defects:
B0 (correctness): the CLI replaces redacted secrets with a truthy
sentinel string prefixed "[REDACTED] ", not absent/null. A
`if not value:` check never triggers the fallback. Three sites
affected: sfdx_info access_token, sfdx_info password,
get_access_token.
B1 (security): every `--target-org={username}` interpolation runs
through sfdx() which uses shell=True. Usernames with shell
metacharacters are an injection footgun.
These tests RED against the current fix; they pin the corrected
detection model and shell_quote contract from the round-3 spec.
- test_sfdx_info_sentinel_token_triggers_fallback: org display
returns redacted sentinel as accessToken; expects the helper to
fall back and return the real token.
- test_fetch_user_password_returns_none_on_sentinel: defensive
guard if sf org auth show-user-password itself ever returns a
sentinel value.
- test_get_access_token_sentinel_triggers_fallback: per-user
lookup path must trigger fallback on sentinel, not just absence.
- test_fetch_access_token_shell_quotes_username: usernames with
shell metacharacters (e.g. semicolons) must be shell_quote'd in
the rendered command string.
Pre-commit black 22.3.0 is incompatible with the local Python 3.14
build (ast.Str removed in 3.12); SKIP=black follows the precedent
set by 71b8e13 on this same branch. The file was reformatted with
uvx black==24.10.0 before commit so CI's modern black stays clean.
The green fix lands in the next commit.
Fixes the two BLOCKING defects from code review of #3987: B0 (correctness): SF CLI 2.136+ replaces redacted secrets with the truthy sentinel "[REDACTED] Use 'sf org auth show-*' to view", not absent/null. The original `if not value:` checks never fired the fallback. Add `_is_redacted()` keying on the "[REDACTED] " prefix and route all three sites (sfdx_info access_token, sfdx_info password, get_access_token) through it. B1 (security): every `--target-org={username}` interpolation runs through sfdx() which uses shell=True. Wrap username in shell_quote() at the three call sites: get_access_token's own `sf org display` line, _fetch_access_token, _fetch_user_password. I1 (behavior): gate the password fallback subprocess on `fallback_fired AND _is_redacted(password)`. Legacy passwordless orgs (`password` absent on old CLI) no longer trigger a pointless extra subprocess; they short-circuit to "no password". Defensive: _fetch_access_token raises and _fetch_user_password returns None when `show-*` itself ever returns a sentinel value. Indicates a future CLI change; better to fail loud than hand back a placeholder. N1 (test hygiene): rename the test fixture org id from the typo'd "OODxxxxxxxxxxxx" (letter O O D) to "00Dxxxxxxxxxxxx" (zero zero D), matching real Salesforce org id shape across 17 call sites in cumulusci/cli/tests/test_org.py. Pre-commit black 22.3.0 is incompatible with local Python 3.14; SKIP=black follows the precedent set by 71b8e13. Files were reformatted with uvx black==24.10.0 first. Closes #3987.
Round 3 review caught that the previous password gate fired one extra subprocess for the most common new-CLI org class. A new CLI returns the token sentinel + OMITS `password` entirely for a passwordless org (web-auth sandbox, scratch org without `force:user:password:generate`). The previous `fallback_fired AND _is_redacted(password)` gate evaluated True/True on every sfdx_info refresh and ran a pointless `sf org auth show-user-password`. Split the helper: - `_is_sentinel(v)` -- strict prefix check, used for the password gate. An absent password is not a sentinel. - `_is_redacted(v)` -- absent OR sentinel, used for the token path. An absent token is as actionable as a redacted one (both mean "ask `sf org auth show-access-token`"). Test coverage: - `test_sfdx_info_new_cli_fetches_token` corrected: assert call_count == 2 (was 3) and password absent. Codifies the passwordless-org-no-fallback contract. - `test_sfdx_info_sentinel_token_triggers_fallback` (round-2) corrected the same way. - `test_sfdx_info_new_cli_fetches_password` rewritten: both fields are now sentinels (the real shape for a scratch org with a generated password on a new CLI), not absence. - New `test_sfdx_info_new_cli_no_password_org`: explicit passwordless-org no-fallback assertion. - New `test_sfdx_info_sentinel_password_uses_fetched`: sfdx_info- level coverage of the both-sentinels path. Was helper-level only. Files reformatted with `uvx ruff format`. Pre-commit black 22.3.0 incompatible with local Python 3.14 (`ast.Str` removed in 3.12); `SKIP=black` follows the precedent set by 71b8e13 + bdc2056. 119/119 tests green in test_config_expensive + test_org.
Address round-4 review comments (no blocking findings; nit polish only). - Add load-bearing trailing-space comment to `_REDACTED_PREFIX`. The CLI sentinel is the literal `"[REDACTED] Use ..."` form; a prefix without the trailing space would also match the unrelated string `"[REDACTED]Use ..."` if Salesforce ever shipped that. - Tighten `_fetch_user_password` docstring: name every failure path (non-zero exit, malformed JSON, sentinel result) and the policy (return None, never raise; debug-log to keep the call diagnosable). - Add debug logs on both `_fetch_user_password` failure branches so transient subprocess / JSON failures surface in `cci --debug` runs without promoting the call to raise. - Drop the `elif not password: password = None` no-op. `result.get()` already returns None for absent keys; the conditional was vestigial from the compound-gate version. - Drop the redundant first assertion in `test_fetch_access_token_shell_quotes_username`. The second assertion is strictly stronger. - Add `Command.call_count == 1` to `test_scratch_info` to codify "legacy CLI: no fallback subprocesses fire" at the test level. - Add `Command.call_count == 3` to `test_sfdx_info_new_cli_fetches_password` to codify the both- sentinels path (org_display + show-access-token + show-user-password). 77/77 tests green in test_config_expensive. `uvx ruff check` and `uvx ruff format --check` clean. SKIP=black per branch precedent (71b8e13 -> bdc2056 -> 7957c49); black 22.3.0 incompatible with local Python 3.14.
Reviewer suggestion. Flag the only quiet failure mode of the sentinel-prefix detection in `_REDACTED_PREFIX`'s docstring: `@salesforce/core` currently hardcodes locale to en_US (per-locale message bundles are an unimplemented TODO upstream), so the "[REDACTED] " literal is stable on every shipping CLI. The literal lives inside the translatable message string, however. If Salesforce later wires up localization, a non-English `redacted.accessToken` could drop the prefix and silently break detection. Note the more robust long-term signal (match sentinel message keys, or "value is not a well-formed access token") near the constant so a future reader sees the upgrade path without spelunking the CLI repo. Comment-only; no behavioral change. 77/77 tests still green. SKIP=black per branch precedent.
Reviewer add-on. The locale-fragility comment names the failure mode but leaves the on-call engineer to infer the symptom. Add one tripwire sentence: if `sf org display --json`'s access token ever round-trips into an API 401 with a human-readable token value, suspect this detection path first. Converts the comment from "here is the theoretical risk" to "here is the symptom that means this assumption broke." Comment-only; no behavioral change.
Contributor
|
@jstvz This is very much needed. But why try and detect if the string is redacted. Could you not simply check the SF CLI version and/or existence of the new commands and, if the commands exist, use them? This would bypass any string format detection issues. |
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
2 participants
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.
Summary
Fixes #3987. Salesforce CLI 2.136+ redacts
accessToken,password, andsfdxAuthUrlfromsf org display --json.SfdxOrgConfigreads those fields, so credential retrieval breaks the moment a user's CLI updates. CumulusCI does not pin the CLI version, so CI and MetaCI builds fail when 2.136+ reaches the stable channel.CLI behavior verified against shipping versions
Confirmed against installed CLI 2.140.6,
@salesforce/plugin-org5.11.8, and@salesforce/core8.31.1. Verified against the upstreammainHEAD of both repos.@salesforce/plugin-org'smessages/secrets-redacted.md:accessToken->[REDACTED] Use 'sf org auth show-access-token' to viewpassword->[REDACTED] Use 'sf org auth show-user-password' to viewsfdxAuthUrl->[REDACTED] Use 'sf org auth show-sfdx-auth-url' to viewSF_TEMP_SHOW_SECRETS(default false redacts). The CLI applies no version, date, or org-type gate. Scratch orgs and persistent orgs behave the same.idis never redacted. Org id is always readable.passwordis present (and then redacted) only when the org has a locally generated password. For passwordless orgs (web-auth sandbox or production, scratch org withoutforce:user:password:generate), the field is absent, not a sentinel. The absent-vs-sentinel distinction drives the password fallback gate.SF_TEMP_SHOW_SECRETS=truere-enables the legacy output. Salesforce removes the env var in summer 2026, so it is a stopgap.Changes
One source file:
cumulusci/core/config/sfdx_org_config.py.[REDACTED]sentinel prefix. Truthiness alone does not fire because the sentinel is a non-empty string.--no-prompt. The--no-promptflag suppresses an interactive confirm that hangs headless processes.sf org auth show-access-tokensf org auth show-user-passwordshow-user-passwordsubprocess. This avoids a pointless extra subprocess on every refresh for the most common new-CLI org class (passwordless).result["id"](raise a clearSfdxOrgExceptionif missing). The previousaccessToken.split("!")[0]derivation no longer works because the token is redacted. Imported-org ids now surface as the 18-character form, consistent with the baseOrgConfig. Salesforce APIs accept both 15- and 18-character ids.sfcommand.sfdx()runs withshell=True, so an unquoted username with shell metacharacters is a command-injection footgun. Applied at all sites.Backward compatibility
org display. The code runs no fallback subprocess.sf org auth show-*subcommands may not exist. The token helper raises a clearSfdxOrgExceptionthat names the attempted commands. The password helper degrades to "no password" and never raises.force_refresh_oauth_tokenis unchanged. It usessf org open -r, which reads no secrets.Test coverage
All tests pass in
test_config_expensive.pyandtest_org.py.New and updated coverage:
show-user-password(subprocess count == 2).Noneon subprocess error, empty output, or sentinel response.Known limitation
The
[REDACTED]prefix is locale-stable on every shipping CLI today.@salesforce/core'sMessages.getLocale()is hardcoded toen_US, and per-locale message bundles are an unimplemented TODO upstream. The prefix lives inside the translatable message string, so if Salesforce later ships localization, a non-Englishredacted.accessTokencould drop the literal[REDACTED]prefix and break detection silently. Detection failure surfaces loudly: the sentinel string becomes the "access token", and the next API call returns 401 with a human-readable token value. The fix is a one-line constant change. A locale-independent signal would cover onlyaccessToken(it has a shape), notpasswordorsfdxAuthUrl, so the alternative would not remove the locale risk from the other two fields. Thesfdx_org_config.pyconstant has a comment that names the failure mode and the tripwire symptom.Test plan
pytest cumulusci/core/config/tests/test_config_expensive.pyand confirm green.pytest cumulusci/cli/tests/test_org.pyand confirm green.cci org info <alias>returns access token and password.cci org info <alias>returns access token, password absent, noshow-user-passwordsubprocess incci --debuglog.Closes #3987.