Skip to content

Fix: Harden onboarding state integrity, chart rendering, and type safety - #757

Open
mertcano wants to merge 1 commit into
0xMiden:mainfrom
mertcano:mertcano-patch-1
Open

Fix: Harden onboarding state integrity, chart rendering, and type safety#757
mertcano wants to merge 1 commit into
0xMiden:mainfrom
mertcano:mertcano-patch-1

Conversation

@mertcano

Copy link
Copy Markdown

Description

This PR addresses onboarding state integrity, dynamic HTML injection, and type safety vulnerabilities within the wallet repository.

Vulnerabilities & Security Defects Remediated:

  • Onboarding State Integrity / Wallet Creation Failure (src/app/pages/Welcome.tsx, src/app/pages/Welcome.test.tsx): The mobile create-password-submit flow previously stored the user's password and navigated to confirmation without guaranteeing that a seedPhrase was generated. This caused the subsequent register() call to throw a "Missing password or seed phrase" error, leaving the user stranded. The flow has been updated to reliably generate a mnemonic if the create flow hasn't already produced one, successfully preserving the mnemonic from passcode and biometric paths. Comprehensive regression tests were also added to cover mnemonic generation, wallet registration, and backend synchronization.
  • Dynamic HTML Injection Surface (src/lib/ui/charts.tsx): The ChartStyle component previously used dangerouslySetInnerHTML to insert dynamically generated CSS, which included caller-supplied chart identifiers and configuration keys. This has been safely refactored to render the CSS string as a standard React text child inside a <style> tag, removing the HTML injection sink while preserving the output contract.
  • Type Safety and Malformed Payload Handling (src/lib/ui/charts.tsx, src/lib/ui/charts.test.tsx): Tooltip and legend components previously used broad any types and unsafely assumed item.payload.fill was always an object. The implementation now uses explicit types for payloads, formatters, and legends, securely handling primitive or null nested payloads with strict record guards (isRecord). Test helpers were also updated to eliminate any casts for configurations and JSX elements.

### Description
This PR addresses onboarding state integrity, dynamic HTML injection, and type safety vulnerabilities within the `wallet` repository.

**Vulnerabilities & Security Defects Remediated:**
* **Onboarding State Integrity / Wallet Creation Failure (`src/app/pages/Welcome.tsx`, `src/app/pages/Welcome.test.tsx`):** The mobile create-password-submit flow previously stored the user's password and navigated to confirmation without guaranteeing that a `seedPhrase` was generated. This caused the subsequent `register()` call to throw a "Missing password or seed phrase" error, leaving the user stranded. The flow has been updated to reliably generate a mnemonic if the create flow hasn't already produced one, successfully preserving the mnemonic from passcode and biometric paths. Comprehensive regression tests were also added to cover mnemonic generation, wallet registration, and backend synchronization.
* **Dynamic HTML Injection Surface (`src/lib/ui/charts.tsx`):** The `ChartStyle` component previously used `dangerouslySetInnerHTML` to insert dynamically generated CSS, which included caller-supplied chart identifiers and configuration keys. This has been safely refactored to render the CSS string as a standard React text child inside a `<style>` tag, removing the HTML injection sink while preserving the output contract.
* **Type Safety and Malformed Payload Handling (`src/lib/ui/charts.tsx`, `src/lib/ui/charts.test.tsx`):** Tooltip and legend components previously used broad `any` types and unsafely assumed `item.payload.fill` was always an object. The implementation now uses explicit types for payloads, formatters, and legends, securely handling primitive or null nested payloads with strict record guards (`isRecord`). Test helpers were also updated to eliminate `any` casts for configurations and JSX elements.
@github-actions

Copy link
Copy Markdown
Contributor

👋 Thanks for contributing to Miden, @mertcano!

We really appreciate you taking the time to open this pull request. Miden is building an edge-first, zero-knowledge blockchain, and thoughtful contributions from the community are a big part of how it gets better. A maintainer will review your changes as soon as they can — in the meantime, please make sure the CI checks are green and that your change follows the repository's contribution guidelines.

We're genuinely excited to have you here. 🧡


One thing we want to be transparent about up front: contributing to this repository will not make you eligible for any token airdrop, allocation, or other reward — not now, and not at any point in the future.

Thanks again for being part of the community — we're glad you chose to contribute. 🚀

@canoplos

Copy link
Copy Markdown

hi.
The type-safety work here is solid. Replacing the any props on ChartTooltipContent / ChartLegendContent with proper ChartPayload types is a real improvement, and it surfaced a genuine null-safety bug in the process: the old item.payload.fill would throw when payload was a string or null, and guarding it with isRecord(item.payload) && typeof item.payload.fill === 'string' (plus passing isRecord(...) ? item.payload : {} into formatter) is the correct fix. isRecord is defined locally and used consistently, including in getPayloadConfigFromPayload. Swapping dangerouslySetInnerHTML for <style>{css}</style> is a nice cleanup too.

One process note, not about the logic: the raw diff is huge (~2400 lines each way) but almost all of it is reformatting — the actual change is only ~120/68 lines with -w. Mixing a full reformat with real logic changes in one PR makes it hard to review the substance and easy for something to hide in the noise. If the reformat came from a Prettier/config change, splitting it into its own commit (or PR) would make this much easier to review and to bisect later. Not blocking, but worth considering.

The substantive changes LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants