Skip to content

[SSF-156] add url sanitization#123

Open
amywng wants to merge 7 commits intomainfrom
acw/SSF-156-sanitize-fm-tracking-link
Open

[SSF-156] add url sanitization#123
amywng wants to merge 7 commits intomainfrom
acw/SSF-156-sanitize-fm-tracking-link

Conversation

@amywng
Copy link
Member

@amywng amywng commented Mar 9, 2026

ℹ️ Issue

Closes SSF-156

📝 Description

  • added method to utils to sanitize URLs (trims whitespace, checks for http/https, validates format w/ URL constructor, ensures hostname exists)
  • added URL sanitization to updateTrackingCostInfo method in order.service.ts
  • wrote/added tests for utils method and orders method
  • updated trackingCostDTO to only accept http/https protocols

✔️ Verification

wrote tests, tests pass
verified behavior in postman

🏕️ (Optional) Future Work / Notes

@amywng amywng requested a review from dburkhart07 March 9, 2026 07:41
Copy link

@dburkhart07 dburkhart07 left a comment

Choose a reason for hiding this comment

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

one more thing!!!!

@amywng amywng requested a review from dburkhart07 March 10, 2026 02:43
Copy link

@dburkhart07 dburkhart07 left a comment

Choose a reason for hiding this comment

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

lgtm!!! 🗽 🍵 🧋

@dburkhart07
Copy link

Note for @sam-schu and @Yurika-Kan for future ticketing. We will likely need some form of proper checking to ensure the url is a dns-registered/reachable one. I initially suggested we implement it in the sanitization check, but this really wouldn't work, since we can't assume the url in the database is valid (e.g i publish a site, we save that as the url, then i take it down, and now the tracking link is not valid). To be safe, I think we should be checking each time we retrieve the order from the backend that the url is dns-registered (but something for a future ticket perhaps). If you have other thoughts let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants