Fix/export sandbox security - #459
Conversation
📝 WalkthroughWalkthroughThe PR adds fail-closed Chromium behavior, scheduled dependency audits, local documentation checks, and reflective public API validation. It also documents view, selection, callback, and security behavior and updates related tests. ChangesSecurity controls
Documentation quality
Public API validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR makes Chromium exports fail closed instead of retrying without sandboxing, and adds scheduled dependency audits plus committed-lockfile inventory validation.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains, and the previous filesystem-scan issue is resolved because inventory discovery now considers only Git-tracked files.
|
| Filename | Overview |
|---|---|
| python/xy/export.py | Removes automatic unsandboxed Chromium fallback while preserving explicit sandbox=False behavior. |
| scripts/verify_dependency_lock_inventory.py | Uses Git’s tracked-file inventory, resolving the prior false failures caused by gitignored or untracked local lockfiles. |
| .github/workflows/security-audit.yml | Adds scheduled and lockfile-triggered audits across the repository’s dependency ecosystems. |
| scripts/verify_local.py | Registers dependency lock inventory validation with the local verification harness. |
| tests/test_figure.py | Updates single-shot Chromium export coverage to require fail-closed sandbox behavior. |
| tests/test_image_export.py | Adds persistent-session coverage ensuring sandbox launch failures are not retried unsandboxed. |
Reviews (4): Last reviewed commit: "Harden lock inventory error handling" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
scripts/check_public_api.py (3)
222-227: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reporting annotation resolution failures.
_return_type_namereturnsNonefor every exception. An export with an unresolvable return annotation is then reported only as "unclassified", which hides the real cause.validate_public_api_inventorystill fails, so the checker does not pass silently. Surfacing the original exception would shorten diagnosis.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check_public_api.py` around lines 222 - 227, Update _return_type_name to report annotation-resolution exceptions while preserving its existing None return behavior, including the original exception details in the diagnostic output so unresolvable return annotations can be diagnosed.
340-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the Selection dunder alias map next to its discovery constant.
SPECIAL_PUBLIC_SELECTION_METHODSat Line 55 decides which dunders enter the inventory.selection_aliasesat Line 373 decides how each dunder is matched in the documentation. The two lists must stay in agreement. If a second dunder joins the public Selection surface and only Line 55 is updated,validate_docs_inventorydemands a literal`__name__`token and reports a missing document entry even when the documentation uses the idiomatic form.Define the alias map as a module constant beside Line 55 and derive the special-method set from its keys.
Separately, note that
_has_doc_referencematches bare substrings such asrows(. An incidental mention in the documentation satisfies the check. The consequence is missed drift, not a false failure.♻️ Proposed single source for the alias map
-SPECIAL_PUBLIC_SELECTION_METHODS = {"__len__"} +SELECTION_METHOD_DOC_ALIASES = {"__len__": "len(selection)"} +SPECIAL_PUBLIC_SELECTION_METHODS = set(SELECTION_METHOD_DOC_ALIASES)- selection_aliases = {"__len__": "len(selection)"} for method in inventory.selection_methods: - token = selection_aliases.get(method) + token = SELECTION_METHOD_DOC_ALIASES.get(method)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check_public_api.py` around lines 340 - 381, Move the Selection dunder alias mapping beside SPECIAL_PUBLIC_SELECTION_METHODS, derive that set from the alias-map keys, and update validate_docs_inventory to reuse the module-level map. Also tighten _has_doc_reference so bare method references such as rows( require an actual documentation reference rather than matching incidental substrings.
261-275: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
_public_methodsmisses inherited public methods.The loop reads only
cls.__dict__. Inherited public methods are not discovered. TodayChartandSelectiondefine their public methods locally, so the inventory is complete. If either class later gains a base class that carries public methods, the inventory under-reports the surface.validate_docs_inventorythen stops requiring documentation for those methods, which defeats the drift check.Consider walking the MRO and excluding
object.♻️ Proposed MRO-aware collection
def _public_methods( cls: type[Any], *, special_public_methods: set[str], ) -> tuple[str, ...]: methods: list[str] = [] - for name, value in cls.__dict__.items(): - if name == "__init__": - continue - if not callable(value) and not isinstance(value, property): - continue - if name.startswith("_") and name not in special_public_methods: - continue - methods.append(name) + seen: set[str] = set() + for klass in cls.__mro__: + if klass is object: + continue + for name, value in klass.__dict__.items(): + if name == "__init__" or name in seen: + continue + if not callable(value) and not isinstance(value, property): + continue + if name.startswith("_") and name not in special_public_methods: + continue + seen.add(name) + methods.append(name) return tuple(methods)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check_public_api.py` around lines 261 - 275, Update `_public_methods` to collect public callables from the class MRO instead of only `cls.__dict__`, while still skipping `object` and preserving the existing `__init__` and underscore/special-public filtering. Use the same `special_public_methods` handling when walking base classes so inherited methods on classes like `Chart` and `Selection` are included in the inventory and still flow correctly into `validate_docs_inventory`.tests/test_public_api.py (2)
293-320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe fake modules do not isolate this test from the real package.
fakeis named"xy".build_public_api_inventoryrunsimportlib.import_module("._figure", pkg.__name__)atscripts/check_public_api.pyLine 298, so it imports the realxy._figureand reflects the realSelection.validate_declarative_api_contractthen callsvalidate_docs_inventory(inventory)at Line 441 with the defaultCHART_METHOD_DOCandSELECTION_METHOD_DOCpaths, so it reads the real documentation files.The effect:
test_public_api_checker_accepts_declarative_api_contractassertserrors == [], and that assertion depends on realSelectionmethods and real documentation content. It passes now only because the fakeChart.figureandChart.htmlnames also appear indocs/api-reference/figure-methods.md. If a public method is added to the realSelectionwithout documentation, this fixture-based test fails with aSelectionerror that names neither the fixture nor the real cause.Accept the Selection source and the documentation paths as parameters of
build_public_api_inventoryandvalidate_declarative_api_contract, then inject fakes here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_public_api.py` around lines 293 - 320, Parameterize build_public_api_inventory and validate_declarative_api_contract to accept the Selection source and chart/selection documentation paths instead of relying on real package modules and default constants. Update this fixture to provide fake Selection data and temporary or fixture-specific documentation paths, ensuring test_public_api_checker_accepts_declarative_api_contract validates only the constructed fake API.
385-398: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the
__len__alias branch.
selection_methodsincludes__len__, and the temporary document containslen(selection). No assertion covers that pairing. If theselection_aliasesmapping invalidate_docs_inventorywere removed,__len__would fall through to_has_doc_reference, none of whose tokens appear in the written text, and an error would be reported. Every current assertion would still pass, because none mentions__len__. Add the negative assertion to pin the alias behavior.💚 Proposed assertion
assert not any("visible_method" in error for error in errors) assert not any("'rows'" in error for error in errors) + assert not any("__len__" in error for error in errors)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_public_api.py` around lines 385 - 398, Add an assertion in validate_docs_inventory coverage to explicitly verify the __len__ alias path in selection_methods. Use the existing check_public_api.validate_docs_inventory call and the current selection_doc setup, and assert that no error entry contains "__len__" so the selection_aliases mapping remains required and the alias continues to resolve through len(selection).tests/test_type_surface.py (1)
31-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeriving the expected inventory from the code under test removes the removal signal.
MARK_FACTORIES,CHART_FACTORIES,CHROME_FACTORIES,SUPPORT_FACTORIES, andCHART_READOUTSnow come from reflection overcomponents.__all__and theChartclass. The previous hard-coded lists acted as an independent expectation.Concrete consequence: if a factory is dropped from
components.__all__, it also disappears from the derived tuple.test_public_factories_are_typed_root_exportsandtest_composition_alpha_contract_is_explicitly_exportedthen iterate fewer names and still pass.validate_public_api_inventoryinscripts/check_public_api.pydoes not close this gap, because it only checks that each listed export is classified.Keep one independent expectation. A minimum-count assertion, or an explicit frozen set of names that must always be present, restores the signal without reintroducing full hard-coded lists.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_type_surface.py` around lines 31 - 42, Restore an independent expectation in tests/test_type_surface.py for the reflected public API inventory: add either minimum-count assertions or a frozen required-name set covering MARK_FACTORIES, CHART_FACTORIES, CHROME_FACTORIES, SUPPORT_FACTORIES, and CHART_READOUTS. Ensure tests such as test_public_factories_are_typed_root_exports and test_composition_alpha_contract_is_explicitly_exported still fail when an expected export is removed from components.__all__ or Chart.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/security-audit.yml:
- Around line 27-29: Update the actions/checkout step in the security-audit
workflow to set persist-credentials to false alongside fetch-depth, ensuring the
workflow token is not retained before dependency installation.
- Line 65: Replace the unpinned npm-based Bun installation in the security-audit
workflow with an integrity-pinned bootstrap: use a setup action pinned to a
commit SHA, or download the official Bun release and verify its checksum before
execution. Preserve the required Bun version while ensuring the installed binary
is independently authenticated.
In `@docs/api-reference/figure-methods.md`:
- Around line 109-126: Update the select() entry in the “View and Selection
State” documentation to describe Chart.select’s keyword-only range, polygon,
rows, and history parameters, removing the unsupported positional selection and
source arguments. Revise the accompanying description to reflect applying these
supported selection options through the chart state path.
In `@scripts/check_public_api.py`:
- Around line 437-441: Update the validation loop over inventory.chart_methods
to accept property descriptors as valid readouts, matching the property handling
already allowed by _public_methods, while continuing to require other entries to
be callable. Preserve the existing error for names that are neither callable nor
properties.
In `@scripts/verify_dependency_lock_inventory.py`:
- Around line 34-37: Update the lockfile discovery loop in the inventory
function to use os.walk with top-down traversal, removing names in
EXCLUDED_DIRECTORIES from dirnames before descending. Preserve the existing file
checks and _is_lockfile filtering for traversed files, while avoiding traversal
into excluded directories such as node_modules, .venv, and target.
In `@scripts/verify_docs_local.py`:
- Around line 19-60: Update the local docs setup or verification flow around the
existing docs/app commands to run “uv sync --project docs/app --frozen --group
dev” before any --no-sync checks, ensuring the docs environment is synchronized
without changing the subsequent test and lint commands.
---
Nitpick comments:
In `@scripts/check_public_api.py`:
- Around line 222-227: Update _return_type_name to report annotation-resolution
exceptions while preserving its existing None return behavior, including the
original exception details in the diagnostic output so unresolvable return
annotations can be diagnosed.
- Around line 340-381: Move the Selection dunder alias mapping beside
SPECIAL_PUBLIC_SELECTION_METHODS, derive that set from the alias-map keys, and
update validate_docs_inventory to reuse the module-level map. Also tighten
_has_doc_reference so bare method references such as rows( require an actual
documentation reference rather than matching incidental substrings.
- Around line 261-275: Update `_public_methods` to collect public callables from
the class MRO instead of only `cls.__dict__`, while still skipping `object` and
preserving the existing `__init__` and underscore/special-public filtering. Use
the same `special_public_methods` handling when walking base classes so
inherited methods on classes like `Chart` and `Selection` are included in the
inventory and still flow correctly into `validate_docs_inventory`.
In `@tests/test_public_api.py`:
- Around line 293-320: Parameterize build_public_api_inventory and
validate_declarative_api_contract to accept the Selection source and
chart/selection documentation paths instead of relying on real package modules
and default constants. Update this fixture to provide fake Selection data and
temporary or fixture-specific documentation paths, ensuring
test_public_api_checker_accepts_declarative_api_contract validates only the
constructed fake API.
- Around line 385-398: Add an assertion in validate_docs_inventory coverage to
explicitly verify the __len__ alias path in selection_methods. Use the existing
check_public_api.validate_docs_inventory call and the current selection_doc
setup, and assert that no error entry contains "__len__" so the
selection_aliases mapping remains required and the alias continues to resolve
through len(selection).
In `@tests/test_type_surface.py`:
- Around line 31-42: Restore an independent expectation in
tests/test_type_surface.py for the reflected public API inventory: add either
minimum-count assertions or a frozen required-name set covering MARK_FACTORIES,
CHART_FACTORIES, CHROME_FACTORIES, SUPPORT_FACTORIES, and CHART_READOUTS. Ensure
tests such as test_public_factories_are_typed_root_exports and
test_composition_alpha_contract_is_explicitly_exported still fail when an
expected export is removed from components.__all__ or Chart.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f22a4bd-7ebe-48f2-9af0-7dfe2c809cca
📒 Files selected for processing (16)
.github/workflows/security-audit.ymlMakefileSECURITY.mddocs/api-reference/events-and-callbacks.mddocs/api-reference/figure-methods.mdpython/xy/export.pyscripts/check_public_api.pyscripts/verify_dependency_lock_inventory.pyscripts/verify_docs_local.pyscripts/verify_local.pyspec/process/production-readiness.mdspec/process/security-audit-2026-07-06.mdtests/test_figure.pytests/test_image_export.pytests/test_public_api.pytests/test_type_surface.py
|
|
||
| - name: Audit Bun lock | ||
| run: | | ||
| npm install --global bun@1.2.20 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file=".github/workflows/security-audit.yml"
printf '%s\n' '--- workflow excerpt ---'
sed -n '1,110p' "$file"
printf '%s\n' '--- related workflow references ---'
rg -n -C 2 'bun|npm install|audit|workflow_dispatch|pull_request|push|permissions|runs-on' .github/workflows "$file" 2>/dev/null || trueRepository: reflex-dev/xy
Length of output: 43325
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import json, urllib.request
url = "https://registry.npmjs.org/bun/1.2.20"
with urllib.request.urlopen(url, timeout=15) as r:
data = json.load(r)
print("name:", data.get("name"))
print("version:", data.get("version"))
print("scripts:", json.dumps(data.get("scripts", {}), sort_keys=True))
print("dist:", json.dumps({
k: data.get("dist", {}).get(k)
for k in ("tarball", "integrity", "shasum")
}, sort_keys=True))
PYRepository: reflex-dev/xy
Length of output: 2437
🌐 Web query:
npm bun 1.2.20 package.json scripts postinstall dist integrity
💡 Result:
In Bun, including version 1.2.20, the handling of package.json scripts like postinstall is distinct from npm due to a "default-secure" security model [1][2][3]. 1. Default Security Behavior: Bun does not execute arbitrary lifecycle scripts (such as postinstall, preinstall, or prepare) for installed dependencies by default [1][2][3]. This is done to mitigate the security risks associated with running unknown code during package installation [1][2][3]. 2. Opt-in via trustedDependencies: To run lifecycle scripts for specific packages, you must explicitly allow them by adding the package name to the trustedDependencies array in your root package.json [1][2][3]. Once added, Bun will execute the lifecycle scripts for those packages upon installation [1][2][3]. 3. Project-Level Scripts: Bun does execute lifecycle scripts defined in your own project's package.json at the appropriate time when you run commands like bun install [1][4]. 4. Integrity and dist: Bun verifies the integrity of downloaded packages [1][4]. Recent updates, including those relevant to the 1.2.x series, have improved how Bun handles and stores integrity hashes (such as sha512) in the lockfile for various dependency types, including remote tarballs, to ensure consistency and security [5]. 5. Dist/Optimization: Bun automatically optimizes postinstall scripts for certain popular packages (e.g., esbuild, sharp) by determining if they are necessary, though this behavior can be configured or disabled using specific environment variables like BUN_FEATURE_FLAG_DISABLE_NATIVE_DEPENDENCY_LINKER [1][4]. For your specific scenario, if you are expecting a postinstall script to run for a dependency and it is not, ensure the package is listed in your package.json's trustedDependencies [1][2]. Integrity verification is handled automatically by Bun and is recorded in the lockfile to maintain security [1][4][5].
Citations:
- 1: https://bun.sh/docs/pm/cli/install
- 2: https://bun.com/docs/pm/lifecycle
- 3: https://bun.sh/docs/pm/lifecycle
- 4: https://bun.com/docs/pm/cli/install
- 5: fix(install): store tarball integrity hash in lockfile for HTTPS dependencies oven-sh/bun#27018
🏁 Script executed:
#!/bin/bash
set -eu
json="$(mktemp)"
trap 'rm -f "$json"' EXIT
curl -fsSLk --max-time 20 'https://registry.npmjs.org/bun/1.2.20' -o "$json"
python3 - "$json" <<'PY'
import json, sys
with open(sys.argv[1]) as f:
data = json.load(f)
print("name:", data.get("name"))
print("version:", data.get("version"))
print("scripts:", json.dumps(data.get("scripts", {}), sort_keys=True))
print("dist:", json.dumps({
k: data.get("dist", {}).get(k)
for k in ("tarball", "integrity", "shasum")
}, sort_keys=True))
PYRepository: reflex-dev/xy
Length of output: 451
🏁 Script executed:
#!/bin/bash
set -eu
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
curl -fsSLk --max-time 30 'https://registry.npmjs.org/bun/-/bun-1.2.20.tgz' -o "$tmp/bun.tgz"
tar -tzf "$tmp/bun.tgz" | sed -n '1,80p'
tar -xzf "$tmp/bun.tgz" -C "$tmp"
printf '%s\n' '--- package manifest ---'
cat "$tmp/package/package.json"
printf '%s\n' '--- install.js (first 240 lines) ---'
sed -n '1,240p' "$tmp/package/install.js"Repository: reflex-dev/xy
Length of output: 9695
🏁 Script executed:
#!/bin/bash
set -eu
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
curl -fsSLk --max-time 30 'https://registry.npmjs.org/bun/-/bun-1.2.20.tgz' -o "$tmp/bun.tgz"
tar -xzf "$tmp/bun.tgz" -C "$tmp"
script="$tmp/package/install.js"
printf '%s\n' '--- installer security-relevant references ---'
rg -n -i 'sha|hash|checksum|integrity|download|fetch|exec|spawn|child_process|https?://|optional|binary|rename|chmod|writeFile|bun' "$script"
printf '%s\n' '--- installer tail ---'
tail -n 180 "$script"Repository: reflex-dev/xy
Length of output: 10718
Security Misconfiguration (CWE-494): Download of Code Without Integrity Check
Reachability: External · Exploitability: Difficult
Use an integrity-pinned Bun bootstrap.
npm install --global bun@1.2.20 runs the package’s postinstall script. That script installs or downloads the platform Bun binary without an independently pinned checksum. Use a commit-SHA-pinned setup action or verify an official release checksum before execution.
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 65-65: ad-hoc installation of packages (adhoc-packages): installs a package outside of a lockfile
(adhoc-packages)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/security-audit.yml at line 65, Replace the unpinned
npm-based Bun installation in the security-audit workflow with an
integrity-pinned bootstrap: use a setup action pinned to a commit SHA, or
download the official Bun release and verify its checksum before execution.
Preserve the required Bun version while ensuring the installed binary is
independently authenticated.
Source: Linters/SAST tools
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 16 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
7c236fa to
e6b55d7
Compare
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
1 participant
Closses #449
Summary
Testing
Summary by CodeRabbit
Security
New Features
Selection.rows()documentation for retrieving selected rows with an optional limit.Documentation
Quality Improvements