Expand test coverage (35%→90%) and fix data-flow security issues - #3
Open
sukritphiboon wants to merge 5 commits into
Open
sukritphiboon wants to merge 5 commits into
sukritphiboon wants to merge 5 commits into
Conversation
Raise source coverage from ~35% to ~89% by filling the biggest gaps: - fc_client: login retry loop (version/port fallbacks), token extraction, pagination (_get_all), URL construction and getter key-fallbacks, all driven by a fake requests.Session (no live VRM needed). - app: Flask routes (collect/progress/cancel/download/update-check/ changelog), the background _run_collection thread, run_headless CLI (password resolution + exit codes) and arg parsing via test_client. - collector: remaining sheet builders (vSummary/vCPU/vMemory/vNetwork/ vHost/vCluster/vDatastore/vSwitch), flatten/build_row helpers, cancellation, and a fully mocked collect_all pipeline. - version_utils: get_latest_release success/missing-keys/error paths. - Add an end-to-end collect -> Excel -> reload integration test. Wire pytest-cov into requirements-dev and enforce --cov-fail-under=80 in CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wq8npTDW4qxQ8RFvUZWtm1
FusionCompute can return either a wrapper dict or a bare list depending on version/endpoint. The list getters called data.get(...) before the isinstance(data, list) fallback, so a top-level list raised AttributeError and the fallback branch was dead code. Add a shared _extract_list() helper and route get_sites, get_vm_nics, get_vm_disks, get_dvswitches, get_portgroups, get_site_portgroups and _get_all through it. Covered by new list-fallback tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wq8npTDW4qxQ8RFvUZWtm1
Inventory values (e.g. VM name/description) originate from FusionCompute and could be attacker-controlled. openpyxl writes a string beginning with '=' as a live formula, so a crafted value like =HYPERLINK(...) would execute when the operator opens the workbook (CWE-1236, CSV/Excel injection). Prefix string cells beginning with = + - @ with a single quote so they render as inert text. Covered by new tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wq8npTDW4qxQ8RFvUZWtm1
TLS verification was hard-disabled (verify=False), exposing credentials to man-in-the-middle attacks. Keep that as the default for self-signed VRM certs, but let operators opt in via FC_INVENTORY_VERIFY_SSL or point at their own trust chain with FC_INVENTORY_CA_BUNDLE. The insecure-request warning is now only suppressed when verification is actually off. Documented and tested. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wq8npTDW4qxQ8RFvUZWtm1
…token logs - Reject state-changing requests whose browser Origin does not match the request host. The tool is unauthenticated, so this blocks CSRF / drive-by POSTs from malicious pages while leaving curl/CLI clients unaffected. - Validate the collect port (numeric, 1-65535) instead of letting a bad value raise deep in the handler. - Stop logging the login response body on success so the session token is never written to fc_inventory.log. Covered by new tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wq8npTDW4qxQ8RFvUZWtm1
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 started as a test-coverage analysis and grew to fix the bugs and vulnerabilities the new tests surfaced. Source coverage rises from ~35% to ~90% (16 → 119 tests), and CI now enforces a coverage floor.
Test coverage
fc_client.pyapp.pycollector.pyexcel_builder.pyversion_utils.pyNew test files:
test_fc_client.py,test_app.py,test_collector_builders.py,test_integration.py,test_version_utils_network.py(all network/Flask I/O faked — no live VRM needed).pytest-covis added torequirements-dev.txtand CI runs--cov-fail-under=80(currently ~95%).Bug fix
fc_clientlist getters threw on top-level list responses.get_sites,get_vm_nics,get_vm_disks,get_dvswitches,get_portgroups,get_site_portgroupsand_get_allcalleddata.get(...)before theisinstance(data, list)fallback, so a bare-list response raisedAttributeErrorand the fallback was dead code. Centralized into a shared_extract_list()helper.Security fixes
=HYPERLINK(...)) were written as live formulas and would execute when the operator opens the export= + - @verify=Falsedefault for self-signed VRM certs, but allow opt-in viaFC_INVENTORY_VERIFY_SSL/FC_INVENTORY_CA_BUNDLE; only suppress the insecure-request warning when verification is off0.0.0.0)Origindoesn't match the request host; curl/CLI clients (no Origin) unaffectedportis numeric and in1–65535Notes / not addressed (would be larger changes — happy to follow up)
FC_INVENTORY_BIND=0.0.0.0should be done behind an authenticating reverse proxy (already noted in the README). Because the legitimate VRM target is itself on an internal network, IP-range SSRF filtering would break normal use — proper access control is the right fix.login()still attempts a plaintext-password method first (kept to preserve compatibility across FC versions); this is now mitigated by opt-in TLS verification.Test plan
pytest -q --cov=. --cov-fail-under=80→ 119 passed, ~95% coverage.🤖 Generated with Claude Code
Generated by Claude Code