Problem/Motivation

Once we update in #3181240: Upgrade typo3/phar-stream-wrapper 3.1.6 then PHP 8 doesn't have the same vulnerability as PHP 7 so there's no need to register the phar stream wrapper.

Proposed resolution

Check PHP version before registering the wrapper and add todo to remove

The wrapper no longer needed according to
- https://wiki.php.net/rfc/phar_stop_autoloading_metadata
- https://github.com/TYPO3/phar-stream-wrapper/issues/64#issuecomment-7191...
- https://github.com/php/php-src/commit/0c238ede019f6ffbe7c996ec1695a747f4...
- https://github.com/ohader/phar-stream-wrapper/commit/3a25049ddd9f49e732d...

Remaining tasks

review/commit

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

Comments

alexpott created an issue. See original summary.

andypost’s picture

Status: Postponed » Active
Pooja Ganjage’s picture

StatusFileSize
new425 bytes

Hi,

Creating a patch for this issue.

Please review the patch.

Let me know if any suggestions.

Thanks.

Pooja Ganjage’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: 3181275-3.patch, failed testing. View results

alexpott’s picture

@Pooja Ganjage that's not the fix here. We can't remove the dependency - we still support PHP 7.3.

IN \Drupal\Core\DrupalKernel::boot() we only need to do this when on PHP 7.

    if (in_array('phar', stream_get_wrappers(), TRUE)) {
      // Set up a stream wrapper to handle insecurities due to PHP's builtin
      // phar stream wrapper. This is not registered as a regular stream wrapper
      // to prevent \Drupal\Core\File\FileSystem::validScheme() treating "phar"
      // as a valid scheme.
      try {
        $behavior = new PharStreamWrapperBehavior();
        PharStreamWrapperManager::initialize(
          $behavior->withAssertion(new PharExtensionInterceptor())
        );
      }
      catch (\LogicException $e) {
        // Continue if the PharStreamWrapperManager is already initialized. For
        // example, this occurs during a module install.
        // @see \Drupal\Core\Extension\ModuleInstaller::install()
      }
      stream_wrapper_unregister('phar');
      stream_wrapper_register('phar', PharStreamWrapper::class);
    }
andypost’s picture

Curious about is there a way to skip unregister if already core's override applied

alexpott’s picture

@andypost can you explain a bit more re #7.

Pooja Ganjage’s picture

StatusFileSize
new474 bytes
Pooja Ganjage’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 9: 3181275-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

andypost’s picture

Issue summary: View changes
StatusFileSize
new775 bytes

@alexpott sorry I was wrong. I thought about initializeSettings() method which have own issue #2708827: Allows DrupalKernel::handle to handle more than one requests (rough support for PHP-PM)

Here's a patch to skip whole block and todo to new issue #3210486: Remove typo3/phar-stream-wrapper and associated code

andypost’s picture

Status: Needs work » Needs review
andypost’s picture

StatusFileSize
new801 bytes
new1.54 KB

Fix broken test

andypost’s picture

StatusFileSize
new1.07 KB
new1.89 KB

It does not throw exception on PHP < 8

Status: Needs review » Needs work

The last submitted patch, 15: 3181275-15.patch, failed testing. View results

andypost’s picture

Status: Needs work » Needs review

Unrelated failure in QuickStartTest::testQuickStartCommand

alexpott’s picture

@andypost I think in #15 you meant to say... It does not throw exception on PHP >= 8 ... it certainly does throw an exception on PHP < 8.

+++ b/core/tests/Drupal/KernelTests/Core/File/PharWrapperTest.php
@@ -24,10 +24,13 @@ public function testPharFile() {
+    // @todo clean-up for PHP 8.0+ https://www.drupal.org/node/3210486
+    if (PHP_VERSION_ID < 80000) {
+      // Ensure that file operations via the phar:// stream wrapper throw an
+      // exception for files without the .phar extension.
+      $this->expectException('TYPO3\PharStreamWrapper\Exception');
+      file_exists("phar://$base/image-2.jpg/index.php");
+    }

What does happen with the regular phar handler in PHP 8 with files that are not phar files?

andypost’s picture

andypost’s picture

Issue summary: View changes

Updated IS with links

According to tests on PHP 8 the used library does not throw https://github.com/ohader/phar-stream-wrapper/commit/3a25049ddd9f49e732d...

andypost’s picture

StatusFileSize
new677 bytes
new2.07 KB

Extended test for PHP 8

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

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.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.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Makes sense to me to do this now more people are moving towards PHP 8.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 1a7b7f7219b to 10.0.x and 1311b624e7e to 9.4.x. Thanks!

Can we get a follow-up to remove the dependency and code on Drupal 10? Thanks!

  • alexpott committed 1a7b7f7 on 10.0.x
    Issue #3181275 by andypost, Pooja Ganjage, alexpott: Do not register...

  • alexpott committed 1311b62 on 9.4.x
    Issue #3181275 by andypost, Pooja Ganjage, alexpott: Do not register...
longwave’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.