Fix Terra Classic / Keplr ADR-036 sign + verify (#1) #23
No reviewers
Labels
No labels
agent:implement
agent:ready
api
bot
bug
ci
enhancement
ready
security
terra-classic
testing
ux
web
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
code/cl8y-ecosystem-legal!23
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "1-fix-terra-classic-keplr-adr-036-verify-correct-frontend-signarbitrary"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
signArbitrary(columbus-5, signer, data)(not OfflineSigner 2-arg misuse).sign/MsgSignDatadigests, with pubkey →terra1…binding and bech32 address validation.skills/terra-classic-adr036/.Test plan
cd api && cargo test(unit + Terra/EVM integration, including abuse negatives)cd api && cargo fmt --check && cargo clippy --all-targets -- -D warningscd bot && cargo fmt --check && cargo clippy -- -D warningscd web && npm run test:e2e -- --workers=5(includesterra-sign+ EVM regression)/sign/terra-classic?property=…once (optional; mocked in CI)test:rust+test:e2egreen on this MR (pipeline 2745946647)Closes #1
added 1 commit
74a80e65- Fix Terra Classic Keplr signing with ADR-036 verify.Compare with previous version
marked this merge request as ready
changed the description
mentioned in issue #1
RECOMMEND: FIX
Review summary (MR !23 → closes #1)
Thorough security + gap review of the Terra Classic / Keplr ADR-036 fix (portal + API + tests + skill/gap docs). Crypto path looks sound and correctly replaces a production-broken stack (OfflineSigner misuse + raw-byte ECDSA). Not a BLOCK — no forgery bypass, no prompt-injection runtime surface, no DeFi/oracle/tokenomic surface (off-chain legal clickwrap only).
Why FIX (not ACCEPT): issue #1 verification criteria are not fully met on this MR, and the gap note overclaims / contradicts itself in places. Address the required items below before merge (or explicitly waive with a follow-up issue).
Issue #1 acceptance checklist
keplr.signArbitrary(chainId, signer, data)web/src/pages/terra.ts(columbus-5)api/src/verify/terra.rs+rejects_raw_message_signaturesigned_latest: truevalidate_timestamp; no Terra HTTP negative integration testscolumbus-5Issue #1 verification criteria
cargo testincludes Terra ADR-036 greenintegration_terra_classic_adr036_wallet_submitpass heretest:rust+test:e2egreen on this MRlint:gitleaks(~9s). In.gitlab-ci.yml, only gitleaks hasrulesformerge_request_event;test:rust/test:e2edo not run on MR pipelinesRelated #4 (broader testing) is not closed by this MR; Terra coverage here is a solid down-payment, not the full #4 matrix.
Security audit
Applicable / N/A threat classes
skills/terra-classic-adr036/SKILL.mdis repo-controlled agent guidance, not a runtime LLM tool ingesting untrusted user content. No BLOCK-level prompt injection.Crypto verdict: sound
sign/MsgSignDataserialize (sorted keys +&/</>escapes).k256VerifierSHA-256s sign-doc bytes (matches CosmJS/Keplr).terra1(HRPterra, 20-byte) binding; bech32 checksum (not prefix-only).build_wallet_message+verify_message_matchesbefore verify..gitlab-ci.yml+ MR pipelinetest:rust/test:e2e— false confidence vs #1 verificationDefaultBodyLimit/ field caps on wallet submit (pre-existing; still open)web/src/pages/terra.ts+account.rsapi/tests/integration_test.rschain_id: ""normalize_sGap analysis (features / UX / testing / packages / DRY)
test:rust+test:e2erun onmerge_request_event(same rules as gitleaks), or paste a pipeline URL where those jobs are green for this SHAgaps/GAP_1786322222.mdmarks Terra Resolved but §5 still says “No Solana/Terra/Telegram end-to-end” and later still lists Terra e2e as missing — update stale sections now that mock Terra e2e existsPOST /signatures/walletString(e); no Keplr install guidanceviem^2.30→^2.55+ largepackage-lock.jsonchurn — split or justifyweb/e2e/helpers/keplr-wallet.ts(TS escape also weaker for rare control chars)Packages:
api+webchanges are appropriate;packages/cl8y-clickwraponly README touch (OK for scope); skill + gap docs are welcome but need consistency.Playwright:
workers: 5already configured; Terra mock e2e is a good addition.Required before ACCEPT
test:rustandtest:e2e(fix jobrules, or equivalent evidence).web/e2e/terra-sign.spec.tsexists; keep remaining Solana/Telegram WebApp P0s clear.Optional / follow-up OK: real Keplr smoke, body limits, client address canonicalize, viem bump split, terms UX (#2), CI breadth (#4).
Bottom line
Ship-quality crypto fix for #1’s core bug, with solid unit vectors and a mocked e2e path. Do not merge yet until MR CI actually exercises Rust + e2e (and the gap note stops contradicting the resolution claim). After that, this can flip to ACCEPT without further crypto changes.
Addendum — CI failures on branch push (same SHA
74a80e65)Correction/expansion of the CI section above:
merge_request_eventlint:gitleakspushlint:rust+lint:rust-botfailed; alltest:*skippedMR-owned lint breakages (must fix in this MR):
cargo fmt --checkdiffs inapi/src/verify/terra.rsandapi/tests/integration_test.rs(runcd api && cargo fmt).-D warnings:clippy::useless_concatinapi/src/verify/terra.rsCosmJS fixture test (concat!(r#"..."#)→ plain string).Pre-existing (not introduced by Terra diff, but still red on push):
lint:rust-botrustfmt drift acrossbot/too_many_argumentsonsubmit_wallet/submit_telegram(and related) when local toolchain is stricter than CI imageUntil fmt/clippy are green and
test:rust/test:e2eactually execute for this SHA, issue #1 verification remains unmet → FIX stands.Follow-up pushed (
62d8d76)Addresses the FIX review items on this MR:
workflow:rulesso MR pipelines run full lint/test (not gitleaks-only)cargo fmt+ clippy clean (api + bot); Terrauseless_concatfixedDefaultBodyLimit; portal bech32 canonicalizeskills/terra-classic-adr036+ README invariants updatedLocal verify:
cargo test30/30; Playwright e2e 13/13 (--workers=5).changed the description
Pushed e2e CI startup fix (
vitehost127.0.0.1, prebuilt API binary, longer webServer timeout). Awaiting greentest:e2e.changed the description
CI green on
6ec0af6: pipeline 2745946647 —lint:*,test:rust,test:e2e, and builds all success.Issue #1 verification for automated CI is met. Optional remaining: one manual real-Keplr smoke.
added 2 commits
62d8d76c- Harden Terra ADR-036 CI, abuse tests, and portal invariants.6ec0af69- Stabilize Playwright e2e startup on CI.Compare with previous version
mentioned in issue #2
mentioned in issue #3
mentioned in commit
03849d3bed