-
Notifications
You must be signed in to change notification settings - Fork 45
isom-2008 feat(redirect): enhance checks for bypassing transition page #2401
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
Merged
adriangohjw
merged 7 commits into
develop
from
adriangohjw/isom-2008-vapt-insufficient-referrer-validation-bypasses-transition
Jul 23, 2025
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
bb8edc1
feat(redirect): enhance with more robust URL checker + extract isFrom…
adriangohjw 567eb21
test(redirect): add comprehensive unit tests for RedirectService func…
adriangohjw 29b40c8
test(redirect): update RedirectService tests to reflect direct redire…
adriangohjw bf3fb8b
test(redirect): update RedirectService test to use full staging URL f…
adriangohjw 9206094
test(redirect): modify RedirectService test to use specific staging U…
adriangohjw 36b91b0
test(redirect): rename test suite to 'redirectFor' for clarity in Red…
adriangohjw 1684f26
test(redirect): refactor RedirectService test setup for improved clar…
adriangohjw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
192 changes: 192 additions & 0 deletions
192
src/server/modules/redirect/services/__tests__/RedirectService.test.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,192 @@ | ||
| import { RedirectService } from '../RedirectService' | ||
| import { UrlRepositoryInterface } from '../../../../repositories/interfaces/UrlRepositoryInterface' | ||
| import { CrawlerCheckService } from '../CrawlerCheckService' | ||
| import { CookieArrayReducerService } from '../CookieArrayReducerService' | ||
| import { LinkStatisticsService } from '../../../analytics/interfaces' | ||
| import { RedirectType } from '../..' | ||
|
|
||
| const ogUrl = 'https://go.gov.sg' | ||
|
|
||
| // Mock the config module | ||
| jest.mock('../../../../config', () => { | ||
| return { | ||
| logger: { | ||
| warn: jest.fn(), | ||
| }, | ||
| ogUrl: 'https://go.gov.sg', | ||
| } | ||
| }) | ||
|
|
||
| // Mock dependencies | ||
| const mockUrlRepository = { | ||
| getLongUrl: jest.fn(), | ||
| } | ||
|
|
||
| const mockCrawlerCheckService = { | ||
| isCrawler: jest.fn(), | ||
| } | ||
|
|
||
| const mockCookieArrayReducerService = { | ||
| userHasVisitedShortlink: jest.fn(), | ||
| writeShortlinkToCookie: jest.fn(), | ||
| } | ||
|
|
||
| const mockLinkStatisticsService = { | ||
| updateLinkStatistics: jest.fn(), | ||
| } | ||
|
|
||
| describe('RedirectService', () => { | ||
| // Create service instance with mocked dependencies | ||
| const redirectService = new RedirectService( | ||
| mockUrlRepository as unknown as UrlRepositoryInterface, | ||
| mockCrawlerCheckService as unknown as CrawlerCheckService, | ||
| mockCookieArrayReducerService as unknown as CookieArrayReducerService, | ||
| mockLinkStatisticsService as unknown as LinkStatisticsService, | ||
| ) | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks() | ||
|
|
||
| // Setup default mock returns after clearing | ||
| mockUrlRepository.getLongUrl.mockResolvedValue('https://example.com') | ||
| mockCrawlerCheckService.isCrawler.mockReturnValue(false) | ||
| mockCookieArrayReducerService.userHasVisitedShortlink.mockReturnValue(false) | ||
| mockCookieArrayReducerService.writeShortlinkToCookie.mockReturnValue([ | ||
| 'test', | ||
| ]) | ||
| }) | ||
|
|
||
| describe('redirectFor', () => { | ||
| it('should allow direct redirect for exact ogUrl match when user has not visited before', async () => { | ||
| const result = await redirectService.redirectFor( | ||
| 'test', | ||
| undefined, | ||
| 'Mozilla/5.0', | ||
| ogUrl, | ||
| ) | ||
|
|
||
| expect(result.redirectType).toBe(RedirectType.Direct) | ||
| }) | ||
|
|
||
| it('should allow direct redirect for ogUrl with path when user has not visited before', async () => { | ||
| const result = await redirectService.redirectFor( | ||
| 'test', | ||
| undefined, | ||
| 'Mozilla/5.0', | ||
| `${ogUrl}/some-path`, | ||
| ) | ||
|
|
||
| expect(result.redirectType).toBe(RedirectType.Direct) | ||
| }) | ||
|
|
||
| it('should allow direct redirect for ogUrl with query params when user has not visited before', async () => { | ||
| const result = await redirectService.redirectFor( | ||
| 'test', | ||
| undefined, | ||
| 'Mozilla/5.0', | ||
| `${ogUrl}?param=value`, | ||
| ) | ||
|
|
||
| expect(result.redirectType).toBe(RedirectType.Direct) | ||
| }) | ||
|
|
||
| it('should show transition page for ogUrl with different protocol when user has not visited before', async () => { | ||
| const result = await redirectService.redirectFor( | ||
| 'test', | ||
| undefined, | ||
| 'Mozilla/5.0', | ||
| 'http://go.gov.sg', // different protocol | ||
| ) | ||
|
|
||
| expect(result.redirectType).toBe(RedirectType.TransitionPage) | ||
| }) | ||
|
|
||
| it('should NOT allow transition page bypass for malicious domain that starts with ogUrl', async () => { | ||
| const result = await redirectService.redirectFor( | ||
| 'test', | ||
| undefined, | ||
| 'Mozilla/5.0', | ||
| `${ogUrl}.malicious.com`, | ||
| ) | ||
|
|
||
| expect(result.redirectType).toBe(RedirectType.TransitionPage) | ||
| }) | ||
|
|
||
| it('should NOT allow transition page bypass for subdomain of ogUrl', async () => { | ||
| const result = await redirectService.redirectFor( | ||
| 'test', | ||
| undefined, | ||
| 'Mozilla/5.0', | ||
| 'https://staging.go.gov.sg', | ||
| ) | ||
|
|
||
| expect(result.redirectType).toBe(RedirectType.TransitionPage) | ||
| }) | ||
|
|
||
| it('should NOT allow transition page bypass for domain containing ogUrl', async () => { | ||
| const result = await redirectService.redirectFor( | ||
| 'test', | ||
| undefined, | ||
| 'Mozilla/5.0', | ||
| 'https://staging.go.gov.sg.malicious.com', | ||
| ) | ||
|
|
||
| expect(result.redirectType).toBe(RedirectType.TransitionPage) | ||
| }) | ||
|
|
||
| it('should NOT allow transition page bypass for completely different domain', async () => { | ||
| const result = await redirectService.redirectFor( | ||
| 'test', | ||
| undefined, | ||
| 'Mozilla/5.0', | ||
| 'https://malicious.com', | ||
| ) | ||
|
|
||
| expect(result.redirectType).toBe(RedirectType.TransitionPage) | ||
| }) | ||
|
|
||
| it('should NOT allow transition page bypass for invalid referrer URL', async () => { | ||
| const result = await redirectService.redirectFor( | ||
| 'test', | ||
| undefined, | ||
| 'Mozilla/5.0', | ||
| 'not-a-valid-url', | ||
| ) | ||
|
|
||
| expect(result.redirectType).toBe(RedirectType.TransitionPage) | ||
| }) | ||
|
|
||
| // TODO: not a regression, but to consider if we want to fix this | ||
| it('should allow direct redirect when user has visited the shortlink before, even from malicious site', async () => { | ||
| // Mock that user has visited this shortlink before | ||
| mockCookieArrayReducerService.userHasVisitedShortlink.mockReturnValue( | ||
| true, | ||
| ) | ||
|
|
||
| const result = await redirectService.redirectFor( | ||
| 'test', | ||
| ['test'], // past visits include this shortlink | ||
| 'Mozilla/5.0', | ||
| 'https://malicious.com', // even from malicious site | ||
| ) | ||
|
|
||
| expect(result.redirectType).toBe(RedirectType.Direct) | ||
| }) | ||
|
|
||
| it('should allow direct redirect when user has visited the shortlink before, even from trusted page', async () => { | ||
| // Mock that user has visited this shortlink before | ||
| mockCookieArrayReducerService.userHasVisitedShortlink.mockReturnValue( | ||
| true, | ||
| ) | ||
|
|
||
| const result = await redirectService.redirectFor( | ||
| 'test', | ||
| ['test'], // past visits include this shortlink | ||
| 'Mozilla/5.0', | ||
| ogUrl, // from trusted page | ||
| ) | ||
|
|
||
| expect(result.redirectType).toBe(RedirectType.Direct) | ||
| }) | ||
| }) | ||
| }) |
Oops, something went wrong.
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.
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.
issue: should use
thisinstead ofRedirectServiceThere 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.
actually im not super familiar with the framework, but i referenced this from an existing code
GoGovSG/src/server/modules/redirect/services/RedirectService.ts
Lines 47 to 49 in 2d94333
GoGovSG/src/server/modules/redirect/services/RedirectService.ts
Lines 98 to 100 in 2d94333
curious - what's the issue for this?
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.
Ah oops sorry was reviewing on mobile, so didn't see the
staticpart. DoingRedirectService.isFromTrustedPageis ok sinceisFromTrustedPageis a static function. If otherwise then should usethis.