feat: don't non-null assert CSPs #8918
Conversation
There was a problem hiding this comment.
Runtime test looking good, tested app and dev env vars whilst serving the build locally.
These guys spooked me a bit, but they were both ephemeral and reproducible on develop.
Requesting changes to the type definitions as we have the opportunity to keep them strict and ensure the pattern you've implemented throughout this PR is enforced.
| @@ -4,6 +4,6 @@ export const csp: Csp = { | |||
| 'connect-src': [ | |||
| // [email protected]: https://github.com/FriendlyCaptcha/friendly-challenge/blob/f110634f17f316b90a9bd59b190291abe3c639bb/src/captcha.ts#L20 | |||
| 'https://api.friendlycaptcha.com/api/v1/puzzle', | |||
| process.env.REACT_APP_WALLET_MIGRATION_URL!, | |||
| process.env.REACT_APP_WALLET_MIGRATION_URL, | |||
There was a problem hiding this comment.
We missed our throw logic on this guy.
There was a problem hiding this comment.
diff --git a/react-app-rewired/headers/csps/wallets/walletMigration.ts b/react-app-rewired/headers/csps/wallets/walletMigration.ts
index 9dbdd615ff..ba6a53ddd4 100644
--- a/react-app-rewired/headers/csps/wallets/walletMigration.ts
+++ b/react-app-rewired/headers/csps/wallets/walletMigration.ts
@@ -1,9 +1,13 @@
import type { Csp } from '../../types'
+const REACT_APP_WALLET_MIGRATION_URL = process.env.REACT_APP_WALLET_MIGRATION_URL
+
+if (!REACT_APP_WALLET_MIGRATION_URL) throw new Error('REACT_APP_WALLET_MIGRATION_URL is required')
+
export const csp: Csp = {
'connect-src': [
// [email protected]: https://github.com/FriendlyCaptcha/friendly-challenge/blob/f110634f17f316b90a9bd59b190291abe3c639bb/src/captcha.ts#L20
'https://api.friendlycaptcha.com/api/v1/puzzle',
- process.env.REACT_APP_WALLET_MIGRATION_URL,
+ REACT_APP_WALLET_MIGRATION_URL,
],
}There was a problem hiding this comment.
Confirming runtime is happy with the below type changes once the diff above is applied 🙏
diff --git a/react-app-rewired/headers/types.ts b/react-app-rewired/headers/types.ts
index e92c32b013..d6eecf75d9 100644
--- a/react-app-rewired/headers/types.ts
+++ b/react-app-rewired/headers/types.ts
@@ -1,3 +1,3 @@
// process.env is not properly typed but could be if we were using vite imports.meta. wen vite?
-export type Csp = Record<string, (string | undefined)[]>
-export type CspEntry = (string | undefined)[]
+export type Csp = Record<string, string[]>
+export type CspEntry = string[]There was a problem hiding this comment.
All applied in b07311e @0xApotheosis, v. nice catch for even more safety!
react-app-rewired/headers/types.ts
Outdated
| export type Csp = Record<string, (string | undefined)[]> | ||
| export type CspEntry = (string | undefined)[] |
There was a problem hiding this comment.
Wait, didn't we just do all the work to confirm that the values in the list are defined?
| export type Csp = Record<string, (string | undefined)[]> | |
| export type CspEntry = (string | undefined)[] | |
| export type Csp = Record<string,(string[]> | |
| export type CspEntry = string[] |
react-app-rewired/headers/util.ts
Outdated
| if (k1 === undefined || k2 === undefined) return 0 | ||
| if (v1 === undefined || v2 === undefined) return 0 |
react-app-rewired/headers/util.ts
Outdated
| if (k1 < k2) return -1 | ||
| if (k1 > k2) return 1 | ||
| if (v1 < v2) return -1 | ||
| if (v1 > v2) return 1 | ||
| return 0 | ||
| }) | ||
| .reduce((a, [k, v]) => { | ||
| if (!k) return a |
|
@0xApotheosis FYI moving to draft until #8924 gets in, see https://github.com/shapeshift/web/pull/8924/files#r1970831176, bigly mergefix incoming |
Description
Continues on the work started in #8912 - only ~150 to go down from 300 when starting!
Issue (if applicable)
relates to #8909
Risk
Medium - this adds runtime checks for CSPs, ensure that both the dev build and actual build are not throwing on undefined CSPs
Testing
Engineering
Operations
Screenshots (if applicable)
dev build: https://jam.dev/c/a0a1135e-e52d-4e0d-902e-20862185cab7
built build: