fix(auth): clear API token after copy + cross-feature regression coverage - #514
Merged
Merged
Conversation
Consolidated follow-up for audit findings on three recently-merged PRs. None are correctness blockers; this is hardening + coverage. # PR #509 — feat(zed): support hosted Zed agent threads scanner.rs: cfg-gate the macOS Zed `Library/Application Support` fallback path with `#[cfg(target_os = "macos")]`. Without it the block ran on Linux too — harmless because the path can't exist there, but inconsistent with the Windows branch which is already cfg-gated. Pure hygiene. lib.rs: document a future-risk in `pricing_multiplier`. Today the +10% Zed-hosted markup is keyed on `message.client == "zed" AND message.provider_id == "zed.dev"`, which is correct because tokscale bundles upstream-provider LiteLLM rows (anthropic/openai/google) for the underlying models. If LiteLLM ever ships rows under provider `zed.dev` that already include the markup, this would double-bill — a comment now warns the next maintainer to thread matched-price provenance through `apply_pricing_if_available` before relying on that case. lib.rs: two negative regression tests for the markup gate, locking in the existing positive test: - non-zed client + provider_id="zed.dev" → no markup (e.g., a claudecode message that mentions the provider should pay base rate, not Zed's hosted rate). - zed client + non-zed provider (BYOK path with provider_id pointing directly at "anthropic", etc.) → no markup. Important forward-compat: if Zed adds BYOK support and the parser switches to upstream provider IDs, the markup must NOT fire. # PR #511 — fix(codex): deduplicate forked token count history lib.rs: add a negative test that locks in two turns whose `last_token_usage` deltas are byte-identical but emitted at distinct timestamps both survive dedup. The fork-dedup key includes timestamp, so this is correct today; without the test, a future selectivity tightening (e.g. dropping timestamp from the key) could silently erase legitimate usage. # PR #512 — feat(auth): support non-interactive API token auth SettingsClient.tsx: clear `createdToken` from React state after the user copies it. The raw token is shown once and only once; once copied it should leave the component tree so it no longer lives in DevTools / extension snapshots of React state. Users who haven't copied yet still have the value in the reveal panel until they click copy or navigate away. authToken.test.ts: add an expired-token test asserting `GET /api/auth/token` returns 401 with `{ error: "API token has expired" }` when `authenticatePersonalToken` resolves with `status: "expired"`. The branch existed in route.ts but was uncovered. README.md: document the auth-token precedence (env `TOKSCALE_API_TOKEN` > saved credentials file) and the revocation flow (Settings > API Tokens > Revoke; takes effect immediately, returns 401 thereafter). # Out of scope (intentionally not addressed) - Import-path consistency in `route.ts` (audit nit N1): `@/lib/auth /bearerToken` does not resolve under vitest because the frontend has no vitest config to mirror tsconfig path aliases. Switching the import to the alias breaks `npx vitest run`. The fix is to add a `vitest.config.ts` with `resolve.alias`, which is a wider surface change than this nit-cleanup PR warrants. The relative import stays as it was on main. - Legacy plaintext OR-clause in personalTokens.ts (audit M2): a pre-existing migration affordance, not introduced by #512; removal should be paired with a one-shot data migration that rehashes any remaining plaintext rows. - Dead `session_id_from_meta`/`session_forked_from_id` fields on `CodexParseState` (audit nit on #511): looks like deliberate groundwork by the original author. Removing it without their consent is overreach. # Validation - `cargo test -p tokscale-core` — 662 passed (3 new tests) - `cargo clippy -p tokscale-core --all-features -- -D warnings` — clean - `cargo fmt --all -- --check` — clean - `npx vitest run __tests__/api/authToken.test.ts __tests__/lib/bearerToken.test.ts` — 8/8 pass Constraint: All fixes must lock in current behavior (negative tests, cfg gates, doc comments) without changing observed behavior of any shipped feature Constraint: No vitest config changes — the import-style nit yields to that constraint Confidence: high Scope-risk: narrow — 5 files, +188/-0, no behavior change
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This was referenced May 5, 2026
junhoyeo
changed the title
junhoyeo
added a commit
that referenced
this pull request
May 10, 2026
) Closes the import-style nit deferred from PR #514. The frontend has no `vitest.config.ts`, so vitest cannot resolve tsconfig path aliases. Production code can mix `@/lib/...` and relative imports freely (Next.js bundles both), but the moment a test file pulls in production code that imports `@/lib/...`, vitest fails to resolve the alias unless the symbol is mocked first via `vi.mock(...)`. The result was that `route.ts` imported `authenticatePersonalToken` via `@/...` (test mocks it) but had to import `getBearerToken` via a four-level relative path (test runs the real implementation, so the alias would fail). PR #514's audit flagged this as nit N1 and reverted the alias-cleanup attempt because the cleanup broke `npx vitest run`. Add a minimal `packages/frontend/vitest.config.ts` that mirrors the `@/*` -> `./src/*` mapping in tsconfig.json. Then convert the two imports that were paying the relative-path tax: - `src/app/api/auth/token/route.ts`: switch `getBearerToken` import from `../../../../lib/auth/bearerToken` to `@/lib/auth/bearerToken` so `route.ts` is internally consistent (the matching `personalTokens` import was already using the alias). - `__tests__/lib/bearerToken.test.ts`: switch `getBearerToken` import from `../../src/lib/auth/bearerToken` to `@/lib/auth/bearerToken` for parity with how production code now reads. Other test files that still use `../../src/lib/...` paths (submit.test.ts, renderProfileBadgeSvg.test.ts, renderIsometric3DSvg.test.ts, renderProfileEmbedSvg.test.ts) are left as-is to keep this PR scoped to the original nit. They can be converted in a separate cleanup once the convention here is established. Validation: `npx vitest run` -> 21 test files, **177 tests pass** (0 regressions). Constraint: vitest must resolve `@/...` without breaking existing tests, and without pulling in `vite-tsconfig-paths` as a new dep Rejected: Use `vite-tsconfig-paths` plugin | adds a dependency for what is a 5-line manual config; not worth the supply-chain cost Rejected: Convert all relative test imports to aliases | scope creep; original nit was about route.ts only Confidence: high Scope-risk: narrow — config-only + 2 import lines Directive: When adding new tests that import production code via `@/...` aliases, no further config is needed; the alias just works.
junhoyeo
added a commit
that referenced
this pull request
May 10, 2026
Closes the M2 security finding deferred from PR #514's audit. # Background Pre-#512 the `api_tokens.token` column stored personal API tokens in plaintext (`tt_<48 hex chars>`). PR #512 introduced SHA-256 at rest for all NEW tokens and a transitional OR-clause in `authenticatePersonalToken` that matched both the hashed value and the raw value, then auto-rehashed the matched row in place. That upgrade fired only when the token's owner re-authenticated, leaving plaintext rows in the DB indefinitely for users who don't. While that bridge was correct as a migration affordance, it widens the breach surface: any read access to the DB during the migration window leaks directly-usable tokens. The audit on PR #512 (forwarded to PR #514's body) flagged this as Medium and asked for the fix to be paired with a one-shot data migration. # Change Two coordinated edits: 1. `migrations/0006_rehash_plaintext_personal_tokens.sql` (new): - `CREATE EXTENSION IF NOT EXISTS pgcrypto` (idempotent; defensive — pgcrypto is widely available on managed Postgres but not always pre-loaded) - Single-statement `UPDATE api_tokens SET token = encode(digest(token, 'sha256'), 'hex') WHERE LEFT(token, 3) = 'tt_'` rehashes every remaining plaintext row in place. - Identification rationale: SHA-256 hex output is `[0-9a-f]{64}` and can never produce the substring `tt_`. Any row whose stored token still starts with `tt_` is definitionally plaintext. Already-hashed rows are skipped. - Idempotent: re-running after migration matches zero rows. 2. `src/lib/auth/personalTokens.ts`: - Drop the `or(eq(token, hashed), eq(token, raw))` clause; the query now matches only `eq(token, hashed)`. - Drop the `isLegacyPlaintext` detection and the rehash-on-use UPDATE branch — the migration finished that work. - Drop the `tokenValue: apiTokens.token` SELECT column — it was only there to detect plaintext. - Drop the `or` import (no longer used). - Add a comment pointing future readers at migration 0006 so the historical context isn't lost. 3. `migrations/meta/_journal.json`: - Append entry for migration 0006. # Validation - `npx vitest run` from `packages/frontend` -> **21 test files / 177 tests pass** (0 regressions). The existing `personalTokens.test.ts` fixtures already use pre-hashed `tokenValue` strings, so they exercise the new (hashed-only) path unchanged. # Deployment notes - The migration must run BEFORE the application code change reaches production, OR they must deploy together. Running the app-side change first against a DB that still contains plaintext rows would lock those users out (their stored token wouldn't match a SHA-256 hash of itself). The included Drizzle migration ordering takes care of this when deployed via the standard `drizzle-kit migrate` -> deploy app sequence. - pgcrypto is required. Verify on the target deployment with `SELECT * FROM pg_extension WHERE extname = 'pgcrypto';` before rolling out. The `CREATE EXTENSION IF NOT EXISTS` in the migration will create it if the role has CREATE privilege; if not, the migration fails loudly rather than silently leaving plaintext behind. # Out of scope - Token rotation policy, max-age, automatic expiry, or per-user revocation rate limits. This PR only addresses at-rest hashing of the EXISTING plaintext rows. Constraint: All existing tokens must continue to authenticate after this lands; no user-visible behavior change beyond the fact that DB reads no longer leak directly-usable values Constraint: The migration must be idempotent and self-contained in a single transaction (Drizzle wraps each migration in BEGIN /COMMIT) Rejected: Two-phase migration with a temporary dual-column schema | needlessly complex; Postgres UPDATE is atomic per row and the table is small (one row per (user, named token)) Rejected: Background trickle migration | leaves plaintext rows visible for the duration; the security ask is "now, not eventually" Rejected: Drop the column and re-issue all tokens | breaks every active token; user-hostile Confidence: high Scope-risk: moderate — touches the central auth-path query and introduces a destructive (in-place rewrite) data migration; covered by the existing 11 personalTokens tests + the broader auth integration suite Directive: Do not re-introduce a plaintext fallback in `authenticatePersonalToken`. The migration is one-way; future legacy paths should be paired with their own one-shot migration. Not-tested: A real production rollout against a DB that still contains plaintext rows; the fix is verified only against the test suite which mocks the DB layer.
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.
Consolidated follow-up for audit findings on the three nit-bearing PRs that just merged. None are correctness blockers; this is hardening + test coverage + docs.
#509 — Zed agent support
#511 — Codex fork dedup
#512 — API token auth
Out of scope
Validation
Scope
Summary by cubic
Addresses audit nits from #509, #511, and #512 to harden behavior without changing it. Adds a macOS-only gate for the Zed threads DB scan, clears copied API tokens from UI state, documents
TOKSCALE_API_TOKENprecedence and revocation, and adds tests pinning Zed pricing markup gates, Codex twin-delta dedup, and expired-token 401s.Written for commit 2370327. Summary will update on new commits.