Repository: solana-labs/solana-program-library (rust)

Issue:
token-js: tlv parser does not handle extra space in the buffer well
allocate a token account with six extra bytes and try to find a post hoc initializable boolean data extension, which takes up five bytes in the repr. parser will attempt to read a u16 past the end of the buffer
 
```
HANA BEGIN
     cpi guard type/len: 11/1, memo transfer type/len: 8/1
     buffer: <Buffer 00 00 00 00 00>

HANA idx: 0, tlv len: 5
     entry type: 0 ==? 11
     entry len: 0
     type idx: 4
  -> newly computed: 4

HANA idx: 4, tlv len: 5

     RangeError [ERR_OUT_OF_RANGE]: The value of "offset" is out of range. It must be >= 0 and <= 3. Received 4
      at new NodeError (node:internal/errors:371:5)
      at boundsError (node:internal/buffer:86:9)
      at Buffer.readUInt16LE (node:internal/buffer:243:5)
      at getExtensionData (file:///home/hana/work/solana/solana-program-library/token/js/src/extensions/extensionType.ts:127:35)
      at Context.<anonymous> (file:///home/hana/work/solana/solana-program-library/token/js/test/e2e-2022/tlv.test.ts:115:24)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
```

fixing this one thing isnt hard but instead of just whacking my mole im writing proptests for the parser

edit: actually i wrote that very confidently but its weird to me that i tell it to alloc six more and i see five here. im not sure how it finds the start or end of the tlv buffer to begin with, writing this will solve all my questions tho

Pull request:
token-js: sanity check header space in tlv data
closes #3912

Patch:
diff --git a/token/cli/src/main.rs b/token/cli/src/main.rs
index 7a89eeb9b..1e20a59e4 100644
--- a/token/cli/src/main.rs
+++ b/token/cli/src/main.rs
@@ -1763,11 +1763,23 @@ async fn command_address(
 async fn command_display(config: &Config<'_>, address: Pubkey) -> CommandResult {
     let account_data = config.get_account_checked(&address).await?;
 
-    let decimals = if let Some(mint_address) = get_token_account_mint(&account_data.data) {
-        Some(config.get_mint_info(&mint_address, None).await?.decimals)
-    } else {
-        None
-    };
+    let (decimals, has_permanent_delegate) =
+        if let Some(mint_address) = get_token_account_mint(&account_data.data) {
+            let mint_account = config.get_account_checked(&mint_address).await?;
+            let mint_state = StateWithExtensionsOwned::<Mint>::unpack(mint_account.data)
+                .map_err(|_| format!("Could not deserialize token mint {}", mint_address))?;
+
+            let has_permanent_delegate =
+                if let Ok(permanent_delegate) = mint_state.get_extension::<PermanentDelegate>() {
+                    Option::<Pubkey>::from(permanent_delegate.delegate).is_some()
+                } else {
+                    false
+                };
+
+            (Some(mint_state.base.decimals), has_permanent_delegate)
+        } else {
+            (None, false)
+        };
 
     let token_data = parse_token(&account_data.data, decimals);
 
@@ -1786,6 +1798,7 @@ async fn command_display(config: &Config<'_>, address: Pubkey) -> CommandResult
                 program_id: config.program_id.to_string(),
                 is_associated: associated_address == address,
                 account,
+                has_permanent_delegate,
             };
 
             Ok(config.output_format.formatted_string(&cli_output))
diff --git a/token/cli/src/output.rs b/token/cli/src/output.rs
index 4fc8d0968..ec194deda 100644
--- a/token/cli/src/output.rs
+++ b/token/cli/src/output.rs
@@ -4,8 +4,8 @@ use serde::{Deserialize, Serialize, Serializer};
 use solana_account_decoder::{
     parse_token::{UiAccountState, UiMint, UiMultisig, UiTokenAccount, UiTokenAmount},
     parse_token_extension::{
-        UiDefaultAccountState, UiExtension, UiInterestBearingConfig, UiMemoTransfer,
-        UiMintCloseAuthority, UiTransferFeeAmount, UiTransferFeeConfig,
+        UiCpiGuard, UiDefaultAccountState, UiExtension, UiInterestBearingConfig, UiMemoTransfer,
+        UiMintCloseAuthority, UiPermanentDelegate, UiTransferFeeAmount, UiTransferFeeConfig,
     },
 };
 use solana_cli_output::{display::writeln_name_value, OutputFormat, QuietDisplay, VerboseDisplay};
@@ -191,6 +191,8 @@ pub(crate) struct CliTokenAccount {
     pub(crate) is_associated: bool,
     #[serde(flatten)]
     pub(crate) account: UiTokenAccount,
+    #[serde(skip_serializing)]
+    pub(crate) has_permanent_delegate: bool,
 }
 
 impl QuietDisplay for CliTokenAccount {}
@@ -257,6 +259,22 @@ impl fmt::Display for CliTokenAccount {
             writeln!(f, "* Please run `spl-token gc` to clean up Aux accounts")?;
         }
 
+        if self.has_permanent_delegate {
+            writeln!(f)?;
+            writeln!(
+                f,
+                "* {} ",
+                style("This token has a permanent delegate!").bold()
+            )?;
+            writeln!(
+                f,
+                "  This means the mint may withdraw {} funds from this account at {} time.",
+                style("all").bold(),
+                style("any").bold(),
+            )?;
+            writeln!(f, "  If this was not adequately disclosed to you, you may be dealing with a malicious mint.")?;
+        }
+
         Ok(())
     }
 }
@@ -607,11 +625,11 @@ fn display_ui_extension(
             writeln_name_value(f, "  Transfer fees withheld:", &withheld_amount.to_string())
         }
         UiExtension::MintCloseAuthority(UiMintCloseAuthority { close_authority }) => {
-            writeln_name_value(
-                f,
-                "  Close authority:",
-                close_authority.as_ref().unwrap_or(&String::new()),
-            )
+            if let Some(close_authority) = close_authority {
+                writeln_name_value(f, "  Close authority:", close_authority)
+            } else {
+                Ok(())
+            }
         }
         UiExtension::ConfidentialTransferMint(_) => unimplemented!(),
         UiExtension::ConfidentialTransferAccount(_) => unimplemented!(),
@@ -656,6 +674,18 @@ fn display_ui_extension(
                 rate_authority.as_ref().unwrap_or(&String::new()),
             )
         }
+        UiExtension::CpiGuard(UiCpiGuard { lock_cpi }) => writeln_name_value(
+            f,
+            "  CPI Guard:",
+            if *lock_cpi { "Enabled" } else { "Disabled" },
+        ),
+        UiExtension::PermanentDelegate(UiPermanentDelegate { delegate }) => {
+            if let Some(delegate) = delegate {
+                writeln_name_value(f, "  Permanent delegate:", delegate)
+            } else {
+                Ok(())
+            }
+        }
         // ExtensionType::Uninitialized is a hack to ensure a mint/account is never the same length as a multisig
         UiExtension::Uninitialized => Ok(()),
         UiExtension::UnparseableExtension => writeln_name_value(
@@ -663,9 +693,6 @@ fn display_ui_extension(
             "    Unparseable extension:",
             "Consider upgrading to a newer version of spl-token",
         ),
-        // XXX HANA this is needed temporarily to make monorepo ci happy while the new cases are added
-        #[allow(unreachable_patterns)]
-        _ => unimplemented!(),
     }
 }
 
diff --git a/token/cli/src/sort.rs b/token/cli/src/sort.rs
index 044a3b713..a70974f9e 100644
--- a/token/cli/src/sort.rs
+++ b/token/cli/src/sort.rs
@@ -76,6 +76,7 @@ pub(crate) fn sort_and_parse_token_accounts(
                         program_id: program_id.to_string(),
                         account: ui_token_account,
                         is_associated,
+                        has_permanent_delegate: false,
                     };
 
                     let entry = cli_accounts.entry(btree_key);
diff --git a/token/js/src/extensions/extensionType.ts b/token/js/src/extensions/extensionType.ts
index 060a0e058..1dda8bf95 100644
--- a/token/js/src/extensions/extensionType.ts
+++ b/token/js/src/extensions/extensionType.ts
@@ -67,6 +67,50 @@ export function getTypeLen(e: ExtensionType): number {
     }
 }
 
+export function isMintExtension(e: ExtensionType): boolean {
+    switch (e) {
+        case ExtensionType.TransferFeeConfig:
+        case ExtensionType.MintCloseAuthority:
+        case ExtensionType.ConfidentialTransferMint:
+        case ExtensionType.DefaultAccountState:
+        case ExtensionType.NonTransferable:
+        case ExtensionType.InterestBearingConfig:
+        case ExtensionType.PermanentDelegate:
+            return true;
+        case ExtensionType.Uninitialized:
+        case ExtensionType.TransferFeeAmount:
+        case ExtensionType.ConfidentialTransferAccount:
+        case ExtensionType.ImmutableOwner:
+        case ExtensionType.MemoTransfer:
+        case ExtensionType.CpiGuard:
+            return false;
+        default:
+            throw Error(`Unknown extension type: ${e}`);
+    }
+}
+
+export function isAccountExtension(e: ExtensionType): boolean {
+    switch (e) {
+        case ExtensionType.TransferFeeAmount:
+        case ExtensionType.ConfidentialTransferAccount:
+        case ExtensionType.ImmutableOwner:
+        case ExtensionType.MemoTransfer:
+        case ExtensionType.CpiGuard:
+            return true;
+        case ExtensionType.Uninitialized:
+        case ExtensionType.TransferFeeConfig:
+        case ExtensionType.MintCloseAuthority:
+        case ExtensionType.ConfidentialTransferMint:
+        case ExtensionType.DefaultAccountState:
+        case ExtensionType.NonTransferable:
+        case ExtensionType.InterestBearingConfig:
+        case ExtensionType.PermanentDelegate:
+            return false;
+        default:
+            throw Error(`Unknown extension type: ${e}`);
+    }
+}
+
 export function getAccountTypeOfMintType(e: ExtensionType): ExtensionType {
     switch (e) {
         case ExtensionType.TransferFeeConfig:
@@ -117,7 +161,7 @@ export function getAccountLen(extensionTypes: ExtensionType[]): number {
 
 export function getExtensionData(extension: ExtensionType, tlvData: Buffer): Buffer | null {
     let extensionTypeIndex = 0;
-    while (extensionTypeIndex < tlvData.length) {
+    while (extensionTypeIndex + TYPE_SIZE + LENGTH_SIZE <= tlvData.length) {
         const entryType = tlvData.readUInt16LE(extensionTypeIndex);
         const entryLength = tlvData.readUInt16LE(extensionTypeIndex + TYPE_SIZE);
         const typeIndex = extensionTypeIndex + TYPE_SIZE + LENGTH_SIZE;


Test patch:
diff --git a/token/js/test/e2e-2022/cpiGuard.test.ts b/token/js/test/e2e-2022/cpiGuard.test.ts
index 4772b97be..18793ad2a 100644
--- a/token/js/test/e2e-2022/cpiGuard.test.ts
+++ b/token/js/test/e2e-2022/cpiGuard.test.ts
@@ -69,6 +69,9 @@ describe('cpiGuard', () => {
 
     it('enable/disable via instruction', async () => {
         let accountInfo = await getAccount(connection, account, undefined, TEST_PROGRAM_ID);
+        let cpiGuard = getCpiGuard(accountInfo);
+
+        expect(cpiGuard).to.be.null;
 
         let transaction = new Transaction().add(
             createEnableCpiGuardInstruction(account, owner.publicKey, [], TEST_PROGRAM_ID)
@@ -76,7 +79,7 @@ describe('cpiGuard', () => {
         await sendAndConfirmTransaction(connection, transaction, [payer, owner], undefined);
 
         accountInfo = await getAccount(connection, account, undefined, TEST_PROGRAM_ID);
-        let cpiGuard = getCpiGuard(accountInfo);
+        cpiGuard = getCpiGuard(accountInfo);
 
         expect(cpiGuard).to.not.be.null;
         if (cpiGuard !== null) {
@@ -99,11 +102,14 @@ describe('cpiGuard', () => {
 
     it('enable/disable via command', async () => {
         let accountInfo = await getAccount(connection, account, undefined, TEST_PROGRAM_ID);
+        let cpiGuard = getCpiGuard(accountInfo);
+
+        expect(cpiGuard).to.be.null;
 
         await enableCpiGuard(connection, payer, account, owner, [], undefined, TEST_PROGRAM_ID);
 
         accountInfo = await getAccount(connection, account, undefined, TEST_PROGRAM_ID);
-        let cpiGuard = getCpiGuard(accountInfo);
+        cpiGuard = getCpiGuard(accountInfo);
 
         expect(cpiGuard).to.not.be.null;
         if (cpiGuard !== null) {
diff --git a/token/js/test/e2e-2022/tlv.test.ts b/token/js/test/e2e-2022/tlv.test.ts
new file mode 100644
index 000000000..7f7cea6dd
--- /dev/null
+++ b/token/js/test/e2e-2022/tlv.test.ts
@@ -0,0 +1,96 @@
+import chai, { expect } from 'chai';
+import chaiAsPromised from 'chai-as-promised';
+chai.use(chaiAsPromised);
+
+import type { Connection, PublicKey, Signer } from '@solana/web3.js';
+import { sendAndConfirmTransaction, Keypair, SystemProgram, Transaction } from '@solana/web3.js';
+
+import type { Account, Mint } from '../../src';
+import {
+    createInitializeAccountInstruction,
+    getAccount,
+    getAccountLen,
+    createMint,
+    ExtensionType,
+    getExtensionData,
+    isAccountExtension,
+} from '../../src';
+import { TEST_PROGRAM_ID, newAccountWithLamports, getConnection } from '../common';
+
+const TEST_TOKEN_DECIMALS = 9;
+const ACCOUNT_EXTENSIONS = Object.values(ExtensionType)
+    .filter(Number.isInteger)
+    .filter((e: any): e is ExtensionType => isAccountExtension(e));
+
+describe('tlv test', () => {
+    let connection: Connection;
+    let payer: Signer;
+    let owner: Keypair;
+
+    before(async () => {
+        connection = await getConnection();
+        payer = await newAccountWithLamports(connection, 1000000000);
+        owner = Keypair.generate();
+    });
+
+    // test that the parser gracefully handles accounts with arbitrary extra bytes
+    it('parse account with extra bytes', async () => {
+        const initTestAccount = async (extraBytes: number) => {
+            const mintKeypair = Keypair.generate();
+            const accountKeypair = Keypair.generate();
+            const account = accountKeypair.publicKey;
+            const accountLen = getAccountLen([]) + extraBytes;
+            const lamports = await connection.getMinimumBalanceForRentExemption(accountLen);
+
+            const mint = await createMint(
+                connection,
+                payer,
+                mintKeypair.publicKey,
+                mintKeypair.publicKey,
+                TEST_TOKEN_DECIMALS,
+                mintKeypair,
+                undefined,
+                TEST_PROGRAM_ID
+            );
+
+            const transaction = new Transaction().add(
+                SystemProgram.createAccount({
+                    fromPubkey: payer.publicKey,
+                    newAccountPubkey: account,
+                    space: accountLen,
+                    lamports,
+                    programId: TEST_PROGRAM_ID,
+                }),
+                createInitializeAccountInstruction(account, mint, owner.publicKey, TEST_PROGRAM_ID)
+            );
+
+            await sendAndConfirmTransaction(connection, transaction, [payer, accountKeypair], undefined);
+
+            return account;
+        };
+
+        const promises: Promise<[number, Account] | undefined>[] = [];
+        for (let i = 0; i < 16; i++) {
+            // trying to alloc exactly one extra byte causes an unpack failure in the program when initializing
+            if (i == 1) continue;
+
+            promises.push(
+                initTestAccount(i)
+                    .then((account: PublicKey) => getAccount(connection, account, undefined, TEST_PROGRAM_ID))
+                    .then((accountInfo: Account) => {
+                        for (const extension of ACCOUNT_EXTENSIONS) {
+                            // realistically this will never fail with a non-null value, it will just throw
+                            expect(
+                                getExtensionData(extension, accountInfo.tlvData),
+                                `account parse test failed: found ${ExtensionType[extension]}, but should not have. \
+                                test case: no extensions, ${i} extra bytes`
+                            ).to.be.null;
+                        }
+                        return Promise.resolve(undefined);
+                    })
+            );
+        }
+
+        await Promise.all(promises);
+    });
+});
diff --git a/token/js/test/unit/index.test.ts b/token/js/test/unit/index.test.ts
index 18f766c11..fec92bca5 100644
--- a/token/js/test/unit/index.test.ts
+++ b/token/js/test/unit/index.test.ts
@@ -16,6 +16,8 @@ import {
     TokenOwnerOffCurveError,
     getAccountLen,
     ExtensionType,
+    isMintExtension,
+    isAccountExtension,
     getAssociatedTokenAddressSync,
     createInitializeAccount2Instruction,
     createInitializeAccount3Instruction,
@@ -238,4 +240,13 @@ describe('extensionType', () => {
         expect(getAccountLen([ExtensionType.ImmutableOwner])).to.eql(170);
         expect(getAccountLen([ExtensionType.PermanentDelegate])).to.eql(202);
     });
+
+    it('exclusive and exhaustive predicates', () => {
+        const exts = Object.values(ExtensionType).filter(Number.isInteger);
+        const mintExts = exts.filter((e: any): e is ExtensionType => isMintExtension(e));
+        const accountExts = exts.filter((e: any): e is ExtensionType => isAccountExtension(e));
+        const collectedExts = [ExtensionType.Uninitialized].concat(mintExts, accountExts);
+
+        expect(collectedExts.sort()).to.eql(exts.sort());
+    });
 });