Feature #4083
closed
Replace GET by POST
0%
Description
These functions should be sent as a POST to pfSense software, which should then do a redirect back to the status page. Going back through your browser history should not bring down your tunnels! Thanks.
Updated by Renato Botelho about 10 years ago
- Subject changed from IPSec disconnect and reconnect should be POST not GET to Replace GET by POST
- Target version changed from 2.2 to 2.2.1
This is a change that needs to be done globally, replace this ticket to a more general description to keep track of it and postpone since it's too late for 2.2
Updated by Chris Buechler almost 10 years ago
- Target version changed from 2.2.1 to 2.2.2
Updated by Chris Buechler over 9 years ago
- Target version changed from 2.2.2 to 2.2.3
Updated by Chris Buechler over 9 years ago
- Target version changed from 2.2.3 to 2.3
Updated by Fernando Munoz about 9 years ago
Is there any ETA for 2.3? it seems the target for this has been moving from version since almost one year ago, that doesn't look good. There are two CSRF issues that I care about right now:
https://localhost:9090/interfaces_vlan.php?act=del&id=1
https://localhost:9090/status_services.php?mode=stopservice&service=ntpd
First one allows deleting VLANs and the second one allows stopping services.
Updated by Anonymous about 9 years ago
Its cold and wet outside so I converted interfaces_vlan.php to POST.
Updated by Jim Thompson about 9 years ago
- Assignee changed from Renato Botelho to Anonymous
- Affected Architecture All added
- Affected Architecture deleted (
)
Just because Chris moves the target from release to release doesn't mean that there is a commitment to fix that issue in the target release.
Updated by Jim Pingle about 9 years ago
Steve Beaver wrote:
status_services.php also converted
The main body of status_services.php is converted but it looks like the shortcut bar service controls, openvpn status, and the widget are still using it via GET. I don't see any other pages/places that are using GET to control services, so if a solution can be found for those that would wrap up that page completely.
Widget: source:src/usr/local/www/widgets/widgets/services_status.widget.php
Shortcut bar service control: source:src/usr/local/www/head.inc#L480
OpenVPN status: source:src/usr/local/www/status_openvpn.php#L180
All three use get_service_control_GET_links() from source:src/etc/inc/service-utils.inc#L488
Perhaps hitting it via GET could land at a confirmation page, similar to what happens for a package install.
Updated by Jeremy Porter almost 9 years ago
- Status changed from New to Assigned
Updated by Renato Botelho almost 9 years ago
- Target version changed from 2.3 to Future
It's a WIP and won't be completed for 2.3
Updated by Jim Pingle almost 8 years ago
- Target version changed from Future to 2.4.0
The recent work here is excellent, for delete, enable/disable, and other actions that result in a config change or firewall state change, everything I've tried works wonderfully.
Using POST to display edit forms (e.g. act=new, act=edit, act=dup and similar) seems to have a couple negative effects though.
For example:
- It breaks breadcrumb links to the current page (leads to an empty item)
- It breaks refreshing the edit page to revert to the previous saved values
- It removes the ability to bookmark a specific item from the list
Looks like if we set 'id' using $_REQUEST in most cases we could use GET (or both) to display the edit forms and POST for actions that result in a config or state change.
Updated by Anonymous almost 8 years ago
Good points. I'll give that some thought.
Updated by Phillip Davis almost 8 years ago
Isn't the principle here that anything that changes stuff (makes a config change, stops/starts a service, applies changes...) should not be able to be done via direct URL parameters (i.e. REQUEST/GET). That avoids users accidentally re-doing actions.
Things that are just queries (load up a page with current settings of something, load up an empty page for a new entry, display status...) can be done directly with URL parameters. That makes it easy to get the data (e.g. if testing, refreshing pages...) and for the code to know what was requested based on the URL parameters.
Updated by Jim Pingle almost 8 years ago
Yes, that is the goal. It isn't only about accidental actions, though, but also CSRF protection.
Updated by Phillip Davis almost 8 years ago
Note issue https://redmine.pfsense.org/issues/7254
The dropdown list of interface names in Firewall Rules was still doing the old "ordinary" code to send "?if=opt1" type of request, but the code that decides what to do is no longer looking at that, it expects a $_POSTed value.
PR https://github.com/pfsense/pfsense/pull/3533 adds code to make that do $_POST based on the selected interface from the list.
But as Jim has commented, this approach in general means that users cannot bookmark a link to a particular interface, and the breadcrumb links would have to somehow be made smarter...
Updated by Phillip Davis almost 8 years ago
And here is a way to "fix" breadcrumb links: https://github.com/pfsense/pfsense/pull/3534
But of course it does not help for users making bookmarks etc.
Updated by Anonymous almost 8 years ago
- Status changed from Assigned to Feedback
All delete, toggle,disable and similar actions have been converted to POST via Javascript
There are two exceptions to this:- No packages have been converted
- The Traffic shaper has not been converted due to the complexity and risk of that task.
Updated by Anonymous almost 8 years ago
- Status changed from Feedback to Resolved
Marking this as resolved. Any further conversions, or the replacement of anchor tags with buttons tags can be opened as a new issue.