Using proxy headers to determine base url

[danmichaelo]

I know you can set skosmos:baseUrl manually in config.ttl, but I want to avoid that since I'm trying to use the same config file in testing and production.

It would be nice if Skosmos could determine base url based on proxy headers (X-Forwarded-Proto and Host) in the same way as Symfony does.

---

Rather than duplicating that functionality, I tried puling in symfony/http-foundation and using it: danmichaelo@5bfa6cf

But if we add this new dependency, it immediately seems like it would make sense to also integrate the Request class of Symfony with the Request class of Skosmos. Either by having the Skosmos class extend the Symfony one, or by wrapping it. Then it would be possible to gradually utilize more and more of the functionality provided by Symfony.

But then we need a way to get the Request object into the guessBaseHref() method in Controller. Which turned out to be harder than i first imagined! Some options:

• Add it as an argument: guessBaseHref(Request $request). Positive: Makes it explicit that the method depends on the request. Does not add state. Negative: getBaseHref() is called from many places, including a few methods in EntityController that do not have $request in their scope now, so we would have to add that too. And getBaseHref() is even called from the WebController constructor... which leads to the next option:
• Add it to the Controller constructor: __construct(Model $model, Request $request). Positive: Easy to implement. Negative: More state. And need to rewrite all the controller methods to use $this->request rather than take $request as an argument. If that's even possible. It seems like the object is mutated in index.php before being passed to the controller methods, so it might not work to just inject it into the constructor. Hm. Since it's passed by reference, it might still work, but then it becomes increasingly hard to trace what happens :/
• A third option could be to follow the first option, but avoid having to pass the request into the WebController constructor by extracting the Twig initialization stuff into a new method initTwig(Response $response) or something like that, which is called from alle the class methods. Positive: Perhaps the cleaner approach? Negative: Lots of refactoring?

---

At this point I found it a good idea to stop and ask: Does it make sense to start integrating http-foundation's Request class with Skosmos' at all? Or skip integration, but still use http-foundation and just initialize it when needed? Or not use http-foundation at all, and just add a new method to Controller that checks for the proxy headers?

At this point I'm leaning slightly towards the last option given that it seems rather difficult to refactor the current code, but I'm not sure.

[osma]

Thanks for the idea and your thorough analysis.

I haven't really used Symfony myself, though I believe Skosmos already depends on parts of it indirectly (I recall seeing that some symfony-related things get installed by Composer). We've been wary of big frameworks so far, because building on them easily leads to an upgrade treadmill, the need to watch out for security vulnerabilities and the like. So my first preference would be your last option too - add just the minimum new functionality to check for proxy headers. But if that's a nontrivial amount of code, I'm willing to reconsider.

[danmichaelo]

Symfony has become very modular in the latest version, so it would be possible to use the http-foundation package without the rest of Symfony, but I backed off from it given the complexity of integrating it.

The way this functionality is implemented in the Symfony package, you specify a list of proxy IPs or IP ranges that you want to trust. I think we can do with just a boolean though: Either you have a proxy that adds X-Forwarded-Proto and X-Forwarded-Host or you don't, so I added the skosmos:trustProxyHeaders as a boolean value. As long as all requests go through the proxy and the proxy always sets X-Forwarded-Proto and X-Forwarded-Host (overwriting any spoofed user headers), I don't think there should be a risk of cache poisoning. Express also allows a boolean value (https://expressjs.com/en/guide/behind-proxies.html).

Update: Having read about the difference between SERVER_NAME and HTTP_HOST, I'm having second thoughts about trusting the X-Forwarded-Host here. We don't trust HTTP_HOST, so we should probably not trust HTTP_X_FORWARDED_HOST either (my assumption that proxies in general overwrites it was wrong).

Note that the X-Forwarded headers have been attempted standardized as Forwarded. The new header is a bit more difficult to parse and it seems like it hasn't gained much traction yet, so I didn't add support for it, but it might be something to consider later.

(And the added test only tests that the configuration option can be set, not that it does what it should do, but I didn't find an easy way to test that..)

[osma]

@danmichaelo Are you happy with the current state of affairs after PR #911 was merged? Can we close this issue?

[osma]

Closing this as I assume it's now done. Please reopen (or even better, open a new issue) if necessary.

[danmichaelo]

Thanks, yes, I already internally considered this closed :)