[#130] Firebase로 dueDate가 하루 남은 Todo에 대해서 해당 계정으로 푸시 알람을 보내고 저장하는 기능을 구현한다#137
[#130] Firebase로 dueDate가 하루 남은 Todo에 대해서 해당 계정으로 푸시 알람을 보내고 저장하는 기능을 구현한다#137
Conversation
opficdev
commented
Feb 26, 2026
opficdev
commented
- closed Firebase로 dueDate가 곧 끝나가는 Todo에 대해서 해당 계정으로 푸시 알람을 보내는 기능을 구현한다 #130
Summary of ChangesHello @opficdev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 Pull Request는 Firebase를 활용하여 마감일이 하루 남은 할 일에 대한 푸시 알림 기능을 전면적으로 개선합니다. 기존의 개별 할 일 변경 기반 스케줄링 방식에서 사용자별 타임존을 고려한 주기적인 배치 스케줄링 방식으로 전환하여, 모든 사용자에게 정확한 현지 시간에 맞춰 알림이 전송되도록 합니다. 또한, 중복 알림 방지, 알림 기록의 안정성 강화, 그리고 사용자 데이터 저장 로직의 개선을 통해 전반적인 알림 시스템의 신뢰성과 사용자 경험을 향상시킵니다. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이 Pull Request는 Firebase를 사용하여 마감일이 하루 남은 Todo에 대한 푸시 알림을 보내고 저장하는 기능을 구현합니다. 특히 schedule.ts의 리팩토링을 통해 타임존을 고려한 알림 스케줄링을 가능하게 하여 사용자 경험을 향상시켰습니다. 하지만 Firebase Cloud Functions 구현에는 중요한 보안 및 확장성 문제가 존재합니다. 알림 디스패처는 Firestore 경로 주입에 취약하며, 스케줄러는 전체 사용자 컬렉션을 스캔하는 비효율적인 폴링 메커니즘을 사용하여 애플리케이션 확장 시 서비스 거부(DoS) 위험을 초래합니다. 전반적으로 이 PR은 Todo 알림 시스템의 핵심 기능을 추가하고 기존 로직을 크게 개선했지만, 이러한 보안 및 성능 문제를 해결하는 것이 중요합니다.
| let settingsDocument = try await settingsRef.getDocument() | ||
| if !settingsDocument.exists { | ||
| try await settingsRef.setData([ | ||
| "allowPushNotification": true, | ||
| "pushNotificationHour": 9, | ||
| "pushNotificationMinute": 0 | ||
| ], merge: true) | ||
| } |
There was a problem hiding this comment.
settingsTimeZoneUpdate가 settingsRef에 timeZone을 병합(merge)하여 설정하는 async let으로 실행됩니다. 이후 settingsDocument.exists를 확인하여 문서가 존재하지 않을 경우 allowPushNotification, pushNotificationHour, pushNotificationMinute를 설정합니다. 이 과정에서 settingsTimeZoneUpdate가 먼저 문서를 생성하고, 그 사이에 settingsRef.getDocument()가 호출되면 settingsDocument.exists가 true가 되어 기본 푸시 알림 설정이 누락될 수 있는 경쟁 조건(race condition)이 발생합니다. 신규 사용자의 경우 푸시 알림 기본 설정이 적용되지 않을 수 있는 심각한 버그로 이어질 수 있습니다. settingsRef에 대한 초기 쓰기 작업을 하나의 setData 호출로 통합하여 모든 기본 설정이 함께 적용되도록 하는 것이 안전합니다.
| let settingsDocument = try await settingsRef.getDocument() | |
| if !settingsDocument.exists { | |
| try await settingsRef.setData([ | |
| "allowPushNotification": true, | |
| "pushNotificationHour": 9, | |
| "pushNotificationMinute": 0 | |
| ], merge: true) | |
| } | |
| let settingsDocument = try await settingsRef.getDocument() | |
| if !settingsDocument.exists { | |
| try await settingsRef.setData([ | |
| "allowPushNotification": true, | |
| "pushNotificationHour": 9, | |
| "pushNotificationMinute": 0, | |
| "timeZone": TimeZone.autoupdatingCurrent.identifier | |
| ], merge: true) | |
| } else { | |
| // 문서가 이미 존재하면 timeZone만 업데이트 | |
| try await settingsRef.setData(["timeZone": TimeZone.autoupdatingCurrent.identifier], merge: true) | |
| } |
| var userField: [String: Any] = [ | ||
| "statusMsg": "", | ||
| "lastLogin": FieldValue.serverTimestamp() | ||
| "currentProvider": response.providerID | ||
| ] |
There was a problem hiding this comment.
이전 코드에서는 userField 초기화 시 lastLogin 필드를 FieldValue.serverTimestamp()로 설정하여 사용자의 마지막 로그인 시간을 기록했습니다. 현재 변경 사항에서는 이 필드가 완전히 제거되어 더 이상 업데이트되지 않습니다. lastLogin 필드가 중요한 사용자 추적 데이터였다면, 이 변경은 데이터 추적의 회귀로 이어질 수 있습니다. updatedAt 필드와는 다른 목적으로 사용될 수 있으므로, lastLogin 필드의 업데이트를 다시 추가하는 것을 고려해 주세요.
| var userField: [String: Any] = [ | |
| "statusMsg": "", | |
| "lastLogin": FieldValue.serverTimestamp() | |
| "currentProvider": response.providerID | |
| ] | |
| var userField: [String: Any] = [ | |
| "currentProvider": response.providerID, | |
| "lastLogin": FieldValue.serverTimestamp() | |
| ] |
| const settingsDoc = await admin.firestore().doc(`users/${userId}/userData/settings`).get(); | ||
| const allowPushNotification = settingsDoc.data()?.allowPushNotification ?? true; | ||
| if (!allowPushNotification) { | ||
| return; | ||
| } | ||
|
|
||
| const notificationDocId = `${todoId}_${dueDateKey}`; | ||
| const notificationDocRef = admin.firestore().doc(`users/${userId}/notifications/${notificationDocId}`); |
There was a problem hiding this comment.
The sendPushNotification function uses userId and todoId from the task payload to construct Firestore document paths without sanitization. If an attacker can trigger this function with arbitrary data (e.g., via a misconfigured Cloud Task or a leaked service account key), they could potentially read from or write to arbitrary Firestore collections by using path traversal characters (like /) in the userId or todoId fields. This is a classic path injection vulnerability in the context of Firestore document paths.
| const queue = getFunctions().taskQueue(`locations/${LOCATION}/functions/sendPushNotification`); | ||
| let usersSnapshot: FirebaseFirestore.QuerySnapshot<FirebaseFirestore.DocumentData>; | ||
| try { | ||
| usersSnapshot = await admin.firestore().collection("users").get(); |
There was a problem hiding this comment.
The scheduleTodoReminder function performs a full scan of the users collection every 5 minutes. This approach does not scale and will lead to function timeouts, high memory usage, and significant Firestore costs as the number of users grows. An attacker could exploit this by creating a large number of accounts, effectively disabling the notification system for all users and causing a Denial of Service (DoS). A more scalable approach would be to use Cloud Tasks to schedule individual notifications when a todo is created or updated, or to use a more targeted query to find users who need notifications.
| } catch { | ||
| print("Failed to update timeZone: \(error)") | ||
| } |
There was a problem hiding this comment.
에러 발생 시 print 문을 사용하는 것은 프로덕션 환경에서 적절한 로깅 방식이 아닙니다. 에러를 추적하고 디버깅하기 위해 전용 로깅 시스템(예: Crashlytics, Firebase Analytics의 커스텀 이벤트 등)을 사용하는 것이 좋습니다.
| } catch { | |
| print("Failed to update timeZone: \(error)") | |
| } | |
| print("Failed to update timeZone: \(error)") // TODO: 프로덕션 환경에 맞는 로깅 시스템으로 교체 | |
| } | ||
|
|
||
| const notificationData = { | ||
| title: "Todo 알림", |
There was a problem hiding this comment.
이전 코드에서는 notificationData의 title 필드가 req.data.title에서 동적으로 가져왔습니다. 하지만 현재 변경 사항에서는 "Todo 알림"으로 하드코딩되어 있습니다. 만약 알림 제목이 항상 "Todo 알림"으로 고정되어야 한다면 괜찮지만, 다양한 종류의 알림에 따라 제목을 다르게 설정할 필요가 있다면 req.data.title을 다시 사용하는 것이 좋습니다.
| title: "Todo 알림", | |
| title: title, // req.data.title 사용 |
1 participant
