mazipan / tanyaaja.in

❓Kumpulkan pertanyaan secara anonim dari siapa saja dengan mudah
https://tanyaaja.in
MIT License
134 stars 36 forks source link

FEAT: Revoke Refresh Tokens on Session-Destroy #129

Open haruelrovix opened 11 months ago

haruelrovix commented 11 months ago

Closes (https://github.com/mazipan/tanyaaja/issues/94)

Description

Tested on Logout βœ…

HTTP Requests
-------------

GET    /_next/static/media/TanyaAja.15bf8495.svg                      200 OK
GET    /_next/static/chunks/app/login/page.js                         200 OK
GET    /_next/static/webpack/webpack.22acd9070cb303c3.hot-update.js   200 OK
GET    /_next/static/webpack/22acd9070cb303c3.webpack.hot-update.json 200 OK
GET    /login                                                         200 OK
GET    /login                                                         200 OK
DELETE /api/private/user/session-destroy                              200 OK
GET    /api/private/user/by-uuid/***                                  200 OK

πŸ“ I haven't checked the Delete User flow, but I guess we need to Revoke the Refresh token there as well.

References:

vercel[bot] commented 11 months ago

Someone is attempting to deploy a commit to a Personal Account owned by @mazipan on Vercel.

@mazipan first needs to authorize it.

mazipan commented 11 months ago

I don't think the destroy session is necessary in the current flow.

Can you check, how if use previous token after doing logout?

Since we already doing logout from Firebase client side, I assume that the old token can not be verified anymore.

If it's safe, then we can remove the destroy session function completely.

mazipan commented 11 months ago

I will help to check from my local.

vercel[bot] commented 11 months ago

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated (UTC)
tanyaaja βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Oct 24, 2023 5:01am
haruelrovix commented 11 months ago

@mazipan I checked it again and I guess both the master and this branch still has the Security issue.

Scenario:

  1. User login successfully
  2. User get JWT token
  3. Now copy the JWT token
  4. Use it to access private endpoints
  5. All should be good
  6. Now LOGOUT the user
  7. Use the JWT token from step 3 to access private endpoints
  8. Ideally, the request should Fail

Turns out it's bigger topic than I expected. To make it ideal, need to introduce two things:

The approach is like documented here: Revoke refresh tokens

Now when we retest common flow above, it will be in ideal state.

{{BASE_URL}}/api/private/question/by-uid/pagination/*** FirebaseAuthError: The Firebase ID token has been revoked.
    at /tanyaaja/node_modules/.pnpm/firebase-admin@11.11.0_encoding@0.1.13/node_modules/firebase-admin/lib/auth/base-auth.js:973:27
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async GET (webpack-internal:///(rsc)/./src/app/api/private/question/by-uid/pagination/[uid]/route.ts:22:34)
    at async /tanyaaja/node_modules/.pnpm/next@13.5.4_react-dom@18.2.0_react@18.2.0/node_modules/next/dist/compiled/next-server/app-route.runtime.dev.js:6:62361 {
  errorInfo: {
    code: 'auth/id-token-revoked',
    message: 'The Firebase ID token has been revoked.'
  },
  codePrefix: 'auth'
}
haruelrovix commented 11 months ago

Fixed the issue πŸ’ͺ🏻

HTTP Requests
-------------

GET    /api/private/question/by-uid/pagination/***   500 Internal Server Error
DELETE /api/private/user/session-destroy.            200 OK
GET    /api/private/user/by-uuid/***                 200 OK
GET    /api/private/question/by-uid/pagination/***   200 OK

πŸ“ Ideally, the response should be 401 Unauthorized instead of 500 Internal Server Error.

  } catch (error: any) {
    console.error(request.url, error)
    if (error.code === 'auth/id-token-revoked') {
      // Token has been revoked. Inform the user to reauthenticate or signOut() the user.
      return NextResponse.json(
        { message: 'Session has expired. Please log in again to continue.' },
        { status: 401 },
      )
    }

    return NextResponse.json(
      { message: 'Error while get question by uid' },
      { status: 500 },
    )
  }

But this will be a huge refactor, let's: