Skip to content

CI gate fixes: externalize Bitwarden secret + fix rust-fmt wrapper#48

Merged
David-Martel merged 2 commits into
mainfrom
security/externalize-bw-credentials-20260619
Jun 20, 2026
Merged

CI gate fixes: externalize Bitwarden secret + fix rust-fmt wrapper#48
David-Martel merged 2 commits into
mainfrom
security/externalize-bw-credentials-20260619

Conversation

@David-Martel

Copy link
Copy Markdown
Owner

Fixes 2 of the pre-existing red CI gates on main.

fix(security)Initialize-BitwardenSession.ps1 hardcoded a real Bitwarden ClientSecret + MasterPassword (tracked → in git history). Replaced with the standard BW_CLIENTID/BW_CLIENTSECRET/BW_PASSWORD env vars. Fixes the Security Scan gate.

⚠️ The old values are still in git history — they must be rotated in Bitwarden and optionally purged from history.

fix(build) — the Rust Format gate failed on a wrapper bug (cargo fmt --all -- --checkInvoke-RustBuild.ps1: parameter name '' is ambiguous), not a real format issue. Switched to cargo fmt --all --check. Verified Build.ps1 -Component format now exits 0.

Still red (out of this PR's scope)

  • PowerShell Lint — 4 pre-existing scripts have syntax errors: Optimize-Disks.ps1 (11), Archive/wsl2-config-analyzer.ps1 (13), Start-WSLDockerServices.ps1 (1), HomeRootArchive/create-priority-repositories.ps1 (1). Need careful repair or archive-exclusion.
  • Clippy (rust-check job) — same -- wrapper bug (-- -D warnings can't just drop --); needs a fix in Invoke-RustBuildCommand.
  • Deploy/rust-functiongemma workspace is structurally broken (runtime not below the workspace root).

🤖 Generated with Claude Code

David-Martel and others added 2 commits June 19, 2026 22:32
Initialize-BitwardenSession.ps1 hardcoded a real ClientSecret + MasterPassword
in CredentialTargets (tracked -> in git history). Replaced the literals with the
standard Bitwarden env vars (BW_CLIENTID / BW_CLIENTSECRET / BW_PASSWORD).
Fixes the CI Security Scan "hardcoded credentials" gate.

NOTE: the old values remain in git history and must be ROTATED in Bitwarden and
(optionally) purged from history separately.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…'t mis-bind it

Invoke-RustInferenceQuality / Invoke-RustQuality passed `cargo fmt --all -- --check`
to Invoke-RustBuild.ps1, whose param binder rejected the standalone `--` token
("parameter name '' is ambiguous"), failing the CI "Rust Format" gate even though
the code is correctly formatted. `cargo fmt --all --check` is equivalent (cargo-fmt
forwards --check to rustfmt) and avoids the bare separator. Verified: Build.ps1
-Component format now exits 0.

NOTE: the clippy/check paths still pass `-- -D warnings` and hit the same latent
wrapper bug (separate CI job); that needs a real fix in Invoke-RustBuildCommand.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 20, 2026 02:38
@David-Martel David-Martel merged commit 06bcc9b into main Jun 20, 2026
@David-Martel David-Martel deleted the security/externalize-bw-credentials-20260619 branch June 20, 2026 02:38

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request modifies Build.ps1 to remove the -- argument separator from cargo fmt commands, and updates Initialize-BitwardenSession.ps1 to use environment variables for Bitwarden credentials. The reviewer noted that replacing the credential target names with environment variables breaks the Windows Credential Manager lookup and risks leaking secrets. Additionally, the reviewer pointed out that the formatting issue in Build.ps1 is a symptom of how arguments are passed to the wrapper script, suggesting a more robust fix using the -- end-of-parameters marker.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +68 to +70
ClientId = $env:BW_CLIENTID
ClientSecret = $env:BW_CLIENTSECRET # pragma: allowlist secret
MasterPassword = $env:BW_PASSWORD # pragma: allowlist secret

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

Replacing the Credential Manager target names with the actual environment variables ($env:BW_CLIENTID, etc.) completely breaks the script's execution.

The Issue

  1. Credential Manager Lookup Failure: Get-BitwardenCredentials loops through $script:Config.CredentialTargets and calls Get-StoredCredentialPassword -Target $target. If the environment variables are set, the script will attempt to query Windows Credential Manager using the actual secrets as target names, which will fail.
  2. Secret Leakage: Passing the actual secrets to cmdkey via $Target leaks them to command-line logging and process lists.
  3. Null Reference: If the environment variables are not set, they evaluate to $null, causing the lookup to fail immediately.

The Solution

To support both environment variables (for CI/automation) and Windows Credential Manager (for local workstations) cleanly:

  1. Revert CredentialTargets to use the generic target name strings.
  2. Modify Invoke-BitwardenSessionInit (around line 561) to check if the environment variables are already populated, and if so, use them directly, bypassing the Credential Manager lookup entirely.

Here is how you can update Invoke-BitwardenSessionInit:

    # Get credentials from environment or Windows Credential Manager
    if ($env:BW_CLIENTID -and $env:BW_CLIENTSECRET -and $env:BW_PASSWORD) {
        $credentials = @{
            ClientId       = $env:BW_CLIENTID
            ClientSecret   = $env:BW_CLIENTSECRET
            MasterPassword = $env:BW_PASSWORD
        }
    } else {
        try {
            $credentials = Get-BitwardenCredentials
        }
        catch {
            Write-Log "Failed to retrieve credentials: $_" -Level ERROR
            return $false
        }
    }
        ClientId       = 'bitwarden/client_id'
        ClientSecret   = 'bitwarden/client_secret'
        MasterPassword = 'bitwarden/master_password'

Comment thread Build.ps1
'clippy' { @('clippy', '--workspace', '--all-targets', '--', '-D', 'warnings') }
'fmt' {
if ($WriteMode) { @('fmt', '--all') } else { @('fmt', '--all', '--', '--check') }
if ($WriteMode) { @('fmt', '--all') } else { @('fmt', '--all', '--check') }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While removing -- from the fmt command works because cargo fmt natively supports --check, the root cause of the parameter name '' is ambiguous error is how Invoke-RustBuildCommand invokes the wrapper script Invoke-RustBuild.ps1.

The Root Cause

In Build.ps1 at line 832:

$wrapperArgs = @('-NoProfile', '-File', $script:RustBuildWrapper, '-Path', $Path, '-CargoArgs') + $effectiveCargoArgs

Because it flattens the arguments and passes them after -CargoArgs, any argument starting with - (like --workspace or --check) or -- itself is parsed by PowerShell as a parameter name for Invoke-RustBuild.ps1, leading to binding errors.

The Solution

The robust and correct fix is to use the -- end-of-parameters marker when calling the wrapper script, which tells PowerShell to treat all subsequent arguments as positional arguments (binding them to $RemainingArgs / $finalArgs):

$wrapperArgs = @('-NoProfile', '-File', $script:RustBuildWrapper, '-Path', $Path, '--') + $effectiveCargoArgs

This root-cause fix will not only make fmt work with or without --, but it will also completely fix the Clippy gate (which must use -- to pass -D warnings to clippy).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to clear two CI gates by (1) removing hardcoded Bitwarden secrets from a startup helper script and (2) fixing the Rust formatting invocation so the format job fails only on real rustfmt diffs rather than a PowerShell wrapper argument-parsing issue.

Changes:

  • Switch Bitwarden session initialization to use BW_CLIENTID / BW_CLIENTSECRET / BW_PASSWORD environment variables instead of hardcoded secrets.
  • Update Rust fmt command construction to use cargo fmt --all --check (avoiding the problematic standalone -- argument through the wrapper).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
Tools/SystemScripts/C-Scripts/Startup/Initialize-BitwardenSession.ps1 Replaces hardcoded Bitwarden credential strings with env-var sourced values.
Build.ps1 Adjusts Rust fmt argument list to avoid the wrapper-breaking -- token.

Comment on lines 66 to +70
$script:Config = @{
CredentialTargets = @{
ClientId = 'bitwarden/client_id'
ClientSecret = 'bitwarden/client_secret'
MasterPassword = 'bitwarden/master_password'
ClientId = $env:BW_CLIENTID
ClientSecret = $env:BW_CLIENTSECRET # pragma: allowlist secret
MasterPassword = $env:BW_PASSWORD # pragma: allowlist secret

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f33091b465

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +68 to +70
ClientId = $env:BW_CLIENTID
ClientSecret = $env:BW_CLIENTSECRET # pragma: allowlist secret
MasterPassword = $env:BW_PASSWORD # pragma: allowlist secret

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep Bitwarden entries as credential target names

When this startup script runs with the documented Bitwarden env vars (BW_CLIENTID/BW_CLIENTSECRET/BW_PASSWORD), these assignments put the actual secret values into CredentialTargets, but Get-BitwardenCredentials still iterates those values and calls Get-StoredCredentialPassword -Target on them. That makes cmdkey/CredRead look for Credential Manager entries named after the secrets instead of bitwarden/client_id, bitwarden/client_secret, and bitwarden/master_password, so initialization fails unless such secret-named entries exist; the same values can also be written to DEBUG/error logs as target names. Keep this map as target names or read the env vars directly without passing them through the Credential Manager lookup.

Useful? React with 👍 / 👎.

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.

2 participants