Add SatsCard parse tests and URL detection#671
Add SatsCard parse tests and URL detection#671guptapratykshh wants to merge 1 commit intobitcoinppl:masterfrom
Conversation
|
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 SatsCard parsing tests and integrates SATSCARD support into MultiFormat with explicit InvalidSatsCard/InvalidTapSigner error mapping; updates Android FFI bindings to expose the new MultiFormat.SatsCard variant and corresponding error handling and tag adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MultiFormat
participant TapCardParser as cove_tap_card::TapCard::parse
Client->>MultiFormat: try_from_string(url)
MultiFormat-->>MultiFormat: detect domain (getsatscard.com / tapsigner.com)
alt getsatscard.com
MultiFormat->>TapCardParser: parse(url)
TapCardParser-->>MultiFormat: SatsCard(...)
MultiFormat-->>Client: MultiFormat::SatsCard(...)
TapCardParser-->>MultiFormat: TapSigner(...)
MultiFormat-->>Client: Err(InvalidSatsCard(...))
TapCardParser-->>MultiFormat: Err(parse_error)
MultiFormat-->>Client: Err(InvalidSatsCard(parse_error))
else tapsigner.com
MultiFormat->>TapCardParser: parse(url)
TapCardParser-->>MultiFormat: TapSigner(...)
MultiFormat-->>Client: MultiFormat::TapSigner(...)
TapCardParser-->>MultiFormat: SatsCard(...)
MultiFormat-->>Client: Err(InvalidTapSigner(...))
TapCardParser-->>MultiFormat: Err(parse_error)
MultiFormat-->>Client: Err(InvalidTapSigner(parse_error))
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Greptile SummaryThis PR adds seven
Confidence Score: 4/5Safe to merge after fixing the unreachable! panic in the getsatscard.com branch. One P1 finding: the new unreachable! arm in the getsatscard.com branch will panic instead of returning an error when a crafted URL (getsatscard.com domain + t=1 param) is parsed. The parse tests are thorough and correct. The SatsCard enum variant and detection branch are otherwise sound. rust/src/multi_format.rs — the getsatscard.com detection branch at lines 175-177 Important Files Changed
Reviews (1): Last reviewed commit: "Add SatsCard parse tests and URL detecti..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/src/multi_format.rs (1)
151-179:⚠️ Potential issue | 🔴 CriticalCritical:
unreachable!()on user-controlled input can panic the process.Both
unreachable!arms are reachable via crafted input because domain detection usesstring.contains(...)while variant selection insideTapCard::parsedepends on thet=1param, not the domain:
- A URL containing
tapsigner.com/startbut omittingt=1(and providingo/r) parses asTapCard::SatsCard→ hits line 161unreachable!and panics.- A URL containing
getsatscard.com/startwitht=1and acfield parses asTapCard::TapSigner→ hits line 176unreachable!and panics.Since this path is fed by QR/NFC scans (untrusted input), this is a DoS/crash vector. Return a recoverable error instead.
🛡️ Proposed fix
match tap_card { cove_tap_card::TapCard::TapSigner(card) => { return Ok(Self::from(card)); } - - cove_tap_card::TapCard::SatsCard(_card) => { - unreachable!("tapsigner.com URL should not parse as a sats card"); - } + cove_tap_card::TapCard::SatsCard(_) => { + warn!("tapsigner.com URL parsed as SatsCard; treating as unrecognized"); + return Err(MultiFormatError::UnrecognizedFormat); + } } } if string.contains("getsatscard.com/start") { let tap_card = cove_tap_card::TapCard::parse(string) .map_err(|e| MultiFormatError::InvalidSatsCard(e.into()))?; match tap_card { cove_tap_card::TapCard::SatsCard(card) => { return Ok(Self::SatsCard(card)); } - - cove_tap_card::TapCard::TapSigner(_card) => { - unreachable!("getsatscard.com URL should not parse as a tap signer"); + cove_tap_card::TapCard::TapSigner(_) => { + warn!("getsatscard.com URL parsed as TapSigner; treating as unrecognized"); + return Err(MultiFormatError::UnrecognizedFormat); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/multi_format.rs` around lines 151 - 179, The code currently uses unreachable!() in the TapCard match arms which can panic on crafted input; replace those unreachable!() calls with recoverable errors by returning Err(...) instead of panicking. Specifically, in the block that calls cove_tap_card::TapCard::parse(...) for strings containing "tapsigner.com/start", change the TapCard::SatsCard(_card) arm to return Err(MultiFormatError::InvalidTapSigner(...)) (or a new error variant like UnexpectedTapCardVariant with context) rather than unreachable!; likewise, in the "getsatscard.com/start" block change the TapCard::TapSigner(_card) arm to return Err(MultiFormatError::InvalidSatsCard(...)) (or the same new contextual variant). Keep using the result of TapCard::parse and include clear context (e.g., expected vs found variant and the original string or parsed fields) when constructing the error so callers can handle malformed/crafted inputs without crashing.
🧹 Nitpick comments (1)
rust/src/multi_format.rs (1)
42-43: WrapSatsCardinArcfor consistency across allMultiFormatvariants.All other variants in
MultiFormatuseArc(e.g.,Address,HardwareExport,Mnemonic,Transaction,TapSignerReady,TapSignerUnused,SignedPsbt). StoringSatsCardby value means the entire struct is cloned wheneverMultiFormatis cloned—this enum derivesClone, crosses FFI boundaries (uniffi), and is compared withEq. WrappingSatsCardinArckeeps cloning costs and ergonomics consistent.
cove_tap_card::SatsCardalready implementsPartialEqandEqvia its derive macros, so no compilation issues exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/multi_format.rs` around lines 42 - 43, MultiFormat's SatsCard variant should be wrapped in Arc to match the other variants and avoid expensive by-value clones: change the enum variant from SatsCard(cove_tap_card::SatsCard) to SatsCard(Arc<cove_tap_card::SatsCard>), add/use std::sync::Arc import, and update any places constructing or pattern-matching on MultiFormat::SatsCard to wrap new SatsCard values with Arc::new(...) or clone the Arc when needed; no changes to derives (Clone/Eq) are required because cove_tap_card::SatsCard already implements Eq/PartialEq.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@rust/src/multi_format.rs`:
- Around line 151-179: The code currently uses unreachable!() in the TapCard
match arms which can panic on crafted input; replace those unreachable!() calls
with recoverable errors by returning Err(...) instead of panicking.
Specifically, in the block that calls cove_tap_card::TapCard::parse(...) for
strings containing "tapsigner.com/start", change the TapCard::SatsCard(_card)
arm to return Err(MultiFormatError::InvalidTapSigner(...)) (or a new error
variant like UnexpectedTapCardVariant with context) rather than unreachable!;
likewise, in the "getsatscard.com/start" block change the
TapCard::TapSigner(_card) arm to return
Err(MultiFormatError::InvalidSatsCard(...)) (or the same new contextual
variant). Keep using the result of TapCard::parse and include clear context
(e.g., expected vs found variant and the original string or parsed fields) when
constructing the error so callers can handle malformed/crafted inputs without
crashing.
---
Nitpick comments:
In `@rust/src/multi_format.rs`:
- Around line 42-43: MultiFormat's SatsCard variant should be wrapped in Arc to
match the other variants and avoid expensive by-value clones: change the enum
variant from SatsCard(cove_tap_card::SatsCard) to
SatsCard(Arc<cove_tap_card::SatsCard>), add/use std::sync::Arc import, and
update any places constructing or pattern-matching on MultiFormat::SatsCard to
wrap new SatsCard values with Arc::new(...) or clone the Arc when needed; no
changes to derives (Clone/Eq) are required because cove_tap_card::SatsCard
already implements Eq/PartialEq.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8b05d37-95f1-4836-9efe-f96e5a81cda7
📒 Files selected for processing (2)
rust/crates/cove-tap-card/src/parse.rsrust/src/multi_format.rs
guptapratykshh
force-pushed
the
satscard-parse-tests-and-multiformat
branch
from
f607bd9 to
7dec1c4
Compare
guptapratykshh
force-pushed
the
satscard-parse-tests-and-multiformat
branch
from
7dec1c4 to
c319dc1
Compare
praveenperera
force-pushed
the
satscard-parse-tests-and-multiformat
branch
2 times, most recently
from
6de0e88 to
92257c8
Compare
guptapratykshh
force-pushed
the
satscard-parse-tests-and-multiformat
branch
from
92257c8 to
e5ee923
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/multi_format.rs`:
- Around line 164-196: The code incorrectly uses substring checks
(string.contains) to detect TapSigner vs SatsCard URLs causing
misclassification; update the detection to parse and normalize the URL (e.g.,
with url::Url::parse) and match the host and path prefix exactly (host ==
"tapsigner.com" and path starts_with "/start", or host == "getsatscard.com" and
path starts_with "/start") before calling cove_tap_card::TapCard::parse; replace
the two string.contains branches with host/path checks and keep the same
handling using TapCard::parse, Self::from(card), Self::SatsCard(card), and the
existing Self::invalid_tap_signer / Self::invalid_sats_card error constructors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b42bf06-3e07-4ebf-8721-849eebb7f020
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (3)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.ktrust/crates/cove-tap-card/src/parse.rsrust/src/multi_format.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rust/crates/cove-tap-card/src/parse.rs
guptapratykshh
force-pushed
the
satscard-parse-tests-and-multiformat
branch
from
e5ee923 to
8135fce
Compare
Add seven focused tests for SatsCard URL parsing covering unsealed state, error state, missing fields, unknown state, identity, and invalid domain. Add SatsCard variant to MultiFormat enum and getsatscard.com URL detection branch.
guptapratykshh
force-pushed
the
satscard-parse-tests-and-multiformat
branch
from
8135fce to
2dda28a
Compare
2 participants
Add seven focused tests for SatsCard URL parsing covering unsealed state, error state, missing fields, unknown state, identity, and invalid domain. Add SatsCard variant to MultiFormat enum and getsatscard.com URL detection branch.
Summary
Testing
Platform Coverage
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests