Harden CSP and add password strength meter#32
Conversation
Use estimator-based password validation so short-but-strong generated passwords can pass while common weak passwords are rejected, and remove wildcard HTTPS connect-src from the key-custody SPA CSP. Made-with: Cursor
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0547395586
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "'self'", | ||
| 'https:', | ||
| ...splitCspSources(process.env.SYSNODE_CSP_CONNECT_SRC), | ||
| // Key-custody pages must not allow arbitrary HTTPS exfiltration. Keep | ||
| // production same-origin by default; deployments that truly need another | ||
| // endpoint can add exact origins via SYSNODE_CSP_CONNECT_SRC. | ||
| ...explicitConnectSources(process.env.SYSNODE_CSP_CONNECT_SRC), |
There was a problem hiding this comment.
connect-src now resolves to 'self' plus optional SYSNODE_CSP_CONNECT_SRC, but the app’s anonymous client still defaults production traffic to https://syscoin.dev (see src/lib/api.js), so default builds with no extra CSP env will have browser-blocked fetch calls for network/governance data. This regresses the out-of-the-box production path unless deployers add a second, synchronized env var; the policy should include the default API origin (or derive allowed origins from the API base setting) to avoid breaking those pages.
Useful? React with 👍 / 👎.
| return (Array.isArray(userInputs) ? userInputs : []) | ||
| .map((value) => String(value || '').trim()) | ||
| .filter(Boolean); |
There was a problem hiding this comment.
normalizeUserInputs forwards each provided value as a single dictionary entry, and call sites pass only the full email string, so passwords containing just the local-part (or other email fragments) are not penalized by the user-input dictionary as intended. In practice this weakens the new “personal info” hardening for values like alice2026! with alice@example.com; split/expand user inputs into meaningful tokens before calling zxcvbn.
Useful? React with 👍 / 👎.
Preserve the exact default public API origin in connect-src and expand user identity inputs into useful zxcvbn dictionary tokens. Made-with: Cursor
|
@codex review Addressed the previous inline findings:
Note: no forward DB migration is needed for this app before production; schema changes on this pre-production line should be reviewed as direct initial-schema changes when applicable. |
|
@codex review Addressed the previous inline findings:
Note: no forward DB migration is needed for this app before production; schema changes on this pre-production line should be reviewed as direct initial-schema changes when applicable. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 237c186336
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const defaultPublicApiBase = | ||
| process.env.REACT_APP_API_BASE || | ||
| (process.env.NODE_ENV === 'production' |
There was a problem hiding this comment.
defaultPublicApiBase is derived from process.env.REACT_APP_API_BASE at server runtime, but the frontend API base in src/lib/api.js is baked in at build time; when a build is produced with REACT_APP_API_BASE set (documented workflow) and deployed without the same runtime env, CSP falls back to https://syscoin.dev while the bundle still calls the custom origin, so connect-src blocks all anonymous API requests in production. Please derive the allowlisted origin from the build-time value (or otherwise guarantee both values are emitted together) so the policy cannot drift.
Useful? React with 👍 / 👎.
Avoid CSP drift from build-time API overrides by making production clients use relative same-origin paths and limiting explicit API base overrides to non-production builds. Made-with: Cursor
|
@codex review Addressed the latest P1 by removing production build/runtime API-base drift entirely:
The previous zxcvbn tokenization fix is still included. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 942421a192
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -38,13 +38,15 @@ https://sysnode.info/vault/* -> backend | |||
| https://sysnode.info/gov/* -> backend | |||
There was a problem hiding this comment.
The same-origin deployment map now lists only /auth/*, /vault/*, and /gov/*, but this commit also changed production anonymous calls to same-origin '/mnStats', '/mnCount', and '/govlist' (src/lib/api.js). If operators follow this README block literally, those anonymous endpoints are not proxied to the backend, so public dashboard data requests will hit the SPA server (GETs return index.html, POST /govlist fails), breaking core data views in production.
Useful? React with 👍 / 👎.
| For local or non-production testing, you can override the API base URL at build time: | ||
|
|
||
| ```bash | ||
| REACT_APP_API_BASE=https://your-backend.example npm run build |
There was a problem hiding this comment.
The example REACT_APP_API_BASE=... npm run build is inconsistent with the new runtime logic: both clients now ignore REACT_APP_API_BASE when NODE_ENV === 'production' (src/lib/apiClient.js and src/lib/api.js). That means readers who use this documented command won’t get the override they expect, which is likely to cause confusing misconfiguration during local/staging tests.
Useful? React with 👍 / 👎.
Document the anonymous API routes required by production same-origin deployments and use a dev-server example for non-production API base overrides. Made-with: Cursor
|
@codex review Addressed the latest README findings:
|
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
connect-srcdefaults to same-origin only and filters wildcard HTTP(S)/WS sources fromSYSNODE_CSP_CONNECT_SRC.Test plan
Notes
npm run buildstill fails with the existing localUnexpected end of JSON inputissue seen on prior branches; CI build remains the authoritative build check.Made with Cursor