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

Dont allow community urls like /c/{id} (fixes #611) #612

Merged
merged 4 commits into from
Apr 8, 2022

Conversation

Nutomic
Copy link
Member

@Nutomic Nutomic commented Apr 6, 2022

If the community identifier in url can be interpreted as a valid integer, lemmy-ui considers it as community id, which breaks things if a community has a name with only numbers.

@Nutomic Nutomic requested a review from dessalines as a code owner April 6, 2022 10:37
@dessalines
Copy link
Member

dessalines commented Apr 6, 2022

This is also an issue for usernames, which is in profile.tsx. There are a few options:

  • Now that fully qualified names are working ( IE /c/[email protected] ), then we can remove the number checks, and also remove numeric ID fetching from the back end completely, and from lemmy-js-client.
  • Leave the numeric fetching in place, but add /username/{number} and /community/{number} in addition to the /c/ and /u/ front end routes. Then when doing that check, use the split[3] == "username" or whatever it is to determine whether it should fetch the number or string version.

I kinda think the 2nd option is best, because there might be some numeric fetching that we're using that I'm missing right now.

@Nutomic
Copy link
Member Author

Nutomic commented Apr 7, 2022

Removed it from profile.tsx as well. I dont think it makes sense to keep around legacy code because "it might still be used somewhere". That way we will just keep adding more logic that needs to be maintained, with no benefit for anyone.

Edit: nevermind, i tried to remove those params and it already causes issues with federation tests which seem tricky to fix. And besides, its much easier for a client dev to request users/communities by id, than constructing the @actor@domain format. So i think its best to remove these http routes from lemmy-ui, but keep id-based api endpoints in lemmy backend.

} else {
id = Number(idOrName);
}
let name_ = idOrName;
Copy link
Member

Choose a reason for hiding this comment

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

You can just do

let name_ = pathSplit[2];

};
setOptionalAuth(form, req.auth);
this.setIdOrName(form, person_id, username);
form.username;
Copy link
Member

Choose a reason for hiding this comment

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

This line might have got in by accident.

};
setOptionalAuth(form, req.auth);
this.setIdOrName(form, person_id, username);
form.username = username;
Copy link
Member

Choose a reason for hiding this comment

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

This line is redundant now since you have the username in the form above.

@Nutomic Nutomic force-pushed the no-numeric-community-url branch from 4f176a7 to f3c7a2c Compare April 8, 2022 13:23
@dessalines dessalines force-pushed the no-numeric-community-url branch from f3c7a2c to 405faa2 Compare April 8, 2022 13:27
@dessalines dessalines enabled auto-merge (squash) April 8, 2022 13:27
@dessalines dessalines merged commit 06c3e54 into main Apr 8, 2022
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