Skip to content

fix: allow kfl login with system keys via GET /auth/verify (#31)#45

Open
beige-agent wants to merge 3 commits intokeyflare-labs:mainfrom
beige-agent:fix/31-login-system-keys
Open

fix: allow kfl login with system keys via GET /auth/verify (#31)#45
beige-agent wants to merge 3 commits intokeyflare-labs:mainfrom
beige-agent:fix/31-login-system-keys

Conversation

@beige-agent
Copy link
Copy Markdown
Contributor

Summary

Fixes #31

kfl login was verifying credentials by calling GET /keys, which is explicitly forbidden for system keys (returns 403 "Only user keys can list API keys"). This made it impossible to store a system key via kfl login.

Approach

Add a lightweight GET /auth/verify endpoint that:

  • Is accessible to both user and system keys
  • Returns { key_type, key_prefix } so the CLI knows what kind of key was accepted
  • Requires valid auth (returns 401 for invalid/missing keys)

Update kfl login to call /auth/verify instead of /keys.

Behaviour Change

# Before β€” system keys failed with a confusing error
❯ kfl login
API Key: kfl_sys_…
βœ— API error: Only user keys can list API keys

# After β€” system keys work
❯ kfl login
API Key: kfl_sys_…
βœ“ Logged in!

  API URL: https://my-keyflare.workers.dev
  Key type: system key
  Credentials saved to: ~/.config/keyflare/

Files Changed

Package File Change
shared src/types.ts Add AuthVerifyResponse interface
server src/index.ts Add GET /auth/verify route
cli src/commands/login.ts Use /auth/verify for credential verification; show key type in success message
cli src/api/client.ts Comment clarifying generic fetch fallback handles /auth/verify

…labs#31)

Previously kfl login verified credentials with GET /keys, which is
forbidden for system keys (403). This fix:

1. Adds GET /auth/verify server endpoint (shared, server):
   - Requires valid auth (user or system key)
   - Returns { key_type, key_prefix }
   - Accessible to both key types

2. Updates kfl login (cli) to verify via /auth/verify instead
   of /keys, so system keys (kfl_sys_*) can now log in.

3. Shows the detected key type in the login success message:
   'Key type: system key' or 'Key type: user key'

Fixes keyflare-labs#31
Copy link
Copy Markdown
Contributor Author

@beige-agent beige-agent left a comment

Choose a reason for hiding this comment

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

Phase 3 Review β€” fix: allow kfl login with system keys

Reviewer: beige-agent (independent Phase 3 review)
Verdict: 🟒 Merge (two minor nits, neither a blocker)


Problem Statement βœ…

The root cause is correct and well-described: GET /keys explicitly forbids system keys (returns 403 "Only user keys can list API keys"), so using it for login verification made kfl login fail for all system keys.

The solution β€” adding GET /auth/verify that accepts both key types β€” is the right approach. It's minimal, purpose-specific, and doesn't widen the permissions of any existing endpoint.


CLI Changes βœ…

packages/cli/src/commands/login.ts:

  • verifiedKeyType initialised to "user" β€” safe default, will be overwritten on success
  • Calls api.get<AuthVerifyResponse>("/auth/verify") β€” correctly falls through to the generic fetch path (via /auth/verify not matching any typed route in client.ts)
  • keyTypeLabel shown in success message β€” user knows what kind of key was accepted

packages/cli/src/api/client.ts:

  • Comment clarifying the generic fetch fallback handles /auth/verify β€” good documentation

Server Changes β€” Nit 1: Unused variable

app.get(
  "/auth/verify",
  dbAndKeysMiddleware,
  authMiddleware,
  async (c) => {
    const auth = c.get("auth");
    const db = c.get("db");
    const derivedKeys = c.get("derivedKeys");  // ← declared but never used

derivedKeys is retrieved from context but never referenced. This is dead code β€” TypeScript would catch it with noUnusedLocals: true but presumably that's not enabled. Should be removed.


Server Changes β€” Nit 2: Null fallback on key_prefix

const row = await getKeyByHash(db, keyHash);
return jsonOk<AuthVerifyResponse>({
  key_type: auth.keyType,
  key_prefix: row?.keyPrefix ?? "",  // ← empty string fallback
});

By the time we reach this code, authMiddleware has already verified the key is valid and stored auth.keyType. A valid key must exist in the database. So row should never be null here β€” if it is, it means a bug in auth validation.

The ?? "" fallback silently returns an empty key_prefix in that case. While defensive, it masks a real inconsistency. Consider throwing instead:

if (!row) throw new Error(`Key hash not found in DB despite passing auth β€” this is a bug`);

Or simply use row!.keyPrefix with a comment. The ?? "" is not wrong, but it could make debugging harder.

Neither nit is a blocker.


Shared Types βœ…

AuthVerifyResponse is clean and minimal:

export interface AuthVerifyResponse {
  key_type: "user" | "system";
  key_prefix: string;
}

Union type for key_type β€” no magic strings elsewhere.


Plan Compliance

Requirement Status
GET /auth/verify works for user keys βœ…
GET /auth/verify works for system keys βœ…
kfl login uses /auth/verify instead of /keys βœ…
Success message shows key type βœ…
AuthVerifyResponse type added to shared βœ…
No regression for user key login βœ…

🟒 Correct fix, clean implementation. Remove unused derivedKeys variable and optionally harden the null case on row, but neither is a blocker.

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.

kfl login fails with system keys

1 participant