JustinBeckwith / linkinator

🐿 Scurry around your site and find all those broken links.
MIT License
1.04k stars 80 forks source link

Fix broken redirect #596

Open maddsua opened 8 months ago

maddsua commented 8 months ago

This fix covers an edgecase when a link that has search params and resolves to a directory (think of /checkout?services=setup-cctv which resolves to a file dist/checkout/index.html) is reported as broken.

The issue lies in an oversight in src/server.ts:83, where request url is assumed to not have search params when a slash appended to it to implement a directory redirect.

It's fixed with actually parsing URL and appending slash only to pathname instead of the entire url string. Also, I implemented it as a separate URL object to not make things complicated - one object for the original URL and the other for redirect URL specifically.

JustinBeckwith commented 4 months ago

Hey, thanks for the PR! Sorry I didn't get to this sooner. To be honest, it's kind of hard for me to tell from your issue and this PR exactly what was broken before 🤔 Could I trouble you to add a test case that demonstrates what's not working? Verify it's broken before your change, and fixed after? Plenty of comments 😆 Thanks!

maddsua commented 4 months ago

Added a test case. It is really similar to the one above it, the only difference is that my fix makes search params work regardless of whether url points to a directory or not.

Also, after pulling new commits I've noticed that typescript started complaining about a type error at: https://github.com/JustinBeckwith/linkinator/blob/929caa7e776ed22393a2437364d502925322563e/src/server.ts#L106

Basically, ts isn't happy about that await

maddsua commented 4 months ago

Hello again! Any updates? Would love to have this fixed as soon as possible. Same goes for any additional changes you may find necessary

maddsua commented 3 months ago

For more context, this is how it fails in one of the projects I am working on:

> check-links
> npx linkinator dist --recurse --silent --skip ^https.+

🏊‍♂️ crawling dist
[0] dist\forms\signup?pid=t1_inet_basic
[0] dist\forms\signup?pid=t2_inet_premium
[0] dist\forms\signup?pid=t3_inet_vip
[0] dist\en\forms\signup?pid=t2_inet_premium
[0] dist\en\forms\signup?pid=t3_inet_vip
[0] dist\en\forms\signup?pid=t1_inet_basic
dist
  [0] dist\forms\signup?pid=t1_inet_basic
  [0] dist\forms\signup?pid=t2_inet_premium
  [0] dist\forms\signup?pid=t3_inet_vip
dist\en\
  [0] dist\en\forms\signup?pid=t2_inet_premium
  [0] dist\en\forms\signup?pid=t3_inet_vip
  [0] dist\en\forms\signup?pid=t1_inet_basic
ERROR: Detected 6 broken links. Scanned 144 links in 0.436 seconds.

I am genuinely surprised that this case was not caught before.

maddsua commented 3 months ago

Hey @JustinBeckwith , I don't wanna be annoying but any chance of you getting another look at this?

I love open source for giving us all a way of sharing amazing tools an collaborating on them instead of writing everything from scratch or forking everything just to add a small feature or fix.

Would absolutely hate to give up on linkinator because of this small but critical for me issue

maddsua commented 1 month ago

@JustinBeckwith maybe you could give an ETA on this one?