Complete RN-03 NFC hardening and stabilize RN SDK tests#1797
Merged
transphorm merged 13 commits intodevfrom Mar 11, 2026
Merged
Complete RN-03 NFC hardening and stabilize RN SDK tests#1797transphorm merged 13 commits intodevfrom
transphorm merged 13 commits intodevfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds strict APDU validation and allowlisting, per-APDU transceive timeouts (configurable, default 10s) with timeout guards, per-command audit metadata and expanded NFC APDU error codes; extends tests and Vitest config; updates security handoff docs; removes embedded webview assets and adjusts asset copy behavior. (50 words) Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Handler as NfcHandler
participant Validator as ValidateAPDU
participant Guard as TimeoutGuard
participant NFC as NFC_Hardware
participant Audit as ErrorAudit
Client->>Handler: handle('scan', { apduCommands })
Handler->>Handler: init audit counters (total, accepted, rejected, timedOut)
loop per command (i)
Handler->>Validator: validateApduCommand(bytes)
alt invalid
Validator->>Audit: record INVALID_PARAMS (i)
Audit->>Handler: throw INVALID_PARAMS + details
Handler->>Client: return error (commandIndex, counts)
else valid
Handler->>Guard: transceive(apdu) with timeout(apduTimeoutMs)
Guard->>NFC: transceive(apdu)
alt timeout
Guard->>Audit: record NFC_APDU_TIMEOUT (i)
Audit->>Handler: throw NFC_APDU_TIMEOUT + details
Handler->>Client: return timeout error (commandIndex, counts)
else success
NFC->>Guard: response
Guard->>Handler: increment accepted
Handler->>Client: append response
end
end
end
Handler->>Client: final responses and audit summary
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rn-sdk/src/handlers/NfcHandler.ts (1)
58-71:⚠️ Potential issue | 🟠 MajorAdd hard limits for APDU batch count and command size.
apduCommandsand per-command hex length are currently unbounded. A malicious WebView payload can force heavy parsing/processing and significantly extend scan duration.🛡️ Proposed bounds guard
const DEFAULT_APDU_TIMEOUT_MS = 10_000; +const MAX_APDU_COMMANDS = 32; +const MAX_APDU_HEX_CHARS = 261 * 2; // short APDU max bytes (with optional Le) function parseApduCommand(hexCommand: string): number[] { const normalized = hexCommand.trim().replace(/\s+/g, '').toUpperCase(); + if (normalized.length > MAX_APDU_HEX_CHARS) { + throw new BridgeHandlerError('INVALID_PARAMS', 'APDU command too long'); + } if (!/^[0-9A-F]+$/.test(normalized) || normalized.length % 2 !== 0) { throw new BridgeHandlerError( 'INVALID_PARAMS', 'Invalid APDU hex command format', ); } @@ const apduCommands = Array.isArray(params.apduCommands) ? params.apduCommands.filter((entry): entry is string => typeof entry === 'string') : []; + if (apduCommands.length > MAX_APDU_COMMANDS) { + throw new BridgeHandlerError('INVALID_PARAMS', 'Too many APDU commands'); + }Also applies to: 306-314
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rn-sdk/src/handlers/NfcHandler.ts` around lines 58 - 71, parseApduCommand currently allows arbitrarily many/large APDU hex strings which a malicious WebView could abuse; add hard limits by validating the number of APDU commands and each command's hex length before parsing (e.g., enforce MAX_APDU_COMMANDS and MAX_HEX_LENGTH constants), and throw BridgeHandlerError('INVALID_PARAMS', ...) if either limit is exceeded; apply the same guard to the other APDU-handling site referenced (the block around lines ~306-314) so both the parseApduCommand function and the alternate handler validate command count and per-command hex length prior to Number.parseInt processing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rn-sdk/HANDOFF.md`:
- Around line 109-112: Update the HANDOFF.md guidance for APDU debug logging:
change the current "local debug mode" allowance for logging apduResponses and
tagId to require an explicit, short-lived, approval-gated debug enablement
process (e.g., a named feature flag or documented approval step), mandate
scrubbed logs and automatic expiry of the debug flag, and explicitly state that
apduResponses and tagId must never be sent to analytics/crash logs or persisted
unless the approval process is followed; reference the sensitive identifiers
"tagId" and "apduResponses" in the text so readers know exactly which fields are
protected.
---
Outside diff comments:
In `@packages/rn-sdk/src/handlers/NfcHandler.ts`:
- Around line 58-71: parseApduCommand currently allows arbitrarily many/large
APDU hex strings which a malicious WebView could abuse; add hard limits by
validating the number of APDU commands and each command's hex length before
parsing (e.g., enforce MAX_APDU_COMMANDS and MAX_HEX_LENGTH constants), and
throw BridgeHandlerError('INVALID_PARAMS', ...) if either limit is exceeded;
apply the same guard to the other APDU-handling site referenced (the block
around lines ~306-314) so both the parseApduCommand function and the alternate
handler validate command count and per-command hex length prior to
Number.parseInt processing.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/rn-sdk/assets/self-wallet/assets/index-CmyM-wgW.js.mapis excluded by!**/*.map
📒 Files selected for processing (8)
packages/rn-sdk/HANDOFF.mdpackages/rn-sdk/assets/self-wallet/assets/index-CmyM-wgW.jspackages/rn-sdk/assets/self-wallet/index.htmlpackages/rn-sdk/src/__tests__/NfcHandler.test.tspackages/rn-sdk/src/handlers/NfcHandler.tspackages/rn-sdk/tests/NfcHandler.test.tspackages/rn-sdk/vitest.config.tsspecs/handoff-p1-fixes/SECURITY-HARDENING.md
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rn-sdk/HANDOFF.md (1)
107-118: Strong data-handling guidance; consider explicit approval workflow language.This section substantially strengthens the debug logging safeguards compared to earlier iterations. The requirements for a named flag (not generic
debug: true), automatic expiry, local-only output, and mandatory scrubbing before release provide comprehensive protection for sensitive NFC data.The "requires re-approval on each launch" option in line 115 provides a form of approval gating. However, to align with the organization's debug-level secrets policy, consider explicitly mentioning an "approval workflow" or "documented approval process" in the criteria. This would make the approval-gating requirement unambiguous.
📋 Optional enhancement for explicit approval language
2. The flag has automatic expiry (e.g., single session, time-limited, or requires re-approval on each launch). + 2. The flag requires approval (e.g., documented approval process, tokenized environment flag) and has automatic expiry (e.g., single session, max 24 hours, or re-approval on each launch).Based on learnings, debug-level secrets require tokenized environment flags with approval workflow and limited lifetime (max 24 hours).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rn-sdk/HANDOFF.md` around lines 107 - 118, Update the NFC Data-Handling Guidance to explicitly require a documented approval workflow and tokenized environment flag for APDU debugging: state that the named debug flag (e.g., NFC_APDU_DEBUG) must be issued via a documented approval process (approval workflow), be tokenized/short-lived (max 24 hours), and require re-approval after expiry or per launch; keep the other constraints (local-only output, no network transmission, logs scrubbed) intact and reference the protected fields tagId and apduResponses when describing the approval requirement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/rn-sdk/HANDOFF.md`:
- Around line 107-118: Update the NFC Data-Handling Guidance to explicitly
require a documented approval workflow and tokenized environment flag for APDU
debugging: state that the named debug flag (e.g., NFC_APDU_DEBUG) must be issued
via a documented approval process (approval workflow), be tokenized/short-lived
(max 24 hours), and require re-approval after expiry or per launch; keep the
other constraints (local-only output, no network transmission, logs scrubbed)
intact and reference the protected fields tagId and apduResponses when
describing the approval requirement.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
packages/rn-sdk/assets/self-wallet/assets/index-BZlxLbn7.js.mapis excluded by!**/*.mappackages/rn-sdk/assets/self-wallet/fonts/Advercase-Regular.otfis excluded by!**/*.otfpackages/rn-sdk/assets/self-wallet/fonts/DINOT-Bold.otfis excluded by!**/*.otfpackages/rn-sdk/assets/self-wallet/fonts/DINOT-Medium.otfis excluded by!**/*.otfpackages/rn-sdk/assets/self-wallet/fonts/IBMPlexMono-Regular.otfis excluded by!**/*.otf
📒 Files selected for processing (5)
packages/rn-sdk/.gitignorepackages/rn-sdk/HANDOFF.mdpackages/rn-sdk/assets/self-wallet/assets/index-BZlxLbn7.jspackages/rn-sdk/assets/self-wallet/assets/index-VdzGwUkN.csspackages/rn-sdk/assets/self-wallet/index.html
💤 Files with no reviewable changes (2)
- packages/rn-sdk/assets/self-wallet/assets/index-VdzGwUkN.css
- packages/rn-sdk/assets/self-wallet/index.html
✅ Files skipped from review due to trivial changes (1)
- packages/rn-sdk/.gitignore
transphorm
changed the title
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/rn-sdk-test-app-ci.yml (1)
38-43:⚠️ Potential issue | 🟠 MajorAdd explicit asset copy before rn-sdk tests
Line 42 runs tests without an explicit
copy-assetsstep in this workflow. That leaves the CI path vulnerable to missing-asset/setup failures the PR is trying to eliminate.Suggested workflow patch
- name: Typecheck rn-sdk run: yarn workspace `@selfxyz/rn-sdk` typecheck + - name: Copy rn-sdk assets + run: yarn workspace `@selfxyz/rn-sdk` copy-assets - name: Test rn-sdk run: yarn workspace `@selfxyz/rn-sdk` test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/rn-sdk-test-app-ci.yml around lines 38 - 43, CI currently runs "Test rn-sdk" without explicitly copying assets; add an explicit asset-copy step (e.g., a step named "Copy rn-sdk assets" that runs "yarn workspace `@selfxyz/rn-sdk` copy-assets" or the repo's canonical asset copy script) before the "Test rn-sdk" step (place it after "Typecheck rn-sdk") so tests run with assets present; update the sequence of steps "Build webview-app", "Typecheck rn-sdk", then the new "Copy rn-sdk assets", and finally "Test rn-sdk".
♻️ Duplicate comments (1)
packages/rn-sdk/HANDOFF.md (1)
113-116:⚠️ Potential issue | 🟠 MajorRequire tokenized, approval-gated debug enablement for raw APDU logging.
The current wording allows a named flag without explicitly requiring an approval-gated token and a hard max lifetime. Please tighten this section to mandate a tokenized debug flag with approval workflow and explicit expiry (e.g., ≤24h).
Proposed documentation update
-- 1. A named debug flag (e.g., `NFC_APDU_DEBUG`) is enabled explicitly by a developer — not by a generic `debug: true` prop. -- 2. The flag has automatic expiry (e.g., single session, time-limited, or requires re-approval on each launch). + 1. A tokenized, approval-gated debug flag (e.g., `NFC_APDU_DEBUG_TOKEN=<issued-token>`) is required; generic debug props are insufficient. + 2. The token must be short-lived with a hard maximum lifetime (e.g., ≤24h) and must expire automatically.Based on learnings: Debug-level secrets require tokenized environment flags (e.g.,
DEBUG_SECRETS_TOKEN=abc123) with approval workflow and limited lifetime (max 24 hours).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rn-sdk/HANDOFF.md` around lines 113 - 116, Update the HANDOFF.md policy text to require a tokenized, approval-gated debug enablement for raw APDU logging: change the first bullet (currently allowing a named debug flag like NFC_APDU_DEBUG) to require a tokenized flag (e.g., DEBUG_SECRETS_TOKEN or NFC_APDU_DEBUG_TOKEN) that must be obtained via an explicit approval workflow, enforce automatic expiry with a hard max lifetime (≤24 hours), and state that tokens must be single-session/time-limited and never enabled via a generic debug prop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rn-sdk/src/__tests__/NfcHandler.test.ts`:
- Around line 237-255: The test currently sets apduTimeoutMs: 1 and relies on
real timers which flakes; switch the test to use fake timers
(vi.useFakeTimers()), keep the mock transceive as a never-resolving promise,
call handler.handle('scan', ...) to start the operation, then advance timers
using vi.advanceTimersByTime(apduTimeoutMs + a small buffer) to trigger the
timeout path in NfcHandler/withTimeout logic, await the rejected promise and
assert the NFC_APDU_TIMEOUT error details, and finally restore real timers with
vi.useRealTimers(); reference NfcHandler, mockNfc.manager.transceive, and the
apduTimeoutMs option to locate the changes.
In `@packages/rn-sdk/src/handlers/NfcHandler.ts`:
- Around line 256-260: The constructor of NfcHandler assigns
options?.apduTimeoutMs directly to this.apduTimeoutMs allowing <=0, NaN or
absurdly large values; validate and clamp the value before assignment by: check
if options?.apduTimeoutMs is a finite number and > 0, otherwise fall back to
DEFAULT_APDU_TIMEOUT_MS, and constrain the value to an upper bound (introduce
MAX_APDU_TIMEOUT_MS constant) so this.apduTimeoutMs =
Math.min(Math.max(validatedValue, 1), MAX_APDU_TIMEOUT_MS) (referencing the
constructor, property apduTimeoutMs, class NfcHandler, and
DEFAULT_APDU_TIMEOUT_MS).
- Around line 58-64: The parseApduCommand function currently accepts unbounded
hexCommand input; add a maximum allowed length check before further processing
to avoid large-memory/CPU parsing attacks. Define a constant MAX_APDU_HEX_LENGTH
(e.g., 2048 characters or a project-appropriate limit) and after computing
normalized call a guard like if (normalized.length === 0 || normalized.length >
MAX_APDU_HEX_LENGTH) throw new BridgeHandlerError('INVALID_PARAMS','Invalid APDU
hex command format'); then continue the existing regex/byte-pair checks so
oversized inputs are rejected early in parseApduCommand.
---
Outside diff comments:
In @.github/workflows/rn-sdk-test-app-ci.yml:
- Around line 38-43: CI currently runs "Test rn-sdk" without explicitly copying
assets; add an explicit asset-copy step (e.g., a step named "Copy rn-sdk assets"
that runs "yarn workspace `@selfxyz/rn-sdk` copy-assets" or the repo's canonical
asset copy script) before the "Test rn-sdk" step (place it after "Typecheck
rn-sdk") so tests run with assets present; update the sequence of steps "Build
webview-app", "Typecheck rn-sdk", then the new "Copy rn-sdk assets", and finally
"Test rn-sdk".
---
Duplicate comments:
In `@packages/rn-sdk/HANDOFF.md`:
- Around line 113-116: Update the HANDOFF.md policy text to require a tokenized,
approval-gated debug enablement for raw APDU logging: change the first bullet
(currently allowing a named debug flag like NFC_APDU_DEBUG) to require a
tokenized flag (e.g., DEBUG_SECRETS_TOKEN or NFC_APDU_DEBUG_TOKEN) that must be
obtained via an explicit approval workflow, enforce automatic expiry with a hard
max lifetime (≤24 hours), and state that tokens must be
single-session/time-limited and never enabled via a generic debug prop.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/rn-sdk-test-app-ci.ymlpackages/rn-sdk/HANDOFF.mdpackages/rn-sdk/package.jsonpackages/rn-sdk/src/__tests__/NfcHandler.test.tspackages/rn-sdk/src/handlers/NfcHandler.tspackages/rn-sdk/tests/NfcHandler.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rn-sdk/package.json
transphorm
changed the title
# Conflicts: # packages/rn-sdk/package.json
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 19414827 | Triggered | Generic Password | 46f5ef1 | app/ios/PassportReaderCore.swift | View secret |
| 19414827 | Triggered | Generic Password | 46f5ef1 | packages/mobile-sdk-alpha/ios/SelfSDK/PassportReaderCore.swift | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
transphorm
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests
Chores