SECURITY + AUTH: Secure auth system#236
Open
Ad1th wants to merge 3 commits into
Open
Conversation
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📝 WalkthroughWalkthroughThis PR implements a complete authentication system for a full-stack application. The backend adds middleware-based request gating, refactored auth routes with structured responses, and CORS/session configuration. The frontend introduces a React context-based auth system with automatic session restoration, integrates login/signup pages with the context API, protects routes, and updates the navbar with authentication-aware UI controls. ChangesAuthentication System Implementation
Sequence DiagramssequenceDiagram
participant User as User
participant Login as Login Page
participant AuthContext as AuthContext
participant Backend as Backend API
participant ProtectedRoute as ProtectedRoute
User->>Login: Enter credentials & submit
Login->>AuthContext: login(email, password)
AuthContext->>Backend: POST /api/auth/login
Backend->>AuthContext: { user, isAuthenticated: true }
AuthContext->>Login: Success message
Login->>User: Navigate to /track
User->>ProtectedRoute: Access /track
ProtectedRoute->>AuthContext: Read isAuthenticated & isLoading
alt Still loading
ProtectedRoute->>User: Show loading screen
else Unauthenticated
ProtectedRoute->>User: Redirect to /login
else Authenticated
ProtectedRoute->>User: Render Tracker component
end
sequenceDiagram
participant Navbar as Navbar
participant User as User
participant AuthContext as AuthContext
participant Backend as Backend API
Navbar->>AuthContext: Read isAuthenticated & user
alt Authenticated
Navbar->>User: Show "Logged in as: username"
Navbar->>User: Show logout button
User->>Navbar: Click logout
Navbar->>AuthContext: logout()
AuthContext->>Backend: GET /api/auth/logout
Backend->>AuthContext: Success
AuthContext->>Navbar: Clear user & isAuthenticated
Navbar->>User: Navigate to home
else Unauthenticated
Navbar->>User: Show login link
end
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thank you @Ad1th for your contribution. Please make sure your PR follows https://github.com/GitMetricsLab/github_tracker/blob/main/CONTRIBUTING.md#-pull-request-guidelines
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pages/Login/Login.tsx (1)
133-135:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix success-state message comparison mismatch.
Line 34 sets
"Login successful!", but this check compares"Login successful"; success feedback will use the error style.Suggested change
- message === "Login successful" + message === "Login successful!"🤖 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 `@src/pages/Login/Login.tsx` around lines 133 - 135, The success/error styling conditional in Login.tsx compares message === "Login successful" but elsewhere the component sets the message to "Login successful!" (with an exclamation), causing successful logins to render the error style; update the comparison in the ternary that builds the CSS class (the expression using message === "Login successful") to match the actual success string (e.g., "Login successful!") or, better, switch to a boolean success flag (e.g., isSuccess) used by the login handler and evaluate that flag instead so the styling reliably reflects success vs error.README.md (1)
48-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove shell prompt prefixes in command blocks.
The command snippets include
$prefixes without output, which triggers markdownlint MD014 and hurts copy/paste usability.Proposed fix
- $ git clone https://github.com/yourusername/github-tracker.git + git clone https://github.com/yourusername/github-tracker.git ... - $ cd github-tracker + cd github-tracker ... - $ npm i - $ npm run dev + npm i + npm run dev ... - $ npm i - $ npm start + npm i + npm start🤖 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 `@README.md` around lines 48 - 70, Remove the shell prompt prefixes from the command code blocks in README.md so commands are plain and copy/paste friendly; specifically update the git clone, cd github-tracker, npm i / npm run dev (frontend) and npm i / npm start (backend) blocks to remove the leading "$" characters in each line so markdownlint MD014 is satisfied and users can copy commands directly.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@backend/routes/auth.js`:
- Around line 21-24: Replace all direct exposure of err.message in
backend/routes/auth.js (the res.status(500).json(...) calls that currently
include err.message) with a generic client-safe message like "Internal server
error" and move the detailed error into a server-side log (e.g.,
console.error(err) or your app logger) immediately before sending the response;
update each occurrence (the res.status(500).json(...) blocks at the shown
locations and the similar blocks around lines 31-34, 42-45, 60-63) so clients
never receive internal error text while full error details remain logged for
debugging.
- Around line 57-64: Change the logout route from a GET to a POST to avoid
state-changing operations over GET: replace router.get("/logout", ...) with
router.post("/logout", ...) and keep the existing req.logout callback logic (the
handler that checks err and returns 500 or 200). Also update any client-side
code or tests that call GET /logout to POST /logout and ensure any CSRF
middleware or authentication checks that apply to POST are applied to this route
(refer to the router.post("/logout") handler and the req.logout callback).
In `@backend/server.js`:
- Around line 27-31: The session middleware is too permissive: ensure
process.env.SESSION_SECRET is required (throw or exit early if missing) and
harden the session cookie in the session({...}) config by adding an explicit
cookie object with secure: true (only in production), httpOnly: true, sameSite:
'lax' or 'strict' as appropriate, and a sensible maxAge; also set
rolling/session expiration behavior if needed (e.g., rolling: true or
cookie.maxAge) and keep resave/saveUninitialized as false. Update the code
around session(...) and the SESSION_SECRET check so the app fails fast when
SESSION_SECRET is absent and the session() call includes the cookie security
options.
In `@README.md`:
- Around line 142-144: The contributors image in the README (<img
src="https://contrib.rocks/image?repo=GitMetricsLab/github_tracker" />) lacks
alt text; update the <img> tag to include a meaningful alt attribute (e.g.,
alt="Contributors to GitMetricsLab/github_tracker") so the image is accessible
and fixes MD045.
---
Outside diff comments:
In `@README.md`:
- Around line 48-70: Remove the shell prompt prefixes from the command code
blocks in README.md so commands are plain and copy/paste friendly; specifically
update the git clone, cd github-tracker, npm i / npm run dev (frontend) and npm
i / npm start (backend) blocks to remove the leading "$" characters in each line
so markdownlint MD014 is satisfied and users can copy commands directly.
In `@src/pages/Login/Login.tsx`:
- Around line 133-135: The success/error styling conditional in Login.tsx
compares message === "Login successful" but elsewhere the component sets the
message to "Login successful!" (with an exclamation), causing successful logins
to render the error style; update the comparison in the ternary that builds the
CSS class (the expression using message === "Login successful") to match the
actual success string (e.g., "Login successful!") or, better, switch to a
boolean success flag (e.g., isSuccess) used by the login handler and evaluate
that flag instead so the styling reliably reflects success vs error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a06857f5-4334-4207-b7e0-4402bc56eab3
📒 Files selected for processing (13)
README.mdbackend/middleware/authMiddleware.jsbackend/routes/auth.jsbackend/server.jssrc/Routes/Router.tsxsrc/components/Navbar.tsxsrc/components/ProtectedRoute.tsxsrc/context/AuthContext.tsxsrc/hooks/useAuth.tssrc/main.tsxsrc/pages/Login/Login.tsxsrc/pages/Signup/Signup.tsxsrc/utils/axiosConfig.ts
Comment on lines
+21
to
+24
| res | ||
| .status(500) | ||
| .json({ message: "Error creating user", error: err.message }); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid returning raw internal error messages to clients.
These responses expose err.message, which can leak backend internals. Return a generic client message and log details server-side.
Suggested change
- res
- .status(500)
- .json({ message: "Error creating user", error: err.message });
+ console.error("Signup error:", err);
+ res.status(500).json({ message: "Error creating user" });- return res
- .status(500)
- .json({ message: "Login error", error: err.message });
+ console.error("Login error:", err);
+ return res.status(500).json({ message: "Login error" });- return res
- .status(500)
- .json({ message: "Session error", error: err.message });
+ console.error("Session error:", err);
+ return res.status(500).json({ message: "Session error" });- return res
- .status(500)
- .json({ message: "Logout failed", error: err.message });
+ console.error("Logout error:", err);
+ return res.status(500).json({ message: "Logout failed" });Also applies to: 31-34, 42-45, 60-63
🤖 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/routes/auth.js` around lines 21 - 24, Replace all direct exposure of
err.message in backend/routes/auth.js (the res.status(500).json(...) calls that
currently include err.message) with a generic client-safe message like "Internal
server error" and move the detailed error into a server-side log (e.g.,
console.error(err) or your app logger) immediately before sending the response;
update each occurrence (the res.status(500).json(...) blocks at the shown
locations and the similar blocks around lines 31-34, 42-45, 60-63) so clients
never receive internal error text while full error details remain logged for
debugging.
Comment on lines
57
to
+64
| router.get("/logout", (req, res) => { | ||
|
|
||
| req.logout((err) => { | ||
|
|
||
| if (err) | ||
| return res.status(500).json({ message: 'Logout failed', error: err.message }); | ||
| else | ||
| res.status(200).json({ message: 'Logged out successfully' }); | ||
| }); | ||
| req.logout((err) => { | ||
| if (err) | ||
| return res | ||
| .status(500) | ||
| .json({ message: "Logout failed", error: err.message }); | ||
| else res.status(200).json({ message: "Logged out successfully" }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Use POST /logout instead of GET /logout.
Logout mutates server session state and should not be a GET endpoint. Keeping it as GET makes cross-site triggering easier and weakens CSRF posture.
Suggested change
-router.get("/logout", (req, res) => {
+router.post("/logout", (req, res) => {
req.logout((err) => {🤖 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/routes/auth.js` around lines 57 - 64, Change the logout route from a
GET to a POST to avoid state-changing operations over GET: replace
router.get("/logout", ...) with router.post("/logout", ...) and keep the
existing req.logout callback logic (the handler that checks err and returns 500
or 200). Also update any client-side code or tests that call GET /logout to POST
/logout and ensure any CSRF middleware or authentication checks that apply to
POST are applied to this route (refer to the router.post("/logout") handler and
the req.logout callback).
Comment on lines
+27
to
+31
| session({ | ||
| secret: process.env.SESSION_SECRET, | ||
| resave: false, | ||
| saveUninitialized: false, | ||
| })); | ||
| }), |
Contributor
There was a problem hiding this comment.
Harden session configuration for authenticated deployments.
Add explicit cookie security settings and fail fast when SESSION_SECRET is missing; current config is too permissive for a security-focused auth flow.
Suggested change
+if (!process.env.SESSION_SECRET) {
+ throw new Error("SESSION_SECRET is required");
+}
+
app.use(
session({
secret: process.env.SESSION_SECRET,
resave: false,
saveUninitialized: false,
+ cookie: {
+ httpOnly: true,
+ sameSite: "lax",
+ secure: process.env.NODE_ENV === "production",
+ },
}),
);📝 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
| session({ | |
| secret: process.env.SESSION_SECRET, | |
| resave: false, | |
| saveUninitialized: false, | |
| })); | |
| }), | |
| if (!process.env.SESSION_SECRET) { | |
| throw new Error("SESSION_SECRET is required"); | |
| } | |
| app.use( | |
| session({ | |
| secret: process.env.SESSION_SECRET, | |
| resave: false, | |
| saveUninitialized: false, | |
| cookie: { | |
| httpOnly: true, | |
| sameSite: "lax", | |
| secure: process.env.NODE_ENV === "production", | |
| }, | |
| }), | |
| ); |
🤖 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/server.js` around lines 27 - 31, The session middleware is too
permissive: ensure process.env.SESSION_SECRET is required (throw or exit early
if missing) and harden the session cookie in the session({...}) config by adding
an explicit cookie object with secure: true (only in production), httpOnly:
true, sameSite: 'lax' or 'strict' as appropriate, and a sensible maxAge; also
set rolling/session expiration behavior if needed (e.g., rolling: true or
cookie.maxAge) and keep resave/saveUninitialized as false. Update the code
around session(...) and the SESSION_SECRET check so the app fails fast when
SESSION_SECRET is absent and the session() call includes the cookie security
options.
Comment on lines
+142
to
+144
| <a href="https://github.com/GitMetricsLab/github_tracker/graphs/contributors"> | ||
| <img src="https://contrib.rocks/image?repo=GitMetricsLab/github_tracker" /> | ||
| </a> |
Contributor
There was a problem hiding this comment.
Add alt text to the contributors image.
The image is missing alt text (MD045), which is an accessibility gap in documentation.
Proposed fix
- <img src="https://contrib.rocks/image?repo=GitMetricsLab/github_tracker" />
+ <img
+ src="https://contrib.rocks/image?repo=GitMetricsLab/github_tracker"
+ alt="Contributors to GitMetricsLab/github_tracker"
+ />📝 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
| <a href="https://github.com/GitMetricsLab/github_tracker/graphs/contributors"> | |
| <img src="https://contrib.rocks/image?repo=GitMetricsLab/github_tracker" /> | |
| </a> | |
| <a href="https://github.com/GitMetricsLab/github_tracker/graphs/contributors"> | |
| <img | |
| src="https://contrib.rocks/image?repo=GitMetricsLab/github_tracker" | |
| alt="Contributors to GitMetricsLab/github_tracker" | |
| /> | |
| </a> |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 143-143: Images should have alternate text (alt text)
(MD045, no-alt-text)
🤖 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 `@README.md` around lines 142 - 144, The contributors image in the README (<img
src="https://contrib.rocks/image?repo=GitMetricsLab/github_tracker" />) lacks
alt text; update the <img> tag to include a meaningful alt attribute (e.g.,
alt="Contributors to GitMetricsLab/github_tracker") so the image is accessible
and fixes MD045.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issue
Add Authentication (Login/Signup) System
Description
This pull request introduces a full-stack authentication system, implementing user login, signup, logout, and session persistence across both the backend and frontend. It also adds route protection and improves the user experience for authenticated flows. The most important changes are grouped below:
Backend Authentication & Session Management:
isAuthenticatedmiddleware inbackend/middleware/authMiddleware.jsto protect API routes.backend/routes/auth.jsto implement robust login logic (with error handling and user data return), a/meroute for current user info, and integrated the new middleware.backend/server.jsto restrict API access to the frontend origin and updated session handling for secure authentication.Frontend Authentication Context & Utilities:
AuthContextandAuthProviderinsrc/context/AuthContext.tsxto manage user state, authentication status, and provide login/signup/logout methods across the app.useAuthhook insrc/hooks/useAuth.tsfor easy access to authentication context in components.AuthProviderinsrc/main.tsxto enable global authentication state.Route Protection & User Experience:
ProtectedRoutecomponent insrc/components/ProtectedRoute.tsxto restrict access to authenticated users and show a loading spinner during auth checks.src/Routes/Router.tsxto protect the/trackroute usingProtectedRoute.Navbar(src/components/Navbar.tsx) to show user info and a logout button when authenticated, and handle logout logic with feedback. [1] [2] [3]Login Flow Improvements:
src/pages/Login/Login.tsx) to use the new authentication context and handle navigation and error messages accordingly. [1] [2] [3]Documentation Updates:
README.md, including backend testing instructions and contributor info. [1] [2] [3] [4] [5]These changes collectively establish a secure, user-friendly authentication foundation for the app.
How Has This Been Tested?
Local, via postman
Type of Change
Summary by CodeRabbit
New Features
Documentation