Skip to content

Bounty #427: Clear transfer private key after submit#508

Open
jtc268 wants to merge 1 commit into
ramimbo:mainfrom
jtc268:codex/b427-clear-transfer-key
Open

Bounty #427: Clear transfer private key after submit#508
jtc268 wants to merge 1 commit into
ramimbo:mainfrom
jtc268:codex/b427-clear-transfer-key

Conversation

@jtc268
Copy link
Copy Markdown

@jtc268 jtc268 commented May 27, 2026

Summary:

  • clear the transfer form private key field after every submit attempt
  • match the existing cleanup behavior already used by the GitHub link and claim wallet forms
  • add a focused wallet.js regression so the transfer flow keeps this behavior

Evidence:

Validation:

  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_wallet_api.py::test_transfer_action_clears_private_key_after_submit_attempt tests/test_wallet_api.py::test_github_wallet_actions_clear_private_key_after_submit_attempt -q: 2 passed
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_wallet_api.py tests/test_wallets.py -q: 44 passed
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest -q: 415 passed
  • uv run --extra dev ruff check .: passed
  • uv run --extra dev ruff format --check .: 79 files already formatted
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m mypy app: success
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python scripts/docs_smoke.py: docs smoke ok
  • git diff --check: clean

Summary by CodeRabbit

  • Bug Fixes

    • Private key field is now automatically cleared after transfer submission completes, ensuring sensitive data is removed from the form following both successful and failed attempts.
  • Tests

    • Added test case to verify private key field clearing behavior in the transfer submission flow.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: fd6531b1-5883-46e1-b5cd-89be9247c3a4

📥 Commits

Reviewing files that changed from the base of the PR and between d8532d4 and 600bf8e.

📒 Files selected for processing (2)
  • app/static/wallet.js
  • tests/test_wallet_api.py

📝 Walkthrough

Walkthrough

This PR adds private key field cleanup to the transfer submission handler. The setupTransfer() function now includes a finally block that clears the private_key_hex field after the submission attempt completes, whether success or failure. A test verifies the handler contains the expected DOM updates and the cleanup logic.

Changes

Private Key Cleanup

Layer / File(s) Summary
Clear private key field after transfer submission
app/static/wallet.js, tests/test_wallet_api.py
Transfer submit handler adds a finally block calling clearPrivateKeyField(form) to clear the private key after submission. Test test_transfer_action_clears_private_key_after_submit_attempt parses the handler and verifies the success/error DOM updates and the finally block that clears the field.

Possibly related PRs

  • ramimbo/mergework#307: Both PRs modify app/static/wallet.js to clear the private_key_hex via clearPrivateKeyField(form) in a finally block after wallet action submit handling, and add/update tests in tests/test_wallet_api.py asserting that cleanup behavior.
🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Bounty Pr Focus ⚠️ Warning PR imports entire project (129 files, 21,889 insertions) instead of focused bounty work. Raw summary claims +2/-0 wallet.js and +13/-0 tests, but actual diff shows full repo. Rebase to include only transfer private-key clearing: setupTransfer finally block and regression test. Exclude entire project infrastructure.
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: clearing the transfer form's private key after submission, matching the concrete surface change in wallet.js.
Description check ✅ Passed The description is comprehensive and well-structured, covering the summary, evidence (with bounty tracking), test validation, and all required check items.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Mergework Public Artifact Hygiene ✅ Passed No investment, price, cash-out, or fabricated payout claims detected. Code changes address security (private key cleanup). Existing docs properly describe MRWK.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@GHX5T-SOL GHX5T-SOL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed PR #508 at current head 600bf8eaeb2fbbc562072baf3e4a8533f22c614e as non-author GHX5T-SOL.

No blocker found.

Evidence checked:

  • Diff is limited to app/static/wallet.js and tests/test_wallet_api.py.
  • Verified setupTransfer() now clears the private-key input in a finally block after both success and failure paths.
  • Cross-checked the behavior against the existing GitHub wallet action cleanup pattern already covered by test_github_wallet_actions_clear_private_key_after_submit_attempt().
  • Verified the new regression anchors the transfer submit handler, success result path, error result path, finally, and clearPrivateKeyField(form) call.
  • GitHub CI passed on the current head. CodeRabbit was still processing at readback, so no CodeRabbit blocker was available to evaluate.

Validation run locally:

  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_wallet_api.py::test_transfer_action_clears_private_key_after_submit_attempt tests/test_wallet_api.py::test_github_wallet_actions_clear_private_key_after_submit_attempt -q -> 2 passed
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_wallet_api.py tests/test_wallets.py -q -> 44 passed
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest -q -> 415 passed
  • uv run --extra dev ruff check . -> passed
  • uv run --extra dev ruff format --check . -> 79 files already formatted
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m mypy app -> success
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python scripts/docs_smoke.py -> docs smoke ok
  • git diff --check origin/main...HEAD -> clean
  • git diff --no-ext-diff origin/main...HEAD | gitleaks stdin --no-banner --redact --exit-code 1 -> no leaks found

Assessment: focused #427 wallet hygiene fix; it removes retained private-key material from the transfer form after submit attempts without changing signing, transfer, nonce, payout, auth, or layout behavior.

Copy link
Copy Markdown

@weilixiong weilixiong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QUALITY_GATE: LOW risk ✅

2 files, +15/-0: adds finally { clearPrivateKeyField(form) } to setupTransfer() in wallet.js, ensuring private key is cleared even on submit error. New test verifies the finally/clear block exists in transfer function.

Verification: Both test_github_wallet_actions_clear_private_key_after_submit_attempt and test_transfer_action_clears_private_key_after_submit_attempt pass (1.17s, 1.22s).

Security-relevant fix (clearing sensitive key material on error path) — correct and minimal.

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.

3 participants