feat(functions): add request validation using zod#522
Conversation
📝 WalkthroughWalkthroughAdds Babel/Jest/TS tooling, Zod-based request validation and schemas, extracts reputation scoring into an exported helper with tests, replaces inline setRole validation with schema validation, refactors certificate generation/email/storage into an idempotent pipeline, updates small imports/ignores, and removes compiled source-map footers. ChangesCloud Functions Build, Validation, and Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 5
🧹 Nitpick comments (3)
cloud-functions/src/reputation.test.ts (2)
1-27: 💤 Low valueThese two tests don't exercise production code.
Both blocks recompute the scoring formula inline and assert the result against that same locally-computed expression, so they pass regardless of what
calculatePointsactually does. The meaningful coverage is in thecalculatePointssuite below (Lines 31-43); these duplicates can be dropped.♻️ Proposed removal
-describe("calculateReputation logic", () => { - test("calculates reputation points correctly", () => { - const attendanceCount = 2; - const registrationCount = 3; - const remindersSet = 1; - - const points = - attendanceCount * 10 + - registrationCount * 2 + - remindersSet; - - expect(points).toBe(27); - }); -}); - -test("calculates zero points correctly", () => { - const attendanceCount = 0; - const registrationCount = 0; - const remindersSet = 0; - - const points = - attendanceCount * 10 + - registrationCount * 2 + - remindersSet; - - expect(points).toBe(0); -}); -🤖 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 `@cloud-functions/src/reputation.test.ts` around lines 1 - 27, The two top-level tests in reputation.test.ts are redundant because they compute the scoring formula inline instead of calling the production function; remove the duplicate test blocks (the describe("calculateReputation logic" suite and the following standalone test) and ensure tests call the real function calculatePoints (used in the existing calculatePoints suite) so coverage exercises production code rather than re-implementing the formula locally.
29-29: 💤 Low valueMove the import to the top and use ESM
import.A mid-file
require("./reputation")is unusual in a.tstest and also triggers the module's top-leveladmin.initializeApp()/admin.firestore()at load time. Hoisting it as a top-levelimport { calculatePoints } from "./reputation";keeps load behavior predictable and consistent with the rest of the suite.🤖 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 `@cloud-functions/src/reputation.test.ts` at line 29, Replace the mid-file CommonJS require with a top-level ESM import to avoid triggering reputation module side-effects unpredictably: remove the in-function/near-line require("./reputation") and add a top-level statement import { calculatePoints } from "./reputation"; at the top of the test file so the reputation module (and its admin.initializeApp()/admin.firestore() calls) is loaded consistently with the rest of the suite.cloud-functions/tsconfig.json (1)
2-10: ⚡ Quick winRestore the dropped TypeScript safety checks.
This config keeps
strict, but it no longer enforcesnoImplicitReturnsandnoUnusedLocals, so missing return paths and dead validation branches can now passnpm run build. Please re-enable equivalent checks here or confirm they moved to another enforced CI step.Suggested config change
"compilerOptions": { "module": "commonjs", "target": "es2017", "outDir": "lib", "rootDir": "src", "strict": true, + "noImplicitReturns": true, + "noUnusedLocals": true, "esModuleInterop": true, "skipLibCheck": true },🤖 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 `@cloud-functions/tsconfig.json` around lines 2 - 10, The tsconfig compilerOptions dropped important safety flags; re-enable noImplicitReturns and noUnusedLocals under compilerOptions (alongside existing strict/esModuleInterop) so TypeScript will catch missing return paths and dead/unused local variables during local builds; update the tsconfig.json "compilerOptions" block to include "noImplicitReturns": true and "noUnusedLocals": true (or confirm and document that an equivalent CI TypeScript/tsc step enforces them if you intentionally moved enforcement).
🤖 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 `@cloud-functions/lib/certificateService.js`:
- Around line 211-228: The logs in the upload/send flow currently print raw
participant.email (seen in the console.error and console.log calls around the
send/upload block), which exposes PII; change those log statements to avoid raw
emails by logging participant.id or a deterministic redacted/hash of the email
(e.g., a hashEmail(participant.email) or redactEmail(participant.email))
instead, and update the error return object to omit the raw email (use the id or
hashed value for the email field) so getErrorMessage, signedUrl and returned
status remain but no raw email is written to logs or returned.
- Around line 109-123: The code currently creates a nearly permanent signed URL
in uploadPdfAndGetUrl and persists it in persistCertificateUrl; instead, stop
generating long-lived signed URLs and persist the storage object path (e.g.,
storagePath or file.name) and a issued timestamp. Update uploadPdfAndGetUrl to
only save the PDF and return its object path (not a signed URL), and change
persistCertificateUrl to store certificateObjectPath (or storagePath) and
certificateIssuedAt; when you need to send or download a PDF, generate a
short-lived signed URL on demand (use file.getSignedUrl with a short expires)
rather than saving one permanently.
In `@cloud-functions/lib/dailyDigest.js`:
- Around line 42-43: The compiled import uses a default export which breaks
runtime; change the import in dailyDigest.ts to use the named Expo export
(import { Expo } from 'expo-server-sdk';), keep the existing instantiation const
expo = new Expo(); and calls like Expo.isExpoPushToken(pushToken), then rebuild
the project so the generated lib/dailyDigest.js reflects the named-export shape
and restores the static isExpoPushToken method.
In `@cloud-functions/lib/eventNotifications.js`:
- Around line 78-120: sendPushNotifications currently throws on any chunk-level
ticket error which causes checkUpcomingEvents to retry the whole event and
resend already-delivered messages; instead make delivery idempotent by handling
errors per-ticket and persisting per-recipient or per-chunk success before
committing the event as notified. Update sendPushNotifications to return
detailed results (accepted indices, rejected tokens/reasons) instead of
throwing; in checkUpcomingEvents call sendPushNotifications and: 1) for accepted
tickets, immediately persist a per-recipient delivery mark (or update a
per-event deliveredTokens list) so retries skip them, 2) for unrecoverable token
errors remove or flag those tokens, and 3) only set eventDoc.ref.notified10Min
when all recipients are either marked delivered or permanently invalid;
reference functions/sendPushNotifications, checkUpcomingEvents,
buildMessagesForEvent and the notified10Min field to locate where to add
per-recipient persistence and change the control flow to avoid full-event
retries on partial success.
In `@cloud-functions/src/validation/schemas.ts`:
- Around line 3-13: The current z.object schemas (setRoleSchema and
getTopContributorsSchema) silently allow unknown fields; update both definitions
to use Zod's strict mode by appending .strict() to each z.object(...) so unknown
payload fields are rejected (e.g., setRoleSchema = z.object({...}).strict() and
getTopContributorsSchema = z.object({...}).strict()).
---
Nitpick comments:
In `@cloud-functions/src/reputation.test.ts`:
- Around line 1-27: The two top-level tests in reputation.test.ts are redundant
because they compute the scoring formula inline instead of calling the
production function; remove the duplicate test blocks (the
describe("calculateReputation logic" suite and the following standalone test)
and ensure tests call the real function calculatePoints (used in the existing
calculatePoints suite) so coverage exercises production code rather than
re-implementing the formula locally.
- Line 29: Replace the mid-file CommonJS require with a top-level ESM import to
avoid triggering reputation module side-effects unpredictably: remove the
in-function/near-line require("./reputation") and add a top-level statement
import { calculatePoints } from "./reputation"; at the top of the test file so
the reputation module (and its admin.initializeApp()/admin.firestore() calls) is
loaded consistently with the rest of the suite.
In `@cloud-functions/tsconfig.json`:
- Around line 2-10: The tsconfig compilerOptions dropped important safety flags;
re-enable noImplicitReturns and noUnusedLocals under compilerOptions (alongside
existing strict/esModuleInterop) so TypeScript will catch missing return paths
and dead/unused local variables during local builds; update the tsconfig.json
"compilerOptions" block to include "noImplicitReturns": true and
"noUnusedLocals": true (or confirm and document that an equivalent CI
TypeScript/tsc step enforces them if you intentionally moved enforcement).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cd2e6958-0ead-4667-bf86-b594018d680a
⛔ Files ignored due to path filters (1)
cloud-functions/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
cloud-functions/babel.config.jscloud-functions/jest.config.jscloud-functions/lib/analyzeAttendance.jscloud-functions/lib/certificateService.jscloud-functions/lib/dailyDigest.jscloud-functions/lib/eventNotifications.jscloud-functions/lib/index.jscloud-functions/lib/lib/participants.jscloud-functions/lib/onEventCreate.jscloud-functions/lib/reminders.jscloud-functions/lib/reputation.jscloud-functions/lib/server.jscloud-functions/lib/setRole.jscloud-functions/lib/utils/distance.jscloud-functions/lib/utils/fraudScore.jscloud-functions/package.jsoncloud-functions/src/reputation.test.tscloud-functions/src/reputation.tscloud-functions/src/setRole.tscloud-functions/src/validation/schemas.tscloud-functions/src/validation/validate.tscloud-functions/tsconfig.json
💤 Files with no reviewable changes (7)
- cloud-functions/lib/utils/distance.js
- cloud-functions/lib/lib/participants.js
- cloud-functions/lib/reminders.js
- cloud-functions/lib/onEventCreate.js
- cloud-functions/lib/utils/fraudScore.js
- cloud-functions/lib/server.js
- cloud-functions/lib/analyzeAttendance.js
| export const setRoleSchema = z.object({ | ||
| uid: z.string().min(1, "uid is required"), | ||
| role: z.enum(["admin", "club", "student"]), | ||
| }); | ||
|
|
||
| export const getTopContributorsSchema = z.object({ | ||
| limit: z.number().int().min(1).max(25).optional(), | ||
| lastPoints: z.number().optional(), | ||
| lastUserId: z.string().optional(), | ||
| startRank: z.number().int().min(1).optional(), | ||
| }); No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant schema file with line numbers
sed -n '1,120p' cloud-functions/src/validation/schemas.ts | nl -ba
# Check Zod import style/version signals
rg -n "from ['\"]zod['\"]|require\\(['\"]zod['\"]\\)|zod" -S cloud-functions/src/validation/schemas.ts cloud-functions/src/validation || true
rg -n "@?types/zod|zod\\b" package.json cloud-functions/package.json pnpm-lock.yaml yarn.lock package-lock.json 2>/dev/null || true
# Find any existing strict/unknown handling patterns used elsewhere in the repo
rg -n "strictObject\\(|.strict\\(|\\.passthrough\\(|\\.strip\\(|\\.unknown\\(|.catchall\\(" cloud-functions/src || true
rg -n "unknownKeys|strip|passthrough" cloud-functions/src || true
# Print the part of schemas.ts around the two schemas (in case there is more)
rg -n "setRoleSchema|getTopContributorsSchema" cloud-functions/src/validation/schemas.tsRepository: roshankumar0036singh/Uni-Event
Length of output: 117
🌐 Web query:
Zod strictObject unknown keys default behavior z.object extra keys accepted stripped vs rejected
💡 Result:
In Zod, the default behavior for z.object schemas is to strip unknown keys (keys not explicitly defined in the schema) during parsing [1][2]. This means that if an input object contains extra properties, they are excluded from the final parsed output [1][3]. To control this behavior, Zod provides the following methods: 1..strip: This explicitly enforces the default behavior of removing unknown keys [1][3]. 2..passthrough: This instructs Zod to include unknown keys in the parsed output instead of stripping them [1][3]. 3..strict: This causes Zod to throw an error if the input object contains any keys that are not explicitly defined in the schema [1][3]. For convenience, you can also use z.strictObject(...), which is functionally equivalent to z.object(...).strict() and is often preferred for data exploration or scenarios where you want to ensure the input strictly matches the defined schema [2][4]. Conversely, z.looseObject(...) acts similarly to using .passthrough() [2]. Note that these methods are not recursive; if you use .strict() on a parent object, it does not automatically enforce strictness on nested child objects [5]. Each object schema must be configured individually to control how unknown keys are handled [5].
Citations:
- 1: https://github.com/colinhacks/zod/blob/cc8ad1981ba580d1250520fde8878073d4b7d40a/README.md
- 2: https://zod.dev/api
- 3: https://github.com/colinhacks/zod/blob/c617ad3edbaf42971485f00042751771c335f9aa/README.md
- 4: https://mtsknn.fi/blog/zod-object-strict/
- 5: Strict by default or strict deep colinhacks/zod#2062
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Print the relevant part of schemas.ts with line numbers (without nl)
python3 - <<'PY'
from pathlib import Path
p = Path("cloud-functions/src/validation/schemas.ts")
lines = p.read_text().splitlines()
for i in range(1, min(len(lines), 40)+1):
print(f"{i:4d}\t{lines[i-1]}")
PY
# 2) Check Zod dependency/version
# Prefer cloud-functions/package.json if present
if [ -f cloud-functions/package.json ]; then
echo "---- cloud-functions/package.json ----"
rg -n "\"zod\"|zod@" cloud-functions/package.json || true
fi
# Also scan root package.json
if [ -f package.json ]; then
echo "---- root package.json ----"
rg -n "\"zod\"|zod@" package.json || true
fi
# 3) Search for existing strict/unknown-handling patterns used elsewhere
rg -n "strictObject\\(|\\.strict\\(|\\.passthrough\\(|\\.strip\\(|\\.unknown\\(|catchall\\(" cloud-functions/src || true
rg -n "unknownKeys|strip|passthrough" cloud-functions/src || trueRepository: roshankumar0036singh/Uni-Event
Length of output: 622
🌐 Web query:
z.strictObject(z.object) is functionally equivalent to z.object(...).strict() unknown keys handling default strip
💡 Result:
Yes, z.strictObject(shape) is functionally equivalent to z.object(shape).strict [1][2]. Both create an object schema that disallows unknown keys, causing Zod to throw an error if any keys not defined in the schema are present during parsing [2][3]. By default, z.object behavior is to "strip" unknown keys, meaning they are silently removed from the output and do not cause validation errors [1][3]. Using.strict (or z.strictObject) explicitly changes this behavior to "strict" mode [2]. While z.strictObject serves as a convenient shorthand, it is essentially a wrapper that applies the strict constraint upon initialization [1][2]. In newer versions of Zod (including the move toward Zod 4), maintainers have increasingly encouraged the use of z.strictObject and z.looseObject as preferred, explicit ways to define strictness [4]. Note that this strictness is not recursive; it only applies to the object level at which it is defined [5].
Citations:
- 1: https://mtsknn.fi/blog/zod-object-strict/
- 2: https://zod.dev/api
- 3: https://github.com/colinhacks/zod/blob/9575cfecbabeb149a4991f931cf24f685ea8c55e/README.md
- 4: https://zod.dev/v4/changelog
- 5: Strict by default or strict deep colinhacks/zod#2062
Reject unknown payload fields (switch these Zod schemas to strict mode)
cloud-functions/src/validation/schemas.ts defines setRoleSchema and getTopContributorsSchema with z.object(...). With Zod v4 (zod ^4.4.3), z.object strips unknown keys by default, so extra fields won’t cause validation to fail—undermining the “reject invalid payloads at function entry” contract. Use strict object schemas for both.
Suggested schema change
-export const setRoleSchema = z.object({
+export const setRoleSchema = z.strictObject({
uid: z.string().min(1, "uid is required"),
role: z.enum(["admin", "club", "student"]),
});
-export const getTopContributorsSchema = z.object({
+export const getTopContributorsSchema = z.strictObject({
limit: z.number().int().min(1).max(25).optional(),
lastPoints: z.number().optional(),
lastUserId: z.string().optional(),
startRank: z.number().int().min(1).optional(),
});🤖 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 `@cloud-functions/src/validation/schemas.ts` around lines 3 - 13, The current
z.object schemas (setRoleSchema and getTopContributorsSchema) silently allow
unknown fields; update both definitions to use Zod's strict mode by appending
.strict() to each z.object(...) so unknown payload fields are rejected (e.g.,
setRoleSchema = z.object({...}).strict() and getTopContributorsSchema =
z.object({...}).strict()).
|
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 `@cloud-functions/src/certificateService.ts`:
- Around line 277-280: The failed outcome object currently returned when email
sending fails is missing certificateUrl; update the object returned in the
email-failure catch block (the one setting email: participant.email || null,
status: 'failed', error: getErrorMessage(error)) to also include certificateUrl:
signedUrl so it matches handleExistingCertificateParticipant and reflects that
the cert was uploaded/persisted; locate this in certificateService.ts around the
email send error handling and add certificateUrl: signedUrl to the outcome.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 88264534-100d-4334-a21f-61adf15869b4
⛔ Files ignored due to path filters (14)
cloud-functions/lib/analyzeAttendance.js.mapis excluded by!**/*.mapcloud-functions/lib/certificateService.js.mapis excluded by!**/*.mapcloud-functions/lib/dailyDigest.js.mapis excluded by!**/*.mapcloud-functions/lib/eventNotifications.js.mapis excluded by!**/*.mapcloud-functions/lib/inactiveUsers.js.mapis excluded by!**/*.mapcloud-functions/lib/index.js.mapis excluded by!**/*.mapcloud-functions/lib/lib/participants.js.mapis excluded by!**/*.mapcloud-functions/lib/onEventCreate.js.mapis excluded by!**/*.mapcloud-functions/lib/reminders.js.mapis excluded by!**/*.mapcloud-functions/lib/reputation.js.mapis excluded by!**/*.mapcloud-functions/lib/server.js.mapis excluded by!**/*.mapcloud-functions/lib/setRole.js.mapis excluded by!**/*.mapcloud-functions/lib/utils/distance.js.mapis excluded by!**/*.mapcloud-functions/lib/utils/fraudScore.js.mapis excluded by!**/*.map
📒 Files selected for processing (15)
cloud-functions/.gitignorecloud-functions/lib/analyzeAttendance.jscloud-functions/lib/certificateService.jscloud-functions/lib/dailyDigest.jscloud-functions/lib/eventNotifications.jscloud-functions/lib/index.jscloud-functions/lib/lib/participants.jscloud-functions/lib/onEventCreate.jscloud-functions/lib/reminders.jscloud-functions/lib/reputation.jscloud-functions/lib/server.jscloud-functions/lib/setRole.jscloud-functions/lib/utils/distance.jscloud-functions/lib/utils/fraudScore.jscloud-functions/src/certificateService.ts
💤 Files with no reviewable changes (13)
- cloud-functions/lib/lib/participants.js
- cloud-functions/lib/utils/fraudScore.js
- cloud-functions/lib/server.js
- cloud-functions/lib/setRole.js
- cloud-functions/lib/analyzeAttendance.js
- cloud-functions/lib/eventNotifications.js
- cloud-functions/lib/dailyDigest.js
- cloud-functions/lib/index.js
- cloud-functions/lib/utils/distance.js
- cloud-functions/lib/onEventCreate.js
- cloud-functions/lib/certificateService.js
- cloud-functions/lib/reminders.js
- cloud-functions/lib/reputation.js
✅ Files skipped from review due to trivial changes (1)
- cloud-functions/.gitignore
| email: participant.email || null, | ||
| status: 'failed', | ||
| error: getErrorMessage(error), | ||
| }; |
There was a problem hiding this comment.
Include certificateUrl in the failed outcome for consistency and completeness.
At this point, the certificate has been successfully uploaded to storage (line 251) and persisted to Firestore (line 254). When email sending fails, the outcome should include certificateUrl: signedUrl to:
- Maintain consistency with
handleExistingCertificateParticipant(line 208), which includescertificateUrlin failed outcomes - Provide complete information about the partial success (cert generated/stored, email failed)
- Aid debugging and enable manual email delivery if needed
✨ Proposed fix to include certificateUrl
return {
email: participant.email || null,
status: 'failed',
error: getErrorMessage(error),
+ certificateUrl: signedUrl,
};📝 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.
| email: participant.email || null, | |
| status: 'failed', | |
| error: getErrorMessage(error), | |
| }; | |
| email: participant.email || null, | |
| status: 'failed', | |
| error: getErrorMessage(error), | |
| certificateUrl: signedUrl, | |
| }; |
🤖 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 `@cloud-functions/src/certificateService.ts` around lines 277 - 280, The failed
outcome object currently returned when email sending fails is missing
certificateUrl; update the object returned in the email-failure catch block (the
one setting email: participant.email || null, status: 'failed', error:
getErrorMessage(error)) to also include certificateUrl: signedUrl so it matches
handleExistingCertificateParticipant and reflects that the cert was
uploaded/persisted; locate this in certificateService.ts around the email send
error handling and add certificateUrl: signedUrl to the outcome.
1 participant
Description
Adds request validation for Firebase callable Cloud Functions using Zod.
This PR introduces a reusable validation utility and centralized schemas to validate incoming request payloads before processing. Invalid requests now return structured HttpsError responses with validation details, helping the frontend provide clearer error messages to users.
Changes Made
src/validation/validate.tsfor reusable schema validationsrc/validation/schemas.tsfor centralized request schemassetRolegetTopContributorsfunctions.https.HttpsErrorFixes #330
Type of change
How Has This Been Tested?
npm run buildChecklist
Summary by CodeRabbit
New Features
Improvements
Removals
Chores