consult token rather than session data in FeaturesController

Can somebody explain more of the rational behind this patch? It seemed to work in the past so is there a problem using the \Drupal::request()->getSession(); Can somebody point me to the documentation for D8 session handling?

All Features is trying to do here is pass the name of the archive being downloaded via the Session data. While this patch seems to change how the data is retrieved from the session, it doesn't address how the 'features_download' filename was originally saved to the session.

Requiring a full request stack seems overkill for just passing data via the session.

But my issue is that your patch only fixes a single call to \Drupal::request(). If you search the Features folder, you'll see dozens of other calls to set/get data via the session. Adding the request stack to all of these other class constructors just really seems overly complicated.

The comment in the docs about not using \Drupal::request() refer to issues around only working for HTTP requests, which is fine in the case of Features because we are only using it to store data for the UI. And we never store the request as warned in the docs.

Even core issues like #2473875: Convert uses of $_SESSION to symfony session retrieved from the request show replacing $_SESSION[] with \Drupal::request()->getSession(). So until I see something more conclusive that documents the best practice for replacing $_SESSION[] in D8, I'm going to stick with what we have.

There are several generic advantages of using non global calls (because this is what this basically is)

1. You can understand a lot about the class by just looking at the constructor.
2. You see easier when a class does too much, aka. has too many dependencies.
3. Its easier to unit test this specific class

Note: The right replacement would have been:

public function downloadExport(Request $request) {
  $session = $request->getSession();
}

Note2: What is the reason to not pass along the filename as parameter in the URL?

What is the reason to not pass along the filename as parameter in the URL?

Um, maybe because I hadn't read the documentation? :)

Um, maybe because I hadn't read the documentation? :)

Cool, that somehow seems way better ...

I looked at this a bit more and recalled that I did try passing a route parameter when I first wrote this. The problem is that in features_file_download() we need to know which file name we're providing access to. We can't just read that information from the URI, since that would open up to all temp files.

Is there a better way than the session to provide this file name information? If not, I'm thinking we do need the session and should use the approach in #6. While we're at it, it would be worth looking at the related \Drupal::request()->getSession() calls in FeaturesGenerationArchive::exportFormSubmit() and features_file_download().

Something like this could work for example.

+++ b/features.module
@@ -28,14 +28,10 @@ function features_help($route_name, RouteMatchInterface $route_match) {
 function features_file_download($uri) {
   $scheme = file_uri_scheme($uri);
   $target = file_uri_target($uri);
-  $session = \Drupal::request()->getSession();
-  if (isset($session)) {
-    $archive_name = $session->get('features_download');
-    if ($scheme == 'temporary' && $target == $archive_name) {
-      return array(
-        'Content-disposition' => 'attachment; filename="' . $archive_name . '"',
-      );
-    }
+  if ($scheme == 'temporary' && $target) {
+    return array(
+      'Content-disposition' => 'attachment; filename="' . $target . '"',
+    );

That's pretty much what I started to do, but backed out of it. My concern is that, by accepting any value of $uri, we're supporting requests to any temporary file (any value of $target), rather than just ones we have created.

Compare these lines in config_file_download(), where access is granted only to the specific file:

function config_file_download($uri) {
  $scheme = file_uri_scheme($uri);
  $target = file_uri_target($uri);
  if ($scheme == 'temporary' && $target == 'config.tar.gz') {

That's the reason I used the session: to be able to pass in the file name, which varies.

Is this a concern?

That's pretty much what I started to do, but backed out of it. My concern is that, by accepting any value of $uri, we're supporting requests to any temporary file (any value of $target), rather than just ones we have created.

An alternative could be to pass along a token as well which we can validate?

An alternative could be to pass along a token as well which we can validate?

That sounds right.

Yeah I thought this would be trivial to achieve, see the interdiff, but sadly this resulted into #2630920: _csrf_token is broken due to cacheability metadata integration, results in rendered links without valid CSRF tokens

.

That tiny interdiff in #14 would have been too easy apparently ;) Thanks for filing the core bug. The workaround meantime make sense.