Feature Proposal: Never POST to a save script for purposes of cancelling an update

Motivation

It was discovered in Tasks.Item11600 that cancel posts to the save script. This has been circumvented by requiring an action URL parameter requesting save or cancel. There really should be no reason to execute the Manage script to cancel the edit. It is much safer to cancel by using a GET to view instead of POST to save with a cancel action.

As reported in Tasks.Item11798, the initial fix was bad, as it was sensitive to the locale / translation of the skin.

Description and Documentation

Implement a cancel function:

• Add a new URL Parameter to the view script: release_lock, which releases any lock held on the current topic by the current user.
• Change the Skin templates so that the Cancel button is a simple link to the view script with the release_lock=1 URL parameter.

Examples

Impact

The existing action_save and action_cancel parameters should still be implemented so that older skins can continue to function as-is.

Using view to cancel from an edit is also clearer in the event log.

Implementation

-- Contributors: GeorgeClark - 03 May 2012

Discussion

Needs another developer to help with the template and Skin part of this. I've made the following change to edit.pattern.tmpl and it appears to be functional, but the button is rendered with the wrong color text (blue vs. black).

-%TMPL:DEF{"button_cancel"}%<input type="submit" class="foswikiButtonCancel" name="action_cancel" id="cancel" title='%MAKETEXT{"Cancel editing and discard changes"}%' %MAKETEXT{"value='Cancel' accesskey='c'"}% />%TMPL:END%
+%TMPL:DEF{"button_cancel"}%<a href='%SCRIPTURLPATH{"view"}%/%BASEWEB%/%BASETOPIC%?release_lock=1' id="cancel" title='%MAKETEXT{"Cancel editing and discard changes"}%' class="foswikiButtonCancel" >%MAKETEXT{"Cancel"}%</a>%TMPL:END%

-- GeorgeClark - 03 May 2012

This code change is a major problem for editors like NatEditPlugin to maintain backwards compatibility.

As far as I understand the motivation of this feature request there have been two bugs:

1. canceling an edit-topic-preferences dialog was buggy
2. the save script didn't properly check the validity of a real save action, thus misinterpreting a cancellation of an edit session as a normal save.

(2) basically was triggered by (1).

Now both bugs have been fixed as far as I read the change-sets of the related bug items. From my understanding fixing those two bugs basically removes the motivation for such a change as proposed in this feature proposal. Talking on IRC, people said, it was "safer" not to call save at all for a cancel, because any new bug in save could happen again to trigger the same error as has happened in (1). Well, any bug in the save script is a serious problem, no matter what the protocol for a cancel action is.

My stance on this is, and pardon me to raise my voice so lately: the save script should be used for anything related to save, including cancelling the edit session. Requiring a proper strikeone token for the cancel action sounds more secure rather than the opposite as stated above.

So can we have some more voices on this, please, as I find this change problematic. I'd rather prefer to maintain a secure + backwards compatible protocol for cancel.

Name	Cancel edit using view (new)	Cancel edit using save (old)	Comment
MichaelDaum			keep backwards compatibility
GeorgeClark			Belts and Suspenders.
SvenDowideit			deprecate cancel using save, so it would be removed in 2 releases time

-- MichaelDaum - 02 Aug 2012

I also find that using save to cancel generates inconsistent logs, in that the Apache log records it as a save, where as the events log does not. It's one more thing for the obsessed admin to chase down. Rich apache tools like awstats show more consistent statistics. I also looked at is as the converse of the rule that you must never modify content from a GET operation, only from POST. If the intent is to not modify content, then a GET to the view script is to me a preferable option. It clearly has no capability of saving.

Note that the old behavior, post to save to cancel, has not been removed. Existing skins, and local custom skins etc. still need to work. So this change is not per se backwards incompatible. Pattern skin and other core extensions however should use the new protocol for best safety and consistency.

-- GeorgeClark - 02 Aug 2012

Releasing a lock is an integral part of an edit-save transaction. Releasing the lock is done by a proper save anyway. So it seems natural to still use save to release the edit lock during a cancel but skip the storing part. Using view is quite off as it is not related to an edit-save transaction in general.

Admitted the Apache log argument is a good one, though I am not sure whether we should change Foswiki just for that reason.

NatEditPlugin can't use the new protocol as it would break cancel on other Foswiki versions where people want to upgrade some plugins only.

-- MichaelDaum - 03 Aug 2012

At least in PatternSkin, this could probably be handled with some conditional templates, if we had a simple way to check the API level of core.

For the new indent syntax, CrawfordCurrie added a new context: SUPPORTS_PARA_INDENT. However I'm wondering if it would be better to generalize this with the core API level, adding, but never removing a context reflecting the API.

• context API22 for Foswiki 1.1
• context API22 and API23 for Foswiki 1.2

-- GeorgeClark - 04 Aug 2012

See AddFeatureLevelContext, instead of the APIxx context.

-- GeorgeClark - 05 Aug 2012

I want to head towards view having a read only (and thus faster) store, with the write store enabled as little as possible - save being the main path.

Releasing a lease is still a read only store, as locks are really global scope session 'hints'. That said, I'd use the deprecation rules for moving from the old way to the new -

-- SvenDowideit - 06 Aug 2012

Sven, could you elaborate in what sense cancelling via view is faster. I don't understand "I want to head towards view having a read only (and thus faster) store". What is it supposed to mean?

-- MichaelDaum - 06 Aug 2012

This change breaks the form flow when cancelling. If you have an edit link with a redirectto param to bring the user back to the start, since implementation this works only when saving. When cancelling, the user is left at the edit topic, not at the topic he should be redirected to. There is no way to let a link follow a redirect parameter.

-- ArthurClemens - 14 Oct 2012