Skip to content

Commit f8297cf

Browse files
fix(#11090): remove password from rate limiter lookup keys
The rate limiter included the raw password as a rate limit key. This caused all login attempts sharing the same password to share a single rate limit bucket, enabling cross-account lockout regardless of username or IP address. Rate limiting by IP and username is sufficient to prevent brute-force attacks against individual accounts.
1 parent 1447938 commit f8297cf

2 files changed

Lines changed: 7 additions & 20 deletions

File tree

api/src/services/rate-limit.js

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,16 +26,10 @@ const getKeys = (req) => {
2626
if (req.body?.user) {
2727
keys.push(req.body.user);
2828
}
29-
if (req.body?.password) {
30-
keys.push(req.body.password);
31-
}
3229
const basicAuth = auth.basicAuthCredentials(req);
3330
if (basicAuth?.username) {
3431
keys.push(basicAuth.username);
3532
}
36-
if (basicAuth?.password) {
37-
keys.push(basicAuth.password);
38-
}
3933
return keys;
4034
};
4135

api/tests/mocha/services/rate-limit.spec.js

Lines changed: 7 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -59,28 +59,24 @@ describe('rate-limit service', () => {
5959
auth.basicAuthCredentials.returns({ username: 'basicuser', password: 'basicpass' });
6060
rateLimit.get.withArgs('quay').resolves();
6161
rateLimit.get.withArgs('key').resolves({ remainingPoints: 1 });
62-
rateLimit.get.withArgs('kee').resolves({ remainingPoints: 1 });
6362
rateLimit.get.withArgs('basicuser').resolves({ remainingPoints: 1 });
64-
rateLimit.get.withArgs('basicpass').resolves({ remainingPoints: 1 });
6563
const actual = await service.isLimited(req);
6664
chai.expect(actual).to.be.false;
67-
chai.expect(rateLimit.get.callCount).to.equal(5);
65+
chai.expect(rateLimit.get.callCount).to.equal(3);
6866
});
6967

70-
it('returns false when any limit reached', async () => {
68+
it('returns true when any limit reached', async () => {
7169
const req = {
7270
ip: 'quay',
7371
body: { user: 'key', password: 'kee' }
7472
};
7573
auth.basicAuthCredentials.returns({ username: 'basicuser', password: 'basicpass' });
7674
rateLimit.get.withArgs('quay').resolves();
77-
rateLimit.get.withArgs('key').resolves({ remainingPoints: 1 });
78-
rateLimit.get.withArgs('kee').resolves({ remainingPoints: 0 });
75+
rateLimit.get.withArgs('key').resolves({ remainingPoints: 0 });
7976
rateLimit.get.withArgs('basicuser').resolves({ remainingPoints: 1 });
80-
rateLimit.get.withArgs('basicpass').resolves({ remainingPoints: 1 });
8177
const actual = await service.isLimited(req);
8278
chai.expect(actual).to.be.true;
83-
chai.expect(rateLimit.get.callCount).to.equal(3); // it short circuits when the first key is found to be limited
79+
chai.expect(rateLimit.get.callCount).to.equal(2);
8480
});
8581

8682
});
@@ -95,12 +91,10 @@ describe('rate-limit service', () => {
9591
auth.basicAuthCredentials.returns({ username: 'basicuser', password: 'basicpass' });
9692
rateLimit.consume.resolves();
9793
await service.consume(req);
98-
chai.expect(rateLimit.consume.callCount).to.equal(5);
94+
chai.expect(rateLimit.consume.callCount).to.equal(3);
9995
chai.expect(rateLimit.consume.args[0][0]).to.equal('quay');
10096
chai.expect(rateLimit.consume.args[1][0]).to.equal('key');
101-
chai.expect(rateLimit.consume.args[2][0]).to.equal('kee');
102-
chai.expect(rateLimit.consume.args[3][0]).to.equal('basicuser');
103-
chai.expect(rateLimit.consume.args[4][0]).to.equal('basicpass');
97+
chai.expect(rateLimit.consume.args[2][0]).to.equal('basicuser');
10498
});
10599

106100
it('ignores rejections', async () => {
@@ -110,9 +104,8 @@ describe('rate-limit service', () => {
110104
};
111105
rateLimit.consume.withArgs('quay').rejects(); // rate limit exceeded
112106
rateLimit.consume.withArgs('key').resolves();
113-
rateLimit.consume.withArgs('kee').resolves();
114107
await service.consume(req);
115-
chai.expect(rateLimit.consume.callCount).to.equal(3);
108+
chai.expect(rateLimit.consume.callCount).to.equal(2);
116109
});
117110

118111
});

0 commit comments

Comments
 (0)