Skip to content

Conversation

@Dinakar77
Copy link
Contributor

@Dinakar77 Dinakar77 commented Dec 16, 2025

Implemented a secure refresh token flow using hashed refresh tokens with a searchable lookup hash to prevent token leakage and improve performance.

Fixed the refresh token validation logic, ensuring correct session lookup, expiry checks, user status verification, and proper token rotation.

Updated logout and logout-all functionality to invalidate sessions safely without relying on raw refresh tokens.

Cleaned up authentication middleware to remove duplicate logic and ensure consistent error handling.

Removed local debug and test scripts (test-authflow.js, testcapstone.js) to avoid leaking credentials and keep the repository production-ready.

Removed sensitive logging related to Supabase configuration and secrets.

Copy link
Contributor

@elanlaw1206 elanlaw1206 left a comment

Choose a reason for hiding this comment

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

Hi Dinakar,

Thanks for the detailed description and the security-focused improvements. Hashing refresh tokens and improving session invalidation are the right direction and align well with OWASP guidance.

Requesting changes before merge due to a few high-impact issues that need tightening, given that this touches core auth/session logic:

  1. services/supabaseClient.js: environment values (Supabase URL / keys) are currently logged. These must be removed to avoid leaking secrets into logs or CI output.

  2. authService refresh/logout flow: the hashed refresh-token approach is good, but the current implementation fetches all active sessions and performs bcrypt comparisons in a loop. This can become expensive at scale and may introduce DoS risk. Please narrow the lookup (e.g. via token identifier / hash prefix / user scope) and ensure logout always updates by session id, not raw refreshToken.

  3. middleware/authenticateToken.js: the diff shows duplicate declarations / possible merge artifacts and mixed error paths. Please clean this up and keep consistent 401 responses for missing, expired, and invalid tokens.

  4. test-authflow.js / testcapstone.js: these appear to be local/manual validation scripts and currently log tokens and credentials. Suggest moving them under a dev-only scripts folder and avoiding logging raw tokens.

Once these are addressed, I’m happy to re-review. The overall security direction is solid, but we need this implementation to be safe, clean, and production-ready before merging into master.

Thanks!
King Hei

@Dinakar77
Copy link
Contributor Author

Hey King,
I made the changes as u asked...and please check. And if there are any changes required, please let me know

@elanlaw1206
Copy link
Contributor

Hi Dinakar,

Thanks for the update, it is good progress overall.

I can’t approve this PR yet because Git is reporting merge conflicts in
middleware/authenticateToken.js (currently 4 conflict blocks).

These conflicts are caused by overlapping changes between this branch and master, where both sides modified the same authentication logic (token parsing, validation, and error handling). Git isn’t able to auto-resolve which version to keep.

Suggested fix:

Pull / rebase the latest master into this branch

Manually resolve the conflicts by keeping one clean, consolidated implementation of:

Authorization header parsing

Missing token handling

Access-token type validation

Remove all conflict markers (<<<<<<<, =======, >>>>>>>)

Verify login and one protected endpoint still work

Once the conflicts are resolved and pushed, I’m happy to re-review and approve 👍
Thanks
King Hei

@Dinakar77 Dinakar77 force-pushed the dinakar_feature_nutrihelp branch from 9b2d89c to 3c5c31a Compare January 22, 2026 12:27
@Dinakar77
Copy link
Contributor Author

Hey King Hei,
image
Screenshot of the Auth flow working

Resolved Row Level Security (RLS) issues by separating anon access from service-role access for session inserts and updates.

Improved session lifecycle management by correctly invalidating old sessions on login, refresh, logout, and logout-all.

Cleaned up the access-token middleware by removing duplicate logic and standardising error handling for invalid or expired tokens.

Removed debug scripts and sensitive logs (raw tokens, secrets) to align with security and team workflow guidelines.

@elanlaw1206
Copy link
Contributor

Hi Dinakar,

Thanks for the detailed update and the context 👍
I can see a lot of effort has gone into improving the auth middleware, token handling, and session lifecycle. The direction makes sense from a security perspective.

Since this PR introduces significant changes to core authentication logic and Supabase access patterns, I’m taking a safety-first approach to make sure there’s no risk of breaking the overall system, especially given this is a multi-trimester project.

Before I approve, could you please help confirm two more things:

  1. Auth flow evidence
    A short log or screenshot demonstrating the auth flow working end-to-end (e.g. login → refresh → logout, or a protected route returning 401/200 as expected).

  2. Environment & key confirmation
    Please confirm that SUPABASE_SERVICE_ROLE_KEY is configured in the deployment environment, or document this requirement clearly in the repo / README/notes if it’s not already set.

Once that confirmation is in place and all checks remain green, I’m happy to approve and move this forward.

Thanks again for the solid work on this.

King Hei

@Dinakar77
Copy link
Contributor Author

Hey King,
As u asked this is the screenshot of the successful auth flow
image

This is the screenshot of the service role key that I use
image

Copy link
Contributor

@elanlaw1206 elanlaw1206 left a comment

Choose a reason for hiding this comment

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

Thanks for the confirmation and auth flow evidence.
All checks look good, approving this PR.

@elanlaw1206 elanlaw1206 merged commit b0c5ce1 into Gopher-Industries:master Jan 28, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants