chore(telemetry): route events through proxy#478
Open
edgarsskore wants to merge 3 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaced Google Analytics event submission with a telemetry-proxy-based approach across the core capture layer and three installation/setup/uninstall scripts. Each endpoint now sends bearer-authenticated POST requests through a primary proxy with automatic fallback to a secondary endpoint on failure, using 5-second timeouts and HTTP 2xx success detection. ChangesTelemetry proxy infrastructure refactoring
Sequence DiagramsequenceDiagram
participant Client as capture_call_tool / capture / capture_ui_event
participant Builder as buildEventProperties
participant Sender as sendToTelemetryProxy
participant Poster as postTelemetryPayload
participant Primary as Primary_Proxy
participant Fallback as Fallback_Proxy
Client->>Builder: build event properties
Client->>Sender: request send
Sender->>Poster: POST primary with Bearer token (5s timeout)
Poster->>Primary: HTTPS POST
alt Primary 2xx
Primary-->>Poster: 2xx
Poster-->>Sender: success
else Primary fails
Poster->>Fallback: POST fallback with Bearer token (5s timeout)
alt Fallback 2xx
Fallback-->>Poster: 2xx
Poster-->>Sender: success
else
Poster-->>Sender: failure
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@setup-claude-server.js`:
- Around line 302-308: The trackEvent function currently only checks
TELEMETRY_PROXY_TOKEN and ignores the user's persisted telemetry opt-out, so
rerunning setup still emits telemetry; update trackEvent (and call sites like
addSetupStep usage) to respect the persisted telemetryEnabled flag by reading
the saved config before sending events (e.g., load the config or accept a
telemetryEnabled param), returning early if telemetryEnabled is false; ensure
this check occurs before emitting early/initial events (before initConfigFile
runs) by loading the minimal persisted setting or threading telemetryEnabled
into setup startup so trackEvent can short-circuit when the user has opted out.
- Around line 365-399: The timeout is being cleared on the 'response' event
which cancels the 5s global deadline once headers arrive and allows a stalled
body to hang postTelemetryPayload(); in postTelemetryPayload(), keep timeoutId
active until the full body is received or an error occurs by moving
clearTimeout(timeoutId) from the 'response' handler into the 'end' handler (and
also call clearTimeout in the 'error' handler), or alternatively replace the
manual setTimeout with req.setTimeout(...) so the timeout covers the entire
request/response exchange; update the handlers in postTelemetryPayload()
accordingly so trackEvent() cannot hang past the 5s deadline.
In `@src/utils/capture.ts`:
- Around line 20-22: TELEMETRY_PROXY_TOKEN is a secret and must not be
hardcoded; remove the TELEMETRY_PROXY_TOKEN constant and stop embedding it in
client code and install/uninstall scripts, and instead have the client call your
backend (or an authenticated service endpoint) at
TELEMETRY_PROXY_URL/TELEMETRY_PROXY_FALLBACK_URL where the server attaches the
bearer token or performs trusted forwarding. Update the capture logic that
references TELEMETRY_PROXY_TOKEN to call the server-side relay (or fetch a
short-lived token from your backend) and add robust fallbacks/error handling
when no token is available; also rotate and remove the duplicated token from any
setup/install/uninstall scripts so the secret only lives on the server.
- Around line 421-437: The capture APIs (capture_call_tool, capture,
capture_ui_event) must be non-blocking and must not throw: change each to
fire-and-forget the telemetry work instead of awaiting buildEventProperties() or
sendToTelemetryProxy() on the caller path; inside the background task call
buildEventProperties(...) and sendToTelemetryProxy(...) but wrap them in a
try/catch to swallow/log errors so no exception escapes (use the existing
function names buildEventProperties and sendToTelemetryProxy), and ensure the
exported functions return immediately (do not await the background promise).
In `@track-installation.js`:
- Around line 324-350: The timeout in postTelemetryPayload is cleared on the
'response' event which can leave the promise unresolved if the body never
finishes; keep the 5s deadline running until the response fully completes and
only clear the timeout when 'end' or 'close' (or on 'error'), destroy the
request inside the timeout handler, and ensure the promise is resolved (false)
on timeout/close/error; update the event handlers for req and res
(req.on('error'), req.on('close'), res.on('end')) to clear timeout and resolve
appropriately while leaving timeoutId active until those terminal events.
In `@uninstall-claude-server.js`:
- Around line 279-313: The postTelemetryPayload function currently clears the 5s
timeout on the 'response' event, which allows a server to send headers and then
stall the body indefinitely; change the logic so the timeout enforces the full
response-body window: do not clear the timeout in the 'response' handler,
instead clear it only when the response 'end' fires (and on request/response
'error'), and ensure the timeout still destroys the request and resolves with
{success:false} when triggered; update related handlers in postTelemetryPayload
(and be mindful of TELEMETRY_PROXY_URL / TELEMETRY_PROXY_FALLBACK_URL and
callers like trackEvent) so the promise always resolves after the body timeout.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f5e3cef-3a8a-4d16-acd9-c276c15348e5
📒 Files selected for processing (5)
setup-claude-server.jssrc/server.tssrc/utils/capture.tstrack-installation.jsuninstall-claude-server.js
Comment on lines
302
to
308
| async function trackEvent(eventName, additionalProps = {}) { | ||
| const trackingStep = addSetupStep(`track_event_${eventName}`); | ||
|
|
||
| if (!GA_MEASUREMENT_ID || !GA_API_SECRET) { | ||
| if (!TELEMETRY_PROXY_TOKEN) { | ||
| updateSetupStep(trackingStep, 'skipped', new Error('GA not configured')); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Honor the existing telemetry opt-out in setup.
This guard only checks whether a proxy token exists. Setup never carries the user's persisted telemetryEnabled flag into trackEvent(), so rerunning setup after opting out still emits setup telemetry, including the early start events before initConfigFile() runs.
🤖 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 `@setup-claude-server.js` around lines 302 - 308, The trackEvent function
currently only checks TELEMETRY_PROXY_TOKEN and ignores the user's persisted
telemetry opt-out, so rerunning setup still emits telemetry; update trackEvent
(and call sites like addSetupStep usage) to respect the persisted
telemetryEnabled flag by reading the saved config before sending events (e.g.,
load the config or accept a telemetryEnabled param), returning early if
telemetryEnabled is false; ensure this check occurs before emitting
early/initial events (before initConfigFile runs) by loading the minimal
persisted setting or threading telemetryEnabled into setup startup so trackEvent
can short-circuit when the user has opted out.
Comment on lines
+20
to
22
| const TELEMETRY_PROXY_URL = 'https://telemetry.desktopcommander.app/mp/collect'; | ||
| const TELEMETRY_PROXY_FALLBACK_URL = 'https://dc-telemetry-proxy-83847352264.europe-west1.run.app/mp/collect'; | ||
| const TELEMETRY_PROXY_TOKEN = 'Od44UB_fTrVfGPGRPLr5QdVgFhuKdiGaBmvazTdxVdQ'; |
Contributor
There was a problem hiding this comment.
Do not ship the proxy bearer in client code.
Line 22 embeds the authorization secret in source that is published to every user, and the same token is duplicated in the setup/install/uninstall scripts. That makes the proxy effectively public: anyone can replay this bearer to inject arbitrary telemetry or burn backend quota. This needs a server-side trust model instead of a shared static secret baked into the client.
🧰 Tools
🪛 Betterleaks (1.2.0)
[high] 22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 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 `@src/utils/capture.ts` around lines 20 - 22, TELEMETRY_PROXY_TOKEN is a secret
and must not be hardcoded; remove the TELEMETRY_PROXY_TOKEN constant and stop
embedding it in client code and install/uninstall scripts, and instead have the
client call your backend (or an authenticated service endpoint) at
TELEMETRY_PROXY_URL/TELEMETRY_PROXY_FALLBACK_URL where the server attaches the
bearer token or performs trusted forwarding. Update the capture logic that
references TELEMETRY_PROXY_TOKEN to call the server-side relay (or fetch a
short-lived token from your backend) and add robust fallbacks/error handling
when no token is available; also rotate and remove the duplicated token from any
setup/install/uninstall scripts so the secret only lives on the server.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
setup-claude-server.js (1)
13-16:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't ship the proxy bearer token in the client scripts.
This token is embedded in a public setup script, so anyone who installs or inspects the package can reuse it to send arbitrary traffic through the telemetry proxy. That makes the proxy authentication effectively public, and the same literal is duplicated in the install/uninstall scripts too. Move authentication to a server-controlled boundary or switch to a non-secret client mechanism.
🤖 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 `@setup-claude-server.js` around lines 13 - 16, The TELEMETRY_PROXY_TOKEN constant is a secret and must not be embedded in client-install scripts; remove the TELEMETRY_PROXY_TOKEN literal from the public setup script and any duplicated install/uninstall scripts, and instead have the client call a server-side endpoint (or use a non-secret client-side mechanism) that injects or brokers authenticated requests to the telemetry proxy; update any references to TELEMETRY_PROXY_TOKEN to call the new server endpoint (or use a publicly safe token flow) so no bearer token is shipped in client code.track-installation.js (1)
46-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor the persisted telemetry opt-out before sending install events.
getClientId()already reads the existing config, but it discardstelemetryEnabled, andtrackInstallation()only checks for a proxy token. That means reinstalls/updates still emitpackage_installedafter a user has opted out. Please thread the saved flag through this path and return early when it isfalse.Suggested direction
-async function getClientId() { +async function getTelemetrySettings() { try { const { homedir } = await import('os'); const { join } = await import('path'); const fs = await import('fs'); @@ if (fs.existsSync(CONFIG_FILE)) { const configData = fs.readFileSync(CONFIG_FILE, 'utf8'); const config = JSON.parse(configData); - if (config.clientId) { - debug(`Using existing clientId from config: ${config.clientId.substring(0, 8)}...`); - return config.clientId; - } + return { + clientId: config.clientId || randomUUID(), + telemetryEnabled: config.telemetryEnabled !== false + }; } @@ - return randomUUID(); + return { clientId: randomUUID(), telemetryEnabled: true }; } catch (error) { debug(`Error reading config file: ${error.message}, using random UUID`); - return randomUUID(); + return { clientId: randomUUID(), telemetryEnabled: true }; } } @@ - const uniqueUserId = await getClientId(); + const { clientId: uniqueUserId, telemetryEnabled } = await getTelemetrySettings(); + if (!telemetryEnabled) { + debug('Telemetry disabled, skipping installation tracking'); + return; + }Also applies to: 272-299
🤖 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 `@track-installation.js` around lines 46 - 74, getClientId currently reads config.json but ignores telemetryEnabled; update getClientId to return both clientId and telemetryEnabled (or a structure indicating opt-out) by reading telemetryEnabled from CONFIG_FILE (same place you read clientId) and propagating it to callers, and then modify trackInstallation to inspect the returned telemetryEnabled and return early (no network/event emit) when telemetryEnabled is false; reference getClientId, trackInstallation, CONFIG_FILE and telemetryEnabled when making the changes so the saved opt-out is honored.
🤖 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.
Outside diff comments:
In `@setup-claude-server.js`:
- Around line 13-16: The TELEMETRY_PROXY_TOKEN constant is a secret and must not
be embedded in client-install scripts; remove the TELEMETRY_PROXY_TOKEN literal
from the public setup script and any duplicated install/uninstall scripts, and
instead have the client call a server-side endpoint (or use a non-secret
client-side mechanism) that injects or brokers authenticated requests to the
telemetry proxy; update any references to TELEMETRY_PROXY_TOKEN to call the new
server endpoint (or use a publicly safe token flow) so no bearer token is
shipped in client code.
In `@track-installation.js`:
- Around line 46-74: getClientId currently reads config.json but ignores
telemetryEnabled; update getClientId to return both clientId and
telemetryEnabled (or a structure indicating opt-out) by reading telemetryEnabled
from CONFIG_FILE (same place you read clientId) and propagating it to callers,
and then modify trackInstallation to inspect the returned telemetryEnabled and
return early (no network/event emit) when telemetryEnabled is false; reference
getClientId, trackInstallation, CONFIG_FILE and telemetryEnabled when making the
changes so the saved opt-out is honored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45e46923-e338-4c97-9fd6-8e646308c210
📒 Files selected for processing (3)
setup-claude-server.jstrack-installation.jsuninstall-claude-server.js
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
1 participant
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 by CodeRabbit