-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add anonymous view notifications for non-logged-in viewers #1643
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 14 commits
cad016b
fbaa359
d47a072
7d7bfc4
a11e2da
b1d7d19
8d8229f
2ca2f56
93a7d14
83031ad
4962452
9f35e61
530e728
04dd97d
c1f59ee
57a49e9
cfccb35
f4d6cff
e77a15b
9136bb5
ec6eef3
d10e157
2a1e0df
8eae09f
b804dc7
0e406c5
a75b9ec
4c9d904
0c87ef5
915e508
509b9f5
d141666
ced5ce2
aa2fb75
ed0ce98
ff31881
a4d1625
935d152
d5674e0
9da8943
ee9a373
883bbf8
cb10822
4a03a8a
4d3c1c7
a734be2
6825367
9e854c9
3c69bca
cb235a3
0db13cc
cf495ee
2481d2b
2bcf989
5dfd9f1
c6c1eb2
080c81f
4305c6a
5ba1611
f587ef1
f3530e7
c1ec572
0cb4ad4
b4eb2d6
c37ec53
0223f5e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -19,7 +19,7 @@ const descriptionMap: Record<NotificationType, string> = { | |||||||||||||||
| reply: `replied to your comment`, | ||||||||||||||||
| view: `viewed your video`, | ||||||||||||||||
| reaction: `reacted to your video`, | ||||||||||||||||
| // mention: `mentioned you in a comment`, | ||||||||||||||||
| anon_view: `viewed your video`, | ||||||||||||||||
| }; | ||||||||||||||||
|
|
||||||||||||||||
| export const NotificationItem = ({ | ||||||||||||||||
|
|
@@ -36,6 +36,11 @@ export const NotificationItem = ({ | |||||||||||||||
| } | ||||||||||||||||
| }; | ||||||||||||||||
|
|
||||||||||||||||
| const isAnonView = notification.type === "anon_view"; | ||||||||||||||||
| const displayName = isAnonView | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor TS narrowing thing: using the discriminant inline avoids any chance
Suggested change
|
||||||||||||||||
| ? notification.anonName | ||||||||||||||||
| : notification.author.name; | ||||||||||||||||
|
|
||||||||||||||||
| return ( | ||||||||||||||||
| <Link | ||||||||||||||||
| href={link} | ||||||||||||||||
|
|
@@ -45,30 +50,39 @@ export const NotificationItem = ({ | |||||||||||||||
| className, | ||||||||||||||||
| )} | ||||||||||||||||
| > | ||||||||||||||||
| {/* Avatar */} | ||||||||||||||||
| <div className="relative flex-shrink-0"> | ||||||||||||||||
| <SignedImageUrl | ||||||||||||||||
| image={notification.author.avatar as ImageUpload.ImageUrl | null} | ||||||||||||||||
| name={notification.author.name} | ||||||||||||||||
| className="relative flex-shrink-0 size-7" | ||||||||||||||||
| letterClass="text-sm" | ||||||||||||||||
| /> | ||||||||||||||||
| {isAnonView ? ( | ||||||||||||||||
| <div className="relative flex-shrink-0 size-7 rounded-full bg-gray-3 flex items-center justify-center text-sm"> | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider reusing
Suggested change
|
||||||||||||||||
| 🐾 | ||||||||||||||||
| </div> | ||||||||||||||||
| ) : ( | ||||||||||||||||
| <SignedImageUrl | ||||||||||||||||
| image={notification.author.avatar as ImageUpload.ImageUrl | null} | ||||||||||||||||
| name={notification.author.name} | ||||||||||||||||
| className="relative flex-shrink-0 size-7" | ||||||||||||||||
| letterClass="text-sm" | ||||||||||||||||
| /> | ||||||||||||||||
| )} | ||||||||||||||||
| {notification.readAt === null && ( | ||||||||||||||||
| <div className="absolute top-0 right-0 size-2.5 rounded-full bg-red-500 border-2 border-gray-1"></div> | ||||||||||||||||
| )} | ||||||||||||||||
| </div> | ||||||||||||||||
|
|
||||||||||||||||
| {/* Content */} | ||||||||||||||||
| <div className="flex flex-col flex-1 justify-center"> | ||||||||||||||||
| <div className="flex gap-1 items-center"> | ||||||||||||||||
| <span className="font-medium text-gray-12 text-[13px]"> | ||||||||||||||||
| {notification.author.name} | ||||||||||||||||
| {displayName} | ||||||||||||||||
| </span> | ||||||||||||||||
| <span className="text-gray-10 text-[13px]"> | ||||||||||||||||
| {descriptionMap[notification.type]} | ||||||||||||||||
| </span> | ||||||||||||||||
| </div> | ||||||||||||||||
|
|
||||||||||||||||
| {isAnonView && notification.location && ( | ||||||||||||||||
| <p className="text-[13px] leading-4 text-gray-11"> | ||||||||||||||||
| {notification.location} | ||||||||||||||||
| </p> | ||||||||||||||||
| )} | ||||||||||||||||
| {(notification.type === "comment" || notification.type === "reply") && ( | ||||||||||||||||
| <p className="mb-2 text-[13px] h-fit italic leading-4 text-gray-11 line-clamp-2"> | ||||||||||||||||
| {notification.comment.content} | ||||||||||||||||
|
|
@@ -79,15 +93,15 @@ export const NotificationItem = ({ | |||||||||||||||
| </p> | ||||||||||||||||
| </div> | ||||||||||||||||
|
|
||||||||||||||||
| {/* Icon */} | ||||||||||||||||
| <div className="flex flex-shrink-0 items-center mt-1"> | ||||||||||||||||
| {notification.type === "comment" && ( | ||||||||||||||||
| <FontAwesomeIcon icon={faComment} className="text-gray-10 size-4" /> | ||||||||||||||||
| )} | ||||||||||||||||
| {notification.type === "reply" && ( | ||||||||||||||||
| <FontAwesomeIcon icon={faReply} className="text-gray-10 size-4" /> | ||||||||||||||||
| )} | ||||||||||||||||
| {notification.type === "view" && ( | ||||||||||||||||
| {(notification.type === "view" || | ||||||||||||||||
| notification.type === "anon_view") && ( | ||||||||||||||||
| <FontAwesomeIcon icon={faEye} className="text-gray-10 size-4" /> | ||||||||||||||||
| )} | ||||||||||||||||
| {notification.type === "reaction" && ( | ||||||||||||||||
|
|
@@ -104,9 +118,9 @@ function getLink(notification: APINotification) { | |||||||||||||||
| return `/s/${notification.videoId}/?reply=${notification.comment.id}`; | ||||||||||||||||
| case "comment": | ||||||||||||||||
| case "reaction": | ||||||||||||||||
| // case "mention": | ||||||||||||||||
| return `/s/${notification.videoId}/?comment=${notification.comment.id}`; | ||||||||||||||||
| default: | ||||||||||||||||
| case "view": | ||||||||||||||||
| case "anon_view": | ||||||||||||||||
| return `/s/${notification.videoId}`; | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,8 +4,44 @@ import { Effect, Option } from "effect"; | |||||||||||||||||||||||
| import type { NextRequest } from "next/server"; | ||||||||||||||||||||||||
| import UAParser from "ua-parser-js"; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| import { getAnonymousName } from "@/lib/anonymous-names"; | ||||||||||||||||||||||||
| import { createAnonymousViewNotification } from "@/lib/Notification"; | ||||||||||||||||||||||||
| import { runPromise } from "@/lib/server"; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| const anonNotifRateLimit = new Map< | ||||||||||||||||||||||||
| string, | ||||||||||||||||||||||||
| { count: number; resetAt: number } | ||||||||||||||||||||||||
| >(); | ||||||||||||||||||||||||
| const ANON_NOTIF_WINDOW_MS = 5 * 60 * 1000; | ||||||||||||||||||||||||
| const ANON_NOTIF_MAX_PER_VIDEO = 50; | ||||||||||||||||||||||||
| const ANON_NOTIF_MAX_ENTRIES = 10_000; | ||||||||||||||||||||||||
| let anonNotifCleanupCounter = 0; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| function checkAnonNotifRateLimit(videoId: string): boolean { | ||||||||||||||||||||||||
| anonNotifCleanupCounter++; | ||||||||||||||||||||||||
| if (anonNotifCleanupCounter % 100 === 0) { | ||||||||||||||||||||||||
| const now = Date.now(); | ||||||||||||||||||||||||
| for (const [k, v] of anonNotifRateLimit) { | ||||||||||||||||||||||||
| if (v.resetAt < now) anonNotifRateLimit.delete(k); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| if (anonNotifRateLimit.size > ANON_NOTIF_MAX_ENTRIES) | ||||||||||||||||||||||||
| anonNotifRateLimit.clear(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overflow clear resets active rate limits When Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/web/app/api/analytics/track/route.ts
Line: 21-29
Comment:
**Overflow clear resets active rate limits**
When `anonNotifRateLimit.size > ANON_NOTIF_MAX_ENTRIES` after the expired-entry sweep, the entire map is cleared with `anonNotifRateLimit.clear()`. This wipes the rate-limit counters for *all* currently-tracked videos simultaneously, creating a brief unprotected window where every video's counter is reset to zero. In practice this matters under heavy load: an attacker monitoring for the periodic counter-spike (every 100 requests) could exploit the reset window to inject another burst of 50 fake notifications per video per instance. A safer approach is to evict the oldest or least-recently-used entries rather than clearing the whole map.
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| const now = Date.now(); | ||||||||||||||||||||||||
| const entry = anonNotifRateLimit.get(videoId); | ||||||||||||||||||||||||
| if (!entry || entry.resetAt < now) { | ||||||||||||||||||||||||
| anonNotifRateLimit.set(videoId, { | ||||||||||||||||||||||||
| count: 1, | ||||||||||||||||||||||||
| resetAt: now + ANON_NOTIF_WINDOW_MS, | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| if (entry.count >= ANON_NOTIF_MAX_PER_VIDEO) return false; | ||||||||||||||||||||||||
| entry.count++; | ||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In-memory rate limiter is unreliable in serverless/multi-instance deployments
Additionally, a client that rotates its The database-level dedup ( Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/web/app/api/analytics/track/route.ts
Line: 11-43
Comment:
**In-memory rate limiter is unreliable in serverless/multi-instance deployments**
`anonNotifRateLimit` is a module-level `Map` that lives only in the memory of a single server instance. In a serverless deployment (Vercel, AWS Lambda, etc.) each function invocation may run in a separate process with its own blank copy of this map. This means the `ANON_NOTIF_MAX_PER_VIDEO = 50` limit applies *per-instance*, not globally. In practice a busy video could receive many multiples of 50 notification-creation attempts per window as traffic is spread across instances.
Additionally, a client that rotates its `sessionId` on every request can generate a fresh DB row (bypassing the dedup key) on every request until the per-instance limit is hit, allowing up to 50 spam notifications per instance per 5-minute window.
The database-level dedup (`onDuplicateKeyUpdate`) is the only true cross-instance guard, but it only deduplicates the *same* session — it does not protect against many different fake sessions. Consider persisting rate-limit state in Redis/KV (the same store likely used elsewhere in this project) so the limit is enforced globally across all instances.
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| interface TrackPayload { | ||||||||||||||||||||||||
| videoId: string; | ||||||||||||||||||||||||
| orgId?: string | null; | ||||||||||||||||||||||||
|
|
@@ -100,6 +136,34 @@ export async function POST(request: NextRequest) { | |||||||||||||||||||||||
| user_id: userId, | ||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||
| !userId && | ||||||||||||||||||||||||
| sessionId !== "anon" && | ||||||||||||||||||||||||
| checkAnonNotifRateLimit(body.videoId) | ||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor edge case: an empty/whitespace
Suggested change
|
||||||||||||||||||||||||
| const anonName = getAnonymousName(sessionId); | ||||||||||||||||||||||||
| const locationParts = [city, country].filter(Boolean); | ||||||||||||||||||||||||
| const location = | ||||||||||||||||||||||||
| locationParts.length > 0 ? locationParts.join(", ") : null; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| yield* Effect.tryPromise(() => | ||||||||||||||||||||||||
| createAnonymousViewNotification({ | ||||||||||||||||||||||||
| videoId: body.videoId, | ||||||||||||||||||||||||
| sessionId, | ||||||||||||||||||||||||
| anonName, | ||||||||||||||||||||||||
| location, | ||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||
| ).pipe( | ||||||||||||||||||||||||
| Effect.catchAll((error) => { | ||||||||||||||||||||||||
| console.error( | ||||||||||||||||||||||||
| "Failed to create anonymous view notification:", | ||||||||||||||||||||||||
| error, | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
| return Effect.void; | ||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| }).pipe(provideOptionalAuth), | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anon_viewlabel inFilterLabelsis unreachable UIanon_viewis not present in theFiltersarray (lines 5–11), so this entry inFilterLabelsis only there for TypeScript type completeness (becauseFilterTypeincludesanon_view). This is not a bug, but a comment explaining why the entry exists would help future readers understand it is intentional and not dead code that can be removed:Prompt To Fix With AI