Skip to content

Conversation

@adriangohjw
Copy link
Contributor

@adriangohjw adriangohjw commented Jul 21, 2025

Problem

What problem are you trying to solve? What issue does this close?

Closes https://linear.app/ogp/issue/ISOM-2003/vapt-active-denial-of-service-of-login-to-gogovsg

Solution

How did you solve the problem?

Features:

  • use email:ip as redis key instead of just email

Improvements:

  • add test case for this

Tests

What tests should be run to confirm functionality?

  • refer to Linear ticket for steps to reproduce

@adriangohjw adriangohjw self-assigned this Jul 21, 2025
@linear
Copy link

linear bot commented Jul 21, 2025

@adriangohjw adriangohjw marked this pull request as ready for review July 21, 2025 22:00
@adriangohjw
Copy link
Contributor Author

bugbot run

cursor[bot]

This comment was marked as outdated.

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Bug: OTP Verification Test Fails Due to IP Mismatch

The POST /api/login/verify integration test primes the OTP cache with a hardcoded IP '127.0.0.1'. However, the actual HTTP request made by supertest may originate from a different IP address. As OTPs are now stored and retrieved using an email:ip key, this IP mismatch prevents successful OTP verification, causing the test to fail.

test/server/api/LoginRoute.test.ts#L58-L65

// Prime cache
getOtpCache().setOtpForEmail('[email protected]', '127.0.0.1', {
hashedOtp: '1',
retries: 100,
})
const res = await request(app)
.post('/api/login/verify')
.send({ email: '[email protected]', otp: '1' })

Fix in CursorFix in Web


Was this report helpful? Give feedback by reacting with 👍 or 👎

@adriangohjw adriangohjw requested review from a team July 21, 2025 22:57
@adriangohjw adriangohjw changed the title feat(otp): update OTP verification to include IP address isom-2003 feat(otp): update OTP verification to include IP address Jul 21, 2025
@adriangohjw adriangohjw added the enhancement New feature or request label Jul 21, 2025
Copy link
Contributor

@dcshzj dcshzj left a comment

Choose a reason for hiding this comment

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

I think ok, but feels a bit like the blind leading the blind lol, need to really test on staging to make sure everything is still working as expected.

@adriangohjw
Copy link
Contributor Author

I think ok, but feels a bit like the blind leading the blind lol, need to really test on staging to make sure everything is still working as expected.

@dcshzj yeah i agree :sadge:

@adriangohjw adriangohjw merged commit 8e052bd into develop Jul 23, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants