Skip to content

Fix & test : polling connect, constant-time login, readonly HTTP writes, and error-page disclosure (PyTest - 6) - #1679

Open
Manik-Khajuria-5 wants to merge 37 commits into
fossasia:devfrom
Manik-Khajuria-5:Testing-6
Open

Fix & test : polling connect, constant-time login, readonly HTTP writes, and error-page disclosure (PyTest - 6)#1679
Manik-Khajuria-5 wants to merge 37 commits into
fossasia:devfrom
Manik-Khajuria-5:Testing-6

Conversation

@Manik-Khajuria-5

@Manik-Khajuria-5 Manik-Khajuria-5 commented Aug 2, 2026

Copy link
Copy Markdown
Member

Description

Four server fixes, each with its regression test:

  • POST /vis_socket_wrap never assigned .request, so it answered 500 Visdom(use_polling=True) could not connect at all.

  • Login compared credentials with ==; now hmac.compare_digest on both halves.

  • -readonly blocked sockets and uploads but not /events, /update, /close, /delete_env, /save, /fork_env or the /win_data write. Adds a check_readonly decorator next to check_auth.

  • debug was set unconditionally, so any 500 rendered the traceback, source paths and request object to the client. Replaced with a show_error_details setting driven by the logging level.

Adds 92 tests: integration/polling_parity.py (38), integration/auth.py (47), 7 error-page tests in edge_cases.py.

Fixes : #1695

Integration note

This PR should be merged after #1673 , as it depends on the lower-layer changes introduced there.

How Has This Been Tested?

Python 3.12.13 on Linux, black under 3.11.

  • Full suite 807 passed / 7 subtests (was 716); python -O green across integration/.
  • Live server: polling client connects and plots; -readonly gives 403 on all nine mutating routes and 200 on reads; /error/500 shows the production card by default and the traceback under -logging_level DEBUG; `example/deno errors.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing funected)
  • Code refactor or cleanup (changes to existing code for improved readability or performance)

Checklist:

  • I adapted the version number under py/visdom/VERSION according to Semantic Versioning
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Summary by Sourcery

Harden the visdom server’s persistence, socket, auth, and update pathways, enforce readonly semantics consistently, and greatly expand unit and integration test coverage (including polling and error-page behavior) while cleaning up test structure and packaging.

New Features:

  • Add dedicated manual visual-check script and documentation for visually inspecting all pane types.
  • Introduce test utilities package with shared fakes, HTTP base test case, socket helpers, and payload builders for reuse across tests.

Bug Fixes:

  • Ensure polling source sockets correctly attach a request so polling clients can connect without AttributeError.
  • Fix socket close handling to avoid double pop of pane data and ensure close events carry pane data for sources.
  • Treat malformed embeddings history and invalid plot history frames as no-ops instead of raising, keeping sockets usable.
  • Cap text, embeddings history, image history, and plot history growth to prevent unbounded in-memory state over long runs.
  • Make JSONStore environment and undo writes atomic via temporary files and rename, preserving previous data on interrupted writes or rename failures.
  • Respect readonly mode for all mutating HTTP routes, including /events, /update, /close, /delete_env, /save, /fork_env, /win_data writes, uploads, and experiment logging.
  • Harden /update handling for plots by validating named trace payload sizes, handling empty data lists, and safely supporting categorical axes.
  • Ensure delete_env and related operations consistently escape environment ids and handle missing envs as no-ops.
  • Prevent error pages from exposing tracebacks and request details by default, only showing them when logging is at DEBUG.
  • Fix login credential verification to use constant-time comparisons for both username and password.
  • Enforce plot history frame bounds and types to avoid TypeError in plot_history updates.

Enhancements:

  • Refine UpdateHandler plot update logic to better handle trace injection, deletion, and marker updates with cleaner utilities.
  • Extend server defaults and application wiring to include configurable max_plot_history, used by plot history panes.
  • Improve LazyEnvData, deep-copy semantics, and JSONStore naming rules with comprehensive tests for durability and name collisions.
  • Tighten window index allocation to avoid index reuse after closes, aligning with undo history rules.
  • Clarify and expand pytest configuration for unit vs integration tests, including test discovery, markers, and exclusions.
  • Document and test login, authentication, and readonly behaviors across HTTP and sockets, including cookie handling.
  • Centralize broadcast_layouts on the base socket handler and share save_all behavior through a synchronous executor-friendly path.
  • Improve server_utils helpers (_is_missing_value, warn_once, id/path helpers) and add targeted unit tests.
  • Restructure existing tests into unit and integration suites and add broad coverage for sockets, updates, environment lifecycle, and edge cases.
  • Exclude tests packages from distribution in setup.py to avoid shipping test modules to users.

Build:

  • Adjust pytest configuration to treat py/tests as test root, include py/tests on pythonpath, collect all *.py under unit/integration, and exclude testutils from collection.

Documentation:

  • Expand testing guide to describe unit vs integration layout, shared fixtures, HTTP test patterns, and markers.
  • Add README documenting manual visual checks and how they complement automated tests.

Tests:

  • Add extensive integration coverage for socket command handling, socket lifecycle, polling parity, auth/login, readonly enforcement, plot updates, text/media updates, window and environment lifecycles, build verification, and edge cases.
  • Add unit tests for data_model durability, LazyEnvData behavior, memory caps, shared_utils, server_utils, tsne backend selection, window construction, and image slider behavior.
  • Refactor storage wiring tests to use shared test utilities and SpyStore.

Chores:

  • Move existing tests into unit/integration namespaces and introduce shared testutils package for reuse.
  • Update packaging configuration to exclude tests from the installed distribution.

Adds conftest.py, a testutils package, and unit tests for window().
AsyncHTTPTestCase is a unittest.TestCase, and pytest will not inject
fixtures into those. Run the Application on a background event loop
instead and talk to it with requests, which is already a runtime
dependency, so HTTP tests can be plain functions.
Lands the window lifecycle file as plain pytest functions on the
visdom_server fixture, dropping the per-file setUp block and the
temp directory it never cleaned up.
Collapses the repeated plot-trace and opts assertions into parametrized
tables, so each case reports separately instead of hiding behind the
first failure.
The reload case now builds its second Application from the app_factory
fixture, which shares env_path with visdom_server, instead of hand-rolling
one against a leaked temp directory.
The update-into-a-missing-env case asserted 'status is 200 or 500',
which could not fail. It now pins the actual behaviour: 200 with
'win does not exist'.
register_window took the index from len(env), so closing any window but
the last produced a duplicate index and the pane order went undefined
after a reload. Use max(i) + 1, which is what the undo path already does.
Three validation checks used a bare assert, so an invalid request came
back as a 500 and, under python -O, was skipped entirely: forking an
unknown env then raised KeyError and a named trace update read past the
data it was given. Raise HTTPError(400) like the other checks do.
Places the two test files dev added into the new layout, and points the
new storage-wiring test at env_payload: it called the local _env helper
this branch had already replaced, which merged cleanly and then failed.
Brings in dev along with the layout placement and the env_payload fix
made on the PR-1 branch. D1 and D2 both survive the auto-merge of
server_utils.py and web_handlers.py.
Replaces the background event loop and requests session with the
AsyncHTTPTestCase base class from PR-1, which already runs the app
in-process on an ephemeral port. The four integration files become
TestCase subclasses; pytest still collects and marks them.
first/second read as two windows; they are the id returned by each call,
which is the same id both times. Also assert only one pane exists.
The guidance still said VisdomHTTPTestCase was scheduled for replacement
by a background-loop fixture. Record the opposite, and the consequences
for anyone writing an HTTP test: no fixtures, no parametrize, share via
a base class.
Both halves now run off VisdomHTTPTestCase instead of their own fixture block.
The three cap tables collapse onto parametrize; the file was untracked until now.
Guard the missing-point check behind a numeric test, and stop indexing data and
traces past their length when an update supplies fewer entries than the plot.
It was the one history pane that grew without bound.
Removing a heatmap names no trace and posts no data, so the shortcut for
opts-only updates returned before the delete branch and left the plot up.
A python client running with use_polling=True POSTs to /vis_socket_wrap
with no sid to get one minted. That path built a VisSocketWrapper and
initialized it without ever assigning .request, so open() raised
AttributeError reaching for request.remote_ip and the endpoint answered
500 -- polling mode could not connect at all. The subscriber route a few
lines below already assigns it; do the same here.
Polling had no tests at all, while AGENTS.md asks for every socket
feature to work over both transports. Covers minting a sid on each
route, the handshake, the three protocol failures, ten commands driven
end to end over HTTP, the readonly short-circuit, the idle reaper and
the pending-message deque.

Two behaviours are pinned rather than changed: a polling subscriber
receives a fourth, redundant layout_update because it registers before
initialize broadcasts, and the deque is unbounded, which is what makes
the reaper load-bearing.
The submitted username and derived key were checked with ==, which
short-circuits on the first differing character. That leaks, through
response timing, how much of a guess was correct, and the short-circuit
between the two checks leaked whether the username alone was right.
hmac.compare_digest on both halves removes the signal.
-readonly stopped socket commands and blocked uploads and experiment
logging, but the HTTP routes that create, update, close, fork, delete
and save were left open, so any client could still mutate a server
started readonly. Adds a check_readonly decorator next to check_auth and
puts it on those six endpoints, plus the one write hidden behind
/win_data, which also serves reads and so checks inline.

readonly moves onto _WEB_APP_ATTRIBUTES, which is where every other
per-request attribute already comes from; the two handlers that copied
it by hand no longer need an initialize override.
Neither gate had any tests. Covers the login page and its form, the four
ways a credential can be wrong, 401 on every authenticated route with
the state left untouched, the same routes succeeding with the issued
cookie, a forged cookie, /health staying public by design, and both
socket kinds closing instead of registering when unauthenticated.

The readonly half asserts 403 and no state change for each of the nine
mutating routes, that reads and the env page still work, and that an
ordinary server still answers 200 on all of them -- otherwise a
decorator that refused everything would look like a pass.
write_error rendered the exception, its traceback and request.__dict__
whenever tornado's debug flag was set, and app.py set that flag
unconditionally -- so every deployment served its own source paths and
request internals on any 500, and error.html's production branch was
unreachable. The flag also turned on autoreload for every server as a
side effect.

Replaces it with a show_error_details setting driven by the root logging
level, so -logging_level DEBUG brings the detail back and nothing else
changes with it.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry @Manik-Khajuria-5, your pull request is larger than the review limit of 150000 diff characters

@sourcery-ai

sourcery-ai Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

This PR hardens the Visdom server by making JSONStore writes atomic, enforcing readonly mode and constant-time login checks, tightening socket and update handlers, and significantly expanding pytest coverage with new unit and integration suites for HTTP, sockets, auth, polling, and environment lifecycle.

Sequence diagram for constant-time login verification

sequenceDiagram
    actor User
    participant LoginHandler
    participant JSONStore
    participant hmac

    User->>LoginHandler: POST /login (username, password)
    LoginHandler->>JSONStore: load_credentials()
    JSONStore-->>LoginHandler: stored_username, stored_password_hash
    LoginHandler->>LoginHandler: hash_password(password, salt)
    LoginHandler->>hmac: compare_digest(username, stored_username)
    hmac-->>LoginHandler: username_ok
    LoginHandler->>hmac: compare_digest(derived_password, stored_password_hash)
    hmac-->>LoginHandler: password_ok
    alt username_ok and password_ok
        LoginHandler->>User: set_secure_cookie(user_password)
    else invalid credentials
        LoginHandler->>User: 400 Bad Request
    end
Loading

Sequence diagram for readonly HTTP write enforcement

sequenceDiagram
    actor Client
    participant PostHandler as MutatingHandler
    participant WinDataHandler
    participant check_auth
    participant check_readonly
    participant reject_readonly

    Client->>MutatingHandler: POST /update (state-changing)
    MutatingHandler->>check_auth: @check_auth
    check_auth-->>MutatingHandler: auth_ok
    MutatingHandler->>check_readonly: @check_readonly
    alt handler.readonly
        check_readonly->>reject_readonly: reject_readonly(handler)
        reject_readonly-->>Client: 403 readonly error
    else not readonly
        check_readonly-->>MutatingHandler: proceed
        MutatingHandler->>Client: 200 OK (state updated)
    end

    Client->>WinDataHandler: POST /win_data (read/write endpoint)
    WinDataHandler->>check_auth: @check_auth
    check_auth-->>WinDataHandler: auth_ok
    WinDataHandler->>WinDataHandler: if "data" in args
    alt handler.readonly and "data" in args
        WinDataHandler->>reject_readonly: reject_readonly(handler)
        reject_readonly-->>Client: 403 readonly error
    else readonly or not, no write
        WinDataHandler-->>Client: 200 OK (read-only response)
    end
Loading

File-Level Changes

Change Details Files
Make environment and undo persistence atomic and better tested.
  • Introduce a shared _atomic_write helper that stages to .tmp then os.replace to avoid truncation on interrupted writes.
  • Use _atomic_write from serialize_env for both primary and hash-fallback env files, and from save_undo for undo stacks.
  • Add durability tests that simulate mid-write failures and failed renames to ensure previous env/undo data is preserved and stray .tmp files are not treated as envs.
  • Add tests for long env IDs using hash filenames, including name collisions and list_envs behavior.
py/visdom/data_model/json_store.py
py/tests/unit/data_model.py
Tighten plot update semantics, add plot-history caps, and make missing-value handling robust.
  • Thread a max_plot_history parameter through UpdateHandler.update_packet/update and cap plot_history content length while preserving newest frames.
  • Refactor plot update loops to iterate zip(idxs, new_data) and use a new _is_missing_value helper to skip all-None/NaN/Inf numeric updates without raising on string (categorical) axes.
  • Allow injecting a new named trace into empty plots and tighten validation so named updates with multiple data entries yield HTTP 400 instead of asserts.
  • Fix heatmap and scatter update edge cases (empty data lists, unnamed delete operations, bad frame indices) and cover them with integration tests.
  • Extend text, embeddings, image_history, and plot_history panes to enforce in-memory caps (DEFAULT_MAX_TEXT_LINES, DEFAULT_MAX_OLD_CONTENT, DEFAULT_MAX_IMAGE_HISTORY, DEFAULT_MAX_PLOT_HISTORY) with targeted unit tests.
  • Update image slider tests and handler scaffolding to honor the new max_plot_history field.
py/visdom/server/handlers/web_handlers.py
py/visdom/utils/shared_utils.py
py/tests/unit/image_slider.py
py/tests/unit/memory_caps.py
py/tests/unit/window_builder.py
py/tests/integration/update_plots.py
py/tests/integration/update_text_media.py
Enforce readonly mode consistently for HTTP and sockets, and introduce a reusable readonly guard.
  • Add reject_readonly and check_readonly helpers that return 403 JSON errors for mutating HTTP routes when app.readonly is true, stacked under check_auth.
  • Decorate POST handlers for events, update, close, delete_env, save, fork_env, win_exists/win_data, experiments/log with check_readonly, and handle the mixed read/write /win_data path by explicitly rejecting writes while allowing reads.
  • Remove redundant readonly wiring from specific handlers (e.g., UploadEnvHandler, ExperimentLogHandler initialize) in favor of the centralized decorator.
  • Add integration tests that in readonly mode all nine mutating HTTP routes respond with 403 and leave state/env files unchanged, while read-only routes still work.
  • Verify that under non-readonly servers all guarded routes still return 200 to prevent regressions.
py/visdom/utils/server_utils.py
py/visdom/server/handlers/web_handlers.py
py/tests/integration/auth.py
Harden auth: constant-time login comparison and socket behavior under login mode, with full integration coverage.
  • Replace username/password equality checks in IndexHandler with hmac.compare_digest on UTF-8 bytes for both username and derived password, ensuring both halves are always compared to avoid timing leaks.
  • Clarify that the client posts sha256(password) and the server stores a salted PBKDF2 hash of that value, with tests asserting stored format and non-equality to the client hash.
  • Ensure login-enabled Application reads COOKIE_SECRET from DEFAULT_ENV_PATH and that unauthorized HTTP routes respond 401 with empty bodies, while health and non-login deployments remain open as before.
  • Add tests verifying that unauthenticated WebSocket subscribers/sources are immediately closed and that sockets open normally when login is disabled.
py/visdom/server/handlers/web_handlers.py
py/visdom/server/app.py
py/tests/integration/auth.py
Fix polling transport so sources/subscribers work, layout broadcasts are shared, and idle reaping behaves correctly, with parity tests vs WebSockets.
  • Move broadcast_layouts implementation from VisSocketWrapper into AnySocketHandlerOrWrapper so polling source sockets can call save_layouts without hitting a ValueError, and have all subscribers receive layout_update messages.
  • In VisSocketWrapper path for polling /vis_socket_wrap, ensure the newly created wrapper has request set (mirroring subscriber open) so open() logging and peer access do not raise AttributeError.
  • Refine close and pop_embeddings_pane behavior to avoid double-pop bugs, missing envs, empty histories, and IndexError/KeyError escaping from on_message.
  • Tighten plot_history frame selection to require an int index (excluding bool) and log-and-drop invalid frames instead of raising TypeError.
  • Add polling_parity integration suite that exercises handshake, send/query protocol, command parity (close, undo, delete_env, save_layouts, update_comment, layout_item_update, echo), readonly mode interactions, idle socket reaping, and message queue ordering.
py/visdom/server/handlers/socket_handlers.py
py/tests/integration/polling_parity.py
Clarify and safely control error-page detail exposure based on logging level instead of Tornado debug, and test both modes.
  • Replace unconditional tornado_settings['debug']=True with a show_error_details flag driven by logging.getLogger().isEnabledFor(logging.DEBUG), decoupling debug error pages from autoreload.
  • Update BaseHandler.write_error to gate exception, traceback, and raw request injection into the error template based on show_error_details rather than debug, leaving status/title unaffected.
  • Add integration tests asserting that /error/500 shows only a generic production message by default (no traceback, no request/paths) and that under root logger DEBUG level the traceback and request fields are present.
  • Ensure the server continues to respond 200 to /health after forced /error/500 and that various error routes (/error/404, non-numeric error ids) behave as expected.
py/visdom/server/app.py
py/visdom/server/handlers/base_handlers.py
py/tests/integration/edge_cases.py
Strengthen socket command handling and lifecycle, including echo, delete, save, undo, embeddings, and layout updates, backed by a dedicated integration suite.
  • Fix close handling to pop pane data only once under escaped eid, avoiding pane_data=None and missed close events for sources when raw and escaped eids differ.
  • Add robust handling for pop_embeddings_pane when old_content is empty or missing, logging and dropping the command instead of raising, and ensuring has_previous and contentID are updated consistently.
  • Ensure echo is implemented only on the source socket class and returned early without falling through to base command dispatch; shared commands still run for sources.
  • Document and test SocketFailureReason codes and to_failure_response shape, and cover polling protocol errors for invalid sids, message types, and closed sockets.
  • Add socket_lifecycle tests for open/register/close behavior of subscribers and sources, including readonly short-circuiting and idempotent close handling.
py/visdom/server/handlers/socket_handlers.py
py/tests/integration/socket_commands.py
py/tests/integration/socket_lifecycle.py
Improve window/env lifecycle correctness and test coverage (HTTP-based create/update/delete, env fork/save/delete/reload) with new integration tests.
  • Add window_lifecycle integration tests covering create, win_exists, win_data, /update behavior, closing single/all windows, and enforcing monotonic non-reused pane indices even under churn.
  • Add environment_lifecycle tests covering implicit env creation, /fork_env deep copy semantics, /save writing env_path/.json, /delete_env behavior (including main protection), and Application reload picking up saved envs.
  • Refactor storage wiring tests to use shared FakeHandler, FakeSocket, SpyStore, and env_payload helpers, ensuring load_env, gather_envs, compare_envs, delete routes, and read helpers all go through JSONStore rather than raw filesystem.
py/tests/integration/window_lifecycle.py
py/tests/integration/environment_lifecycle.py
py/tests/integration/storage_wiring.py
Reorganise tests into unit/integration, add shared test utilities, and adjust pytest/packaging configuration to make helpers importable but not shipped or collected incorrectly.
  • Move and rename existing tests into py/tests/unit and py/tests/integration, introducing pytestmark markers and reorganising fixtures around conftest.py to distinguish pure unit tests from Application/HTTP/socket tests.
  • Add testutils package (FakeHandler, FakeSocket, SpyStore, payload builders, socket_double, VisdomHTTPTestCase) and update existing tests to depend on these instead of ad-hoc SimpleNamespace/inline doubles.
  • Update pytest config to use python_files=['*.py'], include py/tests in pythonpath, and exclude testutils from discovery via norecursedirs; register unit/integration/slow/server markers and document test layout and HTTP-testing constraints in .agents/context/testing.md.
  • Change setup.py find_packages(where='py') to exclude tests packages so py/tests does not end up in the installed distribution, while still allowing testutils imports in the test suite.
  • Add example/manual/visual_check.py and README describing manual visual checks separate from pytest, ensuring they are not collected as tests.
py/tests/test_storage_wiring.py
py/tests/test_image_slider.py
py/tests/test_data_model.py
py/tests/unit/*.py
py/tests/integration/*.py
.agents/context/testing.md
py/tests/conftest.py
py/tests/testutils/*.py
pyproject.toml
setup.py
example/manual/*.py
example/manual/README.md

Possibly linked issues

  • #fix: formalize socket polling failure reasons with SocketFailureReason enum: PR introduces SocketFailureReason enum and updates polling wrappers to return structured failure reasons, exactly solving the issue plus additional server fixes and tests.

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

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.

Add a Python test suite (pytest) for the server and client

1 participant