Skip to content

fix(cli): default include_keys to False in soul export (#273)#274

Open
sshekhar563 wants to merge 2 commits into
qbtrix:devfrom
sshekhar563:fix/include-keys-default
Open

fix(cli): default include_keys to False in soul export (#273)#274
sshekhar563 wants to merge 2 commits into
qbtrix:devfrom
sshekhar563:fix/include-keys-default

Conversation

@sshekhar563

Copy link
Copy Markdown
Collaborator

What

Fixes #273

The soul export command was passing include_keys=True by default, which included the private Ed25519 signing key in every exported .soul file.

Changes

  • Added --include-keys flag to soul export (default: False)
  • Private keys are only included when user explicitly passes --include-keys
  • Help text warns: "Only use when migrating between your own devices"

Usage

# Safe default — no private keys in exported file
soul export aria.soul

# Explicit opt-in for device migration
soul export aria.soul --include-keys

@prakashUXtech prakashUXtech left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch on this one. Exporting the private signing key by default was a real leak, and the fix is right. I went through all the include_keys=True call sites on dev to check nothing else was affected, and the flip is complete: soul export was the only share-intended path that leaked the key, and the in-place saves (remember, evolve, and the rest) correctly keep it. --include-keys still works for migration. Clean change.

Two things before merge, both packaging rather than substance:

1. A test. Since this is a security default, it's worth locking in. A CliRunner test asserting the default export zip contains keys/public.key but not keys/private.key, and that --include-keys puts the private key back. tests/test_cli/ is the spot.

2. Docs. docs/cli-reference.md under soul export doesn't list --include-keys, and the "Portable transfer between machines" line now needs the flag to carry signing power. Worth updating both here.

One suggestion, non-blocking: the migrate-then-delete workflow now silently drops signing power, and with the quiet verification-only mode it's easy to miss until signing stops working. A one-line console notice on default export ("private key not included; pass --include-keys to migrate signing power") would save someone a confusing afternoon.

Also a stray extra blank line snuck in around main.py:705. Drop it while you're in there.

Thanks for the threat write-up on the issue too. Made this easy to verify.

Add --include-keys flag to the export command. Private signing keys are no longer included by default when exporting .soul files, preventing accidental key leaks when sharing files. Users must explicitly pass --include-keys when migrating between their own devices.
- Add 6 tests for export key behavior (tests/test_cli/test_export_keys.py)

- Default export has public.key but NOT private.key

- --include-keys adds private.key back

- Console notice on default export: 'private key not included'

- Update docs/cli-reference.md with --include-keys option

- Remove stray blank line at main.py:707
@sshekhar563 sshekhar563 force-pushed the fix/include-keys-default branch from 55865de to 98d2df4 Compare July 3, 2026 16:02
@sshekhar563 sshekhar563 requested a review from prakashUXtech July 3, 2026 16:11
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