Repository navigation
Conversation
|
|
@greptileai review |
|
@TimeToBuildBob The watcher-side privacy filter should also be configurable via webui (non-technical user-facing central config) similar to the server-side privacy filter (but watcher-only config should also be supported, if e.g. server is untrusted). |
cdf5c3b to
9176770
Compare
|
Good point. The TOML config in this PR covers the watcher-only path you described (useful when the server is untrusted or the user wants filtering before data ever leaves the machine). For webui configurability, the shape I'd expect is:
That touches aw-server and aw-webui (separate repos), so it's naturally a follow-up rather than something that fits cleanly into this PR. Would you prefer I scope it here (which would expand the PR significantly) or open a follow-up issue to track it? Also resolved the rebase conflict with master (#136, #137) — the branch is up to date now. |
codeql-action v2 is deprecated as of 2025-01-10 (GitHub changelog). Update all three action steps (init, autobuild, analyze) to v3. Also bump actions/checkout from v3 to v4 while here. This fixes the failing 'Analyze (python)' CI check on PR ActivityWatch#135.
|
Still waiting for direction on scope — whether to include the webui-configurable path in this PR or track it as a follow-up. The watcher-only TOML config path is ready, CI green, and rebased. Happy to expand or split depending on your preference. |
|
CI-green and mergeable (Greptile 5/5) — waiting only on a maintainer click. This PR is ready to merge, but the bot has pull-only access to this repo and can't self-merge — surfacing it here so it isn't lost. The monitoring loop will stop re-flagging it now that this note is posted. |
🤖 AI code reviewSafe to merge — 3 findings disposed (fixed in 1d68a6b; rejected)Updated after inline dispositions on finding threads — this is the current state; the verdict below is frozen at review time and is kept as the historical record of that pass. Finding disposition
Adds a watcher-side privacy filter that drops or redacts window events before they are sent to aw-server. New privacy_filter.py compiles and applies regex rules; config.py reads [[aw-watcher-window.privacy_filter]] TOML tables; main.py runs the filter first in transform_window and refuses to start under the macOS swift strategy when rules are configured. Adds tests for the filter, config parsing, and swift refusal, plus a README section and CodeQL action version bumps. Not safe to merge — 2 P1 openConfidence 2/5 2 findings · ❌ 2 P1 · 🔒 2 security❌ P1 high · 🔒 security — In transform_window, when privacy_filter_rules is non-empty, apply_privacy_filters returns a shallow copy of the window dict. The subsequent exclude_titles loop mutates that copy in place (current_window["title"] = "excluded"). This is fine for the normal path, but when research_category_map is not None, research_transform is called on the copy and the exclude_titles loop is skipped. The privacy filter's redaction already happened on the copy, so that is preserved. However, the real defect is in the interaction with exclude_apps: the privacy filter copy is made, then exclude_apps checks pattern.search(current_window.get("app", "")) on the copy. That is correct. The actual bug is that apply_privacy_filters returns the original window object when rules is empty (line 110: return window), but when rules is non-empty it returns a copy. In transform_window, when privacy_filter_rules is non-empty but no rule matches, the copy is used and exclude_titles mutates the copy. When privacy_filter_rules is empty, exclude_titles mutates the original current_window dict, which is the same object passed in. This inconsistency is not itself a bug because the caller does not reuse the dict. The real issue is that the privacy filter's redact action replaces the entire field value with the replacement string, but subsequent rules in the same apply_privacy_filters call operate on the redacted value. This is documented and tested. A more serious issue: the redact action replaces the field value, but if a later rule with field="title" and action="drop" matches the replacement string, the event is dropped. That is intended per the test. The actual defect I can trace: in main.py, the swift refusal gate checks raw_privacy_filter_rules, which is the list from config before compilation. If the user configures a rule with an invalid regex, compile_privacy_rules skips it, but raw_privacy_filter_rules is still non-empty, so the watcher refuses to start under swift. This is intentional fail-closed behavior. However, on non-swift strategies, an invalid rule is silently skipped, and the user's intent to filter is not honored — the watcher starts and sends events that the user wanted filtered. The README says invalid rules are skipped with an error log, but the user may not notice. This is a trade-off, not a bug. The real bug I found: in apply_privacy_filters, when a redact rule matches, it sets result[field] = rule["replacement"]. If the same field is matched by a later rule, the later rule sees the replacement value, not the original. This is intended per the test test_multiple_rules_redact_then_drop. But consider a drop rule that comes after a redact rule on a different field: the redact rule changes the title, and the drop rule checks the app field — no interaction. The actual defect is that the privacy filter runs before exclude_apps, but exclude_apps checks the app field on the copy. If a privacy rule redacts the app field (field="app", action="redact"), then exclude_apps sees the redacted app name, not the original. This could cause an app that should be excluded to pass through if the redaction removes the pattern. For example, exclude_apps=[re.compile("1Password")] and a privacy rule redacts app field to "REDACTED" when it matches "bank". If the app is "1Password Bank", the privacy rule redacts it to "REDACTED", and exclude_apps no longer matches, so the event is sent. This is a privacy leak: the user's exclude_apps guarantee is bypassed by the privacy filter's redaction of the app field. The ordering comment says privacy filter runs first so sensitive events never reach other transforms, but it also means exclude_apps runs on a potentially redacted app name. The fix is to run exclude_apps before the privacy filter, or to have the privacy filter not redact the app field in a way that hides the app name from exclude_apps. This is a real correctness bug in the privacy guarantee. How this was verified: Traced transform_window: privacy filter runs at line 461-464, then exclude_apps at line 468-470. If a privacy rule redacts the 'app' field, exclude_apps sees the redacted value. Example: exclude_apps=[re.compile('1Password')], privacy rule {field:'app', pattern:'bank', action:'redact', replacement:'REDACTED'}, window app='1Password Bank' -> privacy redacts app to 'REDACTED', exclude_apps pattern.search('REDACTED') is None, event is sent. This violates the documented exclude_apps privacy guarantee. Consensus: 2/3 passes agreed — pass 2 looked and disagreed ❌ P1 high · 🔒 security — The swift strategy refusal gate in main.py checks How this was verified: Checked compile_privacy_rules: invalid regexes are skipped with logger.error and the rule is not added to compiled. In main.py, privacy_filter_rules is compiled and passed to heartbeat_loop; if all rules are invalid, the list is empty and no filtering occurs. The swift gate uses raw rules, so it fails closed on macOS, but on Linux/Windows the watcher runs unfiltered. Consensus: 2/3 passes agreed — pass 2 looked and disagreed Files changed (9) — the diff as I read it
Previous review passes
Reviewed Maintainer commands
|
Adds a client-side privacy filter that drops or redacts window events before they are sent to aw-server, so sensitive data never leaves the machine at all. New module `aw_watcher_window/privacy_filter.py`: - `compile_privacy_rules(raw)` — validates and compiles regex patterns; invalid regexes and unknown actions are skipped with an error log - `apply_privacy_filters(window, rules)` — returns None (drop) or a filtered copy (redact); never mutates the input dict Config via `[[aw-watcher-window.privacy_filter]]` TOML tables: pattern = "(?i)private browsing|incognito" action = "drop" # or "redact" field = "title" # optional; defaults to "title" replacement = "excluded" # optional; used for redact action Integration in main.py: - Privacy filter runs before research and exclude_title transforms - A None return from transform_window skips the heartbeat entirely - heartbeat_loop passes privacy_filter_rules through 23 new tests in tests/test_privacy_filter.py; all existing tests pass. Mirrors the server-side privacy_filters engine in aw-server-rust (#600) so users can enforce the same rules at both the watcher and server layers. macOS note: the swift strategy bypasses this Python transform; use --strategy jxa or --strategy applescript to enable it on macOS. Git-Session-Id: c4abdfa3-ec34-58ee-9600-b997fa16fbe8
Git-Session-Id: c4abdfa3-ec34-58ee-9600-b997fa16fbe8
codeql-action v2 is deprecated as of 2025-01-10 (GitHub changelog). Update all three action steps (init, autobuild, analyze) to v3. Also bump actions/checkout from v3 to v4 while here. This fixes the failing 'Analyze (python)' CI check on PR ActivityWatch#135. Git-Session-Id: c4abdfa3-ec34-58ee-9600-b997fa16fbe8
The macOS swift strategy sends heartbeats from a separate binary and cannot apply privacy_filter rules. Refuse to start when those rules are configured so titles are not leaked; use jxa or applescript. Also wrap a single privacy_filter table (list(mapping) was silently dropping the rule) and skip non-string field/replacement values. Git-Session-Id: 106b67f2-1ed0-575b-af3a-59ab2170164e
…ompile Git-Session-Id: 62d4746f-0cba-5722-ab4e-4aced59d086d
ef4ac06 to
070f15a
Compare
|
Rebased onto master to resolve conflicts with #156 (exclude_apps, just merged). Conflict resolution in
Also merged the new CI running on the new head. |
…tup fixture The parse_args mock in test_startup.py was missing two attributes that main.py now accesses: args.exclude_apps (added with the --exclude-apps flag) and args.privacy_filter_rules (added by the watcher-side privacy filter). The missing exclude_apps caused an AttributeError that prevented the watcher thread from starting, failing both startup integration tests. privacy_filter_rules is already guarded with getattr() in main.py so it wouldn't crash, but including it makes the fixture match the real args object and avoids future surprises. Git-Session-Id: a2106031-86a2-4f5c-b8dc-39b2cb86b17f
|
The startup-test failure is fixed in All six checks on that head are now green, including Linux, Windows, macOS, and typecheck: https://github.com/ActivityWatch/aw-watcher-window/actions/runs/38556886980 . No additional code change or rerun was needed for this recovery dispatch. |
|
This feels like we're duplicating or re-implementing the feature. It also doesn't address the fact that we want most user settings like this on the server. The only problem with these privacy filter settings is that they are themselves sensitive, at least if they are user-configurable and not picked from some public list. We've discussed this at length before in other issues. I've left comments and such about the design of the whole thing with these constraints. Also, not sure if that privacy filter engine that we have was even built into the Python server for parity. |
|
Valid concerns, and worth a direct answer on each. Duplication: The server-side privacy filter (aw-server-rust) filters data after it arrives at the server. That's fine when the user owns and trusts the server. This PR targets the different case: untrusted server (employer-managed, shared, or a third-party host). In that case, data must be filtered before it ever leaves the machine — server-side filtering can't help there. That's the only reason to put it in the watcher. Sensitivity of filter rules: This is the strongest argument for local TOML config, not against — if the rules are sensitive, they should never reach the server at all. A server-side setting API would expose them on the wire. Server parity / duplication maintenance: Fair. If the Python server doesn't have a working privacy filter either, that's a gap. I don't know the current state there. What I'd suggest: If the untrusted-server scenario is in scope for AW (and it seems like it should be, given the privacy-first ethos), this PR's approach is the right level to implement it. If it's not in scope, or if you'd prefer a different design (e.g., a standalone privacy proxy layer), I'm happy to close this and open a design issue instead. One-word answer from you on direction would help: close (not the right design) vs. continue (this approach is fine, merge when ready). |
|
Closing based on Erik's design feedback (2026-10-14): the watcher-side drop/redact approach duplicates what should live on the server, and user-configurable privacy rules are themselves sensitive. The right design is server-side filtering, per the existing discussion in related issues. |
|
@TimeToBuildBob Reopened and merged this after all, I think it's a good feature for the reasons you described, even if less used. However I want follow-up PRs improving the docstrings etc cross-referencing the other privacy filter approaches etc. Also should check that parity state in aw-server. |
codeql-action v2 is deprecated as of 2025-01-10 (GitHub changelog). Update all three action steps (init, autobuild, analyze) to v3. Also bump actions/checkout from v3 to v4 while here. This fixes the failing 'Analyze (python)' CI check on PR #135.
Summary
Adds a client-side privacy filter that drops or redacts window events before they are sent to aw-server, so sensitive data never leaves the machine at all.
This mirrors the server-side
privacy_filtersengine that landed in aw-server-rust#600, allowing users to enforce the same rules at both layers. Previously the watcher only supported dropping titles viaexclude_title/exclude_titles(redact to"excluded"). This PR adds explicitdropandredactactions with user-configurable regex patterns and replacement strings.What changed
aw_watcher_window/privacy_filter.py(new) — pure Python, no new deps:compile_privacy_rules(raw)— validates and compiles regex patterns; invalid regexes and unknown actions are skipped with an error log rather than crashingapply_privacy_filters(window, rules)— returnsNone(drop) or a filtered copy (redact); never mutates the input dictaw_watcher_window/config.py— exposesprivacy_filter_rulesfrom[[aw-watcher-window.privacy_filter]]TOML array of tablesaw_watcher_window/main.py— privacy filter runs first intransform_window; aNonereturn skips the heartbeat;heartbeat_loopthreads the rules throughtests/test_privacy_filter.py(new) — 23 tests covering compile, drop, redact, input immutability, edge casesREADME.md— documents the new config section with examplesConfig example
Rule fields:
pattern(Python regex),field(default"title"),action("drop"or"redact"),replacement(for redact; default"excluded").Ordering
Privacy filter runs before Research Edition and
exclude_titletransforms. A"drop"rule exits immediately — subsequent rules are not evaluated for that event.macOS note
The default
swiftstrategy bypasses this Python transform (same as Research Edition). Use--strategy jxaor--strategy applescriptto enable watcher-side privacy filtering on macOS.Test plan
python3 -m pytest tests/test_privacy_filter.py -v— 23 passedpython3 -m pytest tests/test_main.py -v— 12 passed (no regressions)droprule matching title: heartbeat skippedredactrule matching title: heartbeat sent with replacement value