Conversation
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
The core changes are correct and well-tested. The password minimum and credential masking threshold changes work as described. However, there are consistency gaps worth addressing: the create-admin-user CLI tool bypasses the new validation, and the frontend form provides no user-facing feedback about the 12-char minimum.
Should Fix
-
tools/create-admin-user.ts — This script inserts users directly into the database via Drizzle, bypassing the tRPC router entirely. Its usage example shows --password changeme (8 chars), which would create a user that violates the new 12-char policy. The tool should enforce the same minimum or at least warn. This creates a real inconsistency: an admin bootstrapping the system via the documented CLI flow will unknowingly create a weak-password account.
-
web/src/components/settings/user-form-dialog.tsx — The password input has no minLength attribute or visible hint about the 12-character requirement. Users will type a short password, submit the form, and get a generic tRPC validation error. Adding minLength=12 and a placeholder like "Minimum 12 characters" would prevent confusion.
Inline Notes
-
src/api/routers/users.ts:26 — The 12-char minimum is enforced here at the tRPC layer, but tools/create-admin-user.ts bypasses this entirely by inserting directly into the DB. Its example usage shows --password changeme (8 chars). Consider adding a matching length check in that script, or at minimum updating the example in its doc comment to use a 12+ char password.
-
src/api/routers/projects.ts:233 — The threshold change from 4 to 12 is correct for the API. Note that the CLI tools (tools/manage-secrets.ts:39, tools/resolve-config.ts:23) have their own maskValue() with a <= 8 threshold and a different format (first4...last4 vs ****last4). If the security rationale applies equally to CLI output, those should be updated too.
Observations (non-blocking)
- tools/manage-secrets.ts:39 and tools/resolve-config.ts:23 — These CLI tools have their own maskValue() with a threshold of <= 8 and a different format (first4...last4). The API endpoint now uses <= 12 with ****last4. These are different contexts (CLI debugging vs API response), so the inconsistency may be intentional, but it is worth being aware of.
🕵️ claude-code · claude-opus-4-6 · run details
- tools/create-admin-user.ts: add runtime check rejecting passwords shorter
than 12 chars, matching the tRPC router validation; update doc comment
examples to use a compliant password instead of 'changeme' (8 chars)
- web/src/components/settings/user-form-dialog.tsx: add minLength={12} to
the password input and update placeholder text so users see the requirement
before submitting the form
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for the thorough review @nhopeatall! I've addressed both of the Should Fix items: tools/create-admin-user.ts — Added an explicit web/src/components/settings/user-form-dialog.tsx — Added Regarding the Observations about 🕵️ claude-code · claude-sonnet-4-6 · run details |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
LGTM — Clean, focused security hardening. The password validation and credential masking changes are correct, well-tested with proper boundary cases, and consistently applied across the API layer, admin tooling, and frontend.
Notes
src/api/routers/projects.ts:233— Masking threshold: The change from<= 4to<= 12is sound — a 5-char secret previously leaked 80% of its content via the last-4-char preview. Worth noting:tools/manage-secrets.tsandtools/resolve-config.tsuse a different masking strategy (<= 8threshold,first4...last4format). That's fine since those are developer CLI tools with a different audience, but if you ever want to unify masking behavior, it might be worth extracting a shared helper.
Should Fix (follow-up)
-
Documentation references outdated passwords:
CLAUDE.md(line 464) still shows--password secret,docs/getting-started.md(line 69),README.md(line 28), andsetup.sh(line 29) all use--password changeme(8 chars). These will fail server-side validation with the new 12-char minimum. Not in this PR's diff, but should be updated in a follow-up to avoid confusing new users. -
CLI test fixtures use short passwords:
tests/unit/cli/dashboard/users/users.test.tsusessecret123(9 chars) andnewpassword(11 chars) as test passwords. These only pass because the CLI tests mock the tRPC client (so Zod validation never fires), but they model invalid API calls. Worth updating for consistency in a follow-up.
🕵️ claude-code · claude-opus-4-6 · run details
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Summary
z.string().min(1)toz.string().min(12)for bothcreateandupdatemutations in the users router'****'for shorter values (threshold changed from<= 4to<= 12)Changes
src/api/routers/users.tscreatemutation:password: z.string().min(1)→z.string().min(12)updatemutation:password: z.string().min(1).optional()→z.string().min(12).optional()src/api/routers/projects.tscredentials.list: masking threshold changed fromrow.value.length <= 4torow.value.length <= 12, preventing 80% secret exposure for short secretstests/unit/api/routers/users.test.tstests/unit/api/routers/projects.test.ts****), 12 chars (shows****), 13 chars (shows****jklm)Test plan
createandupdateboth reject passwords < 12 chars****; 13-char values show last 4 charsTrello card: https://trello.com/c/69c0100c3f84a30b3a5ee67c
🤖 Generated with Claude Code
🕵️ claude-code · claude-sonnet-4-6 · run details