Conversation
github-actions
bot
added
Docs
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| private hashApiKey(key: string): string { | ||
| return createHash("sha256").update(key).digest("hex"); |
Check failure
Code scanning / CodeQL
Use of password hash with insufficient computational effort High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, to fix “use of password hash with insufficient computational effort,” replace direct use of fast cryptographic hash functions (like SHA‑256 via createHash) with a purpose‑built password hashing/KDF function such as bcrypt, Argon2, scrypt, or PBKDF2. These introduce configurable computational (and often memory) cost to make brute‑force attacks substantially harder, even if an attacker obtains the stored hashes.
For this specific code, we should change hashApiKey so that it no longer uses createHash("sha256") and instead uses a slow, salted password hashing function. The rest of the functionality should remain the same: we still only need a non‑reversible representation of the API key to store in authSessions.api_key_hash, and the plaintext API key continues to be carried in the signed cookie for actual API usage. A minimal, well‑known choice is bcrypt. We will:
- Import
bcryptat the top ofapp/server/web/auth.ts. - Change
hashApiKeyfrom a synchronous SHA‑256 hash to an asynchronous bcrypt hash with a reasonable cost factor (e.g., 12). - Adjust the single call site
api_key_hash: this.hashApiKey(apiKey)toawaitthe now‑async function. - Optionally, encode the bcrypt output as returned (it’s already a safe string), so no additional encoding is necessary.
These changes keep the external behavior (the DB still gets a hashed value, callers still pass in an API key string, and the method returns a string) while making offline guessing attacks more expensive if the database is compromised.
| @@ -1,4 +1,5 @@ | ||
| import { createHash, createHmac } from "node:crypto"; | ||
| import bcrypt from "bcrypt"; | ||
|
|
||
| import { eq, lt, sql } from "drizzle-orm"; | ||
| import { LibSQLDatabase } from "drizzle-orm/libsql/driver"; | ||
| @@ -222,7 +223,7 @@ | ||
| await this.opts.db.insert(authSessions).values({ | ||
| id: sid, | ||
| kind: "api_key", | ||
| api_key_hash: this.hashApiKey(apiKey), | ||
| api_key_hash: await this.hashApiKey(apiKey), | ||
| api_key_display: displayName, | ||
| expires_at: new Date(Date.now() + maxAge), | ||
| }); | ||
| @@ -470,8 +471,9 @@ | ||
| return JSON.parse(Buffer.from(signed, "base64url").toString("utf-8")) as CookiePayload; | ||
| } | ||
|
|
||
| private hashApiKey(key: string): string { | ||
| return createHash("sha256").update(key).digest("hex"); | ||
| private async hashApiKey(key: string): Promise<string> { | ||
| const saltRounds = 12; | ||
| return bcrypt.hash(key, saltRounds); | ||
| } | ||
| } | ||
|
|
| @@ -20,7 +20,8 @@ | ||
| "format": "oxfmt" | ||
| }, | ||
| "dependencies": { | ||
| "@libsql/client": "0.17.0" | ||
| "@libsql/client": "0.17.0", | ||
| "bcrypt": "^6.0.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@dnd-kit/core": "^6.3.1", |
| Package | Version | Security advisories |
| bcrypt (npm) | 6.0.0 | None |
There was a problem hiding this comment.
Code Review — Authentication Rework
Overall this is a strong architectural improvement — the discriminated Principal union, server-side sessions, and centralized AuthService are all good moves. A few issues stood out that are worth addressing before merge:
Created locally using Amp
| }); | ||
|
|
||
| const signed = Buffer.from(JSON.stringify(payload)).toString("base64url"); | ||
| const hmac = createHmac("sha256", this.opts.secret).update(signed).digest("base64url"); |
Check failure
Code scanning / CodeQL
Use of password hash with insufficient computational effort High
Copilot Autofix
AI 4 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
The flat-file user store has been deprecated for several versions. All user data now lives in the SQL database. Removes the config field, nix option, docs, and migration logic. Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019cce57-c9e1-7732-9709-8288127573a9
Security: - Add unique constraint on headscale_user_id to prevent hijacking - linkHeadscaleUser now rejects already-claimed Headscale users - Onboarding dropdown filters out claimed users - onboarding-skip action redirects on rejected claims Maintenance: - Replace probabilistic session pruning with setInterval (15m) - Move pruning out of request path into server startup Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019cce57-c9e1-7732-9709-8288127573a9
- Add 'Link Headscale user' option in user menu (admin-only, OIDC users) - Dialog shows only unclaimed Headscale users - New link_user action in user-actions with claim validation - Onboarding now allows skipping the link step with clear messaging - Users who skip are told they can ask an admin to link later Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019cce57-c9e1-7732-9709-8288127573a9
- Onboarding now shows which Headscale user was auto-linked - Members (no ui_access) see a 'Pending Approval' page instead of being silently logged out — their session stays valid - Sign out button on the pending page so users can switch accounts Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019cce57-c9e1-7732-9709-8288127573a9
Member role simply has no UI permissions — not a pending state. Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019cce57-c9e1-7732-9709-8288127573a9
1 participant
No description provided.