Fix redirect loop #2066
Fix redirect loop #2066
Conversation
| assert.ok(fakeNext.called); | ||
| }); | ||
|
|
||
| it('should handle trailing slash exceptions with $lang/$clientApp', () => { |
There was a problem hiding this comment.
I may have missed it, but it would be good to ensure that clientApp and or locale prefixed urls still get redirs if not excluded.
There was a problem hiding this comment.
Ah, I didn't actually test that but I can. Sure.
Edit: as in: they do but I didn't write a test for it.
|
If it hasn't already I wonder if it's worth exploring using react-router to handle the slashes instead of a middleware? The middleware will apply to everything regardless of whether we're actually handling that route or not. If we did this in the router to routes we care about that are missing them it might potentially remove a lot of unecessary redirections? It would also then mean we're always consolidating on directory style urls consistently but should not impact anything addons-server currently does differently. This would also negate the need to wrangle prefixes. See remix-run/react-router#820 (comment) for an example it seems way simpler imho. |
config/default-amo.js
Outdated
| // We use $lang and $clientApp as placeholders so we can have URLs in this | ||
| // list that don't include those URL pieces, if needed. | ||
| validTrailingSlashUrlExceptions: [ | ||
| '/$lang/$clientApp/user/edit', |
There was a problem hiding this comment.
This is nice for completeness but definitely makes the implementation more involved due to needing to wrangle the prefixes. I wonder if this could still be useful if it worked with either globs or even if this array defines path suffixes to match against e.g. don't redirect if the path endsWith /user/edit or are there cases where that would fall apart?
There was a problem hiding this comment.
I suppose that would work fine, we'd still need to split on ? to make sure we didn't get messed with by query strings but I suppose it could work.
I like that idea, I'll give it a go. |
|
We talked about this on vidyo and for now we'll still need the middleware. I've extended the test coverage and fixed an issue with query strings. I've also added back the routes backed out in #2046. And then some. Another r? would be great. |
| export function trailingSlashesMiddleware(req, res, next) { | ||
| const URLParts = req.originalUrl.split('?'); | ||
| export function trailingSlashesMiddleware( | ||
| req, res, next, { _config = config } = {} |
There was a problem hiding this comment.
Does this need to be indented?
There was a problem hiding this comment.
Yeah, it breaks 80 chars if not.
|
@tofumatt r+wc - I reckon let's land this and test it out on -dev. |
tofumatt
force-pushed
the
redirect-loop-2037
branch
from
0ec05e3 to
b6975b1
Compare
Adds a config that prevents trailing slashes from being added to certain URLs.
fixes mozilla/addons#10240