search

Dargon789 / safe-wallet-web

Safe{Wallet} – multisig EVM wallet
https://app.safe.global
GNU General Public License v3.0
1 stars 1 forks source link

Fix code scanning alert no. 1: Client-side URL redirect #10

Closed Dargon789 closed 2 weeks ago

Dargon789 commented 2 weeks ago

Fixes https://github.com/Dargon789/safe-wallet-web/security/code-scanning/1

To fix the problem, we need to ensure that the appUrl is validated against a list of authorized URLs before it is used. This can be achieved by maintaining a list of allowed URLs and checking if the appUrl is in this list before setting it as the src attribute of the iframe.

  1. Create a list of authorized URLs.
  2. Modify the useSafeAppUrl hook to validate the appUrl against this list.
  3. If the appUrl is not in the list, do not set it as the src attribute of the iframe.

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

github-actions[bot] commented 2 weeks ago

Branch preview

❌ Deploy failed!

github-actions[bot] commented 2 weeks ago

Coverage report

[!CAUTION] Test run failed

St.:grey_question:
 Category Percentage Covered / Total
🟡 Statements
72.98% (-0.51% 🔻)
 13402/18363
🔴 Branches
50.45% (-0.89% 🔻)
 3227/6397
🔴 Functions
56.12% (-0.87% 🔻)
 1930/3439
🟡 Lines
74.68% (-0.5% 🔻)
 12190/16322
Show files with reduced coverage 🔻
|
St.:grey_question:
| File | Statements | Branches | Functions | Lines | | :-: | :- | :- | :- | :- | :- | | 🔴 |
`...` / sessionSlice.ts
|
52.94% (-17.65% 🔻)
| 100% |
20% (-40% 🔻)
|
58.33% (-16.67% 🔻)
| | 🟢 |
`...` / safe-app.tsx
| 100% |
33.33% (-66.67% 🔻)
| 100% | 100% | | 🟢 |
`...` / useSafeAppUrl.ts
|
88.24% (-4.07% 🔻)
|
57.14% (-9.52% 🔻)
| 100% |
93.33% (-6.67% 🔻)
| | 🔴 |
`...` / index.tsx
|
45.71% (-37.14% 🔻)
|
0% (-80.95% 🔻)
|
0% (-40% 🔻)
|
48.48% (-39.39% 🔻)
| | 🟡 |
`...` / useSafeAppFromBackend.ts
|
68.42% (-21.05% 🔻)
|
20% (-20% 🔻)
| 100% |
66.67% (-22.22% 🔻)
| | 🔴 |
`...` / useSafeAppFromManifest.ts
|
38.89% (-55.56% 🔻)
|
0% (-100% 🔻)
|
0% (-100% 🔻)
|
46.67% (-46.67% 🔻)
| | 🔴 |
`...` / SafeAppDetails.tsx
|
58.33% (-41.67% 🔻)
|
0% (-100% 🔻)
|
0% (-100% 🔻)
|
58.33% (-41.67% 🔻)
| | 🟢 |
`...` / TryDemo.tsx
|
80% (-20% 🔻)
| 100% | 100% |
80% (-20% 🔻)
| | 🟡 |
`...` / constants.ts
|
50% (-50% 🔻)
| 100% | 100% |
66.67% (-33.33% 🔻)
| | 🔴 |
`...` / AppActions.tsx
|
28.33% (-60% 🔻)
|
0% (-89.47% 🔻)
|
0% (-85.71% 🔻)
|
30.36% (-58.93% 🔻)
| | 🔴 |
`...` / useLastSafe.ts
|
41.67% (-58.33% 🔻)
|
0% (-62.5% 🔻)
|
0% (-100% 🔻)
|
50% (-50% 🔻)
| | 🟡 |
`...` / index.tsx
|
63.64% (-9.09% 🔻)
| 0% | 0% | 70% | | 🔴 |
`...` / useOwnedSafes.ts
|
41.67% (-50% 🔻)
|
0% (-53.85% 🔻)
|
0% (-66.67% 🔻)
|
40.91% (-50% 🔻)
|

Test suite run failed

Failed tests: 10/1573. Failed suites: 3/208.
``` ● useApprovalInfos › returns an ApprovalInfo if the transaction contains an increaseAllowance call TypeError: unknown function (argument="fragment", value="increaseAllowance", code=INVALID_ARGUMENT, version=6.11.1) 50 | 51 | if (txPartial.data.startsWith(INCREASE_ALLOWANCE_SIGNATURE_HASH)) { > 52 | const [spender, amount] = ERC20_INTERFACE.decodeFunctionData('increaseAllowance', txPartial.data) | ^ 53 | return [ 54 | { 55 | amount, at makeError (node_modules/ethers/src.ts/utils/errors.ts:687:21) at assert (node_modules/ethers/src.ts/utils/errors.ts:715:25) at assertArgument (node_modules/ethers/src.ts/utils/errors.ts:727:5) at Interface.decodeFunctionData (node_modules/ethers/src.ts/abi/interface.ts:860:27) at Function.decodeFunctionData [as scanInnerTransaction] (src/services/security/modules/ApprovalModule/index.ts:52:49) at ApprovalModule.scanInnerTransaction [as scanTransaction] (src/services/security/modules/ApprovalModule/index.ts:126:44) at scanTransaction (src/components/tx/ApprovalEditor/hooks/useApprovalInfos.ts:39:37) at mountMemo (node_modules/react-dom/cjs/react-dom.development.js:16406:19) at Object.useMemo (node_modules/react-dom/cjs/react-dom.development.js:16851:16) at useMemo (node_modules/react/cjs/react.development.js:1650:21) at useApprovalInfos (src/components/tx/ApprovalEditor/hooks/useApprovalInfos.ts:37:28) at src/components/tx/ApprovalEditor/hooks/useApprovalInfos.test.ts:88:57 at TestComponent (node_modules/@testing-library/react/dist/pure.js:283:27) at renderWithHooks (node_modules/react-dom/cjs/react-dom.development.js:15486:18) at mountIndeterminateComponent (node_modules/react-dom/cjs/react-dom.development.js:20103:13) at beginWork (node_modules/react-dom/cjs/react-dom.development.js:21626:16) at beginWork$1 (node_modules/react-dom/cjs/react-dom.development.js:27465:14) at performUnitOfWork (node_modules/react-dom/cjs/react-dom.development.js:26599:12) at workLoopSync (node_modules/react-dom/cjs/react-dom.development.js:26505:5) at renderRootSync (node_modules/react-dom/cjs/react-dom.development.js:26473:7) at recoverFromConcurrentError (node_modules/react-dom/cjs/react-dom.development.js:25889:20) at performConcurrentWorkOnRoot (node_modules/react-dom/cjs/react-dom.development.js:25789:22) at flushActQueue (node_modules/react/cjs/react.development.js:2667:24) at act (node_modules/react/cjs/react.development.js:2582:11) at actWithWarning (node_modules/react-dom/cjs/react-dom-test-utils.development.js:1740:10) at node_modules/@testing-library/react/dist/act-compat.js:63:25 at renderRoot (node_modules/@testing-library/react/dist/pure.js:159:26) at render (node_modules/@testing-library/react/dist/pure.js:246:10) at renderHook (node_modules/@testing-library/react/dist/pure.js:293:7) at customRenderHook (src/tests/test-utils.tsx:85:20) at Object. (src/components/tx/ApprovalEditor/hooks/useApprovalInfos.test.ts:88:34) ● useApprovalInfos › returns multiple ApprovalInfos if the transaction is a multiSend containing an approve and increaseAllowance call TypeError: unknown function (argument="fragment", value="increaseAllowance", code=INVALID_ARGUMENT, version=6.11.1) 50 | 51 | if (txPartial.data.startsWith(INCREASE_ALLOWANCE_SIGNATURE_HASH)) { > 52 | const [spender, amount] = ERC20_INTERFACE.decodeFunctionData('increaseAllowance', txPartial.data) | ^ 53 | return [ 54 | { 55 | amount, at makeError (node_modules/ethers/src.ts/utils/errors.ts:687:21) at assert (node_modules/ethers/src.ts/utils/errors.ts:715:25) at assertArgument (node_modules/ethers/src.ts/utils/errors.ts:727:5) at Interface.decodeFunctionData (node_modules/ethers/src.ts/abi/interface.ts:860:27) at Function.decodeFunctionData [as scanInnerTransaction] (src/services/security/modules/ApprovalModule/index.ts:52:49) at scanInnerTransaction (src/services/security/modules/ApprovalModule/index.ts:124:76) at Array.flatMap () at ApprovalModule.flatMap [as scanTransaction] (src/services/security/modules/ApprovalModule/index.ts:124:38) at scanTransaction (src/components/tx/ApprovalEditor/hooks/useApprovalInfos.ts:39:37) at mountMemo (node_modules/react-dom/cjs/react-dom.development.js:16406:19) at Object.useMemo (node_modules/react-dom/cjs/react-dom.development.js:16851:16) at useMemo (node_modules/react/cjs/react.development.js:1650:21) at useApprovalInfos (src/components/tx/ApprovalEditor/hooks/useApprovalInfos.ts:37:28) at src/components/tx/ApprovalEditor/hooks/useApprovalInfos.test.ts:134:57 at TestComponent (node_modules/@testing-library/react/dist/pure.js:283:27) at renderWithHooks (node_modules/react-dom/cjs/react-dom.development.js:15486:18) at mountIndeterminateComponent (node_modules/react-dom/cjs/react-dom.development.js:20103:13) at beginWork (node_modules/react-dom/cjs/react-dom.development.js:21626:16) at beginWork$1 (node_modules/react-dom/cjs/react-dom.development.js:27465:14) at performUnitOfWork (node_modules/react-dom/cjs/react-dom.development.js:26599:12) at workLoopSync (node_modules/react-dom/cjs/react-dom.development.js:26505:5) at renderRootSync (node_modules/react-dom/cjs/react-dom.development.js:26473:7) at recoverFromConcurrentError (node_modules/react-dom/cjs/react-dom.development.js:25889:20) at performConcurrentWorkOnRoot (node_modules/react-dom/cjs/react-dom.development.js:25789:22) at flushActQueue (node_modules/react/cjs/react.development.js:2667:24) at act (node_modules/react/cjs/react.development.js:2582:11) at actWithWarning (node_modules/react-dom/cjs/react-dom-test-utils.development.js:1740:10) at node_modules/@testing-library/react/dist/act-compat.js:63:25 at renderRoot (node_modules/@testing-library/react/dist/pure.js:159:26) at render (node_modules/@testing-library/react/dist/pure.js:246:10) at renderHook (node_modules/@testing-library/react/dist/pure.js:293:7) at customRenderHook (src/tests/test-utils.tsx:85:20) at Object. (src/components/tx/ApprovalEditor/hooks/useApprovalInfos.test.ts:134:34) ● useApprovalInfos › returns multiple ApprovalInfos if the transaction is a multiSend containing 2 approvals and other transaction inbetween TypeError: unknown function (argument="fragment", value="increaseAllowance", code=INVALID_ARGUMENT, version=6.11.1) 50 | 51 | if (txPartial.data.startsWith(INCREASE_ALLOWANCE_SIGNATURE_HASH)) { > 52 | const [spender, amount] = ERC20_INTERFACE.decodeFunctionData('increaseAllowance', txPartial.data) | ^ 53 | return [ 54 | { 55 | amount, at makeError (node_modules/ethers/src.ts/utils/errors.ts:687:21) at assert (node_modules/ethers/src.ts/utils/errors.ts:715:25) at assertArgument (node_modules/ethers/src.ts/utils/errors.ts:727:5) at Interface.decodeFunctionData (node_modules/ethers/src.ts/abi/interface.ts:860:27) at Function.decodeFunctionData [as scanInnerTransaction] (src/services/security/modules/ApprovalModule/index.ts:52:49) at scanInnerTransaction (src/services/security/modules/ApprovalModule/index.ts:124:76) at Array.flatMap () at ApprovalModule.flatMap [as scanTransaction] (src/services/security/modules/ApprovalModule/index.ts:124:38) at scanTransaction (src/components/tx/ApprovalEditor/hooks/useApprovalInfos.ts:39:37) at mountMemo (node_modules/react-dom/cjs/react-dom.development.js:16406:19) at Object.useMemo (node_modules/react-dom/cjs/react-dom.development.js:16851:16) at useMemo (node_modules/react/cjs/react.development.js:1650:21) at useApprovalInfos (src/components/tx/ApprovalEditor/hooks/useApprovalInfos.ts:37:28) at src/components/tx/ApprovalEditor/hooks/useApprovalInfos.test.ts:209:57 at TestComponent (node_modules/@testing-library/react/dist/pure.js:283:27) at renderWithHooks (node_modules/react-dom/cjs/react-dom.development.js:15486:18) at mountIndeterminateComponent (node_modules/react-dom/cjs/react-dom.development.js:20103:13) at beginWork (node_modules/react-dom/cjs/react-dom.development.js:21626:16) at beginWork$1 (node_modules/react-dom/cjs/react-dom.development.js:27465:14) at performUnitOfWork (node_modules/react-dom/cjs/react-dom.development.js:26599:12) at workLoopSync (node_modules/react-dom/cjs/react-dom.development.js:26505:5) at renderRootSync (node_modules/react-dom/cjs/react-dom.development.js:26473:7) at recoverFromConcurrentError (node_modules/react-dom/cjs/react-dom.development.js:25889:20) at performConcurrentWorkOnRoot (node_modules/react-dom/cjs/react-dom.development.js:25789:22) at flushActQueue (node_modules/react/cjs/react.development.js:2667:24) at act (node_modules/react/cjs/react.development.js:2582:11) at actWithWarning (node_modules/react-dom/cjs/react-dom-test-utils.development.js:1740:10) at node_modules/@testing-library/react/dist/act-compat.js:63:25 at renderRoot (node_modules/@testing-library/react/dist/pure.js:159:26) at render (node_modules/@testing-library/react/dist/pure.js:246:10) at renderHook (node_modules/@testing-library/react/dist/pure.js:293:7) at customRenderHook (src/tests/test-utils.tsx:85:20) at Object. (src/components/tx/ApprovalEditor/hooks/useApprovalInfos.test.ts:209:34) ``` --- ``` ● ApprovalEditor › should modify approvals on save TypeError: unknown function (argument="fragment", value="increaseAllowance", code=INVALID_ARGUMENT, version=6.11.1) 156 | { 157 | to: tokenAddress, > 158 | data: ERC20_INTERFACE.encodeFunctionData('increaseAllowance', [spenderAddress, '690000000000']), | ^ 159 | value: '0', 160 | operation: OperationType.Call, 161 | }, at makeError (node_modules/ethers/src.ts/utils/errors.ts:687:21) at assert (node_modules/ethers/src.ts/utils/errors.ts:715:25) at assertArgument (node_modules/ethers/src.ts/utils/errors.ts:727:5) at Interface.encodeFunctionData (node_modules/ethers/src.ts/abi/interface.ts:878:27) at Object.encodeFunctionData (src/components/tx/ApprovalEditor/ApprovalEditor.test.tsx:158:35) ● ApprovalEditor › should modify increaseAllowance on save TypeError: unknown function (argument="fragment", value="increaseAllowance", code=INVALID_ARGUMENT, version=6.11.1) 264 | { 265 | to: tokenAddress, > 266 | data: ERC20_INTERFACE.encodeFunctionData('increaseAllowance', [spenderAddress, '690000000000']), | ^ 267 | value: '0', 268 | operation: OperationType.Call, 269 | }, at makeError (node_modules/ethers/src.ts/utils/errors.ts:687:21) at assert (node_modules/ethers/src.ts/utils/errors.ts:715:25) at assertArgument (node_modules/ethers/src.ts/utils/errors.ts:727:5) at Interface.encodeFunctionData (node_modules/ethers/src.ts/abi/interface.ts:878:27) at Object.encodeFunctionData (src/components/tx/ApprovalEditor/ApprovalEditor.test.tsx:266:35) ``` --- ``` ● Share Safe App Page › Should show the app name, description and URL expect(jest.fn()).toHaveBeenCalledWith(...expected) Expected: "https://apps-portal.safe.global/tx-builder", "1" Number of calls: 0 Ignored nodes: comments, script, style
94 | 95 | await waitFor(() => { > 96 | expect(fetchSafeAppFromManifestSpy).toHaveBeenCalledWith(TX_BUILDER, '1') | ^ 97 | expect(getSafeAppsSpy).toHaveBeenCalledWith('1', { url: TX_BUILDER }) 98 | 99 | expect(screen.getByText('Transaction Builder')).toBeInTheDocument() at toHaveBeenCalledWith (src/tests/pages/apps-share.test.tsx:96:43) at runWithExpensiveErrorDiagnosticsDisabled (node_modules/@testing-library/dom/dist/config.js:47:12) at checkCallback (node_modules/@testing-library/dom/dist/wait-for.js:127:77) at checkRealTimersCallback (node_modules/@testing-library/dom/dist/wait-for.js:121:16) at Timeout.task [as _onTimeout] (node_modules/jsdom/lib/jsdom/browser/Window.js:520:19) ● Share Safe App Page › Should suggest to connect a wallet when user hasn't connected one expect(jest.fn()).toHaveBeenCalledWith(...expected) Expected: "https://apps-portal.safe.global/tx-builder", "1" Number of calls: 0 Ignored nodes: comments, script, style
123 | 124 | await waitFor(() => { > 125 | expect(fetchSafeAppFromManifestSpy).toHaveBeenCalledWith(TX_BUILDER, '1') | ^ 126 | expect(getSafeAppsSpy).toHaveBeenCalledWith('1', { url: TX_BUILDER }) 127 | 128 | expect(screen.getByText('Connect wallet')).toBeInTheDocument() at toHaveBeenCalledWith (src/tests/pages/apps-share.test.tsx:125:43) at runWithExpensiveErrorDiagnosticsDisabled (node_modules/@testing-library/dom/dist/config.js:47:12) at checkCallback (node_modules/@testing-library/dom/dist/wait-for.js:127:77) at checkRealTimersCallback (node_modules/@testing-library/dom/dist/wait-for.js:121:16) at Timeout.task [as _onTimeout] (node_modules/jsdom/lib/jsdom/browser/Window.js:520:19) ● Share Safe App Page › Should show a link to the demo on mainnet expect(jest.fn()).toHaveBeenCalledWith(...expected) Expected: "https://apps-portal.safe.global/tx-builder", "1" Number of calls: 0 Ignored nodes: comments, script, style
148 | 149 | await waitFor(() => { > 150 | expect(fetchSafeAppFromManifestSpy).toHaveBeenCalledWith(TX_BUILDER, '1') | ^ 151 | expect(getSafeAppsSpy).toHaveBeenCalledWith('1', { url: TX_BUILDER }) 152 | 153 | expect(screen.getByText('Try demo')).toBeInTheDocument() at toHaveBeenCalledWith (src/tests/pages/apps-share.test.tsx:150:43) at runWithExpensiveErrorDiagnosticsDisabled (node_modules/@testing-library/dom/dist/config.js:47:12) at checkCallback (node_modules/@testing-library/dom/dist/wait-for.js:127:77) at checkRealTimersCallback (node_modules/@testing-library/dom/dist/wait-for.js:121:16) at Timeout.task [as _onTimeout] (node_modules/jsdom/lib/jsdom/browser/Window.js:520:19) ● Share Safe App Page › Should link to Safe Creation flow when the connected wallet has no owned Safes expect(jest.fn()).toHaveBeenCalledWith(...expected) Expected: "https://apps-portal.safe.global/tx-builder", "5" Number of calls: 0 Ignored nodes: comments, script, style
190 | 191 | await waitFor(() => { > 192 | expect(fetchSafeAppFromManifestSpy).toHaveBeenCalledWith(TX_BUILDER, '5') | ^ 193 | expect(getSafeAppsSpy).toHaveBeenCalledWith('5', { url: TX_BUILDER }) 194 | 195 | expect(screen.getByText('Create new Safe Account')).toBeInTheDocument() at toHaveBeenCalledWith (src/tests/pages/apps-share.test.tsx:192:43) at runWithExpensiveErrorDiagnosticsDisabled (node_modules/@testing-library/dom/dist/config.js:47:12) at checkCallback (node_modules/@testing-library/dom/dist/wait-for.js:127:77) at checkRealTimersCallback (node_modules/@testing-library/dom/dist/wait-for.js:121:16) at Timeout.task [as _onTimeout] (node_modules/jsdom/lib/jsdom/browser/Window.js:520:19) ● Share Safe App Page › Should show a select input with owned safes when the connected wallet owns Safes expect(jest.fn()).toHaveBeenCalledWith(...expected) Expected: "https://apps-portal.safe.global/tx-builder", "1" Number of calls: 0 Ignored nodes: comments, script, style
236 | 237 | await waitFor(() => { > 238 | expect(fetchSafeAppFromManifestSpy).toHaveBeenCalledWith(TX_BUILDER, '1') | ^ 239 | expect(getSafeAppsSpy).toHaveBeenCalledWith('1', { url: TX_BUILDER }) 240 | 241 | expect(screen.getByLabelText('Select a Safe Account')).toBeInTheDocument() at toHaveBeenCalledWith (src/tests/pages/apps-share.test.tsx:238:43) at runWithExpensiveErrorDiagnosticsDisabled (node_modules/@testing-library/dom/dist/config.js:47:12) at checkCallback (node_modules/@testing-library/dom/dist/wait-for.js:127:77) at checkRealTimersCallback (node_modules/@testing-library/dom/dist/wait-for.js:121:16) at Timeout.task [as _onTimeout] (node_modules/jsdom/lib/jsdom/browser/Window.js:520:19) ```

Report generated by 🧪jest coverage report action from 5c26db31635c7ce80d4020e1e44a5047170f18fc