WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 4 months ago

Last modified 4 months ago

#43101 closed defect (bug) (fixed)

Test to ensure MediaElement SWFs aren't accidentally added to build

Reported by: iandunn Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version: 4.9.2
Component: External Libraries Keywords: has-patch
Focuses: Cc:

Description

r42462 removed the Flash files from MediaElement, per #42720. They're no longer necessary, and have a history of security issues.

I think it'd be useful to have a unit test to make sure that the SWFs don't get accidentally added to our build again, since they're still part of MediaElement itself.

43101.diff adds the test, and I think it's ready for commit, but I'd like a second opinion before doing that.

Attachments (3)

43101.diff (2.6 KB) - added by iandunn 5 months ago.
43101.2.diff (2.6 KB) - added by iandunn 4 months ago.
Look at build rather than src, update @since, use @ticket instead of @group
43101.3.diff (1.7 KB) - added by ocean90 4 months ago.

Download all attachments as: .zip

Change History (12)

@iandunn
5 months ago

#1 @DrewAPicture
4 months ago

  • Keywords commit good-first-bug removed

@ocean90 Are you available to please review the patch here?

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


4 months ago

#3 @ocean90
4 months ago

Isn't this already covered by Tests_Admin_IncludesUpdateCore:: test_new_files_are_not_in_old_files_array_compiled()?

https://core.trac.wordpress.org/browser/trunk/tests/phpunit/tests/admin/includesUpdateCore.php

#4 @pento
4 months ago

Presumably MediaElement won't be renaming or adding new SWF files in the future, but it's not a bad idea to be a little more cautious with ensuring we don't accidentally re-add them.

#5 @SergeyBiryukov
4 months ago

  • @group 42720 should be @ticket 42720.
  • @since 4.9.2 should be updated to 5.0.0.

Looks good to me otherwise.

@iandunn
4 months ago

Look at build rather than src, update @since, use @ticket instead of @group

#6 @iandunn
4 months ago

Replying to ocean90:

Isn't this already covered by Tests_Admin_IncludesUpdateCore:: test_new_files_are_not_in_old_files_array_compiled()?

That's a good point, I hadn't thought of that. It would catch the case of re-adding the currently known SWF files, but it wouldn't catch the case of MEjs adding new SWF files, like mediaelement-flash-video-foo.swf. It also makes it explicitly clear that there are security implications of restoring them, which isn't indicated in r42462.

I'm leaning towards it still being a good idea to add it. What do you think, @ocean90 ?

@group 42720 should be @ticket 42720. @since 4.9.2 should be updated to 5.0.0.

Good catches, thanks Sergey. Fixed in 43101.2.diff . That also updates it to look at the build folder, like test_new_files_are_not_in_old_files_array_compiled() does.

@ocean90
4 months ago

#7 @ocean90
4 months ago

43101.3.diff is without the need for glob_recursive().

#8 @iandunn
4 months ago

  • Resolution set to fixed
  • Status changed from new to closed

Fixed in r42762, but I forgot the #...

External Libraries: Test that MediaElement SWF files remain deleted.

The files were removed from Core in r42462 because they're no longer necessary, and have a history of security issues. They remain upstream, though, so this test makes it explicitly clear that they should not be added back in the future without careful consideration and discussion with the Security team.

Tests_Admin_IncludesUpdateCore::test_new_files_are_not_in_old_files_array_compiled() would already catch files with the exact same name, but this test will also catch files with new names, just to be extra cautious.

Props iandunn, ocean90, SergeyBiryukov Fixes 43101

#9 @iandunn
4 months ago

In 42763:

External Libraries: Test for MEjs files in src instead of build.

The build task doesn't get run during TravisCI jobs, so the build folder doesn't exist in that context. Because of that, the test added in r42762 was failling.

Checking for the files in src instead achieves the same goal as that commit, but should pass in Travis.

See #43101
See https://wordpress.slack.com/archives/C02RQBWTW/p1519742993000615

Note: See TracTickets for help on using tickets.