Paths

Table of Contentst

-
src/applications/
-
applications/
-
auth/controller/
-
controller/
3/4
PhabricatorAuthRegisterController.php
-
people/storage/
-
storage/
3/4
PhabricatorUser.php

Differential D25841

Account registration: Restrict Real Name length
ClosedPublic
Actions

Authored by aklapper on Nov 29 2024, 14:22.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15962: Account registration: Handle too long Real Name more gracefully

Commits

rPeb380f922c9a: Account registration: Restrict Real Name length

Summary

Avoid a database exception at user account registration when users enter very long real names by setting a maximum length.
This does not affect existing account data as it is only called in the account registration code.

Fixes T15962

Test Plan

Go to http://phorge.localhost/auth/register/ and enter long values into the "Real Name" field

Diff Detail

Repository

rP Phorge

Branch

T15962 (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 1632
Build 1632: arc lint + arc unit

Event Timeline

aklapper created this revision.Nov 29 2024, 14:22

Owners added a reviewer: O1: Blessed Committers.Nov 29 2024, 14:22

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptNov 29 2024, 14:22

aklapper requested review of this revision.Nov 29 2024, 14:22

Harbormaster completed remote builds in B1632: Diff 2542.Nov 29 2024, 14:22

• l2dy subscribed.Nov 29 2024, 14:29

• l2dy added inline comments.

src/applications/people/storage/PhabricatorUser.php
557	In function `describeValidUsername()`, the number is wrapped in `new PhutilNumber()`. Should they be consistent?

aklapper edited the summary of this revision. (Show Details)Nov 29 2024, 14:30

Thanks for catching that! Amend in new PhutilNumber()

Harbormaster completed remote builds in B1633: Diff 2543.Nov 29 2024, 14:33

aklapper marked an inline comment as done.Nov 29 2024, 14:33

sgtm. Thanks :) just that comment

src/applications/people/storage/PhabricatorUser.php
561	We can just return the result of strlen() <= that Thanks :)

This revision is now accepted and ready to land.Nov 29 2024, 18:11

Simplify return of validateRealName() as proposed by Valerio

Harbormaster completed remote builds in B1634: Diff 2544.Nov 30 2024, 12:17

Double slam accept

aklapper marked an inline comment as done.Dec 2 2024, 19:36

Closed by commit rPeb380f922c9a: Account registration: Restrict Real Name length. · Explain WhyDec 2 2024, 19:36

This revision was automatically updated to reflect the committed changes.

aklapper added a commit: rPeb380f922c9a: Account registration: Restrict Real Name length.

valerio.bozzolan mentioned this in Next Up.Dec 10 2024, 14:03

avivey mentioned this in 2025.18.Fri, May 2, 11:35

Revision Contents
Changeset List

Path

Size

Coverage (All)

Coverage (Touched)

Packages

src/

applications/

auth/

controller/

PhabricatorAuthRegisterController.php

4 lines

O1: Blessed Committers

people/

storage/

PhabricatorUser.php

14 lines

28%

O1: Blessed Committers

Diff 2542

View Options

src/applications/auth/controller/PhabricatorAuthRegisterController.php

Show First 20 Lines • Show All 743 Lines • ▼ Show 20 Lines

private function buildBinaryChange(ArcanistDiffChange $change, $old_binary) {

// original binary for the source and the current binary for the

// destination.

if ($old_binary) {

if ($old_binary->getOriginalFileData() !== null) {

$old_data = $old_binary->getOriginalFileData();

$old_phid = null;

} else {

$old_data = null;

StenAuthorUnsubmitted

Done

Note: Changing the default to '' doesn't work, because $old_data might be set to null by $this->getBlob() on line 762.

Sten: Note: Changing the default to '' doesn't work, because $old_data might be set to null by $this…

$old_phid = $old_binary->getMetadata('old:binary-phid');

}

} else {

$old_data = $change->getOriginalFileData();

$old_phid = $change->getMetadata('old:binary-phid');

}

if ($old_data === null && $old_phid) {

$name = basename($change->getOldPath());

$old_data = $this->getBlob($old_phid, $name);

}

$old_length = strlen($old_data);

$old_length = phutil_nonempty_string($old_data) ? strlen($old_data) : 0;

StenAuthorUnsubmitted

Done

Note: $old_data is explicitly allowed to be null, as per line 781 below.

Sten: Note: $old_data is explicitly allowed to be null, as per line 781 below.

aviveyUnsubmitted

Not Done

$old_data = $this->getBlob($old_phid, $name);

}

- $old_length = ($old_data === null) ? 0 : strlen($old_data);

+ $old_length = phutil_nonempty_string($old_data);

// Here, and below, the binary will be emitted with base85 encoding. This

avivey:

// Here, and below, the binary will be emitted with base85 encoding. This

// encoding encodes each 4 bytes of input in 5 bytes of output, so we may

// need up to 5/4ths as many bytes to represent it.

// We reserve space up front because base85 encoding isn't super cheap. If

// the blob is enormous, we'd rather just bail out now before doing a ton

// of work and then throwing it away anyway.

// However, the data is compressed before it is emitted so we may actually

// end up using fewer bytes. For now, the allocator just assumes the worst

// case since it isn't important to be precise, but we could do a more

// exact job of this.

$this->reserveBytes($old_length * 5 / 4);

if ($old_data === null) {

$old_data = '';

$old_sha1 = str_repeat('0', 40);

} else {

$old_sha1 = sha1("blob {$old_length}\0{$old_data}");

}

$new_phid = $change->getMetadata('new:binary-phid');

$new_data = null;

StenAuthorUnsubmitted

Done

Note: as with $old_data, $new_data can be set to null on line 795 below, so changing the default to '' here doesn't work.

Sten: Note: as with $old_data, $new_data can be set to null on line 795 below, so changing the…

if ($change->getCurrentFileData() !== null) {

$new_data = $change->getCurrentFileData();

} else if ($new_phid) {

$name = basename($change->getCurrentPath());

$new_data = $this->getBlob($new_phid, $name);

}

$new_length = strlen($new_data);

$new_length = phutil_nonempty_string($new_data) ? strlen($new_data) : 0;

$this->reserveBytes($new_length * 5 / 4);

if ($new_data === null) {

$new_data = '';

$new_sha1 = str_repeat('0', 40);

} else {

$new_sha1 = sha1("blob {$new_length}\0{$new_data}");

}

▲ Show 20 Lines • Show All 256 Lines • Show Last 20 Lines

View Options

src/applications/people/storage/PhabricatorUser.php

Show First 20 Lines • Show All 743 Lines • ▼ Show 20 Lines

private function buildBinaryChange(ArcanistDiffChange $change, $old_binary) {

// original binary for the source and the current binary for the

// destination.

if ($old_binary) {

if ($old_binary->getOriginalFileData() !== null) {

$old_data = $old_binary->getOriginalFileData();

$old_phid = null;

} else {

$old_data = null;

StenAuthorUnsubmitted

Done

Note: Changing the default to '' doesn't work, because $old_data might be set to null by $this->getBlob() on line 762.

Sten: Note: Changing the default to '' doesn't work, because $old_data might be set to null by $this…

$old_phid = $old_binary->getMetadata('old:binary-phid');

}

} else {

$old_data = $change->getOriginalFileData();

$old_phid = $change->getMetadata('old:binary-phid');

}

if ($old_data === null && $old_phid) {

$name = basename($change->getOldPath());

$old_data = $this->getBlob($old_phid, $name);

}

$old_length = strlen($old_data);

$old_length = phutil_nonempty_string($old_data) ? strlen($old_data) : 0;

StenAuthorUnsubmitted

Done

Note: $old_data is explicitly allowed to be null, as per line 781 below.

Sten: Note: $old_data is explicitly allowed to be null, as per line 781 below.

aviveyUnsubmitted

Not Done

$old_data = $this->getBlob($old_phid, $name);

}

- $old_length = ($old_data === null) ? 0 : strlen($old_data);

+ $old_length = phutil_nonempty_string($old_data);

// Here, and below, the binary will be emitted with base85 encoding. This

avivey:

// Here, and below, the binary will be emitted with base85 encoding. This

// encoding encodes each 4 bytes of input in 5 bytes of output, so we may

// need up to 5/4ths as many bytes to represent it.

// We reserve space up front because base85 encoding isn't super cheap. If

// the blob is enormous, we'd rather just bail out now before doing a ton

// of work and then throwing it away anyway.

// However, the data is compressed before it is emitted so we may actually

// end up using fewer bytes. For now, the allocator just assumes the worst

// case since it isn't important to be precise, but we could do a more

// exact job of this.

$this->reserveBytes($old_length * 5 / 4);

if ($old_data === null) {

$old_data = '';

$old_sha1 = str_repeat('0', 40);

} else {

$old_sha1 = sha1("blob {$old_length}\0{$old_data}");

}

$new_phid = $change->getMetadata('new:binary-phid');

$new_data = null;

StenAuthorUnsubmitted

Done

Note: as with $old_data, $new_data can be set to null on line 795 below, so changing the default to '' here doesn't work.

Sten: Note: as with $old_data, $new_data can be set to null on line 795 below, so changing the…

if ($change->getCurrentFileData() !== null) {

$new_data = $change->getCurrentFileData();

} else if ($new_phid) {

$name = basename($change->getCurrentPath());

$new_data = $this->getBlob($new_phid, $name);

}

$new_length = strlen($new_data);

$new_length = phutil_nonempty_string($new_data) ? strlen($new_data) : 0;

$this->reserveBytes($new_length * 5 / 4);

if ($new_data === null) {

$new_data = '';

$new_sha1 = str_repeat('0', 40);

} else {

$new_sha1 = sha1("blob {$new_length}\0{$new_data}");

}

▲ Show 20 Lines • Show All 256 Lines • Show Last 20 Lines

Account registration: Restrict Real Name lengthClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 2542

src/applications/auth/controller/PhabricatorAuthRegisterController.php

src/applications/people/storage/PhabricatorUser.php

Account registration: Restrict Real Name length
ClosedPublic
Actions

Revision Contents
Changeset List