Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backend/lightning: don't expose mnemonic words for lightning #2741

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

strmci
Copy link
Collaborator

@strmci strmci commented Jun 4, 2024

This commit ensures the mnemonic words created
for lightning wallet are not exposed to frontend.

@strmci strmci force-pushed the lightning_mnemonic_update branch 2 times, most recently from 80c038f to dc8913a Compare June 4, 2024 16:16
@@ -23,7 +23,7 @@ import (
// LightningAccountConfig is the configuration of a single Lightning account.
type LightningAccountConfig struct {
// Mnemonic is the wallet node generated from the device entropy.
Mnemonic string `json:"mnemonic"`
Mnemonic string `json:"-"`
Copy link
Contributor

Choose a reason for hiding this comment

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

With this you delete the mnemonic also from the config, and the user can't spend anymore 😰 You even changed the test above to reflect this 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm, I'll look into it, thanks.

@strmci strmci force-pushed the lightning_mnemonic_update branch 4 times, most recently from 0e2deee to 97a77d6 Compare July 22, 2024 13:42
@strmci strmci requested a review from Beerosagos July 22, 2024 13:55
Copy link
Collaborator

@Beerosagos Beerosagos left a comment

Choose a reason for hiding this comment

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

Plase, also remove the mnemonic from the frontend object :)

@strmci strmci force-pushed the lightning_mnemonic_update branch from 97a77d6 to 480aa66 Compare July 22, 2024 15:33
@strmci
Copy link
Collaborator Author

strmci commented Jul 22, 2024

Plase, also remove the mnemonic from the frontend object :)

Removed.

Copy link
Collaborator

@Beerosagos Beerosagos left a comment

Choose a reason for hiding this comment

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

utACK

return handlers.backend.Config().LightningConfig()
lightningConfig := handlers.backend.Config().LightningConfig()
for _, account := range lightningConfig.Accounts {
account.Mnemonic = ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why set to empty string? Is there a way to remove the mnemonic field?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's now removed by adding omitempty to the json descriptor (thanks @Beerosagos)

Copy link
Contributor

Choose a reason for hiding this comment

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

This still removes the mnemonic from the actual lightning account object in the config, so it will be gone afterwards, and also persisted on the next config save() I think.

To do this properly, you'd have to change the handler that returns the data to the frontend, probably recreating a structure with the relevant data for the frontend without the mnemonic.

Copy link
Collaborator

@Beerosagos Beerosagos Jul 23, 2024

Choose a reason for hiding this comment

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

Dang, @benma is right. I suggested @strmci this solution. Didn't realize there was a pointer to the LightningAccountConfig, I thought it would have been a deep copy. Sorry for not noticing it 🙏

@strmci strmci force-pushed the lightning_mnemonic_update branch 2 times, most recently from 78694e3 to ed76102 Compare July 24, 2024 10:52
@strmci
Copy link
Collaborator Author

strmci commented Jul 24, 2024

@Beerosagos @benma
I recreated the lightning config without a mnemonic in the handler, PTAL

@strmci strmci force-pushed the lightning_mnemonic_update branch from ed76102 to 13da20e Compare July 24, 2024 11:07
Copy link
Contributor

@benma benma left a comment

Choose a reason for hiding this comment

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

utACK

This commit ensures the mnemonic words created
for lightning wallet are not exposed to frontend.
@strmci strmci force-pushed the lightning_mnemonic_update branch from 13da20e to 04c56d0 Compare July 30, 2024 07:04
@strmci strmci merged commit 03f607f into BitBoxSwiss:staging-ln Jul 30, 2024
3 of 5 checks passed
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.

None yet

4 participants