-
Notifications
You must be signed in to change notification settings - Fork 136
feat: append cli_context param to altimate auth URL for PostHog session correlation #1068
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,15 @@ | ||
| import type { Hooks, PluginInput } from "@opencode-ai/plugin" | ||
| import { createServer } from "http" | ||
| import { randomBytes } from "crypto" | ||
| import { randomBytes, randomUUID } from "crypto" | ||
| import open from "open" | ||
| import { AltimateApi } from "../api/client" | ||
| // altimate_change — onboarding telemetry for the gateway sign-in funnel | ||
| import * as OnboardingTelemetry from "../telemetry/onboarding" | ||
| import fs from "fs" | ||
| import os from "os" | ||
| import path from "path" | ||
| import { InstallationVersion } from "@opencode-ai/core/installation/version" | ||
| import { Log } from "@/altimate/util/log" | ||
|
|
||
| /** | ||
| * Why a failure reason is attached at the rejection site rather than inferred from the message: | ||
|
|
@@ -47,6 +52,76 @@ const DEFAULT_WEB_URL = "https://app.myaltimate.com" | |
| // deliver. | ||
| const DEFAULT_API_URL = "https://api.myaltimate.com" | ||
|
|
||
| const log = Log.create({ service: "altimate-plugin" }) | ||
|
|
||
| // altimate_change start — shared machine-id helper: reads the file when present, | ||
| // mints a new random UUID with exclusive-create (wx flag) when absent so two | ||
| // racing initializers (TUI main thread + server worker) cannot each mint a | ||
| // different id on a fresh install. The loser of the race re-reads what the | ||
| // winner wrote. Used by both buildCliContext and the telemetry module's doInit(). | ||
| export function getOrCreateMachineId(machineIdPath?: string): string { | ||
| const idPath = machineIdPath ?? path.join(os.homedir(), ".altimate", "machine-id") | ||
| try { | ||
| return fs.readFileSync(idPath, "utf8").trim() | ||
| } catch (readErr) { | ||
| if ((readErr as NodeJS.ErrnoException)?.code !== "ENOENT") throw readErr | ||
| } | ||
| // File does not exist — create it exclusively so racing callers converge on | ||
| // the same UUID rather than each writing their own. | ||
| const candidate = randomUUID() | ||
| fs.mkdirSync(path.dirname(idPath), { recursive: true }) | ||
| try { | ||
| fs.writeFileSync(idPath, candidate, { encoding: "utf8", flag: "wx" }) | ||
| return candidate | ||
| } catch { | ||
| // Lost the creation race — read what the winner wrote. | ||
| return fs.readFileSync(idPath, "utf8").trim() | ||
| } | ||
| } | ||
| // altimate_change end | ||
|
|
||
| // Builds a base64url-encoded context blob for correlating this browser auth | ||
| // session with CLI telemetry in PostHog. The machine_id is a random UUID | ||
| // written by the telemetry module — not tied to hardware, OS, or user identity. | ||
| // After sign-in, the frontend calls posthog.alias(email, machine_id) to link | ||
| // the device to the authenticated account. | ||
| export function buildCliContext(machineIdPath?: string): string { | ||
| // altimate_change start — honour the telemetry opt-out: if the user disabled | ||
| // telemetry, do not read or transmit the machine_id (matches the guard in | ||
| // telemetry/index.ts::doInit around ALTIMATE_TELEMETRY_DISABLED). | ||
| let machineId = "" | ||
| if (process.env.ALTIMATE_TELEMETRY_DISABLED !== "true") { | ||
| try { | ||
| machineId = getOrCreateMachineId(machineIdPath) | ||
| } catch (err) { | ||
| const code = (err as NodeJS.ErrnoException)?.code | ||
| const idPath = machineIdPath ?? path.join(os.homedir(), ".altimate", "machine-id") | ||
| if (code === "ENOENT") log.debug("machine-id not present — cli_context will omit machine_id") | ||
| else log.warn("machine-id read failed", { code, path: idPath }) | ||
| } | ||
| } | ||
| // altimate_change end | ||
| // altimate_change start — omit machine_id key when empty (matches telemetry | ||
| // module pattern: `...(machineId && { machine_id: machineId })`). Sending "" | ||
| // is meaningless for posthog.alias() and misleads downstream consumers. | ||
| const ctx: Record<string, unknown> = { v: 1, cli_version: InstallationVersion } | ||
| if (machineId) ctx.machine_id = machineId | ||
| // altimate_change end | ||
| return Buffer.from(JSON.stringify(ctx)).toString("base64url") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MINOR — Documentation: the frontend contract is unspecified, and this payload is untrusted input on that side Nothing records what the consumer must do: decode base64url (not standard base64), require From the frontend's perspective a URL query param is attacker-suppliable — anyone can hand-craft one and load Fix: add JSDoc here stating the contract, and make sure the companion PR validates rather than trusts. |
||
| } | ||
|
|
||
| // altimate_change start — exported so tests can assert on the full URL shape | ||
| // without duplicating the construction logic. | ||
| export function buildAuthorizeUrl(webUrl: string, redirect: string, state: string): string { | ||
| return ( | ||
| `${webUrl}/register?client=altimate-code` + | ||
| `&redirect=${encodeURIComponent(redirect)}` + | ||
| `&state=${state}` + | ||
| `&cli_context=${encodeURIComponent(buildCliContext())}` | ||
| ) | ||
| } | ||
| // altimate_change end | ||
|
|
||
| // The one-time login_token is POSTed to the callback-supplied API base, so that | ||
| // base must be trusted — otherwise a crafted callback could exfiltrate the token | ||
| // to an attacker's server. Allow only HTTPS Altimate-owned hosts, an explicitly | ||
|
|
@@ -341,10 +416,7 @@ export async function AltimateAuthPlugin(_input: PluginInput): Promise<Hooks> { | |
| const redirect = `http://127.0.0.1:${boundPort}/callback` | ||
| // Land on the sign-up page and let the user choose how to authenticate | ||
| // (Google today, more providers later) rather than forcing Google. | ||
| const authorizeUrl = | ||
| `${webUrl}/register?client=altimate-code` + | ||
| `&redirect=${encodeURIComponent(redirect)}` + | ||
| `&state=${state}` | ||
| const authorizeUrl = buildAuthorizeUrl(webUrl, redirect, state) | ||
|
|
||
| // Try to open the browser. Failure is silent because the URL is | ||
| // already surfaced elsewhere: the auth dialog in packages/tui/src/ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,188 @@ | ||
| // altimate_change — tests for cli_context auth URL parameter | ||
| import { afterEach, beforeEach, describe, expect, test } from "bun:test" | ||
| import * as fs from "fs" | ||
| import * as os from "os" | ||
| import * as path from "path" | ||
| import { buildAuthorizeUrl, buildCliContext, getOrCreateMachineId } from "../../src/altimate/plugin/altimate" | ||
|
|
||
| describe("buildCliContext", () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MINOR — Testing: the integration point is untested All three tests exercise Delete the Fix: extract a |
||
| test("returns a valid base64url-encoded JSON blob with machine_id", () => { | ||
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "altimate-ctx-")) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MINOR — Testing: no failure-mode coverage, and the repo's temp-dir fixture is bypassed Untested paths, several of which carry the bugs flagged elsewhere in this review: telemetry opted out, permission-denied on an existing file, empty file (0 bytes), non-UUID or binary contents, oversized file, path-is-a-directory, and symlink. The fixture value Separately, this hand-rolls |
||
| const idPath = path.join(tmpDir, "machine-id") | ||
| fs.writeFileSync(idPath, "test-uuid-1234", "utf8") | ||
|
|
||
| const encoded = buildCliContext(idPath) | ||
|
|
||
| // base64url: only A-Z a-z 0-9 - _ (no +/=) | ||
| expect(encoded).toMatch(/^[A-Za-z0-9\-_]+$/) | ||
|
|
||
| const ctx = JSON.parse(Buffer.from(encoded, "base64url").toString("utf8")) as Record<string, unknown> | ||
| expect(ctx["v"]).toBe(1) | ||
| expect(ctx["machine_id"]).toBe("test-uuid-1234") | ||
| expect(typeof ctx["cli_version"]).toBe("string") | ||
|
|
||
| fs.rmSync(tmpDir, { recursive: true, force: true }) | ||
| }) | ||
|
|
||
| test("creates machine_id file when absent and includes it in context", () => { | ||
| // getOrCreateMachineId mints a UUID when the file is missing, so machine_id | ||
| // is always present (as a non-empty string) unless telemetry is disabled. | ||
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "altimate-ctx-new-")) | ||
| const nonExistentPath = path.join(tmpDir, "subdir", "machine-id") | ||
|
|
||
| const encoded = buildCliContext(nonExistentPath) | ||
| const ctx = JSON.parse(Buffer.from(encoded, "base64url").toString("utf8")) as Record<string, unknown> | ||
|
|
||
| expect(ctx["v"]).toBe(1) | ||
| // machine_id must be present and non-empty (newly minted UUID) | ||
| expect(typeof ctx["machine_id"]).toBe("string") | ||
| expect((ctx["machine_id"] as string).length).toBeGreaterThan(0) | ||
| // The same id must have been written to disk for telemetry to use | ||
| expect(fs.readFileSync(nonExistentPath, "utf8").trim()).toBe(ctx["machine_id"]) | ||
|
|
||
| fs.rmSync(tmpDir, { recursive: true, force: true }) | ||
| }) | ||
|
|
||
| test("trims whitespace from machine-id file", () => { | ||
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "altimate-ctx-ws-")) | ||
| const idPath = path.join(tmpDir, "machine-id") | ||
| // Many editors/tools write a trailing newline | ||
| fs.writeFileSync(idPath, " trimmed-uuid \n", "utf8") | ||
|
|
||
| const encoded = buildCliContext(idPath) | ||
| const ctx = JSON.parse(Buffer.from(encoded, "base64url").toString("utf8")) as Record<string, unknown> | ||
|
|
||
| expect(ctx["machine_id"]).toBe("trimmed-uuid") | ||
|
|
||
| fs.rmSync(tmpDir, { recursive: true, force: true }) | ||
| }) | ||
|
|
||
| describe("telemetry opt-out", () => { | ||
| let savedEnv: string | undefined | ||
|
|
||
| beforeEach(() => { | ||
| savedEnv = process.env.ALTIMATE_TELEMETRY_DISABLED | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| if (savedEnv === undefined) delete process.env.ALTIMATE_TELEMETRY_DISABLED | ||
| else process.env.ALTIMATE_TELEMETRY_DISABLED = savedEnv | ||
| }) | ||
|
|
||
| test("omits machine_id key when ALTIMATE_TELEMETRY_DISABLED=true", () => { | ||
| process.env.ALTIMATE_TELEMETRY_DISABLED = "true" | ||
|
|
||
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "altimate-ctx-disabled-")) | ||
| const idPath = path.join(tmpDir, "machine-id") | ||
| fs.writeFileSync(idPath, "should-not-appear", "utf8") | ||
|
|
||
| const encoded = buildCliContext(idPath) | ||
| const ctx = JSON.parse(Buffer.from(encoded, "base64url").toString("utf8")) as Record<string, unknown> | ||
|
|
||
| // machine_id must be absent when telemetry is disabled | ||
| expect(Object.prototype.hasOwnProperty.call(ctx, "machine_id")).toBe(false) | ||
| expect(ctx["v"]).toBe(1) | ||
|
|
||
| fs.rmSync(tmpDir, { recursive: true, force: true }) | ||
| }) | ||
|
|
||
| test("includes machine_id when ALTIMATE_TELEMETRY_DISABLED is not set", () => { | ||
| delete process.env.ALTIMATE_TELEMETRY_DISABLED | ||
|
|
||
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "altimate-ctx-enabled-")) | ||
| const idPath = path.join(tmpDir, "machine-id") | ||
| fs.writeFileSync(idPath, "expected-uuid", "utf8") | ||
|
|
||
| const encoded = buildCliContext(idPath) | ||
| const ctx = JSON.parse(Buffer.from(encoded, "base64url").toString("utf8")) as Record<string, unknown> | ||
|
|
||
| expect(ctx["machine_id"]).toBe("expected-uuid") | ||
|
|
||
| fs.rmSync(tmpDir, { recursive: true, force: true }) | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| describe("getOrCreateMachineId", () => { | ||
| test("returns existing id when file is present", () => { | ||
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "altimate-mid-")) | ||
| const idPath = path.join(tmpDir, "machine-id") | ||
| fs.writeFileSync(idPath, "existing-uuid\n", "utf8") | ||
|
|
||
| expect(getOrCreateMachineId(idPath)).toBe("existing-uuid") | ||
|
|
||
| fs.rmSync(tmpDir, { recursive: true, force: true }) | ||
| }) | ||
|
|
||
| test("creates a UUID file when absent and returns it", () => { | ||
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "altimate-mid-create-")) | ||
| const idPath = path.join(tmpDir, "subdir", "machine-id") | ||
|
|
||
| const id = getOrCreateMachineId(idPath) | ||
|
|
||
| // Must be a non-empty string that was written to disk | ||
| expect(typeof id).toBe("string") | ||
| expect(id.length).toBeGreaterThan(0) | ||
| expect(fs.readFileSync(idPath, "utf8").trim()).toBe(id) | ||
|
|
||
| fs.rmSync(tmpDir, { recursive: true, force: true }) | ||
| }) | ||
|
|
||
| test("two concurrent callers with absent file converge on the same id", async () => { | ||
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "altimate-mid-race-")) | ||
| const idPath = path.join(tmpDir, "machine-id") | ||
|
|
||
| // Simulate a race: call getOrCreateMachineId twice before either has written | ||
| const [id1, id2] = await Promise.all([ | ||
| Promise.resolve(getOrCreateMachineId(idPath)), | ||
| Promise.resolve(getOrCreateMachineId(idPath)), | ||
| ]) | ||
|
|
||
| expect(id1).toBe(id2) | ||
|
|
||
| fs.rmSync(tmpDir, { recursive: true, force: true }) | ||
| }) | ||
| }) | ||
|
|
||
| describe("buildAuthorizeUrl", () => { | ||
| test("URL contains cli_context param that decodes to valid JSON", () => { | ||
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "altimate-auth-url-")) | ||
| const idPath = path.join(tmpDir, "machine-id") | ||
| fs.writeFileSync(idPath, "url-test-uuid", "utf8") | ||
|
|
||
| // Temporarily override machine-id path via a patched buildCliContext call | ||
| // by providing a custom machine_id file — buildAuthorizeUrl uses buildCliContext() | ||
| // internally with no path arg, so test the URL shape via the exported helper. | ||
| const url = buildAuthorizeUrl("https://app.myaltimate.com", "http://127.0.0.1:7317/callback", "test-state-abc") | ||
|
|
||
| // Must contain cli_context query param | ||
| expect(url).toContain("cli_context=") | ||
| expect(url).toContain("client=altimate-code") | ||
| expect(url).toContain("state=test-state-abc") | ||
| expect(url).toContain("redirect=") | ||
|
|
||
| // Extract and decode cli_context | ||
| const parsed = new URL(url) | ||
| const encoded = parsed.searchParams.get("cli_context") | ||
| expect(encoded).toBeTruthy() | ||
|
|
||
| const ctx = JSON.parse(Buffer.from(encoded!, "base64url").toString("utf8")) as Record<string, unknown> | ||
| expect(ctx["v"]).toBe(1) | ||
| expect(typeof ctx["cli_version"]).toBe("string") | ||
| // machine_id may or may not be present depending on env, but if present must be a string | ||
| if (Object.prototype.hasOwnProperty.call(ctx, "machine_id")) { | ||
| expect(typeof ctx["machine_id"]).toBe("string") | ||
| expect((ctx["machine_id"] as string).length).toBeGreaterThan(0) | ||
| } | ||
|
|
||
| fs.rmSync(tmpDir, { recursive: true, force: true }) | ||
| }) | ||
|
|
||
| test("deleting cli_context line would cause cli_context param to be absent — URL integration is guarded", () => { | ||
| // This test asserts that buildAuthorizeUrl really does embed cli_context. | ||
| // If the &cli_context=... line were removed from buildAuthorizeUrl, this test fails. | ||
| const url = buildAuthorizeUrl("https://app.myaltimate.com", "http://127.0.0.1:7317/callback", "state-xyz") | ||
| const parsed = new URL(url) | ||
| expect(parsed.searchParams.has("cli_context")).toBe(true) | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MINOR — Code Quality: the catch and its log describe a narrower failure than they handle
The bare
catchswallowsEACCES,EISDIR,ELOOP,ENOTDIRand I/O errors, but logs "machine-id file not found". It also says it "will omit machine_id" when it in fact sends"". Someone debugging a missing correlation caused by a permissions problem is actively misled.Fix:
Non-
ENOENTcodes indicate a real local problem and deserve more thandebug.