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

Add unfurling of URLs in chats #11158

Closed
3 tasks
vkjr opened this issue Sep 14, 2020 · 27 comments · Fixed by #11311
Closed
3 tasks

Add unfurling of URLs in chats #11158

vkjr opened this issue Sep 14, 2020 · 27 comments · Fixed by #11311
Assignees
Labels
feature feature requests
Milestone

Comments

@vkjr
Copy link
Contributor

vkjr commented Sep 14, 2020

status-im/status#2

User Story

As a user, I want the app to unfurl URLs so that I can easily see what they refer to without needing to open the URL.

Description

Type: Feature

Summary: If another user shares a URL with me (maybe from a 3rd party app #4605), it would be great if I could see the summary of the content of the URL so I have a quick idea of what it's about.

Expected behavior

  1. User receives URL
  2. App recognizes URL and uses an unfurlingeloper-blog/everything-you-ever-wanted-to-know-about-unfurling-but-were-afraid-to-ask-or-how-to-make-your-e64b4bb9254) library to generate an image (or HTML) content that represents the summary of the URL.

image

Acceptance Criteria

  • Links starting with http:// or https:// are unfurled on the receiving client side only for the URLs in the whitelist if enabled
  • Performance is not noticeably negatively impacted by the new functionality
  • The whitelist includes https://github.com

Security implications

We have to think about the security implications if we do this on the client side (will it be acceptable to access a random URL which might leak metadata about us?)

@vkjr vkjr added the feature feature requests label Sep 14, 2020
@vkjr
Copy link
Contributor Author

vkjr commented Sep 14, 2020

I've recreated issue #4606 from scratch since bounty-related discussions are long and distracting.

@vkjr
Copy link
Contributor Author

vkjr commented Sep 14, 2020

@errorists, could you please check how up-to-date are designs for this feature?
chat part
settings part

@errorists
Copy link
Contributor

errorists commented Sep 14, 2020

I updated the designs to allow more granular whitelisting of domains instead of a global opt-in.
Two entry points:
🅰️ one that's shown if you receive a new message with a link in a chat you're currently in. We should do our best to present this contextually when it's likely to be noticed and only once; otherwise the opt-in switch in Settings alone will go unnoticed.

Frame 51

You can choose to dismiss it, or proceed to select which websites from a list hosted by us (MVP but I see no reason why it couldn't be a community sourced list or multiple lists similar to ad blocking) you want to enable link previews from.

🅱️ Next is in tucked away in Privacy section of Settings. Here you can proceed to the next screen which mirrors the same view found in Chat.

Frame 161

If you enable a website, URLs from that domain get unfurled until disabled. The exact wording could use some massaging 💆

edit: now that I think about it, navigating from Chat to Profile -> Settings instead of a modally presented view embedded in chat, would be the better choice here as it shows where to find it afterwards.

cc @cammellos @hesterbruikman

@vkjr
Copy link
Contributor Author

vkjr commented Sep 14, 2020

@errorists, thanks for thinking it out!
I personally would be glad to have also an option (maybe with additionally shown warning) to allow any url unfurling. Because I don't care where my friend got that funny meme and I want to see it without navigating to jpeg in browser. Wdyt?

@0kok0
Copy link

0kok0 commented Sep 15, 2020

@vkjr @errorists great flow, would like to have fine grained trust/preview settings in a messenger app personally

The security/privacy trade off here is weird and complicated. On a high level, previews trade privacy (cause metadata) for security (cause stupid phishing/click jacking is harder).

  • Generating the preview and sanitizing on a server is what telegram/twitter does. It's strange, breaks e2e encryption, is centralized...
  • Generating the preview on the senders side and rendering only on the receivers side
    • for the sender, this increases privacy but lowers security, as the sender controls the preview generation for a link. Whatsapp does that, and if someone plays around with the preview, things get nasty:
    • for the receiver, privacy is not really affected, as the sender generates the preview probably who has already visited the site to get the link
  • Generating the preview on the receivers side
    • Trades receiver privacy for security, as above attack is more complicated
    • remains a can of worms, as an attacker gets us to render whatever there is on their site when sending a link. One could target whatever code we use to generate the preview on the receiver side. In absence of solid CSP rules, we should default to not render previews as in the above flow. We give up receiver privacy when a receiver generates the preview, but the above attack is more complicated if the attacker has to craft a site that targets the receiver renderer.

@vkjr I would totally not make a switch that renders previews for all senders. Instead, I'd add to the above text that opening or rendering links from untrusted/unknown sources poses a security risk. The policy to whitelist previews only for specific domains is great.

@hesterbruikman
Copy link
Contributor

@vkjr Any thoughts on how a white list can be maintained and used by desktop?

@vkjr
Copy link
Contributor Author

vkjr commented Sep 16, 2020

@hesterbruikman, my suggestion is to maintain trusted urls list inside status-go. This way desktop can easily reuse it and if community wants to extend it - anyone can create a PR. Maybe additional faq can be added to tell community about this list in a code to facilitate PR creation for anyone. wdyt?
cc @cammellos

@cammellos
Copy link
Contributor

status-go sounds like a good place

@vkjr vkjr self-assigned this Sep 16, 2020
@errorists
Copy link
Contributor

IMG_1659

so apparently Signal changed their mind and now it's all URLs

@cammellos
Copy link
Contributor

cammellos commented Sep 18, 2020

Interesting,
signal have links previews generated on the sender side, which hits a signal proxy that connects through ssl to the site and adds some randomness to make sure that sender/receiver are not coupled.
The reason for the whitelist was I believe so that they don't implement what is essentially a proxy for any domain, wonder if things have changed in the way the preview is generated.

@vkjr
Copy link
Contributor Author

vkjr commented Sep 28, 2020

After creating UI ran into the issue - can't find appropriate package to generate link previews. I hoped to use the one I used in old PR for desktop but it is outdated now.
Asked a question on stackoverflow but maybe we will have to create own package and parse opengraph/twitter tags in target url.

@cammellos
Copy link
Contributor

@vkjr not sure we need to use a library for the link preview, as we only support two domains for now and no need to make it generic for the first iteration.

We also probably should consider pulling this information in status-go rather than react-native, so that both desktop and react can benefit from this (this will need to be done asynchronously for performance reasons). @iurimatias what do you think?

@flexsurfer / @Ferossgp what do you think?

@errorists
Copy link
Contributor

@cammellos out of curiosity, which two domains?

@cammellos
Copy link
Contributor

@errorists I was looking at the picture, I assumed github.com & youtube, or am I mistaking?

@errorists
Copy link
Contributor

that's a placeholder only, I have no clue how it should be decided if a website is good enough to be on the whitelist or not :)

@cammellos
Copy link
Contributor

ah :D , I take those two are a safe bets? especially for starting out? maybe some gif website as well, but I would not expand the list a lot for the first iteration, what do you think?

@hesterbruikman
Copy link
Contributor

Naive question, what happens when link unfurling is unfurled...? E.g. https://twitter.com/ethstatus/status/1309133660357091341?s=20 For example, #artproject (currently bridged) is almost exclusively Twitter links, with in turn include image links

@errorists
Copy link
Contributor

yea sorry I should've place PornHub in design instead so it's more obvious :) How about Twitter? if GIFs then Tenor / GIPHY, both have terrible privacy policies so I'm not sure.
but yea I guess something like YouTube, Twitter, GitHub, Reddit, Tenor is a good start and can be expanded upon. Signal had Instagram on their whitelist too, tho not sure of the amount of trackers you hit unfurling from them

@cammellos
Copy link
Contributor

Ok, say to cut the scope to a minimum, we just pick one for this task so we have a fully working implementation, and we add the remaining in separate issues?
We can always decide not to release without them, but at least we can push this out to testing with a single URL as quick as possible, get feedback over it, fix bugs etc and then iterate, what do you say?

@hesterbruikman
Copy link
Contributor

Sounds good! In that case I'd say start with Github to increases chances of CC's testing it.

Separate issues can still be slated for the same release

@cammellos
Copy link
Contributor

Thank you, I'll update the issue and create one for Youtube so we can use it as a template, unless there are objections.

@errorists
Copy link
Contributor

I'd really ask for Twitter instead of Youtube 🙏

@vkjr
Copy link
Contributor Author

vkjr commented Sep 28, 2020

There shouldn't be any difference in implementing url previews for different sites. Basically what we need is to parse tags from pages source code: meta property="og:title", <meta property="twitter:title", etc...
So I think we can easily start from few sites simultaneously, like youtube\github\twitter

@cammellos
Copy link
Contributor

Sure :)
I have just created a skeleton issue , I picked youtube just because designs are already there https://app.zenhub.com/workspaces/status-mobile-5729dbcd8c3f63656a8f6eaa/issues/status-im/status-react/11242 , but feel free to create one for any domain you think we would like to support, and we can prioritize them (I am totally fine with going with twitter first)

@cammellos
Copy link
Contributor

@vkjr up to you, but it's always best to keep the scope to a minimum and don't commit to extra work, as there might be some unknowns that we are not aware of, and it should not delay testing the meat of this PR, which is not the parsing of websites (any library can do that), but the UI/fetching etc (i.e you don't want to block this because it does not work with website x).

@Ferossgp
Copy link
Contributor

Just wondering if was https://oembed.com/ spec considered? I think we could use a subset of features, and have our provider.json somewhere on IPFS

@vkjr
Copy link
Contributor Author

vkjr commented Sep 28, 2020

@Ferossgp, thanks a lot! didn't know about this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature feature requests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants