Project

General

Profile

Feature #4083

Replace GET by POST

Added by Michael Newton over 2 years ago. Updated 6 months ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Web Interface
Target version:
Start date:
12/08/2014
Due date:
% Done:

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.

History

#1 Updated by Jim Thompson over 2 years ago

  • Assignee set to Renato Botelho

#2 Updated by Renato Botelho over 2 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

#3 Updated by Chris Buechler over 2 years ago

  • Target version changed from 2.2.1 to 2.2.2

#4 Updated by Chris Buechler over 2 years ago

  • Target version changed from 2.2.2 to 2.2.3

#5 Updated by Chris Buechler about 2 years ago

  • Target version changed from 2.2.3 to 2.3

#6 Updated by Fernando Munoz almost 2 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.

#7 Updated by Steve Beaver almost 2 years ago

I'll look at those two pages next week.

#8 Updated by Steve Beaver almost 2 years ago

Its cold and wet outside so I converted interfaces_vlan.php to POST.

#9 Updated by Steve Beaver almost 2 years ago

status_services.php also converted

#10 Updated by Jim Thompson almost 2 years ago

  • Assignee changed from Renato Botelho to Steve Beaver
  • Affected Architecture set to All

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.

#11 Updated by Jim Pingle over 1 year 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.

#12 Updated by Jeremy Porter over 1 year ago

  • Status changed from New to Assigned

#13 Updated by Renato Botelho over 1 year ago

  • Target version changed from 2.3 to Future

It's a WIP and won't be completed for 2.3

#14 Updated by Jim Pingle 6 months 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.

#15 Updated by Steve Beaver 6 months ago

Good points. I'll give that some thought.

#16 Updated by Phillip Davis 6 months 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.

#17 Updated by Jim Pingle 6 months ago

Yes, that is the goal. It isn't only about accidental actions, though, but also CSRF protection.

#18 Updated by Phillip Davis 6 months 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...

#19 Updated by Phillip Davis 6 months 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.

#20 Updated by Steve Beaver 6 months 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:
  1. No packages have been converted
  2. The Traffic shaper has not been converted due to the complexity and risk of that task.

#21 Updated by Steve Beaver 6 months 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.

Also available in: Atom PDF