matrix-appservice-irc

Implement a less strict validateUsername for user provided usernames

#1538

Closed

validateUsername is way too restrictive, because appservice's !username command is used primarily to configure the account name (required to authenticate), not just the username (which people rarely care about). For example, validateUsername is very strict about the first character, but account names don't have such restrictions.

Additionally, they have conflicting requirements: IdentGenerator.validateUsername can (and should) be overly restrictive when validating usernames because at worst the username will be mangled (like I said, no one cares about it); but !username really should not risk rejecting valid account names because prevents users from authenticating.

@progval I'm not sure I understand. PR adds a comment inside validateUsername that the rules about the first character were taken from there. Could you please clarify what exactly is wrong with this rule?

but !username really should not risk rejecting valid account names because prevents users from authenticating

I just want to point out that it has always been rejecting valid accounts, so at least some improvement in that regard is definitely wanted.

The "username" passed as argument to !username is a SASL PLAIN authcid, which can be any UTF8 string not containing a null char.

The "username" that IdentGenerator.validateUsername validates is what is passed to the USER command, as a substitute for the ident when the client doesn't run an identd, and the grammar is unspecified.

It makes no sense to use the same code for both just because they are both informally called "username".

Re-reading the code, I just noticed IdentGenerator.validateUsername is only used to validate the SASL username, so it shouldn't be IdentGenerator at all; and the quoted grammar is irrelevant because it's the grammar for USER on solanum, not for SASL.

Instead it should simply check there is no null character in the string.

Oh, I think I see now, so you are saying that handleUsername() function being modified in the PR is called from two different locations: from !username and from some other USER command, right? So, the correct fix would be to only change username sanitization for the !username case.

Re-reading the code, I just noticed IdentGenerator.validateUsername is only used to validate the SASL username, so it shouldn't be IdentGenerator at all; and the quoted grammar is irrelevant because it's the grammar for USER on solanum, not for SASL.

Sorry, the page didn't update with a new comment, so I didn't see it when sent the previous one. So, is it the wrong code to modify for the !username sanitization problem?

yes, it's the wrong code. It makes the bug a little less of a problem by being less strict (as you pointed out), but it's still needlessly strict.

@Half-Shot what's the point of having a bug in the first commit which then is being fixed (with no explanation btw) in the latter commit, if you can just have no bug at all? Please squash the first and last commits into one.

Also, I'm not sure what are the project's rules, but I presume that the changelog entry (whose commit title also would need fixing, because just changelog tells basically nothing about the commit) is better to add at the same commit as the one that implemented functional, in which case it's better to squash with the other two as well.

So I'd recommend to squash all 3 commits into one.

what's the point of having a bug in the first commit which then is being fixed (with no explanation btw) in the latter commit, if you can just have no bug at all?

if you look at the timestamps, the last one was authored almost an hour later; so that's because the commits match the actual development history

what's the point of having a bug in the first commit which then is being fixed (with no explanation btw) in the latter commit, if you can just have no bug at all?

if you look at the timestamps, the last one was authored almost an hour later; so that's because the commits match the actual development history

That's… an interesting fact, but this "history" shouldn't have gotten through to the PR. The obvious problem is this would cause bisectability problems. Then there's also that code review often starts with the first commit and this bug being there is just waste of time for a reviewer who finds it, types a comment about the problem and a possible fix, only to later find out that author did address the problem, but for some reason in another commit.

this "history" shouldn't have gotten through to the PR

the "Flip check" commit was authored after Half-Shot opened the PR. This is the correct way to amend pull-requests; because the alternative is to force-push every time, which has a much worse UI on GitHub (and similar forges), and lacks a description of what each change is.

Then the PR will be squashed-merged, like every other PR in this repo.

And even if it wasn't; that's what git bisect skip is for.

This is the correct way to amend pull-requests; because the alternative is to force-push every time, which has a much worse UI on GitHub (and similar forges), and lacks a description of what each change is.

FTR, Github allows to see code changes between two force-pushes. If the problem is just lack of description attached to them, it may be worked around through administrative means, such as asking to pair every force-push with a comment what was done. Another way, which sometimes employed in other FOSS projects is adding a description of what changed in the body of the amended commit.

Creating new commit for each fix hurts stronger than the obliging contributors to elaborate what changed, because that makes problems to code review, which is usually done by maintainers. And maintainers' time is more precious than that of contributors.

This isn't just my opinion, that works like this in all large source projects: Linux kernel, Mesa, Mutter, libinput, X11… As a matter of fact, since this is a Matrix plugin, I opened a list of PRs for Element sorted them by most commented, and looked at a few random, there are also force-pushes, for example: element-hq/element-desktop#170

Like, imagine you send a large refactoring that amounts to 40 patches to Linux kernel, and then get a feedback for most of them, like asking to add a comment or to make use of an existing function… So what, you address the feedback, and send back 70 patches, where first 40 ones still contain the issues, and then new 30 ones are the fixes to those older ones? 😂😂😂 And perhaps with a very informative title like this one in the PR we discuss? 😄😄😄

Then the PR will squashed-merged, like every other PR in this repo.

Wow… I confirm that, I looked through various Contributing.md files, and there's a note saying commits are always squashed. And permits to push new commits fixing old ones (doesn't require doing so though) So, like, if I refactor the project creating 20 patches, there will be no sane history, upon merging it will turn into an indistinguishable wall of changes, without a way to revert a single commit or even understand what was done.

Oh, well. That's the policy I guess, no point to discuss it here then 🤷‍♂️

Sorry, I see no button "resolve", if somebody can close this discussion, please do so.

FTR, Github allows to see code changes between two force-pushes

I'm aware,that's why I said "worse UI", not "no UI"

Creating new commit for each fix hurts stronger than the obliging contributors to elaborate what changed, because that makes problems to code review, which is usually done by maintainers

How so? they don't have to review commit-per-commit.

Like, imagine you send a large refactoring that amounts to 40 patches to Linux kernel, and then get a feedback for most of them, like asking to add a comment or to make use of an existing function… So what, you address the feedback, and send back 70 patches, where first 40 ones still contain the issues, and then new 30 ones are the fixes to those older ones? And perhaps with a very informative title like this one in the PR we discuss?

Yeah, and that's why large projects like Linux don't (or shouldn't) use Github.

, like, if I refactor the project creating 20 patches, there will be no sane history, upon merging it will turn into an indistinguishable wall of changes, without a way to revert a single commit or even understand what was done.

then don't send 20 patches in a single PR.

How so? they don't have to review commit-per-commit.

Sure, you can start review from the global PR diff. But for good code review you need to also check individual commits for problems, both in the code and in description (because this will be the history that you and other contributors will return to whenever some code raises questions and you want to understand why it was added). So actually, one has to.

Yeah, and that's why large projects like Linux don't (or shouldn't) use Github.

Well, Linux kernel aside, all other projects I mentioned use Gitlab and the force-push UI there is similar.

then don't send 20 patches in a single PR.

Is there a better way? If you suggest sending each one in a separate PR, this will be a nightmare to review, because commits in a PR usually rely on the previous ones.

How so? they don't have to review commit-per-commit.

Sure, you can start review from the global PR diff. But for good code review you need to also check individual commits for problems, both in the code and in description (because this will be the history that you and other contributors will return to whenever some code raises questions and you want to understand why it was added). So actually, one has to.

Oh, but you are right of course in the context of matrix-org policy to squash everything. In that case no point to look at individual commits. My answer was more generic, as in, suitable for usual projects, who retain the original commit history. I mean, there's a reason why history should be retained, but that's a separate topic, which no point to discuss here as there's a project policy.

Well, Linux kernel aside, all other projects I mentioned use Gitlab and the force-push UI there is similar.

I meant PR-based workflows, not Github specifically. This includes Gitlab and Gitea.

Personally I find patch-based workflows (by email like Linux, Phabricator, or Gerrit) much better for this, but alas they aren't popular anymore.

Is there a better way? If you suggest sending each one in a separate PR, this will be a nightmare to review, because commits in a PR usually rely on the previous ones.

If you look at this repo, I have a bunch of open PRs that build toward a single feature I want, and all but the last one have independent commits. Yeah it's painful to write PRs this way, but it's the best way to keep them readable despite Github's limitations.