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

Emojis are not working w/ privacy plugin #2063

Closed
jonaharagon opened this issue Mar 1, 2023 · 16 comments
Closed

Emojis are not working w/ privacy plugin #2063

jonaharagon opened this issue Mar 1, 2023 · 16 comments
Labels
t:bug website bugs or errors

Comments

@jonaharagon
Copy link
Member

jonaharagon commented Mar 1, 2023

Tracking two separate issues I'm having with the mkdocs-material privacy plugin:

  1. Adding emojis to a page (like :gear: on tor.md) results in a broken image because the privacy plugin is not replacing the asset correctly:

    Screenshot 2023-03-01 at 13 59 09@2x

    The broken image is due to CSP not allowing external assets, not because the external asset doesn't exist and the plugin couldn't grab it.

  2. Adding a flag emoji to a page, for example file-sharing.md here, results in a build failure. Fixed upstream.

@jonaharagon jonaharagon added the t:bug website bugs or errors label Mar 1, 2023
@kamilkrzyskow
Copy link

kamilkrzyskow commented Mar 1, 2023

The traceback shows that the issue is related to the privacy plugin. Maybe try to make a minimal example and check it's not a bigger issue with that plugin specifically.

@kamilkrzyskow
Copy link

kamilkrzyskow commented Mar 1, 2023

In your most recent deployment preview https://app.netlify.com/sites/privacyguides/deploys/63ff870b6a65bc0008b0fe28
There is:

6:11:29 PM: INFO     -  Downloading external file: https://cdnjs.cloudflare.com/ajax/libs/twemoji/14.0.2/svg/2699.svg
6:11:30 PM: INFO     -  Downloading external file: https://cdnjs.cloudflare.com/ajax/libs/twemoji/14.0.2/svg/2764.svg

which indicates that it should work. GitHub search didn't show any results but a local file search has lead me to multiple built files including:
https://github.com/privacyguides/privacyguides.org/blob/main/docs/tor.md?plain=1#L83

... :gear: ...

So it does partially work 🤔

@jonaharagon
Copy link
Member Author

Very interesting. The gear icon is actually broken on https://www.privacyguides.org/en/tor/#orbot though now that I'm looking at it, which is the problem I was initially facing at squidfunk/mkdocs-material#3851 almost a year ago. It's extremely weird how the privacy plugin is sometimes breaking the build entirely, and sometimes just not replacing the external file.

It's also very frustrating that it seems like this is only happening to me 🤔

@jonaharagon
Copy link
Member Author

I can't reproduce my :gear: issue on a minimal install which means I'll have to keep poking at it even though it makes no sense. The flag issue is an actual upstream issue that I opened a bug report about now.

@jonaharagon jonaharagon changed the title Flag emojis can't be used Emojis are not working w/ privacy plugin Mar 1, 2023
@squidfunk
Copy link

It seems to be related to how lxml handles Unicode. It's a mess.

@kamilkrzyskow
Copy link

kamilkrzyskow commented Mar 1, 2023

I can't reproduce my :gear: issue on a minimal install which means I'll have to keep poking at it even though it makes no sense. The flag issue is an actual upstream issue that I opened a bug report about now.

It works on my TempFork from yesterday, so it's likely another issue with the HTML.
Ok, it's not, however I don't understand how exactly CSP influences this:
image

@jonaharagon
Copy link
Member Author

If I remove pymdownx.details from the markdown extensions in mkdocs.common.yml my :gear: emoji issue is fixed, but I can't reproduce this with a minimal install yet so I'm still not sure if it's ultimately an issue here or upstream 🤔

@jonaharagon
Copy link
Member Author

however I don't understand how exactly CSP influences this:

Our CSP blocks loading images from external sources. This is a privacy decision, obviously we could just fix it by adding cdnjs.cloudflare.com to the CSP, but the privacy plugin is supposed to convert the image src from https://cdnjs.cloudflare... to assets/external/cdnjs.cloudflare.com/ajax/libs/twemoji/14.0.2/svg/2699.svg and serve it locally, but in this case that is not working for some reason.

@squidfunk
Copy link

Potential fix upstream.

@jonaharagon
Copy link
Member Author

The plot thickens, removing any of these three lines from the mkdocs config also fixes the issue with the gear emoji on the tor page, which makes no sense 🤔

@kamilkrzyskow
Copy link

The plot thickens, removing any of these three lines from the mkdocs config also fixes the issue with the gear emoji on the tor page, which makes no sense 🤔

It feels almost random... squidfunk last time said that changing the order of the plugins in the mkdocs.yml file doesn't affect the order of execution, because the privacy plugin uses the MkDocs event priorities. However, he also said that it works concurrently, so maybe there is an edge-case where instead of the replaced private resource it gets the results from another Markdown parser without the replaced path 🤔. In the Netlifly log the resource was downloaded, but the path wasn't replaced. Those emojis are used on multiple pages, so maybe another page triggered the download and got a correctly swapped path, and the issue lies primarily in the tor.md file 🤷

@jonaharagon
Copy link
Member Author

Looking through.. that gear is used on other pages and loads normally at https://www.privacyguides.org/en/mobile-browsers/#recommended-configuration_1 for example.

It's very inconsistent though. Actually the problem doesn't seem to be limited to just emojis at all, the privacy plugin just generally isn't replacing all assets. Half the pages (including the Tor page) have externally-linked contributor avatars for example:

Screenshot 2023-03-01 at 16 13 10@2x

...while the other half work fine:

Screenshot 2023-03-01 at 16 12 13@2x

But it isn't even consistent on a page-by-page basis. The mobile browsers page has correctly embedded emojis but broken contributor avatars 🤔

@squidfunk
Copy link

However, he also said that it works concurrently, so maybe there is an edge-case where instead of the replaced private resource it gets the results from another Markdown parser without the replaced path

That could be a bug in the plugin, but it would be surprising, as we deliberately reconcile all downloads before applying replacements. Specifically:

  1. Find all images in on_page_content and enqueue them for downloading.
  2. Reconcile after parsing all rendered Markdown files (= HTML) for external images and downloading them.
  3. Replace external assets. There might be other assets in the final HTML (plugins may add images to templates like the git-authors-plugin), which is why they need to be downloaded synchronously (no concurrent flag).

There could still be race conditions. I tried to think of all cases – I recommend to read the comments in the plugin. If a reproduction could be provided, that would be amazing. Happy to fix it and improve the plugin.

@jonaharagon
Copy link
Member Author

I was making this way too complicated in my head. I think this is just a matter of it not handling a lot of external resources on a page correctly. Add enough gears and eventually it breaks:

Screenshot 2023-03-01 at 17 05 26@2x

@squidfunk I can easily reproduce this on a two-file install: Archive.zip

@kamilkrzyskow
Copy link

kamilkrzyskow commented Mar 1, 2023

The plugin wasn't always concurrent, right? Then maybe an older version of the plugin without concurrency might show different results and therefore reassure the idea of it being an issue with the concurrent design itself 🤔However, I don't know if the fix today was related to concurrency or if it fixed something else entirely.

Ok, brute-force debugging with a lot of external resources on the page is much better 👍

@jonaharagon
Copy link
Member Author

Since this does seem to be an upstream plugin issue I'll close this and track squidfunk/mkdocs-material#5127 instead :)

@jonaharagon jonaharagon closed this as not planned Won't fix, can't repro, duplicate, stale Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t:bug website bugs or errors
Development

No branches or pull requests

3 participants