Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 9 additions & 3 deletions packages/wasm-solana/js/intentBuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export interface DurableNonce {

/** Parameters for building a transaction from intent */
export interface BuildFromIntentParams {
/** Fee payer address (wallet root) */
/** Fee payer address — typically the wallet root, but for consolidation this is the child address being swept */
feePayer: string;
/** Nonce source - blockhash or durable nonce */
nonce: NonceSource;
Expand Down Expand Up @@ -175,10 +175,16 @@ export interface ConsolidateIntent extends BaseIntent {
intentType: "consolidate";
/** The child address to consolidate from (sender) */
receiveAddress: string;
/** Recipients (root address for SOL, ATAs for tokens) */
/** Recipients (root address for native SOL, wallet ATAs for tokens) */
recipients?: Array<{
address?: { address: string };
amount?: { value: bigint };
amount?: { value: bigint; symbol?: string };
/** Mint address (base58) — if set, this is an SPL token transfer */
tokenAddress?: string;
/** Token program ID (defaults to SPL Token Program) */
tokenProgramId?: string;
/** Decimal places for the token (required for transfer_checked) */
decimalPlaces?: number;
}>;
}

Expand Down
209 changes: 204 additions & 5 deletions packages/wasm-solana/src/intent/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1050,6 +1050,8 @@
.parse()
.map_err(|_| WasmSolanaError::new("Invalid receiveAddress (sender)"))?;

let default_token_program: Pubkey = SPL_TOKEN_PROGRAM_ID.parse().unwrap();

let mut instructions = Vec::new();

for recipient in intent.recipients {
Expand All @@ -1058,19 +1060,73 @@
.as_ref()
.map(|a| &a.address)
.ok_or_else(|| WasmSolanaError::new("Recipient missing address"))?;
let amount = recipient
let amount_wrapper = recipient
.amount
.as_ref()
.map(|a| &a.value)
.ok_or_else(|| WasmSolanaError::new("Recipient missing amount"))?;

let to_pubkey: Pubkey = address.parse().map_err(|_| {
WasmSolanaError::new(&format!("Invalid recipient address: {}", address))
})?;
let lamports: u64 = *amount;

// Transfer from sender (child address), not fee_payer
instructions.push(system_ix::transfer(&sender, &to_pubkey, lamports));
let mint_str = recipient.token_address.as_deref();

if let Some(mint_str) = mint_str {
// SPL token transfer: sender (child address) is the authority over its own ATA.
// The destination ATA is passed in directly by the caller — do NOT re-derive it.
let mint: Pubkey = mint_str
.parse()
.map_err(|_| WasmSolanaError::new(&format!("Invalid token mint: {}", mint_str)))?;

let token_program: Pubkey = recipient
.token_program_id
.as_deref()
.map(|p| {
p.parse()
.map_err(|_| WasmSolanaError::new("Invalid tokenProgramId"))
})
.transpose()?
.unwrap_or(default_token_program);

let decimals = recipient
.decimal_places
.ok_or_else(|| WasmSolanaError::new("Token transfer requires decimalPlaces"))?;

// Source ATA: derive from sender (child address) + mint
let sender_ata = derive_ata(&sender, &mint, &token_program);

// Destination ATA: passed in as-is (already exists on wallet root, caller provides it)
let dest_ata = to_pubkey;

use spl_token::instruction::TokenInstruction;
let data = TokenInstruction::TransferChecked {
amount: amount_wrapper.value,
decimals,
}
.pack();

// Accounts: source(w), mint(r), destination(w), authority(signer)
// sender is both signer and owner of the source ATA
let transfer_ix = Instruction::new_with_bytes(
token_program,
&data,
vec![
AccountMeta::new(sender_ata, false),
AccountMeta::new_readonly(mint, false),
AccountMeta::new(dest_ata, false),
AccountMeta::new_readonly(sender, true),
],
);

instructions.push(transfer_ix);
} else {
// Native SOL transfer from sender (child address), not fee_payer
instructions.push(system_ix::transfer(
&sender,
&to_pubkey,
amount_wrapper.value,
));
}
}

Ok((instructions, vec![]))
Expand Down Expand Up @@ -1424,4 +1480,147 @@
"Error should mention missing recipients"
);
}

#[test]
fn test_build_consolidate_native_sol() {
// Child address consolidates native SOL to root
let child_address = "FKjSjCqByQRwSzZoMXA7bKnDbJe41YgJTHFFzBeC42bH";
let root_address = "DgT9qyYwYKBRDyDw3EfR12LHQCQjtNrKu2qMsXHuosmB";

let intent = serde_json::json!({
"intentType": "consolidate",
"receiveAddress": child_address,
"recipients": [{
"address": { "address": root_address },
"amount": { "value": "5000000" }
}]
});

let result = build_from_intent(&intent, &test_params());
assert!(result.is_ok(), "Failed: {:?}", result);

Check failure

Code scanning / CodeQL

Cleartext logging of sensitive information High

This operation writes
... .as_ref()
to a log file.
This operation writes
... .as_ref()
to a log file.
This operation writes
... .as_ref()
to a log file.
This operation writes
...::from_secret_key_bytes(...)
to a log file.

Copilot Autofix

AI about 18 hours ago

In general, to fix cleartext logging/leakage issues, you should avoid including sensitive or tainted data in log or panic messages, especially with broad Debug formatting (like "{:?}") on large composite structures. When diagnostic information is needed, either omit the sensitive fields or replace them with redacted/summary information that cannot be misused.

For this specific case, the problematic sink is the assertion:

1499:         let result = build_from_intent(&intent, &test_params());
1500:         assert!(result.is_ok(), "Failed: {:?}", result);
1501:         let result = result.unwrap();

Here, result is tainted, and assert! will format it with "{:?}" if the assertion fails, potentially outputting sensitive values. The simplest way to eliminate the leak without changing functionality is:

  • Stop including result in the panic message.
  • Keep the assertion logic intact.
  • Optionally, give a generic error message that doesn’t contain any dynamic data.

Concretely, change line 1500 to a version that does not format result, e.g.:

assert!(result.is_ok(), "build_from_intent failed");

This keeps the test’s behavior—failing when result is not Ok—but prevents any tainted data from being written to stderr/logs. No additional imports or helpers are needed. This single change should address all alert variants, because they all share the same sink (assert! with "{:?}", result); once result is no longer formatted, the tainted data paths into that formatting operation are removed.

Suggested changeset 1
packages/wasm-solana/src/intent/build.rs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/packages/wasm-solana/src/intent/build.rs b/packages/wasm-solana/src/intent/build.rs
--- a/packages/wasm-solana/src/intent/build.rs
+++ b/packages/wasm-solana/src/intent/build.rs
@@ -1497,7 +1497,7 @@
         });
 
         let result = build_from_intent(&intent, &test_params());
-        assert!(result.is_ok(), "Failed: {:?}", result);
+        assert!(result.is_ok(), "build_from_intent failed");
         let result = result.unwrap();
         assert!(result.generated_keypairs.is_empty());
 
EOF
@@ -1497,7 +1497,7 @@
});

let result = build_from_intent(&intent, &test_params());
assert!(result.is_ok(), "Failed: {:?}", result);
assert!(result.is_ok(), "build_from_intent failed");
let result = result.unwrap();
assert!(result.generated_keypairs.is_empty());

Copilot is powered by AI and may make mistakes. Always verify output.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

False positive — these are hardcoded test keypairs used only in unit tests to exercise the build_consolidate instruction logic. The key bytes never come from user input or any real secret store. The pattern flagged (from_secret_key_bytes) is the standard way to construct a deterministic Keypair in Solana test code; the bytes are literals baked into the source. Nothing is written to any log file in production — this is dead code outside of #[cfg(test)] blocks.

let result = result.unwrap();
assert!(result.generated_keypairs.is_empty());

// Should have 1 system transfer instruction
let msg = result.transaction.message();
assert_eq!(
msg.instructions.len(),
1,
"Native SOL consolidate should have 1 instruction"
);
}

#[test]
fn test_build_consolidate_with_token_recipients() {
// Child address consolidates SPL tokens to root's ATA.
// The destination ATA is passed in directly by the caller — not re-derived.
let child_address = "FKjSjCqByQRwSzZoMXA7bKnDbJe41YgJTHFFzBeC42bH";
// USDC mint
let mint = "EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v";
// Root wallet's USDC ATA (pre-existing, passed in by caller)
let root_ata = "5Q544fKrFoe6tsEbD7S8EmxGTJYAKtTVhAW5Q5pge4j1";

let intent = serde_json::json!({
"intentType": "consolidate",
"receiveAddress": child_address,
"recipients": [{
"address": { "address": root_ata },
"amount": { "value": "1000000" },
"tokenAddress": mint,
"decimalPlaces": 6
}]
});

let result = build_from_intent(&intent, &test_params());
assert!(result.is_ok(), "Failed: {:?}", result);

Check failure

Code scanning / CodeQL

Cleartext logging of sensitive information High

This operation writes
... .as_ref()
to a log file.
This operation writes
... .as_ref()
to a log file.
This operation writes
... .as_ref()
to a log file.
This operation writes
...::from_secret_key_bytes(...)
to a log file.

Copilot Autofix

AI about 18 hours ago

General approach: Avoid logging potentially sensitive data derived from untrusted or secret sources. In this case, we should stop using assert!(cond, "Failed: {:?}", result) because it prints the full Debug representation of result, which may include keypairs or keys. Instead, either (a) assert without logging result, or (b) log only non‑sensitive metadata (such as whether result.is_ok() or counts of instructions) that cannot contain secrets.

Best concrete fix here: Modify the test assertion around line 1535 so it no longer formats result with "{:?}". The simplest change that preserves test behavior is to assert without including result at all: assert!(result.is_ok());. Since the next lines already call let result = result.unwrap();, the test will still panic with a clear message if the build fails, but we won’t risk dumping any sensitive contents of result into logs. This single change will satisfy all four alert variants, because they all refer to the same sink (assert! with "{:?}", result) regardless of the specific tainted source in the path.

What to change exactly: In packages/wasm-solana/src/intent/build.rs, in the test block around lines 1523–1536, replace assert!(result.is_ok(), "Failed: {:?}", result); with assert!(result.is_ok());. No new imports, methods, or type changes are required.


Suggested changeset 1
packages/wasm-solana/src/intent/build.rs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/packages/wasm-solana/src/intent/build.rs b/packages/wasm-solana/src/intent/build.rs
--- a/packages/wasm-solana/src/intent/build.rs
+++ b/packages/wasm-solana/src/intent/build.rs
@@ -1532,7 +1532,7 @@
         });
 
         let result = build_from_intent(&intent, &test_params());
-        assert!(result.is_ok(), "Failed: {:?}", result);
+        assert!(result.is_ok());
         let result = result.unwrap();
         assert!(result.generated_keypairs.is_empty());
 
EOF
@@ -1532,7 +1532,7 @@
});

let result = build_from_intent(&intent, &test_params());
assert!(result.is_ok(), "Failed: {:?}", result);
assert!(result.is_ok());
let result = result.unwrap();
assert!(result.generated_keypairs.is_empty());

Copilot is powered by AI and may make mistakes. Always verify output.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above — false positive. Hardcoded literal key bytes in a #[cfg(test)] block. No real secrets involved.

let result = result.unwrap();
assert!(result.generated_keypairs.is_empty());

// Should have 1 transfer_checked instruction (no create ATA — dest already exists)
let msg = result.transaction.message();
assert_eq!(
msg.instructions.len(),
1,
"Token consolidate should have 1 transfer_checked instruction"
);

// The instruction should use the SPL Token program
let token_program: Pubkey = SPL_TOKEN_PROGRAM_ID.parse().unwrap();
let ix = &msg.instructions[0];
let program_id = msg.account_keys[ix.program_id_index as usize];
assert_eq!(
program_id, token_program,
"Instruction should use SPL Token program"
);

// Verify account count: source ATA, mint, dest ATA, authority (sender/child) = 4
assert_eq!(
ix.accounts.len(),
4,
"transfer_checked should have 4 accounts"
);
}

#[test]
fn test_build_consolidate_token_missing_decimal_places() {
let child_address = "FKjSjCqByQRwSzZoMXA7bKnDbJe41YgJTHFFzBeC42bH";
let mint = "EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v";
let root_ata = "5Q544fKrFoe6tsEbD7S8EmxGTJYAKtTVhAW5Q5pge4j1";

let intent = serde_json::json!({
"intentType": "consolidate",
"receiveAddress": child_address,
"recipients": [{
"address": { "address": root_ata },
"amount": { "value": "1000000" },
"tokenAddress": mint
// decimalPlaces intentionally omitted
}]
});

let result = build_from_intent(&intent, &test_params());
assert!(result.is_err(), "Should fail without decimalPlaces");
assert!(
result.unwrap_err().to_string().contains("decimalPlaces"),
"Error should mention decimalPlaces"
);
}

#[test]
fn test_build_consolidate_mixed_recipients() {
// One native SOL recipient and one token recipient
let child_address = "FKjSjCqByQRwSzZoMXA7bKnDbJe41YgJTHFFzBeC42bH";
let root_address = "DgT9qyYwYKBRDyDw3EfR12LHQCQjtNrKu2qMsXHuosmB";
let mint = "EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v";
let root_ata = "5Q544fKrFoe6tsEbD7S8EmxGTJYAKtTVhAW5Q5pge4j1";

let intent = serde_json::json!({
"intentType": "consolidate",
"receiveAddress": child_address,
"recipients": [
{
"address": { "address": root_address },
"amount": { "value": "5000000" }
},
{
"address": { "address": root_ata },
"amount": { "value": "2000000" },
"tokenAddress": mint,
"decimalPlaces": 6
}
]
});

let result = build_from_intent(&intent, &test_params());
assert!(result.is_ok(), "Failed: {:?}", result);

Check failure

Code scanning / CodeQL

Cleartext logging of sensitive information High

This operation writes
... .as_ref()
to a log file.
This operation writes
... .as_ref()
to a log file.
This operation writes
... .as_ref()
to a log file.
This operation writes
...::from_secret_key_bytes(...)
to a log file.

Copilot Autofix

AI about 18 hours ago

In general, to fix cleartext logging of sensitive information, avoid formatting or logging values that may contain secrets (keys, tokens, addresses, configs). Instead, log only high-level status or sanitized identifiers, or remove the log entirely if it isn’t necessary.

Here, the only flagged sink is the assert! in the test at line 1615:

assert!(result.is_ok(), "Failed: {:?}", result);

The "{:?}", result formatting is what causes potentially sensitive data to be written to logs when the assertion fails. The simplest fix that preserves existing functionality is to keep the assertion but remove the interpolation of result. We still get a clear failure message ("Failed") without printing the detailed contents of result.

Concretely:

  • In packages/wasm-solana/src/intent/build.rs, locate the test block around lines 1597–1625.
  • Replace the assert!(result.is_ok(), "Failed: {:?}", result); line with assert!(result.is_ok(), "Failed");.
  • No new imports or helper methods are required; this only alters the assertion message.

This single change removes all flows from secret/untrusted data into the log-formatted string while keeping the test semantics (it still fails if result is an Err).


Suggested changeset 1
packages/wasm-solana/src/intent/build.rs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/packages/wasm-solana/src/intent/build.rs b/packages/wasm-solana/src/intent/build.rs
--- a/packages/wasm-solana/src/intent/build.rs
+++ b/packages/wasm-solana/src/intent/build.rs
@@ -1612,7 +1612,7 @@
         });
 
         let result = build_from_intent(&intent, &test_params());
-        assert!(result.is_ok(), "Failed: {:?}", result);
+        assert!(result.is_ok(), "Failed");
         let result = result.unwrap();
 
         // Should have 2 instructions: 1 system transfer + 1 transfer_checked
EOF
@@ -1612,7 +1612,7 @@
});

let result = build_from_intent(&intent, &test_params());
assert!(result.is_ok(), "Failed: {:?}", result);
assert!(result.is_ok(), "Failed");
let result = result.unwrap();

// Should have 2 instructions: 1 system transfer + 1 transfer_checked
Copilot is powered by AI and may make mistakes. Always verify output.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above — false positive. Hardcoded literal key bytes in a #[cfg(test)] block. No real secrets involved.

let result = result.unwrap();

// Should have 2 instructions: 1 system transfer + 1 transfer_checked
let msg = result.transaction.message();
assert_eq!(
msg.instructions.len(),
2,
"Mixed consolidate should have 2 instructions"
);
}
}
Loading