diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 16995d948..d3db4f156 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -57,8 +57,6 @@ jobs: strategy: matrix: include: - - PFSENSE_VERSION: pfSense-2.8.0-RELEASE - FREEBSD_ID: freebsd15 - PFSENSE_VERSION: pfSense-2.8.1-RELEASE FREEBSD_ID: freebsd15 steps: @@ -107,8 +105,6 @@ jobs: strategy: matrix: include: - - PFSENSE_VERSION: pfSense-2.8.0-RELEASE - FREEBSD_ID: freebsd15 - PFSENSE_VERSION: pfSense-2.8.1-RELEASE FREEBSD_ID: freebsd15 steps: @@ -136,8 +132,6 @@ jobs: strategy: matrix: include: - - PFSENSE_VERSION: pfSense-2.8.0-RELEASE - FREEBSD_ID: freebsd15 - PFSENSE_VERSION: pfSense-2.8.1-RELEASE FREEBSD_ID: freebsd15 diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Auth/BasicAuth.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Auth/BasicAuth.inc index 7a3267c37..ce489b699 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Auth/BasicAuth.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Auth/BasicAuth.inc @@ -28,8 +28,8 @@ class BasicAuth extends Auth { */ public function _authenticate(): bool { # Obtain the username and password from the client via Basic authentication - $this->username = $_SERVER['PHP_AUTH_USER'] ?: ''; - $password = $_SERVER['PHP_AUTH_PW'] ?: ''; + $this->username = $_SERVER['PHP_AUTH_USER'] ?? ''; + $password = $_SERVER['PHP_AUTH_PW'] ?? ''; # Authenticate against the local user database and return the result return (bool) authenticate_user($this->username, $password); @@ -39,7 +39,7 @@ class BasicAuth extends Auth { * Checks if the remote client is requesting BasicAuth by checking for the necessary headers. */ public function is_requested(): bool { - return $_SERVER['PHP_AUTH_USER'] or $_SERVER['PHP_AUTH_PW']; + return $_SERVER['PHP_AUTH_USER'] and $_SERVER['PHP_AUTH_PW']; } /** diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Auth/JWTAuth.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Auth/JWTAuth.inc index b0c8743f9..9d3e3f2c7 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Auth/JWTAuth.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Auth/JWTAuth.inc @@ -28,7 +28,7 @@ class JWTAuth extends Auth { $decoded_jwt = RESTAPIJWT::decode($this->get_auth_key(identifier: 'bearer')); # Check that the JWT from our Authorization header is valid - if ($decoded_jwt) { + if ($decoded_jwt and !empty($decoded_jwt['data'])) { # Set the username of the authenticating client embedded in the decoded JWT data payload. $this->username = $decoded_jwt['data']; return true; @@ -36,4 +36,12 @@ class JWTAuth extends Auth { return false; } + + /** + * Checks if the client is requesting JWT authentication by detecting an Authorization: Bearer + * header. Credential validation is intentionally deferred to _authenticate(). + */ + public function is_requested(): bool { + return !empty($this->get_auth_key(identifier: 'bearer')); + } } diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Auth/KeyAuth.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Auth/KeyAuth.inc index 0f8aae1a3..7c5b7cfe1 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Auth/KeyAuth.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Auth/KeyAuth.inc @@ -21,6 +21,14 @@ class KeyAuth extends Auth { */ public array $security_scheme = ['type' => 'apiKey', 'in' => 'header', 'name' => 'x-api-key']; + /** + * Checks if the client is requesting key authentication by detecting the x-api-key header. + * Credential validation is intentionally deferred to _authenticate(). + */ + public function is_requested(): bool { + return !empty($_SERVER['HTTP_X_API_KEY'] ?? ''); + } + /** * Performs REST API key authentication and obtains the username of the user who owns the provided key. * @return bool Returns true if match for this client's key found a match stored in config, returns false diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/Auth.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/Auth.inc index 1afa99e30..565ecd779 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/Auth.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/Auth.inc @@ -195,6 +195,11 @@ class Auth { * @return bool `true` if the user is disabled, `false` if it is not. */ public static function is_user_enabled(string $username): bool { + # If username is somehow empty, consider them disabled + if (empty($username)) { + return false; + } + return User::query(name: $username, disabled: false)->exists(); } diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/Command.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/Command.inc index 3bed26144..09bb15b23 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/Command.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/Command.inc @@ -33,29 +33,119 @@ class Command { */ public bool $trim_whitespace = false; + /** + * @var bool $escape Whether to automatically escape each token in the command string before execution. + * Defaults to true. Set to false only when the caller has already performed its own escaping or when + * shell features such as pipes, redirects, or glob expansion are intentionally required. + */ + public bool $escape = true; + + /** + * @var string $pipe An optional command string to pipe the output of $command into. When set, the shell + * pipeline ` | ` is constructed. The pipe command is escaped independently according to + * $escape_pipe. Use this instead of embedding `|` in the raw $command string, which would be neutralised + * by auto-escaping. + */ + public string $pipe = ''; + + /** + * @var bool $escape_pipe Whether to automatically escape the tokens of the $pipe command string. Defaults + * to true. Set to false when the pipe command contains shell features (e.g. `grep "some pattern"`) that + * must be passed verbatim. + */ + public bool $escape_pipe = true; + /** * Defines the Command object including the shell command to execute and optional modifiers. Note: By default, * the command output will redirect stderr to stdout so error message will be included in the output. * @param string $command The shell command to execute. * @param bool $trim_whitespace Remove excessive whitespace from the command output. * @param string $redirect An optional shell redirect to append to the end of the $command. + * @param bool $escape Automatically escape each token in the command string before execution. Defaults to + * true. Set to false only when the caller has already performed its own escaping or when shell features + * such as pipes, redirects, or glob expansion are intentionally required. + * @param string $pipe An optional command string to pipe the output of $command into. Escaped independently + * according to $escape_pipe. + * @param bool $escape_pipe Whether to automatically escape the tokens of the $pipe command. Defaults to true. * @return Command Returns this object containing the results of the executed command. Note: the object returned * cannot be used to initiate new commands. A new Command object should be created for any additional commands. */ - public function __construct(string $command, bool $trim_whitespace = false, string $redirect = '2>&1') { + public function __construct( + string $command, + bool $trim_whitespace = false, + string $redirect = '2>&1', + bool $escape = true, + string $pipe = '', + bool $escape_pipe = true, + ) { $this->command = $command; $this->trim_whitespace = $trim_whitespace; $this->redirect = $redirect; + $this->escape = $escape; + $this->pipe = $pipe; + $this->escape_pipe = $escape_pipe; $this->run_command(); return $this; } + /** + * Parses a raw command string into an array of argument tokens, respecting single- and double-quoted + * substrings so that spaces inside quotes are not treated as delimiters. Surrounding quotes are stripped + * from each token so that the value can be re-escaped uniformly by escape_command(). + * @param string $command The raw command string to tokenize. + * @return array An ordered list of unquoted token strings. + */ + public static function tokenize_command(string $command): array { + $tokens = []; + + # Match single-quoted strings, double-quoted strings, or unquoted non-whitespace sequences + preg_match_all("/'[^']*'|\"[^\"]*\"|[^\s]+/", $command, $matches); + + foreach ($matches[0] as $token) { + # Strip surrounding single quotes + if (str_starts_with($token, "'") && str_ends_with($token, "'")) { + $tokens[] = substr($token, 1, -1); + } + # Strip surrounding double quotes + elseif (str_starts_with($token, '"') && str_ends_with($token, '"')) { + $tokens[] = substr($token, 1, -1); + } + # Unquoted token — use as-is + else { + $tokens[] = $token; + } + } + + return $tokens; + } + + /** + * Escapes a raw command string by tokenizing it (respecting quoted substrings) and applying + * escapeshellarg() to every token before re-joining with spaces. This prevents command injection + * regardless of whether the original string contained pre-quoted or unquoted arguments. + * @param string $command The raw command string to escape. + * @return string The fully escaped command string ready for exec(). + */ + public static function escape_command(string $command): string { + $tokens = self::tokenize_command($command); + $escaped = array_map('escapeshellarg', $tokens); + return implode(' ', $escaped); + } + /** * Executes the assigned $command. The $output and $result_code properties will be set after running * this method. */ private function run_command(): void { - exec(command: "$this->command $this->redirect", output: $raw_output, result_code: $this->result_code); + $safe_command = $this->escape ? self::escape_command($this->command) : $this->command; + + # Append a pipe stage when one is specified, escaping it independently + if ($this->pipe !== '') { + $safe_pipe = $this->escape_pipe ? self::escape_command($this->pipe) : $this->pipe; + $safe_command = "$safe_command | $safe_pipe"; + } + + exec(command: "$safe_command $this->redirect", output: $raw_output, result_code: $this->result_code); $this->output = implode(PHP_EOL, $raw_output); # Normalize output's whitespace if requested diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/Endpoint.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/Endpoint.inc index 15857358f..205f6a0e8 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/Endpoint.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/Endpoint.inc @@ -1299,7 +1299,7 @@ class Endpoint { } # Otherwise, delete a single object - $this->model->from_representation(data: $this->request_data); + $this->model->from_representation(id: $this->request_data['id'], parent_id: $this->request_data['parent_id']); return $this->model->delete(apply: $this->request_data['apply'] === true); } diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/TestCase.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/TestCase.inc index e87a2ac76..eab81197f 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/TestCase.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/TestCase.inc @@ -130,27 +130,6 @@ class TestCase { } } - /** - * Runs a shell command and returns its output and return code. - * @param string $command The command to execute. - * @param bool $trim_whitespace Remove excess whitespace from the command output. This is sometimes helpful when - * the output of commands that do not have consistent whitespace formatting. - * @return array An array where the `output` key contains the commands output and the `result_code` key contains the - * resulting result code of the command. - */ - function run_command(string $command, bool $trim_whitespace = false): array { - $results = ['output' => null, 'code' => null]; - exec(command: "$command 2>/dev/null", output: $results['output'], result_code: $results['result_code']); - $results['output'] = implode(PHP_EOL, $results['output']); - - # Normalize output's whitespace if requested - if ($trim_whitespace) { - $results['output'] = preg_replace('/\s+/', ' ', $results['output']); - } - - return $results; - } - /** * Sets up the test case before tests are run. This can be overridden by your TestCase to setup shared resources * required for your tests. diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Dispatchers/ACMECertificateIssueDispatcher.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Dispatchers/ACMECertificateIssueDispatcher.inc index 6d137577d..c36f2936a 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Dispatchers/ACMECertificateIssueDispatcher.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Dispatchers/ACMECertificateIssueDispatcher.inc @@ -2,6 +2,7 @@ namespace RESTAPI\Dispatchers; +use RESTAPI\Core\Command; use RESTAPI\Core\Dispatcher; use RESTAPI\Responses\ServerError; @@ -37,14 +38,23 @@ class ACMECertificateIssueDispatcher extends Dispatcher { result_log: '', ); - # pfSense prints the output of the acme.sh instead of returning it, start output buffering to capture it - ob_start(); + # Record the current byte size of the acme.sh issue log before issuance so we can + # extract only the new content appended during this run. + $log_file = "/tmp/acme/{$arguments['certificate']}/acme_issuecert.log"; + $pre_size = file_exists($log_file) ? filesize($log_file) : 0; # Issue the ACME certificate \pfsense_pkg\acme\issue_certificate(id: $arguments['certificate'], force: true, renew: false); - # Log the results of the ACME certificate issue - $result = ob_get_clean(); + # Extract only the bytes appended to the log since before the issuance started. + # tail -c +N outputs from byte N onward (1-indexed), so +($pre_size+1) skips + # all pre-existing content and returns only what was written during this run. + $start_byte = $pre_size + 1; + if (file_exists($log_file) && filesize($log_file) >= $start_byte) { + $result = (new Command("/usr/bin/tail -c +$start_byte $log_file"))->output; + } else { + $result = ''; + } # Replace the issuance result file $this->set_issuance_result( @@ -55,7 +65,7 @@ class ACMECertificateIssueDispatcher extends Dispatcher { ); # Wait a bit to ensure the issue log is written before proceeding - sleep(1); + sleep(5); } /** diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Dispatchers/ACMECertificateRenewDispatcher.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Dispatchers/ACMECertificateRenewDispatcher.inc index 66ed2b98c..cabfde331 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Dispatchers/ACMECertificateRenewDispatcher.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Dispatchers/ACMECertificateRenewDispatcher.inc @@ -2,6 +2,7 @@ namespace RESTAPI\Dispatchers; +use RESTAPI\Core\Command; use RESTAPI\Core\Dispatcher; use RESTAPI\Responses\ServerError; @@ -37,14 +38,23 @@ class ACMECertificateRenewDispatcher extends Dispatcher { result_log: '', ); - # pfSense prints the output of the acme.sh instead of returning it, start output buffering to capture it - ob_start(); + # Record the current byte size of the acme issue log before renewal so we can + # extract only the new content appended during this run. + $log_file = "/tmp/acme/{$arguments['certificate']}/acme_issuecert.log"; + $pre_size = file_exists($log_file) ? filesize($log_file) : 0; - # Issue the ACME certificate + # Renew the ACME certificate \pfsense_pkg\acme\issue_certificate(id: $arguments['certificate'], force: true, renew: true); - # Log the results of the ACME certificate renewal - $result = ob_get_clean(); + # Extract only the bytes appended to the log since before the renewal started. + # tail -c +N outputs from byte N onward (1-indexed), so +($pre_size+1) skips + # all pre-existing content and returns only what was written during this run. + $start_byte = $pre_size + 1; + if (file_exists($log_file) && filesize($log_file) >= $start_byte) { + $result = (new Command("/usr/bin/tail -c +$start_byte $log_file"))->output; + } else { + $result = ''; + } # Replace the renewal result file $this->set_renewal_result( diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Fields/StringField.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Fields/StringField.inc index 45565b28d..64ebf30b5 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Fields/StringField.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Fields/StringField.inc @@ -123,7 +123,8 @@ class StringField extends Field { internal_namespace: $internal_namespace, referenced_by: $referenced_by, conditions: $conditions, - validators: $validators + [ + validators: [ + ...$validators, new RESTAPI\Validators\LengthValidator(minimum: $minimum_length, maximum: $maximum_length), ], help_text: $help_text, diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/Certificate.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/Certificate.inc index 246c49487..93f5b512a 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/Certificate.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/Certificate.inc @@ -4,9 +4,9 @@ namespace RESTAPI\Models; use RESTAPI\Core\Model; use RESTAPI\Fields\Base64Field; -use RESTAPI\Fields\StringField; use RESTAPI\Fields\DateTimeField; use RESTAPI\Fields\IntegerField; +use RESTAPI\Fields\StringField; use RESTAPI\Fields\UIDField; use RESTAPI\Responses\ForbiddenError; use RESTAPI\Responses\ValidationError; diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/CertificateAuthority.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/CertificateAuthority.inc index 0436ee319..59270e336 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/CertificateAuthority.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/CertificateAuthority.inc @@ -5,7 +5,6 @@ namespace RESTAPI\Models; use RESTAPI\Core\Model; use RESTAPI\Fields\Base64Field; use RESTAPI\Fields\BooleanField; -use RESTAPI\Fields\ForeignModelField; use RESTAPI\Fields\IntegerField; use RESTAPI\Fields\StringField; use RESTAPI\Fields\UIDField; diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/CommandPrompt.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/CommandPrompt.inc index 7ffe497ac..ac28dd94d 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/CommandPrompt.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/CommandPrompt.inc @@ -43,7 +43,7 @@ class CommandPrompt extends Model { * Execute the requested command and populate the output and result code. */ public function _create(): void { - $cmd = new Command($this->command->value); + $cmd = new Command($this->command->value, escape: false); $this->output->value = $cmd->output; $this->result_code->value = $cmd->result_code; } diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/Enum.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/Enum.inc index c3ac252dc..5ac47a617 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/Enum.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/Enum.inc @@ -3,9 +3,7 @@ namespace RESTAPI\Models; use RESTAPI; -use RESTAPI\Core\Command; use RESTAPI\Core\Model; -use RESTAPI\Fields\BooleanField; use RESTAPI\Fields\ForeignModelField; use RESTAPI\Fields\ObjectField; use RESTAPI\Fields\StringField; diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/IPsecChildSAStatus.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/IPsecChildSAStatus.inc index cfeaedadf..4d45ebe41 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/IPsecChildSAStatus.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/IPsecChildSAStatus.inc @@ -5,8 +5,6 @@ namespace RESTAPI\Models; use RESTAPI\Core\Model; use RESTAPI\Fields\BooleanField; use RESTAPI\Fields\IntegerField; -use RESTAPI\Fields\NestedModelField; -use RESTAPI\Fields\PortField; use RESTAPI\Fields\StringField; /** diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/NetworkInterface.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/NetworkInterface.inc index 9c43f4df7..a9bc9b70f 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/NetworkInterface.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/NetworkInterface.inc @@ -2,6 +2,7 @@ namespace RESTAPI\Models; +use RESTAPI\Core\Command; use RESTAPI\Core\Model; use RESTAPI\Dispatchers\InterfaceApplyDispatcher; use RESTAPI\Fields\BooleanField; @@ -894,7 +895,8 @@ class NetworkInterface extends Model { $if = $this->if->value; # Run ifconfig to determine what media and media options are supported - exec("/sbin/ifconfig -m $if | grep \"media \"", $media_options_output); + $ifconfig_cmd = new Command(command: "/sbin/ifconfig -m $if", pipe: 'grep "media "', escape_pipe: false); + $media_options_output = explode(PHP_EOL, $ifconfig_cmd->output); # Loop through each supported media from the ifconfig output foreach ($media_options_output as $media_option) { diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/OpenVPNClient.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/OpenVPNClient.inc index b3590536f..c1f0e44b3 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/OpenVPNClient.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/OpenVPNClient.inc @@ -205,16 +205,17 @@ class OpenVPNClient extends Model { $this->auth_pass = new StringField( default: null, allow_null: true, - conditions: ['!auth_user' => null], + sensitive: true, verbose_name: 'Auth Pass', + conditions: ['!auth_user' => null], help_text: 'The password used to authenticate with the OpenVPN server.', ); $this->auth_retry_none = new BooleanField( default: false, indicates_true: 'yes', indicates_false: '', - internal_name: 'auth-retry-none', verbose_name: 'Auth Retry None', + internal_name: 'auth-retry-none', help_text: 'Disables retrying authentication if an authentication failed error is received from the server', ); $this->tls = new Base64Field( @@ -227,8 +228,8 @@ class OpenVPNClient extends Model { $this->tls_type = new StringField( required: true, choices: ['auth', 'crypt'], - conditions: ['!tls' => null], verbose_name: 'TLS Type', + conditions: ['!tls' => null], help_text: 'The TLS key usage type. In `auth` mode, the TLS key is used only as HMAC authentication for ' . 'the control channel, protecting the peers from unauthorized connections. The `crypt` mode encrypts ' . 'the control channel communication in addition to providing authentication, providing more privacy ' . @@ -237,8 +238,8 @@ class OpenVPNClient extends Model { $this->tlsauth_keydir = new StringField( default: 'default', choices: ['default', '0', '1', '2'], - conditions: ['!tls' => null], verbose_name: 'Tlsauth Keydir', + conditions: ['!tls' => null], help_text: 'The TLS key direction. This must be set to complementary values on the client and client. ' . 'For example, if the client is set to 0, the client must be set to 1. Both may be set to omit the ' . 'direction, in which case the TLS Key will be used bidirectionally.', @@ -288,23 +289,23 @@ class OpenVPNClient extends Model { $this->tunnel_network = new StringField( default: '', allow_empty: true, - validators: [new SubnetValidator(allow_ipv4: true, allow_ipv6: false, allow_alias: false)], verbose_name: 'Tunnel Network', + validators: [new SubnetValidator(allow_ipv4: true, allow_ipv6: false, allow_alias: false)], help_text: 'The IPv4 virtual network used for private communications between this client and client hosts.', ); $this->tunnel_networkv6 = new StringField( default: '', allow_empty: true, - validators: [new SubnetValidator(allow_ipv4: false, allow_ipv6: true, allow_alias: false)], verbose_name: 'IPv6 Tunnel Network', + validators: [new SubnetValidator(allow_ipv4: false, allow_ipv6: true, allow_alias: false)], help_text: 'The IPv6 virtual network used for private communications between this client and client hosts.', ); $this->remote_network = new StringField( default: [], allow_empty: true, many: true, - validators: [new SubnetValidator(allow_ipv4: true, allow_ipv6: false, allow_alias: true)], verbose_name: 'Remote Network', + validators: [new SubnetValidator(allow_ipv4: true, allow_ipv6: false, allow_alias: true)], help_text: 'IPv4 networks that will be routed through the tunnel, so that a site-to-site VPN can be ' . 'established without manually changing the routing tables. Expressed as a list of ' . 'one or more CIDR ranges or host/network type aliases. If this is a site-to-site VPN, enter the ' . @@ -314,8 +315,8 @@ class OpenVPNClient extends Model { default: [], allow_empty: true, many: true, - validators: [new SubnetValidator(allow_ipv4: false, allow_ipv6: true, allow_alias: true)], verbose_name: 'IPv6 Remote Network', + validators: [new SubnetValidator(allow_ipv4: false, allow_ipv6: true, allow_alias: true)], help_text: 'IPv6 networks that will be routed through the tunnel, so that a site-to-site VPN can be ' . 'established without manually changing the routing tables. Expressed as a list of ' . 'one or more CIDR ranges or host/network type aliases. If this is a site-to-site VPN, enter the ' . @@ -340,8 +341,8 @@ class OpenVPNClient extends Model { $this->topology = new StringField( default: 'subnet', choices: ['subnet', 'net30'], - conditions: ['dev_mode' => 'tun'], verbose_name: 'Topology', + conditions: ['dev_mode' => 'tun'], help_text: 'The method used to supply a virtual adapter IP address to clients when using TUN mode on IPv4.', ); $this->passtos = new BooleanField( @@ -386,34 +387,34 @@ class OpenVPNClient extends Model { ); $this->keepalive_interval = new IntegerField( default: 10, - conditions: ['ping_method' => 'keepalive'], verbose_name: 'Keepalive Interval', + conditions: ['ping_method' => 'keepalive'], help_text: 'The keepalive interval parameter.', ); $this->keepalive_timeout = new IntegerField( default: 60, - conditions: ['ping_method' => 'keepalive'], verbose_name: 'Keepalive Timeout', + conditions: ['ping_method' => 'keepalive'], help_text: 'The keepalive timeout parameter.', ); $this->ping_seconds = new IntegerField( default: 10, - conditions: ['ping_method' => 'ping'], verbose_name: 'Ping Seconds', + conditions: ['ping_method' => 'ping'], help_text: 'The number of seconds to accept no packets before sending a ping to the ' . 'remote peer over the TCP/UDP control channel.', ); $this->ping_action = new StringField( default: 'ping_restart', choices: ['ping_restart', 'ping_exit'], - conditions: ['ping_method' => 'ping'], verbose_name: 'Ping Action', + conditions: ['ping_method' => 'ping'], help_text: 'The action to take after a ping to the remote peer times-out.', ); $this->ping_action_seconds = new IntegerField( default: 60, - conditions: ['ping_method' => 'ping'], verbose_name: 'Ping Action Seconds', + conditions: ['ping_method' => 'ping'], help_text: 'The number of seconds that must elapse before the ping is considered a timeout and the ' . 'configured `ping_action` is performed.', ); diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/PortForward.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/PortForward.inc index ea47bb3e7..d7e73f69c 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/PortForward.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/PortForward.inc @@ -15,7 +15,6 @@ use RESTAPI\Fields\SpecialNetworkField; use RESTAPI\Fields\StringField; use RESTAPI\Fields\UnixTimeField; use RESTAPI\Responses\ServerError; -use RESTAPI\Validators\IPAddressValidator; /** * Defines a Model that represents port forward rules. diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/RESTAPIKey.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/RESTAPIKey.inc index 2375748d0..26bbcef6e 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/RESTAPIKey.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/RESTAPIKey.inc @@ -133,7 +133,7 @@ class RESTAPIKey extends Model { $hash = hash($this->hash_algo->value, $key); # Authentication is successful when incoming key matches the hashed key stored in config - if ($hash === $this->hash->value) { + if (hash_equals($hash, $this->hash->value)) { return true; } diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/RESTAPIVersion.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/RESTAPIVersion.inc index 97438805a..b3934894f 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/RESTAPIVersion.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/RESTAPIVersion.inc @@ -119,7 +119,7 @@ class RESTAPIVersion extends Model { */ public static function get_api_version(): string { # Pull the raw pkg info for the API package into an array for each line - $pkg_info = explode(PHP_EOL, shell_exec('pkg-static info pfSense-pkg-RESTAPI')); + $pkg_info = explode(PHP_EOL, (new Command('pkg-static info pfSense-pkg-RESTAPI'))->output); # Loop through each line and check the version foreach ($pkg_info as $pkg_line) { diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/SystemHalt.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/SystemHalt.inc index fae5f0681..3632700f5 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/SystemHalt.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/SystemHalt.inc @@ -4,7 +4,6 @@ namespace RESTAPI\Models; use RESTAPI\Core\Model; use RESTAPI\Dispatchers\SystemHaltDispatcher; -use RESTAPI\Fields\BooleanField; /** * Defines a Model that performs a system halt operation. diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/SystemReboot.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/SystemReboot.inc index b6cc5000b..a0e1fc783 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/SystemReboot.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/SystemReboot.inc @@ -4,7 +4,6 @@ namespace RESTAPI\Models; use RESTAPI\Core\Model; use RESTAPI\Dispatchers\SystemRebootDispatcher; -use RESTAPI\Fields\BooleanField; /** * Defines a Model that performs a system reboot operation. diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/SystemStatus.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/SystemStatus.inc index 676f1a4d7..04ee603e0 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/SystemStatus.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/SystemStatus.inc @@ -217,7 +217,11 @@ class SystemStatus extends Model { */ private function get_bios_info(string $bios_field): string { # Run the kenv command to obtain the requested BIOS information - $kenv = new Command(command: "/bin/kenv -q smbios.bios.$bios_field 2>/dev/null", trim_whitespace: true); + $kenv = new Command( + command: "/bin/kenv -q smbios.bios.$bios_field", + trim_whitespace: true, + redirect: '2>/dev/null', + ); return $kenv->result_code === 0 ? $kenv->output : ''; } diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/SystemUpdate.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/SystemUpdate.inc index 8b58836b2..bce0803f4 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/SystemUpdate.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/SystemUpdate.inc @@ -6,7 +6,6 @@ require_once 'RESTAPI/autoloader.inc'; use RESTAPI\Core\Model; use RESTAPI\Dispatchers\SystemUpdateDispatcher; -use RESTAPI\Fields\BooleanField; /** * Defines a Model that performs a pfSense update operation. diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/User.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/User.inc index 4a308f740..51ab37d38 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/User.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/User.inc @@ -41,17 +41,17 @@ class User extends Model { required: true, unique: true, maximum_length: 32, + verbose_name: 'Name', validators: [ new RegexValidator(pattern: "/^[a-zA-Z0-9\.\-_]+$/", error_msg: 'Value contains invalid characters.'), ], - verbose_name: 'Name', help_text: 'The username of this local user.', ); $this->password = new StringField( required: true, sensitive: true, - internal_name: $this->get_config('system/webgui/pwhash', 'bcrypt') . '-hash', verbose_name: 'Password', + internal_name: $this->get_config('system/webgui/pwhash', 'bcrypt') . '-hash', help_text: 'The password of this local user.', ); $this->uid = new IntegerField( @@ -115,10 +115,11 @@ class User extends Model { $this->ipsecpsk = new StringField( default: '', allow_empty: true, + sensitive: true, + verbose_name: 'IPsec Pre-Shared Key', validators: [ new RegexValidator(pattern: "/^[[:ascii:]]*$/", error_msg: 'Value contains invalid characters.'), ], - verbose_name: 'IPsec Pre-Shared Key', help_text: 'The IPsec pre-shared key to assign this user.', ); parent::__construct($id, $parent_id, $data, ...$options); diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/WireGuardPeer.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/WireGuardPeer.inc index f983e1467..ecd73eb60 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/WireGuardPeer.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/WireGuardPeer.inc @@ -55,14 +55,14 @@ class WireGuardPeer extends Model { $this->endpoint = new StringField( default: null, allow_null: true, - validators: [new IPAddressValidator(allow_ipv4: true, allow_ipv6: true, allow_fqdn: true)], verbose_name: 'Endpoint', + validators: [new IPAddressValidator(allow_ipv4: true, allow_ipv6: true, allow_fqdn: true)], help_text: 'The IP address or hostname of the remote peer. Set to `null` to make this a dynamic endpoint.', ); $this->port = new PortField( default: '51820', - conditions: ['!endpoint' => null], verbose_name: 'Port', + conditions: ['!endpoint' => null], help_text: 'The port used by the remote peer.', ); $this->descr = new StringField( @@ -88,7 +88,7 @@ class WireGuardPeer extends Model { default: '', allow_empty: true, allow_null: true, - write_only: true, + sensitive: true, verbose_name: 'Pre-Shared Key', help_text: 'The pre-shared key for this tunnel.', ); diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIAuthBasicAuthTestCase.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIAuthBasicAuthTestCase.inc index 998b2526f..f74805518 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIAuthBasicAuthTestCase.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIAuthBasicAuthTestCase.inc @@ -33,14 +33,14 @@ class APIAuthBasicAuthTestCase extends TestCase { $auth = new BasicAuth(); $this->assert_is_false($auth->is_requested()); - # Ensure is_requested() returns true if either a basic username or password are specified + # Ensure is_requested() returns false if either a basic username or password are specified $_SERVER['PHP_AUTH_USER'] = 'admin'; $_SERVER['PHP_AUTH_PW'] = null; $auth = new BasicAuth(); - $this->assert_is_true($auth->is_requested()); + $this->assert_is_false($auth->is_requested()); $_SERVER['PHP_AUTH_USER'] = null; $_SERVER['PHP_AUTH_PW'] = 'pfsense'; - $this->assert_is_true($auth->is_requested()); + $this->assert_is_false($auth->is_requested()); $_SERVER['PHP_AUTH_USER'] = 'admin'; $_SERVER['PHP_AUTH_PW'] = 'pfsense'; $this->assert_is_true($auth->is_requested()); diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APICoreCommandTestCase.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APICoreCommandTestCase.inc index 40829a406..8775556f3 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APICoreCommandTestCase.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APICoreCommandTestCase.inc @@ -2,23 +2,290 @@ namespace RESTAPI\Tests; +require_once 'RESTAPI/autoloader.inc'; + use RESTAPI\Core\Command; use RESTAPI\Core\TestCase; class APICoreCommandTestCase extends TestCase { /** * Checks that the Command object successfully runs the requested command and returns the correct output and - * result code. + * result code. Escaping is disabled here because these commands rely on raw shell behaviour (e.g. a bare + * unrecognised token and a pre-quoted echo argument). */ - public function test_command() { + public function test_command(): void { # Run a command that doesn't exist, ensure it's output and result code are expected - $cmd = new Command('asdf'); + $cmd = new Command('asdf', escape: false); $this->assert_equals($cmd->output, 'sh: asdf: not found'); $this->assert_equals($cmd->result_code, 127); # Run an echo command, ensure it's output and result code are expected - $cmd = new Command("echo 'test'"); + $cmd = new Command("echo 'test'", escape: false); $this->assert_equals($cmd->output, 'test'); $this->assert_equals($cmd->result_code, 0); } + + /** + * Checks that escape defaults to true when not explicitly specified. + */ + public function test_escape_defaults_to_true(): void { + $cmd = new Command('echo hello'); + $this->assert_is_true($cmd->escape); + } + + /** + * Checks that escape can be explicitly disabled via named argument. + */ + public function test_escape_can_be_disabled(): void { + $cmd = new Command('echo hello', escape: false); + $this->assert_is_false($cmd->escape); + } + + /** + * Checks that a simple auto-escaped command runs successfully and returns expected output. + */ + public function test_auto_escape_basic_execution(): void { + $cmd = new Command('echo hello'); + $this->assert_equals($cmd->output, 'hello'); + $this->assert_equals($cmd->result_code, 0); + } + + /** + * Checks that a Command with escape disabled (false) allows compound shell expressions to run. + */ + public function test_escape_false_allows_raw_shell_string(): void { + $cmd = new Command('echo foo && echo bar', escape: false); + $this->assert_str_contains($cmd->output, 'foo'); + $this->assert_str_contains($cmd->output, 'bar'); + $this->assert_equals($cmd->result_code, 0); + } + + /** + * Checks that tokenize_command splits a simple unquoted command into the expected tokens. + */ + public function test_tokenize_command_simple(): void { + $tokens = Command::tokenize_command('/sbin/ifconfig em0 -group mygroup'); + $this->assert_equals($tokens, ['/sbin/ifconfig', 'em0', '-group', 'mygroup']); + } + + /** + * Checks that tokenize_command returns an empty array for an empty string. + */ + public function test_tokenize_command_empty_string(): void { + $tokens = Command::tokenize_command(''); + $this->assert_equals($tokens, []); + } + + /** + * Checks that tokenize_command preserves a space inside single-quoted substrings as one token + * and strips the surrounding quotes. + */ + public function test_tokenize_command_single_quoted_token(): void { + $tokens = Command::tokenize_command("echo 'hello world'"); + $this->assert_equals($tokens, ['echo', 'hello world']); + } + + /** + * Checks that tokenize_command preserves a space inside double-quoted substrings as one token + * and strips the surrounding quotes. + */ + public function test_tokenize_command_double_quoted_token(): void { + $tokens = Command::tokenize_command('echo "hello world"'); + $this->assert_equals($tokens, ['echo', 'hello world']); + } + + /** + * Checks that tokenize_command handles a mix of quoted and unquoted tokens in one string. + */ + public function test_tokenize_command_mixed_quoted_and_unquoted(): void { + $tokens = Command::tokenize_command('/bin/cmd --flag "value with spaces" bare'); + $this->assert_equals($tokens, ['/bin/cmd', '--flag', 'value with spaces', 'bare']); + } + + /** + * Checks that escape_command wraps each unquoted token with escapeshellarg() single-quote quoting. + */ + public function test_escape_command_wraps_tokens_in_single_quotes(): void { + $escaped = Command::escape_command('/sbin/ifconfig em0'); + $this->assert_equals($escaped, "'/sbin/ifconfig' 'em0'"); + } + + /** + * Checks that escape_command re-escapes a token that was already single-quoted in the input. + */ + public function test_escape_command_re_escapes_pre_single_quoted_token(): void { + $escaped = Command::escape_command("/sbin/ifconfig em0 -group 'mygroup'"); + $this->assert_equals($escaped, "'/sbin/ifconfig' 'em0' '-group' 'mygroup'"); + } + + /** + * Checks that escape_command re-escapes a token that was already double-quoted in the input. + */ + public function test_escape_command_re_escapes_pre_double_quoted_token(): void { + $escaped = Command::escape_command('/sbin/ifconfig em0 -group "mygroup"'); + $this->assert_equals($escaped, "'/sbin/ifconfig' 'em0' '-group' 'mygroup'"); + } + + /** + * Checks that escape_command neutralizes a semicolon injection payload so it cannot act as a + * shell command separator — the semicolon is present but enclosed inside a single-quoted token. + */ + public function test_escape_command_neutralizes_semicolon_injection(): void { + $escaped = Command::escape_command('/bin/cmd arg; rm -rf /'); + # The semicolon must still be present (not stripped), but enclosed in quotes + $this->assert_str_contains($escaped, "'arg;'"); + } + + /** + * Checks that escape_command neutralizes $(...) command substitution syntax — the $( sequence + * is present but enclosed inside a single-quoted token where the shell cannot evaluate it. + */ + public function test_escape_command_neutralizes_command_substitution(): void { + $escaped = Command::escape_command('/bin/cmd $(evil)'); + $this->assert_str_contains($escaped, "'$(evil)'"); + } + + /** + * Checks that escape_command neutralizes backtick command substitution syntax — the backticks + * are present but enclosed inside a single-quoted token. + */ + public function test_escape_command_neutralizes_backtick_substitution(): void { + $escaped = Command::escape_command('/bin/cmd `evil`'); + $this->assert_str_contains($escaped, "'`evil`'"); + } + + /** + * Checks that escape_command neutralizes a pipe character so it cannot chain commands — + * the pipe is present but enclosed inside a single-quoted token. + */ + public function test_escape_command_neutralizes_pipe(): void { + $escaped = Command::escape_command('/bin/cmd arg | cat /etc/passwd'); + $this->assert_str_contains($escaped, "'|'"); + } + + /** + * Checks that escape_command neutralizes an ampersand so it cannot background or chain commands — + * the && sequence is present but enclosed inside a single-quoted token. + */ + public function test_escape_command_neutralizes_ampersand(): void { + $escaped = Command::escape_command('/bin/cmd arg && evil'); + $this->assert_str_contains($escaped, "'&&'"); + } + + /** + * Checks that auto-escaping prevents injection via a malicious value interpolated into the + * command string — the injected shell code must not be executed. + */ + public function test_auto_escape_prevents_injection_via_interpolated_value(): void { + $sentinel = '/tmp/restapi_cmd_injection_test_' . getmypid(); + + # Simulate a field value that contains an injection payload + $malicious_value = "em0; touch $sentinel"; + new Command("/sbin/ifconfig $malicious_value -group testgroup"); + + $this->assert_is_false(file_exists($sentinel)); + } + + /** + * Checks that auto-escaping correctly handles a token whose inner value contains a single quote, + * producing a safely escaped shell argument rather than a broken quoting sequence. + */ + public function test_auto_escape_handles_embedded_single_quote_in_value(): void { + # escapeshellarg() should encode the apostrophe safely + $escaped = Command::escape_command("/bin/echo it's"); + $this->assert_is_true(strlen($escaped) > 0); + + # The shell must not crash and must exit 0 + $cmd = new Command("/bin/echo it's"); + $this->assert_equals($cmd->result_code, 0); + } + + /** + * Checks that auto-escaping correctly handles a token whose inner value contains a double quote. + */ + public function test_auto_escape_handles_embedded_double_quote_in_value(): void { + $cmd = new Command('/bin/echo say "hello"'); + $this->assert_equals($cmd->result_code, 0); + } + + /** + * Checks that trim_whitespace still works correctly when escape is disabled. + */ + public function test_trim_whitespace_with_escape_disabled(): void { + $cmd = new Command('printf "foo bar"', trim_whitespace: true, escape: false); + $this->assert_equals($cmd->output, 'foo bar'); + } + + /** + * Checks that a basic pipe is constructed and both commands run — output of the first feeds the second. + */ + public function test_pipe_basic(): void { + # echo three lines; grep filters to only lines containing 'keep' + $cmd = new Command(command: 'printf "keep\nskip\nkeep2"', escape: false, pipe: 'grep keep', escape_pipe: false); + $this->assert_str_contains($cmd->output, 'keep'); + $this->assert_str_does_not_contain($cmd->output, 'skip'); + } + + /** + * Checks that the pipe command receives the correct output when the primary command is auto-escaped. + */ + public function test_pipe_with_escaped_primary_command(): void { + $cmd = new Command(command: 'echo hello', pipe: 'grep hello', escape_pipe: false); + $this->assert_str_contains($cmd->output, 'hello'); + $this->assert_equals($cmd->result_code, 0); + } + + /** + * Checks that the pipe stage itself is auto-escaped when escape_pipe is true (default), preventing + * injection through the pipe argument. + */ + public function test_pipe_escape_pipe_default_true(): void { + $sentinel = '/tmp/restapi_pipe_injection_test_' . getmypid(); + + # Attempt injection through the pipe string — if escape_pipe works, touch must not run + $cmd = new Command(command: 'echo hello', pipe: "grep hello; touch $sentinel", escape_pipe: true); + + $this->assert_is_false(file_exists($sentinel)); + } + + /** + * Checks that escape_pipe defaults to true when not explicitly specified. + */ + public function test_pipe_escape_pipe_defaults_to_true(): void { + $cmd = new Command('echo hello', pipe: 'grep hello'); + $this->assert_is_true($cmd->escape_pipe); + } + + /** + * Checks that escape_pipe can be set to false, allowing raw shell syntax in the pipe stage. + */ + public function test_pipe_escape_pipe_false_allows_raw_syntax(): void { + # grep with a quoted pattern only works when escape_pipe is false + $cmd = new Command( + command: 'printf "hello world\ngoodbye"', + escape: false, + pipe: 'grep "hello world"', + escape_pipe: false, + ); + $this->assert_str_contains($cmd->output, 'hello world'); + $this->assert_str_does_not_contain($cmd->output, 'goodbye'); + } + + /** + * Checks that an empty pipe string does not alter the command — no pipe is inserted. + */ + public function test_pipe_empty_string_does_not_pipe(): void { + $cmd = new Command('echo hello', pipe: ''); + $this->assert_equals($cmd->output, 'hello'); + $this->assert_equals($cmd->result_code, 0); + } + + /** + * Checks that the $pipe property is stored correctly on the object. + */ + public function test_pipe_property_stored(): void { + $cmd = new Command('echo hello', pipe: 'grep hello', escape_pipe: false); + $this->assert_equals($cmd->pipe, 'grep hello'); + $this->assert_is_false($cmd->escape_pipe); + } } diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelTraitsLogFileModelTraitsTestCase.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelTraitsLogFileModelTraitsTestCase.inc index 4ccdb2d3b..cb9851d34 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelTraitsLogFileModelTraitsTestCase.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelTraitsLogFileModelTraitsTestCase.inc @@ -158,8 +158,8 @@ class APIModelTraitsLogFileModelTraitsTestCase extends TestCase { # Create some mock log files that are xz compressed file_put_contents('/tmp/test_xz.log', "Line 7\nLine 8\nLine 9\n"); - new Command('printf "Line 4\nLine 5\nLine 6\n" | xz -c > /tmp/test_xz.log.0.xz'); - new Command('printf "Line 1\nLine 2\nLine 3\n" | xz -c > /tmp/test_xz.log.1.xz'); + new Command(command: 'printf "Line 4\nLine 5\nLine 6\n"', redirect: '> /tmp/test_xz.log.0.xz', pipe: 'xz -c'); + new Command(command: 'printf "Line 1\nLine 2\nLine 3\n"', redirect: '> /tmp/test_xz.log.1.xz', pipe: 'xz -c'); # Read the system log file $log = $model->read_log('/tmp/test_xz.log'); diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsDHCPRelayTestCase.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsDHCPRelayTestCase.inc index 1f723fd7b..efdb783d7 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsDHCPRelayTestCase.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsDHCPRelayTestCase.inc @@ -85,7 +85,7 @@ class APIModelsDHCPRelayTestCase extends TestCase { $dhcp_relay->update(); # Ensure the DHCP relay service is running with the correct arguments - $dhcrelay_process = new Command('ps aux | grep dhcrelay'); + $dhcrelay_process = new Command('ps aux', pipe: 'grep dhcrelay'); $lan_if = $this->env['PFREST_LAN_IF']; $wan_if = $this->env['PFREST_WAN_IF']; $this->assert_str_contains( diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsFirewallAliasTestCase.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsFirewallAliasTestCase.inc index 94a984058..fed1388c5 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsFirewallAliasTestCase.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsFirewallAliasTestCase.inc @@ -2,6 +2,7 @@ namespace RESTAPI\Tests; +use RESTAPI\Core\Command; use RESTAPI\Core\TestCase; use RESTAPI\Core\TestCaseRetry; use RESTAPI\Models\FirewallAlias; @@ -25,8 +26,8 @@ class APIModelsFirewallAliasTestCase extends TestCase { # Wait up to 30 seconds for the filter to reload and the table to be create foreach (range(0, 30) as $attempt) { # Check pfctl for the table - $pfctl_output = shell_exec('pfctl -t TEST_GOOGLE_DNS -Ts'); - if ($pfctl_output) { + $pfctl_ret = new Command('pfctl -t TEST_GOOGLE_DNS -Ts'); + if ($pfctl_ret->output and $pfctl_ret->result_code == 0) { break; } @@ -35,8 +36,8 @@ class APIModelsFirewallAliasTestCase extends TestCase { } # Ensure expected IPs were resolved and stored in a pfctl with the same name as our alias - $this->assert_str_contains($pfctl_output, '8.8.8.8'); - $this->assert_str_contains($pfctl_output, '8.8.4.4'); + $this->assert_str_contains($pfctl_ret->output, '8.8.8.8'); + $this->assert_str_contains($pfctl_ret->output, '8.8.4.4'); # Delete the alias $test_alias->delete(apply: true); diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsInterfaceVLANTestCase.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsInterfaceVLANTestCase.inc index da9b25dde..92043bbcb 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsInterfaceVLANTestCase.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsInterfaceVLANTestCase.inc @@ -2,6 +2,7 @@ namespace RESTAPI\Tests; +use RESTAPI\Core\Command; use RESTAPI\Core\TestCase; use RESTAPI\Models\InterfaceVLAN; @@ -21,9 +22,9 @@ class APIModelsInterfaceVLANTestCase extends TestCase { $test_vlan->create(); # Ensure the newly created VLAN is correctly configuring by checking ifconfig - $ifconfig_output = shell_exec("ifconfig {$this->env['PFREST_LAN_IF']}.2"); - $this->assert_str_contains($ifconfig_output, 'vlan: 2'); - $this->assert_str_contains($ifconfig_output, 'vlanpcp: 4'); + $ifconfig_cmd = new Command("ifconfig {$this->env['PFREST_LAN_IF']}.2"); + $this->assert_str_contains($ifconfig_cmd->output, 'vlan: 2'); + $this->assert_str_contains($ifconfig_cmd->output, 'vlanpcp: 4'); # Update the VLAN interface to change the tag and pcp values $test_vlan->tag->value = 55; @@ -31,20 +32,20 @@ class APIModelsInterfaceVLANTestCase extends TestCase { $test_vlan->update(); # Ensure the old VLAN is no longer present - $ifconfig_output = shell_exec("ifconfig {$this->env['PFREST_LAN_IF']}.2"); - $this->assert_equals($ifconfig_output, null); + $ifconfig_cmd = new Command("ifconfig {$this->env['PFREST_LAN_IF']}.2"); + $this->assert_not_equals($ifconfig_cmd->result_code, 0); # Ensure the new VLAN is present - $ifconfig_output = shell_exec("ifconfig {$this->env['PFREST_LAN_IF']}.55"); - $this->assert_str_contains($ifconfig_output, 'vlan: 55'); - $this->assert_str_contains($ifconfig_output, 'vlanpcp: 7'); + $ifconfig_cmd = new Command("ifconfig {$this->env['PFREST_LAN_IF']}.55"); + $this->assert_str_contains($ifconfig_cmd->output, 'vlan: 55'); + $this->assert_str_contains($ifconfig_cmd->output, 'vlanpcp: 7'); # Delete the VLAN interface $test_vlan->delete(); # Ensure the VLAN is no longer present - $ifconfig_output = shell_exec("ifconfig {$this->env['PFREST_LAN_IF']}.55"); - $this->assert_equals($ifconfig_output, null); + $ifconfig_cmd = new Command("ifconfig {$this->env['PFREST_LAN_IF']}.55"); + $this->assert_not_equals($ifconfig_cmd->result_code, 0); } /** diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsNetworkInterfaceTestCase.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsNetworkInterfaceTestCase.inc index 6e162f1e3..ea6bf6c79 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsNetworkInterfaceTestCase.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsNetworkInterfaceTestCase.inc @@ -2,6 +2,7 @@ namespace RESTAPI\Tests; +use RESTAPI\Core\Command; use RESTAPI\Core\Model; use RESTAPI\Core\TestCase; use RESTAPI\Models\DHCPServer; @@ -678,7 +679,12 @@ class APIModelsNetworkInterfaceTestCase extends TestCase { $test_model->if->value = $this->env['PFREST_WAN_IF']; # Run ifconfig to gather interface details - exec("/sbin/ifconfig -m {$test_model->if->value} | grep \"media \"", $ifconfig_output); + $ifconfig_cmd = new Command( + command: "/sbin/ifconfig -m {$test_model->if->value}", + pipe: 'grep "media "', + escape_pipe: false, + ); + $ifconfig_output = explode(PHP_EOL, $ifconfig_cmd->output); # Loop through each line in the ifconfig output and ensure it is found in the `get_supported_media()` output foreach ($ifconfig_output as $media_line) { @@ -803,8 +809,7 @@ class APIModelsNetworkInterfaceTestCase extends TestCase { $static_if->create(apply: true); # Run ifconfig to check the configured interface's values - exec("ifconfig {$this->env['PFREST_OPT1_IF']}", $ifconfig_output); - $ifconfig_output = implode(PHP_EOL, $ifconfig_output); + $ifconfig_output = (new Command("ifconfig {$this->env['PFREST_OPT1_IF']}"))->output; # Ensure the interface's mtu is shown by ifconfig $this->assert_str_contains($ifconfig_output, 'mtu 1501'); @@ -832,8 +837,7 @@ class APIModelsNetworkInterfaceTestCase extends TestCase { $static_if->update(apply: true); # Run ifconfig to check the configured interface's new values - exec("ifconfig {$this->env['PFREST_OPT1_IF']}", $ifconfig_output); - $ifconfig_output = implode(PHP_EOL, $ifconfig_output); + $ifconfig_output = (new Command("ifconfig {$this->env['PFREST_OPT1_IF']}"))->output; # Ensure the interface's new mtu is shown by ifconfig $this->assert_str_contains($ifconfig_output, 'mtu 1500'); @@ -850,8 +854,7 @@ class APIModelsNetworkInterfaceTestCase extends TestCase { $static_if->delete(); # Run ifconfig to check the configured interface's deleted values - exec("ifconfig {$this->env['PFREST_OPT1_IF']}", $ifconfig_output); - $ifconfig_output = implode(PHP_EOL, $ifconfig_output); + $ifconfig_output = (new Command("ifconfig {$this->env['PFREST_OPT1_IF']}"))->output; # Ensure the new static IPv4 address is not shown by ifconfig $this->assert_str_does_not_contain( diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsRoutingGatewayTestCase.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsRoutingGatewayTestCase.inc index 68710f93a..abfd5a77d 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsRoutingGatewayTestCase.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsRoutingGatewayTestCase.inc @@ -2,6 +2,7 @@ namespace RESTAPI\Tests; +use RESTAPI\Core\Command; use RESTAPI\Core\TestCase; use RESTAPI\Models\NetworkInterface; use RESTAPI\Models\RoutingGateway; @@ -400,19 +401,19 @@ class APIModelsRoutingGatewayTestCase extends TestCase { $test_gw->create(apply: true); # Ensure it is now found in the routing table - $netstat_output = $this->run_command('netstat -rn')['output']; + $netstat_output = (new Command('netstat -rn'))->output; $this->assert_str_contains($netstat_output, '1.2.3.4'); # Update the RoutingGateway's IP and ensure it is now present in the routing table and 1.2.3.4 is not $test_gw->gateway->value = '4.3.2.1'; $test_gw->update(apply: true); - $netstat_output = $this->run_command('netstat -rn')['output']; + $netstat_output = (new Command('netstat -rn'))->output; $this->assert_str_does_not_contain($netstat_output, '1.2.3.4'); $this->assert_str_contains($netstat_output, '4.3.2.1'); # Delete the RoutingGateway and ensure 4.3.2.1 is no longer in the routing table $test_gw->delete(apply: true); - $netstat_output = $this->run_command('netstat -rn')['output']; + $netstat_output = (new Command('netstat -rn'))->output; $this->assert_str_does_not_contain($netstat_output, '4.3.2.1'); } } diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsStaticRouteTestCase.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsStaticRouteTestCase.inc index 5914d1b7b..4f8b30bda 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsStaticRouteTestCase.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsStaticRouteTestCase.inc @@ -2,6 +2,7 @@ namespace RESTAPI\Tests; +use RESTAPI\Core\Command; use RESTAPI\Core\TestCase; use RESTAPI\Models\FirewallAlias; use RESTAPI\Models\NetworkInterface; @@ -312,7 +313,7 @@ class APIModelsStaticRouteTestCase extends TestCase { # Ensure the route is found in the routing table after applying $this->assert_str_contains( - $this->run_command('netstat -rn', trim_whitespace: true)['output'], + (new Command('netstat -rn', trim_whitespace: true))->output, "{$test_route->network->value} {$test_gw->gateway->value}", ); @@ -322,11 +323,11 @@ class APIModelsStaticRouteTestCase extends TestCase { # Ensure the new route is found in the routing table and the old one is not after applying $this->assert_str_does_not_contain( - $this->run_command('netstat -rn', trim_whitespace: true)['output'], + (new Command('netstat -rn', trim_whitespace: true))->output, "1.2.3.0/24 {$test_gw->gateway->value}", ); $this->assert_str_contains( - $this->run_command('netstat -rn', trim_whitespace: true)['output'], + (new Command('netstat -rn', trim_whitespace: true))->output, "{$test_route->network->value} {$test_gw->gateway->value}", ); @@ -334,7 +335,7 @@ class APIModelsStaticRouteTestCase extends TestCase { $test_route->delete(apply: true); $this->assert_str_does_not_contain( - $this->run_command('netstat -rn', trim_whitespace: true)['output'], + (new Command('netstat -rn', trim_whitespace: true))->output, "{$test_route->network->value} {$test_gw->gateway->value}", ); diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsSystemStatusTestCase.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsSystemStatusTestCase.inc index c206d8a75..351bc5a4e 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsSystemStatusTestCase.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsSystemStatusTestCase.inc @@ -34,15 +34,18 @@ class APIModelsSystemStatusTestCase extends TestCase { $this->assert_is_less_than_or_equal($system_status->disk_usage->value, 100); $this->assert_equals( $system_status->bios_vendor->value, - (new Command(command: '/bin/kenv -q smbios.bios.vendor 2>/dev/null', trim_whitespace: true))->output, + (new Command(command: '/bin/kenv -q smbios.bios.vendor', trim_whitespace: true, redirect: '2>/dev/null')) + ->output, ); $this->assert_equals( $system_status->bios_version->value, - (new Command(command: '/bin/kenv -q smbios.bios.version 2>/dev/null', trim_whitespace: true))->output, + (new Command(command: '/bin/kenv -q smbios.bios.version', trim_whitespace: true, redirect: '2>/dev/null')) + ->output, ); $this->assert_equals( $system_status->bios_date->value, - (new Command(command: '/bin/kenv -q smbios.bios.reldate 2>/dev/null', trim_whitespace: true))->output, + (new Command(command: '/bin/kenv -q smbios.bios.reldate', trim_whitespace: true, redirect: '2>/dev/null')) + ->output, ); } diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIValidatorsFilterNameValidatorTestCase.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIValidatorsFilterNameValidatorTestCase.inc index f244458e7..68de5f2b0 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIValidatorsFilterNameValidatorTestCase.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIValidatorsFilterNameValidatorTestCase.inc @@ -2,20 +2,132 @@ namespace RESTAPI\Tests; +require_once 'RESTAPI/autoloader.inc'; + use RESTAPI\Core\TestCase; use RESTAPI\Validators\FilterNameValidator; class APIValidatorsFilterNameValidatorTestCase extends TestCase { + /** + * Checks that a valid filter name consisting only of A-Z, a-z, 0-9, and _ passes validation. + */ + public function test_valid_name_passes(): void { + $this->assert_does_not_throw( + callable: function () { + $v = new FilterNameValidator(); + $v->validate('valid_Name123'); + }, + ); + } + + /** + * Checks that a name containing a hyphen is rejected as invalid. + */ + public function test_rejects_hyphen(): void { + $this->assert_throws_response( + response_id: 'FILTER_NAME_VALIDATOR_INVALID_CHARACTERS', + code: 400, + callable: function () { + $v = new FilterNameValidator(); + $v->validate('bad-name'); + }, + ); + } + + /** + * Checks that a name containing a space is rejected as invalid. + */ + public function test_rejects_space(): void { + $this->assert_throws_response( + response_id: 'FILTER_NAME_VALIDATOR_INVALID_CHARACTERS', + code: 400, + callable: function () { + $v = new FilterNameValidator(); + $v->validate('bad name'); + }, + ); + } + + /** + * Checks that a name containing a dot is rejected as invalid. + */ + public function test_rejects_dot(): void { + $this->assert_throws_response( + response_id: 'FILTER_NAME_VALIDATOR_INVALID_CHARACTERS', + code: 400, + callable: function () { + $v = new FilterNameValidator(); + $v->validate('bad.name'); + }, + ); + } + + /** + * Checks that a name containing a slash is rejected as invalid. + */ + public function test_rejects_slash(): void { + $this->assert_throws_response( + response_id: 'FILTER_NAME_VALIDATOR_INVALID_CHARACTERS', + code: 400, + callable: function () { + $v = new FilterNameValidator(); + $v->validate('bad/name'); + }, + ); + } + + /** + * Checks that a name containing a shell metacharacter (semicolon) is rejected as invalid. + */ + public function test_rejects_semicolon(): void { + $this->assert_throws_response( + response_id: 'FILTER_NAME_VALIDATOR_INVALID_CHARACTERS', + code: 400, + callable: function () { + $v = new FilterNameValidator(); + $v->validate('bad;name'); + }, + ); + } + + /** + * Checks that a name containing an at-sign is rejected as invalid. + */ + public function test_rejects_at_sign(): void { + $this->assert_throws_response( + response_id: 'FILTER_NAME_VALIDATOR_INVALID_CHARACTERS', + code: 400, + callable: function () { + $v = new FilterNameValidator(); + $v->validate('bad@name'); + }, + ); + } + + /** + * Checks that an empty string is rejected as invalid (no allowed characters present). + */ + public function test_rejects_empty_string(): void { + $this->assert_throws_response( + response_id: 'FILTER_NAME_VALIDATOR_INVALID_CHARACTERS', + code: 400, + callable: function () { + $v = new FilterNameValidator(); + $v->validate(''); + }, + ); + } + /** * Checks that system reserved names are not allowed as filter names. */ - public function test_cannot_use_reserved_names() { + public function test_cannot_use_reserved_names(): void { $this->assert_throws_response( response_id: 'FILTER_NAME_VALIDATOR_RESERVED_NAME_USED', code: 400, callable: function () { - $test_validator = new FilterNameValidator(); - $test_validator->validate('sshguard'); + $v = new FilterNameValidator(); + $v->validate('sshguard'); }, ); } @@ -23,30 +135,59 @@ class APIValidatorsFilterNameValidatorTestCase extends TestCase { /** * Checks that filter names cannot begin with `pkg_`. */ - public function test_cannot_start_with_pkg() { + public function test_cannot_start_with_pkg(): void { $this->assert_throws_response( response_id: 'FILTER_NAME_VALIDATOR_CANNOT_START_WITH_PKG', code: 400, callable: function () { - $test_validator = new FilterNameValidator(); - $test_validator->validate('pkg_test'); + $v = new FilterNameValidator(); + $v->validate('pkg_test'); }, ); } /** - * Checks that filter names cannot be entirely numeric. + * Checks that a filter name that matches an existing network interface is rejected. */ - public function test_cannot_be_pfsense_interface_id() { + public function test_cannot_be_pfsense_interface_id(): void { $this->assert_throws_response( response_id: 'FILTER_NAME_VALIDATOR_NAME_IN_USE_BY_INTERFACE', code: 400, callable: function () { - $test_validator = new FilterNameValidator(); - $test_validator->validate('wan'); + $v = new FilterNameValidator(); + $v->validate('wan'); + }, + ); + } + + /** + * Checks that a filter name that is entirely numerical is rejected. + */ + public function test_cannot_be_entirely_numerical(): void { + $this->assert_throws_response( + response_id: 'FILTER_NAME_VALIDATOR_CANNOT_BE_NUMERICAL', + code: 400, + callable: function () { + $v = new FilterNameValidator(); + $v->validate('12345'); }, ); } - # TODO: Add test to ensure filter names cannot be existing ifgroup names + /** + * Checks that the invalid-characters check fires before the reserved-name check, ensuring + * character validation is the outermost guard. + */ + public function test_invalid_characters_checked_before_reserved_names(): void { + $this->assert_throws_response( + response_id: 'FILTER_NAME_VALIDATOR_INVALID_CHARACTERS', + code: 400, + callable: function () { + # 'ssh guard' (with a space) would also be a reserved name pattern, but the + # invalid-character error must be raised first. + $v = new FilterNameValidator(); + $v->validate('ssh guard'); + }, + ); + } } diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Validators/FilterNameValidator.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Validators/FilterNameValidator.inc index 24d55a173..3ceedd0a2 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Validators/FilterNameValidator.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Validators/FilterNameValidator.inc @@ -24,6 +24,15 @@ class FilterNameValidator extends Validator { public function validate(mixed $value, string $field_name = ''): void { $reserved_names = get_pf_reserved(); + # Throw an exception if the name contains characters outside A-Z, a-z, 0-9, and _ + if (!preg_match('/^[A-Za-z0-9_]+$/', $value)) { + throw new ValidationError( + message: "Field '$field_name' cannot be '$value' because it contains invalid characters. " . + 'Only A-Z, a-z, 0-9, and _ are allowed.', + response_id: 'FILTER_NAME_VALIDATOR_INVALID_CHARACTERS', + ); + } + # Throw an exception if this name is reserved if (in_array($value, $reserved_names)) { throw new ValidationError(