Skip to content

fix: make clients shared across all users while keeping work entries per-user #482

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.

Sign up for GitHub

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

Open
devin-ai-integration wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: make clients shared across all users while keeping work entries per-user #482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 0 additions & 1 deletion backend/src/__tests__/database/init.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,6 @@ describe('Database Initialization', () => {
const runCalls = db.run.mock.calls;
const queries = runCalls.map(call => call[0]);

expect(queries.some(q => q.includes('CREATE INDEX IF NOT EXISTS idx_clients_user_email'))).toBe(true);
expect(queries.some(q => q.includes('CREATE INDEX IF NOT EXISTS idx_work_entries_client_id'))).toBe(true);
expect(queries.some(q => q.includes('CREATE INDEX IF NOT EXISTS idx_work_entries_user_email'))).toBe(true);
expect(queries.some(q => q.includes('CREATE INDEX IF NOT EXISTS idx_work_entries_date'))).toBe(true);
Expand Down
2 changes: 1 addition & 1 deletion backend/src/__tests__/routes/clients.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ describe('Client Routes', () => {
expect(response.body).toEqual({ clients: mockClients });
expect(mockDb.all).toHaveBeenCalledWith(
expect.stringContaining('SELECT id, name, description'),
['test@example.com'],
[],
expect.any(Function)
);
});
Expand Down
14 changes: 10 additions & 4 deletions backend/src/__tests__/routes/reports.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,9 +243,8 @@ describe('Report Routes', () => {
});

describe('Data Isolation', () => {
test('should only return data for authenticated user', async () => {
test('should return client data without user filtering but filter work entries by user', async () => {
mockDb.get.mockImplementation((query, params, callback) => {
expect(params).toContain('test@example.com');
callback(null, { id: 1, name: 'Test Client' });
});

Expand All @@ -256,8 +255,15 @@ describe('Report Routes', () => {

await request(app).get('/api/reports/client/1');

// Client lookup should not include user_email
expect(mockDb.get).toHaveBeenCalledWith(
expect.any(String),
[1],
expect.any(Function)
);
// Work entries should still be filtered by user_email
expect(mockDb.all).toHaveBeenCalledWith(
expect.stringContaining('user_email'),
expect.arrayContaining(['test@example.com']),
expect.any(Function)
);
Expand Down Expand Up @@ -347,7 +353,7 @@ describe('Report Routes', () => {

expect(mockDb.get).toHaveBeenCalledWith(
expect.stringContaining('SELECT id, name FROM clients'),
expect.arrayContaining([1, 'test@example.com']),
[1],
expect.any(Function)
);
});
Expand Down Expand Up @@ -433,7 +439,7 @@ describe('Report Routes', () => {

expect(mockDb.get).toHaveBeenCalledWith(
expect.stringContaining('SELECT id, name FROM clients'),
expect.arrayContaining([1, 'test@example.com']),
[1],
expect.any(Function)
);
});
Expand Down
4 changes: 2 additions & 2 deletions backend/src/__tests__/routes/workEntries.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ describe('Work Entry Routes', () => {
});

expect(response.status).toBe(400);
expect(response.body).toEqual({ error: 'Client not found or does not belong to user' });
expect(response.body).toEqual({ error: 'Client not found' });
});

test('should return 400 for missing required fields', async () => {
Expand Down Expand Up @@ -305,7 +305,7 @@ describe('Work Entry Routes', () => {
.send({ clientId: 999 });

expect(response.status).toBe(400);
expect(response.body).toEqual({ error: 'Client not found or does not belong to user' });
expect(response.body).toEqual({ error: 'Client not found' });
});
});

Expand Down
1 change: 0 additions & 1 deletion backend/src/database/init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ async function initializeDatabase() {
`);

// Create indexes for better performance
database.run(`CREATE INDEX IF NOT EXISTS idx_clients_user_email ON clients (user_email)`);
database.run(`CREATE INDEX IF NOT EXISTS idx_work_entries_client_id ON work_entries (client_id)`);
database.run(`CREATE INDEX IF NOT EXISTS idx_work_entries_user_email ON work_entries (user_email)`);
database.run(`CREATE INDEX IF NOT EXISTS idx_work_entries_date ON work_entries (date)`);
Expand Down
51 changes: 15 additions & 36 deletions backend/src/routes/clients.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ const router = express.Router();
// All routes require authentication
router.use(authenticateUser);

// Get all clients for authenticated user
// Get all clients (shared across all users)
router.get('/', (req, res) => {
const db = getDatabase();

db.all(
'SELECT id, name, description, department, email, created_at, updated_at FROM clients WHERE user_email = ? ORDER BY name',
[req.userEmail],
'SELECT id, name, description, department, email, created_at, updated_at FROM clients ORDER BY name',
[],
(err, rows) => {
if (err) {
console.error('Database error:', err);
Expand All @@ -37,8 +37,8 @@ router.get('/:id', (req, res) => {
const db = getDatabase();

db.get(
'SELECT id, name, description, department, email, created_at, updated_at FROM clients WHERE id = ? AND user_email = ?',
[clientId, req.userEmail],
'SELECT id, name, description, department, email, created_at, updated_at FROM clients WHERE id = ?',
[clientId],
(err, row) => {
if (err) {
console.error('Database error:', err);
Expand Down Expand Up @@ -113,10 +113,10 @@ router.put('/:id', (req, res, next) => {

const db = getDatabase();

// Check if client exists and belongs to user
// Check if client exists
db.get(
'SELECT id FROM clients WHERE id = ? AND user_email = ?',
[clientId, req.userEmail],
'SELECT id FROM clients WHERE id = ?',
[clientId],
(err, row) => {
if (err) {
console.error('Database error:', err);
Expand Down Expand Up @@ -152,9 +152,9 @@ router.put('/:id', (req, res, next) => {
}

updates.push('updated_at = CURRENT_TIMESTAMP');
values.push(clientId, req.userEmail);
values.push(clientId);

const query = `UPDATE clients SET ${updates.join(', ')} WHERE id = ? AND user_email = ?`;
const query = `UPDATE clients SET ${updates.join(', ')} WHERE id = ?`;

db.run(query, values, function(err) {
if (err) {
Expand Down Expand Up @@ -186,27 +186,6 @@ router.put('/:id', (req, res, next) => {
}
});

// Delete all clients for authenticated user
Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration bot Apr 15, 2026

Choose a reason for hiding this comment

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

🔴 Frontend "Clear All" button calls removed backend endpoint, causing 404

The PR removed the DELETE /api/clients (delete-all) route handler from backend/src/routes/clients.js:189-208 (old lines), but the frontend still references it. frontend/src/api/client.ts:84-87 defines deleteAllClients() which calls DELETE /api/clients, and frontend/src/pages/ClientsPage.tsx:83-92 wires it into a deleteAllMutation rendered as a "Clear All" button at ClientsPage.tsx:174-184. When a user clicks this button, the request will get a 404 since no route handler matches DELETE /api/clients anymore (the remaining DELETE /:id route won't match an empty id segment).

Prompt for agents 
The DELETE / route for clients was removed from backend/src/routes/clients.js, but the frontend still uses it. Either:

1. Remove the frontend references: delete the deleteAllClients() method in frontend/src/api/client.ts:84-87, remove deleteAllMutation in frontend/src/pages/ClientsPage.tsx:83-92, and remove the Clear All button in ClientsPage.tsx:174-184.

2. Or re-add a DELETE / route to the backend if clearing all clients is still a desired feature (though in a shared-clients model this would be very destructive and should probably be removed).

Option 1 is recommended since deleting all shared clients would be a dangerous operation in the new shared model.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

router.delete('/', (req, res) => {
const db = getDatabase();

db.run(
'DELETE FROM clients WHERE user_email = ?',
[req.userEmail],
function(err) {
if (err) {
console.error('Database error:', err);
return res.status(500).json({ error: 'Failed to delete clients' });
}

res.json({
message: 'All clients deleted successfully',
deletedCount: this.changes
});
}
);
});

// Delete client
router.delete('/:id', (req, res) => {
const clientId = parseInt(req.params.id);
Expand All @@ -217,10 +196,10 @@ router.delete('/:id', (req, res) => {

const db = getDatabase();

// Check if client exists and belongs to user
// Check if client exists
db.get(
'SELECT id FROM clients WHERE id = ? AND user_email = ?',
[clientId, req.userEmail],
'SELECT id FROM clients WHERE id = ?',
[clientId],
Comment on lines 200 to +202
Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration bot Apr 15, 2026

Choose a reason for hiding this comment

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

🔴 Removing user_email check on client delete allows any user to cascade-delete other users' work entries

The authorization check (AND user_email = ?) was removed from the client delete endpoint at backend/src/routes/clients.js:201-202. Since the work_entries table has FOREIGN KEY (client_id) REFERENCES clients (id) ON DELETE CASCADE (backend/src/database/init.js:64), any authenticated user can now delete any client, which cascades to destroy ALL work entries from ALL users referencing that client. Previously, only the client's owner could delete it, so the cascade was safe. Now User A can delete a client created by User B, silently destroying User B's (and all other users') work entries for that client. This contradicts the per-user isolation still enforced on work entry routes (workEntries.js:62, workEntries.js:161, workEntries.js:272).

Prompt for agents 
The client delete endpoint no longer checks user_email, but due to ON DELETE CASCADE on the work_entries foreign key, deleting a shared client destroys all users' work entries for that client. This is a cross-user data loss vulnerability.

Possible fixes:
1. Before deleting a client, check that no other users have work entries referencing it (or only allow deletion if the only work entries belong to the requesting user).
2. Remove CASCADE from the foreign key and instead handle work entry cleanup explicitly — e.g., only delete the requesting user's work entries and reassign or orphan others.
3. Restrict client deletion to the original creator (keep user_email check on DELETE but remove it from read operations).
4. Add a soft-delete mechanism instead of hard-deleting clients.

The fix should be applied in backend/src/routes/clients.js in the DELETE /:id handler, and potentially also in the database schema at backend/src/database/init.js.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

(err, row) => {
if (err) {
console.error('Database error:', err);
Expand All @@ -233,8 +212,8 @@ router.delete('/:id', (req, res) => {

// Delete client (work entries will be deleted due to CASCADE)
db.run(
'DELETE FROM clients WHERE id = ? AND user_email = ?',
[clientId, req.userEmail],
'DELETE FROM clients WHERE id = ?',
[clientId],
function(err) {
if (err) {
console.error('Database error:', err);
Expand Down
18 changes: 9 additions & 9 deletions backend/src/routes/reports.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ router.get('/client/:clientId', (req, res) => {

const db = getDatabase();

// Verify client belongs to user
// Verify client exists
db.get(
'SELECT id, name FROM clients WHERE id = ? AND user_email = ?',
[clientId, req.userEmail],
'SELECT id, name FROM clients WHERE id = ?',
[clientId],
(err, client) => {
if (err) {
console.error('Database error:', err);
Expand Down Expand Up @@ -73,10 +73,10 @@ router.get('/export/csv/:clientId', (req, res) => {

const db = getDatabase();

// Verify client belongs to user and get data
// Verify client exists
db.get(
'SELECT id, name FROM clients WHERE id = ? AND user_email = ?',
[clientId, req.userEmail],
'SELECT id, name FROM clients WHERE id = ?',
[clientId],
(err, client) => {
if (err) {
console.error('Database error:', err);
Expand Down Expand Up @@ -156,10 +156,10 @@ router.get('/export/pdf/:clientId', (req, res) => {

const db = getDatabase();

// Verify client belongs to user and get data
// Verify client exists
db.get(
'SELECT id, name FROM clients WHERE id = ? AND user_email = ?',
[clientId, req.userEmail],
'SELECT id, name FROM clients WHERE id = ?',
[clientId],
(err, client) => {
if (err) {
console.error('Database error:', err);
Expand Down
16 changes: 8 additions & 8 deletions backend/src/routes/workEntries.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,18 +87,18 @@ router.post('/', (req, res, next) => {
const { clientId, hours, description, date } = value;
const db = getDatabase();

// Verify client exists and belongs to user
// Verify client exists
db.get(
'SELECT id FROM clients WHERE id = ? AND user_email = ?',
[clientId, req.userEmail],
'SELECT id FROM clients WHERE id = ?',
[clientId],
(err, row) => {
if (err) {
console.error('Database error:', err);
return res.status(500).json({ error: 'Internal server error' });
}

if (!row) {
return res.status(400).json({ error: 'Client not found or does not belong to user' });
return res.status(400).json({ error: 'Client not found' });
}

// Create work entry
Expand Down Expand Up @@ -170,19 +170,19 @@ router.put('/:id', (req, res, next) => {
return res.status(404).json({ error: 'Work entry not found' });
}

// If clientId is being updated, verify it belongs to user
// If clientId is being updated, verify it exists
if (value.clientId) {
db.get(
'SELECT id FROM clients WHERE id = ? AND user_email = ?',
[value.clientId, req.userEmail],
'SELECT id FROM clients WHERE id = ?',
[value.clientId],
(err, clientRow) => {
if (err) {
console.error('Database error:', err);
return res.status(500).json({ error: 'Internal server error' });
}

if (!clientRow) {
return res.status(400).json({ error: 'Client not found or does not belong to user' });
return res.status(400).json({ error: 'Client not found' });
}

performUpdate();
Expand Down
1 change: 0 additions & 1 deletion docker/overrides/database/init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ async function initializeDatabase() {
`);

// Create indexes for better performance
database.run(`CREATE INDEX IF NOT EXISTS idx_clients_user_email ON clients (user_email)`);
database.run(`CREATE INDEX IF NOT EXISTS idx_work_entries_client_id ON work_entries (client_id)`);
database.run(`CREATE INDEX IF NOT EXISTS idx_work_entries_user_email ON work_entries (user_email)`);
database.run(`CREATE INDEX IF NOT EXISTS idx_work_entries_date ON work_entries (date)`);
Expand Down