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.

WhatDoesItAffect:

%WHATDOESITAFFECT%

edit

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

Do we need to revert this for 1.2, come up with a fix to the redirect, or live with the current situation

-- GeorgeClark - 02 Apr 2014

Isn't it a problem to let anybody, even a guest, click on a release_lock=1 view link and break edit locks of whoever is currently editing the topic? Note that view is actually performing a store operation deleting a lock file, and that this view is a GET. I am feeling very uneasy of opening this up so unprotected. I can't fully understand why a GET to view is safer than a POST to save to cancel an edit. Nor do I understand by now why a change is needed.

And I think Arthur is right noting that it clashes with redirectto as the edit isn't released in that case, is it?

-- MichaelDaum - 02 Apr 2014

Unless you've found a bug, release_lock=1 only releases a lock held by the current user.

The POST to Save to cancel was found to be less safe because of a bug in the save path that saved even when cancel was selected. That was in the code for years, although it was fixed and current versions are not vulnerable.

Belts and Suspenders. If you issue a GET to view, you cannot possibly save.

This new operation has been in trunk for nearly 2 years. If you fixed the bug that caused the redirect to nowhere when canceling edit on a non-existing topic, then I think that this is all moot.

-- GeorgeClark - 02 Apr 2014

As I already said 2 years ago: this proposal is motivated by core bugs, not by a missing feature or the like. The core bugs have been fixed as far as I can see. So the motivation for this feature proposal is gone as well. George, can you please explain why we still want view to release edit locks instead of save? Otherwise I'd suggest we revert.

-- MichaelDaum - 03 Apr 2014

And you just proved my point. You state: "The core bugs have been fixed as far as I can see" Exactly! It's the un-seen bugs that I worry about. If save is not called, then the bugs can not be encountered.

Wikipedia:Formal_verification is a horrifically expensive and time consuming effort. I'm just saying that it's easier to not use the code than to try to subject it to more thorough inspection. With still no guaranty that everything will be found.

-- GeorgeClark - 03 Apr 2014

Being worried about un-seen bugs in save is all fine and well, maybe a bit paranoid but okay. In the end they need a fix. And that's what just happened independent of the invention of release_lock for view. You won't fix bugs in save by not calling save anymore, those will still stay in there ... given there is a bug.

Let me repeat: there was a bug in save which got fixed. There was a feature proposal based on the motivation that save had a bug which doesn't hold anymore as far as I can see.

So what exactly is the motivation for this proposal now? I simply don't get it.

Furthermore, the feature proposed here is perverting view.

-- MichaelDaum - 04 Apr 2014

Commenting is disabled while not logged in