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 description:
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());
+    });
 });