From 8e1bea9812a03b6921a90c7feab735ead10a5a67 Mon Sep 17 00:00:00 2001 From: Jared Hendrickson Date: Wed, 19 Feb 2025 16:54:27 -0700 Subject: [PATCH 1/5] fix: use redirect host and port in associated rules #659 --- .../files/usr/local/pkg/RESTAPI/Models/PortForward.inc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 bb49d4d20..a43ec8673 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 @@ -205,8 +205,8 @@ class PortForward extends Model { protocol: $this->protocol->value, source: $this->source->value, source_port: $this->source_port->value, - destination: $this->destination->value, - destination_port: $this->destination_port->value, + destination: $this->target->value, + destination_port: $this->local_port->value, descr: "Associated rule for port forward rule {$this->associated_rule_id->value}", client: $this->client, ); @@ -246,8 +246,8 @@ class PortForward extends Model { protocol: $this->protocol->value, source: $this->source->value, source_port: $this->source_port->value, - destination: $this->destination->value, - destination_port: $this->destination_port->value, + destination: $this->target->value, + destination_port: $this->local_port->value, ); # Format any errors that occur during the update of the rule so its clear that its associated with the linked rule From 670a338abe35c215f05dfcf5c708b2c49ca808bb Mon Sep 17 00:00:00 2001 From: Jared Hendrickson Date: Sun, 23 Feb 2025 10:15:36 -0700 Subject: [PATCH 2/5] fix: pass client info to each model object created by Model::replace_all() --- .../usr/local/pkg/RESTAPI/Core/Model.inc | 2 +- .../Tests/APIModelsFirewallRuleTestCase.inc | 43 +++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/Model.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/Model.inc index 921ff495d..b491f2d81 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/Model.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/Model.inc @@ -2218,7 +2218,7 @@ class Model { unset($representation_object['id']); # Define a new Model object using this representation and validate - $model_object = new $this(data: $representation_object, skip_init: true); + $model_object = new $this(data: $representation_object, skip_init: true, client: $this->client); $model_object->id = $model_object->get_next_id(); $model_object->validate(modelset: $new_objects); diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsFirewallRuleTestCase.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsFirewallRuleTestCase.inc index 8077a7078..0df06b069 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsFirewallRuleTestCase.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsFirewallRuleTestCase.inc @@ -2,6 +2,7 @@ namespace RESTAPI\Tests; +use RESTAPI\Core\Auth; use RESTAPI\Core\Command; use RESTAPI\Core\TestCase; use RESTAPI\Models\FirewallRule; @@ -904,4 +905,46 @@ class APIModelsFirewallRuleTestCase extends TestCase { $limiter2->delete(apply: true); $limiter1->delete(apply: true); } + + /** + * Checks that the username of the client is correctly identified when replace_all is called. + */ + public function test_replace_all_retains_client_username(): void + { + # Mock the authenticated client + $client = new Auth(); + $client->username = "testuser"; + $client->ip_address = "127.1.2.3"; + + # Replace all firewall rules with a new set + $rule_model = new FirewallRule(client: $client); + $rules = $rule_model->replace_all( + data: [ + [ + 'type' => 'pass', + 'interface' => ['lan'], + 'ipprotocol' => 'inet', + 'source' => 'any', + 'destination' => 'any', + 'descr' => 'test description', + ], + [ + 'type' => 'pass', + 'interface' => ['lan'], + 'ipprotocol' => 'inet', + 'source' => 'any', + 'destination' => 'any', + 'descr' => 'test description 2', + ], + ] + ); + + # Ensure the client username and IP are correctly set in the new rules + foreach ($rules->model_objects as $rule) { + $this->assert_equals($rule->client->username, $client->username); + $this->assert_equals($rule->client->ip_address, $client->ip_address); + $this->assert_equals($rule->created_by->value, "$client->username@$client->ip_address (API)"); + $this->assert_equals($rule->updated_by->value, "$client->username@$client->ip_address (API)"); + } + } } From e981d1b12a6e2a676e8a79ad8b42f6b07d0850c3 Mon Sep 17 00:00:00 2001 From: Jared Hendrickson Date: Sun, 23 Feb 2025 10:19:52 -0700 Subject: [PATCH 3/5] tests: update PortForward tests to check for correct associated rule values --- .../pkg/RESTAPI/Tests/APIModelsPortForwardTestCase.inc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsPortForwardTestCase.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsPortForwardTestCase.inc index c6801bd5d..6dcc29b20 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsPortForwardTestCase.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsPortForwardTestCase.inc @@ -182,7 +182,7 @@ class APIModelsPortForwardTestCase extends TestCase { /** * Checks that port forwards with associated firewall rules correctly create, update and delete the associated rule */ - public function test_associated_firewall_rules() { + public function test_associated_firewall_rules(): void { # Create a port forward to test with $port_forward = new PortForward( interface: 'wan', @@ -208,8 +208,8 @@ class APIModelsPortForwardTestCase extends TestCase { $this->assert_equals($rule_q->first()->protocol->value, 'tcp'); $this->assert_equals($rule_q->first()->source->value, '1.2.3.4'); $this->assert_equals($rule_q->first()->source_port->value, '1234'); - $this->assert_equals($rule_q->first()->destination->value, '4.3.2.1'); - $this->assert_equals($rule_q->first()->destination_port->value, '4321'); + $this->assert_equals($rule_q->first()->destination->value, '127.0.0.1'); + $this->assert_equals($rule_q->first()->destination_port->value, '1234'); # Update the port forward and ensure the associated rule is updated $port_forward->from_representation( @@ -228,8 +228,8 @@ class APIModelsPortForwardTestCase extends TestCase { $this->assert_equals($rule_q->first()->protocol->value, 'udp'); $this->assert_equals($rule_q->first()->source->value, '4321::1'); $this->assert_equals($rule_q->first()->source_port->value, '4321'); - $this->assert_equals($rule_q->first()->destination->value, '1234::1'); - $this->assert_equals($rule_q->first()->destination_port->value, '1234'); + $this->assert_equals($rule_q->first()->destination->value, 'fe80::1'); + $this->assert_equals($rule_q->first()->destination_port->value, '4321'); # Delete the port forward and ensure the associated rule is deleted $port_forward->delete(); From 99e9b6b6f1f1be5b96bff65d73b785d6d323ede8 Mon Sep 17 00:00:00 2001 From: Jared Hendrickson Date: Sun, 23 Feb 2025 10:24:37 -0700 Subject: [PATCH 4/5] fix: allow nested FirewallAlias entries for host and network type Before we were only allowing nested aliases if the type matched which isn't necessary. --- .../files/usr/local/pkg/RESTAPI/Models/FirewallAlias.inc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/FirewallAlias.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/FirewallAlias.inc index 13fff9622..3319078a1 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/FirewallAlias.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/FirewallAlias.inc @@ -108,7 +108,7 @@ class FirewallAlias extends Model { } # Ensure value is an IP, FQDN or alias when `type` is `host` - $host_alias_q = $aliases->query(name: $address, type: 'host'); + $host_alias_q = $aliases->query(name: $address, type__except: 'port'); if ($type === 'host' and !is_ipaddr($address) and !is_fqdn($address) and !$host_alias_q->exists()) { throw new ValidationError( message: "Host alias 'address' value '$address' is not a valid IP, FQDN, or alias.", @@ -116,8 +116,8 @@ class FirewallAlias extends Model { ); } - # Ensure value is a CIDR, FQDN or alias when `type` is `network` - $network_alias_q = $aliases->query(name: $address, type: 'network'); + # Ensure value is a CIDR, FQDN or non-port alias when `type` is `network` + $network_alias_q = $aliases->query(name: $address, type__except: 'port'); if ($type === 'network' and !is_subnet($address) and !is_fqdn($address) and !$network_alias_q->exists()) { throw new ValidationError( message: "Network alias 'address' value '$address' is not a valid CIDR, FQDN, or alias.", From 486c75a03bd2a2b3373677ca6a5b34f4300a46cd Mon Sep 17 00:00:00 2001 From: Jared Hendrickson Date: Sun, 23 Feb 2025 10:25:20 -0700 Subject: [PATCH 5/5] style: run prettier on changed files --- .../pkg/RESTAPI/Tests/APIModelsFirewallRuleTestCase.inc | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsFirewallRuleTestCase.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsFirewallRuleTestCase.inc index 0df06b069..d7eef9619 100644 --- a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsFirewallRuleTestCase.inc +++ b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsFirewallRuleTestCase.inc @@ -909,12 +909,11 @@ class APIModelsFirewallRuleTestCase extends TestCase { /** * Checks that the username of the client is correctly identified when replace_all is called. */ - public function test_replace_all_retains_client_username(): void - { + public function test_replace_all_retains_client_username(): void { # Mock the authenticated client $client = new Auth(); - $client->username = "testuser"; - $client->ip_address = "127.1.2.3"; + $client->username = 'testuser'; + $client->ip_address = '127.1.2.3'; # Replace all firewall rules with a new set $rule_model = new FirewallRule(client: $client); @@ -936,7 +935,7 @@ class APIModelsFirewallRuleTestCase extends TestCase { 'destination' => 'any', 'descr' => 'test description 2', ], - ] + ], ); # Ensure the client username and IP are correctly set in the new rules