renovatebot / renovate

Home of the Renovate CLI: Cross-platform Dependency Automation by Mend.io
https://mend.io/renovate
GNU Affero General Public License v3.0
17.4k stars 2.28k forks source link

Bad link escaping on a Medium post #15958

Open rarkins opened 2 years ago

rarkins commented 2 years ago

How are you running Renovate?

Mend Renovate hosted app on github.com

If you're self-hosting Renovate, tell us what version of Renovate you run.

No response

Please select which platform you are using if self-hosting.

No response

If you're self-hosting Renovate, tell us what version of the platform you run.

No response

Was this something which used to work for you, and then stopped?

I never saw this working

Describe the bug

Fastify release 4.0.0: https://github.com/fastify/fastify/releases/tag/v4.0.0

PR updating it: https://github.com/renovate-reproductions/fastify-4/pull/2

Original text: We are finally shipping Fastify v4, you can read more about it at https://medium.com/@fastifyjs/fastify-v4-ga-59f2103b5f0e

Broken link: We are finally shipping Fastify v4, you can read more about it at https://medium.com/[@​fastifyjs/fastify-v4-ga-59f2103b5f0e](https://togithub.com/fastifyjs/fastify-v4-ga-59f2103b5f0e)

We need to work out why it's being transformed like this.

@waghanza I think this is the problem you intended to alert me to? I didn't see any changelogs in your PR so didn't notice this at first.

Relevant debug logs

Logs ``` Copy/paste the relevant log(s) here, between the starting and ending backticks ```

Have you created a minimal reproduction repository?

I have linked to a minimal reproduction repository in the bug description

Gabriel-Ladzaretti commented 2 years ago

This bug is caused because remark-github mistakenly handles @fastifyjs in https://medium.com/@fastifyjs/fastify-v4-ga-59f2103b5f0e as a "mention" and then adds its default URL to it which is broken anyway, so fixing this directly is a waste of time. here are the supported refs for remark-github.

I think that our best bet would be to do some pre-processing and handle unsupported URLs ourselves. Wrapping such URLs with angle brackets does the trick.

doing so results in this -


Fastify v4!

We are finally shipping Fastify v4, you can read more about it at https://medium.com/@​fastifyjs/fastify-v4-ga-59f2103b5f0e


related code section - https://github.com/renovatebot/renovate/blob/e987069a3aa392183c64a4ff8c4845d124762239/lib/util/markdown.ts#L30-L40

Gabriel-Ladzaretti commented 2 years ago

That said, this might be quite a rare occurrence, as this seems to happen only with unsupported urls containing '@' and we do handle these correctly.

for example this url is handled as expected when removing the '@' symbol while in debug mode and before it is being processed. funny thing is that the url is still valid ¯\(ツ)/¯ .


Fastify v4!

We are finally shipping Fastify v4, you can read more about it at https://medium.com/fastifyjs/fastify-v4-ga-59f2103b5f0e

rarkins commented 2 years ago

%40 is another possibility

Gabriel-Ladzaretti commented 2 years ago

%40 is another possibility

Just checked this case and its handled correctly -


Fastify v4!

We are finally shipping Fastify v4, you can read more about it at https://medium.com/%40fastifyjs/fastify-v4-ga-59f2103b5f0e

Gabriel-Ladzaretti commented 2 years ago

i was thinking of using encodeURIComponent to encode the URIs but this could result in a longer URIs and eating up the available space for the change log section.

what about using plain regex replace to match @ within the URI path and replacing it with %40 (as a workaround)?

This in general seems to be an edge case, do we want to address it regardless?

rarkins commented 2 years ago

I think this is a low priority edge case so let's not continue on it unless we change our mind about the priority. Of course, a community PR would be accepted

colinodell commented 2 years ago

This bug is caused because remark-github mistakenly handles @fastifyjs in https://medium.com/@fastifyjs/fastify-v4-ga-59f2103b5f0e as a "mention" and then adds its default URL to it which is broken anyway

I think this happens because remark is seeing the URL as plain text and not a GFM autolink. Adding the remark-gfm plugin would probably fix this, as it would ensure that GFM autolinks actually get parsed into a link object before remark-github runs, and remark-github contains code to avoid parsing mentions out of link nodes.

However, the presence of ​ in the URL is throwing me off. I think that's coming from sanitizeMarkdown(), but I can't find any code path where both sanitizeMarkdown() and linkify() get called. I suspect that linkify() is being called first follow by sanitizeMarkdown() sometime later but I can't confirm this. And even if I could, that would mean that even if linkify() is modified to use remark-gfm to fix the main issue as described above, sanitizeMarkdown() is still going to mangle/break that link unless we make that smarter too.

rarkins commented 2 years ago

Great research, thanks. If we could perfectly replicate GitHub's autolinking then it would make our life easier, but I'm not sure we can, even from that description. Or do you think that description is "authoritative"? I suspect it's not, and there's plenty of edge cases not described there.

viceice commented 1 year ago

I see another occurence of wrong excaped markdown

https://github.com/renovate-reproductions/broken-changelogs/pull/2

-   \[New] Implement `assert.match()` and `assert.doesNotMatch()`
-   \[Refactor] switching to a maintained Object.assign package
-   \[readme] Add description for usage with webpack and vite ([#​60](https://togithub.com/browserify/commonjs-assert/issues/60))
-   \[readme] Remove duplicate line under usage section ([#​48](https://togithub.com/browserify/commonjs-assert/issues/48))
-   \[Deps] update `is-nan`, `object-is`, `util`
-   \[Dev Deps] update `@babel/cli`, `@babel/core`, `@babel/preset-env`, `airtap`, `core-js`, `cross-eng`, `object.entries`, `object.getownpropertydescriptors`, `tape`

On github it's showing ok, but it's broken on gitea image