Prevent infinite redirect for doc-router `/doc[^/]` #751

Noratrieb · 2025-07-19T20:27:14Z

Previously, it would just put the slice in the Location with no regards
for what was in there. If there was no slash after the /doc, it would
issue a relative redirect, which was still under /doc, causing an
infinite redirect.

/docs/std -> s/std
/docs/s/std -> s/s/std
/docs/s/s/s/std -> ...

We prevent this behavior here by only applying the /doc redirect if
there is a slash directly after it, causing the default /stable rewrite
fallback to be applied to everything else, which will in practice lead
to nice 404 page.

This should not break anything as all such queries currently run into
these loops, including plain /doc.

Making a change to this core part of infrastructure that I can't reasonably test locally scared me, so I just wrote a bunch of unit tests for the thing.

Noratrieb · 2025-07-19T20:30:20Z

I discovered this because someone else found this and posted about it, seemingly because they made a typo and forgot the URLs or something like that.

marcoieni · 2025-07-21T07:57:34Z

thanks for this. The change itself looks good. Regarding the tests, can you document how to run them?

Noratrieb · 2025-07-21T08:20:39Z

There is a comment at the top of the tests that mentions it, just node --test test.mjs

Noratrieb · 2025-07-21T08:21:05Z

wait what that comment got lost, I'll add it

Noratrieb · 2025-07-21T10:24:31Z

It looks like my regression test for the issue also got lost, I'll need to redo that

marcoieni · 2025-07-21T13:30:22Z

I converted the PR to draft so that it's clear that it's not ready to be reviewed yet 👍

Noratrieb · 2025-07-21T17:40:57Z

Ah no, I did make a regression test, just not as part of the table tests. I squashed the comment fix I made earlier into the commit and it should be ready now.

terragrunt/modules/release-distribution/lambdas/doc-router/test.mjs

marcoieni · 2025-07-22T10:25:23Z

also it would be great if you could run the test in CI.

Noratrieb · 2025-07-22T16:13:56Z

I updated the naming and added a new CI workflow based on the previous ones. It's hardly the greatest workflow but it just does the same as the existing ones.

...this test now fails though. it works for me locally. it looks like it redirects / to /stable. live https://doc.rust-lang.org/ seems to do the same, but the source code obviously redirects it to www.rust-lang.org??

Noratrieb · 2025-07-22T16:17:33Z

oh, this was changed on master and my branch was outdated!

Previously, it would just put the slice in the Location with no regards for what was in there. If there was no slash after the `/doc`, it would issue a relative redirect, which was still under `/doc`, causing an infinite redirect. - /docs/std -> s/std - /docs/s/std -> s/s/std - /docs/s/s/s/std -> ... We prevent this behavior here by only applying the `/doc` redirect if there is a slash directly after it, causing the default /stable rewrite fallback to be applied to everything else, which will in practice lead to nice 404 page. This should not break anything as all such queries currently run into these loops, including plain `/doc`.

marcoieni · 2025-07-23T08:00:28Z

applied, thanks!

marcoieni marked this pull request as draft July 21, 2025 13:29

Noratrieb force-pushed the fix-infinite-doc-loop branch from d7ce21a to f4e93a0 Compare July 21, 2025 17:40

Noratrieb marked this pull request as ready for review July 21, 2025 17:40