Skip to content

updated auth#42

Draft
sreedharsreeram wants to merge 2 commits into
mainfrom
feat/browser-auth-flow
Draft

updated auth#42
sreedharsreeram wants to merge 2 commits into
mainfrom
feat/browser-auth-flow

Conversation

@sreedharsreeram
Copy link
Copy Markdown
Member

No description provided.

@sreedharsreeram sreedharsreeram requested a review from Dhravya April 29, 2026 01:54
Copy link
Copy Markdown
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

The new console.supermemory.ai URL is longer than the previous
app.supermemory.ai URL, pushing line 15 over Biome's line-length
limit. This introduced a second formatter error (up from the
pre-existing baseline of 1 error / 4 warnings).

Split the OR expression so the URL string sits on its own
continuation line, exactly as Biome prescribes.

After fix: npm run lint → 1 error / 4 warnings (baseline restored).
Copy link
Copy Markdown
Member

@Dhravya Dhravya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing Review — PR #42 updated auth

Test agent findings — all 7 changed behaviours verified. One bug found and fixed.


🐛 Bug found & fixed (commit fbfb542)

Biome formatting regression in src/lib/auth.js

npm run lint reported 2 errors after this PR — up from the documented pre-existing baseline of 1 error / 4 warnings. The new console.supermemory.ai/auth/agent-connect URL is longer than the old app.supermemory.ai/auth/connect one, pushing line 15 over Biome's line-length limit.

Before (as submitted):

const AUTH_BASE_URL =
  process.env.SUPERMEMORY_AUTH_URL || 'https://console.supermemory.ai/auth/agent-connect';

Fixed (commit fbfb542):

const AUTH_BASE_URL =
  process.env.SUPERMEMORY_AUTH_URL ||
  'https://console.supermemory.ai/auth/agent-connect';

After fix: npm run lint → 1 error / 4 warnings ✅ (baseline restored).


✅ Test results — 41 / 41 passed

Change Tests Result
AUTH_BASE_URL → console.supermemory.ai/auth/agent-connect 3
Ephemeral port (server.listen(0), old fixed 19876 gone) 3
CSRF state token (wrong/missing → 403, correct → 200/400, both param names) 7
AUTH_TIMEOUT default 60000, configurable via env var 7
clearTimeout(timer) on success AND error paths 3
Additional URL params (hostname, os, cwd, cli_version) 8
client identifier claude-code (hyphen, not underscore) 2
Bonus: CSRF uniqueness, AUTH_PORT/19876 fully removed 5

Build: npm run build → 5 hooks, all clean.
Smoke test: Live bundle emits correct console.supermemory.ai URL with all 6 params and a fresh CSRF state token embedded in the callback param.


⚠️ Minor edge case (non-blocking)

SUPERMEMORY_AUTH_TIMEOUT=0 silently falls back to 60000 because Number("0") is falsy. No real-world impact (a 0 ms timeout has no meaningful use), but worth noting if the env var is ever documented.

Copy link
Copy Markdown

@vorflux vorflux Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed -- found 2 issues (1 medium bug, 1 medium data-exposure concern).


Review with Vorflux

Comment thread src/lib/auth.js
client: 'claude-code',
hostname: os.hostname(),
os: `${process.platform}-${os.arch()}`,
cwd: process.cwd(),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium -- cwd and hostname leak local machine metadata in the browser URL.

hostname, cwd, and os are sent as query parameters on a GET request opened in the user's browser. This means they can end up in browser history, proxy logs, server access logs, and referrer headers. cwd is especially sensitive -- it often contains internal repo names, customer names, or usernames (e.g. /home/jdoe/acme-corp/secret-project).

Consider whether these params are strictly necessary. If they are needed server-side, sending them in a POST from the local server (rather than embedding in the browser URL) would avoid the exposure.

Comment thread src/lib/auth.js
Comment on lines +104 to 116
server.listen(0, '127.0.0.1', () => {
const { port } = server.address();
const callbackUrl = `http://localhost:${port}/callback?state=${stateToken}`;
const params = new URLSearchParams({
callback: callbackUrl,
client: 'claude-code',
hostname: os.hostname(),
os: `${process.platform}-${os.arch()}`,
cwd: process.cwd(),
cli_version: '1.0.0',
});
const authUrl = `${AUTH_BASE_URL}?${params.toString()}`;
openBrowser(authUrl);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium -- the manual-visit fallback in context-hook.js:38 is now broken.

When openBrowser() fails (headless Linux, SSH sessions, missing xdg-open), the error message in context-hook.js tells users to visit AUTH_BASE_URL directly. But the new flow requires the ephemeral port and state token in the callback URL -- visiting the bare AUTH_BASE_URL cannot complete the local callback handshake.

The fallback at context-hook.js:38:

If the browser did not open, visit: ${AUTH_BASE_URL}

needs to be updated to include the full authUrl (with callback + state params), or startAuthFlow needs to surface the constructed URL so the caller can display it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants