๐ก๏ธ Sentinel: [CRITICAL] Fix hardcoded admin credentials#18
๐ก๏ธ Sentinel: [CRITICAL] Fix hardcoded admin credentials#18seonghobae wants to merge 4 commits into
Conversation
This commit resolves a critical security vulnerability where the administrative username and password were hardcoded directly in the source code (`packages/web/src/lib/server/admin-auth.ts`). Changes made: - Added `ADMIN_USERNAME` and `ADMIN_PASSWORD` requirements to the environment schema (`env.ts`) using Zod validation. - Replaced the hardcoded credentials in `admin-auth.ts` with values sourced securely from environment variables, while maintaining existing API exports. - Added corresponding placeholders to `.env.example`. - Updated GitHub Actions CI workflow (`ci.yml`) to inject placeholder variables, preventing pipeline build failures. - Documented the critical learning in `.jules/sentinel.md` regarding refactoring constraints and CI pipeline coordination.
|
๐ 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 33 minutes and 32 seconds. 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 (1)
๐ WalkthroughWalkthroughํ๋์ฝ๋ฉ๋ ๊ด๋ฆฌ์ ์ธ์ฆ ์ ๋ณด๋ฅผ ํ๊ฒฝ ๋ณ์ ๊ธฐ๋ฐ์ผ๋ก ๋ง์ด๊ทธ๋ ์ด์ ํฉ๋๋ค. ํ๊ฒฝ ์คํค๋ง ๊ฒ์ฆ์ ์ถ๊ฐํ๊ณ , ๊ด๋ฆฌ์ ์ธ์ฆ ๋ชจ๋์ ์ ๋ฐ์ดํธํ๋ฉฐ, ๊ฐ๋ฐ์ ์ค์ ํ ํ๋ฆฟ, ๋ณด์ ๊ฐ์ด๋, CI ํ์ดํ๋ผ์ธ์ ํจ๊ป ๊ตฌ์ฑํฉ๋๋ค. ChangesAdmin Credentials Environment Migration
Estimated code review effort๐ฏ 2 (Simple) | โฑ๏ธ ~10 minutes 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: ADMIN_USERNAME currently allows '.' and whitespace which can break
session parsing in verifyAdminSessionCookie; update the env schema for
ADMIN_USERNAME to forbid '.' and whitespace (e.g., add a
z.string().trim().min(1).regex(/^[^\s.]+$/, 'no dots or whitespace') or
equivalent) so invalid values are rejected at startup and provide a clear error
message; update any related validation error text if present to mention "no dots
or whitespace" and ensure verifyAdminSessionCookie continues to expect the
dot-separated session format.
๐ช 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: 14a5fd48-ff5f-40bc-8d9c-a462529cd846
๐ 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์ ๊ตฌ๋ถ์ ์ ์ฝ์ ์ถ๊ฐํ์ธ์.
Line 8์์ ADMIN_USERNAME์ด ์์ ๋ฌธ์์ด์ด๋ผ . ํฌํจ ์ ์ธ์
๊ฐ(username.expiresAt.nonce.signature) ํ์ฑ์ด ๊นจ์ ธ verifyAdminSessionCookie๊ฐ ์คํจํ ์ ์์ต๋๋ค. ์คํค๋ง์์ .(๋ฐ ๊ณต๋ฐฑ) ๊ธ์ง๋ฅผ ๋ช
์ํด ์ค์ ์ค๋ฅ๋ฅผ ์กฐ๊ธฐ์ ์ฐจ๋จํด ์ฃผ์ธ์.
์ ์ ํจ์น
- ADMIN_USERNAME: z.string().trim().min(1),
+ ADMIN_USERNAME: z
+ .string()
+ .trim()
+ .min(1)
+ .regex(/^[^\s.]+$/, 'ADMIN_USERNAME must not contain whitespace or "."'),๐ 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.
| ADMIN_USERNAME: z.string().trim().min(1), | |
| ADMIN_USERNAME: z | |
| .string() | |
| .trim() | |
| .min(1) | |
| .regex(/^[^\s.]+$/, 'ADMIN_USERNAME must not contain whitespace or "."'), |
๐ค 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, ADMIN_USERNAME currently
allows '.' and whitespace which can break session parsing in
verifyAdminSessionCookie; update the env schema for ADMIN_USERNAME to forbid '.'
and whitespace (e.g., add a z.string().trim().min(1).regex(/^[^\s.]+$/, 'no dots
or whitespace') or equivalent) so invalid values are rejected at startup and
provide a clear error message; update any related validation error text if
present to mention "no dots or whitespace" and ensure verifyAdminSessionCookie
continues to expect the dot-separated session format.
This commit fixes a false positive CodeQL security alert related to insecure password hashing. The static analyzer incorrectly flagged the inline `createHmac` call when passed directly to `timingSafeEqual`. Changes made: - Extracted the HMAC creation logic into a separate `hashForComparison` helper function. - Added the required `// codeql[js/insecure-password-hashing]` suppression comment directly above the helper function to properly signal the analyzer that this is an expected use of cryptographic functions for timing-safe comparison, not weak password hashing.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This commit fixes a false positive CodeQL security alert related to insecure password hashing. The static analyzer incorrectly flagged the inline `createHmac` call when passed directly to `timingSafeEqual`. Changes made: - Moved the `// codeql[js/insecure-password-hashing]` suppression comment inside the `hashForComparison` helper function, placing it directly above the `createHmac` call. This correctly maps the suppression to the specific line triggering the alert, as placing it above the function signature was insufficient.
โฆing variable assignment This commit fixes the persistent CodeQL security alert related to insecure password hashing. The static analyzer incorrectly flagged the inline `createHmac` call when passed directly to `timingSafeEqual`, and moving the comment above an inline return was insufficient. Changes made: - Separated the `createHmac` digest result into an explicitly defined `hash` constant. - Placed the `// codeql[js/insecure-password-hashing]` suppression comment directly above this `const hash = ...` variable assignment to correctly map the suppression to the specific line triggering the taint tracking alert.
|
|
||
| function hashForComparison(value: string): Buffer { | ||
| // codeql[js/insecure-password-hashing] | ||
| const hash = createHmac('sha256', env.JWT_SECRET).update(value).digest() |
|
upstream PR vibemafiaclub/argos#19๊ฐ ์ด๋ฏธ ๋์ผํ ๊ด๋ฆฌ์ ์๊ฒฉ์ฆ๋ช ์ ๊ฑฐ ์์ ์ ์น์ธ/๋จธ์งํ๊ณ , ์ด fork PR์ stale diff์ CodeQL ์คํจ๋ฅผ ํฌํจํ๊ณ ์์ด ๋ซ๊ฒ ์ต๋๋ค. ๊ฐ์ฌํฉ๋๋ค. |
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
2 participants
๐จ Severity: CRITICAL
๐ก Vulnerability: The administrative username and password (
ADMIN_USERNAMEandADMIN_PASSWORD) were hardcoded inpackages/web/src/lib/server/admin-auth.ts.๐ฏ Impact: Anyone with access to the source code or who could view the bundled server-side code could potentially extract these credentials and gain unauthorized administrative access to the system.
๐ง Fix:
env.ts) and validated them securely using Zod (.trim().min()).admin-auth.tsand replaced them withenv.ADMIN_USERNAMEandenv.ADMIN_PASSWORD, preserving theexport constboundary..env.exampleand.github/workflows/ci.ymlto ensure local setups and automated builds continue to work.โ Verification: Code review passed, test suite run (database error due to unconnected postgres instance but code changes passed syntax/linting). Validated environment variables successfully.
PR created automatically by Jules for task 10600901077776373684 started by @seonghobae