Skip to content
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

Prevent Disguised Links #1428

Closed
wants to merge 4 commits into from

Conversation

tsmith123
Copy link
Contributor

@tsmith123 tsmith123 commented Sep 24, 2024

Description

Fixes #1397

When adding links with a "text part" and an "href part" to the editor, if the text part contains a string that resembles a link then ensure that it's the same as the href part. If it isn't, then render the href part only.

@huumn
Copy link
Member

huumn commented Sep 24, 2024

I think the goal here is to prevent impersonation of any link - not just SN links. It's pretty non-trivial though because we'd need a list tlds. Or maybe simply checking for \[a-zA-Z0-9]\\.[a-zA-Z0-9]\ and seeing that it doesn't match the url would be enough.

@ekzyis made the issue maybe he has a better idea for what this should do.

@tsmith123
Copy link
Contributor Author

No probs. I'll wait for refinement on the requirements and then make the change.

@ekzyis
Copy link
Member

ekzyis commented Sep 25, 2024

Yes, I intended to solve generally misleading links. I think we can use the regexp @huumn mentioned and URL parsing to check if it looks misleading like this:

diff --git a/components/media-or-link.js b/components/media-or-link.js
index 76ac52d7..5639d3f6 100644
--- a/components/media-or-link.js
+++ b/components/media-or-link.js
@@ -11,6 +11,7 @@ import YouTube from 'react-youtube'
 import useDarkMode from './dark-mode'
 
 function LinkRaw ({ href, children, src, rel }) {
+  // prevent misleading users via URLs from origin X disguised as from origin Y
   const isRawURL = /^https?:\/\//.test(children?.[0])
   return (
     // eslint-disable-next-line
diff --git a/components/text.js b/components/text.js
index ac5c2851..8c5c7758 100644
--- a/components/text.js
+++ b/components/text.js
@@ -176,9 +176,21 @@ export default memo(function Text ({ rel, imgproxyUrls, children, tab, itemId, o
         return href
       }
 
-      // If [text](url) was parsed as <a> and text is not empty and not a link itself,
-      // we don't render it as an image since it was probably a conscious choice to include text.
-      const text = children[0]
+      let text = children[0]
+      // we use the actual URL as the text if text in [text](url) is a misleading
+      if (/^\s*(\w+\.)+\w+/.test(text)) {
+        let misleading = false
+        try {
+          if (new URL(text).origin !== new URL(href).origin) misleading = true
+        } catch {}
+
+        try {
+          if (new URL('https://' + text).origin !== new URL(href).origin) misleading = true
+        } catch {}
+
+        if (misleading) text = href
+      }
+
       let url
       try {
         url = !href.startsWith('/') && new URL(href)
@@ -254,7 +266,7 @@ export default memo(function Text ({ rel, imgproxyUrls, children, tab, itemId, o
       }
 
       // assume the link is an image which will fallback to link if it's not
-      return <TextMediaOrLink src={href} rel={rel ?? UNKNOWN_LINK_REL} {...props}>{children}</TextMediaOrLink>
+      return <TextMediaOrLink src={href} rel={rel ?? UNKNOWN_LINK_REL} {...props}>{text}</TextMediaOrLink>
     },
     img: TextMediaOrLink
   }), [outlawed, rel, itemId, Code, P, Heading, Table, TextMediaOrLink])

This seemed to work during my shallow testing. Maybe you can test this @tsmith123 more and if you don't find anything or don't have a better solution, push it to this PR.

@tsmith123
Copy link
Contributor Author

tsmith123 commented Sep 25, 2024

Hey @ekzyis
Thanks for the suggestion. However, I don't think it works correctly as it always seems to return true for misleading whether the text part is the same as the href part or not.

Therefore, I've modified the logic slightly and I've also moved it into its own function called isMisleadingLink (feel free to suggest a new name).

Note: when using the example from the issue description (with the npub link) it renders an embed rather than just the text like in the actual post. I don't know SN well enough to know if this is a recession so if you could check this that would be cool.

@ekzyis ekzyis self-requested a review September 27, 2024 14:09
@huumn
Copy link
Member

huumn commented Sep 28, 2024

I ended up fixing this in #1430 as I was refactoring the nasty Text component. I used your disguised link logic so I'll award you the sats you would've earned if this were merged.

@huumn huumn closed this Sep 28, 2024
@tsmith123
Copy link
Contributor Author

Cheers @huumn appreciate that 👍

@tsmith123
Copy link
Contributor Author

Did your refactor get merged?

@huumn
Copy link
Member

huumn commented Oct 2, 2024

Yep, it's merged. I haven't the last few days of awards though. I'll try to do it by end of day.

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.

It's still possible to disguise links as other links
3 participants