Problem/Motivation

Install new module give AJAX error: 404 on /core/authorize.php/core/authorize.php

Problem exists on Nginx and PHP built-in webserver, but not with Apache.

Steps to reproduce:

0. Install Drupal somehow, if its installed change to its directly goto step 3
1. drush qd --profile=standard --core=drupal-8 -y d8rc1test
2. cd d8rc1test/drupal-8
3. drush rs /
4. Log in
5. click on "Extend"
6. click on "+ Install new module"
7. Paste in any module link, e.g. http://ftp.drupal.org/files/projects/devel-8.x-1.x-dev.tar.gz

Result:

Note that if you follow the same steps above in a browser with JavaScript disabled, you receive a similar "Page not found" error for core/authorize.php/core/authorize.php.

The reason that this error works on the Apache webserver is that, when Apache is processing "core/authorize.php/core/authorize.php", it evaluates each part of the path in sequence. When it notices that there is an executable file "authorize.php" at the relative location core/authorize.php, it executes that script, passing it the entire request URI, "core/authorize.php/core/authorize.php". Apache is therefore resilient to this error. Other web servers that treat the entire request as a distinct path, on the other hand, fail to find the authorize.php script and return a 404 instead.

Proposed resolution

The target URL is being correctly generated via `Url::fromUri('base:core/authorize.php');`, which behaves correctly; however, UnroutedUrlAssembler.buildLocalUrl() does not assemble the correct target URL in all cases.

Therefore it does

  • global $base_path is exposed on the request context: $erequestContext->indexPhpBasePath()
  • This method name was chosen to make it as explicit as possible and reduce a potential misunderstanding with the existing base path (which is the base path of the current front controller, which is identical in case of index.php, but not fore update.php/authorize.php
  • The URL asseembler then uses the global $base_path to generate the resulting URL
  • There used to be magic to add the front controller to the generated URL. This was removed as we now use ALWAYS the $base_path, and this is it.
  • Note: This lets base: behave in a consistent way aka. be relative to index.php instead of another front controller

Remaining tasks

Needs RTBC

Why this should be an RC target

.
This issue is not critical, and does not qualify for rc eligible. However, this fixes a serious problem in a core API that is widely used. The risk of changing the implementation of a widely-used core API needs to be balanced against the risk that this bug could easily manifest in other situations. Tracking down this problem is difficult. Also, if not fixed, some web servers, such as Nginx, will not work correctly with all Drupal 8 features.

User interface changes

None.

API changes

None. Fixes a flaw in an existing API which was not correctly fulfilling its contracts.

Data model changes

None.

CommentFileSizeAuthor
#94 interdiff.txt13.7 KBdawehner
#94 2583799-94.patch1.69 KBdawehner
#91 2583799-91.patch19.28 KBdawehner
#91 interdiff.txt4.13 KBdawehner
#89 2583799-89.patch10.83 KBgreg.1.anderson
#87 2583799-87.patch10.82 KBgreg.1.anderson
#86 2583799-86.patch9.06 KBgreg.1.anderson
#81 2583799-81.patch1.74 KBdawehner
#77 interdiff.txt762 bytesdawehner
#77 2583799-77.patch19.29 KBdawehner
#76 interdiff.txt3 KBdawehner
#76 2583799-75.patch19.24 KBdawehner
#69 interdiff-67-to-69.txt666 bytesgreg.1.anderson
#69 2583799-69.patch16.24 KBgreg.1.anderson
#67 interdiff.txt639 bytesdawehner
#67 2583799-67.patch16.24 KBdawehner
#64 2583799-64.patch15.62 KBdawehner
#41 install_new_module-2583799-41.patch8 KBgreg.1.anderson
#41 install_new_module-2583799-41-test.patch5.85 KBgreg.1.anderson
install_new_module-2583799-40.patch8.11 KBgreg.1.anderson
install_new_module-2583799-40-test.patch5.85 KBgreg.1.anderson
#37 install_new_module-2583799-37.patch7.88 KBgreg.1.anderson
#37 install_new_module-2583799-37-test.patch5.81 KBgreg.1.anderson
#35 install_new_module-2583799-35.patch5.81 KBgreg.1.anderson
#35 install_new_module-2583799-35-test.patch3.91 KBgreg.1.anderson
#26 install_new_module-2583799-26.patch5.85 KBgreg.1.anderson
#26 install_new_module-2583799-26-test.patch3.96 KBgreg.1.anderson
#25 install_new_module-2583799-22.patch2.74 KBcilefen
#25 install_new_module-2583799-22-test.patch856 bytescilefen
#13 drupal-buildLocalUrl-2583799-13-D8.patch1.9 KBgreg.1.anderson
Screen Shot 2015-10-08 at 10.03.41 AM.png55.3 KBgreg.1.anderson
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greg.1.anderson created an issue. See original summary.

cilefen’s picture

I could not reproduce this my local Apache server, but I could on the built-in PHP server running drush rs as described in the summary.

On the console for the built-in server there are lots of these:

[Thu Oct  8 15:27:50 2015] PHP Deprecated:  Automatically populating $HTTP_RAW_POST_DATA is deprecated and will be removed in a future version. To avoid this warning set 'always_populate_raw_post_data' to '-1' in php.ini and use the php://input stream instead. in Unknown on line 0
[Thu Oct  8 15:27:50 2015] PHP Warning:  Cannot modify header information - headers already sent in Unknown on line 0
greg.1.anderson’s picture

The commonality here might be non-Apache webservers.

dawehner’s picture

Issue summary: View changes

The problem is somehow that the URL generated for the batch request itself, not the initial request to /core/authorize.php looks like /core/authorize.php

Adapted the issue summary to make it even easier to reproduce.

cilefen’s picture

RandomSeb’s picture

I have this issue installing or updating modules through the UI, running nginx and drupal8-rc1 (redownloaded today).

I have found that the ajax call is wrong.. it is trying to call:

http://my.domain.com/core/authorize.php/core/authorize.php?batch=1&id=22...

/core/authorize.php/core/authorize.php

For whatever reason /core/authorize.php is being added to the ajax call twice.

I manually loaded:
http://my.domain.com/core/authorize.php/?batch=1&id=22&op=do_nojs&op=do&...

And it completed the operation successfully:
{"status":true,"percentage":"100","message":"Completed 2 of 2.","label":""}

I will now dig through the relevant code to try to find why it would be adding an extra /core/authorize.php (or perhaps it is just doing that and apache ignores the path error for whatever reason?)

RandomSeb’s picture

I solved this with a bandaid on nginx, in my web server config:

server {
   ...
    rewrite ^/core/authorize.php/core/authorize.php(.*) http://d8.testtesttesttest.com/core/authorize.php?$1 permanent;
   ...
}

(that domain is fake of course)

greg.1.anderson’s picture

It does appear that Drupal Core is requesting core/authorize.php/core/authorize.php, even when running under Apache. I guess Apache matches the first core/authorize.php against the file that exists at that location, and does not care that the extra core/authorize.php is there. nginx and the Apache built-in web server are not as accommodating, though.

greg.1.anderson’s picture

`system_authorized_batch_process()` generates the correct URL, and the first redirect to core/authorize.php is correct, but by the time we have reached core/authorize.php itself, the Batch URL contains `/core/authorize.php/core/authorize.php?batch=1&id=12&op=do_nojs`. Still not sure where this comes from.

cilefen’s picture

Issue tags: +Ajax, +JavaScript
greg.1.anderson’s picture

Issue tags: -Ajax, -JavaScript

This problem actually does not have anything to do with Ajax or Javascript. The same error occurs if you turn off JavaScript; the only difference is that you get a "page not found" for core/authorize.php/core/authorize.php instead of an Ajax error. This problem is therefore generally easier to trace with Javascript turned off.

The bad URL is returned from system_authorized_get_url():

function system_authorized_get_url(array $options = array()) {
  // core/authorize.php is an unrouted URL, so using the base: scheme is
  // the correct usage for this case.
  $url = Url::fromUri('base:core/authorize.php');
  $url_options = $url->getOptions();
  $url->setOptions($options + $url_options);
  return $url;
}

_batch_progress_page() attempts to generate the redirect url from $batch['url'], which was provided by the above function.

\Drupal\Core\Utility\UnroutedUrlAssembler.buildLocalUrl() does the actual assembly. The problem is that $request->getBasePath() is "/core/" , and $options['script'] is "authorize.php/"; since our current page request is core/authrorize.php, this is correct. The current uri at this point is "core/authorize.php", as provided. These things are all concatenated together, so it behaves as implemented, and core/authrorize.php/core/authrorize.php is generated.

Changing system_authorized_get_url() to Url::fromUri('base:/core/authorize.php'); (note additional leading "/") is ineffective, because buildLocalUrl() explicitly removes leading slashes. This is necessary for other use-cases for this code.

Ideally, we would like to see our URI in buildLocalUrl() be "base:" instead of "base:/core/authorize.php", but we cannot simply change system_authorized_get_url() to return this value, as system_authorized_get_url() is also used at the beginning of the request to redirect to core/authorize.php at the beginning of the batch process.

So, in short, I have found the problem, but do not yet have a simple solution that works without breaking other things.

greg.1.anderson’s picture

In #12, I started with the assumption that buildLocalUrl() behaved correctly, and that the solution would be to get the correct data to this function such that it produced the correct result. With further investigation of the data sent to this routine in different situations, it became clear that the target values were correct -- the callers know what URL they want to go to, this is easy -- the problem was that buildLocalUrl() was not correctly handling URL generation when (a) the target was a php script (e.g. authorize.php), and/or (b) the target was not at the Drupal root (e.g. /core/authorize.php).

The attached patch checks for both of these conditions, and compensates appropriately, so that the correct target URL is always generated. With this patch applied, both the Javascript and the no-Javascript cases work correctly, and it is possible to install modules through the update module on Nginx.

It seems that when this routine is functioning correctly, the $base_url is "/" and the script is empty. In other instances (e.g. core/authorize.php), the resulting URL doubles up the path in a way that happens to work on Apache, limiting the exposure of this bug. I have not found any instances where this bug manifest other than as reported here.

greg.1.anderson’s picture

Status: Active » Needs review
almaudoh’s picture

Issue tags: +Needs tests

So, in short, I have found the problem, but do not yet have a simple solution

Good work to find the problem. I've not really looked in-depth but I think this is where the problem lies:

$url = Url::fromUri('base:core/authorize.php');

Url::fromUri('base:....') should return a url based off the Drupal install path, irrespective of the location of the running script (whether index.php or core/authorize.php). It's behavior should be predictable.

greg.1.anderson’s picture

Title: Install new module give AJAX error: 404 on /core/authorize.php/core/authorize.php » Install new module exposes bug in UnroutedUrlAssembler.buildLocalUrl() resulting in duplicated paths in URLs, such as /core/authorize.php/core/authorize.php
Priority: Normal » Major
Issue summary: View changes
Status: Needs review » Needs work

Perhaps #15 was written prior to #13. Url::fromUri() is returning a data structure that is correct; it is UnroutedUrlAssembler.buildLocalUrl() that mis-renders it in certain situations. In any event, I appreciate the feedback.

The submitted patch still needs testing (and tests) covering other situations, including:

1) Installing Drupal in a subdirectory.

This probably works with the submitted patch unless the subdirectory is named "core". I need to confirm this, and possibly adjust the patch.

2) Using Drupal without clean URLs.

I have not tried this, but I suspect that installing modules through the update module's admin UI might fail without this patch. Adjustments to the fix here might also be necessary for this use case.

I hope to be able to spend more time on this later today or tomorrow, but collaboration appreciated.

I am upgrading the severity of this issue to "major", because buildLocalUrl() is an important core API, and it is currently not fulfilling its contracts correctly, to the detriments of platforms that should be supported by Drupal (Nginx, et. a.). This bug might impact other present behaviors not yet discovered, or cause problems with future code. I do not believe that it meets the criteria of a critical, though.

greg.1.anderson’s picture

Confirmed #13 also works on the php built-in web server.

greg.1.anderson’s picture

Confirmed #13 also works with non-clean URLs in a subdirectory, *except* that at the very last page, the links are not correct.

Starting URL:

http://test8.dev:7777/subsite/index.php/admin/modules/install

Links on confirmation page:

"Install another module" -> http://test8.dev:7777/subsite/admin/modules/install

This is probably because, in my patch, I necessarily need to redirect from '/subsite/index.php' to '/subsite/core/authorize.php', not '/subsite/index.php/core/authorize.php'. Once we are at '/subsite/core/authorize.php', buildLocalUrl() has no good way to know that it should go back to '/subsite/index.php/'. Not sure how the existing code was expected to work in this instance; it could be that I do not have my site configured correctly. i.e., I do not have $base_url set, as it appears this variable is set up automatically in Drupal 8.

greg.1.anderson’s picture

Ah, regarding #18, I see that clean URLs are required in Drupal 8, so the non-clean URL case does not need to be considered.

greg.1.anderson’s picture

Confirmed #13 works with clean URLs in a subdirectory, e.g. http://test8.dev:7777/subsite/admin/modules/install

cilefen’s picture

Since the cause was found to be outside of the extension system, should we change the component field on this issue to "base system" or maybe the "routing system"?

greg.1.anderson’s picture

Tried to install Drupal 8 into a subdirectory named "/core" (i.e. /path/to/drupal/core/core/authorize.php), and found that breaks everything including rendering of the home page, even without this patch installed. I did not investigate the cause or causes for this, but I do remember seeing at least one place where the string "core" is removed if it exists at the beginning of a URL.

Perhaps this is acceptable behavior (don't install Drupal 8 in a subdirectory named "core")?

greg.1.anderson’s picture

@cilefen: Yes, I also thought that changing the component to something else would be appropriate, but I did not see a component specifically for the URL component, which is in Drupal/Core/Utility. Although the symptoms of this bug have to do with routing, the bug only manifests for non-routed URLs, so I think that "routing system" is perhaps not the best choice, unless Drupal/Core/Utility is considered to be de-facto part of the routing system. Perhaps "base system" is most appropriate?

cilefen’s picture

Component: extension system » base system

Let's go with "base" then.

cilefen’s picture

greg.1.anderson’s picture

Thanks for writing some tests. I added another one that more closely mimics the calling environment of the original use-case where the failure was discovered.

cilefen’s picture

+++ b/core/tests/Drupal/Tests/Core/Utility/UnroutedUrlAssemblerTest.php
@@ -122,18 +122,22 @@ public function testAssembleWithLocalUri($uri, array $options, $subdir, $expecte
+      ['base:/bar/foo.php', ['script' => 'foo.php/'], TRUE, '', '/subdir/bar/foo.php'],
+      ['base:/bar/foo.php', ['script' => 'foo.php/'], FALSE, '', '/bar/foo.php'],

My options array with the script setting could be off the mark.

greg.1.anderson’s picture

Oh, didn't notice what you had done in $options there; just copied it over. That might indeed fail; I guess for now we'll just let the tests finish running and see how they come out.

greg.1.anderson’s picture

@clifen: Okay, your test got a fail and a pass, which is good. I think that the 'script' option is irrelevant here, but your test still covered about 50% of the change introduced in the patch, so it is a good test. I was going to change it to 'base:bar/foo.php', though (removing leading slash); the existing test for /drupal.org is testing to ensure that there is no doubled-up slash at the beginning of the generated URL if a leading slash appears on the input URL, but the recommended / expected / usual case here is no leading slash on "base:" URLs. I don't know if it's worth the time to take out the 'script' option and leading slash; might be slightly more correct to do that, but doesn't hurt anything as it is.

I didn't run my tests locally yet; we'll see if they come out with a fail & a pass as well.

cilefen’s picture

The quickest way to run it:

phpunit -c core --filter UnroutedUrlAssemblerTest

greg.1.anderson’s picture

Thanks for the phunit commandline -- helpful!

Looks like all of the tests are coming out as expected, and we have all of the necessary use cases covered. I'll rewrite the summery per https://www.drupal.org/issue-summaries and https://www.drupal.org/core/d8-allowed-changes to see if we can get an RTBC and 'rc target' tag on this.

greg.1.anderson’s picture

Issue summary: View changes
Issue tags: -Needs tests +rc target triage
greg.1.anderson’s picture

greg.1.anderson’s picture

Status: Needs review » Needs work
Issue tags: -rc target triage +Needs tests

I don't think the tests are testing what they are supposed to be testing. I will look at this a bit more tomorrow, and then update the status again.

greg.1.anderson’s picture

Neither my tests nor @cilefen's tests were duplicating the web server environment quite correctly. These new patches include improved tests with correct mocks, so the failing tests now look the same as what is observed in a web server.

I also reworked the fix in this patch for both of the behavior changes introduced.

1. The '/core/' directory is now explicitly removed. This mimics what is done in DrupalKernel.initializeRequestGlobals() when it sets up $base_path. There is some question about how this code should be factored; since DrupalKernel.initializeRequestGlobals() is deprecated, I decided to duplicate the functionality rather than introduce a dependency that will need to be cleaned up. Note that the construction of $base_secure_url and $base_insecure_url is also duplicated in UnroutedUrlAssembler.buildLocalUrl(). I also left this code unchanged, for the same reason previously stated, and also to minimize changes to the function being fixed.

2. I attempted a simpler fix for handling the $script component, but it did not correctly handle scripts such as update.php, so I went back to my original fix.

Tested successfully under the PHP built-in webserver, and using Nginx on the Pantheon platform.

greg.1.anderson’s picture

Issue summary: View changes

Updated summary. Described why this is not a problem under Apache, and updated the "Remaining tasks" section.

greg.1.anderson’s picture

Trying to re-upload my patches, as the TestBot was not picking them up before.

greg.1.anderson’s picture

#37 did not correctly account for scripts such as update.php, so I went back to my original solution in #41.

tim.plunkett’s picture

Status: Needs work » Needs review

Issue must be marked NR for the bot to care

greg.1.anderson’s picture

Thank you @tim.plunkett! Didn't notice that I forgot to do that.

greg.1.anderson’s picture

The testbot decided to queue and re-run all of the tests after I deleted #41, and re-uploaded a new copy of the issues. Note that #41-test.patch failed as expected, and the #41.patch passed.

alexpott’s picture

Issue tags: -rc target triage +rc target

Discussed with @catch and we agree that this is nasty and therefore should be committed during RC.

greg.1.anderson’s picture

Damien Tournoud provided some helpful advice on an alternate workaround:

I confirmed my assumption: nginx is running PCRE in greedy mode (which is the default),
so a fastcgi_split_path_info like:

fastcgi_split_path_info ^(.+\.php)(.*)$;

Will split at the last ".php" because the first ".+" repeater is greedy.

He suggested replacing the default with the following expression:

fastcgi_split_path_info ^(.+?\.php)(|/.*)$;

I was not able to get that to work myself. In any event, it would be helpful to get the RTBC'ed and committed, as this bug could potentially manifest in other scenarios as well.

populist’s picture

Status: Needs review » Reviewed & tested by the community

I think it is important that Drupal 8 works without requiring the kind of custom NGINX configuration as mentioned in #53. This is especially true for operations like adding modules through the GUI which are typically undertaken by users less inclined/able to do custom NGINX rules.

I went ahead and tested the patch in #41 using an NGINX web server. Without the patch, the user gets an error when trying to install a new module using the GUI. With the patch, the user gets the module installed (assuming the web server is writable).

dawehner’s picture

Here is a question, why can't we use global $base_path or rather move that kind of base path onto the request context and use it via there?

greg.1.anderson’s picture

@dawehner: I did not want to use the existing `global $base_path` because that code is marked as deprecated, and I did not want to add a new reference to it. There is already duplicated logic in this part of the code vis-a-vis the $base_path calculation, so it seemed better to do it the way that I did. Using `$base_path` would also be a perfectly valid solution, in my mind no better or worse.

The correct solution, of course, is the second solution you hint at: the request context should be correctly calculated, so that all code that relies on it can count on its validity. However, I was concerned about the potentially large impact such a change would have. I felt it was better to put off this change until 8.1, and make a lower-impact change that, while not pretty, at least makes the function calculate the correct result instead of an incorrect result.

It's down to the wire; I'm hoping that 8.0.0-stable will have a working UnroutedUrlAssembler rather than a broken one. I'm happy to switch to any technique that will get the job done, but at this point I don't think every option is feasible, given the limited time available.

dawehner’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
15.62 KB

Let's see whether this is it.

heykarthikwithu’s picture

+  /**
+   * Setes the index php base path.
+   *

spelling mistake "Sets".

Status: Needs review » Needs work

The last submitted patch, 64: 2583799-64.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
16.24 KB
639 bytes

Breaking fundamentals is problematic, always

Status: Needs review » Needs work

The last submitted patch, 67: 2583799-67.patch, failed testing.

greg.1.anderson’s picture

Make the parameter order in core.services.yml match the parameter order in UnroutedUrlAssembler constructor.

greg.1.anderson’s picture

Status: Needs work » Needs review

Nice bot tests patch.

The last submitted patch, 64: 2583799-64.patch, failed testing.

The last submitted patch, 67: 2583799-67.patch, failed testing.

greg.1.anderson’s picture

Status: Needs review » Needs work

I did some ad-hoc testing with #69. It does correctly allow the module installation feature to work; however, if you visit http://example.com/update.php, then the link on the "Continue" button is http://example.com/selection instead of http://example.com/update.php/selection.

The last submitted patch, 69: 2583799-69.patch, failed testing.

greg.1.anderson’s picture

The good news is that the number of failing tests is greatly reduced in #69, and those that are failing appear to be directly related to #73. e.g.:

fail: [Browser] Line 253 of core/modules/system/src/Tests/Update/UpdatePathTestBase.php:
Link Apply pending updates does not exist on http://localhost/checkout/selection

The test is failing because it ends up at http://localhost/checkout/selection instead of http://localhost/checkout/update.php/selection.

Investigating.

dawehner’s picture

Status: Needs work » Needs review
FileSize
19.24 KB
3 KB

This could solve like most of the issues

dawehner’s picture

Fixed the unit test for real.

dawehner’s picture

Issue summary: View changes

Worked on the issue summary ...

The last submitted patch, 76: 2583799-75.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 77: 2583799-77.patch, failed testing.

dawehner’s picture

FileSize
1.74 KB

This is a late in the night but total different approach patch.

greg.1.anderson’s picture

You've got a couple of var_dump's in your patch, but other than that, I am very hopeful about the outlook for this approach.

greg.1.anderson’s picture

I've only had a couple of minutes to do ad-hoc testing, but #81 is redirecting to core/core/authorized.php (got rid of an extra authorized.php, but an extra 'core' remains).

Maybe Url::fromUri('base:authorize.php', ['base_path' => trim($GLOBALS['base_path'], '/'), 'script' => '']); would do the trick; will try that next.

greg.1.anderson’s picture

#83 brings us to authorize.php (core/core removed), so that won't do either.

greg.1.anderson’s picture

On the subject of whether or not Drupal 8 requires clean URLs or not the page https://www.drupal.org/requirements/webserver says:

You can use the Apache 'mod_rewrite' extension to allow for clean URLs. Note that with Drupal 8, clean urls are enabled by default and can't be disabled, so mod_rewrite needs to be installed and enabled for Drupal 8 to work.

There is a similar disclaimer for Nginx. However, in the Requirements section if core/INSTALL.txt, it says:

OPTIONAL SERVER REQUIREMENTS
----------------------------

- If you want to use Drupal's "Clean URLs" feature on an Apache web server, you
  will need the mod_rewrite module and the ability to use local .htaccess
  files.

So, it would seem that the web page is probably wrong, and INSTALL.txt is probably right. Alex Pott said in IRC that clean URLs is not required.

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
9.06 KB

I changed my mind about #81. I do not think that I like it after all, because it relies on changing the way that the caller uses Url::fromUri(). While this has initial appeal in terms of minimizing impact, the larger problem is that the longer this problem goes unaddressed, the more places in the code we will need to fiddle with the parameters to Url::fromUri() to get the right result. #2548095: Add option to Url() to force the site base_url to be used also attempts to take this route, adding flags to the options; I think it is a mistake. Far better to ensure that all of the callers of this function are correct and consistent, and encapsulate the workarounds, as needed, inside the implementation.

#77 was the right idea. It corrected the inconsistency between how update.php and authorize.php expected Url::fromUri() to work. It also removed index.php from the resulting URL, a simplification it seems we are not ready to make (c.f. #85). I think that continuing with #77 will get us to the right solution, but it will need some work.

Now, the advantage to #41 is that all its tests pass, and it maintains the ability to run without clean URLs. Here is a new patch that combines the update.php consistency from #77 with the other parts of #41. Let's see if all of the tests pass with this patch, and then figure out where to go from here.

greg.1.anderson’s picture

One step at a time. There's no point in getting rid of the "/core/" hack in my patch until we get rid of it in the $base_url calculation. This patch enhances #86 to do just that. If these tests all pass, then I will go on to the next step.

Status: Needs review » Needs work

The last submitted patch, 87: 2583799-87.patch, failed testing.

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
10.83 KB

Fix typo in #87.

Status: Needs review » Needs work

The last submitted patch, 89: 2583799-89.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.13 KB
19.28 KB

I'm wondering whether we should explore more the old route ... to be honest, so what about doing something like this?

Status: Needs review » Needs work

The last submitted patch, 91: 2583799-91.patch, failed testing.

greg.1.anderson’s picture

I can agree to adding options to FromUri as long as:

1. $base_url or other globals do not need to be referenced when creating a URI
2. There can be a function such as system_authorized_get_url() that is called both from the standard front controller and a custom front controller (e.g. core/authorize.php), and the result is usable in either context without system_authorized_get_url() needing to know the context it was called from.

Looks like #91 satisfied both of those criteria.

The request context already has a reference to $base_url, so adding $base_path here does not make things any worse. This can be cleaned up at a later time.

I presume that 'urlj' should just be removed. I haven't had a chance to look at the failures yet, but if these can be fixed easily, then this is a good route to take.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.69 KB
13.7 KB

I presume that 'urlj' should just be removed. I haven't had a chance to look at the failures yet, but if these can be fixed easily, then this is a good route to take.

Oh yeah, this was just a temporary idea.

2. There can be a function such as system_authorized_get_url() that is called both from the standard front controller and a custom front controller (e.g. core/authorize.php), and the result is usable in either context without system_authorized_get_url() needing to know the context it was called from.

Yeah exactly that was the idea, even I still think, that adding the front controller by default is wrong, but this can be discussed in a follow up.

So I'm wondering whether the following patch is actually all we need?

Status: Needs review » Needs work

The last submitted patch, 94: 2583799-94.patch, failed testing.

greg.1.anderson’s picture

#94 is still redirecting to core/core/authorize.php.

greg.1.anderson’s picture

#91 works perfectly in ad-hoc testing. I did not confirm why the tests are failing in #91 and #94, or why #94 is not generating the correct result.

bskydive’s picture

bug are still present in drupal 8.0.5 + php7.0.5(zend 3.0.0)+ CentOS7+ nginx1.8.1 + php-fpm + postgreSQL9.5 with workaround in nginx.conf:
https://www.nginx.com/resources/wiki/start/topics/recipes/drupal/

Can't install via web-admin:
https://ftp.drupal.org/files/projects/drupal8_zymphonies_theme-8.x-1.0.t...

Error: 403 Forbidden nginx

after applying patch #91 drupal became broken:
*7 FastCGI sent in stderr: "PHP message: TypeError: Argument 2 passed to Drupal\Core\Utility\UnroutedUrlAssembler::__construct() must be an instance of Drupal\Core\Routing\RequestContext, instance of Drupal\Core\PathProcessor\PathProcessorManager given, called in /home/MhPtaxBy7/drupal/core/lib/Drupal/Component/DependencyInjection/Container.php on line 273 in /home/drupal/core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php on line 56" while reading response header from upstream, client: 192.168.1.111, server: vm.drupal, request: "GET /admin/theme/install HTTP/1.1", upstream: "fastcgi://unix:/var/run/php-fpm.sock:", host: "vm.drupal", referrer: "http://vm.drupal/admin/appearance"

xjm’s picture

Issue tags: -rc target

This was previously marked as an RC target for 8.0.0; however, the current proposed approach looks patch-safe. So removing the tag. We might consider a more complete fix in 8.2.x if needed.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

borepstein’s picture

I have experienced the same thing, only I got HTTP code 403:

https://www.drupal.org/node/2780307

Not that that is necessarily significant but I thought I'd mention that.

I got exactly the configuration mentioned above: Nignx, PHP, PHP-FPM, Drupal 8, etc.

Boris.

rpmskret’s picture

I installed a new drupal 8 site yesterday (drupal 8.1.8). Today I cannot install a theme via the web interface. This appears to be caused by the same issues as the drupal core issue posted here. Here is some information courtesy of my website:

Installing bootstrap
An error has occurred.
Please continue to the error page

An AJAX HTTP error occurred.
HTTP Result Code: 403
Debugging information follows.
Path: /core/authorize.php/core/authorize.php?batch=1&id=6&op=do_nojs&op=do
StatusText: Forbidden
ResponseText: 
403 Forbidden
Forbidden
You don't have permission to access /core/authorize.php/
on this server.
Apache/2.4.18 (Ubuntu) Server at www.ic4ever.com Port 80

This seems to be a pretty major issue and I hope this helps.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

cilefen’s picture

Component: base system » routing system
Issue tags: +Triaged core major

@catch, @effulgentsia, @xjm and I discussed this issue and we agree that it is properly prioritized at major, considering its impact and scope. @Cottser and @alexpott supported the decision in separate communications. It would be interesting to see if this is reproducible on IIS, although that would not on its own change the priority.

boozemun’s picture

I get the same error with Drupal 8.2.5 using Mac OS Server 10.12 and MySQL:

Update manager

Notice: Undefined index: log in update_authorize_install_batch_finished() (line 293 of core/modules/update/update.authorize.inc).
Warning: Invalid argument supplied for foreach() in update_authorize_install_batch_finished() (line 293 of core/modules/update/update.authorize.inc).
Notice: Undefined index: log in update_authorize_install_batch_finished() (line 334 of core/modules/update/update.authorize.inc).
Notice: Undefined index: tasks in update_authorize_install_batch_finished() (line 335 of core/modules/update/update.authorize.inc).
Installation failed! See the log below for more information.

I've given up on Drupal 8 problem after problem forget it

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

paluna’s picture

I am getting the same error consistently.

Software stack:
- Drupal drupal-8.2.6
- Ubuntu 14.04.2 LTS
- Apache/2.4.23
- PHP 7.0.6-1+donate.sury.org~trusty+1
- php7.0-fpm:amd64/trusty 7.0.6-1+donate.sury.org~trusty+1

Notes:
- mod_security2 is enabled but it does not block Drupal in any way.
- I tried to de-duplicate the URL but that results in the same error. ProxyPassMatch ^/(core/authorize\.php/core/authorize\.php(.*)?)$ fcgi://127.0.0.1:9001/var/apache-www-80/core/authorize.php disablereuse=On retry=10

Error when installing a theme: Error page#1:
An AJAX HTTP error occurred.
HTTP Result Code: 403
Debugging information follows.
Path: /core/authorize.php/core/authorize.php?batch=1&id=38&op=finished&op=do
StatusText: Forbidden

Error page #2:
Notice: Undefined index: log in update_authorize_install_batch_finished() (line 293 of core/modules/update/update.authorize.inc).
Warning: Invalid argument supplied for foreach() in update_authorize_install_batch_finished() (line 293 of core/modules/update/update.authorize.inc).
Notice: Undefined index: log in update_authorize_install_batch_finished() (line 334 of core/modules/update/update.authorize.inc).
Notice: Undefined index: tasks in update_authorize_install_batch_finished() (line 335 of core/modules/update/update.authorize.inc).
Installation failed! See the log below for more information.

=> What I am doing right now is upload the theme or module manually and then activacte it using the Admin UI when needed. It is not the optimal procedure but it works. A fix would make configuration and deployment much easier, especially for new Drupal developers like me.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mdesimone’s picture

I have this exact same error.

Drupal 8.5.0
PHP 7.1.15
nginx 1.13.9

Using a fork off the git repo. My master branch started from tag 8.5.0-rc1 and merged in tag 8.5.0. So I'm in a pretty good position to test stuff; there's effectively no code of mine in there.

Basically, I'm starting from a fresh 8.5.0 install and trying to install @font-your-face 8.x-3.2.
Nginx config block passing control to PHP-FPM:

        location ~ \.php(/|$) {
                fastcgi_split_path_info ^(.+?\.php)(|/.*)$;
                include /opt/local/etc/nginx/fastcgi.conf;
                # Block httpoxy attacks. See https://httpoxy.org/.
                fastcgi_param HTTP_PROXY "";
                fastcgi_param SCRIPT_FILENAME $document_root$fastcgi_script_name;
                fastcgi_param PATH_INFO $fastcgi_path_info;
                fastcgi_param QUERY_STRING $query_string;
                fastcgi_intercept_errors on;
                fastcgi_pass unix:/opt/local/var/run/php71/php-fpm.sock;
        }

I have no idea what patch above to apply. For now I'll apply the Nginx workaround found here:

rewrite ^/core/authorize.php/core/authorize.php(.)$ /core/authorize.php$1;

lightweight’s picture

I'm getting the same error as @paluna in #107, but with Nginx and PHP-FPM 7.x with Drupal 8.4+

greg.1.anderson’s picture

@lightweight: I wrote a blog post on how to configure Nginx to avoid this and other similar potential pitfalls in Drupal 8:

https://pantheon.io/blog/update-your-nginx-config-drupal-8

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

prlw1’s picture

Just saw this with drupal 8.6.4, php 7.2.12, nginx 1.15.6. Problem is, I already have

                fastcgi_split_path_info ^(.+?\.php)(|/.*)$;

in my nginx.conf. That looks the same as in the blog post...

Just for the record:

An AJAX HTTP error occurred.
HTTP Result Code: 403
Debugging information follows.
Path: /core/authorize.php/core/authorize.php?batch=1&id=7&op=do_nojs&op=do
StatusText: Forbidden
ResponseText: 
403 Forbidden
403 Forbidden
nginx/1.15.6

Fixed by brute force:

        # https://www.drupal.org/project/drupal/issues/2583799
        rewrite ^/core/authorize.php/core/authorize.php(.*)$ /core/authorize.php$1;

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Gorf’s picture

I wasn't sure if I should update this bug or not but I am getting this error on 8.9.1, MySQL backend and PHP 7.3.11 using php-fpm behind Nginx.

An AJAX HTTP error occurred.
HTTP Result Code: 404
Debugging information follows.
Path: /core/authorize.php/core/authorize.php?batch=1&id=6&op=do_nojs&op=do
StatusText: error
ResponseText: {"message":"No route found for \u0022POST \/core\/authorize.php\/core\/authorize.php\u0022 (from \u0022https:\/\/www.sweetbeachcabin.com\/core\/authorize.php?batch=1\u0026id=6\u0026op=start\u0022)"}

if I click on the link for error logs I get this output:

Notice: Undefined index: log in update_authorize_install_batch_finished() (line 295 of core/modules/update/update.authorize.inc).
Warning: Invalid argument supplied for foreach() in update_authorize_install_batch_finished() (line 295 of core/modules/update/update.authorize.inc).
Notice: Undefined index: log in update_authorize_install_batch_finished() (line 337 of core/modules/update/update.authorize.inc).
Notice: Undefined index: tasks in update_authorize_install_batch_finished() (line 338 of core/modules/update/update.authorize.inc).
Installation failed! See the log below for more information.
greg.1.anderson’s picture

@Gorf: Please see item #2 in my blog post: https://pantheon.io/blog/update-your-nginx-config-drupal-8

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

shenzhuxi’s picture

Hi @greg.1.anderson, the patch only works with php built-in web server when ht.router.php is in the root directory.
https://www.drupal.org/project/drupal/issues/3341324 patch won't fix it.

For the URL issues through years, I feel that it may be better for Drupal to stop using weird dot position in URLs and only use dot for extension name.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.