fix(security): fail closed when CRON_SECRET is unset to prevent Bearer-undefined bypass#29654
Draft
codewithsupra wants to merge 7 commits into
Draft
fix(security): fail closed when CRON_SECRET is unset to prevent Bearer-undefined bypass#29654codewithsupra wants to merge 7 commits into
codewithsupra wants to merge 7 commits into
Conversation
…n rendering Tailwind's preflight CSS resets h1–h3 to unstyled text, so heading formatting set in the event description editor was invisible on the booking page. Apply the same inline-style pattern already used for <ul>, <ol>, and <a> in both markdownToSafeHTML (server) and markdownToSafeHTMLClient (client) so headings render with correct size and weight. Fixes calcom#29645 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When seatsPerTimeSlot is set and no booking exists yet, the else branch was unconditionally setting shouldReserveSlot = false, blocking the very first person from reserving a seated-event slot. The else block is incorrect: an absent booking means no seats are taken, so the slot is available. Remove it from both the tRPC handler and the v2 API service so that shouldReserveSlot stays true until attendees actually fill all seats. Adds regression tests covering the first-attendee (should reserve) and full-seats (should not reserve) cases. Fixes calcom#29448 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r-undefined bypass
When CRON_SECRET is not in the environment, template literals produce
"Bearer undefined". Endpoints that compared authHeader === \`Bearer \${process.env.CRON_SECRET}\`
would accept any request sending "Authorization: Bearer undefined",
giving unauthenticated callers access to tasker.cleanup() and
processor.processQueue().
The array-based calendar cron routes had the same problem: they included
\`Bearer \${process.env.CRON_SECRET}\` in the allowed-key list without
guarding against CRON_SECRET being undefined.
Fixes:
- tasker cleanup/cron: add !process.env.CRON_SECRET guard so the check
fails closed when the variable is absent
- calendar cron routes: build the allowed-key list by filtering out
undefined values before calling .includes()
- .env.example: document CRON_SECRET so deployers know to set it
Fixes calcom#29565
Contributor
|
Welcome to Cal.diy, @codewithsupra! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
github-actions
Bot
added
❗️ .env changes
Contributor
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/trpc/server/routers/viewer/slots/reserveSlot.handler.test.ts (1)
65-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
as unknown as anywith a typed Prisma test double.These casts bypass exactly the contract this test is supposed to protect, so the test can keep passing after Prisma shape changes. A narrow typed stub/helper is safer here. As per coding guidelines, "Never use
as any- use proper type-safe solutions instead."Also applies to: 101-116
🤖 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/trpc/server/routers/viewer/slots/reserveSlot.handler.test.ts` around lines 65 - 79, The Prisma test double in reserveSlot.handler.test.ts is using unsafe casts that bypass the contract this test should enforce. Replace the `prismaStub` and related setup in the `reserveSlot` test cases with a narrow, type-safe Prisma test helper/stub that matches the `eventType`, `booking`, and `selectedSlots` shapes expected by the handler, and remove the `as unknown as any` casts entirely. Keep the stub minimal but properly typed so Prisma shape changes surface as test failures rather than being hidden by `any`.Source: Coding guidelines
apps/web/app/api/cron/calendar-subscriptions/route.ts (1)
27-28: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winShare the cron allowlist builder across these routes.
The same
validCronKeysconstruction now appears incalendar-subscriptions,calendar-subscriptions-cleanup, andselected-calendars. Pulling it behind one validator would reduce the chance of the next secret-handling fix landing on only a subset of cron endpoints.🤖 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 `@apps/web/app/api/cron/calendar-subscriptions/route.ts` around lines 27 - 28, The cron allowlist construction is duplicated across the cron route handlers, so centralize it into one shared validator/helper and use it from calendar-subscriptions, calendar-subscriptions-cleanup, and selected-calendars. Move the `validCronKeys` building logic behind a reusable function or utility, then have each route call that shared helper before comparing `apiKey`, so secret handling stays consistent across all cron endpoints.packages/features/tasker/api/cron.ts (1)
8-9: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExtract this tasker auth predicate into a shared helper.
This exact guard now lives in both
packages/features/tasker/api/cron.tsandpackages/features/tasker/api/cleanup.ts. Centralizing it would make the next auth fix land once instead of relying on both endpoints staying in sync.🤖 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/features/tasker/api/cron.ts` around lines 8 - 9, Extract the duplicated tasker auth guard into a shared helper so both cron and cleanup use the same predicate. Move the `process.env.CRON_SECRET` plus `authHeader` Bearer check out of `cron.ts` into a reusable function and update the existing guard in `cleanup.ts` to call it too, keeping the behavior identical while centralizing the logic.
🤖 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 @.env.example:
- Around line 68-69: The CRON_SECRET comment is too broad and implies all cron
endpoints fail closed when it is unset, but the calendar cron routes still
support CRON_API_KEY. Update the documentation near CRON_SECRET to describe only
the auth contract that actually uses this secret, and make the wording explicit
that other cron routes may authenticate differently. Use the CRON_SECRET entry
in the env example and the calendar cron auth behavior as the reference points
when tightening the text.
In `@packages/trpc/server/routers/viewer/slots/reserveSlot.handler.test.ts`:
- Around line 54-56: The test cleanup in afterEach currently always deletes
NEXT_PUBLIC_WEBAPP_URL, which can overwrite an existing preconfigured value for
later tests. Update the cleanup in reserveSlot.handler.test.ts to preserve and
restore the previous process.env.NEXT_PUBLIC_WEBAPP_URL value around each test
instead of unconditionally deleting it, using the existing afterEach setup that
also calls vi.resetModules().
---
Nitpick comments:
In `@apps/web/app/api/cron/calendar-subscriptions/route.ts`:
- Around line 27-28: The cron allowlist construction is duplicated across the
cron route handlers, so centralize it into one shared validator/helper and use
it from calendar-subscriptions, calendar-subscriptions-cleanup, and
selected-calendars. Move the `validCronKeys` building logic behind a reusable
function or utility, then have each route call that shared helper before
comparing `apiKey`, so secret handling stays consistent across all cron
endpoints.
In `@packages/features/tasker/api/cron.ts`:
- Around line 8-9: Extract the duplicated tasker auth guard into a shared helper
so both cron and cleanup use the same predicate. Move the
`process.env.CRON_SECRET` plus `authHeader` Bearer check out of `cron.ts` into a
reusable function and update the existing guard in `cleanup.ts` to call it too,
keeping the behavior identical while centralizing the logic.
In `@packages/trpc/server/routers/viewer/slots/reserveSlot.handler.test.ts`:
- Around line 65-79: The Prisma test double in reserveSlot.handler.test.ts is
using unsafe casts that bypass the contract this test should enforce. Replace
the `prismaStub` and related setup in the `reserveSlot` test cases with a
narrow, type-safe Prisma test helper/stub that matches the `eventType`,
`booking`, and `selectedSlots` shapes expected by the handler, and remove the
`as unknown as any` casts entirely. Keep the stub minimal but properly typed so
Prisma shape changes surface as test failures rather than being hidden by `any`.
🪄 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: 5a8af310-c773-4781-9e3e-5351fc1bebc5
📒 Files selected for processing (11)
.env.exampleapps/api/v2/src/modules/slots/slots-2024-04-15/services/slots.service.tsapps/web/app/api/cron/calendar-subscriptions-cleanup/route.tsapps/web/app/api/cron/calendar-subscriptions/route.tsapps/web/app/api/cron/selected-calendars/route.tspackages/features/tasker/api/cleanup.tspackages/features/tasker/api/cron.tspackages/lib/markdownToSafeHTML.tspackages/lib/markdownToSafeHTMLClient.tspackages/trpc/server/routers/viewer/slots/reserveSlot.handler.test.tspackages/trpc/server/routers/viewer/slots/reserveSlot.handler.ts
💤 Files with no reviewable changes (2)
- apps/api/v2/src/modules/slots/slots-2024-04-15/services/slots.service.ts
- packages/trpc/server/routers/viewer/slots/reserveSlot.handler.ts
Contributor
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/lib/cronAuth.ts`:
- Around line 14-20: The auth check in cronAuth.ts is incorrectly allowing
CRON_SECRET through the query-string fallback because apiKey is compared against
the same allow-list as the Authorization header. Update the logic in the auth
helper so CRON_SECRET is accepted only when it comes from
req.headers.get("authorization"), while the query-param path continues to work
only for CRON_API_KEY. Keep the validation in the same cronAuth.ts flow and use
the existing apiKey/validKeys structure, but split the handling so Bearer
${process.env.CRON_SECRET} is never matched from
req.nextUrl.searchParams.get("apiKey").
🪄 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: 5bcab88e-5dfe-48af-a58e-01a8df783d16
📒 Files selected for processing (4)
apps/web/app/api/cron/calendar-subscriptions-cleanup/route.tsapps/web/app/api/cron/calendar-subscriptions/route.tsapps/web/app/api/cron/selected-calendars/route.tspackages/lib/cronAuth.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/api/cron/calendar-subscriptions-cleanup/route.ts
… .env.example wording
bandhan-majumder
requested changes
Jun 27, 2026
bandhan-majumder
left a comment
bandhan-majumder
left a comment
Member
There was a problem hiding this comment.
please attach a demo
…leaking env state
Author
|
Hi @bandhan-majumder, thanks for the review! Since this is a backend security fix with no UI change, here's a demonstration of the before/after behaviour using Before (vulnerable): # CRON_SECRET=mysecret
# This incorrectly authorised because ?apiKey was merged with the Authorization header check
curl -X POST https://your-app.com/api/cron/calendar-subscriptions?apiKey=Bearer%20mysecret
# → 200 OK ⚠️ bypassed the header-only contractAfter (fixed): # CRON_SECRET=mysecret — now only checked against Authorization header
curl -X POST https://your-app.com/api/cron/calendar-subscriptions?apiKey=Bearer%20mysecret
# → 401 Unauthorized ✅ query-param bypass blocked
# CRON_API_KEY=myapikey — still works from query param (intentional)
curl -X POST https://your-app.com/api/cron/calendar-subscriptions?apiKey=myapikey
# → 200 OK ✅
# Bearer header still works for both
curl -H "Authorization: Bearer mysecret" -X POST https://your-app.com/api/cron/calendar-subscriptions
# → 200 OK ✅The fix is in Also pushed a small follow-up commit to fix the test |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes an authentication bypass on cron/tasker endpoints that occurs when
CRON_SECRETis not set in the environment.Root cause
When
CRON_SECRETis absent from the environment, JavaScript template literals evaluateprocess.env.CRON_SECRETtoundefined, producing the literal string"Bearer undefined". Endpoints checkingauthHeader !== \Bearer ${process.env.CRON_SECRET}`would accept any request that sentAuthorization: Bearer undefined`.The array-based calendar cron routes had the same flaw —
\Bearer ${process.env.CRON_SECRET}`was included in the allowed-key array unconditionally, so"Bearer undefined"` was always an accepted key.Affected endpoints:
POST/GET /api/tasks/cron— triggersprocessor.processQueue()GET /api/tasks/cleanup— triggerstasker.cleanup()(deletes data)GET /api/cron/calendar-subscriptionsGET /api/cron/calendar-subscriptions-cleanupGET /api/cron/selected-calendarsFix
!process.env.CRON_SECRETguard so the check fails closed when the variable is absentundefinedvalues before calling.includes().env.example: documentCRON_SECRETso deployers know to set itFixes #29565