Restructure package as plottle and secure formula evaluation - #23
Merged
Conversation
Audit findings A-01, A-02, A-04, A-05, A-08, A-11a, A-16, A-26.
Decisions recorded as TDEC-009..TDEC-013 in the mediator repo.
A-01 (Critical) -- session files no longer unpickle. _deserialize_data
called pickle.loads on a base64 blob taken from an uploaded .json, so
opening a shared session file executed its author's code. Arrays now
carry an explicit dtype + shape + raw buffer and are rebuilt with
np.frombuffer, with a length check so a truncated file fails loudly
instead of producing a wrong array. The generic "pickled" fallback is
removed; unrepresentable values become an "unsupported" placeholder.
Legacy pickle-encoded entries are refused, not loaded, and
load_session_from_file now returns what it skipped so the Export page can
tell the user. A dropped dataset can no longer remain the active
selection. Regression tests include a canary that fails if the decoder
ever executes a payload again.
A-02 (Critical) -- the eval() sandbox is replaced. {"__builtins__": {}}
is not a sandbox: the type hierarchy reaches an importer. Formulas are
now evaluated by an AST whitelist that rejects attribute access, indirect
calls, imports, lambdas and comprehensions, and bounds exponents and
expression length. Callees resolve against the function whitelist rather
than the namespace, so no column can become callable. All 17 documented
expression forms verified working; 15 escape attempts verified blocked.
SECURITY.md no longer claims a namespace restriction that never held, and
now states what is and is not guaranteed.
A-16 -- .pkl removed from the uploader and batch-folder extension lists;
load_pickle stays in the Python API where the caller has chosen to trust
the file. The GUI explains the refusal rather than just omitting it.
A-04 (Critical) -- tomllib is 3.11+ but the project supports 3.9, so the
Settings page raised ModuleNotFoundError on 3.9/3.10 while every other
page worked. Guarded with a tomli fallback plus a conditional dependency.
A-05 (Critical) -- streamlit floor 1.25.0 -> 1.49.0 in all three
requirement files; the code uses width="stretch" at 64 sites. Also added
the !=3.9.7 exclusion streamlit itself carries, and the missing
requests/h5py/xarray/netcdf4 to binder so the Binder badge can open the
formats the docs advertise.
A-11a -- test_cli.py used a bare 'python', so all 27 CLI tests failed in
any virtualenv. Now sys.executable with an absolute script path. This
took the suite from 877 passed / 27 failed to 954 passed / 0 failed.
A-26 -- version had drifted across six files. modules/__init__.py is now
the single source; pyproject reads it dynamically and cli.py, Home.py and
docs/conf.py import it. test_version asserted '1.0.0' and had gone stale
unnoticed because CI never ran test_cli.py -- it now asserts against the
package version.
A-08 -- launch.command and setup.command set to mode 755; without the
execute bit macOS Finder refuses to run them, so the documented
double-click setup failed on every fresh clone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013YLgSnkyRUosy5sxWUb33P
Audit finding A-03 (Critical); decision TDEC-014. Closes the last of the
hazards TDEC-008 identified back in April but which never landed on main.
Before this change the built wheel's top_level.txt was:
cli
modules
so `pip install plottle` claimed both `modules` and `cli` as top-level
import names in site-packages. Any project with a local modules.py or
cli.py -- or any other distribution claiming those names -- would collide,
with import resolution silently depending on sys.path order. Publishing
that to PyPI is irreversible.
top_level.txt is now just `plottle`, and the console script resolves to
plottle.cli:main so `py-modules = ["cli"]` is gone.
Mechanical parts:
- git mv modules plottle; all `from modules.*` / `import modules.*`
rewritten, including quoted mock.patch targets, which a bulk import
rewrite misses (the mediator repo's G-008 flags exactly this).
- pyproject: include = ["plottle*"], per-file-ignores, coverage omit, and
the dynamic version attr all repointed.
- CI: ruff/mypy paths and --cov=plottle.
- launch.bat / launch.command: streamlit run plottle/Home.py.
Removed the repo-root __init__.py. It was not in the wheel and nothing
imported it, but with the repo directory itself named `plottle`, a root
__init__.py makes the checkout importable as a package named `plottle`
that shadows the real one depending on sys.path order. Its version literal
was already folded into plottle/__init__.py by the previous commit.
CLI invocation follows the move: docs now use the `plottle` console
script, with `python -m plottle.cli` for a source checkout. That command
only exists if the package is actually installed, which the documented
setup never did (A-36) -- so setup.command, setup.bat and the README now
run `pip install -e ".[formats,nist]"` rather than
`pip install -r requirements.txt`.
Those two extras are new, and keep runtime parity with what
requirements.txt used to install while leaving the dev toolchain out of a
student's environment (A-37, A-41): `formats` = h5py/xarray/netcdf4,
`nist` = requests. An `all` extra bundles both.
954 passed, 21 skipped. ruff check and ruff format clean on plottle/.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013YLgSnkyRUosy5sxWUb33P
Audit finding A-09; mediator G-012. The built wheel previously contained no data files at all -- its only non-Python entries were dist-info metadata. logo.png, logo.ico, assets/, example-data/ and docs/gallery/ all lived at the repository root, outside the package, so packages.find never saw them. Because Home.py guards every asset with .exists(), nothing crashed; a pip-installed GUI simply came up with no logo, no NCCU branding, a warning on the Data Upload examples tab, and an empty Gallery. Running from a source checkout hid all of it from the developer. Moved into the package and declared via [tool.setuptools.package-data]: logo.png, logo.ico, assets/nccu-*.png -> plottle/assets/ example-data/Artificial/ -> plottle/example-data/Artificial/ docs/gallery/*.png + manifest.json -> plottle/gallery/ Paths in Home.py, 1_Data_Upload.py and 8_Gallery.py now resolve relative to the package directory rather than the repo root, and generate_gallery.py writes to the new location. Moving the gallery PNGs out of docs/ costs the Pages site nothing -- docs/index.html contains no <img> tags and never referenced them. generate_examples.py moves from inside the data directory to the repo root alongside generate_gallery.py, so the package holds only shipped code and data. Its output path and the Data Upload page's hint were updated to match. Verified per G-012 -- built the wheel, installed it into a clean venv, and resolved every asset from /tmp with the repo nowhere on sys.path: 29 data files present, all 13 gallery PNGs match the manifest, and the `plottle` console script reports 2.0.1. Not addressed here: .streamlit/config.toml still only works from a source checkout, because Streamlit reads it from the working directory rather than from the package. That is the same root cause as A-17/A-18 (theme handling) and is fixed there, not by packaging. 954 passed, 21 skipped. ruff check and ruff format clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013YLgSnkyRUosy5sxWUb33P
CORRECTION to the audit's A-10. I reported that CI "currently produces 6 mypy
errors, so the next push to main goes red". That was wrong, and wrong in a
way that mattered. Verified by running CI's exact lint commands against
origin/main in a CI-like environment:
mypy ... -> Success: no issues found in 13 source files
ruff -> All checks passed!
The 6 errors I measured were an artifact of my local environment having
matplotlib and numpy installed. CI's lint job installed only
`ruff mypy types-PyYAML types-requests`, so with no scientific stack present
--ignore-missing-imports collapsed numpy, scipy, pandas, matplotlib, seaborn,
plotly and streamlit to `Any`.
The real finding is worse than the one I reported: mypy was printing
"Success" while checking essentially nothing. For a library whose entire
surface is third-party array and figure types, every meaningful type
relationship was unchecked. It was green because it was blind.
A-10 -- the lint job now installs the project (`pip install -e
".[dev,formats,nist]"`), so mypy checks against real stubs. That surfaced 8
errors, now all fixed:
- report.py x3: add_axes is typed for a 4-tuple, not a list.
- plotting.py: histogram's `bins` accepted an ndarray, which is valid at
runtime but not per matplotlib's stub -- converted, and it no longer
shadows the parameter with the edges hist() returns.
- plotting.py x2: set_theta_* live on PolarAxes; plt.subplots is typed as
returning the base Axes. Narrowed with cast.
- data_tools.py x3: my own _safe_eval from f1302ae reused `op` across
branches and left the operator maps unannotated. These two CI *would*
have caught -- so the step was not useless, just nearly blind.
ruff and mypy are now pinned exactly in the dev extra; unpinned lint tools
mean CI can turn red with no code change, and dependabot's dev-tools group
already exists to bump them.
A-11 -- `pytest tests/` replaces the hand-written 15-file list, so
test_cli.py (27 tests) and test_integration.py (14) run for the first time.
A-12 -- 6-cell matrix: ubuntu x 3.9/3.10/3.11/3.12 plus macos x 3.12 and
windows x 3.11, with fail-fast disabled. Verified beforehand that all 75
source files parse under 3.9 syntax with no `X | Y` annotations or match
statements, so the 3.9 cells are plausible rather than aspirational.
macOS x 3.9 -- the exact G-010 environment -- is not in the matrix: macOS
runners are arm64 and actions/setup-python has no 3.9 build for them. Rather
than pin an Intel runner that GitHub is retiring, tests/test_gui_paths.py
covers it OS-independently: 13 tests asserting the GUI path constants stay
`.resolve()`-anchored (G-010), that Home.py routes only to page files that
exist, and that every packaged asset and gallery/example entry actually ships
(A-09). Both guards were mutation-tested -- removing `.resolve()` fails 2
tests, hiding one asset fails 1 -- so they are not vacuous.
A-38 -- added `permissions: contents: read`, a concurrency group, and pip
caching. docs.yml action versions bumped to match tests.yml, which is what
the four open dependabot PRs cover.
967 passed, 21 skipped. ruff, ruff format and mypy all clean with the
project installed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013YLgSnkyRUosy5sxWUb33P
Fixes the one red cell on PR #23. Six of seven were green; ubuntu x Python 3.9 failed in 16 seconds: ERROR: No matching distribution found for mypy==2.3.0; extra == "dev" (from versions: ... 1.18.2, 1.19.0, 1.19.1) This is my bug from 4930ae7, not a Python 3.9 incompatibility. mypy 2.3.0 requires Python >=3.10, and I had the test job installing `.[dev,formats,nist]` -- so the 3.9 cell died resolving lint tooling it never runs, before a single test executed. Notably the library itself was never the problem: pip had already resolved numpy, pandas, scipy, matplotlib, streamlit>=1.49 and tomli for 3.9 without complaint. TDEC-012's Python 3.9 floor stands. Added a `test` extra (pytest + pytest-cov only) and pointed the test matrix at `.[test,formats,nist]`. `dev` now includes `plottle[test]` and keeps the lint tools, so the lint job and contributors are unchanged. The underlying mistake was coupling the test matrix's dependency floor to the lint tools' own Python support. Comments on both the extra and the workflow step record that the matrix must install `[test,...]`, never `[dev,...]`. Verified in a clean venv that `[test,formats,nist]` resolves with no mypy, ruff, build, twine or types-* leaking in, and with pytest, pytest-cov, h5py, xarray, netcdf4, requests, streamlit and numpy all present. My first check of this used `pip --dry-run --report` in the working venv and was worthless -- that report lists only newly-installed packages, so everything already present read as missing. Clean venv, per G-018. 967 passed, 21 skipped locally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013YLgSnkyRUosy5sxWUb33P
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
This PR restructures the codebase from a
modules/package layout to a top-levelplottle/package, making it installable via pip with aplottleCLI command. It also replaces unsafeeval()-based formula evaluation with a hardened AST-based sandbox that prevents runtime escapes.Key Changes
Package Restructuring
modules/→plottle/throughout the codebasecli.pyintoplottle/cli.pyand made it a console script entry pointpyproject.tomlto declareplottleas the package with proper entry pointsmodules.*toplottle.*logo.png,assets/,example-data/,gallery/) inside theplottle/package so they ship with pip installsstreamlit run plottle/Home.pyFormula Evaluation Security (Audit A-02)
eval(expr, {"__builtins__": {}}, ns)with a custom_safe_eval()function inplottle/data_tools.py+,-,*,/,//,%,**+,-,not==,!=,<,<=,>,>=and,or_MAX_POW_EXPONENT = 1024,_MAX_EXPRESSION_LENGTH = 2000)ValueErrorwith descriptive messages for any disallowed constructSession Serialization Safety (Audit A-09)
_serialize_data()inplottle/utils/session_state.pyto_json(orient="split"){"__type__": "unsupported", "__class__": ..., "__reason__": ...}placeholder_deserialize_data()to reconstruct arrays from dtype/shape/bytes without picklePath Resolution Fixes (Audit G-010)
.resolve()to allPath(__file__)expressions in GUI pages to handle relative__file__understreamlit runtests/test_gui_paths.pyto prevent future path-handling defectsTesting & CI
modules.*toplottle.*TestFormulaExpressionSandbox)Documentation
plottlecommand instead ofpython cli.pyNotable Implementation Details
_eval()function that handles each node type explicitly, raisingValueErrorfor anything not whitelistedhttps://claude.ai/code/session_013YLgSnkyRUosy5sxWUb33P