diff --git a/.github/release-drafter.yml b/.github/release-drafter.yml new file mode 100644 index 0000000..710a81d --- /dev/null +++ b/.github/release-drafter.yml @@ -0,0 +1,56 @@ +name-template: "v$RESOLVED_VERSION" +tag-template: "v$RESOLVED_VERSION" + +categories: + - title: "⚠️ Breaking changes" + labels: + - "breaking-change" + - title: "🚀 Features" + labels: + - "feature" + - "enhancement" + - title: "🐛 Bug fixes" + labels: + - "bug" + - "fix" + - title: "🧰 Maintenance" + labels: + - "chore" + - "maintenance" + - "refactor" + - "ci" + - "documentation" + - "dependencies" + +exclude-labels: + - "skip-changelog" + +change-template: "- $TITLE (#$NUMBER) @$AUTHOR" +change-title-escapes: '\<*_&' + +version-resolver: + major: + labels: + - "breaking-change" + minor: + labels: + - "feature" + - "enhancement" + patch: + labels: + - "bug" + - "fix" + - "chore" + - "maintenance" + - "refactor" + - "ci" + - "documentation" + - "dependencies" + default: patch + +template: | + ## Changes + + $CHANGES + + **Full changelog**: https://github.com/$OWNER/$REPOSITORY/compare/$PREVIOUS_TAG...v$RESOLVED_VERSION diff --git a/CLAUDE.md b/CLAUDE.md index 2b4d894..5008a31 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -22,9 +22,87 @@ them on hardware that has the feature before shipping that piece: - `GET /api/v2/system/packages` shape — no add-on packages installed (it 500s in that state, see Firmware section). - Whether `DELETE /api/v2/diagnostics/arp_table/entry` accepts an IP string for `id`. -- Whether `DELETE /api/v2/firewall/states` supports a usable source/dest filter. -No integration code has been rewritten yet — this document is the deliverable. +Resolved on hardware since the first draft — see "Findings from live hardware" below: +`DELETE /api/v2/firewall/states` source/dest filtering; the `disabled` field on +GUI-authored rules; `PATCH /firewall/rule` and empty `statetype`. + +The integration has since been rewritten and shipped (see repo history through +`v0.10.0`); this document is kept as the reference for pfRest behaviour. + +## Findings from live hardware + +Discovered running the shipped integration against the live Netgate 6100 +(26.07-RELEASE, pfRest v2.10.2). These are pfRest behaviours, not integration +bugs, and matter to anyone touching the same endpoints. + +### `disabled` is wrong for rules disabled in the pfSense web UI + +`GET /api/v2/firewall/rules` (and the NAT list endpoints) return +`"disabled": false` for **every rule that was disabled through the pfSense +webConfigurator**, even though the GUI shows them disabled and pf is not loading +them. Confirmed against a rule whose `config.xml` literally contains +``. + +Cause: pfRest models `disabled` as a `BooleanField` with the default +`indicates_true: ''`, and `BooleanField::_from_internal()` does a **strict** +`$internal_value === $this->indicates_true` comparison. pfSense's own GUI writes +the content-less element ``, which pfSense's XML parser +loads as an empty array (not `''`), so `[] === ''` is false and pfRest reports +`false`. Rules disabled **through the REST API** store pfRest's own `''` token +and round-trip correctly. + +Consequence: `is_on = not rule.get("disabled")` is correct code fed bad data — +GUI-disabled rules show as ON in Home Assistant. There is no clean client-side +fix; the API genuinely does not expose the real state for these rules. Report +upstream (https://github.com/pfrest/pfSense-pkg-RESTAPI) and/or check for a newer +package. `git blame` the `disabled` handling before assuming the integration is +at fault. + +### `PATCH /firewall/rule` rejects a blank `statetype` + +`PATCH /api/v2/firewall/rule {"id": N, "disabled": }` returns +`400 FIELD_EMPTY_NOT_ALLOWED: Field 'statetype' cannot be empty` for any rule +whose `statetype` is `""` in the config — again, the GUI writes +`` for some rules (mostly `block`/`reject`). pfRest +re-validates the **whole** object on a PATCH, so an unrelated toggle fails. + +Same root cause family: `statetype` is a `StringField(default: 'keep state', +choices: [...])` with no `allow_empty`, and `''` is present-but-invalid so the +default never applies. + +Workaround shipped in the client (`_set_rule_disabled` / +`_RULE_REQUIRED_DEFAULTS`): when the rule's `statetype` is falsy, include +`"statetype": "keep state"` (pfSense's own default, a behavioural no-op) in the +PATCH body. Extend the table if other empty-but-required fields surface. + +### `DELETE /api/v2/firewall/states` — the filter works + +The earlier LIVE-CHECK is resolved. The **prefix** filter is reliable: + +- `DELETE /api/v2/firewall/states?source__startswith=` and + `?destination__startswith=` both delete the matching states. + State endpoints render as `ip:port` (IPv4), so a host is `"10.1.1.1:"` and an + octet-aligned network is its leading octets plus a dot (`"10.0.10."`). +- `limit=0` means "no limit" for both GET and DELETE — a single call deletes + **every** matching state, no per-call cap. (`limit=100000` behaves + pathologically; use `0`.) +- The DELETE response `data` is the list of deleted state objects, so `len(data)` + is the deleted count. +- Not usable for non-octet-aligned CIDRs (e.g. `/25`) or IPv6 via `startswith`; + the shipped `kill_states_for_rule` skips those rather than shelling out. +- Exact-match query params (`source=`) did **not** work with a bare IP; only + the `__startswith` / `__contains` operators matched. + +### Aside: HACS "no update available" + +If a release goes unnoticed by HACS, check for a **version regression** — HACS +compares versions semantically and treats a lower `manifest.json` / tag as a +downgrade. This fork's manifest was briefly at `3.0.0` before the first `0.9.x` +tags existed; anyone whose HACS recorded `2.x`/`3.x` must remove and re-add the +integration. Keep the tag, `manifest.json`, and shipped content in lockstep +(see `.github/workflows/release.yml`, which now *verifies* tag == manifest +rather than rewriting the manifest after the tag). ## Target environment (confirmed) @@ -250,6 +328,12 @@ The methods the integration actually calls, and their REST replacements: `nat/outbound/mapping`. **`PATCH` takes no `?apply=` param — the separate apply call is mandatory.** Server-side `query[...]` filtering was unreliable in testing (returned unfiltered results); keep finding rules client-side. +- **`disabled` is unreliable on read** — pfRest reports `false` for rules + disabled via the pfSense GUI (see "Findings from live hardware"). `is_on` + looks backwards for those rules and there is no client-side fix. +- **`statetype` gotcha on write** — a bare `{"id", "disabled"}` PATCH 400s with + `FIELD_EMPTY_NOT_ALLOWED` on rules whose `statetype` is blank; the client + backfills `"statetype": "keep state"`. Same section. ### Aliases @@ -277,13 +361,15 @@ The methods the integration actually calls, and their REST replacements: ### State table -- `reset_state_table()` → `DELETE /api/v2/firewall/states` (no query). Default - `limit` is 100 — pass a large `limit` or loop until empty, **or** use - `command_prompt` with `pfctl -F states` for exact parity. -- `kill_states(source, destination)` → - `DELETE /api/v2/firewall/states?query[...]` — **LIVE-CHECK** the queryable - field names (`src` / `dst`?). Fallback that matches current behaviour: - `command_prompt` with `pfctl -k [-k ]`. +- `reset_state_table()` → `DELETE /api/v2/firewall/states` — pass **`limit=0`** + ("no limit"; a single call clears everything). The default page is 100. +- `kill_states(source, destination)` — still uses `pfctl -k` via + `command_prompt` (the client keeps this for the service). For the + rule-switch state reset, `kill_states_for_rule` instead uses + `DELETE /api/v2/firewall/states?{source,destination}__startswith=` + with `limit=0` — the prefix filter is confirmed working on hardware (see + "Findings from live hardware"). `startswith` can't express `/25`-style + networks or IPv6, which that path deliberately skips. ### System control diff --git a/README.md b/README.md index 1908613..b4a60c6 100644 --- a/README.md +++ b/README.md @@ -5,10 +5,19 @@ API v2** (`pfSense-pkg-RESTAPI`). It surfaces live system, interface, gateway, DHCP and VPN telemetry as entities, and lets you drive firewall rules, NAT rules, services, aliases and routing from Home Assistant. -[![Latest Release](https://img.shields.io/github/v/release/nolsen311/Pfsense-pro?style=for-the-badge&color=007ec6)](https://github.com/nolsen311/Pfsense-pro/releases) -[![License](https://img.shields.io/github/license/nolsen311/Pfsense-pro?style=for-the-badge&color=007ec6)](https://github.com/nolsen311/Pfsense-pro/blob/main/LICENSE) -[![Tests](https://img.shields.io/github/actions/workflow/status/nolsen311/Pfsense-pro/pytest.yml?style=for-the-badge&label=TESTS&color=5dbb0f)](https://github.com/nolsen311/Pfsense-pro/actions/workflows/pytest.yml) -[![HACS Validation](https://img.shields.io/github/actions/workflow/status/nolsen311/Pfsense-pro/hacs.yaml?style=for-the-badge&label=HACS&color=5dbb0f)](https://github.com/nolsen311/Pfsense-pro/actions/workflows/hacs.yaml) +> **Origin.** This project started as a fork of +> [DonTranQuiL/Pfsense-pro](https://github.com/DonTranQuiL/Pfsense-pro), but has +> since been extensively rewritten — the XML-RPC client was replaced with a +> native async REST API v2 client and most of the integration was reworked, with +> the help of Claude Cowork. It has been de-forked from the upstream repository so +> that pull requests don't get opened against DonTranQuiL/Pfsense-pro by mistake. +> It is now maintained as a standalone project. + +[![Latest Release](https://img.shields.io/github/v/release/nolsen311/Pfsense-REST?style=for-the-badge&color=007ec6)](https://github.com/nolsen311/Pfsense-REST/releases) +[![License](https://img.shields.io/github/license/nolsen311/Pfsense-REST?style=for-the-badge&color=007ec6)](https://github.com/nolsen311/Pfsense-REST/blob/main/LICENSE) +[![CI](https://img.shields.io/github/actions/workflow/status/nolsen311/Pfsense-REST/hass-ci.yml?branch=main&style=for-the-badge&label=CI&color=5dbb0f)](https://github.com/nolsen311/Pfsense-REST/actions/workflows/hass-ci.yml) +[![hassfest](https://img.shields.io/github/actions/workflow/status/nolsen311/Pfsense-REST/hassfest.yaml?branch=main&style=for-the-badge&label=HASSFEST&color=5dbb0f)](https://github.com/nolsen311/Pfsense-REST/actions/workflows/hassfest.yaml) +[![HACS Validation](https://img.shields.io/github/actions/workflow/status/nolsen311/Pfsense-REST/hacs.yaml?branch=main&style=for-the-badge&label=HACS&color=5dbb0f)](https://github.com/nolsen311/Pfsense-REST/actions/workflows/hacs.yaml) [![HACS Custom](https://img.shields.io/badge/HACS-Custom-ff6e27?style=for-the-badge)](https://hacs.xyz/) --- diff --git a/custom_components/pfsense/__init__.py b/custom_components/pfsense/__init__.py index 416d0af..7f6f5e3 100644 --- a/custom_components/pfsense/__init__.py +++ b/custom_components/pfsense/__init__.py @@ -195,6 +195,7 @@ async def async_update_data(): coordinator = DataUpdateCoordinator( hass, _LOGGER, + config_entry=entry, name=f"{entry.title} pfSense state", update_method=async_update_data, update_interval=timedelta(seconds=scan_interval), @@ -233,6 +234,7 @@ async def async_update_device_tracker_data(): device_tracker_coordinator = DataUpdateCoordinator( hass, _LOGGER, + config_entry=entry, name=f"{entry.title} pfSense device tracker state", update_method=async_update_device_tracker_data, update_interval=timedelta(seconds=device_tracker_scan_interval), diff --git a/custom_components/pfsense/binary_sensor.py b/custom_components/pfsense/binary_sensor.py index 05b8bb0..145e329 100644 --- a/custom_components/pfsense/binary_sensor.py +++ b/custom_components/pfsense/binary_sensor.py @@ -39,7 +39,7 @@ def process_entities_callback(hass, config_entry): name="CARP Status", icon="mdi:gauge", ), - False, + True, ) ] diff --git a/custom_components/pfsense/manifest.json b/custom_components/pfsense/manifest.json index 0898b1e..03dec7f 100644 --- a/custom_components/pfsense/manifest.json +++ b/custom_components/pfsense/manifest.json @@ -6,11 +6,11 @@ ], "config_flow": true, "dependencies": [], - "documentation": "https://github.com/nolsen311/Pfsense-pro", + "documentation": "https://github.com/nolsen311/Pfsense-REST", "iot_class": "local_polling", - "issue_tracker": "https://github.com/nolsen311/Pfsense-pro/issues", + "issue_tracker": "https://github.com/nolsen311/Pfsense-REST/issues", "requirements": [ "mac-vendor-lookup>=0.1.11" ], - "version": "0.10.0" + "version": "0.10.1" } diff --git a/tests/test_init.py b/tests/test_init.py index cf163bc..ded4237 100644 --- a/tests/test_init.py +++ b/tests/test_init.py @@ -5,8 +5,8 @@ import pytest from pytest_homeassistant_custom_component.common import MockConfigEntry -from custom_components.pfsense import async_setup_entry, async_unload_entry from custom_components.pfsense.const import CONF_API_KEY, DOMAIN +from homeassistant.config_entries import ConfigEntryState from homeassistant.const import CONF_URL, CONF_VERIFY_SSL from homeassistant.core import HomeAssistant @@ -60,6 +60,8 @@ async def test_setup_and_unload_entry(hass: HomeAssistant): entry = MockConfigEntry( domain=DOMAIN, version=3, + title="router.local", + unique_id="abc", data={ CONF_URL: "https://192.168.1.1:8444", CONF_API_KEY: "k", @@ -77,22 +79,18 @@ async def test_setup_and_unload_entry(hass: HomeAssistant): ), patch("custom_components.pfsense.async_load_cache", return_value=None), patch("custom_components.pfsense.async_save_cache"), - patch( - "homeassistant.config_entries.ConfigEntries.async_forward_entry_setups", - return_value=True, - ) as mock_forward, ): - assert await async_setup_entry(hass, entry) is True - assert mock_forward.called - assert DOMAIN in hass.data - - with patch( - "homeassistant.config_entries.ConfigEntries.async_unload_platforms", - return_value=True, - ) as mock_unload: - assert await async_unload_entry(hass, entry) is True - assert mock_unload.called - assert entry.entry_id not in hass.data.get(DOMAIN, {}) + assert await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.state is ConfigEntryState.LOADED + assert entry.entry_id in hass.data[DOMAIN] + + assert await hass.config_entries.async_unload(entry.entry_id) + await hass.async_block_till_done() + + assert entry.state is ConfigEntryState.NOT_LOADED + assert entry.entry_id not in hass.data.get(DOMAIN, {}) @pytest.mark.asyncio