Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Sanitise operations on commit to ShareDB #2484

Merged
merged 1 commit into from
Nov 27, 2023
Merged

Conversation

DafyddLlyr
Copy link
Contributor

@DafyddLlyr DafyddLlyr commented Nov 27, 2023

What does this PR do?

  • Uses DOMPurify to sanitise input on "commit" to ShareDB

Next steps

  • Look at blocking flow updates via GraphQL API
  • Try to work out a reliable testing methodology (in another PR). It looks like Playwright has a WebSocket class that should allow the intercept of an outgoing frame to attempt to inject unsafe content (Docs: https://playwright.dev/docs/api/class-websocket)

Demo

Screen.Recording.2023-11-27.at.14.53.29.mov

"jsonwebtoken": "^8.5.1",
"pg": "^8.11.3",
"sharedb": "^3.3.1",
"sharedb": "^4.1.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only breaking changes in v4 is dropped support for Node 14.

Docs: https://github.com/share/sharedb/releases/tag/v4.0.0

@@ -16,6 +18,10 @@ const sharedb = new ShareDB({
}),
});

// Setup JSDOM and DOMPurify
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't totally connected last week that sharedb qualifies as "on the server" and therefore was trying to use without JSDOM, but this makes much more sense in hindsight 👍

@DafyddLlyr DafyddLlyr marked this pull request as ready for review November 27, 2023 15:02
@DafyddLlyr DafyddLlyr requested a review from a team November 27, 2023 15:04
Copy link

github-actions bot commented Nov 27, 2023

Removed vultr server and associated DNS entries

Copy link
Member

@jessicamcinchak jessicamcinchak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sorting this one out!

@@ -16,6 +18,10 @@ const sharedb = new ShareDB({
}),
});

// Setup JSDOM and DOMPurify
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't totally connected last week that sharedb qualifies as "on the server" and therefore was trying to use without JSDOM, but this makes much more sense in hindsight 👍

@jessicamcinchak
Copy link
Member

[Placeholder] Respond to Security Audit feedback

@DafyddLlyr DafyddLlyr merged commit ef16e2a into main Nov 27, 2023
12 checks passed
@DafyddLlyr DafyddLlyr deleted the dp/sharedb-dompurify branch November 27, 2023 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants