Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 36 additions & 1 deletion backend/controllers/cronController.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ import { createClient } from "@supabase/supabase-js";
import webpush from "web-push";
import { sanitizeNotificationActionUrl } from "../utils/notificationActionUrl.js";
import { collectExpiredSubscriptionIds } from "../utils/pushDeliveryCleanup.js";
import {
isPushAllowedForCategory,
notificationTypeToCategory,
} from "../utils/notificationPreferences.js";

const getSupabaseClient = () => {
const supabaseUrl = process.env.SUPABASE_URL || process.env.VITE_SUPABASE_URL;
Expand Down Expand Up @@ -89,7 +93,7 @@ export const dispatchPushNotifications = async (req, res, next) => {
.is("push_sent_at", null)
.is("push_failed_at", null)
.or(`push_claimed_at.is.null,push_claimed_at.lt.${claimExpiryThreshold}`)
.select("id,user_id,title,body,action_url,push_attempts")
.select("id,user_id,title,body,action_url,push_attempts,type")
.limit(100);

if (claimError) {
Expand Down Expand Up @@ -123,6 +127,27 @@ export const dispatchPushNotifications = async (req, res, next) => {
return res.status(500).json({ error: subError.message });
}

const { data: profiles, error: prefsError } = await supabase
.from("profiles")
.select("id, notification_preferences")
.in("id", userIds);

if (prefsError) {
const notificationIds = notifications.map((n) => n.id);
if (notificationIds.length > 0) {
await supabase
.from("notifications")
.update({ push_claimed_at: null })
.in("id", notificationIds);
}
return res.status(500).json({ error: prefsError.message });
Comment on lines +135 to +143

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Handle failures when changing preference-related notification state.

If the profile query fails, check the claim rollback result. A rollback failure leaves claimed notifications unavailable until the claim TTL expires.

If the opt-out push_sent_at update fails, do not report the notification as processed. The row can otherwise be reclaimed and repeatedly evaluated.

Proposed fix
-        await supabase
+        const { error: rollbackError } = await supabase
           .from("notifications")
           .update({ push_claimed_at: null })
           .in("id", notificationIds);
+        if (rollbackError) {
+          return res.status(500).json({
+            error: `Preference fetch failed, and rollback failed: ${rollbackError.message}`,
+          });
+        }

-        await supabase
+        const { error: skipError } = await supabase
           .from("notifications")
           .update({ push_sent_at: new Date().toISOString() })
           .eq("id", notification.id);
+        if (skipError) throw skipError;

Also applies to: 165-168

🤖 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 `@backend/controllers/cronController.js` around lines 135 - 143, Update the
notification state handling in the cron controller: in the prefsError rollback
path, inspect the result of the notifications update that clears push_claimed_at
and handle or report rollback failures instead of ignoring them. In the opt-out
path, check the push_sent_at update result and ensure failures do not mark the
notification as processed or allow it to be reclaimed for repeated evaluation.

}

const prefsByUser = {};
for (const profile of profiles || []) {
prefsByUser[profile.id] = profile.notification_preferences;
}

// Group subscriptions by user_id for O(1) lookup per notification.
const subsByUser = {};
for (const sub of allSubscriptions || []) {
Expand All @@ -134,6 +159,16 @@ export const dispatchPushNotifications = async (req, res, next) => {
const expiredSubscriptionIds = new Set();

for (const notification of notifications) {
const category = notificationTypeToCategory(notification.type);
if (!isPushAllowedForCategory(prefsByUser[notification.user_id], category)) {
// Opted out — mark handled so the claim doesn't retry forever.
await supabase
.from("notifications")
.update({ push_sent_at: new Date().toISOString() })
.eq("id", notification.id);
continue;
}

const subscriptions = subsByUser[notification.user_id] || [];

const pushResults = await Promise.allSettled(
Expand Down
28 changes: 26 additions & 2 deletions backend/controllers/notificationController.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ import { createClient } from "@supabase/supabase-js";
import webpush from "web-push";
import { sanitizeNotificationActionUrl } from "../utils/notificationActionUrl.js";
import { collectExpiredSubscriptionIds } from "../utils/pushDeliveryCleanup.js";
import {
isPushAllowedForCategory,
resolvePushCategory,
} from "../utils/notificationPreferences.js";

const getSupabaseClient = () => {
const supabaseUrl = process.env.SUPABASE_URL || process.env.VITE_SUPABASE_URL;
Expand All @@ -27,7 +31,7 @@ export const sendPushNotification = async (req, res, next) => {
// Auth is already handled by either requireAuth or webhookSecret middleware in the route.
// Assuming requireAuth sets req.user

const { user_id, title, body, action_url } = req.body;
const { user_id, title, body, action_url, type, category } = req.body;

if (!user_id || !title || !body) {
return res.status(400).json({
Expand Down Expand Up @@ -60,6 +64,26 @@ export const sendPushNotification = async (req, res, next) => {
webpush.setVapidDetails(vapidSubject, vapidPublicKey, vapidPrivateKey);
const supabase = getSupabaseClient();
const safeActionUrl = sanitizeNotificationActionUrl(action_url);
const pushCategory = resolvePushCategory({ type, category });

const { data: profile, error: profileError } = await supabase
.from("profiles")
.select("notification_preferences")
.eq("id", user_id)
.maybeSingle();

if (profileError) {
return res.status(500).json({ error: profileError.message });
}

if (!isPushAllowedForCategory(profile?.notification_preferences, pushCategory)) {
return res.json({
sent: 0,
failed: 0,
skipped: true,
reason: "preference_disabled",
});
}

const { data: subscriptions, error } = await supabase
.from("push_subscriptions")
Expand Down Expand Up @@ -111,4 +135,4 @@ export const sendPushNotification = async (req, res, next) => {
} catch (error) {
next(error);
}
};
};
51 changes: 50 additions & 1 deletion backend/tests/dispatchPushNotifications.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const MAX_PUSH_ATTEMPTS = 5;
// ─── Shared mutable state across mock DB calls ───────────────────────────────
let dbRows = [];
let subscriptionStore = []; // { id, user_id, endpoint, p256dh, auth }
let profileStore = []; // { id, notification_preferences }
let endpointBehavior = {}; // endpoint -> "success" | "fail" | "expired"

const makeSupabaseMock = () => {
Expand Down Expand Up @@ -104,6 +105,13 @@ const makeSupabaseMock = () => {
return resolve({ data: subs, error: null });
}

// Preference lookup for claimed notification recipients.
if (table === "profiles" && !_operation) {
const userIds = _filters["id__in"] || [];
const rows = profileStore.filter((p) => userIds.includes(p.id));
return resolve({ data: rows, error: null });
}

return resolve({ data: [], error: null });
},
};
Expand Down Expand Up @@ -164,6 +172,7 @@ const seedRow = (overrides = {}) => ({
title: "Title",
body: "Body",
action_url: "/notifications",
type: "message",
push_sent_at: null,
push_claimed_at: null,
push_failed_at: null,
Expand All @@ -184,6 +193,7 @@ describe("dispatchPushNotifications", () => {
endpointBehavior = {};
dbRows = [];
subscriptionStore = [];
profileStore = [];
app = await buildApp();
});

Expand Down Expand Up @@ -361,4 +371,43 @@ describe("dispatchPushNotifications", () => {
expect(res2.body).toEqual({ sent: 1, processed: 1 });
});
});
});


describe("notification preferences (issue #1900)", () => {
it("skips push delivery when the category inApp preference is disabled", async () => {
dbRows = [
seedRow({ id: "notif-muted", user_id: "user-muted", type: "message" }),
];
subscriptionStore = [
{
id: "sub-muted",
user_id: "user-muted",
endpoint: "ep-muted",
p256dh: "k",
auth: "a",
},
];
profileStore = [
{
id: "user-muted",
notification_preferences: {
messages: { inApp: false },
sessions: { inApp: true },
friends: { inApp: true },
},
},
];
endpointBehavior["ep-muted"] = "success";

const webpush = (await import("web-push")).default;
webpush.sendNotification.mockClear();

const res = await request(app).post("/dispatch");
expect(res.status).toBe(200);
expect(res.body).toEqual({ sent: 0, processed: 1 });
expect(dbRows[0].push_sent_at).not.toBeNull();
expect(webpush.sendNotification).not.toHaveBeenCalled();
});
});

});
46 changes: 46 additions & 0 deletions backend/tests/notificationPreferences.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import { describe, expect, it } from "vitest";
import {
isPushAllowedForCategory,
notificationTypeToCategory,
resolvePushCategory,
} from "../utils/notificationPreferences.js";

describe("notificationTypeToCategory", () => {
it("maps message and session-related types onto settings categories", () => {
expect(notificationTypeToCategory("message")).toBe("messages");
expect(notificationTypeToCategory("session_reminder")).toBe("sessions");
expect(notificationTypeToCategory("mentorship_reminder")).toBe("sessions");
expect(notificationTypeToCategory("announcement")).toBe("sessions");
expect(notificationTypeToCategory("friend_request")).toBe("friends");
});

it("returns null for unmapped types", () => {
expect(notificationTypeToCategory("system")).toBeNull();
expect(notificationTypeToCategory(undefined)).toBeNull();
});
});

describe("isPushAllowedForCategory", () => {
it("allows delivery when preferences are missing or the category is unknown", () => {
expect(isPushAllowedForCategory(null, "messages")).toBe(true);
expect(isPushAllowedForCategory({}, null)).toBe(true);
});

it("suppresses push when the category inApp channel is disabled", () => {
const prefs = {
messages: { inApp: false },
sessions: { inApp: true },
friends: { inApp: true },
};

expect(isPushAllowedForCategory(prefs, "messages")).toBe(false);
expect(isPushAllowedForCategory(prefs, "sessions")).toBe(true);
});
});

describe("resolvePushCategory", () => {
it("prefers an explicit category over type mapping", () => {
expect(resolvePushCategory({ type: "message", category: "friends" })).toBe("friends");
expect(resolvePushCategory({ type: "session_reminder" })).toBe("sessions");
});
});
11 changes: 6 additions & 5 deletions backend/tests/uploadPhoto.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,9 @@ describe("POST /api/users/upload-photo", () => {

expect(res.status).toBe(200);
expect(res.body.success).toBe(true);
// Filename must contain the authenticated user's ID
expect(res.body.fileUrl).toMatch(new RegExp(`profile-${TEST_USER_ID}-`));
// Supabase storage path is scoped to the authenticated user's ID
expect(res.body.fileUrl).toContain(`${TEST_USER_ID}/`);
expect(storageUploadMock).toHaveBeenCalled();
Comment on lines +169 to +171

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Assert the storage path directly.

toHaveBeenCalled() proves only that an upload occurred. The URL assertion proves user scoping only if the getPublicUrl mock derives the URL from the upload path. Assert the first .upload() argument directly.

Suggested assertion
     expect(res.body.fileUrl).toContain(`${TEST_USER_ID}/`);
     expect(storageUploadMock).toHaveBeenCalled();
+    expect(storageUploadMock.mock.calls[0][0]).toMatch(
+      new RegExp(`^${TEST_USER_ID}/`),
+    );
📝 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.

Suggested change
// Supabase storage path is scoped to the authenticated user's ID
expect(res.body.fileUrl).toContain(`${TEST_USER_ID}/`);
expect(storageUploadMock).toHaveBeenCalled();
// Supabase storage path is scoped to the authenticated user's ID
expect(res.body.fileUrl).toContain(`${TEST_USER_ID}/`);
expect(storageUploadMock).toHaveBeenCalled();
expect(storageUploadMock.mock.calls[0][0]).toMatch(
new RegExp(`^${TEST_USER_ID}/`),
);
🤖 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 `@backend/tests/uploadPhoto.test.js` around lines 169 - 171, Update the upload
test around storageUploadMock to assert that its first call received a path
containing TEST_USER_ID followed by the expected filename or path structure.
Keep the existing fileUrl and upload-invocation assertions, but make the
user-scoping verification depend directly on the .upload() argument rather than
the generated public URL.

});

it("returns 200 when a valid JWT is supplied via HttpOnly cookie", async () => {
Expand Down Expand Up @@ -221,9 +222,9 @@ describe("POST /api/users/upload-photo", () => {
expect(res.body.error).toMatch(/no file/i);
});

it("returns 413 when the uploaded file exceeds the 5MB size limit", async () => {
it("returns 413 when the uploaded file exceeds the 2MB size limit", async () => {
const token = makeToken();
const oversized = Buffer.alloc(6 * 1024 * 1024, 0xff); // 6 MB of 0xFF bytes
const oversized = Buffer.alloc(3 * 1024 * 1024, 0xff); // 3 MB of 0xFF bytes
const res = await request(app)
.post("/api/users/upload-photo")
.set("Authorization", `Bearer ${token}`)
Expand All @@ -233,7 +234,7 @@ describe("POST /api/users/upload-photo", () => {
});

expect(res.status).toBe(413);
expect(res.body.error).toMatch(/5mb/i);
expect(res.body.error).toMatch(/2mb/i);
});
});

Expand Down
53 changes: 53 additions & 0 deletions backend/utils/notificationPreferences.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
export const DEFAULT_NOTIFICATION_PREFERENCES = {
messages: { email: false, inApp: true },
sessions: { email: false, inApp: true },
friends: { email: false, inApp: true },
};

/**
* Map a notifications.type enum value to a Settings preference category.
* Unmapped types (e.g. system) return null and are treated as allowed.
*/
export function notificationTypeToCategory(type) {
switch (type) {
case "message":
return "messages";
case "session_reminder":
case "mentorship_reminder":
case "mentorship_reminder_overdue":
case "announcement":
return "sessions";
case "friend_request":
case "connection_request":
return "friends";
default:
return null;
}
}

/**
* Push delivery follows the inApp channel for the given category.
* Missing prefs / unknown categories default to allowing delivery.
*/
export function isPushAllowedForCategory(preferences, category) {
if (!category) return true;

const prefs = preferences && typeof preferences === "object"
? preferences
: DEFAULT_NOTIFICATION_PREFERENCES;

const channel = prefs[category];
if (!channel || typeof channel !== "object") return true;

return channel.inApp !== false;
}

export function resolvePushCategory({ type, category } = {}) {
if (typeof category === "string" && category.length > 0) {
return category;
}
if (typeof type === "string" && type.length > 0) {
return notificationTypeToCategory(type);
}
return null;
}
34 changes: 34 additions & 0 deletions docs/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,3 +130,37 @@ Sends a browser push notification to all subscribed devices for a given `user_id
```

**Security**: Standard users may only send push notifications to themselves (IDOR prevention). Webhook callers authenticated via `WEBHOOK_SECRET` may send to any user.

## File Upload Routes

Authenticated multipart uploads are written to Supabase Storage. Storage paths are generated on the server from the caller's user id — clients cannot choose arbitrary object keys.

### `POST /api/upload`

General-purpose upload for `avatars`, `profiles`, and `resources` buckets.

**Auth**: valid Supabase JWT (`Authorization` header or `access_token` cookie)

**Form fields**:
- `folder`: one of `avatars`, `profiles`, `resources`
- `file`: the file to upload

**Validation**:
- MIME type must match the destination folder allow-list
- Magic byte / content-type verification rejects spoofed uploads
- Binary content and null bytes are rejected for text resource uploads

### `POST /api/users/upload-photo`

Profile-photo upload into the `profiles` bucket (2MB limit).

**Auth**: valid Supabase JWT (`Authorization` header or `access_token` cookie)

**Form fields**:
- `profilePhoto`: JPEG, PNG, WebP, or GIF image

**Validation**:
- 2MB size limit
- Strict image MIME allow-list
- Magic byte verification that file content matches the declared image type

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Correct the profile-photo MIME validation claim.

backend/routes/users.js checks the declared MIME and detected MIME against the allow-list separately. It does not verify that both values match. Therefore, an allowed JPEG declaration with PNG content is accepted.

Update this bullet to describe verification of an allowed image type, or add an explicit comparison in the route.

🤖 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 `@docs/api.md` at line 165, Update the profile-photo validation bullet to claim
only that the image type is checked against the allowed MIME types; do not claim
magic-byte content matching unless the corresponding comparison is added in the
user upload route.

- Per-user rate limit (10 uploads per hour)
Loading
Loading