Open sea security provider warning message #17662

filipsekulic · 2023-02-08T14:11:21Z

Explanation

Fixes Wallet Tx Security Provider: Warning Message #16258

Manual Testing Steps

Go to test-dapp
Select one of the options:

SEND LEGACY TRANSACTION
SEND EIP 1559 TRANSACTION
ETH SIGN
PERSONAL SIGN
SIGN TYPED DATA
SIGN TYPED DATA V3
SIGN TYPED DATA V4
SIGN IN WITH ETHEREUM
SIGN IN WITH ETHEREUM (W/ RESOURCES)
SIGN IN WITH ETHEREUM (BAD ACCOUNT)
SIGN IN WITH ETHEREUM (MALFORMED)

The warning message will appear depending on the input data sent to the OpenSea security provider.

github-actions · 2023-02-08T14:12:32Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

dzfjz · 2023-02-09T09:51:58Z

Verified by QA

jpuri · 2023-02-09T18:46:58Z

...rm-page-container/confirm-page-container-content/confirm-page-container-content.component.js

+          <SecurityProviderBannerMessage
+            securityProviderResponse={txData?.securityProviderResponse}
+          />
+        )}
        <ConfirmPageContainerSummary
          className={classnames({
            'confirm-page-container-summary--border':


Will be nice to have test coverage of the code change in this component.

Fixed here: 40ce40f

jpuri · 2023-02-09T18:47:29Z

Code changes look good to me, will be nice to have some test more coverage.

metamaskbot · 2023-02-13T09:21:12Z

Builds ready [186dd8b]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1450 ± 108 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	92	214	134	29	14
		domContentLoaded	1087	1944	1416	214	103
		load	1087	1945	1450	224	108
		domInteractive	1087	1944	1416	214	103

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 211 bytes
ui: 4104 bytes
common: 0 bytes

jpuri · 2023-02-14T18:51:17Z

ui/components/app/signature-request/signature-request.component.js

+            <SecurityProviderBannerMessage
+              securityProviderResponse={txData?.securityProviderResponse}
+            />
+          )}


From changes in snaps I am in doubt error message is showing up by default.

This will be true txData?.securityProviderResponse?.flagAsDangerous !== 0 even if txData?.securityProviderResponse?.flagAsDangerous is undefined.

Yeah, good catch! The securityProviderResponse was added into beforeEach and it affected all the tests including the ones that check matching the snapshot. It is fixed now and you can check the changes.

segun

few comments about constants

segun · 2023-02-14T19:12:21Z

ui/components/app/security-provider-banner-message/security-provider-banner-message.test.js

+
+  it('should render SecurityProviderBannerMessage component properly when flagAsDangerous is 1', () => {
+    const securityProviderResponse = {
+      flagAsDangerous: 1,


Can we use named constants here, 0, 1, 2 doesn't mean much and could be confusing maintaining this in the future.

segun · 2023-02-14T19:13:18Z

ui/components/app/signature-request-original/signature-request-original.test.js

+
+  it('should not render SecurityProviderBannerMessage component when flagAsDangerous is 0', () => {
+    props.txData.securityProviderResponse = {
+      flagAsDangerous: 0,


Same comment as above...we should use named constant here.

metamaskbot · 2023-02-17T16:34:37Z

Builds ready [c72bed0]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1598 ± 39 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	96	165	127	18	9
		domContentLoaded	1426	1717	1573	71	34
		load	1426	1812	1598	82	39
		domInteractive	1426	1717	1573	71	34

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 211 bytes
ui: 6152 bytes
common: 0 bytes

jpuri

LGTM

seaona · 2023-02-20T14:47:48Z

I've QAd the PR and overall looks good. I have some open questions which are not blockers but always good to clarify:

On SIWE signatures, we are sending the validate request but we don't display the Warning/Dangerous message. Is this expected?

siwe-warning2.mp4

When we receive a 500 response, (for example, if their server is down) the functionality simply stops working and we do not display any Warning/Malicious message. Is this expected?

opensea-500-response.mp4

If we receive a flagAsDangerous value different than 0, 1 or 2, we display the fallback Request not verified. Is this expected?

opensea-5.mp4

In the case we receive a response with flagAsDangerousvalue of 1, but with reason or reason_header empty, is it okay to display the empty banner?

opensea-empty-header-reason.mp4

cc @bschorchit

bschorchit · 2023-02-20T16:22:17Z

Thank you for your amazing 👀 here @seaona ! ❤️

@filipsekulic could we also display the warning for the SIWE signature requests? This is a new signature request that's being introduced in the next release (10.26). Thanks!
We should display the request not verified warning (screenshot below) if we receive a 500 response from the API. @filipsekulic could you update this?

Yes, this is the intended fallback warning for any error case which should also include if the answer different than 0, 1 and 2.
Oh nice catch! We didn't anticipated that. We should have a fallback copy for these case. @filipsekulic could you include the below fallback copy for when the request is flagged as malicious and either the reason_title or the reason parameter is empty? Thank you!
Fallback copy reason_title : Request flagged as malicious
Fallback copy for reason : The security provider has not shared additional details.

filipsekulic · 2023-02-21T13:27:24Z

Hey @bschorchit and @seaona!
I fixed the issues mentioned. Thank you both! 😊

Added.SIWE.mov

Status.code.500.mov

Missing.reason.or.reason.header.mov

metamaskbot · 2023-02-21T13:38:34Z

Builds ready [85db441]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1660 ± 62 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	96	188	123	20	10
		domContentLoaded	1364	1931	1619	118	57
		load	1364	1931	1660	130	62
		domInteractive	1364	1931	1619	118	57

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 211 bytes
ui: 7551 bytes
common: 0 bytes

codecov-commenter · 2023-02-21T13:39:54Z

Codecov Report

Merging #17662 (85db441) into develop (962a861) will increase coverage by 0.03%.
The diff coverage is 80.85%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##           develop   #17662      +/-   ##
===========================================
+ Coverage    62.23%   62.26%   +0.03%     
===========================================
  Files          902      904       +2     
  Lines        35139    35188      +49     
  Branches      8889     8921      +32     
===========================================
+ Hits         21868    21909      +41     
- Misses       13271    13279       +8

Impacted Files	Coverage Δ
...page-container/confirm-page-container.component.js	`63.64% <ø> (ø)`
...p/signature-request-siwe/signature-request-siwe.js	`2.17% <0.00%> (-0.33%)`	⬇️
app/scripts/lib/security-provider-helpers.js	`30.43% <50.00%> (+14.65%)`	⬆️
...banner-message/security-provider-banner-message.js	`90.00% <90.00%> (ø)`
...ontent/confirm-page-container-content.component.js	`80.77% <100.00%> (+2.51%)`	⬆️
...sage/security-provider-banner-message.constants.js	`100.00% <100.00%> (ø)`
...t-original/signature-request-original.component.js	`52.78% <100.00%> (+2.78%)`	⬆️
...p/signature-request/signature-request.component.js	`75.68% <100.00%> (+2.15%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

metamaskbot · 2023-02-21T15:49:40Z

Builds ready [f25e7c0]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1669 ± 90 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	97	141	118	14	7
		domContentLoaded	1339	2084	1613	157	76
		load	1340	2194	1669	187	90
		domInteractive	1339	2084	1613	157	76

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 211 bytes
ui: 7551 bytes
common: 0 bytes

seaona · 2023-02-21T15:59:24Z

thank you for the clarifications @bschorchit . I see all the issues fixed! Great work @filipsekulic !!

segun

LGTM

filipsekulic self-assigned this Feb 8, 2023

filipsekulic marked this pull request as ready for review February 9, 2023 12:52

filipsekulic requested a review from a team as a code owner February 9, 2023 12:52

filipsekulic requested a review from danjm February 9, 2023 12:52

jpuri reviewed Feb 9, 2023

View reviewed changes

filipsekulic force-pushed the open-sea-security-provider-warning-message branch from 0478bca to 40ce40f Compare February 10, 2023 15:47

filipsekulic requested a review from jpuri February 10, 2023 15:48

filipsekulic force-pushed the open-sea-security-provider-warning-message branch from 40ce40f to 186dd8b Compare February 13, 2023 08:55

jpuri reviewed Feb 14, 2023

View reviewed changes

segun reviewed Feb 14, 2023

View reviewed changes

filipsekulic added 5 commits February 17, 2023 14:58

Warning message for the OpenSea security provider

ead938d

Updated snapshots

507f1ea

Removed flask usage

d83157e

Covered more test cases

17b766f

Code refactor

7d86790

filipsekulic force-pushed the open-sea-security-provider-warning-message branch from 02c2f7d to 7d86790 Compare February 17, 2023 13:59

filipsekulic added 2 commits February 17, 2023 15:34

Fixed errors

c7caadc

Code refactor

c72bed0

jpuri previously approved these changes Feb 20, 2023

View reviewed changes

filipsekulic requested a review from segun February 20, 2023 14:22

Fixed few issues

85db441

filipsekulic dismissed jpuri’s stale review via 85db441 February 21, 2023 13:16

Covered more code with tests

f25e7c0

jpuri approved these changes Feb 21, 2023

View reviewed changes

segun approved these changes Feb 23, 2023

View reviewed changes

bschorchit merged commit 2acd51a into develop Feb 23, 2023

bschorchit deleted the open-sea-security-provider-warning-message branch February 23, 2023 11:38

github-actions bot locked and limited conversation to collaborators Feb 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Open sea security provider warning message #17662

Open sea security provider warning message #17662

filipsekulic commented Feb 8, 2023 •

edited

Loading

github-actions bot commented Feb 8, 2023

dzfjz commented Feb 9, 2023

jpuri Feb 9, 2023

filipsekulic Feb 10, 2023

jpuri commented Feb 9, 2023

metamaskbot commented Feb 13, 2023

jpuri Feb 14, 2023

filipsekulic Feb 17, 2023

segun left a comment

segun Feb 14, 2023

filipsekulic Feb 17, 2023

segun Feb 14, 2023

filipsekulic Feb 17, 2023

metamaskbot commented Feb 17, 2023

jpuri left a comment

seaona commented Feb 20, 2023

bschorchit commented Feb 20, 2023

filipsekulic commented Feb 21, 2023

metamaskbot commented Feb 21, 2023

codecov-commenter commented Feb 21, 2023

metamaskbot commented Feb 21, 2023

seaona commented Feb 21, 2023

segun left a comment

Open sea security provider warning message #17662

Open sea security provider warning message #17662

Conversation

filipsekulic commented Feb 8, 2023 • edited Loading

Explanation

Manual Testing Steps

github-actions bot commented Feb 8, 2023

dzfjz commented Feb 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpuri commented Feb 9, 2023

metamaskbot commented Feb 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

segun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metamaskbot commented Feb 17, 2023

jpuri left a comment

Choose a reason for hiding this comment

seaona commented Feb 20, 2023

bschorchit commented Feb 20, 2023

filipsekulic commented Feb 21, 2023

metamaskbot commented Feb 21, 2023

codecov-commenter commented Feb 21, 2023

Codecov Report

metamaskbot commented Feb 21, 2023

seaona commented Feb 21, 2023

segun left a comment

Choose a reason for hiding this comment

filipsekulic commented Feb 8, 2023 •

edited

Loading