feat: restrict safe pass safe app access for ofac blocked addresses #4066

schmanu · 2024-08-13T16:07:07Z

What this PR changes

Reuses ofac detection for Safe{Pass} Safe app
Reject WalletConnect request from Safe{Pass} for sanctioned addresses.

How to test it

Impersonate a blocked address
- Try to open the Safe{Pass} app (community.safe.global)
- Try to connect via WalletConnect to community.safe.global

Screenshots

Open Task

Checklist

I've tested the branch on mobile 📱
I've documented how it affects the analytics (if at all) 📊
I've written a unit/e2e test for it (if applicable) 🧑‍💻

github-actions · 2024-08-13T16:07:26Z

Branch preview

✅ Deploy successful!

Website:
https://feat_safe_pass_ofac_check--walletweb.review.5afe.dev/home?safe=eth:0xA77DE01e157f9f57C7c4A326eeE9C4874D0598b6

Storybook:
https://feat_safe_pass_ofac_check--walletweb.review.5afe.dev/storybook/

github-actions · 2024-08-13T16:08:17Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

github-actions · 2024-08-13T16:10:53Z

📦 Next.js Bundle Analysis for safe-wallet-web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page	Size (compressed)
`global`	`961.24 KB` (🟡 +12 B)

Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Five Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page	Size (compressed)	First Load
`/apps/open`	`55.18 KB` (🟡 +1.99 KB)	1016.42 KB
`/balances`	`30.94 KB` (🟡 +5 B)	992.18 KB
`/home`	`58.24 KB` (🟡 +41 B)	1019.48 KB
`/stake`	`597 B` (🟢 -5 B)	961.82 KB
`/swap`	`733 B` (🟡 +1 B)	961.96 KB

Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

github-actions · 2024-08-13T16:13:19Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	74.52% (+0.05% 🔼)	12940/17365
🔴	Branches	52.84% (+0.01% 🔼)	3118/5901
🔴	Functions	58.73% (+0.05% 🔼)	1904/3242
🟡	Lines	76.23% (+0.05% 🔼)	11756/15421

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / useSanctionedAddress.ts	100%	100%	100%	100%
🔴	`...` / index.tsx	46.67%	0%	0%	53.85%
🟢	`...` / index.tsx	62.5%	0%	0%	83.33%

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟡	`...` / index.tsx	76.25% (+2.28% 🔼)	55.26% (-5.34% 🔻)	40%	76.32% (+2.4% 🔼)

Test suite run success

1508 tests passing in 205 suites.

Report generated by 🧪jest coverage report action from de491bf

notion-workspace · 2024-08-13T16:32:48Z

OFAC compliance for EOAs and countries

usame-algan · 2024-08-14T10:20:54Z

src/hooks/useSanctionedAddress.ts

+  if (isSafeSanctioned) {
+    return safeAddress
+  }
+  if (isWalletSanctioned) {
+    return wallet?.address
+  }


data contains the error message in case the useGetIsSanctionedQuery throws so I think it would falsely say that an address is sanctioned if the network request fails.

I think that is not true. The error seems to be transformed and returned in the error field of useGetIsSanctionedQuery

It is expected to either return a data or error field.
https://redux-toolkit.js.org/rtk-query/usage/customizing-queries#implementing-a-queryfn

Should we remove the data assignment on error then inside the ofacApi query?

return { error: { status: 'CUSTOM_ERROR', data: (error as Error).message } }

github-actions · 2024-08-15T12:18:14Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

src/components/safe-apps/AppFrame/index.tsx

src/features/walletconnect/components/WcProposalForm/index.tsx

katspaugh · 2024-08-19T14:10:34Z

src/features/walletconnect/services/utils.ts

@@ -50,6 +50,10 @@ export const isBlockedBridge = (origin: string) => {
  return BlockedBridges.some((bridge) => origin.includes(bridge))
 }

+export const isSafePassApp = (origin: string) => {
+  return origin.includes('community.safe.global')


Can we if not avoid hardcode then at least make it a constant?

Can we use the constant SAFE_PASS_URL here?

katspaugh · 2024-08-19T14:13:31Z

I think checking blocked addresses from within apps/open and WalletConnect is a bit excessive. It would be better if we just hide all links to this app, and inside the app itself check for blocked addresses (would have to port that hook there).

schmanu · 2024-08-19T14:15:23Z

I think checking blocked addresses from within apps/open and WalletConnect is a bit excessive. It would be better if we just hide all links to this app, and inside the app itself check for blocked addresses (would have to port that hook there).

You cannot receive the connected wallet within the Safe app. We only get the safeAddress. That's why the check is within Safe{Wallet}.

katspaugh · 2024-08-23T06:38:05Z

Geoblocking is now in place, so it would be good to hide the header widget if the app cannot be loaded.

github-actions · 2024-09-17T15:04:21Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

usame-algan · 2024-10-09T11:41:52Z

Geoblocking is now in place, so it would be good to hide the header widget if the app cannot be loaded.

Should we do this in the scope of this PR? @schmanu

schmanu · 2024-10-14T11:16:05Z

Geoblocking is now in place, so it would be good to hide the header widget if the app cannot be loaded.

Should we do this in the scope of this PR? @schmanu

This was already done separately: #4148

feat: restrict safe pass safe app access for ofac blocked addresses

4dd18e4

feat: block wc requests for sanctioned addresses if from Safe{Pass}

05ac797

schmanu requested a review from usame-algan August 13, 2024 16:31

usame-algan reviewed Aug 14, 2024

View reviewed changes

schmanu requested a review from usame-algan August 15, 2024 12:00

fix: do not show SafePass widgets if sanctioned

4a9818d

fix: return error directly

5480f2f

schmanu requested a review from katspaugh August 19, 2024 13:57

katspaugh reviewed Aug 19, 2024

View reviewed changes

schmanu added 2 commits August 19, 2024 16:37

fix: pass token to skip sanctions check

75a926c

fix: reuse isSafePassApp util

da03cde

Merge remote-tracking branch 'origin/dev' into feat/safe-pass-ofac-check

c3a6485

schmanu requested a review from katspaugh September 17, 2024 15:03

usame-algan approved these changes Oct 9, 2024

View reviewed changes

Merge remote-tracking branch 'origin/dev' into feat/safe-pass-ofac-check

de491bf

schmanu merged commit 731bfe1 into dev Oct 14, 2024
15 checks passed

schmanu deleted the feat/safe-pass-ofac-check branch October 14, 2024 11:16

github-actions bot locked and limited conversation to collaborators Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: restrict safe pass safe app access for ofac blocked addresses #4066

feat: restrict safe pass safe app access for ofac blocked addresses #4066

schmanu commented Aug 13, 2024 •

edited

Loading

github-actions bot commented Aug 13, 2024 •

edited

Loading

github-actions bot commented Aug 13, 2024 •

edited

Loading

github-actions bot commented Aug 13, 2024 •

edited

Loading

github-actions bot commented Aug 13, 2024 •

edited

Loading

notion-workspace bot commented Aug 13, 2024

usame-algan Aug 14, 2024

schmanu Aug 15, 2024

schmanu Aug 15, 2024

usame-algan Aug 15, 2024

github-actions bot commented Aug 15, 2024 •

edited

Loading

katspaugh Aug 19, 2024

usame-algan Oct 9, 2024

katspaugh commented Aug 19, 2024

schmanu commented Aug 19, 2024

katspaugh commented Aug 23, 2024

github-actions bot commented Sep 17, 2024

usame-algan commented Oct 9, 2024

schmanu commented Oct 14, 2024

feat: restrict safe pass safe app access for ofac blocked addresses #4066

feat: restrict safe pass safe app access for ofac blocked addresses #4066

Conversation

schmanu commented Aug 13, 2024 • edited Loading

What this PR changes

How to test it

Screenshots

Open Task

Checklist

github-actions bot commented Aug 13, 2024 • edited Loading

Branch preview

github-actions bot commented Aug 13, 2024 • edited Loading

ESLint Summary View Full Report

github-actions bot commented Aug 13, 2024 • edited Loading

📦 Next.js Bundle Analysis for safe-wallet-web

⚠️ Global Bundle Size Increased

Five Pages Changed Size

github-actions bot commented Aug 13, 2024 • edited Loading

Coverage report

Test suite run success

notion-workspace bot commented Aug 13, 2024

usame-algan Aug 14, 2024

Choose a reason for hiding this comment

schmanu Aug 15, 2024

Choose a reason for hiding this comment

schmanu Aug 15, 2024

Choose a reason for hiding this comment

usame-algan Aug 15, 2024

Choose a reason for hiding this comment

github-actions bot commented Aug 15, 2024 • edited Loading

ESLint Summary View Full Report

katspaugh Aug 19, 2024

Choose a reason for hiding this comment

usame-algan Oct 9, 2024

Choose a reason for hiding this comment

katspaugh commented Aug 19, 2024

schmanu commented Aug 19, 2024

katspaugh commented Aug 23, 2024

github-actions bot commented Sep 17, 2024

ESLint Summary View Full Report

usame-algan commented Oct 9, 2024

schmanu commented Oct 14, 2024

schmanu commented Aug 13, 2024 •

edited

Loading

github-actions bot commented Aug 13, 2024 •

edited

Loading

github-actions bot commented Aug 13, 2024 •

edited

Loading

github-actions bot commented Aug 13, 2024 •

edited

Loading

github-actions bot commented Aug 13, 2024 •

edited

Loading

github-actions bot commented Aug 15, 2024 •

edited

Loading