Problem/Motivation

Compressed files, binaries, and databases are difficult to maintain and inspect.

This issue is to address:
./core/modules/update/tests/aaa_update_test.tar.gz

Background information

This was first reported to the Drupal security team who determined this could be a public followup.

Steps to reproduce

Proposed resolution

This archive is used explicitly to test uploading an archive. I see a few options for solving this.

Option 1:

  • Extract the archive
  • Just before uploading archive the directory
  • Run test

Downside, this means an archive process runs every test.
Upside, test artifact remains local.

Option 2:

  • Delete archive
  • Move to contrib or related space to create the archive as a release
  • Load the archive from that location for this test

Option 3:

  • Extract Archive
  • Delete Archive
  • Have periodic build that generates the archive (or on a branch where the source changes for testing)

Remaining tasks

./core/modules/update/tests/aaa_update_test.tar.gz

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3455819

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan commented

Ok the only reference to this archive file is here:

    // Check to ensure an existing module can't be reinstalled. Also checks that
    // the archive was extracted since we can't know if the module is already
    // installed until after extraction.
    $validArchiveFile = __DIR__ . '/../../aaa_update_test.tar.gz';
    $edit = [
      'files[project_upload]' => $validArchiveFile,
    ];

I think we can just extract it and compress it right before this step as part of the step. It creates a bit of churn on each test, but it does solve the immediate security concern.

There are a lot of references in the code to modules/update/tests/aaa_update_test.module, that file does not exist though so I'm not sure if there is a process I'm missing that extracts the archive first.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
Status: Active » Needs review

Setting needs review for the two approaches in the IS.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Option 2 is not viable because we should be able to run tests without internet access.

I wonder if this is even worthwhile because update manager's ability to handle tarballs is going away once we have Automatic Updates, perhaps we should just do nothing until then (same for #3455818: Decompress files for update_test_new_module)

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
quietone’s picture

quietone commented

I agree with longwave. And would rather see progress on #3403649: Rework database update tests so we don't have to ship database dumps in git

nicxvan’s picture

nicxvan commented

I think as long as there is a way to generate it locally just like https://www.drupal.org/project/drupal/issues/3403649#comment-15331668 suggests it should be fine.

I also think this is much lower stakes and easier to tinker with than full DB dump generation.

We can also just postpone this and #3455818: Decompress files for update_test_new_module

smustgrave’s picture

smustgrave commented

So should this be postponed?

smustgrave’s picture

smustgrave commented
Status: Needs review » Postponed

Let me know if I'm wrong on this assumption.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

quietone opened merge request !16494

quietone’s picture

quietone commented
Title: Decompress files for aaa_update_test » Removed unused aaa_update_test.tar.gz
Component: phpunit » update.module
Category: Bug report » Task
Status: Postponed » Needs review
Issue tags: -Security

I can't find any usages of the file in core. Uses were removing in #3417136: Remove adding an extension via a URL and #3062757: Remove deprecated legacy include files from Drupal 9 .

I created an MR to remove the file and tests are passing. So, I have changed the title to show that this is removing a file. And not a bug.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Yes, this is unused since #3417136: Remove adding an extension via a URL as far as I can tell, there are no remaining references at all now.

  • godotislate committed b2a155be on main 
    task: #3455819 Removed unused aaa_update_test.tar.gz

By: nicxvan
By:...

  • godotislate committed 24c62565 on 11.x 
    task: #3455819 Removed unused aaa_update_test.tar.gz

By: nicxvan
By:...
godotislate’s picture

godotislate
he/him
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed b2a155b to main and 24c6256 to 11.x. Thanks!

Do we bother doing this on 11.4.x? I'm going to mark this fixed, but can reopen if we want to backport.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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