๐ก๏ธ Sentinel: [CRITICAL] Fix hardcoded admin credentials#38
๐ก๏ธ Sentinel: [CRITICAL] Fix hardcoded admin credentials#38seonghobae wants to merge 2 commits into
Conversation
Moved hardcoded ADMIN_USERNAME and ADMIN_PASSWORD in admin-auth.ts to environment variables. Added validation via Zod in env.ts. Updated .env.example and CI workflow with placeholder values.
|
๐ Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a ๐ emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Review limit reached
More reviews will be available in 14 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. โ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. ๐ฆ How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. โน๏ธ Review infoโ๏ธ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ๐ Files selected for processing (3)
๐ WalkthroughWalkthroughํ๋์ฝ๋ฉ๋ ๊ด๋ฆฌ์ ์ฌ์ฉ์๋ช
( Changes๊ด๋ฆฌ์ ์๊ฒฉ ์ฆ๋ช ํ๊ฒฝ ์ค์
Estimated code review effort๐ฏ 2 (Simple) | โฑ๏ธ ~10 ๋ถ Poem
๐ฅ Pre-merge checks | โ 5โ Passed checks (5 passed)
โ๏ธ Tip: You can configure your own custom pre-merge checks in the settings. โจ Finishing Touches๐งช Generate unit tests (beta)
โจ Simplify code
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
๐ค Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/web/src/lib/server/env.ts`:
- Line 8: Session validation breaks when ADMIN_USERNAME contains '.' because
admin session format in admin-auth.ts is username.expiresAt.nonce.signature and
validation splits on '.' expecting 4 parts; update the ADMIN_USERNAME schema in
env.ts to forbid dots (e.g., require no '.' via a regex or refine) so usernames
like "admin.ops" cannot be created, and ensure the change references
ADMIN_USERNAME and the session creation/validation logic in admin-auth.ts to
keep the 4-part split invariant.
๐ช Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
โน๏ธ Review info
โ๏ธ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0e77c05-aa3a-4730-9d03-cdbd5ad8b72d
๐ Files selected for processing (5)
.github/workflows/ci.yml.jules/sentinel.mdpackages/web/.env.examplepackages/web/src/lib/server/admin-auth.tspackages/web/src/lib/server/env.ts
| DATABASE_URL: z.string().min(1), | ||
| DIRECT_URL: z.string().min(1), | ||
| JWT_SECRET: z.string().min(32), | ||
| ADMIN_USERNAME: z.string().trim().min(1), |
There was a problem hiding this comment.
ADMIN_USERNAME์ . ํ์ฉ ์ ๊ด๋ฆฌ์ ์ธ์
๊ฒ์ฆ์ด ๊นจ์ง๋๋ค.
packages/web/src/lib/server/admin-auth.ts์ ์ธ์
๊ฐ์ username.expiresAt.nonce.signature ํํ๋ก ์์ฑ๋๊ณ (Line 40), ๊ฒ์ฆ ์ split('.') ๊ฒฐ๊ณผ๋ฅผ 4๊ฐ๋ก ๊ณ ์ ๊ฒ์ฌํฉ๋๋ค(Line 64).
ํ์ฌ ์คํค๋ง๋ ADMIN_USERNAME์ .๋ฅผ ํ์ฉํ๋ฏ๋ก(์: admin.ops), ์ ์ ๋ก๊ทธ์ธ ํ์๋ ์ธ์
์ด ํญ์ ๋ฌดํจ ์ฒ๋ฆฌ๋ ์ ์์ต๋๋ค.
๐ง ์ ์ ์์
const EnvSchema = z.object({
DATABASE_URL: z.string().min(1),
DIRECT_URL: z.string().min(1),
JWT_SECRET: z.string().min(32),
- ADMIN_USERNAME: z.string().trim().min(1),
+ ADMIN_USERNAME: z
+ .string()
+ .trim()
+ .min(1)
+ .regex(/^[^.]+$/, 'ADMIN_USERNAME must not contain "."'),
ADMIN_PASSWORD: z.string().trim().min(8),
})๐ค Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/web/src/lib/server/env.ts` at line 8, Session validation breaks when
ADMIN_USERNAME contains '.' because admin session format in admin-auth.ts is
username.expiresAt.nonce.signature and validation splits on '.' expecting 4
parts; update the ADMIN_USERNAME schema in env.ts to forbid dots (e.g., require
no '.' via a regex or refine) so usernames like "admin.ops" cannot be created,
and ensure the change references ADMIN_USERNAME and the session
creation/validation logic in admin-auth.ts to keep the 4-part split invariant.
Resolved CodeQL \`js/insecure-password-hashing\` alert by replacing fast HMAC-SHA256 with \`pbkdf2\`. Target hash is precomputed synchronously at module load time to avoid runtime blocking, and incoming password is hashed asynchronously to prevent DoS. Updated route.ts to await the now-asynchronous verifyAdminCredentials function.
|
์ต์ upstream/main ๊ธฐ์ค์ผ๋ก ์ด PR์ 82๊ฐ ํ์ผ์ stale diff๋ฅผ ํฌํจํฉ๋๋ค. ํนํ .claude agent/skill ํ์ผ ์ญ์ ์ ์ค๋๋ ์ํฌํ๋ก์ฐ/lockfile/env ๋ณ๊ฒฝ์ด ์์ฌ ์์ด ๊ทธ๋๋ก ์ ์งํ๊ฑฐ๋ upstream์ ์ฌ๋ฆฌ๋ฉด ์ด๋ฏธ ๋ฐ์๋ ๋ณ๊ฒฝ์ ๋๋๋ฆด ์ํ์ด ํฝ๋๋ค. ํ์ฌ ํ๋ upstream #30 โ upstream #21 ์์๋ก ์ ๋ฆฌ ์ค์ ๋๋ค. ์ด PR์ ์์ด๋์ด๊ฐ ์ฌ์ ํ ํ์ํ๋ฉด ์ต์ upstream/main์์ ๋จ์ผ ๋ชฉ์ ๋ธ๋์น๋ก ๊นจ๋ํ๊ฒ ๋ค์ ์ถ์ถํ๊ฒ ์ต๋๋ค. stale PR๋ก ๋ซ์ต๋๋ค. |
Understood. Acknowledging that this PR is closed as obsolete and stopping work on this task. |
1 participant
๐จ Severity: CRITICAL
๐ก Vulnerability: Hardcoded
ADMIN_USERNAMEandADMIN_PASSWORDwere present in the source code (packages/web/src/lib/server/admin-auth.ts). This is a critical security vulnerability, as anyone with access to the source code (or decompiled artifacts) could obtain the admin credentials and gain unauthorized access.๐ฏ Impact: An attacker with these credentials could bypass authentication, accessing the admin panel and performing unauthorized actions, potentially compromising user data and system integrity.
๐ง Fix:
packages/web/src/lib/server/admin-auth.ts.env.ADMIN_USERNAME,env.ADMIN_PASSWORD).packages/web/src/lib/server/env.ts, ensuring they are non-empty strings (using.trim().min(N))..env.examplewith secure placeholders to guide developers during setup..github/workflows/ci.ymlto inject placeholder values for these required environment variables, ensuring the CI build continues to pass.โ Verification:
pnpm --filter @argos/web buildsucceeds when the required environment variables are set.PR created automatically by Jules for task 17384522501331634607 started by @seonghobae