Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#38187 closed defect (bug) (fixed)

Regex special chars should be escaped in WP_oEmbed

Reported by: jrf's profile jrf Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.0
Component: Embeds Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Just noticed four oembed regexes which each contained a non-escaped dot which potentially could result in incorrect matches (not very likely).

. in a regex means "match anything", it needs to be escaped \. to be interpreted as a literal period character.

So, for example, http://daiwly/ would match when it shouldn't. Though as I said, very unlikely as that would not make for a valid url.

These are probably artifacts from when each of these were transformed from a non-regex key into a regex based key.

Attachments (3)

trac-38187-fix-wildcard-escaping-oembed-regexes.patch (4.9 KB) - added by jrf 7 years ago.
trac-38187-unit-tests-for-the-oembed-regex-fixes.patch (3.8 KB) - added by jrf 7 years ago.
Adjust the related unit test provider array + add unit tests for the issue
trac-38187-remove-unused-variable-from-data-provider.patch (760 bytes) - added by jrf 7 years ago.
Remove copy/paste artifact - unused and unnecessary variable in the test data provider method.

Download all attachments as: .zip

Change History (12)

#1 @jrf
7 years ago

  • Keywords has-patch has-unit-tests added

@jrf
7 years ago

Adjust the related unit test provider array + add unit tests for the issue

#2 @SergeyBiryukov
7 years ago

  • Milestone changed from Awaiting Review to 4.7
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#3 @SergeyBiryukov
7 years ago

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

In 38684:

Embeds: Escape periods in oEmbed regex.

Props jrf.
Fixes #38187.

@jrf
7 years ago

Remove copy/paste artifact - unused and unnecessary variable in the test data provider method.

#4 @jrf
7 years ago

Thanks for merging.

And sorry but I just noticed there was still a copy/paste artifact in the test code. Uploaded a patch to remove it.

#5 follow-up: @jrf
7 years ago

@SergeyBiryukov Just checking - did you see my last response & patch ?

#6 in reply to: ↑ 5 @SergeyBiryukov
7 years ago

Replying to jrf:

@SergeyBiryukov Just checking - did you see my last response & patch ?

Seeing it now, thanks! It's OK to reopen the ticket if there are any additional fixes to make sure they don't get missed :)

#7 @SergeyBiryukov
7 years ago

In 38714:

Unit Tests: Remove unused variable in Tests_oEmbed::dataShouldNotMatchOembedRegex().

Props jrf.
See #38187.

#8 follow-up: @jrf
7 years ago

@SergeyBiryukov Thanks!

The reopen status has this ominous The resolution will be deleted note next to it which sounds as if any commits made for the ticket would be automatically reverted, so I was a bit wary about using it. Hmm.. maybe a ticket should be opened in meta about clarifying the message ?

#9 in reply to: ↑ 8 @SergeyBiryukov
7 years ago

Replying to jrf:

The reopen status has this ominous The resolution will be deleted note next to it which sounds as if any commits made for the ticket would be automatically reverted, so I was a bit wary about using it. Hmm.. maybe a ticket should be opened in meta about clarifying the message ?

Good point, never thought about that. Yeah, feel free to create a meta ticket.

Note: See TracTickets for help on using tickets.