Implement state screen #22
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stevenconner
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
stevenconner
changed the title
SeanBarker182
approved these changes
Sep 23, 2025
Contributor
SeanBarker182
left a comment
SeanBarker182
left a comment
There was a problem hiding this comment.
Looks good and works for me on both Redux and MST!
app/state/connectToServer.ts
Outdated
| // Create a safe object with only expected properties to prevent prototype pollution | ||
| const safeChange = { | ||
| path: change.path, | ||
| value: change.value, |
Contributor
There was a problem hiding this comment.
If using the util above:
Suggested change
| value: change.value, | |
| value: sanitizeValue(change.value), |
app/state/connectToServer.ts
Outdated
| const existingSubscription = currentSubscriptions[existingSubscriptionIndex] | ||
| currentSubscriptions[existingSubscriptionIndex] = { | ||
| path: existingSubscription.path, | ||
| value: change.value, |
Contributor
There was a problem hiding this comment.
If using the util above:
Suggested change
| value: change.value, | |
| value: sanitizeValue(change.value), |
app/state/connectToServer.ts
Outdated
| const clientId = data?.conn?.clientId | ||
| if (!clientIds.includes(clientId)) { | ||
| setClientIds((prev) => [...prev, clientId]) | ||
| // setActiveClientId(clientId) |
Contributor
There was a problem hiding this comment.
Should this be commented out?
Contributor
Author
There was a problem hiding this comment.
whoops lol, was testing the opposite side of it
| if (existingSubscriptionIndex !== -1) { | ||
| // Create a safe object with only expected properties to prevent prototype pollution | ||
| const existingSubscription = currentSubscriptions[existingSubscriptionIndex] | ||
| currentSubscriptions[existingSubscriptionIndex] = { |
Check warning
Code scanning / CodeQL
Prototype-polluting assignment Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix prototype pollution risks in this code, the best approach is to avoid using potentially polluted object keys directly, especially those derived from user-controlled input. The patch should:
- Ensure we never use
"__proto__","constructor", or"prototype"(or similarly risky keys) as object keys. - Continue to use
isSafeKey, but strengthen it if needed (not shown, but suggest robust implementation). - Use a Map for mapping client IDs to their subscriptions instead of a plain object, OR create a prototype-less object via
Object.create(null). Since we only have this code shown, and can't refactor the wider state shape, we will add explicit checks to forbid dangerous keys. - Add an explicit check before using
data.cmd.clientIdandchange.pathas keys, even afterisSafeKey, to block prototype pollution keys. - Update the code in the block where
setStateSubscriptionsByClientIdis called to bail out if unsafe keys are present. - Add a utility predicate to reject keys
"__proto__","constructor", or"prototype", and use it alongsideisSafeKey.
No dependency is needed; we can add a constant or function for the forbidden key check.
This change is localized to the handler for the state.values.change command, inside the ws.socket.onmessage block.
Suggested changeset
1
app/state/connectToServer.ts
| @@ -109,7 +109,13 @@ | ||
| if (data.cmd.type === "state.values.change") { | ||
| console.log("state.values.change", data.cmd) | ||
| data.cmd.payload.changes.forEach((change: StateSubscription) => { | ||
| if (!isSafeKey(data.cmd.clientId) || !isSafeKey(change.path)) { | ||
| // Defense in depth: reject forbidden keys explicitly | ||
| if ( | ||
| !isSafeKey(data.cmd.clientId) || | ||
| isForbiddenKey(data.cmd.clientId) || | ||
| !isSafeKey(change.path) || | ||
| isForbiddenKey(change.path) | ||
| ) { | ||
| console.warn( | ||
| "Ignored suspicious property name in state.values.change:", | ||
| data.cmd.clientId, |
Copilot is powered by AI and may make mistakes. Always verify output.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.