jaredhendrickson13 / pfsense-api

The missing REST and GraphQL API package for pfSense
https://pfrest.org/
Apache License 2.0
677 stars 104 forks source link

"Field `id` is required" when patching /api/v2/firewall/rule #504

Closed michelsup closed 2 months ago

michelsup commented 3 months ago

Describe the bug When you go to /api/v2/documentation#/FIREWALL/patchFirewallRuleEndpoint, fill an id, and Execute you get a 400 error.

To Reproduce Go to /api/v2/documentation#/FIREWALL/patchFirewallRuleEndpoint Select "application/x-www-form-urlencoded" Enter an existing rule id Click Execute

Expected behavior Get a 200 response

Screenshots or Response curl -X 'PATCH' \ 'https://xxxxx:4443/api/v2/firewall/rule' \ -H 'accept: application/json' \ -H 'Authorization: Basic YWRtaW46UHdldDY0NTch' \ -H 'Content-Type: application/x-www-form-urlencoded' \ -d 'descr=&gateway=&log=&source_port=&floating=&pdnpipe=&ackqueue=&statetype=keep%20state&tcp_flags_set=&quick=&destination=&defaultqueue=&destination_port=&tcp_flags_any=&icmptype=any&sched=&dnpipe=&protocol=&interface=&type=&source=&id=25&disabled=&tcp_flags_out_of=&ipprotocol=&direction=any'

{ "code": 400, "status": "bad request", "response_id": "MODEL_REQUIRES_ID", "message": "Fieldidis required.", "data": [] }

pfSense Version & Package Version:

Affected Endpoints:

jaredhendrickson13 commented 3 months ago

The application/x-www-form-urlencoded content-type expects the parameters to be included the URL itself rather than the request body, for example:

curl -s -k -u admin:pfsense -H "content-type: application/x-www-form-urlencoded" -H "accept: application/json" -X PATCH 'https://localhost/api/v2/firewall/rule?id=0'
{
  "code": 200,
  "status": "ok",
  "response_id": "SUCCESS",
  "message": "",
  "data": {
    "id": 0,
    "type": "pass",
    "interface": [
      "lan"
    ],
    "ipprotocol": "inet",
    "protocol": null,
    "icmptype": null,
    "source": "lan",
    "source_port": null,
    "destination": "any",
    "destination_port": null,
    "descr": "Default allow LAN to any rule",
    "disabled": false,
    "log": false,
    "statetype": "keep state",
    "tcp_flags_any": false,
    "tcp_flags_out_of": null,
    "tcp_flags_set": null,
    "gateway": null,
    "sched": null,
    "dnpipe": null,
    "pdnpipe": null,
    "defaultqueue": null,
    "ackqueue": null,
    "floating": false,
    "quick": null,
    "direction": null,
    "tracker": 100000101,
    "created_time": 1713996066,
    "created_by": "",
    "updated_time": 1713996066,
    "updated_by": "admin@127.0.0.1 (API)"
  }
}

However, application/x-www-form-urlencoded is not recommended for requests other than GET and DELETE requests because it only supports type inference and the API requires strict typing (e.g. there's no way to differentiate "true" from true, or "443" from 443). application/json is the recommended content-type for POST, PATCH and PUT requests.

michelsup commented 2 months ago

Thank you for your quick reply. My bad then, I should have miss the info on the documentation. I would suggest you to remove the ability to use application/x-www-form-urlencoded within the documentation if it's not recommended, and to be more precise, if it plainly doesn't work as expected/suggested in /api/v2/documentation#/FIREWALL/patchFirewallRuleEndpoint. BTW, thank you for the hint and, overall, your work.

jaredhendrickson13 commented 2 months ago

After looking into this a little more, I do think this is a valid issue. I'll keep this open as an enhancement since the API should allow URL encoded parameters to be submitted via request body per HTTP standard. It was also supported in v1 so there's not really a valid reason to have that limitation in v2. I'll make sure to update documentation accordingly as well.

Thanks!