Problem/Motivation
Public follow up to improve tests and documentation after SA-CORE-2023-005 was released.
Redacted copy paste below, some items might not be relevant anymore:
+++ b/core/assets/scaffold/files/default.settings.php
@@ -531,6 +531,25 @@
+# $settings['file_sa_core_2023_005_schemes'] = ['porcelain'];
The default value of "porcelain" confused me at first because I thought it was a meaningful technical term (as in "porcelain diff" in git). After re-reading the docblock I realize it is just example code. The docblock does not make it clear that "porcelain" is just an example of a totally custom scheme -- it's implied from the use of the words "china" and "plate", but not stated explicitly, and might be especially difficult for folks who don't have English as a first language to grasp.
It might be better for it to be "example" or something? With the full path as example://example_path/filename.png?
+++ b/core/modules/file/tests/src/Functional/FileAccessTest.php
@@ -0,0 +1,94 @@
+ // treat '#' as separating a fragment. Do not test '?' since that is not a
+ // valid filename character on Windows.
We could conditionally test ? on not-Windows? Or test that ? is disallowed? It's unclear to me whether or not we expect ? to not be in the data at this point or if we are just ignoring it for the test because we want environment-agnostic tests.
+++ b/core/modules/file/tests/src/Functional/FileAccessTest.php
@@ -0,0 +1,94 @@
+ // must also include the script prefix, because Apache is configured to not
Over 80 chars.
+++ b/core/modules/file/tests/src/Kernel/PathTraversalTest.php
@@ -0,0 +1,61 @@
+ * The file path to test, including the stream wrapper scheme prefix (e.g.
+ * 'public://foo/bar.txt'.
+++ b/core/tests/Drupal/KernelTests/Core/StreamWrapper/StreamWrapperManagerTest.php
@@ -60,4 +91,99 @@ public function providerTestUriScheme() {
+ * The file path to test, including the stream wrapper scheme prefix (e.g.
+ * 'public://foo/bar.txt'.
Unclosed parenthesis.
+++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
@@ -123,8 +137,13 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
+ // Previous versions of the code generated the URL with a token based on the
+ // URI before it was normalized. Those URLs may be cached, so consider the
+ // token valid if it matches one based on the original or normalized URI.
+ // @see Drupal\image\Entity\ImageStyle::buildUrl().
$token = $request->query->get(IMAGE_DERIVATIVE_TOKEN, '');
- $token_is_valid = hash_equals($image_style->getPathToken($image_uri), $token);
+ $token_is_valid = hash_equals($image_style->getPathToken($image_uri), $token)
+ || hash_equals($image_style->getPathToken($scheme . '://' . $target), $token);
My kneejerk reading the code comment here was "We should just be invalidating old caches rather than supporting them" -- is "cache" the right word here?
+++ b/core/tests/Drupal/KernelTests/Core/StreamWrapper/StreamWrapperManagerTest.php
@@ -23,6 +27,22 @@ class StreamWrapperManagerTest extends KernelTestBase {
+ protected $scheme = 'dummy-external-readonly';
...
+ protected $classname = 'Drupal\file_test\StreamWrapper\DummyExternalReadOnlyWrapper';
"Dummy" is an ableist word.
+++ b/core/tests/Drupal/KernelTests/Core/StreamWrapper/StreamWrapperManagerTest.php
@@ -60,4 +91,99 @@ public function providerTestUriScheme() {
+ * Tests that normalizeUri resolves relative paths.
...
+ * Tests that normalizeUri exceptions apply only to local schemes.
::normalizeUr() should have parens and at least a double colon prefix since it is not a global function; probably the class name also.
+++ b/core/tests/Drupal/KernelTests/Core/StreamWrapper/StreamWrapperNormalizationTestTrait.php
@@ -0,0 +1,272 @@
+ * DataProvider for relative path tests.
"Data provider", not "DataProvider".
+++ b/core/tests/Drupal/KernelTests/Core/StreamWrapper/StreamWrapperNormalizationTestTrait.php
@@ -0,0 +1,272 @@
+ // We include the query and fragment tests from 5.4.2
+ // however query('?') and fragment('#') characters are not subject to
+ // special processing as part of a filesystem path. We deviate from the
+ // RFC examples by validating normalization is done after these
+ // characters in a file path.
Very minor issue with early wrapping, a double space after the period, and missing punctuation;
+++ b/core/assets/scaffold/files/default.settings.php
@@ -531,6 +531,25 @@
+ * File schemes whose paths should not be normalized:
This should end with a period, not a colon, but this is very unimportant.
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Background information
