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.

Sign up for GitHub

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

Add schema checks for addresses in url endpoints #2692

Merged
merged 4 commits into from
Jan 17, 2025
Merged

Add schema checks for addresses in url endpoints #2692

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1099fdf
Add bech32 schema checks for addresses in url endpoints
shrenujb Jan 16, 2025
d49bf25
change schema check to only character limit and alphanumeric
shrenujb Jan 16, 2025
e92cd6e
fix some tests
shrenujb Jan 17, 2025
0b5c03d
fix another test
shrenujb Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion indexer/packages/postgres/__tests__/helpers/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export const blockedAddress: string = 'dydx1f9k5qldwmqrnwy8hcgp4fw6heuvszt35egvt
export const vaultAddress: string = 'dydx1c0m5x87llaunl5sgv3q5vd7j5uha26d2q2r2q0';

// ============== Subaccounts ==============
export const defaultWalletAddress: string = 'defaultWalletAddress';
export const defaultWalletAddress: string = 'dydx199tqg4wdlnu4qjlxchpd7seg454937hjrknju4';

export const defaultSubaccount: SubaccountCreateObject = {
address: defaultAddress,
Expand Down
2 changes: 2 additions & 0 deletions indexer/pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 6 additions & 14 deletions indexer/services/comlink/__tests__/controllers/api/v4/addresses-controller.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { Secp256k1 } from '@cosmjs/crypto';
import { toBech32 } from '@cosmjs/encoding';
import { DateTime } from 'luxon';
import { verifyADR36Amino } from '@keplr-wallet/cosmos';
import { defaultAddress3 } from '@dydxprotocol-indexer/postgres/build/__tests__/helpers/constants';

jest.mock('@cosmjs/crypto', () => ({
...jest.requireActual('@cosmjs/crypto'),
Expand Down Expand Up @@ -211,15 +212,15 @@ describe('addresses-controller#V4', () => {
it('Get / with non-existent address and subaccount number returns 404', async () => {
const response: request.Response = await sendRequest({
type: RequestMethod.GET,
path: `/v4/addresses/${invalidAddress}/subaccountNumber/` +
path: `/v4/addresses/${defaultAddress3}/subaccountNumber/` +
`${testConstants.defaultSubaccount.subaccountNumber}`,
expectedStatus: 404,
});

expect(response.body).toEqual({
errors: [
{
msg: `No subaccount found with address ${invalidAddress} and ` +
msg: `No subaccount found with address ${defaultAddress3} and ` +
`subaccountNumber ${testConstants.defaultSubaccount.subaccountNumber}`,
},
],
Expand Down Expand Up @@ -411,15 +412,10 @@ describe('addresses-controller#V4', () => {
expect(response.body).toEqual({
errors: [
{
msg: `No subaccounts found for address ${invalidAddress}`,
msg: 'No subaccounts found for address invalidAddress',
},
],
});
expect(stats.increment).toHaveBeenCalledWith('comlink.addresses-controller.response_status_code.404', 1,
{
path: '/:address',
method: 'GET',
});
});
});

Expand Down Expand Up @@ -561,15 +557,15 @@ describe('addresses-controller#V4', () => {
it('Get /:address/parentSubaccountNumber/ with non-existent address returns 404', async () => {
const response: request.Response = await sendRequest({
type: RequestMethod.GET,
path: `/v4/addresses/${invalidAddress}/parentSubaccountNumber/` +
path: `/v4/addresses/${defaultAddress3}/parentSubaccountNumber/` +
`${testConstants.defaultSubaccount.subaccountNumber}`,
expectedStatus: 404,
});

expect(response.body).toEqual({
errors: [
{
msg: `No subaccounts found for address ${invalidAddress} and ` +
msg: `No subaccounts found for address ${defaultAddress3} and ` +
`parentSubaccountNumber ${testConstants.defaultSubaccount.subaccountNumber}`,
},
],
Expand Down Expand Up @@ -741,10 +737,6 @@ describe('addresses-controller#V4', () => {
},
],
});
expect(statsSpy).toHaveBeenCalledWith('comlink.addresses-controller.response_status_code.400', 1, {
path: '/:address/registerToken',
method: 'POST',
});
});

it.each([
Expand Down
4 changes: 2 additions & 2 deletions indexer/services/comlink/__tests__/controllers/api/v4/historical-pnl-controller.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,14 +264,14 @@ describe('pnlTicks-controller#V4', () => {
it('Get /historical-pnl with non-existent address and subaccount number returns 404', async () => {
const response: request.Response = await sendRequest({
type: RequestMethod.GET,
path: '/v4/historical-pnl?address=invalid_address&subaccountNumber=100',
path: '/v4/historical-pnl?address=invalidaddress&subaccountNumber=100',
expectedStatus: 404,
});

expect(response.body).toEqual({
errors: [
{
msg: 'No subaccount found with address invalid_address and subaccountNumber 100',
msg: 'No subaccount found with address invalidaddress and subaccountNumber 100',
},
],
});
Expand Down
8 changes: 4 additions & 4 deletions indexer/services/comlink/__tests__/controllers/api/v4/transfers-controller.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,14 +433,14 @@ describe('transfers-controller#V4', () => {
it('Get /transfers with non-existent address and subaccount number returns 404', async () => {
const response: request.Response = await sendRequest({
type: RequestMethod.GET,
path: '/v4/transfers?address=invalid_address&subaccountNumber=100',
path: '/v4/transfers?address=invalidaddress&subaccountNumber=100',
expectedStatus: 404,
});

expect(response.body).toEqual({
errors: [
{
msg: 'No subaccount found with address invalid_address and subaccountNumber 100',
msg: 'No subaccount found with address invalidaddress and subaccountNumber 100',
},
],
});
Expand Down Expand Up @@ -981,14 +981,14 @@ describe('transfers-controller#V4', () => {
it('Get /transfers/parentSubaccountNumber with non-existent address and subaccount number returns 404', async () => {
const response: request.Response = await sendRequest({
type: RequestMethod.GET,
path: '/v4/transfers/parentSubaccountNumber?address=invalid_address&parentSubaccountNumber=100',
path: '/v4/transfers/parentSubaccountNumber?address=invalidaddress&parentSubaccountNumber=100',
expectedStatus: 404,
});

expect(response.body).toEqual({
errors: [
{
msg: 'No subaccount found with address invalid_address and parentSubaccountNumber 100',
msg: 'No subaccount found with address invalidaddress and parentSubaccountNumber 100',
},
],
});
Expand Down
7 changes: 6 additions & 1 deletion indexer/services/comlink/__tests__/lib/validation/schemas.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,12 @@ describe('schemas', () => {
const defaultAddress: string = testConstants.defaultSubaccount.address;
describe('CheckSubaccountSchema', () => {
it.each([
['missing address', { subaccountNumber: defaultSubaccountNumber }, 'address', 'Invalid value'],
[
'missingaddress',
{ subaccountNumber: defaultSubaccountNumber },
'address',
'address must be a valid dydx address',
],
Comment on lines +20 to +25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance test coverage for address validation.

The test's error message suggests bech32 validation ("must be a valid dydx address"), but it only tests for a missing address. Additional test cases are needed to verify bech32 validation once the schemas are updated to use isValidDydxAddress.

Add these test cases:

[
  'invalid bech32 format',
  { address: 'invalid123', subaccountNumber: defaultSubaccountNumber },
  'address',
  'address must be a valid dydx address',
],
[
  'wrong prefix',
  { address: 'cosmos1abc...', subaccountNumber: defaultSubaccountNumber },
  'address',
  'address must be a valid dydx address',
],
[
  'valid dydx address',
  { address: 'dydx1...', subaccountNumber: defaultSubaccountNumber },
  undefined,
  undefined,
],

[
'missing subaccountNumber',
{ address: defaultAddress },
Expand Down
1 change: 1 addition & 0 deletions indexer/services/comlink/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"@dydxprotocol-indexer/v4-protos": "workspace:^0.0.1",
"@keplr-wallet/cosmos": "^0.12.122",
"@tsoa/runtime": "^5.0.0",
"bech32": "1.1.4",
"big.js": "^6.2.1",
"binary-searching": "^2.0.5",
"body-parser": "^1.20.0",
Expand Down
33 changes: 33 additions & 0 deletions indexer/services/comlink/src/lib/validation/schemas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
MAX_PARENT_SUBACCOUNTS,
CHILD_SUBACCOUNT_MULTIPLIER,
} from '@dydxprotocol-indexer/postgres';
import { decode } from 'bech32';
import { body, checkSchema, ParamSchema } from 'express-validator';

import config from '../../config';
Expand All @@ -12,6 +13,10 @@ export const CheckSubaccountSchema = checkSchema({
address: {
in: ['params', 'query'],
isString: true,
custom: {
options: isValidAddress,
},
errorMessage: 'address must be a valid dydx address',
Comment on lines +16 to +19
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Inconsistency between error message and validation logic.

The error message states "address must be a valid dydx address", but isValidAddress only checks for alphanumeric characters and length ≤ 90. Consider either:

  1. Using isValidDydxAddress instead of isValidAddress to match the error message, or
  2. Updating the error message to accurately reflect the current validation criteria

Example fix:

  address: {
    in: ['params', 'query'],
    isString: true,
    custom: {
-     options: isValidAddress,
+     options: isValidDydxAddress,
    },
    errorMessage: 'address must be a valid dydx address',
  },

Or:

  address: {
    in: ['params', 'query'],
    isString: true,
    custom: {
      options: isValidAddress,
    },
-   errorMessage: 'address must be a valid dydx address',
+   errorMessage: 'address must be alphanumeric and not exceed 90 characters',
  },

Also applies to: 34-37, 52-55

💡 Codebase verification

Review comment is correct - schemas should use stricter validation

The codebase confirms that while isValidDydxAddress is properly implemented to enforce bech32 validation (requiring 'dydx1' prefix and valid bech32 format), the schemas are still using the more lenient isValidAddress function that only checks for length and alphanumeric characters.

  • The validation is used across multiple controllers including:
    • addresses-controller.ts
    • transfers-controller.ts
    • orders-controller.ts
    • fills-controller.ts
    • perpetual-positions-controller.ts

Switching to isValidDydxAddress would properly enforce the PR's objective of adding bech32 schema checks.

🔗 Analysis chain

Align implementation with PR objectives.

The PR's objective is to add bech32 schema checks for addresses, but the current implementation uses a more lenient validation (isValidAddress) in the schemas instead of the bech32-specific validation (isValidDydxAddress). This doesn't fully achieve the PR's goal of ensuring addresses are valid dydx addresses.

Consider using isValidDydxAddress in all schemas to enforce proper bech32 validation as intended by the PR.

Let's verify the impact of this change:

Also applies to: 34-37, 52-55, 289-292

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Find all files that might be affected by this change
echo "Files potentially using these schemas:"
rg -l "CheckSubaccountSchema|CheckParentSubaccountSchema|CheckAddressSchema" --type ts

echo -e "\nEndpoints using these schemas:"
rg -A 2 "\.use\((Check(Subaccount|ParentSubaccount|Address)Schema)" --type ts

Length of output: 1108

Script:

#!/bin/bash
# Find implementation of validation functions
echo "=== Validation function implementations ==="
ast-grep --pattern 'function isValidAddress' 
ast-grep --pattern 'function isValidDydxAddress'

echo -e "\n=== Usage in controllers ==="
# Look for schema usage in controllers with context
rg "CheckSubaccountSchema|CheckParentSubaccountSchema|CheckAddressSchema" "indexer/services/comlink/src/controllers/api/v4/*.ts" -B 1 -A 1

echo -e "\n=== Test coverage ==="
# Check test coverage for these validations
rg "isValidAddress|isValidDydxAddress" "indexer/services/comlink/__tests__/" -A 2

Length of output: 642

Script:

#!/bin/bash
# Find validation function implementations with broader patterns
echo "=== Validation implementations ==="
rg "isValidAddress|isValidDydxAddress" "indexer/services/comlink/src/" -A 2

echo -e "\n=== Exports and imports ==="
rg "export.*isValid|import.*isValid" "indexer/services/comlink/src/" -A 1

echo -e "\n=== Schema usage in controllers ==="
# Fixed controller path
rg "CheckSubaccountSchema|CheckParentSubaccountSchema|CheckAddressSchema" "indexer/services/comlink/src/controllers/" -B 1 -A 1

echo -e "\n=== Schema definitions ==="
rg "CheckSubaccountSchema|CheckParentSubaccountSchema|CheckAddressSchema" "indexer/services/comlink/src/lib/validation/" -B 1 -A 3

Length of output: 13609

},
subaccountNumber: {
in: ['params', 'query'],
Expand All @@ -26,6 +31,10 @@ export const CheckParentSubaccountSchema = checkSchema({
address: {
in: ['params', 'query'],
isString: true,
custom: {
options: isValidAddress,
},
errorMessage: 'address must be a valid dydx address',
},
parentSubaccountNumber: {
in: ['params', 'query'],
Expand All @@ -40,6 +49,10 @@ export const checkAddressSchemaRecord: Record<string, ParamSchema> = {
address: {
in: ['params'],
isString: true,
custom: {
options: isValidAddress,
},
errorMessage: 'address must be a valid address',
},
};

Expand Down Expand Up @@ -262,3 +275,23 @@ export const RegisterTokenValidationSchema = [
return true;
}),
];

function verifyIsBech32(address: string): Error | undefined {
try {
decode(address);
} catch (error) {
return error;
}

return undefined;
}
Comment on lines +279 to +287
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add input validation to verifyIsBech32.

The function should validate its input before attempting to decode.

 function verifyIsBech32(address: string): Error | undefined {
+  if (!address || typeof address !== 'string') {
+    return new Error('Invalid input: address must be a non-empty string');
+  }
   try {
     decode(address);
   } catch (error) {
-    return error;
+    return error instanceof Error ? error : new Error('Invalid bech32 format');
   }
   return undefined;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function verifyIsBech32(address: string): Error | undefined {
try {
decode(address);
} catch (error) {
return error;
}
return undefined;
}
function verifyIsBech32(address: string): Error | undefined {
if (!address || typeof address !== 'string') {
return new Error('Invalid input: address must be a non-empty string');
}
try {
decode(address);
} catch (error) {
return error instanceof Error ? error : new Error('Invalid bech32 format');
}
return undefined;
}


export function isValidDydxAddress(address: string): boolean {
// An address is valid if it starts with `dydx1` and is Bech32 format.
return address.startsWith('dydx1') && (verifyIsBech32(address) === undefined);
}
Comment on lines +289 to +292
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add input validation to isValidDydxAddress.

The function should validate its input and handle edge cases.

 export function isValidDydxAddress(address: string): boolean {
+  if (!address || typeof address !== 'string') {
+    return false;
+  }
   // An address is valid if it starts with `dydx1` and is Bech32 format.
   return address.startsWith('dydx1') && (verifyIsBech32(address) === undefined);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export function isValidDydxAddress(address: string): boolean {
// An address is valid if it starts with `dydx1` and is Bech32 format.
return address.startsWith('dydx1') && (verifyIsBech32(address) === undefined);
}
export function isValidDydxAddress(address: string): boolean {
if (!address || typeof address !== 'string') {
return false;
}
// An address is valid if it starts with `dydx1` and is Bech32 format.
return address.startsWith('dydx1') && (verifyIsBech32(address) === undefined);
}


export function isValidAddress(address: string): boolean {
// Address is valid if its under 90 characters and alphanumeric
return address.length <= 90 && /^[a-zA-Z0-9]*$/.test(address);
}
Loading