Skip to content

fix(installer): Windows one-client guard reads clientId via ConvertFrom-Json (#200)#340

Open
LukasWodka wants to merge 1 commit into
developfrom
fix/200-windows-one-client-guard
Open

fix(installer): Windows one-client guard reads clientId via ConvertFrom-Json (#200)#340
LukasWodka wants to merge 1 commit into
developfrom
fix/200-windows-one-client-guard

Conversation

@LukasWodka

@LukasWodka LukasWodka commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Summary

The Windows one-client-per-machine guard only recognized an existing install when helm get values happened to render clientId in double quotes (clientId:\s*"([^"]+)"). Helm re-serializes stored values on get, so typical clientIds come back unquoted — the guard silently matched nothing and the installer would re-point the machine to a different Client ID, defeating the protection from #192.

Fix: read the values as JSON (helm get values <release> -n <ns> -o json | ConvertFrom-Json) and take .clientId directly — quoting disappears at that layer, and it matches the adjacent comment that already claimed ConvertFrom-Json. Handles null (release with no user values), a missing clientId key, and unparsable output from one release without aborting the scan of the remaining releases. Mirrors what the bash installer's _extract_yaml_value tolerance achieves (scripts/lib/install-client-helm.sh), but without regex-scraping YAML at all.

Tests: the two existing guard tests now serve real-helm-shaped output (JSON for -o json, unquoted YAML otherwise — so a regression back to YAML-scraping fails them), plus new Pester cases covering unquoted / single-quoted / double-quoted YAML views, a null-values release that must not abort the scan, and values without a clientId key that must not trip the guard.

scripts/install-k8s.ps1 is on the signed-manifest surface — scripts/manifest.sha256 regenerated via scripts/gen-manifest.sh (only the install-k8s.ps1 line changed; --check passes).

Note: open PR #337 also touches scripts/install-k8s.ps1 + scripts/manifest.sha256 (+ the same test file) — whichever merges second gets a trivial manifest conflict; resolve by re-running bash scripts/gen-manifest.sh.

Related

Closes #200 · Refs #192, #199

Type of change

  • Feature
  • Bug fix
  • Tech-debt / refactor
  • Docs
  • Security / hardening
  • Breaking change

Test plan

No pwsh on this machine, so the CI-equivalent suite ran in a mcr.microsoft.com/powershell:lts-7.4 Linux container (same invocation as installer-tests.yaml):

  • AST parse of both edited files — OK
  • PSScriptAnalyzer at CI's error gate — no errors (warnings unchanged, all pre-existing lines)
  • Pester >= 5.5 over scripts/tests/95 passed, 0 failed, 6 skipped (skips are the -Skip:(-not $IsWindows) blocks, same as CI's Linux leg); all 7 one-client-guard tests green
  • bash scripts/gen-manifest.sh --check — passes

The real-Windows leg runs in CI (unit-pester, windows-latest).

Deployment notes

None — installer script only; ships with the next release artifact regeneration (manifest already updated).

Checklist

  • Tests added / updated and passing locally
  • Docs updated if behavior or config changed — N/A (behavior restored to what the comment already claimed)
  • No secrets / credentials in the diff
  • For security-sensitive paths: appropriate reviewer requested

🤖 Generated with Claude Code


Note

Low Risk
Installer-only hardening that restores intended one-client protection; behavior is covered by new and updated Pester tests with no runtime service changes.

Overview
Fixes #200: the Windows installer’s one-client-per-machine check no longer scrapes helm get values YAML with a double-quote regex. It now uses helm get values … -o json and ConvertFrom-Json to read .clientId, so unquoted Helm re-serialization can’t bypass the guard and silently replace an existing client.

The release scan is hardened: skip failed/empty JSON, treat literal null and missing clientId as non-matches, and continue on parse errors so one bad release doesn’t stop the rest.

Pester mocks match real Helm (JSON for -o json, varied YAML otherwise) and add cases for quoting styles, null values, and values without clientId. scripts/manifest.sha256 is updated for the signed install-k8s.ps1 artifact.

Reviewed by Cursor Bugbot for commit 64151b7. Bugbot is set up for automated code reviews on this repo. Configure here.

…om-Json (#200)

The one-client-per-machine guard regex-scraped `helm get values` YAML and
only matched a DOUBLE-QUOTED clientId. Helm re-serializes values on `get`,
so typical clientIds come back unquoted -- the guard silently matched
nothing and a re-install could re-point the machine to a different Client
ID, defeating the protection from #192 (Bugbot finding on #199).

Read the values as JSON instead (`helm get values <rel> -n <ns> -o json |
ConvertFrom-Json` -> `.clientId`), which sidesteps YAML quoting entirely
and matches the adjacent comment that already claimed ConvertFrom-Json.
A release with no user values (literal `null`), a missing clientId key, or
unparsable output from one release is skipped without aborting the scan of
the remaining releases.

Tests: existing guard mocks now serve real-helm-shaped output (JSON for
`-o json`, unquoted YAML otherwise), plus new cases for unquoted / single-
quoted / double-quoted YAML views, a null-values release mid-scan, and
values without a clientId key. Pester (Linux container, lts-7.4): 95
passed, 0 failed, 6 skipped ($IsWindows skips).

scripts/manifest.sha256 regenerated (install-k8s.ps1 line only) -- the
file is on the signed-manifest surface; gen-manifest.sh --check passes.

Closes #200

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
Contributor Author

👋 Heads-up — Code review queue is at 48 / 30

Above the WIP limit. The team convention is to review existing PRs before opening new work.

Open PRs currently in Code review (oldest first):

Pull from review before opening new work. (This is a nudge from the kanban WIP check, not a block.)

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