From ee039ca59640dceeed7609878334518378a552ee Mon Sep 17 00:00:00 2001 From: cornzzy <116248852+cornzzy@users.noreply.github.com> Date: Wed, 24 Jul 2024 22:51:29 +0330 Subject: [PATCH] fix(server): block access when traffic limit is set to 0 bytes * Fix isOverDataLimit logic to stop allowing 0 traffic byte issue #1515 This if statement is just wrong and doesn't block access when traffic limit is set to 0 byte. This one character fixes it. Now if we create a new access key, set the limit to 0 before first usage, the client no longer will be able to connect. * Rename `isOverDataLimit` to `reachedDataLimit`. * Add a test case to prevent regression. * Rename old variable. * Update comment now that variable has changed meaning slightly. Co-authored-by: Vinicius Fortuna --------- Co-authored-by: sbruens Co-authored-by: Vinicius Fortuna --- src/shadowbox/model/access_key.ts | 4 +- .../server/server_access_key.spec.ts | 89 +++++++++++-------- src/shadowbox/server/server_access_key.ts | 10 +-- 3 files changed, 59 insertions(+), 44 deletions(-) diff --git a/src/shadowbox/model/access_key.ts b/src/shadowbox/model/access_key.ts index 88f6e58e..5ed57b18 100644 --- a/src/shadowbox/model/access_key.ts +++ b/src/shadowbox/model/access_key.ts @@ -39,8 +39,8 @@ export interface AccessKey { readonly name: string; // Parameters to access the proxy readonly proxyParams: ProxyParams; - // Whether the access key has exceeded the data transfer limit. - readonly isOverDataLimit: boolean; + // Whether the access key has reached the data transfer limit. + readonly reachedDataLimit: boolean; // The key's current data limit. If it exists, it overrides the server default data limit. readonly dataLimit?: DataLimit; } diff --git a/src/shadowbox/server/server_access_key.spec.ts b/src/shadowbox/server/server_access_key.spec.ts index 65f889db..d950ece6 100644 --- a/src/shadowbox/server/server_access_key.spec.ts +++ b/src/shadowbox/server/server_access_key.spec.ts @@ -183,7 +183,7 @@ describe('ServerAccessKeyRepository', () => { it('Creates access keys under limit', async (done) => { const repo = new RepoBuilder().build(); const accessKey = await repo.createNewAccessKey(); - expect(accessKey.isOverDataLimit).toBeFalsy(); + expect(accessKey.reachedDataLimit).toBeFalsy(); done(); }); @@ -346,13 +346,13 @@ describe('ServerAccessKeyRepository', () => { const key = await repo.createNewAccessKey(); await setKeyLimitAndEnforce(repo, key.id, {bytes: 0}); - expect(key.isOverDataLimit).toBeTruthy(); + expect(key.reachedDataLimit).toBeTruthy(); let serverKeys = server.getAccessKeys(); expect(serverKeys.length).toEqual(0); await setKeyLimitAndEnforce(repo, key.id, {bytes: 1000}); - expect(key.isOverDataLimit).toBeFalsy(); + expect(key.reachedDataLimit).toBeFalsy(); serverKeys = server.getAccessKeys(); expect(serverKeys.length).toEqual(1); expect(serverKeys[0].id).toEqual(key.id); @@ -371,13 +371,13 @@ describe('ServerAccessKeyRepository', () => { const higherLimitThanDefault = await repo.createNewAccessKey(); await repo.setDefaultDataLimit({bytes: 1000}); - expect(lowerLimitThanDefault.isOverDataLimit).toBeFalsy(); + expect(lowerLimitThanDefault.reachedDataLimit).toBeFalsy(); await setKeyLimitAndEnforce(repo, lowerLimitThanDefault.id, {bytes: 500}); - expect(lowerLimitThanDefault.isOverDataLimit).toBeTruthy(); + expect(lowerLimitThanDefault.reachedDataLimit).toBeTruthy(); - expect(higherLimitThanDefault.isOverDataLimit).toBeTruthy(); + expect(higherLimitThanDefault.reachedDataLimit).toBeTruthy(); await setKeyLimitAndEnforce(repo, higherLimitThanDefault.id, {bytes: 1500}); - expect(higherLimitThanDefault.isOverDataLimit).toBeFalsy(); + expect(higherLimitThanDefault.reachedDataLimit).toBeFalsy(); done(); }); @@ -404,10 +404,10 @@ describe('ServerAccessKeyRepository', () => { await repo.start(new ManualClock()); await repo.setDefaultDataLimit({bytes: 0}); await setKeyLimitAndEnforce(repo, key.id, {bytes: 1000}); - expect(key.isOverDataLimit).toBeFalsy(); + expect(key.reachedDataLimit).toBeFalsy(); await removeKeyLimitAndEnforce(repo, key.id); - expect(key.isOverDataLimit).toBeTruthy(); + expect(key.reachedDataLimit).toBeTruthy(); done(); }); @@ -422,13 +422,13 @@ describe('ServerAccessKeyRepository', () => { const key = await repo.createNewAccessKey(); await setKeyLimitAndEnforce(repo, key.id, {bytes: 0}); - expect(key.isOverDataLimit).toBeTruthy(); + expect(key.reachedDataLimit).toBeTruthy(); let serverKeys = server.getAccessKeys(); expect(serverKeys.length).toEqual(0); await setKeyLimitAndEnforce(repo, key.id, {bytes: 1000}); - expect(key.isOverDataLimit).toBeFalsy(); + expect(key.reachedDataLimit).toBeFalsy(); serverKeys = server.getAccessKeys(); expect(serverKeys.length).toEqual(1); expect(serverKeys[0].id).toEqual(key.id); @@ -447,13 +447,13 @@ describe('ServerAccessKeyRepository', () => { const higherLimitThanDefault = await repo.createNewAccessKey(); await repo.setDefaultDataLimit({bytes: 1000}); - expect(lowerLimitThanDefault.isOverDataLimit).toBeFalsy(); + expect(lowerLimitThanDefault.reachedDataLimit).toBeFalsy(); await setKeyLimitAndEnforce(repo, lowerLimitThanDefault.id, {bytes: 500}); - expect(lowerLimitThanDefault.isOverDataLimit).toBeTruthy(); + expect(lowerLimitThanDefault.reachedDataLimit).toBeTruthy(); - expect(higherLimitThanDefault.isOverDataLimit).toBeTruthy(); + expect(higherLimitThanDefault.reachedDataLimit).toBeTruthy(); await setKeyLimitAndEnforce(repo, higherLimitThanDefault.id, {bytes: 1500}); - expect(higherLimitThanDefault.isOverDataLimit).toBeFalsy(); + expect(higherLimitThanDefault.reachedDataLimit).toBeFalsy(); done(); }); @@ -487,10 +487,10 @@ describe('ServerAccessKeyRepository', () => { await repo.start(new ManualClock()); await repo.setDefaultDataLimit({bytes: 0}); await setKeyLimitAndEnforce(repo, key.id, {bytes: 1000}); - expect(key.isOverDataLimit).toBeFalsy(); + expect(key.reachedDataLimit).toBeFalsy(); await removeKeyLimitAndEnforce(repo, key.id); - expect(key.isOverDataLimit).toBeTruthy(); + expect(key.reachedDataLimit).toBeTruthy(); done(); }); @@ -505,11 +505,11 @@ describe('ServerAccessKeyRepository', () => { await repo.start(new ManualClock()); await setKeyLimitAndEnforce(repo, key.id, {bytes: 0}); - expect(key.isOverDataLimit).toBeTruthy(); + expect(key.reachedDataLimit).toBeTruthy(); expect(server.getAccessKeys().length).toEqual(0); await removeKeyLimitAndEnforce(repo, key.id); - expect(key.isOverDataLimit).toBeFalsy(); + expect(key.reachedDataLimit).toBeFalsy(); expect(server.getAccessKeys().length).toEqual(1); done(); }); @@ -537,8 +537,8 @@ describe('ServerAccessKeyRepository', () => { // We enforce asynchronously, in setAccessKeyDataLimit, so explicitly call it here to make sure // enforcement is done before we make assertions. await repo.enforceAccessKeyDataLimits(); - expect(accessKey1.isOverDataLimit).toBeTruthy(); - expect(accessKey2.isOverDataLimit).toBeFalsy(); + expect(accessKey1.reachedDataLimit).toBeTruthy(); + expect(accessKey2.reachedDataLimit).toBeFalsy(); // We determine which access keys have been enabled/disabled by accessing them from // the server's perspective, ensuring `server.update` has been called. let serverAccessKeys = server.getAccessKeys(); @@ -549,8 +549,8 @@ describe('ServerAccessKeyRepository', () => { prometheusClient.bytesTransferredById = {'0': 500, '1': 1000}; repo.setDefaultDataLimit({bytes: 700}); await repo.enforceAccessKeyDataLimits(); - expect(accessKey1.isOverDataLimit).toBeFalsy(); - expect(accessKey2.isOverDataLimit).toBeTruthy(); + expect(accessKey1.reachedDataLimit).toBeFalsy(); + expect(accessKey2.reachedDataLimit).toBeTruthy(); serverAccessKeys = server.getAccessKeys(); expect(serverAccessKeys.length).toEqual(1); expect(serverAccessKeys[0].id).toEqual(accessKey1.id); @@ -586,8 +586,8 @@ describe('ServerAccessKeyRepository', () => { // enforcement is done before we make assertions. await repo.enforceAccessKeyDataLimits(); expect(server.getAccessKeys().length).toEqual(2); - expect(accessKey1.isOverDataLimit).toBeFalsy(); - expect(accessKey2.isOverDataLimit).toBeFalsy(); + expect(accessKey1.reachedDataLimit).toBeFalsy(); + expect(accessKey2.reachedDataLimit).toBeFalsy(); done(); }); @@ -609,7 +609,7 @@ describe('ServerAccessKeyRepository', () => { } await repo.enforceAccessKeyDataLimits(); for (const key of repo.listAccessKeys()) { - expect(key.isOverDataLimit).toEqual( + expect(key.reachedDataLimit).toEqual( prometheusClient.bytesTransferredById[key.id] > limit.bytes ); } @@ -618,7 +618,7 @@ describe('ServerAccessKeyRepository', () => { await repo.enforceAccessKeyDataLimits(); for (const key of repo.listAccessKeys()) { - expect(key.isOverDataLimit).toEqual( + expect(key.reachedDataLimit).toEqual( prometheusClient.bytesTransferredById[key.id] > limit.bytes ); } @@ -636,14 +636,14 @@ describe('ServerAccessKeyRepository', () => { await setKeyLimitAndEnforce(repo, perKeyLimited.id, {bytes: 100}); await repo.enforceAccessKeyDataLimits(); - expect(perKeyLimited.isOverDataLimit).toBeTruthy(); - expect(defaultLimited.isOverDataLimit).toBeFalsy(); + expect(perKeyLimited.reachedDataLimit).toBeTruthy(); + expect(defaultLimited.reachedDataLimit).toBeFalsy(); prometheusClient.bytesTransferredById[perKeyLimited.id] = 50; prometheusClient.bytesTransferredById[defaultLimited.id] = 600; await repo.enforceAccessKeyDataLimits(); - expect(perKeyLimited.isOverDataLimit).toBeFalsy(); - expect(defaultLimited.isOverDataLimit).toBeTruthy(); + expect(perKeyLimited.reachedDataLimit).toBeFalsy(); + expect(defaultLimited.reachedDataLimit).toBeTruthy(); done(); }); @@ -673,6 +673,21 @@ describe('ServerAccessKeyRepository', () => { done(); }); + it('enforceAccessKeyDataLimits disables on exact data limit', async (done) => { + const server = new FakeShadowsocksServer(); + const prometheusClient = new FakePrometheusClient({'0': 0}); + const repo = new RepoBuilder() + .prometheusClient(prometheusClient) + .shadowsocksServer(server) + .build(); + await repo.createNewAccessKey({dataLimit: {bytes: 0}}); + + await repo.enforceAccessKeyDataLimits(); + + expect(server.getAccessKeys().length).toEqual(0); + done(); + }); + it('Repos created with an existing file restore access keys', async (done) => { const config = new InMemoryConfig({accessKeys: [], nextId: 0}); const repo1 = new RepoBuilder().keyConfig(config).build(); @@ -741,9 +756,9 @@ describe('ServerAccessKeyRepository', () => { await repo.start(clock); await clock.runCallbacks(); - expect(accessKey1.isOverDataLimit).toBeTruthy(); - expect(accessKey2.isOverDataLimit).toBeFalsy(); - expect(accessKey3.isOverDataLimit).toBeTruthy(); + expect(accessKey1.reachedDataLimit).toBeTruthy(); + expect(accessKey2.reachedDataLimit).toBeFalsy(); + expect(accessKey3.reachedDataLimit).toBeTruthy(); let serverAccessKeys = await server.getAccessKeys(); expect(serverAccessKeys.length).toEqual(1); expect(serverAccessKeys[0].id).toEqual(accessKey2.id); @@ -751,9 +766,9 @@ describe('ServerAccessKeyRepository', () => { // Simulate a change in usage. prometheusClient.bytesTransferredById = {'0': 100, '1': 200, '2': 1000}; await clock.runCallbacks(); - expect(accessKey1.isOverDataLimit).toBeFalsy(); - expect(accessKey2.isOverDataLimit).toBeFalsy(); - expect(accessKey3.isOverDataLimit).toBeTruthy(); + expect(accessKey1.reachedDataLimit).toBeFalsy(); + expect(accessKey2.reachedDataLimit).toBeFalsy(); + expect(accessKey3.reachedDataLimit).toBeTruthy(); serverAccessKeys = await server.getAccessKeys(); expect(serverAccessKeys.length).toEqual(2); expect(serverAccessKeys[0].id).toEqual(accessKey1.id); diff --git a/src/shadowbox/server/server_access_key.ts b/src/shadowbox/server/server_access_key.ts index a5243070..2c522498 100644 --- a/src/shadowbox/server/server_access_key.ts +++ b/src/shadowbox/server/server_access_key.ts @@ -50,7 +50,7 @@ export interface AccessKeyConfigJson { // AccessKey implementation with write access enabled on properties that may change. class ServerAccessKey implements AccessKey { - isOverDataLimit = false; + reachedDataLimit = false; constructor( readonly id: AccessKeyId, public name: string, @@ -308,13 +308,13 @@ export class ServerAccessKeyRepository implements AccessKeyRepository { let limitStatusChanged = false; for (const accessKey of this.accessKeys) { const usageBytes = bytesTransferredById[accessKey.id] ?? 0; - const wasOverDataLimit = accessKey.isOverDataLimit; + const oldReachedDataLimit = accessKey.reachedDataLimit; let limitBytes = (accessKey.dataLimit ?? this._defaultDataLimit)?.bytes; if (limitBytes === undefined) { limitBytes = Number.POSITIVE_INFINITY; } - accessKey.isOverDataLimit = usageBytes > limitBytes; - limitStatusChanged = accessKey.isOverDataLimit !== wasOverDataLimit || limitStatusChanged; + accessKey.reachedDataLimit = usageBytes >= limitBytes; + limitStatusChanged = accessKey.reachedDataLimit !== oldReachedDataLimit || limitStatusChanged; } if (limitStatusChanged) { await this.updateServer(); @@ -323,7 +323,7 @@ export class ServerAccessKeyRepository implements AccessKeyRepository { private updateServer(): Promise { const serverAccessKeys = this.accessKeys - .filter((key) => !key.isOverDataLimit) + .filter((key) => !key.reachedDataLimit) .map((key) => { return { id: key.id,