Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent coauthors capability check infinite loop #46

Merged
merged 2 commits into from
Feb 13, 2024

Conversation

chickenn00dle
Copy link
Contributor

@chickenn00dle chickenn00dle commented Feb 12, 2024

Closes 0/1206331646877116/1206535636784422/f

This fixes an issue where, when NP Network, Distributor, CAP, and NP Newsletters are active, sending a newsletters causes an infinite capability check which results in a fatal memory error.

The issue happens because we are creating a Distributor Post object to check a distributed posts linked status when filtering coauthors via the get_coauthors hook. But this same hook is eventually triggered in the instantiation of the Distributor Post object.

This PR fixes this by checking a posts linked status directly via post meta.

Testing steps

  1. Make sure you have a test site set up with Newspack Network, Distributor, Co-Authors Plus, and Newsletters, and mailchimp ESP setup
  2. Create a new newsletter and send it to an audience while watching debug log
  3. On trunk, you should see an memory fatal. On this branch you should not

Screenshot 2024-02-12 at 15 55 57

@chickenn00dle chickenn00dle requested a review from a team as a code owner February 12, 2024 20:57
@leogermani
Copy link
Contributor

Nice catch!

But will this make the posts included in the newsletters to not display the filtered by lines?

Did you figure out why exactly this causes an OOM?

@chickenn00dle
Copy link
Contributor Author

chickenn00dle commented Feb 12, 2024

But will this make the posts included in the newsletters to not display the filtered by lines?

Ah. I'm not sure. TBH I'm not very familiar with co-authors plus and haven't really set it up to test properly. But will try setting up and confirming today.

Did you figure out why exactly this causes an OOM?

It looks like there might be an infinite loop triggered when filtering coauthors via our network customization. It looks like in the process of creating the distributor post for the newsletter, co-authors plus triggers the get_coauthors hook which repeats the cycle.

@claudiulodro
Copy link
Contributor

That is a pretty common way of ending up with infinite loops (circular hooks). Good sleuthing!

@chickenn00dle
Copy link
Contributor Author

@leogermani, I've set up and tested this and it looks like distributed, coauthored posts still appear with the expected by lines. Here are the inserted posts from a test email sent to myself:

Screenshot 2024-02-12 at 17 40 05

I'm not sure if this covers all of the expected cases, or if this is exactly what you were referring to. Let me know if I'm off base.

@chickenn00dle
Copy link
Contributor Author

For informational purposes, after @leogermani pointed out an issue with guest authors with this approach, I did some more digging into the exact problem here.

  1. When Newspack Network is active, we filter a posts coauthors on the get_coauthors hook. In the process we create a DistributorPost object here:
    $distributor_post = new DistributorPost( $post_id );
  2. The distribor post fetches the permalink in its constructor here: https://github.com/10up/distributor/blob/1f180d74db804a057f7331ce62338e571dd73350/includes/classes/DistributorPost.php#L198
  3. Which eventually triggers the user_has_cap hook via a call to current_user_can: https://github.com/WordPress/WordPress/blob/53b99200d6000ff43effb3fd615cbe6a65d6b5e4/wp-includes/link-template.php#L133C1-L133C46
  4. The CAP plugin uses this hook to allow guest authors to edit posts and in the process gets all coauthors: https://github.com/Automattic/Co-Authors-Plus/blob/68fc80d5795fd83cedd8fa7a51fe30b015caf0c4/template-tags.php#L65
  5. This eventually calls the get_coauthors hook: https://github.com/Automattic/Co-Authors-Plus/blob/68fc80d5795fd83cedd8fa7a51fe30b015caf0c4/template-tags.php#L40

@leogermani
Copy link
Contributor

leogermani commented Feb 13, 2024

Yay! Thanks for catching this @chickenn00dle ! So good when these things are surfaced.

It seems the only reason we are using a DistributorPost object is to check the is_linked property. Let's find another wayto do this.

Ideally by using a method provided by Distributor. But if not possible, by reading the data directly (I guess it's a post meta)

@chickenn00dle chickenn00dle force-pushed the fix/no-coauthor-filter-on-rest-request branch from 8276b43 to ac1eb9a Compare February 13, 2024 18:54
@chickenn00dle chickenn00dle changed the title fix: prevent coauthor filter on rest request fix: prevent coauthors capability check infinite loop Feb 13, 2024
@chickenn00dle chickenn00dle force-pushed the fix/no-coauthor-filter-on-rest-request branch from ac1eb9a to e080ed8 Compare February 13, 2024 19:02
Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phenomenal

@chickenn00dle chickenn00dle merged commit 9565cef into trunk Feb 13, 2024
3 checks passed
matticbot pushed a commit that referenced this pull request Feb 15, 2024
# [1.2.0-alpha.2](v1.2.0-alpha.1...v1.2.0-alpha.2) (2024-02-15)

### Bug Fixes

* assorted error handling fixes ([#52](#52)) ([234f883](234f883))
* dynamic class property deprecated warnings ([#47](#47)) ([693865c](693865c))
* prevent coauthors capability check infinite loop ([#46](#46)) ([9565cef](9565cef))
* set Yoast primary category ([#41](#41)) ([3457d19](3457d19))

### Features

* sync billing and shipping addresses ([#50](#50)) ([6a05580](6a05580))
* sync publish, trash post statuses ([#42](#42)) ([fd5d8b9](fd5d8b9))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.2.0-alpha.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Feb 20, 2024
# [1.2.0](v1.1.0...v1.2.0) (2024-02-20)

### Bug Fixes

* assorted error handling fixes ([#52](#52)) ([234f883](234f883))
* **data-listeners:** handle no user in wc data update ([4eeefc0](4eeefc0))
* dynamic class property deprecated warnings ([#47](#47)) ([693865c](693865c))
* prevent coauthors capability check infinite loop ([#46](#46)) ([9565cef](9565cef))
* race condition when creating a Node ([1946473](1946473))
* set Yoast primary category ([#41](#41)) ([3457d19](3457d19))

### Features

* **ci:** add epic/* release workflow and rename `master` to `trunk` ([#39](#39)) ([9cee51d](9cee51d))
* sync billing and shipping addresses ([#50](#50)) ([6a05580](6a05580))
* sync publish, trash post statuses ([#42](#42)) ([fd5d8b9](fd5d8b9))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.2.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants