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

Help to avoid sending over the mnemonic to nexus for search #656

Merged
merged 3 commits into from Jul 6, 2023

Conversation

csillag
Copy link
Contributor

@csillag csillag commented Jul 4, 2023

Up to now, we could only search for strings in a few very strictly specified format. (Ie. block number, tx hash, etc.) But now that we can do free full-text search for token names, it has become a bit of a security vulnerability that it's possible to accidentally copy and send sensitive information, like your wallet private key mnemonics, if you are working with the wallet and the explorer at the same time, and accidentally copy the wrong string to the wrong field.

Although would probably not be an everyday event, it's worth to add a simple check to warn about this, before doing it.

If it happens, this is what we do:

image

@github-actions
Copy link

github-actions bot commented Jul 4, 2023

Deployed to Cloudflare Pages

Latest commit: 8131714f982c1bcb1845ce9dfa75c526303c030e
Status:✅ Deploy successful!
Preview URL: https://b301e2ee.oasis-explorer.pages.dev

@csillag csillag force-pushed the csillag/dont-reveal-secrets branch 15 times, most recently from fabec01 to 8c05974 Compare July 4, 2023 16:41
@csillag csillag marked this pull request as ready for review July 4, 2023 16:44
@csillag csillag requested a review from lukaw3d July 4, 2023 16:44
@csillag csillag force-pushed the csillag/dont-reveal-secrets branch from 8c05974 to fcdd0a9 Compare July 4, 2023 16:50
@csillag csillag changed the title Help to avoid sending over the mnemonics for search Help to avoid sending over the mnemonic to nexus for search Jul 4, 2023
@nhynes
Copy link

nhynes commented Jul 4, 2023

Wait, why don't you instead detect the mnemonic and send it to your personal computer? You're just leaving money on the table!

@csillag
Copy link
Contributor Author

csillag commented Jul 4, 2023

Wait, why don't you instead detect the mnemonic and send it to your personal computer? You're just leaving money on the table!

That will come in a subsequent change, when the buzz around explorer has already faded... 😁

Copy link
Member

@lukaw3d lukaw3d left a comment

Choose a reason for hiding this comment

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

I don't think most of this is worth the maintenance burden. I definitely don't want TFunction inside routes :/ and sending slightly invalid mnemonic isn't safe either

@csillag
Copy link
Contributor Author

csillag commented Jul 4, 2023

I don't think most of this is worth the maintenance burden. I definitely don't want TFunction inside routes :/

If we are OK with having to say "I command thee" in English, independently of i18n, then we can get rid of most of the extra complexity, including the one impacting the routes.

sending slightly invalid mnemonic isn't safe either

My guees would be that this typically happens using copy-paste. Copy-paste doesnt usually introduce slight errors...

@csillag csillag force-pushed the csillag/dont-reveal-secrets branch from fcdd0a9 to 4805c18 Compare July 4, 2023 19:23
@csillag
Copy link
Contributor Author

csillag commented Jul 4, 2023

If we are OK with having to say "I command thee" in English, independently of i18n, then we can get rid of most of the extra complexity, including the one impacting the routes.

I simplified the implementation.

before after
image image

The routing code is no longer impacted in any way.

@csillag csillag requested a review from lukaw3d July 4, 2023 19:25
@csillag csillag force-pushed the csillag/dont-reveal-secrets branch from 4805c18 to 5f114ad Compare July 4, 2023 19:27
@csillag
Copy link
Contributor Author

csillag commented Jul 4, 2023

I don't think most of this is worth the maintenance burden.

@lukaw3d how about the new, simplified implementation? (With no TFunction creep)

@csillag
Copy link
Contributor Author

csillag commented Jul 4, 2023

Wait, I have another idea...

@csillag
Copy link
Contributor Author

csillag commented Jul 5, 2023

If a user is doing this (which arguably is stupid) it would even give us more reason to make the message shorter and clearer IMO. A stupid user definitely won't be reading this.

OK, rephreasing in idiocracy lingo.

src/app/components/Search/index.tsx Outdated Show resolved Hide resolved
src/app/components/Search/index.tsx Outdated Show resolved Hide resolved
@@ -364,6 +364,10 @@
},
"search": {
"placeholder": "Address, Block, Contract, Txn Hash, Transaction ID, Token name, etc",
"error": {
"tooShort": "Please enter at least 3 characters for searching",
"privacy": "This thing that you are trying to search for looks an awful lot like a mnemonic for an Oasis wallet. Please note that this is super-secret data that should never ever be shared with anyone; not even with such excellent services as our {{ appName }}. That being said, we are not here to tell you what you can or can not do with your own data, so if you insist, we WILL search for it. So, if you really think that there is a token with a name that contains this, then in order to signify that you understand and accept the terrible risk of sending this data to our servers, please insert this to the beginning of your search: '{{ wordsOfPower }}'! (Without quotation marks, of course.) If you do so, we will comply with your command."
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the message matters much; this is fine. Bit awkward for future translators

@csillag csillag force-pushed the csillag/dont-reveal-secrets branch from e0867d7 to e974b6b Compare July 5, 2023 15:50
@donouwens
Copy link
Collaborator

Yes but first I want to change the UX according to @donouwens's suggestion about the search suggestions.

I would suggest this for the 'too little characters' notification: https://www.figma.com/file/ifCrok8cP5ymEYjMa2PIi9/Block-Explorer?type=design&node-id=6624%3A246974&mode=design&t=Gk19GONKNbhNZF6X-1

@csillag csillag force-pushed the csillag/dont-reveal-secrets branch from c6687d2 to 48e4368 Compare July 5, 2023 17:00
@csillag
Copy link
Contributor Author

csillag commented Jul 5, 2023

Maybe we can create an integration for the second one that works in the same way as the search suggestions under the search bar (https://www.figma.com/file/ifCrok8cP5ymEYjMa2PIi9/Block-Explorer?type=design&node-id=6092%3A227275&mode=design&t=3K5SlcjhCb4mkoJv-1)?

Did that now:

image
image
image

Also, when using the Words of Power:

image
image
image

@csillag
Copy link
Contributor Author

csillag commented Jul 5, 2023

Continuing the discussion about the "too short" case in #671

@donouwens
Copy link
Collaborator

@csillag
Copy link
Contributor Author

csillag commented Jul 5, 2023

Could this design and text work @csillag?

Sure, but now that we have split this to two, let's finish and merge #671 first, and then I will return to this.

@csillag csillag force-pushed the csillag/dont-reveal-secrets branch from 48e4368 to 0597d1b Compare July 6, 2023 15:59
@csillag csillag force-pushed the csillag/dont-reveal-secrets branch 4 times, most recently from b5c8f34 to 127bced Compare July 6, 2023 16:08
@csillag
Copy link
Contributor Author

csillag commented Jul 6, 2023

@csillag csillag force-pushed the csillag/dont-reveal-secrets branch from 127bced to 5cce1b9 Compare July 6, 2023 16:12
@csillag
Copy link
Contributor Author

csillag commented Jul 6, 2023

@lukaw3d this has now been updated after merging the "too short" warning message separately.

@csillag csillag requested a review from lukaw3d July 6, 2023 16:13
@csillag csillag force-pushed the csillag/dont-reveal-secrets branch from 5cce1b9 to 8131714 Compare July 6, 2023 18:35
@csillag csillag enabled auto-merge July 6, 2023 18:36
@csillag csillag merged commit ddc4482 into master Jul 6, 2023
7 checks passed
@csillag csillag deleted the csillag/dont-reveal-secrets branch July 6, 2023 18:37
@donouwens
Copy link
Collaborator

@csillag This could work, but I feel we should stay closer to the new design. I’ve updated it a little here: https://www.figma.com/file/ifCrok8cP5ymEYjMa2PIi9/Block-Explorer?type=design&node-id=6673%3A248818&mode=design&t=NgiFzpta47LLPkRx-1

Also, the icon used is incorrect.

@csillag
Copy link
Contributor Author

csillag commented Jul 7, 2023

See follow up in #689

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

5 participants