Provide a setting to override base url when creating links to public files.

Because this is a security improvement, I think it's ok to still do for 8.0, though if it doesn't get done in time, it can also go into 8.1.

Provide a config setting...Possibly a configuration option

I disagree with making it config. I think just a settings.php setting is enough. Just like file_public_path. This means it can be shown in the UI, but not edited. See FileSystemForm::buildForm().

The decision to use settings instead of config for file_public_path was made in #1856766: Convert file_public_path to the new settings system (make it not configurable via UI or YAML). Not sure that the reasons there necessarily apply to whatever we name this new thing (file_public_host?), but I think the two variables should be in the same system (i.e., either both config or both settings, but not one of each).

Hi,

I have found this security issue.

Thanks Drupal for fix this issue.

regards

Why not just make this an alter hook instead? It feels like we're limiting ourselves with what can be done.

Couldn't this also be accomplished by using hook_file_url_alter(), especially since the example code in that hook is exactly this use case?

Hrm, it just feels like we're investing a whole other thing that duplicates something already in core. The UI addition is good, but could also be made to work with the result of hook_file_url_alter().

I think this is different than hook_file_url_alter(), because hook_file_url_alter() does something a little different than its name implies. hook_file_url_alter() is for altering the URI of a file prior to the URI being converted to a URL. So, for example, a CDN module could use the hook to alter public://foo.txt to my-cdn://foo.txt and then let the my-cdn stream wrapper decide how to map that to an external URL (via getExternalUrl()). Of course, http and https URIs are also URLs, so hook_file_url_alter() can also be used to alter public://foo.txt directly to http://.../foo.txt, and you're right that the documented example implementation of hook_file_url_alter() does that, which perhaps isn't ideal.

So I don't think the use-case here should be implemented via hook_file_url_alter(), because then we'd lose the information that it's still a public:// file. For example, suppose a CDN module ran at a higher weight, then our implementation would turn public://foo.txt to http://internal-not-cdn.subdomain/foo.txt and the CDN module wouldn't then recognize this as something that should be further mapped to a CDN. In other words, I think as much as possible, hook_file_url_alter() should keep $uri as a URI and defer conversion to URL to the stream wrapper.

But, I think using a replacement pattern here is overkill. I think the setting should be an exact base URL, like file_public_base_url, and if someone needs it to be dynamic, then they can implement the code to make it so. Note that it can be a protocol-relative base URL, and probably should be in the commented out example we put into default.settings.php.

Your settings.php needs to be different on dev and prod anyway, due to $databases. And if you're smart enough to make $databases vary by environment via PHP code in the settings.php or its includes, then you can figure out how to do the same for $settings['file_public_base_url']. Same for multisite.

E.g., you can make $settings['file_public_base_url'] an object that implements a __toString() method if you want to make it dependent on $base_url; not sure if default.settings.php should mention that in a comment or not.

#13: the URI vs URL distinction is technically correct, but Drupal 7 didn't make that distinction. It was just "file URLs", period. The CDN module's hook_file_url_alter() implementation does receive a file URI and then returns a file URL. It doesn't use stream wrappers.

So I don't think the use-case here should be implemented via hook_file_url_alter(), because then we'd lose the information that it's still a public:// file.

This makes a ton of sense. And is the best justification for #10, and answers the concerns in #11 by Dave Reid.

Code review

Overall, +1, thanks to the excellent rationale provided by @effulgentsia.

1. +++ b/core/lib/Drupal/Core/StreamWrapper/PublicStream.php
   @@ -21,6 +21,20 @@
   +   * The instance base path.
   ...
   +   * Instance public base URL with scheme.
   

   The mention of "Instance" here is very confusing/weird. What instance of what?

2. +++ b/sites/default/default.settings.php
   @@ -455,6 +455,18 @@
   + * accessing public files. This can be used for a simple CDN integration, or
   + * to improve security by serving user-uploaded files from a different domain
   

   Nit: 80 cols.

3. +++ b/sites/default/default.settings.php
   @@ -455,6 +455,18 @@
   + * or sub-domain pointing to the same server
   

   Nit: s/sub-domain/subdomain/ (I think?)
   Nit: Missing trailing period.

Still missing test coverage.

Do you think we can get rid of the configurable $base_url global in settings.php with this change?

Filed #2528988: Remove the option to specify a base_url from within settings.php.

Tests look good :)

1. +++ b/core/lib/Drupal/Core/StreamWrapper/PublicStream.php
   @@ -53,7 +53,25 @@ public function getDirectoryPath() {
   +  public static function baseUrl() {
   
   +++ b/core/modules/system/src/Form/FileSystemForm.php
   @@ -89,6 +89,13 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#markup' => PublicStream::baseUrl(),
   

   This means you're making it an API, but there's no interface. So if somebody provides alternative implementation of the stream_wrapper.public interface, this will break.

   Either needs to not be public, or needs an interface.

2. +++ b/core/modules/file/src/Tests/DownloadTest.php
   @@ -113,14 +113,25 @@ function testFileCreateUrl() {
   +    // Test public files with a different host name from settings.
   

   s/host name/base URL/

   (For consistency.)

+++ b/core/lib/Drupal/Core/StreamWrapper/PublicStream.php
@@ -53,7 +53,25 @@ public function getDirectoryPath() {
+    return static::baseUrl() . '/' . $this->getDirectoryPath() . '/' . UrlHelper::encodePath($path);
...
+++ b/sites/default/default.settings.php
@@ -455,6 +455,18 @@
+ * accessing public files. This can be used for a simple CDN integration, or

For simple CDN integration, we might not want the directory path automatically appended. Should we make the return value in the first line just baseUrl() . '/' . UrlHelper::encodePath($path), and require the setting, if specified, to include sites/default/files if that's wanted?

#27 is an excellent point.

Isn't that already supported by all the magic in: file_create_url() ?

See #13: this does it without needing to implement the alter hook and worry about how it plays with other implementations of the alter. Since it's a good security best practice, it's nice to support as a simple setting.

AFAICT all feedback has been addressed.

+++ b/core/lib/Drupal/Core/StreamWrapper/PublicStream.php
@@ -53,7 +53,29 @@ public function getDirectoryPath() {
+   *   The external base URL for public://

Nit: Missing trailing period. Can be fixed on commit.

Adding credit to @jplopezy for reporting the original security issue and to @Wim Leers for substantive reviews.

Pushed to 8.0.x! Thanks for the nice security hardening.

wtf, d.o?