WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 years ago

#34419 new enhancement

Update Pingback Processing Code

Reported by: dshanske Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version:
Component: Pings/Trackbacks Keywords: needs-patch needs-refresh needs-unit-tests
Focuses: Cc:

Description

Related to #34141, which would pass the retrieved pingback source to preprocess_comment and comment_post for additional processing, pingback_ping currently strips all content from the source as part of its processing.

Suggesting that specifically:

  1. Sanitization be done using core sanitization functions(wp_kses, etc)
  2. The code that returns relevant errors(URL not found, etc), be processed ahead of anything else.
  3. The older code that duplicates the functionality of url_to_postid noted as FIXME be removed. If there really is another difference, that should be an enhancement to url_to_postid.

This, combined with the enhancements in #34141, would allow moving toward improving linkback presentation(#32653), which is a long term goal.

Attachments (1)

34419.diff (9.3 KB) - added by dshanske 2 years ago.
Refactoring of Pingback Code

Download all attachments as: .zip

Change History (9)

#1 @dshanske
3 years ago

  • Keywords needs-patch added
  • Severity changed from normal to minor

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


2 years ago

#3 @dshanske
2 years ago

Just looking at this again. The current pingback_ping code checks for the source material from the remote site before checking to see if there is a comment with a URL that matches the incoming before it goes through all the work of getting it.

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


2 years ago

@dshanske
2 years ago

Refactoring of Pingback Code

#5 @dshanske
2 years ago

Added in this refactoring of the pingback code. I had experimented with this before in the Improve Linkback Display Ticket, #32653, but I broke it into these two pieces. The only display change is from the [...] to a simple sentence. That doesn't go anywhere near far enough in my opinion, but it is a start toward a lot more than can be done, but now that the source is passed through...I think that is better done attaching the enhancement to a filter.

This ticket was mentioned in Slack in #core-comments by dshanske. View the logs.


2 years ago

#7 @rachelbaker
2 years ago

  • Keywords needs-refresh needs-unit-tests added

@dshanske There is so much going on in this patch, which makes understanding the implications difficult. Some initial thoughts.

  1. I suggest using this ticket to address either the earlier processing of errors OR the sanitization, both of which will need unit tests.
  1. I would address removing the legacy conditional url_to_postid and //$way debugging line leftover from r30139 in a separate "bug" ticket (referencing the related commit) because that is a "good first bug" and straightforward.
  1. I would address adding the new ping_source_verified filter in a separate "enhancement" ticket.
  1. I see you are increasing the size of the response limit from 150 KB to 1 MB, why is that needed? The current limit was kept low to prevent denial of service attacks (see #4137). I would caution against increasing the number.

#8 @dshanske
2 years ago

Seems I may have overscoped here. Collectively, this was an attempt to update the entire function, but I see the point of breaking it into pieces.

  1. I really have to learn how to write unit tests. It's a weakness of mine, I admit. I may need help with that.
  1. Will split it into a separate ticket.
  1. This was basically an attempt at better early rejection to address spammy pings, paired with HEAD request and the immediate rejection of media content types(which is what we do in reverse when pinging a site). The current logic looks for a URL in the content...but again...will break it off into a separate ticket.
  1. The average size of a webpage keeps increasing. But, point taken. May be better to make that a filtered option to increase if I feel it is needed, as opposed to hard-coding. And also, a separate ticket.

I guess I will be opening more tickets then I close again...

Note: See TracTickets for help on using tickets.