feat: optional OneCLI/MITM proxy credential routing - #17
Conversation
Gate proxy + placeholder-credential behavior on ONECLI_URL so non-OneCLI runs stay byte-for-byte identical. When set (via `onecli run`): - install undici ProxyAgent for global fetch (Reclaim, TripIt, Telegram) - fetch TripIt iCal via proxied fetch instead of node-ical fromURL - Google Calendar uses a static Bearer with no OAuth self-refresh - accept placeholder TripIt/Reclaim credentials for gateway injection - OOO opt-in via ENABLE_OOO or GOOGLE_* placeholder presence Adds undici, lib/proxy.mjs, tests for proxy routing / Google bypass / placeholder acceptance, and README/.env.example docs.
|
@copilot review this pull request |
jbaruch
left a comment
There was a problem hiding this comment.
Requesting Copilot review
There was a problem hiding this comment.
Pull request overview
Adds an opt-in OneCLI / MITM credential-gateway mode (gated by ONECLI_URL) so outbound HTTP can be routed through a proxy for credential injection, while keeping the non-OneCLI path unchanged.
Changes:
- Installs an undici
ProxyAgentdispatcher (only in OneCLI mode) so globalfetchrespectsHTTPS_PROXY, with optional CA plumbing viaNODE_EXTRA_CA_CERTS. - Updates TripIt iCal fetching to use proxied global
fetchunder OneCLI; updates Google Calendar client creation to avoid OAuth refresh and use a static Bearer placeholder under OneCLI. - Adds unit tests and documentation for OneCLI mode and placeholder credential behavior.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sync.mjs | Early proxy dispatcher install; reuse a single Google Calendar client; OneCLI-specific error messaging around OOO sync. |
| README.md | Documents OneCLI mode usage, proxy/CA behavior, placeholder credential injection, and OOO opt-in rules. |
| package.json | Adds undici dependency for ProxyAgent / global dispatcher support. |
| package-lock.json | Locks undici dependency and records its Node engine requirement. |
| lib/tripit.mjs | Uses proxied global fetch + parseICS in OneCLI mode to avoid node-ical’s non-proxied URL fetching. |
| lib/proxy.mjs | New module to detect OneCLI mode and install/reset the global undici proxy dispatcher (with optional CA loading). |
| lib/proxy.test.mjs | New tests for OneCLI gating, fail-fast behavior, CA handling, and proxy routing for Reclaim + TripIt. |
| lib/google-calendar.mjs | OneCLI-mode static Bearer auth path + new isOooOptedIn helper; preserves OAuth refresh path outside OneCLI. |
| lib/google-calendar.test.mjs | Expands tests to cover OneCLI opt-in behavior and verify no OAuth token refresh network calls occur. |
| CHANGELOG.md | Notes the new optional OneCLI credential-gateway mode and behavior changes when gated on ONECLI_URL. |
| .env.example | Adds OneCLI-related env var documentation and OOO opt-in examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "geo-tz": "^8.1.1", | ||
| "googleapis": "^171.4.0", | ||
| "node-ical": "^0.20.1" | ||
| "node-ical": "^0.20.1", | ||
| "undici": "^8.7.0" | ||
| }, |
There was a problem hiding this comment.
Fixed in 7feebc9 — declared "engines": { "node": ">=22.19.0" } to match undici@8. Tried pinning undici 7 for broader Node 22, but its ProxyAgent hung the mock-proxy tests; staying on undici 8 + engines is the cleaner contract (Dockerfile already uses node:22-alpine).
| try { | ||
| oooStats = await syncOooEvents(client, gcal, futureTrips); | ||
| } catch (err) { | ||
| // Surface OneCLI Google-connection misconfig clearly (gateway 401 | ||
| // when the built-in Google Calendar connection isn't authorized). | ||
| if (process.env.ONECLI_URL && /401|unauthorized|invalid.?credential/i.test(err.message)) { | ||
| throw new Error( | ||
| `Google Calendar OOO failed under OneCLI (${err.message}). ` + | ||
| 'Configure and authorize the OneCLI Google Calendar connection ' + | ||
| '(`onecli apps configure`) so the gateway can inject a real Bearer, ' + | ||
| 'or set ENABLE_OOO=0 to skip OOO.', | ||
| ); | ||
| } | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
Fixed in 7feebc9 — safe msg extraction (err instanceof Error ? err.message : String(err)) and rethrow with { cause: err } so non-Error throws don't get swallowed as a TypeError on .message.
Review complete. The implementation is well-structured and the gate on One fix applied (commit
Other observations (no action needed):
CodeQL scan also came back clean. |
- Declare engines.node >=22.19.0 to match undici@8's requirement (node:22-alpine consumers need a recent 22.x; undici 7 hung proxy tests) - Harden OneCLI OOO error wrapping: safe message extraction and Error cause
| // Spins up a local HTTP "origin" server and a local HTTP CONNECT-capable | ||
| // proxy. With ONECLI_URL + HTTPS_PROXY pointing at the mock proxy, undici's | ||
| // ProxyAgent must forward the request — we assert the proxy saw it and | ||
| // the origin saw the expected placeholder credentials. |
There was a problem hiding this comment.
Fixed in c3d94d2 — corrected the suite header and split the suites: HTTP forward (absolute-form) vs HTTPS CONNECT. The CONNECT suite now stands up an https:// origin with an ephemeral self-signed cert (SAN=IP:127.0.0.1), a CONNECT-tunnel mock proxy, and asserts both the CONNECT target and the placeholder Bearer on the origin.
| // Minimal HTTP forward proxy: rewrites absolute-form request-target | ||
| // to origin-form and relays to the real origin. Enough for undici | ||
| // ProxyAgent's non-CONNECT (http://) path. | ||
| proxyServer = http.createServer((req, res) => { |
There was a problem hiding this comment.
Fixed in c3d94d2 — added proxy routing under OneCLI mode (HTTPS CONNECT) covering Reclaim + TripIt over https:// via CONNECT, with the self-signed origin cert trusted through NODE_EXTRA_CA_CERTS / requestTls.ca. Production-relevant path is now exercised; 184 tests green.
Address Copilot re-review: the mock proxy was documented as CONNECT-capable but only handled HTTP absolute-form. Split HTTP-forward vs HTTPS-CONNECT suites and assert CONNECT tunnels for https:// Reclaim/TripIt origins with an ephemeral self-signed cert trusted via NODE_EXTRA_CA_CERTS.
| execFileSync('openssl', [ | ||
| 'req', '-x509', '-newkey', 'rsa:2048', | ||
| '-keyout', keyPath, '-out', certPath, | ||
| '-days', '1', '-nodes', | ||
| '-subj', '/CN=127.0.0.1', | ||
| '-addext', 'subjectAltName=IP:127.0.0.1,DNS:localhost', | ||
| ], { stdio: 'pipe' }); | ||
| const key = readFileSync(keyPath); | ||
| const cert = readFileSync(certPath); |
There was a problem hiding this comment.
Fixed in 13179df — openssl spawn failures (ENOENT / missing binary) now throw an actionable message that the HTTPS CONNECT suite needs openssl on PATH; other openssl failures include stderr detail.
| // Install undici ProxyAgent before any HTTP when running under OneCLI | ||
| // (`ONECLI_URL` set by `onecli run`). No-op otherwise — keeps non-OneCLI | ||
| // behavior byte-for-byte identical. Must be the first side-effect import. | ||
| import { installProxyDispatcher } from './lib/proxy.mjs'; | ||
| installProxyDispatcher(); |
There was a problem hiding this comment.
Fixed in 13179df — rewrote the comment to state the ESM truth: all static imports evaluate before the module body, so install is the explicit installProxyDispatcher() call (safe only because lib/* do no HTTP at load time).
Address Copilot review on c3d94d2: - Document that ESM evaluates all static imports before the module body, so proxy install is the explicit call (not import order) - Surface a clear error when openssl is missing for HTTPS CONNECT tests
| /** | ||
| * Whether OOO / Google Calendar is opted in under the current env. | ||
| * Exported for sync.mjs dry-run messaging. | ||
| */ |
There was a problem hiding this comment.
Fixed in d42b596 — JSDoc now says used by createGCalClient and sync skip messaging (no false claim about dry-run importing it).
| if (!gcal) { | ||
| console.log('\n OOO blocks: skipped (Google Calendar credentials not configured)'); | ||
| } else { |
There was a problem hiding this comment.
Fixed in d42b596 — sync and dry-run both use oooSkipReason() so OneCLI operators see ENABLE_OOO / placeholder guidance instead of "credentials not configured".
| if (futureTrips.length > 0 && gcal) { | ||
| console.log(`\n── OOO blocks (would create) ──`); | ||
| for (const t of futureTrips) { | ||
| console.log(` ${OOO_PREFIX}${t.summary} ${t.startDate} → ${t.endDate}`); |
There was a problem hiding this comment.
Fixed in d42b596 — same as the sync path; dry-run now uses oooSkipReason(googleCreds).
Copilot: dry-run/sync said "credentials not configured" even when ONECLI_URL is set and OOO simply wasn't opted in. Add oooSkipReason() and use it for both skip paths; fix isOooOptedIn JSDoc.
| // OneCLI mode: install undici ProxyAgent before any HTTP. | ||
| // ESM evaluates all static imports (the whole dependency graph) before this | ||
| // module body runs, so import order does NOT control when the agent is | ||
| // installed — the explicit call below does. That is safe because lib/* | ||
| // only export functions; they do not perform HTTP at load time. Any future | ||
| // top-level HTTP in an imported module would race this and break OneCLI. | ||
| // No-op when ONECLI_URL is unset (byte-for-byte identical non-OneCLI path). | ||
| import { installProxyDispatcher } from './lib/proxy.mjs'; | ||
| installProxyDispatcher(); |
There was a problem hiding this comment.
Fixed in fba32c1 — reworded: no-op is about HTTP routing (no ProxyAgent / dispatcher swap), not zero module-graph difference; undici may still load via the import.
| try { | ||
| execFileSync('openssl', [ | ||
| 'req', '-x509', '-newkey', 'rsa:2048', | ||
| '-keyout', keyPath, '-out', certPath, | ||
| '-days', '1', '-nodes', | ||
| '-subj', '/CN=127.0.0.1', | ||
| '-addext', 'subjectAltName=IP:127.0.0.1,DNS:localhost', | ||
| ], { stdio: 'pipe' }); | ||
| } catch (err) { | ||
| const detail = err.stderr?.toString?.() || err.message || String(err); | ||
| const missing = err.code === 'ENOENT' | ||
| || /not found|No such file|spawn openssl/i.test(detail); | ||
| throw new Error( |
There was a problem hiding this comment.
Fixed in fba32c1 — preflight openssl version; suite uses describe.skip when openssl is missing so minimal environments skip cleanly instead of hard-failing.
- Skip HTTPS CONNECT suite when openssl is not on PATH (describe.skip) - Clarify that OneCLI off means no dispatcher swap, not zero module-graph change
| const msg = err instanceof Error ? err.message : String(err); | ||
| if (process.env.ONECLI_URL && /401|unauthorized|invalid.?credential/i.test(msg)) { | ||
| throw new Error( | ||
| `Google Calendar OOO failed under OneCLI (${msg}). ` + | ||
| 'Configure and authorize the OneCLI Google Calendar connection ' + | ||
| '(`onecli apps configure`) so the gateway can inject a real Bearer, ' + | ||
| 'or set ENABLE_OOO=0 to skip OOO.', | ||
| { cause: err }, | ||
| ); | ||
| } | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
Fixed in 9cef7da — non-Error throws are wrapped in new Error(msg, { cause }) before rethrow; top-level fatal path also uses safe message extraction.
| const flag = process.env.ENABLE_OOO; | ||
| if (flag === '0' || flag === 'false') return false; | ||
| if (flag === '1' || flag === 'true') return true; |
There was a problem hiding this comment.
Fixed in 9cef7da — shared enableOooFlag() lowercases ENABLE_OOO; TRUE/FALSE/yes/no/on/off work.
| if (process.env.ONECLI_URL) { | ||
| const flag = process.env.ENABLE_OOO; | ||
| if (flag === '0' || flag === 'false') { | ||
| return 'OOO disabled under OneCLI (ENABLE_OOO=0)'; | ||
| } | ||
| return 'OOO not opted in under OneCLI (set ENABLE_OOO=1, or set GOOGLE_* placeholders as opt-in markers)'; | ||
| } | ||
| return 'Google Calendar credentials not configured'; |
There was a problem hiding this comment.
Fixed in 9cef7da — oooSkipReason uses the same case-insensitive enableOooFlag().
- ENABLE_OOO accepts TRUE/FALSE/etc. case-insensitively in opt-in + skip reason - OOO catch and top-level fatal path always surface a string message
| const caPath = process.env.NODE_EXTRA_CA_CERTS; | ||
| if (caPath) { | ||
| try { | ||
| const ca = readFileSync(caPath); | ||
| agentOpts.requestTls = { ca, rejectUnauthorized: true }; | ||
| agentOpts.proxyTls = { ca, rejectUnauthorized: true }; |
There was a problem hiding this comment.
Fixed in b19be30 — ca: [...tls.rootCertificates, extraCa] so we append like NODE_EXTRA_CA_CERTS, not replace the default trust store.
NODE_EXTRA_CA_CERTS appends at Node startup; undici requestTls.ca replaces. Append extra CA to tls.rootCertificates so public roots stay available.
| import { installProxyDispatcher } from './lib/proxy.mjs'; | ||
| installProxyDispatcher(); |
There was a problem hiding this comment.
Fixed in 3871080 — installProxyDispatcher() now runs inside the main try after CLI/output parsing, so missing HTTPS_PROXY flows through the same fatal path (including --output=json). Verified smoke: errors array contains the actionable message.
Move installProxyDispatcher() after CLI/output setup into the main try so ONECLI_URL without HTTPS_PROXY exits via --output=json shaping.
| it('reads NODE_EXTRA_CA_CERTS into requestTls when the file exists', () => { | ||
| const dir = mkdtempSync(join(tmpdir(), 'onecli-ca-')); | ||
| const caPath = join(dir, 'ca.pem'); | ||
| // Minimal PEM-looking blob — ProxyAgent only needs to read the bytes | ||
| writeFileSync(caPath, '-----BEGIN CERTIFICATE-----\nMIIB\n-----END CERTIFICATE-----\n'); | ||
| process.env.ONECLI_URL = 'http://127.0.0.1:9999'; | ||
| process.env.HTTPS_PROXY = 'http://127.0.0.1:8888'; | ||
| process.env.NODE_EXTRA_CA_CERTS = caPath; | ||
| const agent = installProxyDispatcher(); | ||
| assert.ok(agent instanceof ProxyAgent); | ||
| unlinkSync(caPath); | ||
| }); |
There was a problem hiding this comment.
Fixed in 8f0714c — temp CA dir is rmSync’d in a finally block (whole dir, not just the PEM).
rmSync the whole mkdtemp dir in finally so /tmp is not left littered.
| it('routes Reclaim listEntries through the proxy with a placeholder Bearer', async () => { | ||
| // listEntries hardcodes api.app.reclaim.ai; we exercise the same client | ||
| // + dispatcher path with a fetch that targets the mock origin. | ||
| const client = createClient('PLACEHOLDER-RECLAIM-TOKEN'); |
There was a problem hiding this comment.
Fixed in b4dc65c — renamed to “Reclaim-style global fetch”; comment explains why we cannot call listEntries directly without a base-URL seam.
| * @returns {ProxyAgent|null} the installed agent, or null when not in OneCLI mode | ||
| * @throws {Error} when ONECLI_URL is set but HTTPS_PROXY is missing | ||
| */ |
- Rename test to say global fetch, not listEntries (no base-URL seam) - Document NODE_EXTRA_CA_CERTS read failures in installProxyDispatcher @throws
Summary
ONECLI_URL(set byonecli run)ProxyAgentfor globalfetch, TripIt iCal via proxied fetch, Google Calendar static Bearer with no OAuth self-refresh, placeholder TripIt/Reclaim credentials accepted for gateway injectionONECLI_URLis unset: behavior is unchanged; existing tests pass as beforeCloses #16.
Test plan
npm test— 182 pass (proxy routing, placeholder acceptance, Google no-refresh, non-OneCLI regression)ONECLI_URLunset: dry-run/sync behave as todayonecli run: TripIt + Reclaim requests hit the gateway; placeholders swap correctlyENABLE_OOO=1: OOO works without local OAuth refreshENABLE_OOO=0skips OOO)