๐ก๏ธ Sentinel: [CRITICAL] Fix hardcoded admin credentials#35
๐ก๏ธ Sentinel: [CRITICAL] Fix hardcoded admin credentials#35seonghobae wants to merge 2 commits into
Conversation
|
๐ 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. |
๐ WalkthroughSummary by CodeRabbit
Walkthrough๊ด๋ฆฌ์ ๊ณ์ ์ ๋ณด๋ฅผ ํ๊ฒฝ๋ณ์๋ก ์ด๋ํ๊ณ Zod ๊ฒ์ฆ์ ์ถ๊ฐํ์ผ๋ฉฐ, PBKDF2 ๊ธฐ๋ฐ ๋น๋ฐ๋ฒํธ ๊ฒ์ฆ์ผ๋ก ๋ก๊ทธ์ธ ํ๋ฆ์ ๋น๋๊ธฐํํ๊ณ .env.example ๋ฐ CI์ ํ๋ ์ด์คํ๋๋ฅผ ์ถ๊ฐํ์ต๋๋ค. ๋ณ๊ฒฝ ์ฌํญ๊ด๋ฆฌ์ ์๊ฒฉ์ฆ๋ช ํ๊ฒฝ ๋ณ์ํ
Sequence Diagram(s): sequenceDiagram
participant Client as ํด๋ผ์ด์ธํธ
participant API as POST /api/admin/login
participant Auth as admin-auth (verifyAdminCredentials)
Client->>API: ๋ก๊ทธ์ธ ์์ฒญ (username, password)
API->>Auth: await verifyAdminCredentials({username, password})
Auth->>Auth: pbkdf2Async(input password) -> ํด์ ๋น๊ต(timingSafeEqual) against TARGET_PASSWORD_HASH
Auth-->>API: ๊ฒ์ฆ ๊ฒฐ๊ณผ (true/false)
API-->>Client: 200 + ์ธ์
์ฟ ํค ๋๋ 401
์์ ์ฝ๋ ๋ฆฌ๋ทฐ ๋ ธ๋ ฅ๐ฏ 3 (Moderate) | โฑ๏ธ ~20 minutes ์
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
โ Failed checks (1 error)
โ Passed checks (4 passed)
โจ Finishing Touches๐ Generate docstrings
๐งช 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 @.jules/sentinel.md:
- Line 2: Update the sentinel entry heading that currently reads "## 2024-05-24
- [CRITICAL] Fix Hardcoded Admin Credentials in Authentication Logic" so the
date reflects the actual PR creation/fix date (change 2024-05-24 to 2026-06-07);
locate and edit the header line in .jules/sentinel.md (the "## ... - [CRITICAL]
Fix Hardcoded Admin Credentials in Authentication Logic" line) to use the
corrected date.
๐ช 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: f5d1fddb-993c-456b-a712-dfd0d04a92ea
๐ 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
| @@ -0,0 +1,5 @@ | |||
|
|
|||
| ## 2024-05-24 - [CRITICAL] Fix Hardcoded Admin Credentials in Authentication Logic | |||
There was a problem hiding this comment.
์ผํฐ๋ฌ ๊ธฐ๋ก์ ๋ ์ง๋ฅผ ์์ ํ์ธ์.
๋ฌธ์์ ๊ธฐ๋ก๋ ๋ ์ง๊ฐ 2024-05-24์ด์ง๋ง ์ด PR์ 2026-06-07์ ์์ฑ๋์์ต๋๋ค. ์ผํฐ๋ฌ ๊ธฐ๋ก์ ๋ ์ง๋ ์ค์ ์์ ์ด ์ด๋ฃจ์ด์ง ๋ ์ง๋ฅผ ์ ํํ๊ฒ ๋ฐ์ํด์ผ ํฉ๋๋ค.
๐ ๋ ์ง ์์ ์ ์
-## 2024-05-24 - [CRITICAL] Fix Hardcoded Admin Credentials in Authentication Logic
+## 2026-06-07 - [CRITICAL] Fix Hardcoded Admin Credentials in Authentication Logic๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## 2024-05-24 - [CRITICAL] Fix Hardcoded Admin Credentials in Authentication Logic | |
| ## 2026-06-07 - [CRITICAL] Fix Hardcoded Admin Credentials in Authentication Logic |
๐ค 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 @.jules/sentinel.md at line 2, Update the sentinel entry heading that
currently reads "## 2024-05-24 - [CRITICAL] Fix Hardcoded Admin Credentials in
Authentication Logic" so the date reflects the actual PR creation/fix date
(change 2024-05-24 to 2026-06-07); locate and edit the header line in
.jules/sentinel.md (the "## ... - [CRITICAL] Fix Hardcoded Admin Credentials in
Authentication Logic" line) to use the corrected date.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and canโt be posted inline due to platform limitations.
โ ๏ธ Outside diff range comments (1)
packages/web/src/lib/server/admin-auth.ts (1)
10-11:โ ๏ธ Potential issue | ๐ก Minor
ADMIN_PASSWORD์ธ๋ถ ๋ด๋ณด๋ด๊ธฐ(export) ์ ๊ฑฐ ์์ฒญ (admin-auth.ts 10-11)
ADMIN_PASSWORD๋TARGET_PASSWORD_HASH๋ฅผ ๊ณ์ฐํ๋ ๋ฐ๋ง ํ์ํ๋ฉฐ, ํ์ฌexport const ADMIN_PASSWORD๋ก ํ๋ฌธ ๋น๋ฐ๋ฒํธ๋ฅผ ๋ชจ๋ ์ธํฐํ์ด์ค์ ๋ ธ์ถํ๊ณ ์์ต๋๋ค.ADMIN_PASSWORD๋ฅผ ๋ค๋ฅธ ํ์ผ์์ named import๋ก ์ฌ์ฉํ๋ ํ์ ๋ ์์ดconst๋ก ๋น๋ด๋ณด๋ด๋ ๊ฒ ์์ ํฉ๋๋ค.๐ ํ๋ฌธ ๋น๋ฐ๋ฒํธ ๋ ธ์ถ ์ ๊ฑฐ
-export const ADMIN_USERNAME = env.ADMIN_USERNAME -export const ADMIN_PASSWORD = env.ADMIN_PASSWORD +export const ADMIN_USERNAME = env.ADMIN_USERNAME +const ADMIN_PASSWORD = env.ADMIN_PASSWORD๐ค 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/admin-auth.ts` around lines 10 - 11, ADMIN_PASSWORD is currently exported but only needed internally to compute TARGET_PASSWORD_HASH; change export const ADMIN_PASSWORD to a non-exported const (remove export) and keep export for ADMIN_USERNAME only; update any internal uses in this module to reference the new local const and ensure no other modules import ADMIN_PASSWORD (remove or refactor those imports if present) so the plaintext password is not exposed from admin-auth.ts.
๐ค 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/admin-auth.ts`:
- Around line 49-62: Always run the PBKDF2 hashing regardless of the username
match to avoid timing leakage: call pbkdf2Async(...) unconditionally, compute
passwordMatch = timingSafeEqual(inputPasswordHash, TARGET_PASSWORD_HASH),
compute usernameMatch = safeEqual(input.username, ADMIN_USERNAME), then return
usernameMatch && passwordMatch (ensure inputPasswordHash is computed before
those comparisons). Reference pbkdf2Async, timingSafeEqual, safeEqual,
ADMIN_USERNAME, TARGET_PASSWORD_HASH, and the HASH_* constants when making the
change.
- Around line 20-24: Replace the hardcoded HASH_SALT in admin-auth.ts with an
environment-driven salt: add an ADMIN_PASSWORD_SALT (or
ADMIN_PASSWORD_HASH+ADMIN_PASSWORD_SALT) to the app env module (e.g., export it
from packages/web/src/lib/server/env.ts) and update the code paths that
reference HASH_SALT to read the salt from that env variable; if you choose to
store ADMIN_PASSWORD_HASH instead of plain ADMIN_PASSWORD, adapt the
authentication routines (the PBKDF2/verify logic in admin-auth.ts that uses
HASH_ITERATIONS, HASH_KEYLEN, HASH_DIGEST) to compare against the provided hash
and use the env salt, and ensure a clear runtime error is thrown when the
required env variables are missing.
---
Outside diff comments:
In `@packages/web/src/lib/server/admin-auth.ts`:
- Around line 10-11: ADMIN_PASSWORD is currently exported but only needed
internally to compute TARGET_PASSWORD_HASH; change export const ADMIN_PASSWORD
to a non-exported const (remove export) and keep export for ADMIN_USERNAME only;
update any internal uses in this module to reference the new local const and
ensure no other modules import ADMIN_PASSWORD (remove or refactor those imports
if present) so the plaintext password is not exposed from admin-auth.ts.
๐ช 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: 357ab05c-bbf0-4bfd-b40f-d192c154695f
๐ Files selected for processing (2)
packages/web/src/app/api/admin/login/route.tspackages/web/src/lib/server/admin-auth.ts
| // PBKDF2 parameters for secure password hashing | ||
| const HASH_SALT = 'argos_admin_salt' | ||
| const HASH_ITERATIONS = 100000 | ||
| const HASH_KEYLEN = 64 | ||
| const HASH_DIGEST = 'sha512' |
There was a problem hiding this comment.
์ ์ ์ํธ ์ฌ์ฉ์ ๋ณด์ ๋ชจ๋ฒ ์ฌ๋ก ์๋ฐ
HASH_SALT๊ฐ ์์ค ์ฝ๋์ ํ๋์ฝ๋ฉ๋์ด ์์ต๋๋ค. ๊ณต๊ฒฉ์๊ฐ ์์ค ์ฝ๋์ ์ ๊ทผํ๋ฉด ์ด ์ํธ๋ก ๋ ์ธ๋ณด์ฐ ํ
์ด๋ธ์ ์ฌ์ ๊ณ์ฐํ ์ ์์ต๋๋ค.
๊ถ์ฅ ํด๊ฒฐ์ฑ :
ADMIN_PASSWORD_SALTํ๊ฒฝ๋ณ์๋ฅผ ์ถ๊ฐํ๊ณ ๋ฐฐํฌ ์ ๋๋ค ์์ฑ๋ ๊ฐ ์ฌ์ฉ- ๋๋
ADMIN_PASSWORD๋์ ์ด๋ฏธ ํด์๋ADMIN_PASSWORD_HASH์ ์ํธ๋ฅผ ํ๊ฒฝ๋ณ์๋ก ์ ์ฅ
๋จ์ผ ๊ด๋ฆฌ์ ๊ณ์ ์ 100k iterations๋ฅผ ์ฌ์ฉํ๋ฏ๋ก ์ฆ๊ฐ์ ์ธ ์ํ์ ์๋์ง๋ง, ๋ณด์ ์ฌ์ธต ๋ฐฉ์ด ์ธก๋ฉด์์ ๊ฐ์ ์ด ํ์ํฉ๋๋ค.
๐ก๏ธ ํ๊ฒฝ๋ณ์ ๊ธฐ๋ฐ ์ํธ ์ฌ์ฉ ์ ์
packages/web/src/lib/server/env.ts์ ์ถ๊ฐ:
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_PASSWORD: z.string().trim().min(8),
+ ADMIN_PASSWORD_SALT: z.string().min(16),
})admin-auth.ts ์์ :
-const HASH_SALT = 'argos_admin_salt'
+const HASH_SALT = env.ADMIN_PASSWORD_SALT๐ค 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/admin-auth.ts` around lines 20 - 24, Replace the
hardcoded HASH_SALT in admin-auth.ts with an environment-driven salt: add an
ADMIN_PASSWORD_SALT (or ADMIN_PASSWORD_HASH+ADMIN_PASSWORD_SALT) to the app env
module (e.g., export it from packages/web/src/lib/server/env.ts) and update the
code paths that reference HASH_SALT to read the salt from that env variable; if
you choose to store ADMIN_PASSWORD_HASH instead of plain ADMIN_PASSWORD, adapt
the authentication routines (the PBKDF2/verify logic in admin-auth.ts that uses
HASH_ITERATIONS, HASH_KEYLEN, HASH_DIGEST) to compare against the provided hash
and use the env salt, and ensure a clear runtime error is thrown when the
required env variables are missing.
| if (!safeEqual(input.username, ADMIN_USERNAME)) { | ||
| return false | ||
| } | ||
|
|
||
| // Hash the incoming password asynchronously to avoid blocking the event loop | ||
| const inputPasswordHash = await pbkdf2Async( | ||
| input.password, | ||
| HASH_SALT, | ||
| HASH_ITERATIONS, | ||
| HASH_KEYLEN, | ||
| HASH_DIGEST | ||
| ) | ||
|
|
||
| return timingSafeEqual(inputPasswordHash, TARGET_PASSWORD_HASH) |
There was a problem hiding this comment.
ํ์ด๋ฐ ์ค๋ผํด์ ํตํ ์ฌ์ฉ์๋ช ์ด๊ฑฐ ๊ฐ๋ฅ์ฑ
์ฌ์ฉ์๋ช ๋ถ์ผ์น ์ ์ฆ์ ๋ฐํํ๊ณ (Line 49-51), ๋น๋ฐ๋ฒํธ ๊ฒ์ฆ์ PBKDF2 ํด์ ๊ณ์ฐ ํ ์ํ๋ฉ๋๋ค. ์ด ์๊ฐ ์ฐจ์ด(~์๋ฐฑms)๋ก ๊ณต๊ฒฉ์๊ฐ ์ ํจํ ์ฌ์ฉ์๋ช ์ ์ด๊ฑฐํ ์ ์์ต๋๋ค.
๋จ์ผ ๊ด๋ฆฌ์ ๊ณ์ ์์คํ ์์๋ ์ํ๋๊ฐ ๋ฎ์ง๋ง, ์์ ํ ์์ ์๊ฐ ๊ฒ์ฆ์ ์ํด ์ฌ์ฉ์๋ช ๊ณผ ๊ด๊ณ์์ด ํญ์ PBKDF2๋ฅผ ์ํํ๋ ๊ฒ์ด ์ข์ต๋๋ค.
โฑ๏ธ ์์ ์๊ฐ ๊ฒ์ฆ ์ ์
export async function verifyAdminCredentials(input: {
username: string
password: string
}): Promise<boolean> {
- if (!safeEqual(input.username, ADMIN_USERNAME)) {
- return false
- }
-
// Hash the incoming password asynchronously to avoid blocking the event loop
const inputPasswordHash = await pbkdf2Async(
input.password,
HASH_SALT,
HASH_ITERATIONS,
HASH_KEYLEN,
HASH_DIGEST
)
- return timingSafeEqual(inputPasswordHash, TARGET_PASSWORD_HASH)
+ const usernameMatch = safeEqual(input.username, ADMIN_USERNAME)
+ const passwordMatch = timingSafeEqual(inputPasswordHash, TARGET_PASSWORD_HASH)
+ return usernameMatch && passwordMatch
}๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!safeEqual(input.username, ADMIN_USERNAME)) { | |
| return false | |
| } | |
| // Hash the incoming password asynchronously to avoid blocking the event loop | |
| const inputPasswordHash = await pbkdf2Async( | |
| input.password, | |
| HASH_SALT, | |
| HASH_ITERATIONS, | |
| HASH_KEYLEN, | |
| HASH_DIGEST | |
| ) | |
| return timingSafeEqual(inputPasswordHash, TARGET_PASSWORD_HASH) | |
| // Hash the incoming password asynchronously to avoid blocking the event loop | |
| const inputPasswordHash = await pbkdf2Async( | |
| input.password, | |
| HASH_SALT, | |
| HASH_ITERATIONS, | |
| HASH_KEYLEN, | |
| HASH_DIGEST | |
| ) | |
| const usernameMatch = safeEqual(input.username, ADMIN_USERNAME) | |
| const passwordMatch = timingSafeEqual(inputPasswordHash, TARGET_PASSWORD_HASH) | |
| return usernameMatch && passwordMatch |
๐ค 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/admin-auth.ts` around lines 49 - 62, Always run
the PBKDF2 hashing regardless of the username match to avoid timing leakage:
call pbkdf2Async(...) unconditionally, compute passwordMatch =
timingSafeEqual(inputPasswordHash, TARGET_PASSWORD_HASH), compute usernameMatch
= safeEqual(input.username, ADMIN_USERNAME), then return usernameMatch &&
passwordMatch (ensure inputPasswordHash is computed before those comparisons).
Reference pbkdf2Async, timingSafeEqual, safeEqual, ADMIN_USERNAME,
TARGET_PASSWORD_HASH, and the HASH_* constants when making the change.
|
์ต์ 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 work is now obsolete and stopping work on this task. |
1 participant
๐จ Severity: CRITICAL
๐ก Vulnerability: Hardcoded
ADMIN_USERNAMEandADMIN_PASSWORDwere discovered inpackages/web/src/lib/server/admin-auth.ts.๐ฏ Impact: Anyone with read access to the repository could discover the administrator credentials, leading to a full authentication bypass and unauthorized administrative access. Furthermore, hardcoded credentials cannot be easily rotated or managed per-environment.
๐ง Fix:
ADMIN_USERNAMEandADMIN_PASSWORDto the Zod environment schema inpackages/web/src/lib/server/env.tsto ensure they are required at runtime.admin-auth.tswith references toenv.ADMIN_USERNAMEandenv.ADMIN_PASSWORD, preserving the existingexport constAPI contract.packages/web/.env.exampleand.github/workflows/ci.ymlto include the new required environment variables to prevent CI/CD regressions..jules/sentinel.md.โ Verification:
pnpm run buildandpnpm lint.ADMIN_USERNAMEandADMIN_PASSWORDโit should fail to start.PR created automatically by Jules for task 2027270219903167306 started by @seonghobae