-
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
Changes from 4 commits
bb8edc1
567eb21
29b40c8
bf3fb8b
9206094
36b91b0
1684f26
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,195 @@ | ||
| 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', () => { | ||
| let redirectService: RedirectService | ||
|
|
||
| beforeEach(() => { | ||
| // Reset mocks | ||
| jest.clearAllMocks() | ||
|
|
||
| // Setup default mock returns | ||
| mockUrlRepository.getLongUrl.mockResolvedValue('https://example.com') | ||
| mockCrawlerCheckService.isCrawler.mockReturnValue(false) | ||
| mockCookieArrayReducerService.userHasVisitedShortlink.mockReturnValue(false) | ||
| mockCookieArrayReducerService.writeShortlinkToCookie.mockReturnValue([ | ||
| 'test', | ||
| ]) | ||
|
|
||
| // Create service instance with mocked dependencies | ||
| redirectService = new RedirectService( | ||
| mockUrlRepository as unknown as UrlRepositoryInterface, | ||
| mockCrawlerCheckService as unknown as CrawlerCheckService, | ||
| mockCookieArrayReducerService as unknown as CookieArrayReducerService, | ||
| mockLinkStatisticsService as unknown as LinkStatisticsService, | ||
| ) | ||
|
||
| }) | ||
|
|
||
| describe('referrer validation', () => { | ||
|
||
| 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.${ogUrl}.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) | ||
| }) | ||
| }) | ||
| }) | ||
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.