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

Built-in privacy plugin does not replace glightbox plugin's href #6248

Closed
4 tasks done
Guts opened this issue Oct 26, 2023 · 11 comments
Closed
4 tasks done

Built-in privacy plugin does not replace glightbox plugin's href #6248

Guts opened this issue Oct 26, 2023 · 11 comments
Labels
change request Issue requests a new feature or improvement upstream Issue must be taken upstream

Comments

@Guts
Copy link
Contributor

Guts commented Oct 26, 2023

Context

I hesitated before creating this issue because I'm not sure that the privacy plugin has to support other plugins, like glightbox here. But since this plugin is listed in the theme documentation, I thought it was at least interesting to point it out.

cc @blueswen

Bug description

Privacy plugin does not replace image URL in href HTML tag added by the glightbox plugin. So, a call to external website is still performed, breaking the privacy behavior and also the offline promise.

Related links

Reproduction

9.4.6+insiders.4.42.2-privacy-not-handling-glightbox.zip

Steps to reproduce

  1. Create a mkdocs setup with glighbox and privacy plugins enabled

  2. Write some markdown referencing external image:

    # Page with image
    
    ![test image](https://squidfunk.github.io/mkdocs-material/assets/images/spotlight/social-cards.png)
  3. Build the website

  4. See the output HTML:

    <a class="glightbox"
      href="https://squidfunk.github.io/mkdocs-material/assets/images/spotlight/social-cards.png"
      data-type="image"
      data-width="100%"
      data-height="auto"
      data-desc-position="bottom">
        <img 
          alt="test image"
          src="../assets/external/squidfunk.github.io/mkdocs-material/assets/images/spotlight/social-cards.png">
    </a>
  5. See the network tab reporting calls to external source:

Screenshot from 2023-10-26 13-59-21

Browser

No response

Before submitting

@blueswen
Copy link
Sponsor Contributor

This issue is related to mkdocs-glightbox, which initially retrieves the src attribute from the img tag and sets it as the href for the a tag, to serve as the source for the lightbox image (https://github.com/blueswen/mkdocs-glightbox/blob/v0.3.4/mkdocs_glightbox/plugin.py#L131). I will take time this weekend to find anything I can do.

@squidfunk squidfunk added the needs investigation Issue must be investigated by the maintainers label Oct 26, 2023
@Guts
Copy link
Contributor Author

Guts commented Oct 26, 2023

This issue is related to mkdocs-glightbox, which initially retrieves the src attribute from the img tag and sets it as the href for the a tag, to serve as the source for the lightbox image (https://github.com/blueswen/mkdocs-glightbox/blob/v0.3.4/mkdocs_glightbox/plugin.py#L131). I will take time this weekend to find anything I can do.

As said, I've hesitated where to create this issue. If I created it on this repo, that's because your plugin does not promise anything about privacy so its behavior is something normal.

But I think this case is interesting: are the theme contributors guarantee some use cases related to the theme features but also to 3rd party plugins if they are mentioned/recommended in the documentation?

Speaking about the plugin, maybe it can behave differently depending on theme config?

@squidfunk
Copy link
Owner

Thanks for reporting. We'll look into it.

are the theme contributors guarantee some use cases related to the theme features but also to 3rd party plugins if they are mentioned/recommended in the documentation?

We cannot guarantee anything when it comes to third-party plugins, as they are not under our control. We do our best to ensure that our plugins work with as many third party plugins as possible. Without looking at it I'm quite confident that we can and should fix this on our side, but I need some time to investigate.

I haven't looked at the source of the glightbox plugin yet, but the privacy plugin runs after other plugins that do not need explicit priorities while leaving the possibility for other plugins to run after the privacy plugin. We're using event priorities for that, currently at -50. It might be related to an ordering issue, but I'm not sure.

Speaking about the plugin, maybe it can behave differently depending on theme config?

I'm not sure what you mean by that.

@squidfunk
Copy link
Owner

squidfunk commented Oct 26, 2023

Okay I think I know what might be happening. The privacy plugin replaces all external scripts, styles and images that a browser downloads in on_post_page, so after all rendering has taken place. We're using regular expressions for that, corresponding with the following selectors:

  • img[src]
  • script[src]
  • link[href]

It seems that the plugin takes the img[src] and injects it into a[href] before the privacy plugin does its job. The privacy plugin does not replace external links to assets, because those are not downloaded inline by the browser, thus there is no danger regarding GDPR. I'm not sure this is a valid use case that we should address, because it would mean that we would download all external assets, or the author would need to explicitly exclude them.

Thus, I'm not sure whether we can fix it on our side.

@Guts
Copy link
Contributor Author

Guts commented Oct 26, 2023

I'm not sure what you mean by that.

Something like:

If config.theme.name == material and privacy in config.plugins and config.plugins.privacy.enabled:

    # handle href differently 

@Guts
Copy link
Contributor Author

Guts commented Oct 26, 2023

We cannot guarantee anything when it comes to third-party plugins, as they are not under our control. We do our best to ensure that our plugins work with as many third party plugins as possible.

Sure, I completely understand with your point of view and I adhere to the underlying principles.
It's good to say it again 😉. Maybe to mention it explicitly on doc sections related to third-party plugins?

@squidfunk
Copy link
Owner

I've investigated and came to the conclusion that it does not make sense to fix this on our side. Here's why:

Problem

The mkdocs-glightbox plugin currently detects img elements during build time (after the site has been rendered as HTML) and wraps them with a anchor elements. As already noted in #6248 (comment), the privacy plugin only considers assets that are actually rendered to be downloaded. However, the glightbox plugin will wrap the image in an anchor and set href to the image. Normally, an a is a link to an external or internal resource – nothing that is rendered or downloaded without the users consent, thus not under GDPR. This is beyond the scope of the privacy plugin.

Why? Because if we would start downloading resources that are referenced in a[href] attributes, we would need another way to tell the plugin what not to download. Otherwise the plugin will try to download all external resources, which will make configuration much more challenging and the final site will probably blow up in size.

Possible solution

IMHO, there's no need for the glightbox plugin to wrap the img elements during build time – glightbox needs JavaScript to function (open the inspector when you click on an image, and you will see that the JavaScript changes several properties, e.g. on the body). As there's no possibility to make it work without JavaScript (correct me if I'm wrong), I think it's a better idea to defer wrapping of image elements into the browser. The glightbox plugin could just mark elements with a specific class or attribute, e.g. data-glightbox, and provide a small JavaScript that wraps the img elements when the page has loaded. This would mean the correct image would be picked up every time.

This would not only solve the problem at hand, but also be easier to extend to other resources to be presented in the ligtbox, because very soon, the optimize plugin will receive the capability to generate multiple variants of images and wrap them in picture tags. The glightbox plugin could the pick the appropriate source in the picture tag.

If you need help getting this to work, @blueswen, let me know. Wrapping an element in JavaScript is quite simple..

Closing as upstream issue.

@squidfunk squidfunk closed this as not planned Won't fix, can't repro, duplicate, stale Oct 27, 2023
@squidfunk squidfunk added change request Issue requests a new feature or improvement upstream Issue must be taken upstream and removed needs investigation Issue must be investigated by the maintainers labels Oct 27, 2023
@Guts
Copy link
Contributor Author

Guts commented Oct 28, 2023

Thanks for your time to investigate and your detailed answer.

@blueswen I'm not JS good enough to help you on this but if you want, I can create an issue on your repo?

@blueswen
Copy link
Sponsor Contributor

@Guts Creating an issue on my repo would be great. I'll take some time to address this.

@squidfunk Thanks for the detailed information and taking the time to investigate this. I'd love to improve compatibility with Material Theme.

@squidfunk
Copy link
Owner

@blueswen let me know if you want to go down the JavaScript path, and when you run into trouble. Happy to assist.

@Guts
Copy link
Contributor Author

Guts commented Nov 10, 2023

Upstream issue created: blueswen/mkdocs-glightbox#25

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change request Issue requests a new feature or improvement upstream Issue must be taken upstream
Projects
None yet
Development

No branches or pull requests

3 participants