fix(security): fail closed on cron routes when CRON_SECRET is unset#29737
fix(security): fail closed on cron routes when CRON_SECRET is unset#29737TechGenius-Karan wants to merge 1 commit into
Conversation
|
Welcome to Cal.diy, @TechGenius-Karan! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis change introduces a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CronRoute
participant EnvVars
Client->>CronRoute: Request with apiKey/authorization header
CronRoute->>EnvVars: Read CRON_API_KEY, CRON_SECRET
EnvVars-->>CronRoute: Values (may be unset)
CronRoute->>CronRoute: Build validKeys list
alt validKeys empty or apiKey not matched
CronRoute-->>Client: 403/401 Forbidden/Unauthorized
else apiKey matches
CronRoute-->>Client: Proceed with handler logic
end
Related issues: None specified. Related PRs: None specified. Suggested labels: security, bug-fix, cron Suggested reviewers: None specified. Poem A rabbit hops through cron-lit code, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
What does this PR do?
Fixes #29565
when
CRON_SECRETisn't set (it's not in.env.example), the template literal evaluates to"Bearer undefined"— so any request with that exact header string passes auth. same goes for the array-based routes where["CRON_API_KEY_VALUE", "Bearer undefined"].includes(...)happily matches.tasker routes (
cleanup.ts,cron.ts) — added a!process.env.CRON_SECRETguard so the check fails closed when the secret is missing:calendar cron routes — replaced the array
.includes()with an explicitvalidKeysarray that only gets populated when the env vars are actually set. if both are unset,validKeysis empty and the check rejects:also added
CRON_SECRET=to.env.examplewith a generation note, fixed the"Forbiden"typo incalendar-subscriptions/route.ts, and added a regression test for the fail-closed path (both env vars unset → 403).Visual Demo (For contributors especially)
not applicable — auth logic fix, no UI changes.
Mandatory Tasks (DO NOT REMOVE)
CRON_SECRETto.env.examplewhich is the relevant docs.added a regression test to the existing
route.test.tsthat sendsBearer undefinedwith both env vars unset and asserts 403.How should this be tested?
CRON_SECRETin your envAuthorization: Bearer undefinedto/api/tasks/cleanupor/api/tasks/cronor run the unit tests:
cd apps/web && yarn test— the new test covers the fail-closed case.Checklist
noProcessEnvwarnings already present throughout these files)