Skip to content

Harden input handling, fix data-deletion and device bugs, add CI and tests - #131

Closed
beauregardhenry wants to merge 26 commits into
WISPR-lab:mainfrom
beauregardhenry:main
Closed

beauregardhenry wants to merge 26 commits into
WISPR-lab:mainfrom
beauregardhenry:main

Conversation

@beauregardhenry

Copy link
Copy Markdown

Summary

Hardens request and command handling, fixes several bugs in device handling and data deletion, adds CI with lint ratchets, and splits evidence_collection.py into smaller modules. The test suite goes from none to 398 tests.

This is a large PR. Each section below maps to a group of commits, so it can be reviewed by theme. I can split it into separate PRs if you prefer.

Motivation

Sherloc handles sensitive client data in a clinic setting, so the failure modes matter more than in a typical Flask app. Three problems stood out:

  • "Delete client data" removed files but left client notes and scan rows in fieldstudy.db.
  • Device serials and app ids reached shell commands without validation.
  • Several routes and buttons were broken or unsafe (/view_results, "Close App", uninstall on multi-device setups).

Changes

Security

  • Validate device serials and app ids before they reach a shell command (sherloc/inputcheck.py), and fix the quoting in the Android and iOS uninstall commands.
  • Reject requests with a foreign Host, a cross-origin Origin on state-changing methods, or a non-same-origin Sec-Fetch-Site. The app binds to 127.0.0.1 by default. SHERLOC_HOST and SHERLOC_ALLOWED_HOSTS change this.
  • Untrack pii.key and flask.secret. Keys are generated on first use with 0600 permissions. A key file that still holds a previously published value is replaced.
  • Escape user text in the PDF printout and turn off local file access in wkhtmltopdf.
  • Reduce app-store descriptions to a short allow-list of formatting tags before rendering.
  • Delete, kill and delete-app routes are POST-only. The buttons are now forms.
  • Limit screenshot paths to the screenshots directory.
  • Request log lines show the route pattern, not the URL. SQL echo is off by default (SHERLOC_SQL_ECHO=1 turns it on).

Bug fixes

  • "Delete client data" now also empties the database, overwrites deleted content and compacts the file. It also no longer stops at the first missing folder.
  • Android and iOS uninstall target the scanned device (adb -s, ideviceinstaller --udid). delete_app checks that the serial matches the scan.
  • iOS screenshots work when the install path has a space. The pymobiledevice3 tunnel is stopped after each screenshot.
  • "Close App and End Session" works again. It relied on werkzeug.server.shutdown, which Werkzeug 2.1 removed.
  • /view_results no longer raises NameError. Dead code that referenced undefined names is removed.
  • python main.py test now enters test mode. The argument is applied before config is imported.
  • A missing blocklist file now exits with an error instead of status 0.
  • 30 mutable default arguments, 15 invalid escape sequences and one bare except are fixed.

Refactors (no behavior change)

  • config no longer creates files or folders on import.
  • Removed the duplicate run_command. The remaining one is run_capture.
  • Split evidence_collection.py (1,942 lines) into evidence_choices.py, evidence_model.py and evidence_forms.py. evidence_collection re-exports every public name, and a test pins that API.

CI

  • tests.yml runs pytest on pushes and pull requests.
  • tests/test_lint_ratchet.py fails on new undefined names, invalid escapes, mutable defaults, bare except, shell=True and eval. Existing findings are recorded in tests/lint_baseline.json (2 remain).
  • super-linter checks changed files for serious flake8 errors and executable bits. The style linters were failing on about 170 existing findings.
  • All GitHub Actions are pinned to commit hashes, with a test that enforces it.
  • The stalkerware-indicators workflow had a duplicate token: key that made IOC_UPDATE_KEY ignored. It no longer tries to open a PR on pull request runs.

Breaking changes

  • Existing installs get new keys on pull. Stored HSN_ device identifiers will change. The old key values remain in git history.
  • Delete and kill endpoints no longer accept GET. Anything that called them with a link or fetch GET must use POST.
  • Sherloc listens on 127.0.0.1 by default. Anyone serving it to other machines must set SHERLOC_HOST and SHERLOC_ALLOWED_HOSTS.
  • The serial validator is an allow-list. A real device with an unusual serial (an IPv6 adb target, for example) may be rejected.

Testing

  • pytest from the repo root: 398 passed, 1 skipped (needs exiftool).
  • Bug fixes and security changes were written test-first and confirmed failing before the fix.
  • I did not click through the UI in a browser, and I did not test a real iOS uninstall. --udid is taken from the ideviceinstaller man page. Please check both if you have a device.
  • The iOS tunnel device selection (start-tunnel) is unverified.

Checklist

  • Tests added or updated
  • CHANGELOG.md updated under [Unreleased]
  • CI passes (pytest, run-lint)
  • No new lint findings against the baseline
  • Manual browser check of the delete buttons and "Close App" (maintainer or reviewer with a device)

claude and others added 26 commits October 1, 2026 09:48
Add pytest.ini (pythonpath, testpaths) and tests/conftest.py, which sets the
working directory to sherloc/ and skips test_parse_dump.py when its gitignored
phone dump fixture is absent. `pytest` now collects and passes 8 tests with no
PYTHONPATH or cwd setup. Ignore the runtime-generated app-info.db.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XhPxfKpkaybwKYS3YTNVHn
Serials and app ids reached shell=True command lines unquoted or with
ineffective quoting. Reachable through POST /scan (devid), /delete/app
(appid), and GET /privacy/... (ser), with no authentication and a default
bind to 0.0.0.0. A device could also report a hostile serial.

- Add inputcheck with strict validators for serials, app ids, and path parts
- Validate at the scanner methods, the routes, and device listing
- Remove the `!r` that re-enabled $(...) expansion after shlex.quote
- Constrain screenshot paths to the screenshots directory
- Add request guards: Host allow-list (DNS rebinding), Sec-Fetch-Site and
  Origin checks (cross-site requests, including GET /evidence/delete-data)
- Listen on 127.0.0.1 by default (SHERLOC_HOST to override)

Tests were written first and failed on the old code (70 failures when the old
scanner module is restored); 160 pass now.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XhPxfKpkaybwKYS3YTNVHn
pii.key and flask.secret were committed to the public repo, so anyone
could compute HSN_ device identifiers and forge session cookies.

- untrack both files and add the key paths to .gitignore
- create key files with 0600 permissions
- replace a key file that is the wrong length or matches the hash of a
  previously published key (only hashes are stored)
- tests: no key material tracked, key paths ignored, key creation rules

Existing installs get new keys after pulling; stored HSN_ ids change.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XhPxfKpkaybwKYS3YTNVHn
The printout was rendered with Jinja autoescape off and wkhtmltopdf had
enable-local-file-access, so free text such as a note containing an
iframe with a file:// URL put a local file's contents into the report.

- render with autoescape=True; split out render_printout_html and
  printout_pdf_options so they can be tested
- remove enable-local-file-access (images load over http via url_root)
- find wkhtmltopdf on PATH before the hard-coded macOS path
- tests, including an end-to-end PDF check that fails on the old code

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XhPxfKpkaybwKYS3YTNVHn
- tests.yml runs pytest (with wkhtmltopdf and poppler for the printout test)
- tests/test_lint_ratchet.py fails on new F821, W605, B006, E722, S602 and
  S307 findings; existing ones are baselined per file and rule
- IOC workflow: paths now under sherloc/, one token key (the duplicate
  made IOC_UPDATE_KEY dead), script exits 1 when requirements are missing

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XhPxfKpkaybwKYS3YTNVHn
- super-linter: changed files only, flake8 for serious errors plus the
  bash exec-bit check; style linters off (existing code has ~170 findings)
- chmod +x three shell scripts
- IOC workflow: write permissions, skip PR creation on pull_request runs,
  drop upstream-maintainer assignees/reviewers
- move the request-guard import to the top of web/__init__.py (E402)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XhPxfKpkaybwKYS3YTNVHn
delete_client_data removed files but left clients_notes, scan_res and
app_info rows in fieldstudy.db. It now deletes every row with
secure_delete on, then checkpoints and VACUUMs so the text is not left
in free pages. The schema is kept.

SQLALCHEMY_ECHO was always on and logged client notes and serials;
it is now off unless SHERLOC_SQL_ECHO=1.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XhPxfKpkaybwKYS3YTNVHn
- /view_results raised NameError (last_serial) on any existing scan; fixed
- delete unreachable code in index.py and uncalled functions that used
  undefined names (android_permissions.gather_permissions_labels,
  db.update_app_deleteinfo, which also had an SQL typo)
- ratchet baseline lowered; no F821 findings remain
- flake8 in super-linter now selects undefined names (F821-F823)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XhPxfKpkaybwKYS3YTNVHn
Stacked PRs target a feature branch, so the branches: [main] filter meant
no checks ran on them.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XhPxfKpkaybwKYS3YTNVHn
Delete client data from the database; fix undefined-name bugs (carries #2 to main)
- app descriptions (Play Store / App Store crawl text) were rendered with
  |safe; they now pass through an allow-list cleaner (htmlclean.py)
- /details/app validates app id and serial (the serial reaches a file
  path) and 404s on an unknown device
- delete-data, delete scan, delete account, delete app and /kill accept
  POST only; the buttons are forms; no template links to them
- /kill used werkzeug.server.shutdown, removed in Werkzeug 2.1, so it
  always failed; it now interrupts the process after responding
- app ids go into onclick handlers via tojson
- tests first; web guard tests moved from GET to POST

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XhPxfKpkaybwKYS3YTNVHn
Sanitize crawled descriptions; make destructive routes POST-only
- Android uninstall passes -s <serial>, iOS passes --udid <serial>
  (flag per the ideviceinstaller man page); both validate the serial
- delete_app takes the real serial from the page and accepts it only if
  its HMAC matches the serial stored for the scan (the DB holds only HSN_)
- iosScreenshot passes an argument list (paths with spaces), strips the
  RSD values, and always terminates the start-tunnel process
- tests first

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XhPxfKpkaybwKYS3YTNVHn
Target the scanned device on uninstall; fix iOS screenshot process
Request lines included paths and query strings, which carry device
serials and app ids, and went to the terminal. They now show the matched
route pattern ("-" when nothing matched).

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XhPxfKpkaybwKYS3YTNVHn
Log the route pattern instead of the request URL
- replace 30 mutable default arguments with None and a fresh value in the
  body (mechanical; a loop variable named `dict` in AccountInvestigation
  would have broken it, so it is renamed; a test now builds every data
  class with no arguments)
- raw-string the 15 invalid escape sequences (AST verified identical)
- bare except in /scan -> except ValueError
- `main.py test` now sets TEST before config is imported (runmode.py);
  the old call discarded set_test_mode's return value
- missing blocklist exits with an error instead of status 0
- ratchet baseline now holds only the two shell=True calls in run_command

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XhPxfKpkaybwKYS3YTNVHn
A tag can be moved after review. Hashes were read from each action's
repository (git ls-remote): checkout v4, setup-python v5,
create-pull-request v5, super-linter v4.10.0 (the version the lint job
already pulls). A test fails on any unpinned action.

checkout v3 and setup-python v3 in the indicators workflow move to v4/v5.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XhPxfKpkaybwKYS3YTNVHn
Lint cleanup; fix test-mode flag and blocklist exit
Pin GitHub Actions to full commit hashes
- PII_KEY and FLASK_SECRET are created on first use (module __getattr__),
  and the reports folder in config.ensure_dirs() at app start. Importing
  config creates nothing, so scripts and tests no longer write key files
- delete_client_data ignores missing folders, so it always reaches the
  database wipe (it used to raise on a missing phone_dumps folder)
- rename the second run_command (returns text, not a process) to
  run_capture to end the name clash
- tests first

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XhPxfKpkaybwKYS3YTNVHn
…ommand

Remove import-time side effects from config; make delete robust
The module held constants, 40 data classes, 40 forms and the file and
PDF functions in 1,900 lines. Constants, data classes and forms move to
their own modules. evidence_collection re-exports every name, and a test
pins the public API so no import breaks.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XhPxfKpkaybwKYS3YTNVHn
…ollection

Split evidence_collection.py into choices, model and forms
@sophiestephenson

Copy link
Copy Markdown
Collaborator

Hi @beauregardhenry. Thank you for submitting this PR. I see that Claude has made most of these commits, which raises warning signs for me. Before I review the changes, I would like to know a little more about why you are submitting this PR and what motivated the specific changes that have been made. Please describe this in a comment. Thank you.

@beauregardhenry

Copy link
Copy Markdown
Author

Fair. Please disregard and think no more about it.

I intended to contribute to Sherloc, a separate project with a different aim.

Love the work that you've done.

Please accept my apology for making a mistake.

Sincerely,
Beau

@rchatterjee rchatterjee closed this Oct 7, 2026
@beauregardhenry

Copy link
Copy Markdown
Author

I saw that your project was initially based on ISDi. I have looked at ISDi and created my own fork which goes much further than the original project. I believe your project would benefit from a CI/CD pipeline, hardening measures, refactoring, test coverage, and test ratchets. That was the intent. The area where I live needs projects like Sherloc, and I wanted to contribute meaningfully. I may have gone about it wrongly. I will continue working on my fork of your project. If at any time you would like to consider merging some of what I do, please let me know and I would be happy to work together. I do not remember the acronym at the moment, but technology enabled abuse needs to end. I would enjoy contributing to any project that uses technology to catch abusers and provide evidence that strengthens cases for those who stand up.

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.

4 participants