From a24a4688ebe5c6370008ea3d7b3ce708d3cf7cfe Mon Sep 17 00:00:00 2001 From: antanst <> Date: Tue, 12 Aug 2025 18:55:47 +0300 Subject: [PATCH] notes,tasks: standardize forbidden responses and centralize access checks - Add hasAccess middleware to note/task GET by id endpoints - Ensure 403 Forbidden when user lacks access to existing resources - Preserve 404 for genuinely missing IDs - Update list queries to include shared items via ownershipOrPermissionWhere - Align subtasks endpoint to return 403 when parent not accessible - Update integration tests accordingly --- backend/routes/notes.js | 122 +++++++---- backend/routes/tasks.js | 225 ++++++++++++++++----- backend/tests/integration/notes.test.js | 12 +- backend/tests/integration/subtasks.test.js | 8 +- backend/tests/integration/tasks.test.js | 8 +- 5 files changed, 269 insertions(+), 106 deletions(-) diff --git a/backend/routes/notes.js b/backend/routes/notes.js index 3afa843..8e4f936 100644 --- a/backend/routes/notes.js +++ b/backend/routes/notes.js @@ -4,6 +4,8 @@ const { Op } = require('sequelize'); const { extractUidFromSlug } = require('../utils/slug-utils'); const { validateTagName } = require('../utils/validation'); const router = express.Router(); +const permissionsService = require('../services/permissionsService'); +const { hasAccess } = require('../middleware/authorize'); // Helper function to update note tags async function updateNoteTags(note, tagsArray, userId) { @@ -62,7 +64,10 @@ router.get('/notes', async (req, res) => { const orderBy = req.query.order_by || 'title:asc'; const [orderColumn, orderDirection] = orderBy.split(':'); - let whereClause = { user_id: req.session.userId }; + const whereClause = await permissionsService.ownershipOrPermissionWhere( + 'note', + req.session.userId + ); let includeClause = [ { model: Tag, @@ -97,31 +102,34 @@ router.get('/notes', async (req, res) => { }); // GET /api/note/:id (supports both numeric ID and uid-slug) -router.get('/note/:id', async (req, res) => { +router.get( + '/note/:id', + hasAccess( + 'ro', + 'note', + async (req) => { + const identifier = req.params.id; + if (/^\d+$/.test(identifier)) { + const n = await Note.findOne({ where: { id: parseInt(identifier) }, attributes: ['uid'] }); + return n?.uid; + } + const uid = extractUidFromSlug(identifier); + return uid; + }, + { notFoundMessage: 'Note not found.' } + ), + async (req, res) => { try { - if (!req.session || !req.session.userId) { - return res.status(401).json({ error: 'Authentication required' }); - } - const identifier = req.params.id; let whereClause; - - // Check if identifier is numeric (regular ID) or uid-slug if (/^\d+$/.test(identifier)) { - // It's a numeric ID - whereClause = { - id: parseInt(identifier), - user_id: req.session.userId, - }; + whereClause = { id: parseInt(identifier) }; } else { - // It's a uid-slug, extract the uid const uid = extractUidFromSlug(identifier); if (!uid) { - return res - .status(400) - .json({ error: 'Invalid note identifier' }); + return res.status(400).json({ error: 'Invalid note identifier' }); } - whereClause = { uid: uid, user_id: req.session.userId }; + whereClause = { uid }; } const note = await Note.findOne({ @@ -143,13 +151,15 @@ router.get('/note/:id', async (req, res) => { if (!note) { return res.status(404).json({ error: 'Note not found.' }); } + // access ensured by middleware res.json(note); } catch (error) { console.error('Error fetching note:', error); res.status(500).json({ error: 'Internal server error' }); } -}); +} +); // POST /api/note router.post('/note', async (req, res) => { @@ -168,12 +178,20 @@ router.post('/note', async (req, res) => { // Handle project assignment if (project_id && project_id.toString().trim()) { - const project = await Project.findOne({ - where: { id: project_id, user_id: req.session.userId }, - }); + const project = await Project.findOne({ where: { id: project_id } }); if (!project) { return res.status(400).json({ error: 'Invalid project.' }); } + const projectAccess = await permissionsService.getAccess( + req.session.userId, + 'project', + project.uid + ); + const isOwner = project.user_id === req.session.userId; + const canWrite = isOwner || projectAccess === 'rw' || projectAccess === 'admin'; + if (!canWrite) { + return res.status(403).json({ error: 'Forbidden' }); + } noteAttributes.project_id = project_id; } @@ -223,19 +241,27 @@ router.post('/note', async (req, res) => { }); // PATCH /api/note/:id -router.patch('/note/:id', async (req, res) => { +router.patch( + '/note/:id', + hasAccess( + 'rw', + 'note', + async (req) => { + const n = await Note.findOne({ where: { id: req.params.id }, attributes: ['uid'] }); + return n?.uid; + }, + { notFoundMessage: 'Note not found.' } + ), + async (req, res) => { try { - if (!req.session || !req.session.userId) { - return res.status(401).json({ error: 'Authentication required' }); - } - const note = await Note.findOne({ - where: { id: req.params.id, user_id: req.session.userId }, + where: { id: req.params.id }, }); if (!note) { return res.status(404).json({ error: 'Note not found.' }); } + // access ensured by middleware const { title, content, project_id, tags } = req.body; @@ -246,12 +272,20 @@ router.patch('/note/:id', async (req, res) => { // Handle project assignment if (project_id !== undefined) { if (project_id && project_id.toString().trim()) { - const project = await Project.findOne({ - where: { id: project_id, user_id: req.session.userId }, - }); + const project = await Project.findOne({ where: { id: project_id } }); if (!project) { return res.status(400).json({ error: 'Invalid project.' }); } + const projectAccess = await permissionsService.getAccess( + req.session.userId, + 'project', + project.uid + ); + const isOwner = project.user_id === req.session.userId; + const canWrite = isOwner || projectAccess === 'rw' || projectAccess === 'admin'; + if (!canWrite) { + return res.status(403).json({ error: 'Forbidden' }); + } updateData.project_id = project_id; } else { updateData.project_id = null; @@ -299,22 +333,31 @@ router.patch('/note/:id', async (req, res) => { : [error.message], }); } -}); +} +); // DELETE /api/note/:id -router.delete('/note/:id', async (req, res) => { +router.delete( + '/note/:id', + hasAccess( + 'rw', + 'note', + async (req) => { + const n = await Note.findOne({ where: { id: req.params.id }, attributes: ['uid'] }); + return n?.uid; + }, + { notFoundMessage: 'Note not found.' } + ), + async (req, res) => { try { - if (!req.session || !req.session.userId) { - return res.status(401).json({ error: 'Authentication required' }); - } - const note = await Note.findOne({ - where: { id: req.params.id, user_id: req.session.userId }, + where: { id: req.params.id }, }); if (!note) { return res.status(404).json({ error: 'Note not found.' }); } + // access ensured by middleware await note.destroy(); res.json({ message: 'Note deleted successfully.' }); @@ -324,6 +367,7 @@ router.delete('/note/:id', async (req, res) => { error: 'There was a problem deleting the note.', }); } -}); +} +); module.exports = router; diff --git a/backend/routes/tasks.js b/backend/routes/tasks.js index bc9c3c3..64575b4 100644 --- a/backend/routes/tasks.js +++ b/backend/routes/tasks.js @@ -1,6 +1,8 @@ const express = require('express'); const { Task, Tag, Project, TaskEvent, sequelize } = require('../models'); const { Op } = require('sequelize'); +const permissionsService = require('../services/permissionsService'); +const { hasAccess } = require('../middleware/authorize'); const { generateRecurringTasks, handleTaskCompletion, @@ -454,16 +456,19 @@ async function undoneAllSubtasks(parentTaskId, userId) { // Filter tasks by parameters async function filterTasksByParams(params, userId, userTimezone) { - // Disable search functionality for upcoming view - if (params.type === 'upcoming') { +// Include owned or shared tasks; exclude subtasks by default + const ownedOrShared = await permissionsService.ownershipOrPermissionWhere( + 'task', + 'userId' + ); +if (params.type === 'upcoming') { // Remove search-related parameters to prevent search functionality params = { ...params, client_side_filtering: false }; delete params.search; } - let whereClause = { - user_id: userId, - parent_task_id: null, // Exclude subtasks from main task lists + ...ownedOrShared, + parent_task_id: null, }; // Include both recurring templates and instances, but handle them appropriately @@ -567,7 +572,7 @@ async function filterTasksByParams(params, userId, userTimezone) { // Override the default whereClause to include recurring instances // NOTE: Search functionality is disabled for upcoming view - ignore client_side_filtering whereClause = { - user_id: userId, + ...ownedOrShared, parent_task_id: null, // Exclude subtasks from main task lists due_date: { [Op.between]: [upcomingRange.start, upcomingRange.end], @@ -699,19 +704,19 @@ async function filterTasksByParams(params, userId, userTimezone) { // Compute task metrics async function computeTaskMetrics(userId, userTimezone = 'UTC') { + const visibleTasksWhere = await permissionsService.ownershipOrPermissionWhere('task', userId); const totalOpenTasks = await Task.count({ - where: { - user_id: userId, + where: { ...visibleTasksWhere, status: { [Op.ne]: Task.STATUS.DONE }, - parent_task_id: null, // Exclude subtasks - recurring_parent_id: null, // Exclude recurring instances - }, - }); + parent_task_id: null, // Exclude subtasks + recurring_parent_id: null, // Exclude recurring instances + } + }); const oneMonthAgo = new Date(Date.now() - 30 * 24 * 60 * 60 * 1000); const tasksPendingOverMonth = await Task.count({ where: { - user_id: userId, + ...visibleTasksWhere, status: { [Op.ne]: Task.STATUS.DONE }, created_at: { [Op.lt]: oneMonthAgo }, parent_task_id: null, // Exclude subtasks @@ -721,7 +726,7 @@ async function computeTaskMetrics(userId, userTimezone = 'UTC') { const tasksInProgress = await Task.findAll({ where: { - user_id: userId, + ...visibleTasksWhere, status: { [Op.in]: [Task.STATUS.IN_PROGRESS, 'in_progress'] }, parent_task_id: null, // Exclude subtasks recurring_parent_id: null, // Exclude recurring instances @@ -758,7 +763,7 @@ async function computeTaskMetrics(userId, userTimezone = 'UTC') { // Get tasks in today plan const todayPlanTasks = await Task.findAll({ where: { - user_id: userId, + ...visibleTasksWhere, today: true, status: { [Op.notIn]: [ @@ -809,7 +814,7 @@ async function computeTaskMetrics(userId, userTimezone = 'UTC') { const tasksDueToday = await Task.findAll({ where: { - user_id: userId, + ...visibleTasksWhere, status: { [Op.notIn]: [ Task.STATUS.DONE, @@ -889,7 +894,7 @@ async function computeTaskMetrics(userId, userTimezone = 'UTC') { // Get tasks without projects (excluding someday tagged tasks) const nonProjectTasks = await Task.findAll({ where: { - user_id: userId, + ...visibleTasksWhere, status: { [Op.in]: [Task.STATUS.NOT_STARTED, Task.STATUS.WAITING], }, @@ -934,7 +939,7 @@ async function computeTaskMetrics(userId, userTimezone = 'UTC') { // Get tasks with projects (excluding someday tagged tasks) const projectTasks = await Task.findAll({ where: { - user_id: userId, + ...visibleTasksWhere, status: { [Op.in]: [Task.STATUS.NOT_STARTED, Task.STATUS.WAITING], }, @@ -1315,8 +1320,10 @@ router.get('/task', async (req, res) => { .json({ error: 'uid query parameter is required' }); } - const task = await Task.findOne({ - where: { uid: uid, user_id: req.currentUser.id }, + const task = await Task.findOne({ + where: { + uid: uid, + }, include: [ { model: Tag, @@ -1347,6 +1354,16 @@ router.get('/task', async (req, res) => { return res.status(404).json({ error: 'Task not found.' }); } + // Ensure read access to the task + const access = await permissionsService.getAccess( + req.currentUser.id, + 'task', + task.uid + ); + if (access === 'none') { + return res.status(403).json({ error: 'Forbidden' }); + } + const serializedTask = await serializeTask( task, req.currentUser.timezone @@ -1360,10 +1377,24 @@ router.get('/task', async (req, res) => { }); // GET /api/task/:id -router.get('/task/:id', async (req, res) => { +router.get( + '/task/:id', + hasAccess( + 'ro', + 'task', + async (req) => { + const t = await Task.findOne({ + where: { id: req.params.id }, + attributes: ['uid'], + }); + return t?.uid; + }, + { notFoundMessage: 'Task not found.' } + ), + async (req, res) => { try { const task = await Task.findOne({ - where: { id: req.params.id, user_id: req.currentUser.id }, + where: { id: req.params.id }, include: [ { model: Tag, @@ -1389,9 +1420,9 @@ router.get('/task/:id', async (req, res) => { ], }); - if (!task) { - return res.status(404).json({ error: 'Task not found.' }); - } + if (!task) { + return res.status(404).json({ error: 'Task not found.' }); + } const serializedTask = await serializeTask( task, @@ -1399,20 +1430,34 @@ router.get('/task/:id', async (req, res) => { { skipDisplayNameTransform: true } ); - res.json(serializedTask); - } catch (error) { - console.error('Error fetching task:', error); - res.status(500).json({ error: 'Internal server error' }); + res.json(serializedTask); + } catch (error) { + console.error('Error fetching task:', error); + res.status(500).json({ error: 'Internal server error' }); + } } -}); +); // GET /api/task/:id/subtasks router.get('/task/:id/subtasks', async (req, res) => { try { + // Ensure parent visibility first + const parent = await Task.findOne({ where: { id: req.params.id } }); + if (!parent) { + return res.json([]); + } + const pAccess = await permissionsService.getAccess( + req.currentUser.id, + 'task', + parent.uid + ); + if (pAccess === 'none') { + return res.status(403).json({ error: 'Forbidden' }); + } + const subtasks = await Task.findAll({ where: { parent_task_id: req.params.id, - user_id: req.currentUser.id, }, include: [ { @@ -1513,23 +1558,39 @@ router.post('/task', async (req, res) => { // Handle project assignment if (project_id && project_id.toString().trim()) { - const project = await Project.findOne({ - where: { id: project_id, user_id: req.currentUser.id }, - }); + const project = await Project.findOne({ where: { id: project_id } }); if (!project) { return res.status(400).json({ error: 'Invalid project.' }); } + const projectAccess = await permissionsService.getAccess( + req.currentUser.id, + 'project', + project.uid + ); + const isOwner = project.user_id === req.currentUser.id; + const canWrite = isOwner || projectAccess === 'rw' || projectAccess === 'admin'; + if (!canWrite) { + return res.status(403).json({ error: 'Forbidden' }); + } taskAttributes.project_id = project_id; } // Handle parent task assignment if (parent_task_id && parent_task_id.toString().trim()) { - const parentTask = await Task.findOne({ - where: { id: parent_task_id, user_id: req.currentUser.id }, - }); + const parentTask = await Task.findOne({ where: { id: parent_task_id } }); if (!parentTask) { return res.status(400).json({ error: 'Invalid parent task.' }); } + const parentAccess = await permissionsService.getAccess( + req.currentUser.id, + 'task', + parentTask.uid + ); + const isOwner = parentTask.user_id === req.currentUser.id; + const canWrite = isOwner || parentAccess === 'rw' || parentAccess === 'admin'; + if (!canWrite) { + return res.status(400).json({ error: 'Invalid parent task.' }); + } taskAttributes.parent_task_id = parent_task_id; } @@ -1644,7 +1705,18 @@ router.post('/task', async (req, res) => { }); // PATCH /api/task/:id -router.patch('/task/:id', async (req, res) => { +router.patch( + '/task/:id', + hasAccess( + 'rw', + 'task', + async (req) => { + const t = await Task.findOne({ where: { id: req.params.id }, attributes: ['uid'] }); + return t?.uid; + }, + { notFoundMessage: 'Task not found.' } + ), + async (req, res) => { try { const { name, @@ -1672,7 +1744,7 @@ router.patch('/task/:id', async (req, res) => { const tagsData = tags || Tags; const task = await Task.findOne({ - where: { id: req.params.id, user_id: req.currentUser.id }, + where: { id: req.params.id }, include: [ { model: Tag, @@ -1685,6 +1757,7 @@ router.patch('/task/:id', async (req, res) => { if (!task) { return res.status(404).json({ error: 'Task not found.' }); } + // access ensured by middleware // Capture old values for event logging const oldValues = { @@ -1837,12 +1910,20 @@ router.patch('/task/:id', async (req, res) => { // Handle project assignment if (project_id && project_id.toString().trim()) { - const project = await Project.findOne({ - where: { id: project_id, user_id: req.currentUser.id }, - }); + const project = await Project.findOne({ where: { id: project_id } }); if (!project) { return res.status(400).json({ error: 'Invalid project.' }); } + const projectAccess = await permissionsService.getAccess( + req.currentUser.id, + 'project', + project.uid + ); + const isOwner = project.user_id === req.currentUser.id; + const canWrite = isOwner || projectAccess === 'rw' || projectAccess === 'admin'; + if (!canWrite) { + return res.status(403).json({ error: 'Forbidden' }); + } taskAttributes.project_id = project_id; } else { taskAttributes.project_id = null; @@ -2234,13 +2315,25 @@ router.patch('/task/:id', async (req, res) => { : [error.message], }); } -}); +} +); // PATCH /api/task/:id/toggle_completion -router.patch('/task/:id/toggle_completion', async (req, res) => { +router.patch( + '/task/:id/toggle_completion', + hasAccess( + 'rw', + 'task', + async (req) => { + const t = await Task.findOne({ where: { id: req.params.id }, attributes: ['uid'] }); + return t?.uid; + }, + { notFoundMessage: 'Task not found.' } + ), + async (req, res) => { try { const task = await Task.findOne({ - where: { id: req.params.id, user_id: req.currentUser.id }, + where: { id: req.params.id }, include: [ { model: Tag, @@ -2269,6 +2362,7 @@ router.patch('/task/:id/toggle_completion', async (req, res) => { if (!task) { return res.status(404).json({ error: 'Task not found.' }); } + // access ensured by middleware // Track if parent-child logic was executed let parentChildLogicExecuted = false; @@ -2383,13 +2477,25 @@ router.patch('/task/:id/toggle_completion', async (req, res) => { details: error.message, }); } -}); +} +); // DELETE /api/task/:id -router.delete('/task/:id', async (req, res) => { +router.delete( + '/task/:id', + hasAccess( + 'rw', + 'task', + async (req) => { + const t = await Task.findOne({ where: { id: req.params.id }, attributes: ['uid'] }); + return t?.uid; + }, + { notFoundMessage: 'Task not found.' } + ), + async (req, res) => { try { const task = await Task.findOne({ - where: { id: req.params.id, user_id: req.currentUser.id }, + where: { id: req.params.id }, }); if (!task) { @@ -2516,7 +2622,8 @@ router.delete('/task/:id', async (req, res) => { error: 'There was a problem deleting the task.', }); } -}); +} +); // POST /api/tasks/generate-recurring router.post('/tasks/generate-recurring', async (req, res) => { @@ -2539,10 +2646,21 @@ router.post('/tasks/generate-recurring', async (req, res) => { }); // PATCH /api/task/:id/toggle-today -router.patch('/task/:id/toggle-today', async (req, res) => { +router.patch( + '/task/:id/toggle-today', + hasAccess( + 'rw', + 'task', + async (req) => { + const t = await Task.findOne({ where: { id: req.params.id }, attributes: ['uid'] }); + return t?.uid; + }, + { notFoundMessage: 'Task not found.' } + ), + async (req, res) => { try { const task = await Task.findOne({ - where: { id: req.params.id, user_id: req.currentUser.id }, + where: { id: req.params.id }, include: [ { model: Tag, @@ -2561,6 +2679,8 @@ router.patch('/task/:id/toggle-today', async (req, res) => { return res.status(404).json({ error: 'Task not found.' }); } + // access ensured by middleware + // Toggle the today flag const newTodayValue = !task.today; const updateData = { today: newTodayValue }; @@ -2601,7 +2721,8 @@ router.patch('/task/:id/toggle-today', async (req, res) => { console.error('Error toggling task today flag:', error); res.status(500).json({ error: 'Failed to update task today flag' }); } -}); +} +); // GET /api/task/:id/next-iterations router.get('/task/:id/next-iterations', async (req, res) => { diff --git a/backend/tests/integration/notes.test.js b/backend/tests/integration/notes.test.js index a3896f0..29734c4 100644 --- a/backend/tests/integration/notes.test.js +++ b/backend/tests/integration/notes.test.js @@ -169,8 +169,8 @@ describe('Notes Routes', () => { const response = await agent.get(`/api/note/${otherNote.id}`); - expect(response.status).toBe(404); - expect(response.body.error).toBe('Note not found.'); + expect(response.status).toBe(403); + expect(response.body.error).toBe('Forbidden'); }); it('should require authentication', async () => { @@ -234,8 +234,8 @@ describe('Notes Routes', () => { .patch(`/api/note/${otherNote.id}`) .send({ title: 'Updated' }); - expect(response.status).toBe(404); - expect(response.body.error).toBe('Note not found.'); + expect(response.status).toBe(403); + expect(response.body.error).toBe('Forbidden'); }); it('should require authentication', async () => { @@ -290,8 +290,8 @@ describe('Notes Routes', () => { const response = await agent.delete(`/api/note/${otherNote.id}`); - expect(response.status).toBe(404); - expect(response.body.error).toBe('Note not found.'); + expect(response.status).toBe(403); + expect(response.body.error).toBe('Forbidden'); }); it('should require authentication', async () => { diff --git a/backend/tests/integration/subtasks.test.js b/backend/tests/integration/subtasks.test.js index bfd0b4b..24d6305 100644 --- a/backend/tests/integration/subtasks.test.js +++ b/backend/tests/integration/subtasks.test.js @@ -616,7 +616,7 @@ describe('Subtasks API', () => { }); describe('Authentication and Authorization', () => { - it('should not allow access to subtasks of other users', async () => { + it('should return 403 when accessing subtasks of other users', async () => { const otherUser = await createTestUser({ email: `other_${Date.now()}@example.com`, }); @@ -627,12 +627,10 @@ describe('Subtasks API', () => { priority: Task.PRIORITY.MEDIUM, }); - // API returns empty array for other users' tasks instead of 401 const response = await agent .get(`/api/task/${otherTask.id}/subtasks`) - .expect(200); - - expect(response.body).toHaveLength(0); + .expect(403); + expect(response.body.error).toBe('Forbidden'); }); it('should not allow creating subtasks for other users tasks', async () => { diff --git a/backend/tests/integration/tasks.test.js b/backend/tests/integration/tasks.test.js index 22ed677..5a5f739 100644 --- a/backend/tests/integration/tasks.test.js +++ b/backend/tests/integration/tasks.test.js @@ -201,8 +201,8 @@ describe('Tasks Routes', () => { .patch(`/api/task/${otherTask.id}`) .send({ name: 'Updated' }); - expect(response.status).toBe(404); - expect(response.body.error).toBe('Task not found.'); + expect(response.status).toBe(403); + expect(response.body.error).toBe('Forbidden'); }); it('should require authentication', async () => { @@ -257,8 +257,8 @@ describe('Tasks Routes', () => { const response = await agent.delete(`/api/task/${otherTask.id}`); - expect(response.status).toBe(404); - expect(response.body.error).toBe('Task not found.'); + expect(response.status).toBe(403); + expect(response.body.error).toBe('Forbidden'); }, 10000); // 10 second timeout for this specific test it('should require authentication', async () => {