mirror of
https://github.com/garrytan/gstack.git
synced 2026-06-05 01:28:15 +02:00
v1.55.1.0 fix: telemetry consent accuracy + gstack-slug cache sanitization (#1848)
* fix(gstack-slug): sanitize cached slug before eval The compute and fallback paths filter slug output to [a-zA-Z0-9._-], but a value read straight from ~/.gstack/slug-cache was echoed into eval output unsanitized. A locally-planted cache file could inject shell into eval "$(gstack-slug)". Re-sanitize on every path so the invariant the file header promises actually holds, and heal a poisoned cache on the next write. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(telemetry): accurate consent copy + JSON-safe repo basename The telemetry consent prompt promised "no repo names" while the preamble epilogue records the repo basename in the local skill-usage.jsonl. It is already stripped before any remote upload, so it never left the machine, but the copy was unqualified. Reword it to state repo name is local-only and stripped before upload. Also sanitize the basename to [a-zA-Z0-9._-] before it goes into the hand-built JSON, so a repo directory name containing quotes or newlines can neither break the JSON nor leak a fragment past the regex stripper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(docs): regenerate SKILL.md + ship goldens for telemetry change Generated output of the preceding resolver change: the corrected consent copy and sanitized repo basename now appear in every skill preamble. Golden ship fixtures refreshed to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(telemetry): enforce no-repo-identity-egress invariant Pins the contract that repo/branch identity in the synced skill-usage.jsonl is stripped before the remote POST. Three checks: a floor (the three known fields), coverage (every repo/branch field a producer writes into skill-usage.jsonl is stripped, so a future producer rename can't silently leak), and behavior (runs the actual sed strip expressions over a sample event). Scoped to the synced file, so the local-only timeline branch field is correctly excluded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(gstack-slug): regression test for cached-slug eval injection Proves a poisoned ~/.gstack/slug-cache file cannot inject shell metacharacters into gstack-slug output (the value consumed by eval). Verified red when the cache-read sanitization is removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore: bump version and changelog (v1.55.1.0) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
+2
-2
@@ -65,7 +65,7 @@ _QUESTION_TUNING=$(~/.claude/skills/gstack/bin/gstack-config get question_tuning
|
||||
echo "QUESTION_TUNING: $_QUESTION_TUNING"
|
||||
mkdir -p ~/.gstack/analytics
|
||||
if [ "$_TEL" != "off" ]; then
|
||||
echo '{"skill":"ship","ts":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","repo":"'$(basename "$(git rev-parse --show-toplevel 2>/dev/null)" 2>/dev/null || echo "unknown")'"}' >> ~/.gstack/analytics/skill-usage.jsonl 2>/dev/null || true
|
||||
echo '{"skill":"ship","ts":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","repo":"'$(_repo=$(basename "$(git rev-parse --show-toplevel 2>/dev/null)" 2>/dev/null | tr -cd 'a-zA-Z0-9._-'); echo "${_repo:-unknown}")'"}' >> ~/.gstack/analytics/skill-usage.jsonl 2>/dev/null || true
|
||||
fi
|
||||
for _PF in $(find ~/.gstack/analytics -maxdepth 1 -name '.pending-*' 2>/dev/null); do
|
||||
if [ -f "$_PF" ]; then
|
||||
@@ -175,7 +175,7 @@ Only run `open` if yes. Always run `touch`.
|
||||
|
||||
If `TEL_PROMPTED` is `no` AND `LAKE_INTRO` is `yes`: ask telemetry once via AskUserQuestion:
|
||||
|
||||
> Help gstack get better. Share usage data only: skill, duration, crashes, stable device ID. No code, file paths, or repo names.
|
||||
> Help gstack get better. Share usage data only: skill, duration, crashes, stable device ID. No code or file paths. Your repo name is recorded locally only and stripped before any upload.
|
||||
|
||||
Options:
|
||||
- A) Help gstack get better! (recommended)
|
||||
|
||||
+2
-2
@@ -51,7 +51,7 @@ _QUESTION_TUNING=$($GSTACK_BIN/gstack-config get question_tuning 2>/dev/null ||
|
||||
echo "QUESTION_TUNING: $_QUESTION_TUNING"
|
||||
mkdir -p ~/.gstack/analytics
|
||||
if [ "$_TEL" != "off" ]; then
|
||||
echo '{"skill":"ship","ts":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","repo":"'$(basename "$(git rev-parse --show-toplevel 2>/dev/null)" 2>/dev/null || echo "unknown")'"}' >> ~/.gstack/analytics/skill-usage.jsonl 2>/dev/null || true
|
||||
echo '{"skill":"ship","ts":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","repo":"'$(_repo=$(basename "$(git rev-parse --show-toplevel 2>/dev/null)" 2>/dev/null | tr -cd 'a-zA-Z0-9._-'); echo "${_repo:-unknown}")'"}' >> ~/.gstack/analytics/skill-usage.jsonl 2>/dev/null || true
|
||||
fi
|
||||
for _PF in $(find ~/.gstack/analytics -maxdepth 1 -name '.pending-*' 2>/dev/null); do
|
||||
if [ -f "$_PF" ]; then
|
||||
@@ -161,7 +161,7 @@ Only run `open` if yes. Always run `touch`.
|
||||
|
||||
If `TEL_PROMPTED` is `no` AND `LAKE_INTRO` is `yes`: ask telemetry once via AskUserQuestion:
|
||||
|
||||
> Help gstack get better. Share usage data only: skill, duration, crashes, stable device ID. No code, file paths, or repo names.
|
||||
> Help gstack get better. Share usage data only: skill, duration, crashes, stable device ID. No code or file paths. Your repo name is recorded locally only and stripped before any upload.
|
||||
|
||||
Options:
|
||||
- A) Help gstack get better! (recommended)
|
||||
|
||||
+2
-2
@@ -53,7 +53,7 @@ _QUESTION_TUNING=$($GSTACK_BIN/gstack-config get question_tuning 2>/dev/null ||
|
||||
echo "QUESTION_TUNING: $_QUESTION_TUNING"
|
||||
mkdir -p ~/.gstack/analytics
|
||||
if [ "$_TEL" != "off" ]; then
|
||||
echo '{"skill":"ship","ts":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","repo":"'$(basename "$(git rev-parse --show-toplevel 2>/dev/null)" 2>/dev/null || echo "unknown")'"}' >> ~/.gstack/analytics/skill-usage.jsonl 2>/dev/null || true
|
||||
echo '{"skill":"ship","ts":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","repo":"'$(_repo=$(basename "$(git rev-parse --show-toplevel 2>/dev/null)" 2>/dev/null | tr -cd 'a-zA-Z0-9._-'); echo "${_repo:-unknown}")'"}' >> ~/.gstack/analytics/skill-usage.jsonl 2>/dev/null || true
|
||||
fi
|
||||
for _PF in $(find ~/.gstack/analytics -maxdepth 1 -name '.pending-*' 2>/dev/null); do
|
||||
if [ -f "$_PF" ]; then
|
||||
@@ -163,7 +163,7 @@ Only run `open` if yes. Always run `touch`.
|
||||
|
||||
If `TEL_PROMPTED` is `no` AND `LAKE_INTRO` is `yes`: ask telemetry once via AskUserQuestion:
|
||||
|
||||
> Help gstack get better. Share usage data only: skill, duration, crashes, stable device ID. No code, file paths, or repo names.
|
||||
> Help gstack get better. Share usage data only: skill, duration, crashes, stable device ID. No code or file paths. Your repo name is recorded locally only and stripped before any upload.
|
||||
|
||||
Options:
|
||||
- A) Help gstack get better! (recommended)
|
||||
|
||||
@@ -0,0 +1,65 @@
|
||||
/**
|
||||
* gstack-slug cache-read sanitization.
|
||||
*
|
||||
* `eval "$(gstack-slug)"` is how callers load SLUG/BRANCH. The compute and
|
||||
* fallback paths filter to [a-zA-Z0-9._-], but a value read straight from the
|
||||
* cache file used to be echoed unsanitized — a planted cache file could inject
|
||||
* shell. This pins the fix: a poisoned cache must never produce shell
|
||||
* metacharacters in the SLUG= output line.
|
||||
*/
|
||||
|
||||
import { describe, test, expect } from 'bun:test';
|
||||
import { spawnSync } from 'bun';
|
||||
import fs from 'fs';
|
||||
import os from 'os';
|
||||
import path from 'path';
|
||||
|
||||
const ROOT = path.resolve(__dirname, '..');
|
||||
const SLUG_BIN = path.join(ROOT, 'bin', 'gstack-slug');
|
||||
|
||||
/** Reproduce the script's cache-key derivation: absolute path with / -> _. */
|
||||
function cacheKeyFor(dir: string): string {
|
||||
return dir.replace(/\//g, '_');
|
||||
}
|
||||
|
||||
function runSlug(cwd: string, home: string) {
|
||||
return spawnSync([SLUG_BIN], {
|
||||
cwd,
|
||||
env: { ...process.env, HOME: home },
|
||||
});
|
||||
}
|
||||
|
||||
describe('gstack-slug cache-read sanitization', () => {
|
||||
test('a poisoned cache file cannot inject shell metacharacters into output', () => {
|
||||
const home = fs.mkdtempSync(path.join(os.tmpdir(), 'gslug-home-'));
|
||||
const proj = fs.mkdtempSync(path.join(os.tmpdir(), 'gslug-proj-'));
|
||||
try {
|
||||
const cacheDir = path.join(home, '.gstack', 'slug-cache');
|
||||
fs.mkdirSync(cacheDir, { recursive: true });
|
||||
// realpath: macOS tmpdir is a symlink (/var -> /private/var); the script
|
||||
// runs in the resolved cwd, so key off the resolved path.
|
||||
const realProj = fs.realpathSync(proj);
|
||||
const payload = 'evil"; touch ' + path.join(home, 'pwned') + '; echo "x';
|
||||
fs.writeFileSync(path.join(cacheDir, cacheKeyFor(realProj)), payload);
|
||||
|
||||
const out = runSlug(realProj, home);
|
||||
const stdout = out.stdout.toString();
|
||||
|
||||
const slugLine = stdout.split('\n').find((l) => l.startsWith('SLUG='));
|
||||
expect(slugLine).toBeDefined();
|
||||
const slugValue = slugLine!.slice('SLUG='.length);
|
||||
|
||||
// The value must be sanitized: only [a-zA-Z0-9._-], no quotes/semicolons/spaces.
|
||||
expect(slugValue).toMatch(/^[a-zA-Z0-9._-]*$/);
|
||||
expect(slugLine).not.toContain('"');
|
||||
expect(slugLine).not.toContain(';');
|
||||
expect(slugLine).not.toContain(' ');
|
||||
|
||||
// And the injection must not have fired during the script's own run.
|
||||
expect(fs.existsSync(path.join(home, 'pwned'))).toBe(false);
|
||||
} finally {
|
||||
fs.rmSync(home, { recursive: true, force: true });
|
||||
fs.rmSync(proj, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,137 @@
|
||||
/**
|
||||
* Telemetry "no repo identity egress" invariant.
|
||||
*
|
||||
* The telemetry consent copy promises a user's repo name is recorded locally
|
||||
* only and stripped before any upload (scripts/resolvers/preamble/
|
||||
* generate-telemetry-prompt.ts). Two producers write repo/branch identity into
|
||||
* the local skill-usage.jsonl:
|
||||
*
|
||||
* - the preamble epilogue → "repo"
|
||||
* (scripts/resolvers/preamble/generate-preamble-bash.ts)
|
||||
* - gstack-telemetry-log → "_repo_slug", "_branch"
|
||||
* (bin/gstack-telemetry-log)
|
||||
*
|
||||
* gstack-telemetry-sync MUST strip every one of those fields before the remote
|
||||
* POST (bin/gstack-telemetry-sync). This test enforces that contract three ways:
|
||||
*
|
||||
* 1. Coverage — every repo/branch field the producers emit is also stripped.
|
||||
* Catches "added a new repo field, forgot to strip it" (the rename-to-_repo
|
||||
* landmine, or any future producer drift).
|
||||
* 2. Behavior — run the ACTUAL sed strip expressions from the sync script over
|
||||
* a sample event line and assert no repo/branch field survives, while benign
|
||||
* fields do. Catches a broken/edited regex, not just a missing line.
|
||||
* 3. Floor — the three known fields are always in the stripped set, so deleting
|
||||
* a strip rule fails CI even if a producer also stops emitting it.
|
||||
*/
|
||||
|
||||
import { describe, test, expect } from 'bun:test';
|
||||
import { spawnSync } from 'bun';
|
||||
import fs from 'fs';
|
||||
import path from 'path';
|
||||
|
||||
const ROOT = path.resolve(__dirname, '..');
|
||||
const SYNC = path.join(ROOT, 'bin', 'gstack-telemetry-sync');
|
||||
const PREAMBLE = path.join(ROOT, 'scripts', 'resolvers', 'preamble', 'generate-preamble-bash.ts');
|
||||
const TEL_LOG = path.join(ROOT, 'bin', 'gstack-telemetry-log');
|
||||
|
||||
// Fields that identify the user's repo/branch. The promise is that NONE of
|
||||
// these reach the network. Add to this floor if a new identity field is born.
|
||||
const REPO_IDENTITY_FLOOR = ['repo', '_repo_slug', '_branch'];
|
||||
|
||||
const isRepoIdentity = (field: string) => /repo|branch/i.test(field);
|
||||
|
||||
/** Pull every `sed -e 's/.../g'` expression out of the sync script. */
|
||||
function extractSedExprs(scriptText: string): string[] {
|
||||
return [...scriptText.matchAll(/-e\s+'(s\/[^']*)'/g)].map((m) => m[1]);
|
||||
}
|
||||
|
||||
/** The JSON key a strip expression targets, e.g. `,"repo":"[^"]*"` -> `repo`. */
|
||||
function fieldFromSedExpr(expr: string): string | null {
|
||||
const m = expr.match(/,"([A-Za-z_][A-Za-z0-9_]*)":/);
|
||||
return m ? m[1] : null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Repo/branch JSON keys a producer writes INTO skill-usage.jsonl — the only
|
||||
* file gstack-telemetry-sync reads and uploads. Scoped to the emission lines
|
||||
* that target the synced file so local-only sinks (e.g. the timeline log, which
|
||||
* carries "branch" but is never synced) don't count against the egress invariant.
|
||||
*/
|
||||
function emittedRepoFields(lines: string[]): string[] {
|
||||
const text = lines.join('\n');
|
||||
const keys = [...text.matchAll(/"([A-Za-z_][A-Za-z0-9_]*)":/g)].map((m) => m[1]);
|
||||
return [...new Set(keys.filter(isRepoIdentity))];
|
||||
}
|
||||
|
||||
describe('telemetry no-repo-identity-egress invariant', () => {
|
||||
const syncText = fs.readFileSync(SYNC, 'utf-8');
|
||||
const sedExprs = extractSedExprs(syncText);
|
||||
const strippedRepoExprs = sedExprs.filter((e) => {
|
||||
const f = fieldFromSedExpr(e);
|
||||
return f !== null && isRepoIdentity(f);
|
||||
});
|
||||
const strippedFields = new Set(
|
||||
strippedRepoExprs.map(fieldFromSedExpr).filter((f): f is string => f !== null),
|
||||
);
|
||||
|
||||
test('floor: the three known repo-identity fields are stripped', () => {
|
||||
for (const field of REPO_IDENTITY_FLOOR) {
|
||||
expect(strippedFields.has(field)).toBe(true);
|
||||
}
|
||||
});
|
||||
|
||||
test('coverage: every repo/branch field the producers emit into skill-usage.jsonl is stripped', () => {
|
||||
// Only emission lines that target the synced file (skill-usage.jsonl). The
|
||||
// preamble appends directly; gstack-telemetry-log builds the synced event
|
||||
// with a `printf '{"v":1,...` line into $JSONL_FILE (= skill-usage.jsonl).
|
||||
const preambleSynced = fs
|
||||
.readFileSync(PREAMBLE, 'utf-8')
|
||||
.split('\n')
|
||||
.filter((l) => l.includes('skill-usage.jsonl'));
|
||||
const telLogSynced = fs
|
||||
.readFileSync(TEL_LOG, 'utf-8')
|
||||
.split('\n')
|
||||
.filter((l) => l.includes('"v":1') || l.includes('skill-usage'));
|
||||
const emitted = new Set<string>([
|
||||
...emittedRepoFields(preambleSynced),
|
||||
...emittedRepoFields(telLogSynced),
|
||||
]);
|
||||
// The preamble must emit "repo" — guards against the test silently passing
|
||||
// because a regex stopped matching the producer.
|
||||
expect(emitted.has('repo')).toBe(true);
|
||||
for (const field of emitted) {
|
||||
expect(
|
||||
strippedFields.has(field),
|
||||
`producer emits repo-identity field "${field}" but gstack-telemetry-sync does not strip it (would leak to remote)`,
|
||||
).toBe(true);
|
||||
}
|
||||
});
|
||||
|
||||
test('behavior: the real sed expressions remove repo identity, keep benign fields', () => {
|
||||
const sample =
|
||||
'{"v":1,"ts":"2026-06-02T00:00:00Z","skill":"design-shotgun",' +
|
||||
'"repo":"my-secret-repo","_repo_slug":"acme-my-secret-repo","_branch":"feature-x",' +
|
||||
'"sessions":3,"installation_id":"abc123"}';
|
||||
|
||||
const sedArgs: string[] = [];
|
||||
for (const e of strippedRepoExprs) {
|
||||
sedArgs.push('-e', e);
|
||||
}
|
||||
const out = spawnSync(['sed', ...sedArgs], {
|
||||
stdin: Buffer.from(sample),
|
||||
});
|
||||
const cleaned = out.stdout.toString();
|
||||
|
||||
// No repo/branch identity survives, value or key.
|
||||
expect(cleaned).not.toContain('my-secret-repo');
|
||||
expect(cleaned).not.toContain('feature-x');
|
||||
expect(cleaned).not.toContain('"repo"');
|
||||
expect(cleaned).not.toContain('_repo_slug');
|
||||
expect(cleaned).not.toContain('_branch');
|
||||
|
||||
// Benign fields are untouched — the strip is surgical, not a blanket wipe.
|
||||
expect(cleaned).toContain('"skill":"design-shotgun"');
|
||||
expect(cleaned).toContain('"sessions":3');
|
||||
expect(cleaned).toContain('"ts":"2026-06-02T00:00:00Z"');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user