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

Bad redirections in URLs with trailing slashes #820

Closed
TheLortex opened this issue Jan 16, 2023 · 4 comments
Closed

Bad redirections in URLs with trailing slashes #820

TheLortex opened this issue Jan 16, 2023 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@TheLortex
Copy link
Contributor

The trailing slash middleware #170 introduced problems in some situations:

Thanks to @reynir who identified the issue in mirage.io

@cuihtlauac cuihtlauac added the bug Something isn't working label Jan 16, 2023
@cuihtlauac
Copy link
Collaborator

cuihtlauac commented Jan 16, 2023

A bit of context:

  1. Initial issue: Handle trailing slash #166
  2. Faulty PR: refactor no_trailing_slash middleware  #170
  3. Issue in mirage-www: mirage.io/community/ returns an blank 404 page mirage/mirage-www#772
  4. PR in mirage-www: Handle trailing slashes mirage/mirage-www#788
  5. New (this) issue: Handle trailing slashes mirage/mirage-www#788 (comment)

Summary: the code introduced in #170 is faulty. It went unnoticed until it was copied into mirage-www. Thanks to @TheLortex and @reynir for reporting this.

Questions:

  1. @TheLortex are you working on fixing this, or do you want to fix it first on our side?
  2. Isn't the copying suggesting this should go into dream?

@TheLortex
Copy link
Contributor Author

@cuihtlauac I'm not actively working on it, so I would be happy if you can fix it on you side.

Isn't the copying suggesting this should go into dream?

That's a complex matter because we don't know if the canonical path for given route should end by a slash or not. not ending by a slash every path is an opinionated decision so it might have difficulties going in dream. But I agree that would be useful.

@reynir
Copy link
Contributor

reynir commented Jan 24, 2023

I think it could make sense to upstream functions to Dream that help deal with the somewhat complex semantics of the Location header, at least. I agree about getting it into dream could be difficult - I personally don't like it much as it can break semantics of relative links in the html and I made a library with a different approach (though still suffering from the same bug) https://github.com/reynir/dream-normalize-route. Another point against upstreaming is that dream hasn't seen seen any activity in a while.

@cuihtlauac cuihtlauac self-assigned this Jan 30, 2023
@cuihtlauac
Copy link
Collaborator

Fixed by #866

@github-project-automation github-project-automation bot moved this to 📋 Backlog in OCaml.org Mar 10, 2023
@tmattio tmattio moved this from 📋 Backlog to ✅ Done in OCaml.org Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

3 participants