fix(installer): Windows one-client guard reads clientId via ConvertFrom-Json (#200)#340
Open
LukasWodka wants to merge 1 commit into
Open
fix(installer): Windows one-client guard reads clientId via ConvertFrom-Json (#200)#340LukasWodka wants to merge 1 commit into
LukasWodka wants to merge 1 commit into
Conversation
…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>
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.) |
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
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
The Windows one-client-per-machine guard only recognized an existing install when
helm get valueshappened to renderclientIdin double quotes (clientId:\s*"([^"]+)"). Helm re-serializes stored values onget, 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.clientIddirectly — quoting disappears at that layer, and it matches the adjacent comment that already claimed ConvertFrom-Json. Handlesnull(release with no user values), a missingclientIdkey, and unparsable output from one release without aborting the scan of the remaining releases. Mirrors what the bash installer's_extract_yaml_valuetolerance 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, anull-values release that must not abort the scan, and values without aclientIdkey that must not trip the guard.scripts/install-k8s.ps1is on the signed-manifest surface —scripts/manifest.sha256regenerated viascripts/gen-manifest.sh(only the install-k8s.ps1 line changed;--checkpasses).Related
Closes #200 · Refs #192, #199
Type of change
Test plan
No
pwshon this machine, so the CI-equivalent suite ran in amcr.microsoft.com/powershell:lts-7.4Linux container (same invocation asinstaller-tests.yaml):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 greenbash scripts/gen-manifest.sh --check— passesThe 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
🤖 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 valuesYAML with a double-quote regex. It now useshelm get values … -o jsonandConvertFrom-Jsonto 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
nulland missingclientIdas non-matches, andcontinueon 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,nullvalues, and values withoutclientId.scripts/manifest.sha256is updated for the signedinstall-k8s.ps1artifact.Reviewed by Cursor Bugbot for commit 64151b7. Bugbot is set up for automated code reviews on this repo. Configure here.