Mix error messages to hide user balance #693
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ekzyis
force-pushed
the
691-balance-privacy-leak
branch
from
76316da to
bd500f3
Compare
|
Fix looks good as a stop gap. |
|
Maybe a longer term solution is to show the original message if the user is funding their own SN wallet. Not sure how difficult that is to do but it would be helpful. Maybe even just posting a help message on the wallet funding page stating the limit. That way it is more obvious to a user trying to fund their wallet. |
|
Yep agreed. We are already getting people reporting this error and being confused. |
Mhh, since we can tell from the session who is trying to fund which wallet, I think this should be doable. Good idea! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
One possible (temporary?) solution for #691 would be to not respond with a error message which tells that creating this invoice would exceed a limit.
By just responding with a different error message, the attacker cannot distinguish between "too many invoices" or "invoice would exceed limit". This isn't ideal but still a lot better imo.
I haven't tested this or checked if this would break something but if our code is good, we're not relying on error messages so this change should be fine.edit: Okay, I will test this, don't want to rely on @huumn superhuman skills to know exactly the impact of every line of code changed just by looking at it, lol
TODO:
Okay, pretty sure this doesn't break anything. I searched for all kind of substrings of the previous error message, and tested depositing and withdrawing. All good.