Skip to content

Conversation

@joefarebrother
Copy link
Contributor

Models remote flow sources for the websockets library.
Currently omitted are handlers set up via route maps, e.g. with websockets.asyncio.router.route; these would require a little more complex handling of dataflow through classes of werkzeug.routing.

Copilot AI review requested due to automatic review settings December 1, 2025 20:15
@joefarebrother joefarebrother requested a review from a team as a code owner December 1, 2025 20:15
Copilot finished reviewing on behalf of joefarebrother December 1, 2025 20:18
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds modeling for remote flow sources in the websockets PyPI package. The implementation provides taint tracking for WebSocket server connections through handler parameters and their methods like recv() and recv_streaming().

  • Models websocket handlers registered via serve and unix_serve functions in both asyncio and sync variants
  • Implements taint tracking for ServerConnection instances and their message-receiving methods
  • Adds comprehensive test coverage for both synchronous and asynchronous websocket handlers

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
python/ql/lib/semmle/python/frameworks/Websockets.qll New framework modeling file that defines websocket handlers, ServerConnection instances, and taint propagation rules
python/ql/lib/semmle/python/Frameworks.qll Imports the new Websockets framework module
python/ql/lib/change-notes/2025-12-01-websockets.md Documents the addition of remote flow source modeling for websockets
python/ql/test/library-tests/frameworks/websockets/taint_test_sync.py Test file for synchronous websocket taint tracking scenarios
python/ql/test/library-tests/frameworks/websockets/taint_test_asyncio.py Test file for asynchronous websocket taint tracking scenarios
python/ql/test/library-tests/frameworks/websockets/response_test.py Test file covering various handler registration patterns including route handlers
python/ql/test/library-tests/frameworks/websockets/InlineTaintTest.ql QL test query for inline taint test framework
python/ql/test/library-tests/frameworks/websockets/InlineTaintTest.expected Expected results file (auto-generated)
python/ql/test/library-tests/frameworks/websockets/ConceptsTest.ql QL test query for concepts test framework
python/ql/test/library-tests/frameworks/websockets/ConceptsTest.expected Expected results file for concepts test

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +68 to +69
private class HandlerParam extends DataFlow::Node, InstanceSource {
HandlerParam() { exists(WebSocketHandler h | this = DataFlow::parameterNode(h.getArg(0))) }
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The HandlerParam class should extend RemoteFlowSource::Range to properly model websocket handler parameters as remote flow sources, similar to how other frameworks like Pyramid and Tornado model their handler parameters. This class should also implement the getSourceType() method to return an appropriate description like "websockets handler parameter".

Suggested change
private class HandlerParam extends DataFlow::Node, InstanceSource {
HandlerParam() { exists(WebSocketHandler h | this = DataFlow::parameterNode(h.getArg(0))) }
private class HandlerParam extends RemoteFlowSource::Range, InstanceSource {
HandlerParam() { exists(WebSocketHandler h | this = DataFlow::parameterNode(h.getArg(0))) }
override string getSourceType() { result = "websockets handler parameter" }

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This parameter is already classified as a remote flow source due to WebSocketHandler being RequestHandler.

* calls, or a special parameter that will be set when functions are called by an external
* library.
*
* Use the predicate `WebSocket::instance()` to get references to instances of `websockets.asyncio.ServerConnection` and `websockets.sync.ServerConnection`.
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The documentation references WebSocket::instance() but the correct predicate name is ServerConnection::instance(). This should be updated to match the actual module and predicate structure.

Suggested change
* Use the predicate `WebSocket::instance()` to get references to instances of `websockets.asyncio.ServerConnection` and `websockets.sync.ServerConnection`.
* Use the predicate `ServerConnection::instance()` to get references to instances of `websockets.asyncio.ServerConnection` and `websockets.sync.ServerConnection`.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant