feat: check for malicious link just before redirect #2403

dcshzj · 2025-07-22T10:56:14Z

Problem

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

Closes ISOM-2006.

Solution

How did you solve the problem?

Features:

Add a check on the destination URL to ensure that it is not malicious just before redirecting the user.
Save the malicious scan result into the database with an expiry date.
Automatically disable the link and email the link owner when the destination URL is detected to be malicious.

Tests

What tests should be run to confirm functionality?

TO BE ADDED

linear · 2025-07-22T10:56:17Z

ISOM-2006 [VAPT] Go scanning is only done at creation

Copilot

Pull Request Overview

This PR implements malicious link detection and prevention in the URL shortening service. When users access shortened URLs, the system now performs safety checks on the destination URLs and automatically deactivates malicious links while notifying link owners.

Adds real-time malicious URL detection during redirects with database caching of scan results
Implements automatic link deactivation and email notifications for detected threats
Updates repository and service interfaces to support safety browsing functionality

Reviewed Changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`src/server/repositories/UrlRepository.ts`	Core implementation for safe browsing expiry tracking and URL deactivation
`src/server/modules/redirect/services/RedirectService.ts`	Malicious link detection logic during redirect flow
`src/server/services/email.ts`	Email notification service for deactivated malicious links
`src/server/modules/user/services/UrlManagementService.ts`	Service methods for deactivating malicious URLs and notifying owners
`src/server/util/safeBrowsing.ts`	Utility for calculating safe browsing result expiry dates

Comments suppressed due to low confidence (2)

test/server/repositories/UrlRepository.test.ts:711

Test implementation is skipped for updateSafeBrowsingExpiry method. This leaves the happy path uncovered for a critical security feature.

    it.skip('should update the safe browsing expiry for an existing shortUrl', () => {})

test/server/repositories/UrlRepository.test.ts:725

Test implementation is skipped for deactivateShortUrl method. This leaves the happy path uncovered for a critical security feature.

    it.skip('should deactivate an existing shortUrl', () => {})

src/server/services/email.ts

src/server/modules/redirect/services/RedirectService.ts

seaerchin

issues wrt the data structure being changed impacting the code, especially because we rely on it for doing our code scanning.

src/server/modules/redirect/services/RedirectService.ts

seaerchin · 2025-08-04T06:00:24Z

src/server/repositories/UrlRepository.ts

-          resolve(cacheLongUrl)
+
+          try {
+            const redirectDestination = JSON.parse(cacheLongUrl)


[issue] i think there are 2 issues here:

first, i think we updated the data structure from string -> json, which might cause issues when we're trying to read the new value. i think our new code cannot reject outright and we'd need to handle the fallback of parsing a string value rather than our json encoded value.

secondly, because our deploy window is so long, we need to handle the case where our old code reads a json encoded value. this will cause our existing flow to mess up, so it needs to be handled properly too.

from our offline discussion, i think we can do a small release so that we wrap the JSON.parse method here to try reading it as a JSON value first, then fallback to raw string

Updated in 647fee7, will push a second PR to revert back the changes

seaerchin · 2025-08-04T06:06:00Z

src/server/repositories/UrlRepository.ts

-        logger.error(`Unable to cache short URL: ${error}`),
-      )
-      return longUrl
+  public getLongUrl: (shortUrl: string) => Promise<RedirectDestination> =


we need to relax this return condition to RedirectDestination | string due to the previously mentioned issue of data format changing

Let me do a split of the PRs so they get released separately.

src/server/repositories/UrlRepository.ts

seaerchin · 2025-08-04T06:30:31Z

(some stuff that claude pointed out)

our scanning takes place in the hot path, which might delay the redirect, especially because this is user -> us -> webrisk
rate limiting for threat scanning API call (i think we don't need this because we cache our results and we also check if we need to scan first)
silent failure on UrlManagementService:237 when we try to lookup users (<- afaict, we don't throw an error but we do catch the null case)

adriangohjw · 2025-08-04T06:50:10Z

bugbot run

cursor · 2025-08-04T06:53:01Z

src/server/repositories/UrlRepository.ts

+                `longUrl not found in cache:\tshortUrl=${shortUrl}`,
+              ),
+            )
+          }


Bug: Cache Parsing Error Causes Premature Cache Misses

The cache format for short URL redirects changed from plain strings to JSON objects. getLongUrlFromCache attempts to JSON.parse() existing string-formatted cache entries. On parsing failure, it incorrectly rejects with NotFoundError, causing valid cached URLs to return 404s or be treated as cache misses until old entries expire. The code should gracefully handle the legacy string format.

Wonderful, this was the same point raised by @seaerchin earlier. To mitigate, we will do as a 3-step release:

Update app code to handle both cases, using the JSON.parse function optimistically, then falling back to using strings

Update app code to start inserting using the new JSON object

Update app code to remove the handling of the string case

praise bugbot

seaerchin · 2025-08-07T07:29:29Z

src/server/repositories/UrlRepository.ts

+        const redirectDestination = await this.getLongUrlFromDatabase(shortUrl)
+        // FIXME: Cache the entire RedirectDestination object once all app
+        // clients are updated to use the new RedirectDestination type.
+        this.cacheShortUrl(shortUrl, redirectDestination.longUrl).catch(


[question] why are we not awaiting the cache call? doesn't seem like it has an impact, but just curious

Hmm I think it's cos the adding to cache part can be done later, so it doesn't impact the person being redirected

* Merge pull request #2402 from opengovsg/feat/add-safebrowsing-expiry feat: add safeBrowsingExpiry column to urls table * feat: check for malicious link just before redirect (#2403) * feat: check for malicious link just before redirect * fix: do not update as JSON object yet * fix: update method signature * 1.83.0

dcshzj requested review from a team and Copilot July 22, 2025 10:56

This comment was marked as outdated.

Sign in to view

dcshzj force-pushed the feat/actively-check-malicious branch 2 times, most recently from d384206 to d9f9416 Compare July 22, 2025 11:02

dcshzj requested a review from Copilot July 22, 2025 11:03

This comment was marked as outdated.

Sign in to view

dcshzj force-pushed the feat/actively-check-malicious branch from d9f9416 to 2ce81ff Compare July 22, 2025 13:46

dcshzj requested a review from Copilot July 22, 2025 13:46

Copilot AI reviewed Jul 22, 2025

View reviewed changes

src/server/services/email.ts Show resolved Hide resolved

src/server/modules/redirect/services/RedirectService.ts Show resolved Hide resolved

dcshzj force-pushed the feat/actively-check-malicious branch from 2ce81ff to 705b25f Compare July 23, 2025 02:19

harishv7 requested review from gweiying and thanhdatle July 23, 2025 04:58

seaerchin reviewed Aug 4, 2025

View reviewed changes

cursor bot reviewed Aug 4, 2025

View reviewed changes

dcshzj force-pushed the feat/actively-check-malicious branch 2 times, most recently from 6e62156 to 647fee7 Compare August 4, 2025 16:03

dcshzj requested a review from seaerchin August 5, 2025 02:59

dcshzj mentioned this pull request Aug 6, 2025

feat: start writing RedirectionDestination in redis cache #2407

Merged

seaerchin approved these changes Aug 7, 2025

View reviewed changes

dcshzj changed the base branch from feat/add-safebrowsing-expiry to develop August 20, 2025 10:14

dcshzj added 2 commits August 20, 2025 18:25

feat: check for malicious link just before redirect

8131e2f

fix: do not update as JSON object yet

2203591

dcshzj force-pushed the feat/actively-check-malicious branch from 647fee7 to 2203591 Compare August 20, 2025 10:25

fix: update method signature

15ca42a

dcshzj merged commit ad43c42 into develop Aug 20, 2025
16 checks passed

feat: check for malicious link just before redirect #2403

feat: check for malicious link just before redirect #2403

Uh oh!

Conversation

dcshzj commented Jul 22, 2025

Problem

Solution

Tests

Uh oh!

linear bot commented Jul 22, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

seaerchin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seaerchin Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

dcshzj Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seaerchin Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

dcshzj Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seaerchin commented Aug 4, 2025

Uh oh!

adriangohjw commented Aug 4, 2025

Uh oh!

cursor bot Aug 4, 2025

Choose a reason for hiding this comment

Bug: Cache Parsing Error Causes Premature Cache Misses

Uh oh!

dcshzj Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

seaerchin Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

seaerchin Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

dcshzj Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dcshzj Aug 4, 2025 •

edited

Loading