Skip to content

fix(commands): Improve phone number input validation of OCC commands #17173

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.

Sign up for GitHub

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

Merged
nickvergessen merged 3 commits into from
Mar 30, 2026
Merged

fix(commands): Improve phone number input validation of OCC commands #17173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d0d23ee
fix(commands): Improve phone number input validation of OCC commands
nickvergessen Feb 19, 2026
1339617
fix(sip): Check for invalid phone numbers in setupcheck
nickvergessen Feb 20, 2026
062bacc
test(sip): Add more phone numbers to the test
nickvergessen Mar 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions lib/Command/PhoneNumber/AddPhoneNumber.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
use OC\Core\Command\Base;
use OCA\Talk\Model\PhoneNumber;
use OCA\Talk\Model\PhoneNumberMapper;
use OCA\Talk\Service\PhoneNumberValidation;
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\IPhoneNumberUtil;
use OCP\IUser;
use OCP\IUserManager;
use Symfony\Component\Console\Input\InputArgument;
Expand All @@ -24,7 +24,7 @@ class AddPhoneNumber extends Base {

public function __construct(
private IUserManager $userManager,
private IPhoneNumberUtil $phoneNumberUtil,
private PhoneNumberValidation $phoneNumberValidation,
private PhoneNumberMapper $mapper,
) {
parent::__construct();
Expand Down Expand Up @@ -66,12 +66,12 @@ protected function execute(InputInterface $input, OutputInterface $output): int
}
$userId = $user->getUID();

$phoneNumberStandard = preg_match('/^[0-9]{1,20}$/', $phoneNumber) ? $phoneNumber : $this->phoneNumberUtil->convertToStandardFormat($phoneNumber);
if ($phoneNumberStandard === null) {
try {
$phoneNumber = $this->phoneNumberValidation->validateNumber($phoneNumber);
} catch (\InvalidArgumentException) {
$output->writeln('<error>Not a valid phone number ' . $phoneNumber . '. The format is invalid.</error>');
return self::FAILURE;
}
$phoneNumber = $phoneNumberStandard;

try {
$entry = $this->mapper->findByPhoneNumber($phoneNumber);
Expand Down
10 changes: 5 additions & 5 deletions lib/Command/PhoneNumber/ImportPhoneNumbers.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
use OC\Core\Command\Base;
use OCA\Talk\Model\PhoneNumber;
use OCA\Talk\Model\PhoneNumberMapper;
use OCA\Talk\Service\PhoneNumberValidation;
use OCP\IDBConnection;
use OCP\IPhoneNumberUtil;
use OCP\IUser;
use OCP\IUserManager;
use Symfony\Component\Console\Input\InputInterface;
Expand All @@ -23,7 +23,7 @@ class ImportPhoneNumbers extends Base {

public function __construct(
private IUserManager $userManager,
private IPhoneNumberUtil $phoneNumberUtil,
private PhoneNumberValidation $phoneNumberValidation,
private PhoneNumberMapper $mapper,
private IDBConnection $db,
) {
Expand Down Expand Up @@ -72,12 +72,12 @@ protected function execute(InputInterface $input, OutputInterface $output): int
continue;
}

$phoneNumberStandard = preg_match('/^[0-9]{1,20}$/', $row[0]) ? $row[0] : $this->phoneNumberUtil->convertToStandardFormat($row[0]);
if ($phoneNumberStandard === null) {
try {
$row[0] = $this->phoneNumberValidation->validateNumber($row[0]);
} catch (\InvalidArgumentException) {
$output->writeln('<error>Not a valid phone number ' . $row[0] . '. The format is invalid.</error>');
return self::FAILURE;
}
$row[0] = $phoneNumberStandard;

$user = $this->userManager->get($row[1]);
if (!$user instanceof IUser) {
Expand Down
55 changes: 55 additions & 0 deletions lib/Service/PhoneNumberValidation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
<?php

declare(strict_types=1);
/**
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OCA\Talk\Service;

use OCP\IConfig;
use OCP\IPhoneNumberUtil;

class PhoneNumberValidation {

public function __construct(
protected IPhoneNumberUtil $phoneNumberUtil,
protected IConfig $config,
) {
}

/**
* Validate input as a phone number
*
* - Local number: allow
* - International number
* a. If valid, strip + and allow
* b. If invalid, throw
* @throws \InvalidArgumentException When the number is invalid
*/
public function validateNumber(string $phoneNumber): string {

if (
// Not an internation number
!str_starts_with($phoneNumber, '0')
// And matches a local number or dial-through
&& preg_match('/^[0-9]{1,20}$/', $phoneNumber)
) {
return $phoneNumber;

@nickvergessen nickvergessen Feb 19, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@fancycode is the free pass for local numbers okay? Or should e.g. 0711 be replaced with 49711 if DE is specified as default phone prefix. I assume it would be bad, as it would prevent specifying 0711… even if that would be what your SIP gate receives

@fancycode fancycode Feb 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

An internal extension should never start with a 0, so you could add the international prefix if configured (i.e. 071112345 becomes +4971112345) or just reject such numbers.

To make things worse, the 00 prefix for international numbers is only if calling from Germany to another country. For example calling from the US to another country needs 011 as prefix, from Australia it's 0011. See https://en.wikipedia.org/wiki/List_of_international_call_prefixes for other prefixes. So replacing 00 with a + will fail for Australian people trying to configure 00114971112345 as German number in Stuttgart.

You could try to guess based on the default phone prefix what the user might try to do, but maybe the easiest is to simply reject numbers that start with a 0.

@nickvergessen nickvergessen Mar 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So the only thing I can do is look for a leading 0 or +
try to convert to standard and strip of the +, if there was no + return it's invalid?

@fancycode fancycode Mar 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, I think that would be the best, though libphonenumber supports detecting valid numbers with a leading 0 if a country is configured.

Some testcases (assuming DE is configured as default phone country):

4971112345 -> 4971112345
+4971112345 -> 4971112345
4911223344 -> reject (should be detected to be invalid by libphonenumber)
+4911223344 -> reject (should be detected to be invalid by libphonenumber)
004971112345 -> 4971112345 (should be detected to be valid by libphonenumber, would be invalid if Australia is configured as default phone country)
00112345678901 -> 112345678901 (looks like a valid US number for libphonenumber)
00111122233344 -> reject (not a valid US number)
071112345 -> 4971112345 (0711 is a German local prefix)
011223344 -> reject (01122 is not a valid German local prefix)
3001 -> 3001 (might be an extension in a local PBX)
03001 -> reject (030 could be Berlin but 01 is too short)

@nickvergessen nickvergessen Mar 20, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

00112345678901 -> 112345678901 (looks like a valid US number for libphonenumber)

This one is striping off a leading 1, but sure why. But the normalizing returns: +12345678901 so only a single 1 not 2 as you posted.

}

$defaultRegion = $this->config->getSystemValueString('default_phone_region') ?: null;
$standardPhoneNumber = $this->phoneNumberUtil->convertToStandardFormat($phoneNumber, $defaultRegion);

if ($standardPhoneNumber === null) {
throw new \InvalidArgumentException();
}

if (str_starts_with($standardPhoneNumber, '+')) {
return substr($standardPhoneNumber, 1);
}

throw new \InvalidArgumentException();
}
}
28 changes: 28 additions & 0 deletions lib/SetupCheck/SIPConfiguration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@
namespace OCA\Talk\SetupCheck;

use OCA\Talk\Config;
use OCP\IDBConnection;
use OCP\IL10N;
use OCP\SetupCheck\ISetupCheck;
use OCP\SetupCheck\SetupResult;

class SIPConfiguration implements ISetupCheck {
public function __construct(
protected readonly Config $talkConfig,
protected readonly IDBConnection $connection,
protected readonly IL10N $l,
) {
}
Expand All @@ -39,9 +41,35 @@ public function run(): SetupResult {
if ($this->talkConfig->getSignalingMode() === Config::SIGNALING_INTERNAL) {
return SetupResult::success($this->l->t('Using the SIP functionality requires a High-performance backend.'));
}

$query = $this->connection->getQueryBuilder();
$query->select('phone_number')
->from('talk_phone_numbers')
->where($query->expr()->like('phone_number', $query->createNamedParameter(
$this->connection->escapeLikeParameter('+') . '%'
)))
->orWhere($query->expr()->like('phone_number', $query->createNamedParameter(
$this->connection->escapeLikeParameter('0') . '%'
)));

$result = $query->executeQuery();
$invalidNumbers = $result->fetchFirstColumn();
$result->closeCursor();

if (!empty($invalidNumbers)) {
$message = $this->l->t("Assigned Talk phone numbers must not start with + or 0. Please remove or update the following numbers:\n{list}");
$message = str_replace(
'{list}',
implode("\n", $invalidNumbers),
$message
);
return SetupResult::error($message, 'https://portal.nextcloud.com/article/Nextcloud-Talk/Nextcloud-Talk-Phone/Direct-Dial-in#content-provisioning');
}

if ($this->talkConfig->getSIPSharedSecret() === '' && $this->talkConfig->getDialInInfo() === '') {
return SetupResult::info($this->l->t('No SIP backend configured'));
}

return SetupResult::success();
}
}
75 changes: 62 additions & 13 deletions tests/integration/features/command/phone-number.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,41 +14,41 @@ Feature: command/phone-number

Given invoking occ with "talk:phone-number:add +49-160-123-12-12 participant1"
Then the command failed with exit code 0
And the command output contains the text "Phone number +491601231212 is now assigned to participant1"
And the command output contains the text "Phone number 491601231212 is now assigned to participant1"

Given invoking occ with "talk:phone-number:find --phone +491601231212"
Given invoking occ with "talk:phone-number:find --phone 491601231212"
Then the command failed with exit code 0
And the command output contains the text "Phone number +491601231212 is assigned to participant1"
And the command output contains the text "Phone number 491601231212 is assigned to participant1"

Given invoking occ with "talk:phone-number:find --phone +49160123121234"
Given invoking occ with "talk:phone-number:find --phone 49160123121234"
Then the command failed with exit code 1
And the command output contains the text "Phone number +49160123121234 could not be found"
And the command output contains the text "Phone number 49160123121234 could not be found"

Given invoking occ with "talk:phone-number:find --user participant1"
Then the command failed with exit code 0
And the command output contains the text "participant1 has phone number +491601231212 assigned"
And the command output contains the text "participant1 has phone number 491601231212 assigned"

Given invoking occ with "talk:phone-number:find --user participant2"
Then the command failed with exit code 1
And the command output contains the text "No phone number found for participant2"

Given invoking occ with "talk:phone-number:add +49-160-123-1213 participant1"
Then the command failed with exit code 0
And the command output contains the text "Phone number +491601231213 is now assigned to participant1"
And the command output contains the text "Phone number 491601231213 is now assigned to participant1"

Given invoking occ with "talk:phone-number:find --user participant1"
Then the command failed with exit code 0
And the command output contains the text "participant1 has the following phone numbers assigned:"
And the command output contains the text "- +491601231212"
And the command output contains the text "- +491601231213"
And the command output contains the text "- 491601231212"
And the command output contains the text "- 491601231213"

Given invoking occ with "talk:phone-number:add +49-160-123-1212 participant2"
Then the command failed with exit code 1
And the command output contains the text "Phone number is already assigned to participant1"

Given invoking occ with "talk:phone-number:add --force '+49-160-123-12-12' participant2"
Then the command failed with exit code 0
And the command output contains the text "Phone number +491601231212 is now assigned to participant2"
And the command output contains the text "Phone number 491601231212 is now assigned to participant2"
And the command output contains the text "Was assigned to participant1"

Given invoking occ with "talk:phone-number:add 23 participant2"
Expand All @@ -58,14 +58,14 @@ Feature: command/phone-number
Given invoking occ with "talk:phone-number:find --user participant2"
Then the command failed with exit code 0
And the command output contains the text "participant2 has the following phone numbers assigned:"
And the command output contains the text "- +491601231212"
And the command output contains the text "- 491601231212"
And the command output contains the text "- 23"

Given invoking occ with "talk:phone-number:find --user participant1"
Then the command failed with exit code 0
And the command output contains the text "participant1 has phone number +491601231213 assigned"
And the command output contains the text "participant1 has phone number 491601231213 assigned"

Given invoking occ with "talk:phone-number:remove +491601231213"
Given invoking occ with "talk:phone-number:remove 491601231213"
Then the command failed with exit code 0

Given invoking occ with "talk:phone-number:find --user participant1"
Expand All @@ -78,3 +78,52 @@ Feature: command/phone-number
Given invoking occ with "talk:phone-number:find --user participant2"
Then the command failed with exit code 1
And the command output contains the text "No phone number found for participant2"

Scenario: Phone number validation
Given invoking occ with "config:system:set default_phone_region --value DE"
# Invalid phone number with +
Given invoking occ with "talk:phone-number:add +4911223344 participant1"
Then the command failed with exit code 1
And the command output contains the text "Not a valid phone number +4911223344. The format is invalid."

# Valid German number
Given invoking occ with "talk:phone-number:add 004971112347 participant1"
Then the command failed with exit code 0
And the command output contains the text "Phone number 4971112347 is now assigned to participant1"

Given invoking occ with "talk:phone-number:add +4971112346 participant1"
Then the command failed with exit code 0
And the command output contains the text "Phone number 4971112346 is now assigned to participant1"

Given invoking occ with "talk:phone-number:add 4971112345 participant1"
Then the command failed with exit code 0
And the command output contains the text "Phone number 4971112345 is now assigned to participant1"

Given invoking occ with "talk:phone-number:add 071112348 participant1"
Then the command failed with exit code 0
And the command output contains the text "Phone number 4971112348 is now assigned to participant1"

# 01122 is not a valid prefix in Germany
Given invoking occ with "talk:phone-number:add 011223344 participant1"
Then the command failed with exit code 1
And the command output contains the text "Not a valid phone number 011223344. The format is invalid."

# Local PBX
Given invoking occ with "talk:phone-number:add 3001 participant1"
Then the command failed with exit code 0
And the command output contains the text "Phone number 3001 is now assigned to participant1"

# Local number 030 Berlin, but 01 is too short
Given invoking occ with "talk:phone-number:add 03001 participant1"
Then the command failed with exit code 1
And the command output contains the text "Not a valid phone number 03001. The format is invalid."

# Invalid German number but seen as local PBX
Given invoking occ with "talk:phone-number:add 4911223344 participant1"
Then the command failed with exit code 0
And the command output contains the text "Phone number 4911223344 is now assigned to participant1"

# Valid US number
Given invoking occ with "talk:phone-number:add 00112345678901 participant1"
Then the command failed with exit code 0
And the command output contains the text "Phone number 12345678901 is now assigned to participant1"
Loading