Skip to content

Commit 0a810c2

Browse files
niftylettuceniftylettuce
committed
fix: fixed redirect issue (methods needed to be considered)
1 parent 99e84bc commit 0a810c2

File tree

5 files changed

+4355
-4360
lines changed

5 files changed

+4355
-4360
lines changed

‎.eslintrc

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
11
{
2-
"extends": ["eslint:recommended", "plugin:node/recommended"]
2+
"extends": ["eslint:recommended", "plugin:node/recommended"],
3+
"rules": {
4+
"node/no-unsupported-features/es-builtins": "off"
5+
}
36
}

‎package.json

Lines changed: 28 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -21,35 +21,37 @@
2121
"Nick Baugh <[email protected]> (http://niftylettuce.com/)"
2222
],
2323
"dependencies": {
24-
"depd": "^1.1.2"
24+
"depd": "^2.0.0",
25+
"url-parse": "^1.4.7"
2526
},
2627
"devDependencies": {
27-
"@babel/cli": "^7.1.1",
28-
"@babel/core": "^7.1.1",
29-
"@babel/preset-env": "^7.1.0",
30-
"@commitlint/cli": "^7.1.2",
31-
"@commitlint/config-conventional": "^7.1.2",
32-
"ava": "^0.25.0",
33-
"codecov": "^3.1.0",
34-
"cross-env": "^5.2.0",
35-
"eslint": "5.6.1",
36-
"eslint-config-prettier": "^3.1.0",
37-
"eslint-config-xo-lass": "^1.0.2",
38-
"eslint-plugin-node": "^7.0.1",
39-
"eslint-plugin-prettier": "^2.7.0",
40-
"express": "^4.16.3",
41-
"express-session": "^1.15.6",
42-
"fetch-cookie": "^0.7.2",
28+
"@babel/cli": "^7.7.7",
29+
"@babel/core": "^7.7.7",
30+
"@babel/preset-env": "^7.7.7",
31+
"@commitlint/cli": "^8.2.0",
32+
"@commitlint/config-conventional": "^8.2.0",
33+
"ava": "^2.4.0",
34+
"cabin": "^5.0.13",
35+
"codecov": "^3.6.1",
36+
"cross-env": "^6.0.3",
37+
"eslint": "6.8.0",
38+
"eslint-config-prettier": "^6.7.0",
39+
"eslint-config-xo-lass": "^1.0.3",
40+
"eslint-plugin-node": "^10.0.0",
41+
"eslint-plugin-prettier": "^3.1.2",
42+
"express": "^4.17.1",
43+
"express-session": "^1.17.0",
44+
"fetch-cookie": "^0.7.3",
4345
"fixpack": "^2.3.1",
44-
"husky": "^1.1.1",
45-
"lint-staged": "^7.3.0",
46-
"node-fetch": "^2.2.0",
47-
"nyc": "^13.0.1",
48-
"prettier": "^1.14.3",
49-
"remark-cli": "^5.0.0",
50-
"remark-preset-github": "^0.0.8",
51-
"rimraf": "^2.6.2",
52-
"xo": "^0.23.0"
46+
"husky": "^3.1.0",
47+
"lint-staged": "^9.5.0",
48+
"node-fetch": "^2.6.0",
49+
"nyc": "^14.1.1",
50+
"prettier": "^1.19.1",
51+
"remark-cli": "^7.0.1",
52+
"remark-preset-github": "^0.0.16",
53+
"rimraf": "^3.0.0",
54+
"xo": "^0.25.3"
5355
},
5456
"engines": {
5557
"node": ">=6.4"

‎src/index.js

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
const { parse } = require('url');
1+
const Url = require('url-parse');
22
const deprecate = require('depd')('express');
33

44
module.exports = opts => {
@@ -17,7 +17,9 @@ module.exports = opts => {
1717
res.end = function(chunk, encoding) {
1818
if (!req.xhr) {
1919
req.session.prevPrevPath = req.session.prevPath;
20+
req.session.prevPrevMethod = req.session.prevMethod;
2021
req.session.prevPath = req.originalUrl;
22+
req.session.prevMethod = req.method;
2123
// if it was a redirect then store how many times
2224
// so that we can limit the max number of redirects
2325
if ([301, 302].includes(res.statusCode))
@@ -27,6 +29,7 @@ module.exports = opts => {
2729
: 1;
2830
else req.session.maxRedirects = 0;
2931
}
32+
3033
end.call(res, chunk, encoding);
3134
};
3235

@@ -51,10 +54,16 @@ module.exports = opts => {
5154
address = this.location(address).get('Location');
5255

5356
req.prevPrevPath = req.session.prevPrevPath || defaultPath;
57+
req.prevPrevMethod = req.session.prevPrevMethod || 'GET';
5458
req.prevPath = req.session.prevPath || defaultPath;
59+
req.prevMethod = req.session.prevMethod || req.method;
5560
req.maxRedirects = req.session.maxRedirects || 1;
5661

57-
if (req.prevPath && address === req.prevPath) {
62+
if (
63+
req.prevPath &&
64+
address === req.prevPath &&
65+
req.method === req.prevMethod
66+
) {
5867
if (
5968
req.prevPrevPath &&
6069
address !== req.prevPrevPath &&
@@ -64,14 +73,15 @@ module.exports = opts => {
6473
} else {
6574
// if the prevPrevPath w/o querystring is !== prevPrevPath
6675
// then redirect then to prevPrevPath w/o querystring
67-
const { pathname } = parse(req.prevPrevPath);
76+
const { pathname } = new Url(req.prevPrevPath, {});
6877
if (pathname === req.prevPrevPath) address = '/';
6978
else address = pathname;
7079
}
7180
}
7281

7382
redirect.call(res, status, address);
7483
};
84+
7585
next();
7686
};
7787
};

‎test/test.js

Lines changed: 62 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
1-
const test = require('ava');
1+
const Cabin = require('cabin');
22
const express = require('express');
33
const fetch = require('fetch-cookie/node-fetch')(require('node-fetch'));
44
const session = require('express-session');
5+
const test = require('ava');
56

67
const redirectLoop = require('..');
78

9+
const cabin = new Cabin();
10+
811
test.beforeEach(t => {
912
const app = express();
1013
app.use(
@@ -14,6 +17,7 @@ test.beforeEach(t => {
1417
saveUninitialized: true
1518
})
1619
);
20+
app.use(cabin.middleware);
1721
app.use(redirectLoop());
1822
app.get('/', (req, res) => res.sendStatus(200));
1923
app.get('/bar', (req, res) => res.redirect('/foo'));
@@ -24,47 +28,97 @@ test.beforeEach(t => {
2428
app.get('/1', (req, res) => res.redirect('/2'));
2529
app.get('/2', (req, res) => res.redirect('/3'));
2630
app.get('/3', (req, res) => res.redirect('/4'));
27-
app.get('/4', (req, res) => res.redirect('/4'));
31+
app.get('/4', (req, res) => res.redirect('/4')); // <-- should be 5
2832
app.get('/5', (req, res) => res.redirect('/6'));
2933
app.get('/6', (req, res) => res.redirect('/7'));
34+
app.get('/form', (req, res) => res.sendStatus(200));
35+
app.post('/form', (req, res) => res.redirect('/form'));
36+
app.use((err, req, res, next) => {
37+
console.log('err', err);
38+
next(err, req, res, next);
39+
});
3040
const server = app.listen();
3141
t.context.url = `http://localhost:${server.address().port}/`;
3242
});
3343

3444
test('caps at max of 5 redirects', async t => {
35-
const res = await fetch(`${t.context.url}1`, { credentials: 'include' });
36-
t.is(res.url, t.context.url);
45+
const res = await fetch(`${t.context.url}1`, {
46+
credentials: 'include'
47+
});
48+
console.log('res', res, 'res.body', res.body);
3749
t.is(res.status, 200);
50+
t.is(res.url, t.context.url);
3851
t.pass();
3952
});
4053

4154
test('/beep => 200 => /boop => /beep', async t => {
4255
let res = await fetch(`${t.context.url}beep`, { credentials: 'include' });
43-
t.is(res.url, `${t.context.url}beep`);
4456
t.is(res.status, 200);
45-
res = await fetch(`${t.context.url}boop`, { credentials: 'include' });
4657
t.is(res.url, `${t.context.url}beep`);
58+
res = await fetch(`${t.context.url}boop`, { credentials: 'include' });
4759
t.is(res.status, 200);
60+
t.is(res.url, `${t.context.url}beep`);
4861
t.pass();
4962
});
5063

5164
test('/bar => /foo => /', async t => {
5265
const res = await fetch(`${t.context.url}bar`, { credentials: 'include' });
53-
t.is(res.url, t.context.url);
5466
t.is(res.status, 200);
67+
t.is(res.url, t.context.url);
5568
t.pass();
5669
});
5770

5871
test('/foo => /', async t => {
5972
const res = await fetch(`${t.context.url}foo`, { credentials: 'include' });
60-
t.is(res.url, t.context.url);
6173
t.is(res.status, 200);
74+
t.is(res.url, t.context.url);
6275
t.pass();
6376
});
6477

6578
test('/baz => /bar => /foo => /', async t => {
6679
const res = await fetch(`${t.context.url}baz`, { credentials: 'include' });
80+
t.is(res.status, 200);
6781
t.is(res.url, t.context.url);
82+
t.pass();
83+
});
84+
85+
test('prevents incorrect redirect to earlier path', async t => {
86+
// GET / -> GET /form -> POST /form -> GET /form
87+
let res = await fetch(t.context.url, { credentials: 'include' });
6888
t.is(res.status, 200);
89+
t.is(res.url, t.context.url);
90+
res = await fetch(`${t.context.url}form`, { credentials: 'include' });
91+
t.is(res.status, 200);
92+
t.is(res.url, `${t.context.url}form`);
93+
res = await fetch(`${t.context.url}form`, {
94+
method: 'POST',
95+
credentials: 'include',
96+
redirect: 'manual'
97+
});
98+
t.is(res.status, 302);
99+
t.is(res.headers.get('location'), `${t.context.url}form`);
100+
101+
// GET /form -> POST /form -> GET /form -> POST /form
102+
res = await fetch(`${t.context.url}form`, { credentials: 'include' });
103+
t.is(res.status, 200);
104+
t.is(res.url, `${t.context.url}form`);
105+
res = await fetch(`${t.context.url}form`, {
106+
method: 'POST',
107+
credentials: 'include',
108+
redirect: 'manual'
109+
});
110+
t.is(res.status, 302);
111+
t.is(res.headers.get('location'), `${t.context.url}form`);
112+
res = await fetch(`${t.context.url}form`, { credentials: 'include' });
113+
t.is(res.status, 200);
114+
t.is(res.url, `${t.context.url}form`);
115+
res = await fetch(`${t.context.url}form`, {
116+
method: 'POST',
117+
credentials: 'include',
118+
redirect: 'manual'
119+
});
120+
t.is(res.status, 302);
121+
t.is(res.headers.get('location'), `${t.context.url}form`);
122+
69123
t.pass();
70124
});

0 commit comments

Comments
 (0)