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

WordPress sometimes returns JSON to a Webbrowser instead of the HTML page. #580

Closed
uk3 opened this issue Nov 27, 2023 · 36 comments
Closed
Assignees
Labels
Needs triage [Type] Bug Something isn't working

Comments

@uk3
Copy link
Contributor

uk3 commented Nov 27, 2023

Quick summary

Sometimes and for some users WordPress returns a JSON object to a regular browser request interested of the normal website.

This (sometimes) persists on reload
signal-2023-11-27-21-53-44-776

Steps to reproduce

No idea 🤷

What you expected to happen

The normal website.
Uploading Screenshot_20231127_220836_Chrome.jpg…

What actually happened

I see JSON in the browser

Impact

Some (< 50%)

Available workarounds?

No and the platform is unusable

Logs or notes

No response

@uk3 uk3 added [Type] Bug Something isn't working Needs triage labels Nov 27, 2023
@pfefferle
Copy link
Member

This is normally a caching issue! This results also in ActivityPub issues, because the API requests also see HTML from time to time.

@pfefferle
Copy link
Member

@timnolte
Copy link

@uk3 what caching plugins and/or hosting platform are you on?

@uk3
Copy link
Contributor Author

uk3 commented Nov 28, 2023

@uk3 what caching plugins and/or hosting platform are you on?

Austrian company "easyname".

I am running redis, varnish and PHP Opcache. (All three are provided by the hoster)

@pfefferle
Copy link
Member

redis and OPcache should be no issue, because they only cache objects or PHP code.

varnish on the other hand is an output cache and could cause issues when not configured to differentiate by Accept header.

Maybe this link helps https://dustinrue.com/2023/09/wordpress-activitypub-and-cloudflare/ or this one #446 (reply in thread)

@uk3
Copy link
Contributor Author

uk3 commented Nov 28, 2023

Hi,
thanks for the pointers. It seems that the solution is quite simple:

# .htaccess
<IfModule mod_headers.c>
    Header merge Vary "Accept"
</IfModule>

Now (since i was able to reproduce it with the hints), it seems to be fixed and different content is served based on the Accept-Header.

@pfefferle
Copy link
Member

I will close it then, please re-open when still an issue!

@snarfed
Copy link

snarfed commented Dec 29, 2023

Looks like this is hitting other users: https://mozilla.social/@wedistribute/111660443230760980 . More background: https://snarfed.org/2023-12-28_we-distribute-in-mozilla-social-lrhodes-rndanger-tchamb-mozilla-social

We discussed a bit on #indieweb-dev. Consider adding Accept to Vary in the plugin itself to prevent this? Might be worthwhile since this seems like a pretty ugly bug, even if it's not that common.

@timnolte
Copy link

Just noting that Accept: Vary is not necessarily a silver bullet for all cases. Configuring that generically doesn't work for the OpenLiteSpeed caching.

@snarfed
Copy link

snarfed commented Dec 29, 2023

Interesting! I assume you mean Vary: Accept. Sounds like OpenLiteSpeed may not be handling Vary correctly, or something else is going on? Vary: Accept is definitely necessary here, and should fix this bug in the majority of cases, but sure, maybe not 100%.

@timnolte
Copy link

Well, there is another problem with the Vary: Accept header is that it is not specific to the content type. This could inflate the cache and cause many more cache misses ballooning the cache storage volume required with unnecessary cache objects that are actually duplicates. The other problem is that this could also cause issues with preventing the Accept-Encoding from working causing compressed response issues. There can be only 1 Vary header.

@timnolte
Copy link

You can read some more about how this can happen where standard web requests from different browsers are not all standard in the requests they may send.

https://www.fastly.com/blog/best-practices-using-vary-header

You might wonder if all browsers these days send the same Accept-Encoding header.

Unfortunately, the answer is no.

I sampled 100,000 requests on one of our caches, and got 44 different Accept-Encoding headers. (For those interested in how I did that, or the numbers, see here)

If all of those requests had been for the same URL, we would have had 44 "different" versions in our cache.

My concern is adding this into the plugin, in a generic blanket way, has the risk of even more toll on a users web server and caching than they had expected. This setting should be an option only with sufficient warning and caution for monitoring to users.

@timnolte
Copy link

Also, I understand that my specific quote is about the Accept-Encoding variant of using the Vary header but the concept still applies to the even more generic Accept header value.

@snarfed
Copy link

snarfed commented Dec 29, 2023

Definitely understood! You can include multiple headers in Vary, eg Vary: Accept Accept-Encoding. As for bloating caches, you're right, I'm very familiar with that problem.

However, as far as I can tell, without Vary: Accept ... or an equivalent, the plugin will currently serve unreadable JSON instead of usable HTML to an arbitrary number of normal users in browsers. I do get the downsides, and I'd love to find an alternative with better tradeoffs! But barring that, I can't imagine many cases where I would knowingly choose to serve users garbage in order to preserve cache efficiency.

One alternative would be to change the plugin to use different URLs as AP object ids, instead of reusing post URLs as is. That would avoid this problem more or less entirely. However, AP object ids can't easily be changed once they've been used in the wild, so realistically the plugin would need to preserve object ids that it's already published as is, which means that existing users would all still need Vary: Accept ....

@pfefferle
Copy link
Member

@timnolte @snarfed what about adding the Vary header latest possible and only if we are sure it is an ActivityPub request?

Maybe here:

Then we shouldn't get the issue of cache flooding on classic requests, but only on AP requests?

Does that make sense?

@snarfed
Copy link

snarfed commented Dec 29, 2023

The problem is, I expect HTML responses are also getting cached and served to AP requests for AS2 JSON. If you only serve Vary: Accept to AP requests, you'll fix the bad user-facing responses, but not the bad AP responses.

(And apologies if I got heated here! I guess I care about correctness and the AP ecosystem, but I know you all do too. In the end, of course, it's entirely up to you all what you choose to do!)

@timnolte
Copy link

timnolte commented Dec 29, 2023

I totally get the passion. As a support developer at my agency and a WordPress plugin developer I'm also thinking about the potential impacts and the wider aspects of just WordPress hosting. I spent a good amount of time myself digging into a working OpenLiteSpeed caching configuration that can target the AP JSON & HTML requests properly.

My biggest concern is that, depending on the caching setup someone is using on their site, if they are on a more limited hosting plan a caching setup that inflates the cache exponentially could end up costing users a lot of money unexpectedly. That is the last thing I'd want to see happen to someone. If the choice was between potentially serving bad data to an end user or potentially costing a site owner extra money they didn't plan for I'd live with potentially serving bad data.

@timnolte
Copy link

FYI, I'm also not saying we should completely eliminate the idea of adding Vary: Accept but I think if we add it initially make it not on my default but as an option that can be turned on with some clear documentation and notice about the potential impacts. If a user knows that this could balloon their cache and cause an increase in storage, and potential cost, when enabling it then we are giving end users the control to make that decision for themselves.

@snarfed
Copy link

snarfed commented Dec 29, 2023

Good points! Thanks for the detailed explanation. Sounds like we may need an option for whether to serve Vary: Accept, along with loud warnings in both the documentation and the admin panel describing the symptoms (on either side) and how the option can fix them. After that, the next important decision is whether to default it on or off. I'll defer that one to you all!

@pfefferle
Copy link
Member

pfefferle commented Dec 29, 2023

@snarfed the thing is, that the Vary header does not magically solve all caching issues, otherwise I surely would have already added it. The majority of WordPress caching plugins simply ignore the header! So we still have to fight with or against that caching issue!

To my Vary header proposal (apologies if this sounds naive, as the Vary concept is relatively new to me): If we don't add the header to HTML requests, they'll be cached as before. For AP JSON requests, we'd instruct the cache to use a different (special) bucket through the Vary header. Why might this not work? I assume the special bucket won't be used to serve requests without the Vary header, ensuring they still receive HTML responses. On AP JSON requests, the cache will check the designated AP bucket instead, right?

@snarfed
Copy link

snarfed commented Dec 29, 2023

You're right about internal WP caching plugins! You need Vary to make external caches along the HTTP serving path work right, and you may need to do something else for internal caching plugins too.

The problem with only Varying AP responses is that it gives external caches inconsistent instructions. When an external cache first sees an HTML response without Vary, it interprets that as "I can cache and return this response again to any other request for the same URL, regardless of request headers." It won't see an AP response with Vary unless that first cached response expires and the next request it sees is an AP request.

@pfefferle
Copy link
Member

pfefferle commented Dec 29, 2023

So you are saying that the external cache is only checking the headers if there is nothing in the cache? But how could the cache know where to look at? I would assume, that it at least has to check the Vary header for every request?!?

But to maybe limit it otherwise: Could we add the Vary header only to URLs that might be relevant for AP? So maybe start with only adding it to the Author ID URLs, maybe followed by single Post ID URLs?!?

So maybe adding it here:

@snarfed
Copy link

snarfed commented Dec 29, 2023

So you are saying that the external cache is only checking the headers if there is nothing in the cache? But how could the cache know where to look at? I would assume, that it at least has to check the Vary header for every request?!?

When there's nothing in the cache, it passes the request through to your server to get a response to serve. When the cache has an HTML response with no Vary header, it serves that response to all requests for the same URL. It won't pass those requests to your server - that's the point of caching 😁 - so it won't see a Vary header until the cached response expires.

More: https://www.fastly.com/blog/best-practices-using-vary-header#normalization

But to maybe limit it otherwise: Could we add the Vary header only to URLs that might be relevant for AP? So maybe start with only adding it to the Author ID URLs, maybe followed by single Post ID URLs?!?

Definitely! I expect post URLs are the majority of most WordPress sites' URLs, so I don't know how much benefit this will have, but yes, good idea regardless.

@snarfed
Copy link

snarfed commented Dec 29, 2023

Btw that article also has the good idea to minimize cache bloat by normalizing headers like Accept that are included in Vary. If you do that thoroughly enough, you can prevent duplicate cache entries entirely! https://www.fastly.com/blog/best-practices-using-vary-header#normalization

The catch is that your external cache(s) need support for that kind of programmatic normalization, which may not always be easy or available.

@pfefferle
Copy link
Member

🤦‍♂️ I always make the same mistake! The Vary header is set by the responder, not the requester!!!

Sorry @snarfed and thanks for your patience!

@snarfed
Copy link

snarfed commented Jan 31, 2024

@capjamesg and I are seeing this intermittently right now on wedistribute.org, eg https://wedistribute.org/podcast/bridgyfed-ryan-barrett/ , cc @DeadSuperHero. @pfefferle do you want to maybe consider reopening this issue until you've added the Vary header?

@pfefferle pfefferle reopened this Jan 31, 2024
@pfefferle
Copy link
Member

pfefferle commented Jan 31, 2024

@snarfed the Vary header would not fix the issue! I hope this is only a glitch, otherwise I will file an internal issue, to re-check the caching (I think I know where the site is hosted 😉 ).

@pfefferle
Copy link
Member

pfefferle commented Jan 31, 2024

The site, including Content Negotiation, works fine on my end (and it would not work when having a caching issue).

@timnolte
Copy link

So I feel like the real question here is whether this plugin is going to start providing detection and support for various caching and server configurations or if that needs to be implemented into the caching plugins/services. Trying to get something into this plugin may be a pretty large maintenance effort, in terms of having to change things as those plugins/services change.

I have actually started working on implementing support for the ActivityPub plugin within the LiteSpeed Cache plugin as there are very specific .htaccess Rewrite entries that need to be included. Initially I'm proposing an option that users would turn on manually, with detection of the ActivityPub plugin is installed and activated.

@snarfed
Copy link

snarfed commented Jan 31, 2024

@pfefferle thanks for reopening, and @timnolte thanks for looking into server config!

Re Vary, I'm guessing you mean that it's maybe necessary but not sufficient? You do need to serve it to make external caches work correctly here, but if you mean that it wouldn't fix WordPress's internal caches like plugins, or maybe web server plugins outside WordPress (like @timnolte is investigating?), agreed, that may be true at least sometimes.

@timnolte
Copy link

timnolte commented Feb 1, 2024

So knowing that the LiteSpeed web servers need some specific caching configuration for ActivityPub I started thinking about Nginx caching configuration. I happened upon a Kinsta article that indicates they are sending the Vary: Accept-Encoding by default. (https://kinsta.com/knowledgebase/specify-vary-accept-encoding-header/) Along with that I found that Nginx doesn't support/allow, according to RFCs, duplicate Vary headers. (https://trac.nginx.org/nginx/ticket/1423) This may mean that on some hosting automatically trying to apply the Vary header from the ActivityPub plugin could completely break caching altogether.

@snarfed
Copy link

snarfed commented Feb 1, 2024

Interesting, nice sleuthing @timnolte! Reading the bottom of that nginx ticket, it looks like they actually fixed this back in May 2022, so nginx now does handle multiple Vary headers.

(I also doubt the interpretation of Vary as not comma-separated - and if it is, then Kinsta's behavior is arguably wrong, and they should update any existing header value instead of adding a new one - but all that probably doesn't matter if nginx supports multiple header values now.)

@timnolte
Copy link

timnolte commented Feb 1, 2024

Grr, stupid GitHub app commenting is garbage. Reposting as I was trying to delete the appearance of a duplicate comment.

So to restate. The Nginx fix was to address a 43 character Vary headset length limit issue. The fix I don't believe allows multiple instances of the Vary header as stated in the RFC.

@timnolte
Copy link

timnolte commented Feb 1, 2024

@snarfed also I don't see any issues with the Kinsta header settings. What should be tested is setting up Nginx caching with the the Vary: Accept-Encoding header set at the server level and then having the ActivityPub plugin also try and set the Vary: Accept header and test how the Nginx caching responds.

@snarfed
Copy link

snarfed commented Feb 1, 2024

So to restate. The Nginx fix was to address a 43 character Vary headset length limit issue. The fix I don't believe allows multiple instances of the Vary header as stated in the RFC.

Huh. Are we maybe looking at different nginx changes? I'm looking at https://trac.nginx.org/nginx/changeset/cd73509f21e2daa817bf5e9074d266277915c941/nginx , linked from https://trac.nginx.org/nginx/ticket/1423#comment:7 . The message for that changeset is Upstream: handling of multiple Vary headers (ticket #1423). Previously, only the last header value was used when caching. Looking at the code, it adds a loop over the u->headers_in.vary linked list that concatenates all of the header values together, separated by commas.

It does tweak a usage of NGX_HTTP_CACHE_VARY_LEN, but I don't see that it changes that constant's value.

@timnolte
Copy link

timnolte commented Feb 1, 2024

@snarfed OK, I think I was perhaps misreading "multiple header values" in terms of multiple values in the Vary header entry, which are supposed to be comma separated. I wasn't quite interpreting that as multiple instances of the Vary header. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs triage [Type] Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants