WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 3 months ago

#41784 closed enhancement (fixed)

Twitter oembeds include visitor tracking

Reported by: norcross Owned by: johnbillion
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Embeds Keywords: has-patch commit
Focuses: Cc:

Description

Currently, Twitter adds a visitor tracking mechanism when tweets are embedded. They allow for sites to turn it off via a meta tag, or adding the dnt parameter to the embed request URL. This can be re-enabled using the existing oembed_fetch_url filter. Thanks to @joostdevalk for writing the code.

Attachments (1)

41784.diff (546 bytes) - added by norcross 3 months ago.
Additional line for adding the dnt argument

Download all attachments as: .zip

Change History (15)

@norcross
3 months ago

Additional line for adding the dnt argument

#1 @norcross
3 months ago

  • Keywords has-patch 2nd-opinion added

#2 @joostdevalk
3 months ago

+1, obviously. Seems such a simple way to prevent a lot of unnecessary tracking. Core doesn’t track visitors, why would we let other providers do that?

#3 @joostdevalk
3 months ago

Note that this works for more than just Twitter, it seems to be a pseudo standard, see for instance:

http://docs.embed.ly/docs/oembed

#4 @SergeyBiryukov
3 months ago

  • Milestone changed from Awaiting Review to 4.9

#5 follow-up: @johnbillion
3 months ago

  • Keywords 2nd-opinion removed
  • Type changed from defect (bug) to enhancement
  • Version trunk deleted

+1 from me as long as it doesn't cause any of the other oEmbed providers to malfunction. I can't imagine it would, but it needs to be double checked. The tests in the patch on #32360 could be used to verify this, with a bit of persuading.

#6 follow-up: @norcross
3 months ago

I pulled down the oembed test from #32360 and the testing results were identical when including the dnt flag, but another set of eyes would be helpful.

#7 in reply to: ↑ 6 @boogah
3 months ago

Replying to norcross:

I pulled down the oembed test from #32360 and the testing results were identical when including the dnt flag, but another set of eyes would be helpful.

I am also seeing identical output between the oEmbed tests on trunk and with @norcross's patch in place.

#8 in reply to: ↑ 5 @mikeschroder
3 months ago

Replying to johnbillion:

+1 from me as long as it doesn't cause any of the other oEmbed providers to malfunction. I can't imagine it would, but it needs to be double checked. The tests in the patch on #32360 could be used to verify this, with a bit of persuading.

+1 for doing this from me as well.

#9 @matt
3 months ago

This looks like a good change, let's get it in trunk and see if there are any unintended downsides.

#10 @peterwilsoncc
3 months ago

  • Keywords commit added
  • Owner set to peterwilsoncc
  • Status changed from new to assigned

I'll aim for Monday AEST.

#11 follow-up: @netweb
3 months ago

If the tests from # #32360 are going to be pulled in, please remove Vine before committing

The Vine oEmbed is being removed for 4.9 in #41817

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

Replying to netweb:

If the tests from # #32360 are going to be pulled in, please remove Vine before committing

They've gone in once and were subsequently reverted as they were frequently throwing errors due to API limits and network issues. I'll use them to test locally but leave them as their own ticket.

#13 @johnbillion
3 months ago

  • Owner changed from peterwilsoncc to johnbillion

@peterwilsoncc I spent the afternoon on Wednesday updating the tests in #32360 and I can confirm that adding the dnt parameter doesn't break tests for any of the providers.

I'll get this in.

#14 @johnbillion
3 months ago

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

In 41345:

Embeds: Add the dnt (Do Not Track) query parameter to all oEmbed provider URLs.

This means that, for those providers that support this somewhat de-facto standard, visitor tracking is disabled by default for all embeds.

Props norcross, joostdevalk

Fixes #41784

Note: See TracTickets for help on using tickets.