feat(analytics): persist installer setup identity - #18
patrickleet wants to merge 6 commits into
Conversation
…econciliation-epic/install-download-join]]
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe installer now persists validated UUIDv4 setup IDs in protected configuration state. It rejects unsafe paths and preserves valid existing state. New Bats coverage and a required CI job validate the behavior. ChangesSetup identity persistence
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🔵 Low · up to The installer now persists a setup identity, but an insecure existing configuration directory could let another account replace the identity file and access that value. The change is otherwise mergeable with explicit owner awareness or follow-up to validate directory ownership and permissions. Sequence Diagram(s)sequenceDiagram
participant Installer as install.sh
participant Binary as Installed binary
participant State as GitKB setup state
participant CI as GitHub Actions
Installer->>Binary: Verify installation
Installer->>State: Persist validated setup UUID
State-->>Installer: Accept or reject state update
CI->>Installer: Check shell syntax
CI->>State: Run setup-state security tests
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test-install.yml (1)
166-184: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not mask a failed setup-state job when platform jobs are skipped.
test-setup-statehas no upstream-success condition. If the upstream workflow fails,test-linuxandtest-macosare skipped, then Lines 175-178 makeresultssucceed before Line 183 checks a failedtest-setup-stateresult.Apply the same
workflow_runcondition totest-setup-state, and only take the skipped-success path when all three test jobs are skipped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test-install.yml around lines 166 - 184, Update the test-setup-state job condition to use the same workflow_run success requirement as test-linux and test-macos. In the Aggregate results step, change the early skipped-success path so it applies only when test-setup-state, test-linux, and test-macos all have a skipped result, ensuring a failed setup job cannot be masked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test-install.yml:
- Around line 25-35: Update the test-setup-state job to pin actions/checkout@v4
to a full commit SHA, add persist-credentials: false to that checkout step, and
define job-level permissions with contents: read. Apply the same checkout
hardening to every other job in this workflow that checks out repository code.
In `@install.sh`:
- Around line 62-105: Update the install attribution persistence function
containing umask 077 so its temporary restrictive umask cannot leak into the
caller when install.sh is sourced. Run the function body in a subshell or save
the previous umask and restore it on every return path, including mkdir,
validation, existing-state, and temporary-file failure exits.
- Around line 49-57: Validate config_home for symlinks or non-directory existing
paths before deriving or creating state_dir, so mkdir -p cannot follow an
attacker-selected XDG_CONFIG_HOME target. Preserve the existing state_dir
validation and add a Bats test covering a symlinked XDG_CONFIG_HOME.
In `@scripts/atc-setup-identity-proof.sh`:
- Around line 31-35: Update the checksum selection in the proof setup flow to
verify shasum is available before invoking it. When neither sha256sum nor shasum
exists, set status=failed and prevent the proof from emitting a passed result
with an empty evidence_sha; preserve the existing checksum behavior when either
utility is present.
In `@tests/install-setup-id.bats`:
- Around line 21-22: Add a reusable permission-check helper in the Bats test
file, following the installer proof script’s fallback from GNU stat -c '%a' to
macOS stat -f '%Lp'. Update all affected setup-id and gitkb permission
assertions to use this helper while preserving the expected 600 and 700 values.
---
Outside diff comments:
In @.github/workflows/test-install.yml:
- Around line 166-184: Update the test-setup-state job condition to use the same
workflow_run success requirement as test-linux and test-macos. In the Aggregate
results step, change the early skipped-success path so it applies only when
test-setup-state, test-linux, and test-macos all have a skipped result, ensuring
a failed setup job cannot be masked.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a1d0c5eb-34ef-4782-a611-585e8ac1fc4d
📒 Files selected for processing (5)
.atc/productionize-proof.json.github/workflows/test-install.ymlinstall.shscripts/atc-setup-identity-proof.shtests/install-setup-id.bats
Emit recursively key-sorted JSON with exactly one trailing newline so current ATC can verify the pinned disposable acceptance receipt. [[tasks/posthog-baseline-v1-reconciliation-epic/correlation-smoke]]
Implements [[tasks/posthog-baseline-v1-reconciliation-epic/install-download-join]]
|
Productionization review started for pinned head Implements [[webanalytics-session-identity]] |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@install.sh`:
- Around line 43-45: Update the UUID validation in persist_setup_id and the
existing setup-id file validation to reject multi-line or appended content,
requiring exactly one valid UUID followed by the expected trailing newline.
Replace line-oriented grep checks with whole-string validation, and add Bats
coverage for valid UUID values plus setup IDs and state files containing
appended lines.
- Around line 55-58: Update the config_home validation block in install.sh to
reject existing directories not owned by the current user or writable by
group/other users, while preserving the existing symlink and non-directory
checks. Add a Bats test covering a foreign-owned or group/other-writable
configuration home and verify attribution state is not written.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 28be1633-3fe0-453d-9650-edcd44a0e291
📒 Files selected for processing (5)
.github/workflows/test-install.yml.gitignoreinstall.shscripts/atc-setup-identity-proof.shtests/install-setup-id.bats
| if [ -L "$config_home" ] || { [ -e "$config_home" ] && [ ! -d "$config_home" ]; }; then | ||
| warn "Could not save install attribution state: unsafe GitKB config directory" | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate existing config_home ownership and permissions.
An existing config_home can be foreign-owned or writable by another account. That account can replace gitkb after the state-directory checks. The installer can then create the temporary file in an attacker-controlled directory and disclose the setup ID before the hard-link operation fails.
Reject an existing config_home unless the current user owns it and group and other users cannot write it. Add a Bats case for an insecure configuration home.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install.sh` around lines 55 - 58, Update the config_home validation block in
install.sh to reject existing directories not owned by the current user or
writable by group/other users, while preserving the existing symlink and
non-directory checks. Add a Bats test covering a foreign-owned or
group/other-writable configuration home and verify attribution state is not
written.
|
Review pass 1 found one actionable workflow issue on Ref: [[tasks/posthog-baseline-v1-reconciliation-epic/install-download-join]] |
Implements [[feat/webanalytics-session-identity]]
|
Fix pass 1 is confirmed at Implements [[feat/webanalytics-session-identity]] |
|
Review pass 2 found one actionable installer validation issue on Ref: [[tasks/posthog-baseline-v1-reconciliation-epic/install-download-join]] |
Implements [[feat/webanalytics-session-identity]]
|
Fix pass 2 is confirmed at Implements [[feat/webanalytics-session-identity]] |
|
Review pass 3 is clean on final head Ref: [[tasks/posthog-baseline-v1-reconciliation-epic/install-download-join]] |
|
Productionization completed successfully on final head Implements [[feat/webanalytics-session-identity]] |
|
Should this id be created by git-kb's binary, instead? That way, regardless of how it's installed (curl or brew), there's a single method? |
Summary
Security
Verification
bash -n install.shactionlint .github/workflows/test-install.ymlSummary by CodeRabbit
New Features
Bug Fixes
Tests