nette/application: restoreRequest and redirects

First of all I'd like to say that I've written this RFC as I understand the behaviour. In case I failed to mention something relevant or wrote something that is untrue, it's fully unintentional. Of course I might have missed or misinterpreted something. In such case please tell me!

I'm not sure if my solution is the best, some people probably won't like it. Maybe the best solution is to preserve the behaviour as is because of compatibility and/or because my solution might be overcomplicated. I'm fine with this RFC getting denied, in such case I will just rework it as an addon.

Analysis of the current behaviour

How canonicalize works: The purpose of canonicalize is to automatically redirect to a canonical URL for the current request. It means that when several URLs can trigger the same request (usually through different routers), the user gets redirected to the URL that he was supposed to use in the first place – the one that the router generates for the request when creating a link to it. Canonicalize only works when the HTTP method is GET or HEAD.

How forward works: When you call Presenter::forward(), new request is created and gets 'FORWARD' as method (instead of GET, POST or anything else). Therefore canonicalization won't be triggered for such request.

How restoreRequest works: When a request is restored using Presenter::restoreRequest(), the request is loaded from the session and gets a RESTORED flag to be distinguishable from the normal requests. Then the presenter forwards to this request. However the method is NOT set to FORWARD in this case. Therefore canonicalization IS triggered if the method was GET or HEAD. The URL won't match of course so the user gets redirected.

How I understand it

I believe that the redirecting behaviour for restored request was not intended and is actually a bug. Reasons:

• canonicalization is meant to redirect one-way URLs to the canonical one, not to deal with forwarded requests
• restoreRequest uses ForwardResponse, which means that the restored request is meant to be treated as a forwarded request
• if the redirect was intentional it would probably be done in the restoreRequest method itself in the first place
• the URL changes for GET requests but not for POST requests which I believe most people were never aware of
• with the redirect the RESTORED flag is lost which might even be a security issue for come people

(It was a security issue for me, see the appendix of this RFC.)

@JanTvrdík: You were opposing me the most when I sent a commit to fix it so what do you think about these reasons? I'd like to hear your opinion.

Consequences

Many people relied on this behaviour of restoreRequest. The common use-case is that when the user got logged out automatically the request got stored. When he loggs in again the application restores the request so that the user could continue from whereever he was and wouldn't lose his unsaved form data for example. Of course after he logged in again it's better to change the URL as well to where he was when he got logged out. The restoreRequest was good for this (at least for GET requests) so many people used it.

When I tried to fix the behaviour with this commit it was a BC break which is why it was reverted shortly after. (And it was the correct decision.)

Solution

If you have a better solution or a suggestion to improve this one, please tell me.

Step 1: New method redirectToRequest

In my opinion the restoreRequest method should not trigger a redirect. Not by itself and not via canonicalization either. Consider the original commit to also be a part of the proposed solution.

But I do understand that the behaviour with redirect is useful so I implemented a new method for it.

	public function redirectToRequest($key)
	{
		$session = $this->getSession('Nette.Application/requests');
		if (!isset($session[$key]) || ($session[$key][0] !== NULL && $session[$key][0] !== $this->getUser()->getId())) {
			return;
		}
		$request = clone $session[$key][1];
		unset($session[$key]);
		// it's useless to set the RESTORED flag, it will be lost anyway
		// $request->setFlag(\Nette\Application\Request::RESTORED, TRUE);
		$parameters = $request->getParameters();
		$parameters[self::FLASH_KEY] = $this->getParameter(self::FLASH_KEY);
		$request->setParameters($parameters);
		// $refUrl copied from Presenter::createRequest()
		$refUrl = new \Nette\Http\Url($this->httpRequest->getUrl());
		$refUrl->setPath($this->httpRequest->getUrl()->getScriptPath());
		$url = $this->router->constructUrl($request, $refUrl);
		$this->sendResponse(new RedirectResponse($url));
	}

Of course this will throw away POST data so it is not ideal. Also after a redirect the RESTORED flag is lost. Both of these issues are solved by the solution below.

Step 2: Emulation for POST requests

It would be better to redirect to the correct URL even in case of POST requests and preserve the POST data of course. Well it's AFAIK impossible to actually redirect to a POST request but we can emulate it by keeping the stored request in session a bit longer, adding a new parameter to the URL and retrieving the POST data in Presenter::run() right after the redirect.

	const REQUEST_KEY = '_rid';

	public function redirectToRequest($key)
	{
		$session = $this->getSession('Nette.Application/requests');
		if (!isset($session[$key]) || ($session[$key][0] !== NULL && $session[$key][0] !== $this->getUser()->getId())) {
			return;
		}
		$request = clone $session[$key][1]; // the unset was removed
		$parameters = $request->getParameters();
		$parameters[self::FLASH_KEY] = $this->getParameter(self::FLASH_KEY);
		$parameters[self::REQUEST_KEY] = $key; // added magic parameter
		$request->setParameters($parameters);
		// $refUrl copied from Presenter::createRequest()
		$refUrl = new \Nette\Http\Url($this->httpRequest->getUrl());
		$refUrl->setPath($this->httpRequest->getUrl()->getScriptPath());
		$url = $this->router->constructUrl($request, $refUrl);
		$this->sendResponse(new \Nette\Application\RedirectResponse($url));
	}

	public function run(\Nette\Application\Request $request)
	{
		if (isset($request->getParameters()[self::REQUEST_KEY])) {
			$session = $this->getSession('Nette.Application/requests');
			$key = $request->getParameters()[self::REQUEST_KEY];
			if (isset($session[$key]) && ($session[$key][0] === NULL || $session[$key][0] === $this->getUser()->getId())) {
				$stored = clone $session[$key][1]; // no unset beacuse F5 would break, this however pollutes session
				/** @var \Nette\Application\Request $stored */
				if ($stored->getPresenterName() === $request->getPresenterName()) { // TODO: check action as well?
					// set the RESTORED flag
					$stored->setFlag(\Nette\Application\Request::RESTORED, TRUE);
					// preserve flash messages from the current request
					$parameters = $stored->getParameters();
					if (isset($request->getParameters()[self::FLASH_KEY])) {
						$parameters[self::FLASH_KEY] = $request->getParameters()[self::FLASH_KEY];
					} else {
						unset($parameters[self::FLASH_KEY]);
					}
					$stored->setParameters($parameters);
					// replace the actual request by the restored one
					$request = $stored;
				}
			}
		}
		// Continue with normal code of Presenter::run()
	}

The main issues of this solution that I expect people to dislike are the magical _rid parameter in URL when redirecting and the fact that the request is not removed from session to keep F5 working.

@DavidGrudl If you want feel free to use any part of this when creating a PR with RequestStorage.

Appendix

This is meant more as a warning for other people than as a suggestion for a change. When a request is stored and later restored, it's not safe to immediately do things like updating your database. The original request that was stored might have been triggered by someone else from the same computer when the real user was absent. Nette\Application\UI\Form is partially protected from this because even if the form recieves a submit signal, it won't fire its events if the request is a restored one. I said partially because this protection is lost when GET method is used. In that case redirect happens and the RESTORED flag is lost.

However other things than forms (like signals for deletion) are most probably not safe from this in your application. I believe that database modifications should almost always be done in signals. In fact I even consider this a best-practice. Therefore I went as far as not calling PresenterComponent::signalReceived() at all for restored requests. Here is what I use in my Presenter.

	public function processSignal()
	{
		if (!$this->request->hasFlag(Request::RESTORED)) {
			parent::processSignal();
		}
	}

I'm not sure if such change should be in Nette itself though. I'd like to hear your opinion about this issue as well.

---

Finally I'd like to thank @castamir for being the first to read this RFC and helping me greatly improve the text. :-)

Last edited by enumag (2015-02-25 20:54)