-
Notifications
You must be signed in to change notification settings - Fork 909
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 issue with public keys when redeeming tokens #1907
Conversation
payment_credential.SetKey("credential", base::Value(std::move(credential))); | ||
|
||
payment_credential.SetKey("publicKey", base::Value(wallet_info.public_key)); | ||
payment_credential.SetKey("publicKey", base::Value(token_info.public_key)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to understand if the current behavior (before this patch) is a privacy issue; probably not because at this stage in redemption, the server has to see the wallet public key anyway so it can do the payment?
If the wallet public key were accidentally sent in earlier stages, that would be a privacy problem since it's a persistent identifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diracdeltas apologies I did not tag you earlier as got pulled onto another issue. The wallet public key was sent. Should we arrange a call with Jimmy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diracdeltas: @amirsaber is going to fix the database once the fix has gone live
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, I would just like to understand the scope of the bug. If the wallet public key was sent in Step 1 of https://docs.google.com/document/d/1gqe_RISTHu3VxfzpMKds__X73yO3aIgdeB5fBvp8da0/edit#heading=h.2hb43kpd1ag3, then there would be a privacy issue I think. If it was sent in Step 4, there would be no problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it was sent in Step 1, then all records of those requests which include the wallet address should be deleted. We should also think about what to do for clients which stay on the unfixed version (and thus will keep sending their wallet address for a while).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diracdeltas I can confirm it was sent in step 4. With regards to clients who stay on the unfixed version which we plan to patch before it goes to beta, Amir and Jimmy have a plan in place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
fixes brave/brave-browser#3655
Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests
) onnpm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Confirm that viewed Ads are cashed out on Ads Serve. You can pass the command-line argument
--rewards=debug=true
to change the redemption period from ~7 days to ~25 minutes, once your ad views have been redeemed @jimmy @michael.mclaughlin are able to confirm the payments were credited to the users rewardsReviewer Checklist: