WordPress.org

Make WordPress Core

Opened 5 weeks ago

Last modified 4 weeks ago

#44195 reviewing defect (bug)

"Silence is golden" index.html generates output

Reported by: Ov3rfly Owned by: johnbillion
Milestone: 4.9.7 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch commit
Focuses: Cc:

Description

Unwanted listing of folders in wp-content is ususally prevented via a file index.php with a php comment // Silence is golden. which does not return any output.

  • wp-content/index.php
  • wp-content/plugins/index.php
  • wp-content/themes/index.php
  • ...

The new storage folder for personal data uses index.html with the text Silence is golden.

  • wp-content/uploads/wp-personal-data-exports/index.html

Due to consistency the file index.html should not generate any output = be empty.

Not sure why index.html is used here at all, maybe to avoid putting a .php in uploads folder? Maybe somebody knows the reason, thanks for a heads up.

Attachments (4)

44195.diff (751 bytes) - added by rafsuntaskin 5 weeks ago.
Create consistent .php with commented line
44195.2.diff (1.0 KB) - added by audrasjb 4 weeks ago.
Fix: delete legacy index.html file
44195.3.diff (1.3 KB) - added by audrasjb 4 weeks ago.
Replace index.html with index.php in clean up function wp_privacy_delete_old_export_files()
44195.4.diff (509 bytes) - added by audrasjb 4 weeks ago.
replace HTML text with HTML comment to avoid to display anything.

Download all attachments as: .zip

Change History (19)

@rafsuntaskin
5 weeks ago

Create consistent .php with commented line

#1 @allendav
5 weeks ago

The html approach was copied from WooCommerce. I am all for changing this to be more consistent with the rest of WordPress core.

#2 @Ov3rfly
5 weeks ago

Note that the filename also needs to be changed in wp_privacy_delete_old_export_files(), otherwise index.php gets deleted three days after installation by the clean up cron for old export files, with disasterous results...

#3 @swissspidy
5 weeks ago

  • Keywords needs-patch added

I see no reason why this should be an HTML file.

Some hosts prevent PHP files from being directly executed inside the uploads folder (or even wp-content). In this case you'd get an error instead of a directory listing.

And if you get a directory listing, your hosting is poorly configured anyway.

#4 @johnbillion
4 weeks ago

  • Milestone changed from Awaiting Review to 4.9.7

The reason these index files exist is precisely for poorly configured hosts that do have open directory listings. I think this should be updated so the text is inside a comment.

The problematic code is inside wp_privacy_generate_personal_data_export_file().

#5 @rafsuntaskin
4 weeks ago

@johnbillion @swissspidy

Is there any problem with the provided patch?? I can update if needed.

Thanks

#6 @johnbillion
4 weeks ago

  • Keywords has-patch added; needs-patch removed
  • Owner set to johnbillion
  • Status changed from new to reviewing

@audrasjb
4 weeks ago

Fix: delete legacy index.html file

#7 @audrasjb
4 weeks ago

Hello,

In 44195.2.diff, added a check on index.html using file_exists() to delete 4.9.6 legacy files.

Not sure this is specifically needed here but I guess it is always nice to keep folders clean before adding new files :)

Cheers, Jb

#8 @Ov3rfly
4 weeks ago

@audrasjb: Not necessary, the clean up cron will take care of this, if the filename is changed as described in #2.

@audrasjb
4 weeks ago

Replace index.html with index.php in clean up function wp_privacy_delete_old_export_files()

#9 @audrasjb
4 weeks ago

@Ov3rfly you are absolutely right.

So I guess we need to add index.php to wp_privacy_delete_old_export_files() function.

[edit: okay, that's what you asked in comment #2]

Fixed in 44195.3.diff

Cheers,

Jb

Last edited 4 weeks ago by audrasjb (previous) (diff)

#10 @jorbin
4 weeks ago

I agree with @johnbillion. This should be kept as an html file but the content moved into an HTML comment. Putting executed files in the uploads dir isn't something we should encourage.

@audrasjb
4 weeks ago

replace HTML text with HTML comment to avoid to display anything.

#11 @audrasjb
4 weeks ago

44195.4.diff : Ok, let's avoid to insert any .php file in /uploads folder, and replace HTML text with a comment.

Last edited 4 weeks ago by audrasjb (previous) (diff)

#12 @Ov3rfly
4 weeks ago

Due to consistency the file index.html should not generate any output = be empty.

An HTML comment is content. There should be no content. That's the main reason for this ticket...

#13 @pento
4 weeks ago

@Ov3rfly: Could you explain why you need this file to contain nothing at all?

#14 @Ov3rfly
4 weeks ago

Due to consistency the file index.html should not generate any output = be empty.

Serious software development needs consistency. Same rules should apply everywhere.

This ticket asks to follow a simple rule: No output from files which protect open directory listings.

With more consistency in everybody's mind, there would be a lot less tickets necessary, e.g. about bypassing existing filesystem api in #44204, and many more...

What a bug reporter personally needs or does not need is of no interest here.

#15 @pento
4 weeks ago

  • Keywords commit added

Thank you for clarifying why you made this ticket, @Ov3rfly. It's important to understand whether this was related to a technical requirement, or whether it was a style issue.

As it's a style issue, the more important part of consistency here is the presence of the comment, explaining that the file is intentionally blank, rather than the file returning no data when requested over HTTP.

I'm not particularly fussed that the current behaviour returns visible output, but I'm fine with changing the behaviour to that in 44195.4.diff.

Note: See TracTickets for help on using tickets.