-
Notifications
You must be signed in to change notification settings - Fork 62
🔧 Feature: Implement Error Handling for Backend APIs #159
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 all commits
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 | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,37 +5,60 @@ const router = express.Router(); | |||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // Signup route | ||||||||||||||||||||||||||||||||
| router.post("/signup", async (req, res) => { | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| const { username, email, password } = req.body; | ||||||||||||||||||||||||||||||||
| const { username, email, password } = req.body; | ||||||||||||||||||||||||||||||||
|
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. 🛠️ Refactor suggestion Consider adding input validation before processing. The route directly destructures Add input validation before processing: router.post("/signup", async (req, res) => {
const { username, email, password } = req.body;
+
+ // Validate required fields
+ if (!username || !email || !password) {
+ return res.status(400).json({ message: 'All fields are required' });
+ }
try {📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||
| const existingUser = await User.findOne( {email} ); | ||||||||||||||||||||||||||||||||
| const existingUser = await User.findOne({ email }); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| if (existingUser) | ||||||||||||||||||||||||||||||||
| return res.status(400).json( {message: 'User already exists'} ); | ||||||||||||||||||||||||||||||||
| if (existingUser) { | ||||||||||||||||||||||||||||||||
| return res.status(400).json({ message: 'User already exists' }); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| const newUser = new User( {username, email, password} ); | ||||||||||||||||||||||||||||||||
| const newUser = new User({ username, email, password }); | ||||||||||||||||||||||||||||||||
| await newUser.save(); | ||||||||||||||||||||||||||||||||
| res.status(201).json( {message: 'User created successfully'} ); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| return res.status(201).json({ message: 'User created successfully' }); | ||||||||||||||||||||||||||||||||
| } catch (err) { | ||||||||||||||||||||||||||||||||
| res.status(500).json({ message: 'Error creating user', error: err.message }); | ||||||||||||||||||||||||||||||||
| console.error("Signup Error:", err.message); | ||||||||||||||||||||||||||||||||
| return res.status(500).json({ | ||||||||||||||||||||||||||||||||
| message: 'Error creating user', | ||||||||||||||||||||||||||||||||
| error: err.message | ||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
Comment on lines
+22
to
27
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. Security concern: Exposing internal error messages. The error response includes Consider sanitizing error messages for client responses: } catch (err) {
console.error("Signup Error:", err.message);
return res.status(500).json({
- message: 'Error creating user',
- error: err.message
+ message: 'Error creating user'
});
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // Login route | ||||||||||||||||||||||||||||||||
| router.post("/login", passport.authenticate('local'), (req, res) => { | ||||||||||||||||||||||||||||||||
| res.status(200).json( { message: 'Login successful', user: req.user } ); | ||||||||||||||||||||||||||||||||
| router.post("/login", (req, res, next) => { | ||||||||||||||||||||||||||||||||
| passport.authenticate('local', (err, user, info) => { | ||||||||||||||||||||||||||||||||
| if (err) { | ||||||||||||||||||||||||||||||||
| console.error("Login Error:", err.message); | ||||||||||||||||||||||||||||||||
| return res.status(500).json({ message: 'Internal Server Error', error: err.message }); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| if (!user) { | ||||||||||||||||||||||||||||||||
| return res.status(401).json({ message: 'Invalid credentials', error: info?.message || 'Authentication failed' }); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| req.logIn(user, (err) => { | ||||||||||||||||||||||||||||||||
| if (err) { | ||||||||||||||||||||||||||||||||
| console.error("Session Error:", err.message); | ||||||||||||||||||||||||||||||||
| return res.status(500).json({ message: 'Login failed', error: err.message }); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| return res.status(200).json({ message: 'Login successful', user }); | ||||||||||||||||||||||||||||||||
|
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. Security concern: Exposing user object in response. Returning the entire user object might expose sensitive information like hashed passwords or internal fields. Consider returning only safe user fields: - return res.status(200).json({ message: 'Login successful', user });
+ return res.status(200).json({
+ message: 'Login successful',
+ user: { id: user._id, username: user.username, email: user.email }
+ });📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||
| })(req, res, next); | ||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // Logout route | ||||||||||||||||||||||||||||||||
| router.get("/logout", (req, res) => { | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| req.logout((err) => { | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| if (err) | ||||||||||||||||||||||||||||||||
| if (err) { | ||||||||||||||||||||||||||||||||
| console.error("Logout Error:", err.message); | ||||||||||||||||||||||||||||||||
| return res.status(500).json({ message: 'Logout failed', error: err.message }); | ||||||||||||||||||||||||||||||||
| else | ||||||||||||||||||||||||||||||||
| res.status(200).json({ message: 'Logged out successfully' }); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| return res.status(200).json({ message: 'Logged out successfully' }); | ||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,29 +11,51 @@ require('./config/passportConfig'); | |||||||||||||||||
|
|
||||||||||||||||||
| const app = express(); | ||||||||||||||||||
|
|
||||||||||||||||||
| // CORS configuration | ||||||||||||||||||
| app.use(cors('*')); | ||||||||||||||||||
| // ✅ CORS configuration (recommended) | ||||||||||||||||||
| app.use(cors({ | ||||||||||||||||||
| origin: '*', // You can replace * with specific domains in production | ||||||||||||||||||
| credentials: true, | ||||||||||||||||||
| })); | ||||||||||||||||||
|
Comment on lines
+15
to
+18
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. Security risk: Overly permissive CORS configuration. Setting Configure CORS more securely: app.use(cors({
- origin: '*', // You can replace * with specific domains in production
+ origin: process.env.ALLOWED_ORIGINS?.split(',') || ['http://localhost:3000'],
credentials: true,
}));Add 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
|
|
||||||||||||||||||
| // Middleware | ||||||||||||||||||
| // ✅ Middleware | ||||||||||||||||||
| app.use(bodyParser.json()); | ||||||||||||||||||
|
|
||||||||||||||||||
| app.use(session({ | ||||||||||||||||||
| secret: process.env.SESSION_SECRET, | ||||||||||||||||||
| resave: false, | ||||||||||||||||||
| saveUninitialized: false, | ||||||||||||||||||
| })); | ||||||||||||||||||
|
|
||||||||||||||||||
| app.use(passport.initialize()); | ||||||||||||||||||
| app.use(passport.session()); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Routes | ||||||||||||||||||
| // ✅ Routes | ||||||||||||||||||
| const authRoutes = require('./routes/auth'); | ||||||||||||||||||
| app.use('/api/auth', authRoutes); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Connect to MongoDB | ||||||||||||||||||
| mongoose.connect(process.env.MONGO_URI, {}).then(() => { | ||||||||||||||||||
| console.log('Connected to MongoDB'); | ||||||||||||||||||
| app.listen(process.env.PORT, () => { | ||||||||||||||||||
| console.log(`Server running on port ${process.env.PORT}`); | ||||||||||||||||||
| // ✅ Fallback route for 404 Not Found | ||||||||||||||||||
| app.use((req, res, next) => { | ||||||||||||||||||
| res.status(404).json({ message: 'Route not found' }); | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| // ✅ Global error-handling middleware | ||||||||||||||||||
| app.use((err, req, res, next) => { | ||||||||||||||||||
| console.error('Unhandled Error:', err.stack); | ||||||||||||||||||
| res.status(err.status || 500).json({ | ||||||||||||||||||
| message: err.message || 'Internal Server Error', | ||||||||||||||||||
| error: process.env.NODE_ENV === 'production' ? undefined : err.stack, | ||||||||||||||||||
| }); | ||||||||||||||||||
| }).catch((err) => { | ||||||||||||||||||
| console.log('MongoDB connection error:', err); | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| // ✅ Connect to MongoDB and start server | ||||||||||||||||||
| mongoose.connect(process.env.MONGO_URI, {}) | ||||||||||||||||||
| .then(() => { | ||||||||||||||||||
| console.log('Connected to MongoDB'); | ||||||||||||||||||
| app.listen(process.env.PORT, () => { | ||||||||||||||||||
| console.log(`Server running on port ${process.env.PORT}`); | ||||||||||||||||||
| }); | ||||||||||||||||||
| }) | ||||||||||||||||||
| .catch((err) => { | ||||||||||||||||||
| console.error('MongoDB connection error:', err); | ||||||||||||||||||
| process.exit(1); // exit the process on DB failure | ||||||||||||||||||
| }); | ||||||||||||||||||
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.
🛠️ Refactor suggestion
Consider strengthening password requirements.
The current minimum length of 6 characters might be insufficient for security. Consider adding complexity requirements.
Enhance password validation:
password: { type: String, required: [true, "Password is required"], - minlength: [6, "Password must be at least 6 characters long"], + minlength: [8, "Password must be at least 8 characters long"], + validate: { + validator: function(password) { + return /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[@$!%*?&])[A-Za-z\d@$!%*?&]/.test(password); + }, + message: "Password must contain at least one uppercase letter, one lowercase letter, one number, and one special character" + } },📝 Committable suggestion
🤖 Prompt for AI Agents