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

Fix SSSS overwrite if user never enters their SSSS key #4087

Closed
wants to merge 2 commits into from

Conversation

foldleft
Copy link
Contributor

@foldleft foldleft commented Feb 18, 2020

fixes: element-hq/element-web#12352

This bug caused the cross-signing panel to indicate that there were no cross-signing keys available if the user did not provide any keys, even though cross-signing is set up. This allowed the user to re-initialize the cross-signing store, destroying whatever was there previously.

Before giving keys to the client
Screenshot 2020-02-21 at 10 12 57

After giving keys to the client
Screenshot 2020-02-21 at 10 13 21

@foldleft foldleft requested a review from a team February 18, 2020 17:30
@turt2live
Copy link
Member

@foldleft can this PR get a title that makes sense in a changelog please?

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

sorry, I don't understand the problem here at all - can we also get a description of what is wrong and why we don't need to check whether keys are on the device please?

@foldleft foldleft changed the title fix ssss overwrite fix ssss overwrite if user never enters their ssss key Feb 19, 2020
@@ -128,7 +128,6 @@ export default class CrossSigningPanel extends React.PureComponent {
}

const enabled = (
crossSigningPublicKeysOnDevice &&
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this going to cause the summarisedStatus status to say it's enabled when it isn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

weirdly, crossSigningPublicKeysOnDevice seems to only be true if the private keys are also on the device. Even if the device doesn't have keys, the cross-signing can be initialised.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, crossSigningPublicKeysOnDevice is whether the public key is trusted or not. My point was that in the if block just below, we'll now only hit the second case if crossSigningPrivateKeysInStorage is true but secretStorageKeyInAccount is false. So this corrects the second if block but breaks the first?

@foldleft foldleft requested a review from dbkr February 19, 2020 10:13
@jryans
Copy link
Collaborator

jryans commented Feb 19, 2020

@foldleft Please also use title case on PRs for the changelog.

@foldleft foldleft changed the title fix ssss overwrite if user never enters their ssss key Fix SSSS overwrite if user never enters their ssss key Feb 19, 2020
@foldleft foldleft changed the title Fix SSSS overwrite if user never enters their ssss key Fix SSSS overwrite if user never enters their SSSS key Feb 19, 2020
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

seems fine from a code perspective, but it sounds like some copy/design will need to be done as part of this?

I'm relying on Dave's review here.

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.

possible bootstrap overwrite of ssss
4 participants