WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#35322 closed defect (bug) (fixed)

oEmbed generates link to an image in the wp-admin directory

Reported by: nachtwaechter Owned by: swissspidy
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.4
Component: Embeds Keywords: has-patch commit
Focuses: Cc:

Description

The embedded article contains a link to the WordPress logo in the wp-admin directory, if no website icon is specified explicitly.

For most users, it is not an issue. But some people – like me – prefer to have HTTP authentification on the whole wp-admin directory to harden their installation. For those people, an embedded post or page in social media sites or something similar gives an authentification dialog window in the browser while displaying, which is ugly, unwanted and looks like a bug.

There is a very simple workaround, which I am going to describe in a German posting soon: Just set a website icon, which is then used instead of the default WordPress logo. (Why German? Because it is my native tongue and it is spoken by all of my readers.)

But from my point of view, it is an error in the WordPress core. The error is in the sometimes wrong assumption, wp-admin is a world-readable directory in every installation.

I suggest placing a WordPress logo in the wp-includes directory and using this version in embedded views, allowing for using HTTP authentification in wp-admin.

Attachments (4)

35322.patch (2.0 KB) - added by thewanderingbrit 3 years ago.
35322.1.diff (1.6 KB) - added by thewanderingbrit 3 years ago.
Revised to include src directory
35322.1.patch (6.4 KB) - added by thewanderingbrit 3 years ago.
35322-442.diff (7.8 KB) - added by peterwilsoncc 3 years ago.

Download all attachments as: .zip

Change History (20)

#1 follow-up: @dd32
3 years ago

  • Milestone changed from Awaiting Review to 4.5

This is definitely something that should be done.

If it didn't require additional images being added I'd have even suggested it would be 4.4.x material (We'd have to use an externally-hosted image if we wanted to move away from the existing wp-admin image)

#2 @johnbillion
3 years ago

  • Component changed from General to Embeds
  • Keywords needs-patch added

#3 in reply to: ↑ 1 ; follow-up: @peterwilsoncc
3 years ago

Replying to dd32:

If it didn't require additional images being added I'd have even suggested it would be 4.4.x material (We'd have to use an externally-hosted image if we wanted to move away from the existing wp-admin image)

I was thinking 4.4.x too, moving the logo files to wp-includes is a relatively minor change.

#4 in reply to: ↑ 3 @dd32
3 years ago

Replying to peterwilsoncc:

Replying to dd32:

If it didn't require additional images being added I'd have even suggested it would be 4.4.x material (We'd have to use an externally-hosted image if we wanted to move away from the existing wp-admin image)

I was thinking 4.4.x too, moving the logo files to wp-includes is a relatively minor change.

To clarify my reasoning for not including the file move/copy in 4.4.1, is that we don't add new files in point releases to as to make point releases more reliable, it's easy to modify a file, but can't always create one. For background updates we prefer to have the wider coverage of only needing to be able to modify, not create.

#5 @thewanderingbrit
3 years ago

  • Keywords has-patch added; needs-patch removed

This patch moves the image to wp-includes/images and fixes the references to them in the embed template.

Thanks for the valuable contribution @nachtwaechter. Much appreciated!

Last edited 3 years ago by thewanderingbrit (previous) (diff)

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


3 years ago

#7 @Otto42
3 years ago

Instead of moving the image, it should probably be a copy, in case any plugins/themes/other expect it to be in the admin directory and are linking to it there.

#8 @thewanderingbrit
3 years ago

Agreed. I used the wrong term. I did in fact copy the image. The original remains in place.

@thewanderingbrit
3 years ago

Revised to include src directory

#9 @thewanderingbrit
3 years ago

attachment:35322.1.diff now correctly includes the src directory, but I notice that, with the diff being created by GitHub from these directions, the image isn't included in the diff. diff created from this PR.

Last edited 3 years ago by thewanderingbrit (previous) (diff)

#10 @peterwilsoncc
3 years ago

Following comment:3, @dd32 and I had a super quick discussion in slack on 4.4.2 patch without new files. It's in 35322-442.diff:

  • I moved the title HTML to a function to avoid double coding.
  • Markup unchanged if site logo is defined.
  • If no logo is defined, an empty span is placed on the page with a w-logo background as a data uri.
  • Logo is SVG for most, PNG for IE8 & earlier

The IE8/png fallback looks a bit mucky, it might be possible to clean it up.

#11 follow-up: @swissspidy
3 years ago

  • Keywords needs-refresh needs-unit-tests added
  • Summary changed from OEmbed generates link to an image in the wp-admin directory to oEmbed generates link to an image in the wp-admin directory

Some notes on the second patch:

  • I wanted to create a new function for that as well, though a name like print_embed_site_title() sounds better. No need for an $echo parameter though, just echo.
  • The sprintf in that function is wrong. Also, the admin_url( 'images/w-logo-blue.png' ) fallback is not needed since there is always an site icon because of has_site_icon() check
  • Tests for that function would be nice.

#12 in reply to: ↑ 11 @peterwilsoncc
3 years ago

Replying to swissspidy:

Some notes on the second patch:

  • I wanted to create a new function for that as well, though a name like print_embed_site_title() sounds better. No need for an $echo parameter though, just echo.
  • The sprintf in that function is wrong. Also, the admin_url( 'images/w-logo-blue.png' ) fallback is not needed since there is always an site icon because of has_site_icon() check
  • Tests for that function would be nice.

FWIW, I think the second patch is too much for the benefit it provides. As a logo is an image and new files can be created in a major release, patch 1 seems more appropriate.

What are your thoughts?

#13 @dd32
3 years ago

FWIW I agree that the 4.4.x patch is too big for the benefit it provides. wp-admin is not a secret, and very few sites have HTTP Auth in place. Those sites can either go ahead and assign a site logo or allow that file through.

#14 @swissspidy
3 years ago

  • Keywords needs-refresh needs-unit-tests removed
  • Owner set to swissspidy
  • Status changed from new to assigned

I love the simplicity of 35322.1.diff. Something like 35322-442.diff can be looked at as part of #34561.

We should go with the former for 4.5, as it's a relatively minor change with just 1 new file.

#15 @swissspidy
3 years ago

  • Keywords commit added

#16 @swissspidy
3 years ago

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

In 36635:

Embeds: Load the default site icon from the wp-includes directory.

Files inside the wp-admin directory may not be publicly available. This copies the blue WordPress logo to wp-includes/images.

Props thewanderingbrit.
Fixes #35322.

Note: See TracTickets for help on using tickets.