processing / p5.js-website

New p5.js website!
http://p5js.org
MIT License
17 stars 83 forks source link

Incorrect og:url and twitter:url Meta Tags point to localhost:4321, causing Mixed Content Issues on p5.js website #523

Closed drashyakuruwa closed 1 month ago

drashyakuruwa commented 2 months ago

Most appropriate sections of the p5.js website?

Tutorials

What is your operating system?

Windows

Web browser and version

Version 128.0.6613.120 (Official Build) (64-bit)

Actual Behavior

When visiting pages such as https://p5js.org/tutorials/get-started/ and several tutorial pages, Mixed Content errors appear in the browser console. These errors indicate that while the page is served over HTTPS, it attempts to load resources over HTTP, which is blocked by modern browsers for security reasons.

image

When exploring the code it appears like the issue is in the file src/components/Head/index astro and the problematic lines are:

Line 42: meta property="og:url" content=(Astro.url.href) Line 50: meta name="twitter:url" content={Astro.url.href)

In Astro, Astro.url does not exist by default. To dynamically generate the URL, you need to access the current page's URL using the appropriate method or variable. Unfortunately, Astro.url.href is likely causing problems because it isn't a valid API in the Astro framework.

Expected Behavior

Steps to reproduce

  1. Visit any tutorial page on the p5.js website (for example, (https://p5js.org/tutorials/get-started/)).
  2. Inspect the page's source code (right-click > View Page Source).
  3. Locate the tags with the name="twitter:url" and property="og:url" attribute.
  4. Observe that the URL is http://localhost:4321/tutorials/get-started/ instead of the correct live URL.

image

Would you like to work on the issue?

Yes, I can work on the issue, have traced the problematic line and I know the fix.

limzykenneth commented 1 month ago

Hi @drashyakuruwa thanks for identifying where the issue comes from but can you briefly describe what the proposed fix would be?

drashyakuruwa commented 1 month ago

Hello @limzykenneth, the fix of this would be to use Astro.request.url instead, as we can get the raw URL directly as requested by the client using that:

<meta property="og:url" content={Astro.request.url} />
<meta name="twitter:url" content={Astro.request.url} />

this provides the exact URL string as seen in the browser, including the domain, path, and query parameters. So, in development, we will still see the localhost URL instead of the production URL, but when deployed, Astro.request.url would provide the actual live URL requested by the client.

limzykenneth commented 1 month ago

@drashyakuruwa I don't think that will work either, Astro.url is constructed from Astro.request.url and so Astro.url.href should basically have the same value as Astro.request.url in default configuration, the other thing is that Astro does not have a run time, it is statically built and the URL values are determined at built time not runtime, which means that there is no way for Astro.request.url to know the actual live URL requested by the client.

The suggested solution in Astro's docs is to set the site configuration instead which will be populated for Astro.url at built time with the set value.

drashyakuruwa commented 1 month ago

@limzykenneth Ohh, yes, this makes sense now! I misunderstood Astro's static nature. I see that adding site: 'https://p5js.org' in the astro.config.mjs file is the right approach. Using Astro.site, we can check if the URL matches the production URL and set a fallback for local development. Thank you for helping clarify that!

limzykenneth commented 1 month ago

Adding site: 'https://p5js.org' should be all that is needed to fix this and I don't think we need to implement anything else. @drashyakuruwa If you'd like to create a PR that add that config you can go ahead. Thanks.

drashyakuruwa commented 1 month ago

@limzykenneth yes yes, I would like to do it, I will create it, Thank you.

drashyakuruwa commented 1 month ago

@limzykenneth There is already PR raised by Shibom for #547 , which has the fix I was going to do.

limzykenneth commented 1 month ago

Thanks @drashyakuruwa. This is now fixed by #547