RequestFactory: invalid byte sequences in $_SERVER['REQUEST_URI']

[JanTvrdik]

The goal is to not simply ignore invalid byte sequences in URL, but throw Exception or return NULL. Current Nette behavior is unwanted, e.g. these URLs should not be accepted at all:

http://doc.nette.org/en/2.2/%C0%C0%C0%C0%C0templating
https://www.rohlik.cz/stranka/%C0kontakt

---

How would you prefer to handle it? Exception or by returning NULL. Both break the RequestFactory API. I would prefer the exception, e. g. Nette\Http\InvalidRequestException. Do you like the name?

cc @fprochazka

[fprochazka]

Exception is fine with me. But the main problem here is how to present the error to the user. If I visit such URL I definitely shouldn't see error page generated by Tracy. It should fall to error presenter - I don't wanna have log full of bluescreens - I want to have one string line in access.log. But that is a problem, because you can access the request in bootstrap, custom scripts or just simply somewhere else, before the app hits Application::processRequest().

One thing that crosses my mind is to return instance of Http\InvalidRequest that would provide backwards compatibility, the control characters could be removed from the url in this request and when such Http\InvalidRequest hits Application, it could directly forward to ErrorPresenter, without even hiting the router.

[JanTvrdik]

you can access the request in bootstrap, custom scripts or just simply somewhere else

Then it should be your job to handle it. But it is obviously a nasty BC break.

return instance of Http\InvalidRequest that would provide backwards compatibility

👎 That does not feel right.

---

Another think to consider – if you choose to change the API and throw exception, we may also reconsider how we handle control characters (note: that is something different than invalid encoding which is what the exception is primary for). Currently we ignore keys with control characters and discard values with control characters.

[fprochazka]

The whole problem is that the usage of Http\Request in Nette\Application (the api) is really bad.

The api should look like this

$app = $container->getByType(Nette\Application\Application::class);
$app->handle($container->getByType(Nette\Http\RequestFactory::class)->create());

Then, you have perfect control of when the request is created and what you do with it.

$app = $container->getByType(Nette\Application\Application::class);
try {
    $app->handle($container->getByType(Nette\Http\RequestFactory::class)->create());
} catch (Nette\Http\InvalidRequestException $e) {
    $app->handleInvalidRequest($e); // this could for example open the error presenter
}

In the end, the entire thing could be hidden in $app->run()

public function run()
{
    try {
        $this->handle($this->httpFactory->create());
    } catch (Nette\Http\InvalidRequestException $e) {
        $this->handleInvalidRequest($e); // this could for example open the error presenter
    }
}

At this point, you're gaining the possibility to handle the exception created by invalid request in appropriate level of abstraction. It should either be rejected by the http server (nginx), or it should be handled by the front controller as bad request and error presenter should be shown.

Having Http\Request as a service is bad IMHO.

The service Http\Request should be replaced with Http\RequestStack, with BC compatible methods and it should always return the values of the last Http\Request. But the point is, you get a "singleton" container, just like the User is for Identity and the Http\Request can be changed how you please. This would also provide possibility of better handling requests in applications that are controlled by react.php

---

Symfony has much better front controller API ... We should investigate how Symfony and other frameworks handle this situation and then decide what is best for us.

[JanTvrdik]

So Http\RequestStack would be (for most practical purposes) something like Nullable<Http\Request> in C#?

[fprochazka]

I was thinking something like

class RequestStack implements IRequest
{
    private $requests = [];
    private $lastRequest;

    public function append(IRequestFactory $factory)
    {
        $this->lastRequest = $factory->create();
        $this->requests[] = $this->lastRequest;
        return $this->lastRequest;
    }

    public function getUrl()
    {
        return $this->lastRequest->getUrl();
    }

    // ...
}

How will the stack be handled is a detail, it could even be container only for the lastRequest. The point is, that we must have a container for the Request object, it should have never been a service (just like Identity is also not a service).

[hrach]

+1 Only thing which bothers me is not logging this exception. Don't worry about making the bc break. Can't imagine how somehow would create theses malformed urls intentionaly.

[JanTvrdik]

Related – I would also like to change how we handle invalid Host header. Currently it is simply ignored.

[JanTvrdik]

@fprochazka I look at the RequestStack in Symfony. It was introduced in Symfony 2.4 (a year ago). Explanation of the motivation for that decision is available in a blogpost and documentation.

It seems to be the best solution to the problem. Thoughts? cc @dg

---

Note: Nette needs the RequestStack much less than Symfony because we already have Application\Request with Application\Request's stack managed by Application\Application.

---

Maybe – just maybe – we may 1) remove Http\Request from DIC 2) Move Application\Application::$requests to Application\RequestStack and put that in the DIC. 3) Add optional $httpRequest parameter to Application\Request. Is that stupid? I think that (1) and (2) are fine, not really sure about (3) though.

[fprochazka]

@JanTvrdik well, beeing the one suggesting it, I hardly can disagree :)

[dg]

RequestStack is not suitable for Nette, because it solves a different problem (as @JanTvrdik said, Nette has stack of Application\Request). When you need to create Http\Request inside Application, Nette has native solution: interface & generated factory.

[fprochazka]

@dg I'm not sure you understand the problem. The problem is that Http\Request is a value object not a service. Having it as a service is wrong and it's causing problems. That's why new service that will contain the Request object should be introduced. Where is it created and how it's handled is a detail.

[dg]

@fprochazka I understand.

(btw, value object can be service.)

[fprochazka]

Well obviousely, but that doesn't make it right.

Identity is not a service.

[dg]

"Identity is not a service → value object as a service is wrong" is fallacy of the converse.

[fprochazka]

The Http\Request is an immutable value object (FYI it should probably clone the url it returns from getUrl, it might be changed otherwise). We have now here two ways to solve this problem

1. The DIC in Nette handles scopes and can drop instances of services, that can change when next request occurs. This also means it knows the entire graph of services that has to be dropped when the Http\Request service is changed - this concept is known to all advanced DIC containers in all advanced languages, like C# and If I'm not mistaken, even Symfony has now scopes in DIC
2. You introduce container object for the value object, that will then be service instead - which is obviousely much simpler solution

Because Nette has no scopes in DIC, having value objects as a service is wrong. Simple as that.

[dg]

@fprochazka in czech. Prove that for each value object is wrong to have it as a service.

[fprochazka]

I don't want to say "there isn't a case where having value object as a service is a good thing", but I can't think of any.

I also don't have to prove it (at least not about Http\Request), because the symfony community has already proven that having request VO as a service was a bad decision by practise and therefore I'm applying that findings on Nette. It's exactly the same case. You prove them (and every other high level framework in other languages that came to the same conclusion) wrong :)

In other case (there are two, the specific one with Http\Request and my general statement about VO as service beeing wrong, which is my opinion not a fact), you prove me wrong by counter example (or other weapon of your choise) - where is it a good thing to have a VO directly as a service in DIC? I've never stated "for every" but if you wanna generalize, let me correct myself:

Because Nette has no scopes mechanizm in DIC, having MOST value objects as services is wrong.

Also about the Identity example, that you like to repeat instead of continuing in the discussion, I haven't stated that there is an implication, it was just a very good example, because it's the most obviouse one.

[hrach]

Basically, DIC should almost always hold only services, not the entities, crates, etc. This is the general rule, which also aplies here.

[JanTvrdik]

Regarding Http\RequestStack – could anyone describe a real-world use-case where it would hold more than a single Http\Request instance? I was originally thinking about React.PHP but even there it makes no sense, because you would actually need a queue not a stack.

[fprochazka]

Stack makes sense with subrequests in symfony. They don't have snippets and controls. They have subrequests (recursive calling of controllers and their output is then composed into a single template), which can be converted to proxy-level subrequests, which makes caching them a reall bliss.

I don't think that makes much sense with Nette, implementing this would be probably hard. Stack might make sense when you have forwarded requests. But I'm not sure.

[dg]

@fprochazka You said that the value object cannot be a service, which is a strong & general statement, but you cannot prove it (the fact that some value objects cannot be used as services is really not an evidence, see fallacy of the converse).

I would prefer to end debate about VO.

[fprochazka]

Would you preffer to end it at this issue (because it's a little bit off topic), or end it all together? Because I don't think the debate is over, because you haven't conviced me and neither have I convinced you.

[dg]

Here. It is off topic.