Skip to content

Improve error handling: fix swallowed errors and add proper propagation #1

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 3 commits into
base: main
Choose a base branch
from
Open

Improve error handling: fix swallowed errors and add proper propagation #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2da1ada
Improve error handling: fix swallowed errors and add proper propagation
0xpritam Jun 3, 2026
0804633
Update test expectation to match improved error message
0xpritam Jun 4, 2026
444f615
Add JWT_SECRET guard test and restore env vars after login tests
0xpritam Jun 4, 2026
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
13 changes: 12 additions & 1 deletion backend/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// 1. Load the environment variables FIRST
require('dotenv').config();
console.log("DEBUG -> JWT_SECRET VALUE IS:", process.env.JWT_SECRET);

// 2. NOW load your app, database, and models
const app = require('./src/app');
const sequelize = require('./src/config/database');
Expand All @@ -21,7 +21,18 @@ async function startServer() {
});
} catch (error) {
console.error('Failed to start server:', error);
process.exit(1);
}
}

process.on('unhandledRejection', (reason, promise) => {
console.error('Unhandled Rejection at:', promise, 'reason:', reason);
process.exit(1);
});

process.on('uncaughtException', (error) => {
console.error('Uncaught Exception:', error);
process.exit(1);
});

startServer();
29 changes: 28 additions & 1 deletion backend/src/__tests__/auth.controller.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,20 +91,31 @@ describe('register', () => {
await register(req, res);
expect(res.status).toHaveBeenCalledWith(500);
expect(res.json).toHaveBeenCalledWith(
expect.objectContaining({ success: false, message: 'Server error' })
expect.objectContaining({ success: false, message: 'An unexpected error occurred during registration' })
);
});
});

// ── LOGIN ───────────────────────────────────────────────────────────────────

describe('login', () => {
const originalJwtSecret = process.env.JWT_SECRET;
const originalJwtExpiresIn = process.env.JWT_EXPIRES_IN;

beforeEach(() => {
jest.clearAllMocks();
process.env.JWT_SECRET = 'test-secret';
process.env.JWT_EXPIRES_IN = '1h';
});

afterAll(() => {
if (originalJwtSecret === undefined) delete process.env.JWT_SECRET;
else process.env.JWT_SECRET = originalJwtSecret;

if (originalJwtExpiresIn === undefined) delete process.env.JWT_EXPIRES_IN;
else process.env.JWT_EXPIRES_IN = originalJwtExpiresIn;
});

it('returns 400 when email is missing', async () => {
const { req, res } = mockReqRes({ password: '123456' });
await login(req, res);
Expand Down Expand Up @@ -180,6 +191,22 @@ describe('login', () => {
);
});

it('returns 500 when JWT_SECRET is not configured', async () => {
delete process.env.JWT_SECRET;
const fakeUser = { id: '1', name: 'Test', email: 'a@b.com', password: 'hashed' };
User.findOne.mockResolvedValue(fakeUser);
bcrypt.compare.mockResolvedValue(true);

const { req, res } = mockReqRes({ email: 'a@b.com', password: '123456' });
await login(req, res);

expect(res.status).toHaveBeenCalledWith(500);
expect(res.json).toHaveBeenCalledWith(
expect.objectContaining({ success: false, message: 'Server configuration error' })
);
expect(jwt.sign).not.toHaveBeenCalled();
});

it('returns 500 on unexpected error', async () => {
User.findOne.mockRejectedValue(new Error('DB down'));
const { req, res } = mockReqRes({ email: 'a@b.com', password: '123456' });
Expand Down
24 changes: 24 additions & 0 deletions backend/src/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,28 @@ app.get('/api/health', (req, res) => {
});
});

// 404 handler for unmatched routes
app.use((req, res) => {
res.status(404).json({
success: false,
message: `Route ${req.method} ${req.originalUrl} not found`,
});
});

// Global error-handling middleware
app.use((err, req, res, _next) => {
if (err.type === 'entity.parse.failed') {
return res.status(400).json({
success: false,
message: 'Malformed JSON in request body',
});
}

console.error('Unhandled error:', err);
res.status(err.status || 500).json({
success: false,
message: err.message || 'Internal server error',
});
Comment on lines +41 to +45

@coderabbitai coderabbitai Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid leaking internal error details to clients on 500s.

For unexpected errors (no intentional err.status), err.message can expose internals such as driver/SQL/stack text to clients. Surface err.message only for explicit client errors (4xx) and return a generic message otherwise.

🛡️ Proposed fix 
     console.error('Unhandled error:', err);
-    res.status(err.status || 500).json({
-        success: false,
-        message: err.message || 'Internal server error',
-    });
+    const status = err.status || 500;
+    res.status(status).json({
+        success: false,
+        message: status < 500 ? (err.message || 'Request error') : 'Internal server error',
+    });
📝 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
console.error('Unhandled error:', err);
res.status(err.status || 500).json({
success: false,
message: err.message || 'Internal server error',
});
console.error('Unhandled error:', err);
const status = err.status || 500;
res.status(status).json({
success: false,
message: status < 500 ? (err.message || 'Request error') : 'Internal server error',
});
🤖 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/src/app.js` around lines 41 - 45, The current error handler logs the
full error and returns err.message to clients even on 500s; change the logic in
the error-handling block that uses console.error, res.status(err.status ||
500).json(...) so that only explicit client errors (status >=400 && <500)
surface err.message to the JSON response, while unexpected server errors (no
err.status or status >=500) return a generic message like "Internal server
error"; continue to console.error the full err (and stack) for server-side
diagnostics but do not include those details in the response JSON.

});

module.exports = app;
8 changes: 8 additions & 0 deletions backend/src/config/database.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
const { Sequelize } = require('sequelize');
require('dotenv').config();

const requiredVars = ['DB_NAME', 'DB_USER', 'DB_PASSWORD', 'DB_HOST', 'DB_PORT'];
const missing = requiredVars.filter((key) => !process.env[key]);
if (missing.length > 0) {
throw new Error(
`Missing required database environment variables: ${missing.join(', ')}`
);
}

const sequelize = new Sequelize(
process.env.DB_NAME,
process.env.DB_USER,
Expand Down
26 changes: 25 additions & 1 deletion backend/src/controllers/auth.controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,26 @@ if (!name || !email || !password) {
},
});
} catch (error) {
if (error.name === 'SequelizeValidationError') {
const messages = error.errors.map((e) => e.message);
return res.status(400).json({
success: false,
message: 'Validation failed',
errors: messages,
});
}

if (error.name === 'SequelizeUniqueConstraintError') {
return res.status(409).json({
success: false,
message: 'Email already registered',
});
}

console.error('Register error:', error);
return res.status(500).json({
success: false,
message: 'Server error',
message: 'An unexpected error occurred during registration',
});
}
};
Expand Down Expand Up @@ -91,6 +107,14 @@ const login = async (req, res) => {
}

// 4. Generate JWT token
if (!process.env.JWT_SECRET) {
console.error('JWT_SECRET is not configured');
return res.status(500).json({
success: false,
message: 'Server configuration error',
});
}

const token = jwt.sign(
{ id: user.id, email: user.email },
process.env.JWT_SECRET,
Expand Down