-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Hides the '.bsky.social' while also addressing impersonation and security issues #5155
base: main
Are you sure you want to change the base?
Conversation
Enables users to mention without using the full handle by assuming a mention without a valid top level domain contains '.bsky.social'
Checks if '.bsky.social' is the domain and hides it if that's true, it still shows the full handle for custom domains. Also added a Toggle Function to allow users to easily view the full handle for any profile.
How would this would work for people who have custom domains as handle? For example, what if someone has, currently, the handle What if someone else registered the domain For me, having the domain in the handle makes sense in the context of a federated social network, in which handles can come from different PDS's. |
Yes, it would appear as foo.bar but if you go to the main profile and toggle full handle you'd see the entire thing. It only hides '.bsky.social' it won't hide any custom handle at all. When mentioning it checks for a valid top level domain and, if not recognized, it assumes '.bsky.social' as it is handling the mention. |
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.
Expilct
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.
I'm still of the general opinion that making a change like this is a dangerous idea. And here are some specific comments on the exact change too.
} | ||
|
||
const DEFAULT_DOMAIN = 'bsky.social'; | ||
const TLD_REGEX = /.\w+$/; // Regex to check for a valid top-level domain |
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.
.
is special in regex and matches anything. So this whole regex is, mostly, a match anything regex.
if (mention) { | ||
// Check if the mention contains a top-level domain to determine if it's a full handle | ||
const hasTLD = TLD_REGEX.test(mention.did); | ||
const fullMentionHandle = hasTLD ? mention.did : `${mention.did}@${DEFAULT_DOMAIN}`; |
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.
Why is there an @
being put in the middle of a handle?
import {msg} from '@lingui/macro' | ||
import {useLingui} from '@lingui/react' | ||
import {useNavigation} from '@react-navigation/native' | ||
import React, { Fragment, useCallback, useMemo, useState } from 'react'; |
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.
Did an auto reformatter get used on the entire file? It is very hard to see what the actual differences are.
// Process mentions | ||
if (mention) { | ||
// Check if the mention contains a top-level domain to determine if it's a full handle | ||
const hasTLD = TLD_REGEX.test(mention.did); |
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.
Is the intention of this to allow the richtext did attribute stored in the post record to have a partial handle in it? I don't understand why. This change should be purely cosmetic in the UI.
Seems like a bad idea for me. Having to go profile by profile to make sure it’s not an impersonation rather being able to check directly in the feed can make impersonation way easier as most people might not check that. Just wanted to understand more, I ain’t bsky developer, so I have no veto nor any kind of power here. Thanks for the explanation and best of luck in your PR. Also, not sure if this was already discussed, but in big open source projects, might be a good idea to have an RFC or any discussion attached to your PR, as it can be discussed whether it fits the roadmap or if they are willing to accept changes, at least they won’t have to go through all PRs trying to understand what’s happening, why it’s happening, how it’s happening, pros and cons, and maybe some security issues. |
Muitas pessoas querem essa mudança. Nem todos gostam desse bsky.com, eu muito menos. Sendo assim, admito que é uma boa sugestão. |
Do jeito que foi implementado é uma péssima solução. Se não fosse uma rede federada onde você pode ter vários servidores descentralizados e colocar outros domínios, eu até entenderia a mudança, mas ter essa parte, ou algum outro tipo de identificar, é essencial pra evitar personificar outras contas. |
While this certainly gives a more "twitter-esque" feel, I am strongly against it for multiple reasons:
I definitely understand the friction caused and why the change is wanted, but that's one of the aspects of a decentralised network. Atproto does an absolutely impressive job at reducing the friction usually introduced by such architectures, but the domain part is an essential part of the whole thing. |
Addresses full handle handling to make the interface cleaner and more user friendly
Inspired by: @Caitano09 pull request #5117