@asm89 looks like a good concept. You should improve you PR comment and code.

What about HInclude, it seems the code has been removed ? (I think I understand but the reason why butit should be mentionned in your post)

Furthermore, if you do not use ESI, _internal uris will never be generated!

Wrong, HInclude. And we don't really care if they are generated or not. The important thing is whether they are routed or not.

Finally there are also other ways to make sure that _internal urls are only requested by your reverse proxy (e.g. Varnish) and are not accessible for the outside world.

Not everybody use a RP & HInclude must be accessible to the outside world.

And one last but important q, why would using a controller be better than using a path ?

@vicb the main point is ease of use. that being said, i doubt that @asm89 would be working on this feature if he wouldn't be concerned about legacy code.

@lsmith77 I don't really get your point here. @asm89 asks for feedback and I give some.
ease of use ? Using a named route should be as easy as using a controller name.

@vicb: i was replying to the question "And one last but important q, why would using a controller be better than using a path ?" not having to define a route is easier than having to define one, at least initially because is one required step less, since often render calls are to controllers that are otherwise not supposed to be accessible .. there are of course plenty of cases where using a route is better and more flexible.

Ok, thanks for clarification. In fact I'd like to know if asm89 as any other reason in mind (ie something we would have missed)

Hiya guys!

I'm also interested in this, for the same as @lsmith77 mentioned - usability, i.e. not needing to create+secure a route that's only used for a non-standalone render. That's especially important for developers that don't use ESI or HInclude, but just come across features that require them to need to render another controller.

I don't care as much about the implementation - I would be ok with or without needing to create routes to use ESI/HInclude, as long as you didn't need to create (and secure) a route to use a non-standalone render.

The internal URL stuff is convenient for people using ESI/HInclude, but it's also a bit magical, requires some extra "teaching" for people to understand how it works. In that way, I don't mind the route for those cases.

Thanks!

From a Drupal perspective, the most important usability factor is not having to manually create additional routes for each thing that you could non-subrequest-access (ESI, etc.). We're still working toward making the entire page mostly partial-based, and not having to replicate every partial we could possibly have as a unique route would make life a lot easier.

I'm unclear how the signing would work for hInclude, though. Since that's always coming from the browser, what exactly is the benefit?

@Crell avoiding links forged by the client by modifying the url. If the client modifies the target code, he won't be able to provide the update hash

@vicb

What about HInclude, it seems the code has been removed ? (I think I understand but the reason why butit should be > mentionned in your post)

With signed uris HInclude could in theory stay, but it's probably still a bad idea. There could be another way of rendering hinclude tags with routes imo.

Not everybody use a RP & HInclude must be accessible to the outside world.

Yes, so adding HInclude was one of the things that fully opened up a security hole it seems. It would be nice however to attempt to fix it. :)

And one last but important q, why would using a controller be better than using a path ?

Not having to create a route for every single controller action? Why should a controller action be tied to the routing system? If you rely on rendering partials, having to add a route for all of them is a real dealbreaker imo (not to mention each partial now has to generate a url, go through the url matcher etc).

The point is that the creation of _internal urls and the interpretation of them was broken, but that doesn't mean that rendering partials "{% render 'BlogBundle:Post:list' with { 'limit': 2} %}" is suddenly a bad idea. Is it really worth the BC break in a 2.2 version?

@fabpot Any thoughts?

@asm89 thanks for adding details I think were missing from your original PR.

I wish the severity of the original issue would have been understood sooner & faster by the core team (I include myself here) so that we might have found a better solution.

@vicb Yes, but since 2.2 is not out yet the complete removal of the feature could still be reverted?

"reverted" is probably not a word I would use here.

2.2 is supposed to be feature complete sometime next week & entering a 2 month "stabilization" phase. Strictly speaking the modification you request is beyond the scope of the stabilization phase... So I think only @fabpot could answer your question.

I spoke to several people about the removal of the _internal routes (some from Symfony and others from the eZpublish and Drupal projects). It is clear that people liked the old way, especially those who rely on this feature heavily for internal sub-requests only (no ESI, no hinclude).

I decided to remove these internal routes because of the big security issue they allow when not used properly.

However, the proposal in this PR sounds good, but instead of reverting and applying this patch, I'd like to first finish and merge PR #6459 because it will then be much easier to implement what you propose selectively (depending on the rendering strategy). For instance, for the default and ESI rendering strategies, we can automatically reject any request not coming from localhost or trusted proxies; and use a signed URI for Hinclude.

@fabpot @vicb #6459 would make it easier to add different rendering strategies indeed, this is very nice. Will it however also re-introduce the {% render 'BlogBundle:Post:list' with { 'limit': 2} %} syntax?

As the signature for the tag/function should be the same for everything, I was thinking about adding a new controller function to support controller names:

{% render url('path') %}
{% render path('path') %}
{% render controller('BlogBundle:Post:list', { 'limit': 2}) %}

@fabpot I don't understand how url() and path() would differ there. Having controller() be a separate op makes sense to me, but how would that work outside of Twig? (Drupal's unlikely to be calling HttpKernel::render() from in a Twig file, more likely from its own PHP code.)

@asm89 I have been thinking a little more about this PR.

I think that having 2 different paths (secure vs HSI) is a must have.

However I am not really comfortable with URL signing. Your app will still be exposed to brute force attacks. There could be an other simple solution that was exposed to @fabpot and @schmittjoh in my initial threat report. Hopefuly I can get feedback soon. The final solution might be a conjuction of both signing & this - I am sorry to be vague here but I think not giving too many details is preferable.

@fabpot In that case, maybe render could still behave as it did in 2.0 and 2.1 and render_uri could be added for the now options? Is it really worth breaking BC on this?

@vicb If you're comfortable with it you can e-mail me about this too (iam.asm89 [at] gma...). While I tend to agree with the general feeling that the app will be exposed to brute force attacks when using url signing, in the end this isn't different from your remember_me cookie being vulnerable for these kind of attacks (see TokenBasedRememberMeServices). Apart from that it's still possible to secure the _internal route with for example varnish configuration.

@asm89 the difference being that remember_me is optional and you would probably not enable the default implementation if you need a strong security.

@asm89 and brute-forcing an URL is much more powerull for an hacker: you can get access to the DB, file system, ...

see #6459 for an implementation of the signing strategy.

Closing in favor of #6459