Skip to content

Conversation

@lukasmasuch
Copy link
Collaborator

@lukasmasuch lukasmasuch commented Oct 13, 2025

@github-actions
Copy link
Contributor

github-actions bot commented Oct 13, 2025

✅ PR preview is ready!

Name Link
📦 Wheel file https://core-previews.s3-us-west-2.amazonaws.com/pr-12772/streamlit-1.52.1-py3-none-any.whl
📦 @streamlit/component-v2-lib Download from artifacts
🕹️ Preview app pr-12772.streamlit.app (☁️ Deploy here if not accessible)

@lukasmasuch lukasmasuch requested a review from Copilot October 13, 2025 13:20
@lukasmasuch lukasmasuch added change:feature PR contains new feature or enhancement implementation impact:users PR changes affect end users labels Oct 13, 2025
@snyk-io
Copy link
Contributor

snyk-io bot commented Oct 13, 2025

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

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 pull request introduces optional support for running Streamlit with Starlette instead of Tornado as the server framework. The prototype adds a new configuration flag server.useStarlette that allows switching between the existing Tornado implementation and a new Starlette/ASGI-based server.

  • Adds Starlette as an optional dependency with required supporting libraries (uvicorn, python-multipart, anyio)
  • Implements a complete Starlette application with HTTP endpoints, WebSocket handling, and authentication routes
  • Provides comprehensive migration documentation outlining the technical approach and implementation plan

Reviewed Changes

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

Show a summary per file
File Description
tornado-starlette-migration.md Detailed migration documentation covering scope, risks, implementation plan, and technical mapping from Tornado to Starlette
lib/streamlit/config.py Adds server.useStarlette configuration option to enable experimental Starlette server
lib/streamlit/web/server/server.py Modifies server startup to conditionally use Starlette implementation based on config flag
lib/streamlit/web/server/starlette_app.py Core Starlette application implementation with all HTTP endpoints and WebSocket handling
lib/streamlit/web/server/starlette_auth_routes.py Authentication routes implementation for Starlette using authlib integration
lib/setup.py Adds optional "starlette" dependency group with required packages
lib/test-requirements.txt Adds Starlette-related testing dependencies
lib/tests/streamlit/web/server/starlette_*.py Comprehensive test coverage for Starlette implementation
lib/tests/streamlit/web/server/server_test.py Updates existing server tests to cover both Tornado and Starlette implementations

@lukasmasuch lukasmasuch requested a review from Copilot October 13, 2025 13:39
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

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

@lukasmasuch lukasmasuch added the security-assessment-completed Security assessment has been completed for PR label Oct 13, 2025
@lukasmasuch
Copy link
Collaborator Author

@cursor review

@lukasmasuch lukasmasuch added the ai-review If applied to PR or issue will run AI review workflow label Dec 15, 2025
@lukasmasuch lukasmasuch requested a review from Copilot December 15, 2025 18:10
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

Copilot reviewed 28 out of 28 changed files in this pull request and generated 4 comments.

@github-actions github-actions bot removed the ai-review If applied to PR or issue will run AI review workflow label Dec 15, 2025
@github-actions
Copy link
Contributor

Summary

This PR adds optional support for running Streamlit with Starlette/Uvicorn as an alternative to the existing Tornado server implementation. The new server is controlled by a new config option server.useStarlette (default: false). The implementation is comprehensive, including:

  • A complete Starlette-based web server (starlette_server.py)
  • All HTTP routes (health, metrics, media, upload, components, static files, auth)
  • WebSocket handling for client-server communication
  • XSRF protection compatible with Tornado's token format
  • Custom GZip middleware that excludes audio/video from compression
  • Extensive unit test coverage (~2,600 lines of tests)

This addresses many long-standing community requests for ASGI support, better async performance, and FastAPI integration capabilities.

Code Quality

The code is well-structured with clear separation of concerns and follows the project's coding conventions. Key observations:

Strengths

  1. Clean architecture: Each module has a single responsibility (routes, websocket, auth, static files, etc.)
  2. Proper type annotations: All new code uses type hints throughout
  3. Good use of async/await: The implementation properly leverages async patterns
  4. Comprehensive documentation: Module and function docstrings explain purpose and behavior
  5. Security-conscious design: XSRF validation, origin checking, path traversal protection are all in place

Issues to Address

  1. Private attribute access (lib/streamlit/web/server/starlette/starlette_app.py:125):

    main_script_path = getattr(runtime, "_main_script_path", None)

    The TODO comment acknowledges this. Consider exposing this as a public property on Runtime.

  2. E2E tests now run exclusively with Starlette (e2e_playwright/conftest.py:1146-1147):

    "--server.useStarlette",
    "true",

    This means the Tornado backend is no longer E2E tested in CI. This could lead to undetected regressions in the Tornado code path. Consider running E2E tests for both backends, or at least having a subset run with Tornado.

  3. Lazy module loading change (e2e_playwright/lazy_loaded_modules.py):
    The rich module was removed from the lazy-loaded modules list, suggesting it's now imported at startup. This may impact cold start times.

  4. typing-extensions version bump (lib/setup.py:60):

    "typing-extensions>=4.10.0, <5",

    Increased from 4.4.0 to 4.10.0. This could break existing installations. Recommend documenting this change in release notes.

  5. Unix socket support missing (lib/streamlit/web/server/starlette/starlette_server.py:145-147):

    if _server_address_is_unix_socket():
        raise RuntimeError("Unix sockets are not supported with Starlette currently.")

    This is properly documented with an error message, but users relying on Unix sockets cannot use Starlette mode.

Test Coverage

The test coverage is excellent with comprehensive unit tests for all new modules:

Test File Test Classes Coverage
starlette_app_test.py 30+ tests Full endpoint coverage
starlette_server_test.py Socket binding, port retry, lifecycle Good
starlette_websocket_test.py Protocol parsing, user info, XSRF Good
starlette_routes_test.py CORS, base URL handling Good
starlette_auth_routes_test.py OAuth flow, cookies Good
starlette_app_utils_test.py Range parsing, signing, XSRF Excellent
starlette_static_routes_test.py SPA fallback, caching, mounts Good

Test Quality Assessment

  • ✅ Tests follow pytest conventions and have docstrings
  • ✅ Use of patch_config_options for configuration testing
  • ✅ Proper use of fixtures (starlette_client, tmp_path)
  • ✅ Testing edge cases (invalid tokens, missing headers, etc.)
  • ✅ Security scenarios tested (XSRF validation, origin checks)

Gaps Identified

  1. No cross-backend compatibility tests: No tests verify that Tornado and Starlette produce identical responses for the same requests.
  2. Mixed cookie signing in tests: Some tests in starlette_websocket_test.py use Tornado's create_signed_value while testing Starlette's _parse_user_cookie_signed, which only works with Starlette-signed cookies.

Backwards Compatibility

Aspect Status Notes
Default behavior ✅ Unchanged server.useStarlette defaults to false
Auth cookies ⚠️ Breaking Switching backends invalidates cookies (documented)
Unix sockets ❌ Not supported Users must continue using Tornado
typing-extensions ⚠️ Version bump From 4.4.0 to 4.10.0
Config API ✅ Additive New option doesn't affect existing config

The feature is opt-in, so existing users are unaffected unless they enable server.useStarlette.

Security & Risk

Security Assessment ✅

  1. XSRF Protection: Properly implemented with Tornado-compatible token format
  2. Cookie Signing: Uses itsdangerous with proper secret management
  3. Origin Validation: WebSocket connections validate origin headers
  4. Path Traversal: build_safe_abspath prevents directory traversal attacks
  5. Upload Size Limits: Enforced via server.maxUploadSize config
  6. CORS: Proper handling with configurable allowed origins

Risk Assessment

  1. Medium Risk: E2E tests no longer cover Tornado - could introduce silent regressions
  2. Low Risk: Performance characteristics may differ between backends
  3. Low Risk: Edge cases in WebSocket handling may behave differently

Recommendations

  1. Critical: Consider adding a CI job that runs E2E tests with Tornado (--server.useStarlette false) to prevent regressions in the default code path.

  2. High: Fix the mixed cookie signing in starlette_websocket_test.py - lines 261-262 and 276-277 use Tornado's create_signed_value but _parse_user_cookie_signed only decodes Starlette-signed cookies:

    # Lines 261-262, 276-277 - Uses tornado.web.create_signed_value
    # but _parse_user_cookie_signed uses itsdangerous
  3. Medium: Document the typing-extensions version bump in release notes as it could affect users with pinned dependencies.

  4. Low: Expose _main_script_path as a public property on Runtime to avoid accessing private attributes.

  5. Low: Consider adding a flag to lazy_loaded_modules_test.py to accommodate the rich module being loaded earlier.

  6. Documentation: Update user documentation to note:

    • Auth cookies are invalidated when switching between Tornado and Starlette
    • Unix sockets are not supported with Starlette

Verdict

CHANGES REQUESTED: The implementation is high-quality and well-tested, but running E2E tests exclusively with Starlette creates a significant risk of silent regressions in the Tornado backend, which remains the default. This should be addressed before merge by either running E2E tests for both backends or explicitly acknowledging Tornado E2E coverage is being dropped.


This is an automated AI review. Please verify the feedback and use your judgment.

@github-actions github-actions bot added the do-not-merge PR is blocked from merging label Dec 15, 2025
@lukasmasuch lukasmasuch removed the do-not-merge PR is blocked from merging label Dec 15, 2025
@github-actions
Copy link
Contributor

Summary

This PR introduces an experimental Starlette-based server implementation (server.useStarlette) as an alternative to the existing Tornado backend. It includes the core server logic, WebSocket handling, static file serving, media/component routes, and authentication using Authlib.

Code Quality

The code is well-structured and modular.

  • Organization: The new code is neatly organized under lib/streamlit/web/server/starlette/.
  • Async I/O: Correctly uses anyio for asynchronous file operations.
  • Middleware: Appropriately uses Starlette middleware for sessions and GZip compression (with custom exclusion for media files).
  • Type Safety: Generally good type annotations. Some type: ignore directives are present, likely due to external library typing limitations or dynamic nature of some components, which is acceptable.

Test Coverage

  • Unit Tests: Comprehensive unit tests provided in lib/tests/streamlit/web/server/starlette/, covering endpoints, WebSocket logic, range requests, and server lifecycle.
  • E2E Tests: The PR updates e2e_playwright/conftest.py to enable --server.useStarlette by default for the test suite. This ensures the new server is battle-tested against the existing E2E suite.

Backwards Compatibility

  • Feature Flag: The feature is opt-in via server.useStarlette = true.
  • Cookie Compatibility: As noted in the code comments, authentication and XSRF cookies are not compatible between Tornado and Starlette backends due to different signing mechanisms (itsdangerous vs Tornado's secret). This will require users to re-authenticate if they switch backends, which is an acceptable trade-off for an experimental feature.
  • API Parity: The implementation strives to match Tornado's behavior, including XSRF token formats and WebSocket subprotocols.

Security & Risk

  • XSRF: The implementation includes XSRF token generation and validation logic that mirrors the existing Tornado implementation.
  • Path Traversal: Uses build_safe_abspath for serving static and component files to prevent traversal attacks.
  • Input Validation: Rejects WebSocket text frames (enforcing binary protobufs) and validates file upload sizes.
  • Memory Usage: File uploads are read entirely into memory (await upload.read()) before processing. This matches existing behavior but remains a potential resource bottleneck for large files.

Recommendations

  1. E2E Test Configuration: Enabling server.useStarlette in conftest.py switches the entire E2E suite to Starlette. If this PR is merged as-is, the develop branch will stop testing the Tornado backend in E2E runs. Consider if a parallel test job or a parameterized fixture is preferred to maintain coverage for the stable Tornado backend while testing Starlette.
  2. Performance: Monitor the memory usage of _upload_put in starlette_routes.py for large file uploads under load, as it reads the full content into memory.

Verdict

APPROVED: The changes are high quality, well-tested, and introduce a significant architectural improvement behind a feature flag.

@github-actions
Copy link
Contributor

Summary

This PR adds an experimental Starlette + Uvicorn server implementation behind the new config flag server.useStarlette, alongside extensive new Python unit tests for the Starlette routes/websocket/auth/static handling.

Key touchpoints include:

  • New config option: server.useStarlette (lib/streamlit/config.py).
  • Server entrypoint now conditionally starts either Tornado or Starlette (lib/streamlit/web/server/server.py).
  • New Starlette server package under lib/streamlit/web/server/starlette/.
  • E2E harness now starts Streamlit with --server.useStarlette true by default (e2e_playwright/conftest.py).

Code Quality

  • Overall structure: The Starlette implementation is reasonably modularized (separate modules for routes, websocket, static assets, gzip, server lifecycle), and the code is generally typed and documented.

  • Private attribute access: create_starlette_app reaches into runtime private attributes for main_script_path and media storage:

    • lib/streamlit/web/server/starlette/starlette_app.py:93-127 uses media_manager._storage and getattr(runtime, "_main_script_path", None). This is already called out via TODO, but it’s still a maintainability risk.
  • Cookie compatibility: Multiple places note (correctly) that Starlette cookies signed via itsdangerous are not compatible with Tornado secure cookies:

    • lib/streamlit/web/server/starlette/starlette_app_utils.py:108-121
    • lib/streamlit/web/server/starlette/starlette_auth_routes.py:143-163
    • lib/streamlit/web/server/starlette/starlette_websocket.py:116-123
      This is probably acceptable for an experimental opt-in, but it should be clearly documented in user-facing docs and/or the config option description.
  • Unix socket behavior diverges: Starlette mode currently errors out if server.address is a unix socket:

    • lib/streamlit/web/server/starlette/starlette_server.py:144-147
      This is OK as a limitation, but it should be called out explicitly (docs / config description) and ideally covered by a unit test.

Test Coverage

  • Python unit tests: Strong coverage was added for the Starlette implementation (lib/tests/streamlit/web/server/starlette/*). The tests generally follow repo guidelines (docstrings, typing). Examples:

    • Upload XSRF path coverage: lib/tests/streamlit/web/server/starlette/starlette_app_test.py:525-556
    • Websocket auth cookie parsing: lib/tests/streamlit/web/server/starlette/starlette_app_test.py:624-680
  • Missing critical test: There is currently no test that the Starlette WebSocket endpoint rejects disallowed Origins (see Security section). This is an important parity/security requirement.

  • E2E tests: The PR changes the Playwright harness to always run Streamlit with Starlette:

    • e2e_playwright/conftest.py:1128-1150
      This means the existing default (Tornado) path is no longer exercised by Playwright e2e, and all e2e baselines now depend on Starlette behavior. For an optional server backend, this is risky.

    Recommendation: keep e2e default on Tornado, and add a separate Starlette-specific e2e job/fixture (or opt-in via env var) so CI can validate both backends.

Backwards Compatibility

  • Default behavior: server.useStarlette defaults to False (lib/streamlit/config.py:1006-1014), so normal users should remain on Tornado.

  • E2E harness change is not representative of user defaults: Forcing Starlette in e2e (e2e_playwright/conftest.py:1146-1148) may hide Tornado regressions and introduces new flakiness surface for unrelated changes.

  • Auth cookie incompatibility when switching backends: Switching between Tornado and Starlette will invalidate existing auth cookies (expected per inline comments) and should be treated as a compatibility caveat for the experimental flag.

Security & Risk

  1. CHANGES REQUESTED: WebSocket Origin allowlisting is missing in Starlette implementation

In Tornado, the browser websocket handler checks origins via is_url_from_allowed_origins:

  • lib/streamlit/web/server/browser_websocket_handler.py:80-83

In Starlette, the WebSocket endpoint accepts connections without any Origin validation:

  • lib/streamlit/web/server/starlette/starlette_websocket.py:223-228 (await websocket.accept(...) happens unconditionally)

This is a security regression: without an Origin check, Streamlit becomes more exposed to cross-site WebSocket hijacking attempts when server.useStarlette is enabled.

Expected fix: implement an Origin validation step for websockets mirroring Tornado’s behavior (likely using server_util.is_url_from_allowed_origins(origin) or equivalent logic), and add unit tests covering:

  • Allowed origin passes
  • Disallowed origin is rejected (close / raise before accepting)
  • Behavior when Origin header is missing
  1. Risk: Auth cookie flags may not match Tornado expectations

starlette_auth_routes._set_auth_cookie sets the cookie as httponly=True but does not explicitly set secure, samesite, or path:

  • lib/streamlit/web/server/starlette/starlette_auth_routes.py:143-163

Even if Starlette defaults are reasonable, for parity and clarity it would be safer to explicitly set:

  • secure=bool(config.get_option("server.sslCertFile"))
  • samesite="lax" (if intended)
  • a path consistent with server.baseUrlPath if required
  1. Risk: e2e suite now depends on Starlette backend

This is a large surface area change and can introduce regressions/flakes unrelated to the actual PR goals.

Recommendations

  1. Add WebSocket Origin allowlisting to the Starlette websocket endpoint, matching Tornado behavior, and add unit tests for it.

    • Relevant files: lib/streamlit/web/server/starlette/starlette_websocket.py (handler), lib/tests/streamlit/web/server/starlette/starlette_websocket_test.py (new tests).
  2. Rework Playwright harness so it does not force Starlette for all e2e tests.

    • Suggested approach: keep default Tornado for existing e2e; add a separate Starlette e2e run (CI matrix or opt-in fixture/env var) to validate the experimental backend.
    • Relevant file: e2e_playwright/conftest.py:1128-1150.
  3. Make auth cookie settings explicit for parity (secure/samesite/path) in Starlette auth routes.

    • Relevant file: lib/streamlit/web/server/starlette/starlette_auth_routes.py:143-163.
  4. Document Starlette-mode limitations (e.g., no unix socket support) and cookie incompatibility when switching backends.

    • Relevant file: lib/streamlit/web/server/starlette/starlette_server.py:144-147.

Verdict

CHANGES REQUESTED: The Starlette websocket endpoint currently lacks Origin allowlisting (security regression vs Tornado) and the PR forces Starlette for all Playwright e2e tests, reducing coverage of the default Tornado path.


This is an automated AI review. Please verify the feedback and use your judgment.

@github-actions github-actions bot added the do-not-merge PR is blocked from merging label Dec 15, 2025
@streamlit streamlit deleted a comment from github-actions bot Dec 15, 2025
@sfc-gh-lmasuch sfc-gh-lmasuch removed the do-not-merge PR is blocked from merging label Dec 15, 2025
@lukasmasuch
Copy link
Collaborator Author

Closing this since the feature was merged in via other PRs. This will be released as experimental in 1.53.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:feature PR contains new feature or enhancement implementation impact:users PR changes affect end users security-assessment-completed Security assessment has been completed for PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants