feat: add SPL token transfer support to build_consolidate#216
Conversation
build_consolidate previously only emitted native SOL system transfers. Now it also handles SPL token recipients by emitting transfer_checked instructions, reusing the same pattern as build_payment. The source ATA is derived from the sender (child address) + mint; the destination ATA is passed in directly by the caller (already exists on the wallet root). Also updates the ConsolidateIntent TypeScript type to include tokenAddress, tokenProgramId, and decimalPlaces on recipients, matching PaymentIntent. Ticket: BTC-3188
06ec850 to
874f1c4
Compare
packages/wasm-solana/src/intent/build.rs
Dismissed
| }); | ||
|
|
||
| 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
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 16 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
resultin 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.
| @@ -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()); | ||
|
|
There was a problem hiding this comment.
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.
packages/wasm-solana/src/intent/build.rs
Dismissed
| }); | ||
|
|
||
| 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
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 16 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.
| @@ -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()); | ||
|
|
There was a problem hiding this comment.
Same as above — false positive. Hardcoded literal key bytes in a #[cfg(test)] block. No real secrets involved.
packages/wasm-solana/src/intent/build.rs
Dismissed
| }); | ||
|
|
||
| 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
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 16 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 withassert!(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).
| @@ -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 |
There was a problem hiding this comment.
Same as above — false positive. Hardcoded literal key bytes in a #[cfg(test)] block. No real secrets involved.
build_consolidate previously only emitted native SOL system transfers.
Now it also handles SPL token recipients by emitting transfer_checked
instructions, reusing the same pattern as build_payment. The source ATA
is derived from the sender (child address) + mint; the destination ATA
is passed in directly by the caller (already exists on the wallet root).
Also updates the ConsolidateIntent TypeScript type to include tokenAddress,
tokenProgramId, and decimalPlaces on recipients, matching PaymentIntent.
Ticket: BTC-3188