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

Address security concern dapps #13398

Merged
merged 3 commits into from
May 20, 2022
Merged

Conversation

qamarngr
Copy link
Contributor

@qamarngr qamarngr commented May 19, 2022

Resolves brave/brave-browser#21798
Resolves brave/brave-browser#22913

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Same as in PR

@qamarngr qamarngr requested a review from a team as a code owner May 19, 2022 21:34
@qamarngr qamarngr added this to the 1.41.x - Nightly milestone May 19, 2022
@Pavneet-Sing
Copy link

Pavneet-Sing commented May 20, 2022

Try to use domain layer as much as possible to avoid future refactoring.
BTW, the iOS CI is failing, seems like some tests are failing, Cross check with @SergeyZhukovsky or iOS team.
[Update]: IOS CI issues is in progress.

@Pavneet-Sing
Copy link

While you are on signature panel, Could you also please add some bottom padding to the description and close the brave/brave-browser#22913 ticket, TIA.

@qamarngr
Copy link
Contributor Author

Try to use domain layer as much as possible to avoid future refactoring. BTW, the iOS CI is failing, seems like some tests are failing, Cross check with @SergeyZhukovsky or iOS team. [Update]: IOS CI issues is in progress.

I'll start using domain layer from my next ticket, I just wanted to develop little more understanding around it.

@SergeyZhukovsky
Copy link
Member

++ but please remove unused
private BraveWalletService mBraveWalletService;
before merge

@qamarngr
Copy link
Contributor Author

iOS CI failed because of a known problem not related to that PR

@qamarngr qamarngr merged commit cfa5655 into master May 20, 2022
@qamarngr qamarngr deleted the address-security-concern-dapps branch May 20, 2022 16:41
@qamarngr qamarngr restored the address-security-concern-dapps branch May 20, 2022 18:23
brave-builds pushed a commit that referenced this pull request May 20, 2022
@srirambv
Copy link
Contributor

Verification passed on Oppo Reno 5 with Android 12 running 1.41.18 custom Dapp support build

  • Verified steps from #12701
  • Verified eTLD+1 is shown in bold on panel while transacting with a Dapp
image image image image image image

A DApp is requesting your public encryption key
</message>
<message name="IDS_BRAVE_WALLET_PROVIDE_ENCRYPTION_KEY_DESCRIPTION" desc="Provide encryption key panel details">
is requesting your wallets public encryption key. If you consent to providing this key, the site will be able to compose encrypted messages to you.
Copy link
Collaborator

Choose a reason for hiding this comment

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

wallet's?

Copy link
Member

Choose a reason for hiding this comment

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

yes @qamarngr could you pls fix and uplift?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same in IDS_BRAVE_WALLET_PROVIDE_ENCRYPTION_KEY_TITLE in components/resources/wallet_strings.grdp

Copy link
Member

@bbondy bbondy Jun 22, 2022

Choose a reason for hiding this comment

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

It also looks like this string isn't using a proper variable for the origin in question. Languages can't be translated 1:1 via concatenation like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bbondy will fix it this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mkarolin What should be fixed in IDS_BRAVE_WALLET_PROVIDE_ENCRYPTION_KEY_TITLE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants