fix(cron): reject "Bearer undefined" when CRON_SECRET is unset#29569
fix(cron): reject "Bearer undefined" when CRON_SECRET is unset#29569yashs33244 wants to merge 2 commits into
Conversation
… is unset
The tasker cleanup/cron routes and the calendar-subscription cron routes
authorize by comparing the Authorization header to `Bearer ${process.env.CRON_SECRET}`.
CRON_SECRET is not in .env.example, so on a default deployment it is undefined
and the literal becomes the string "Bearer undefined". An unauthenticated caller
sending "Authorization: Bearer undefined" passes the check and can trigger
tasker.cleanup() (which deletes data) and queue processing. The array-based
calendar-subscription checks had the same flaw via
[CRON_API_KEY, `Bearer ${CRON_SECRET}`].includes(...).
Make every check fail closed when the secret is unset: guard the single-secret
routes on a missing CRON_SECRET, and build the calendar-subscription allow-list
from defined values only while rejecting a missing apiKey. Document CRON_SECRET
in .env.example and declare it in the env type file. Add the tasker routes' first
auth tests plus a 'Bearer undefined' regression test for the cleanup cron route.
Fixes calcom#29565
|
Welcome to Cal.diy, @yashs33244! 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
|
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 (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe pull request introduces 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/features/tasker/api/__tests__/auth.test.ts (1)
105-135: ⚡ Quick winComplete cron route test coverage to match cleanup route.
The cleanup route has 4 test cases, but the cron route only has 3. The missing test is:
- "rejects a request with no authorization header when CRON_SECRET is unset"
Adding this test would ensure consistent coverage across both routes and complete the authentication test matrix.
📝 Suggested test case
test("rejects 'Bearer undefined' when CRON_SECRET is unset", async () => { vi.stubEnv("CRON_SECRET", undefined); const { GET } = await import("../cron"); const response = await GET(buildRequest("Bearer undefined")); expect(response.status).toBe(401); expect(processQueueMock).not.toHaveBeenCalled(); }); + test("rejects a request with no authorization header when CRON_SECRET is unset", async () => { + vi.stubEnv("CRON_SECRET", undefined); + const { GET } = await import("../cron"); + + const response = await GET(buildRequest()); + + expect(response.status).toBe(401); + expect(processQueueMock).not.toHaveBeenCalled(); + }); test("accepts the correct Bearer token via POST when CRON_SECRET is set", async () => {🤖 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/__tests__/auth.test.ts` around lines 105 - 135, Add a missing test case to the "cron route" describe block to match the cleanup route's test coverage. Create a new test that verifies rejection of a request with no authorization header (completely absent, not just "Bearer undefined") when CRON_SECRET is unset. This test should use buildRequest with no authorization header (or an empty string), expect a 401 status response, and verify that processQueueMock is not called. This test completes the authentication matrix by covering the case of a completely missing authorization header, complementing the existing test that checks for the "Bearer undefined" token.apps/web/app/api/cron/calendar-subscriptions-cleanup/__tests__/route.test.ts (1)
104-116: 💤 Low valueConsider clarifying test name to reflect both environment variables.
The test name mentions "when CRON_SECRET is unset," but the test also sets
CRON_API_KEYtoundefined(line 105). While the test is correct and comprehensively covers the default configuration scenario, the name could be more precise.📝 Suggested name improvement
- test("should return 403 for 'Bearer undefined' when CRON_SECRET is unset", async () => { + test("should return 403 for 'Bearer undefined' when CRON_SECRET and CRON_API_KEY are unset", async () => { vi.stubEnv("CRON_API_KEY", undefined); vi.stubEnv("CRON_SECRET", undefined);Alternatively, if you want to test the scenario where only
CRON_SECRETis unset:test("should return 403 for 'Bearer undefined' when CRON_SECRET is unset", async () => { - vi.stubEnv("CRON_API_KEY", undefined); vi.stubEnv("CRON_SECRET", undefined);🤖 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-cleanup/__tests__/route.test.ts` around lines 104 - 116, The test name for the function is misleading because it only mentions "CRON_SECRET is unset" but the test actually unsets both CRON_API_KEY and CRON_SECRET. Update the test name (the string parameter passed to the test function) to accurately reflect that both environment variables are being set to undefined, making the test's purpose clearer and more precise.
🤖 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 `@apps/web/app/api/cron/calendar-subscriptions/route.ts`:
- Line 33: Fix the spelling error in the error message returned by
NextResponse.json at line 33. Change "Forbiden" to "Forbidden" in the message
field to correct the typo in the HTTP 403 error response.
In `@packages/features/tasker/api/__tests__/auth.test.ts`:
- Line 34: In cleanup.ts at line 4, replace the barrel import `import tasker
from ".."` with a direct import from the tasker-factory module: `import {
getTasker } from "../tasker-factory"`. Then update any references to the
`tasker` variable in the file to use `getTasker()` function calls instead to
obtain the tasker instance.
---
Nitpick comments:
In
`@apps/web/app/api/cron/calendar-subscriptions-cleanup/__tests__/route.test.ts`:
- Around line 104-116: The test name for the function is misleading because it
only mentions "CRON_SECRET is unset" but the test actually unsets both
CRON_API_KEY and CRON_SECRET. Update the test name (the string parameter passed
to the test function) to accurately reflect that both environment variables are
being set to undefined, making the test's purpose clearer and more precise.
In `@packages/features/tasker/api/__tests__/auth.test.ts`:
- Around line 105-135: Add a missing test case to the "cron route" describe
block to match the cleanup route's test coverage. Create a new test that
verifies rejection of a request with no authorization header (completely absent,
not just "Bearer undefined") when CRON_SECRET is unset. This test should use
buildRequest with no authorization header (or an empty string), expect a 401
status response, and verify that processQueueMock is not called. This test
completes the authentication matrix by covering the case of a completely missing
authorization header, complementing the existing test that checks for the
"Bearer undefined" token.
🪄 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: ca6a6fe1-8114-47ca-b877-9f24f747b4d3
📒 Files selected for processing (9)
.env.exampleapps/web/app/api/cron/calendar-subscriptions-cleanup/__tests__/route.test.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/__tests__/auth.test.tspackages/features/tasker/api/cleanup.tspackages/features/tasker/api/cron.tspackages/types/environment.d.ts
|
|
||
| // The routes under test import from "..", which resolves to packages/features/tasker | ||
| // relative to those files. From this test file that module is "../../index". | ||
| vi.mock("../../index", () => ({ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if cleanup.ts and cron.ts use barrel imports
echo "=== Checking imports in cleanup.ts ==="
rg -n "^import.*from ['\"]\.\.(/index)?(\.ts)?['\"]" packages/features/tasker/api/cleanup.ts
echo -e "\n=== Checking imports in cron.ts ==="
rg -n "^import.*from ['\"]\.\.(/index)?(\.ts)?['\"]" packages/features/tasker/api/cron.ts
echo -e "\n=== Full import blocks for context ==="
rg -n -A1 "^import" packages/features/tasker/api/cleanup.ts packages/features/tasker/api/cron.ts | head -20Repository: calcom/cal.diy
Length of output: 873
🏁 Script executed:
#!/bin/bash
# Check what's exported from the index.ts barrel file
echo "=== Contents of packages/features/tasker/index.ts ==="
cat -n packages/features/tasker/index.ts
echo -e "\n=== Check all imports from '..' or barrel files in tasker/api ==="
rg -n "from ['\"]\.\.(/index)?(\.ts)?['\"]" packages/features/tasker/api/
echo -e "\n=== Check how 'tasker' is used in cleanup.ts ==="
rg -n "tasker\." packages/features/tasker/api/cleanup.tsRepository: calcom/cal.diy
Length of output: 888
🏁 Script executed:
#!/bin/bash
# Check the structure of tasker-factory to understand the correct import path
echo "=== Contents of packages/features/tasker/tasker-factory.ts ==="
head -30 packages/features/tasker/tasker-factory.ts
echo -e "\n=== Check if there are other files in packages/features/tasker root ==="
ls -la packages/features/tasker/*.ts 2>/dev/nullRepository: calcom/cal.diy
Length of output: 1909
Replace barrel import in cleanup.ts with direct import from tasker-factory.
cleanup.ts line 4 imports from ".." (resolving to the barrel file index.ts), which violates the guideline to import directly from source files. Replace import tasker from ".." with import { getTasker } from "../tasker-factory" and call getTasker() to obtain the tasker instance.
🤖 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/__tests__/auth.test.ts` at line 34, In
cleanup.ts at line 4, replace the barrel import `import tasker from ".."` with a
direct import from the tasker-factory module: `import { getTasker } from
"../tasker-factory"`. Then update any references to the `tasker` variable in the
file to use `getTasker()` function calls instead to obtain the tasker instance.
Source: Coding guidelines
|
|
Address review on calcom#29569: - Fix the misspelled "Forbiden" 403 message in the calendar-subscriptions route to match the sibling cron routes; update the route tests accordingly. - Add the missing 'no authorization header when CRON_SECRET is unset' case to the tasker cron auth tests so the cron and cleanup routes have symmetric coverage.
|
Thanks for the review! Addressed in 4f51692:
On the barrel-import note in Locally: tasker auth + calendar-subscriptions route tests pass (17/17), and the typo fix is red-green proven (reverting the source makes the two 403 assertions fail). |
Fixes #29565.
Summary
Several cron/tasker route handlers authorize by comparing the
Authorizationheader to`Bearer ${process.env.CRON_SECRET}`. WhenCRON_SECRETis unset (it is not in.env.example), that literal becomes the string"Bearer undefined", so a request withAuthorization: Bearer undefinedis accepted. The array-based routes had the same issue via[CRON_API_KEY, \Bearer ${CRON_SECRET}`].includes(...)`.This makes the checks fail-closed:
"Bearer undefined"match).filter(Boolean)) and reject a missingapiKeyCRON_SECRETin.env.exampleandpackages/types/environment.d.tsExisting status codes / messages are preserved to keep the diff focused.
Tests
Bearer undefinedregression testauth.test.ts7/7,calendar-subscriptions(-cleanup)19/19,biome lintclean,tscclean on changed filesNote
A review surfaced a related but separate misconfiguration class: four other cron routes can be bypassed if an operator explicitly sets
CRON_API_KEY="". That is outside this issue's scope — happy to follow up in a separate PR if useful.