Skip to content

fix(aw-sync): forward API key from server config, stop panicking on 401 - #640

Merged
ErikBjare merged 5 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/aw-sync-api-key-and-401-panic
Jul 31, 2026
Merged

fix(aw-sync): forward API key from server config, stop panicking on 401#640
ErikBjare merged 5 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/aw-sync-api-key-and-401-panic

Conversation

@TimeToBuildBob

@TimeToBuildBob TimeToBuildBob commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Fixes #639

What broke

When [auth] api_key is set in the aw-server-rust config, aw-sync enters a crash loop because of two compounding bugs:

  1. API key never forwarded: AwClient::new() always passes None for api_key, so every request to an authenticated server returns 401.
  2. 401 hits an unwrap and panics: Several methods in the AwClient implementation of AccessMethod (including get_buckets, called at startup) used .unwrap() instead of propagating errors — turning any HTTP error into a hard crash.

Fix

  • aw-sync/src/util.rs reads the server port and [auth] api_key together from the selected server config.
  • aw-sync/src/main.rs accepts -c / --config, matching aw-server's config override, and passes the key to AwClient::new_with_api_key().
  • Local server config is read only for exact loopback hosts (127.0.0.1, ::1, or localhost). A caller-selected remote --host never causes the local credential file to be read or its key to be attached.
  • aw-sync/src/accessmethod.rs propagates HTTP failures instead of panicking in get_buckets, get_events, get_event_count, create_bucket, and non-404 get_bucket responses.

Testing

  • cargo test -p aw-sync --all-targets --no-fail-fast
  • cargo check -p aw-sync --all-targets
  • cargo fmt --all -- --check
  • Pre-commit cargo clippy
  • cargo run -q -p aw-sync --bin aw-sync -- --help (verified -c, --config <CONFIG>)

Added focused tests for custom config port/key parsing, production/testing fallback ports, and exact loopback-host classification.

Usage with a custom server config

Pass the same path to both processes:

aw-server --config /custom/path.toml
aw-sync --config /custom/path.toml

An explicit aw-sync --port still takes precedence over the port read from that config.

Out of scope

Android sync uses a JNI-provided port and has no server-config path. Passing Android auth credentials through JNI is a separate interface change and is not included here.

…n 401

Two bugs compounded to cause a restart loop when auth is configured:

1. AwClient was always created with api_key=None, so requests to an
   authenticated server always got 401.

2. Several .unwrap() calls in the AwClient AccessMethod impl (get_buckets,
   get_events, get_event_count, create_bucket) panicked on any HTTP error
   instead of propagating it as an Err.

Fixes:
- Add get_server_api_key() to util.rs, reading [auth] api_key from the same
  server config file already parsed for the port.
- Use AwClient::new_with_api_key() in main() with the key read above.
- Replace all .unwrap() calls in the AwClient AccessMethod impl with
  .map_err(|e| e.to_string()), so HTTP errors (including 401) become
  recoverable errors instead of panics.
- Also fix get_bucket to not panic on unexpected status codes.

Fixes ActivityWatch#639
Comment thread aw-sync/src/main.rs Outdated
Comment thread aw-sync/src/util.rs Outdated
@greptile-apps

greptile-apps Bot commented Jul 28, 2026

Copy link
Copy Markdown

Greptile Summary

This PR forwards local aw-server authentication into aw-sync while replacing panic-prone HTTP handling with propagated errors.

  • Reads the port and API key from the selected server configuration, including custom config paths.
  • Restricts local credential loading to loopback targets and normalizes IPv6 literals for URL construction.
  • Adds the matching CLI option, documentation, and focused configuration and host-classification tests.
  • Converts several AwClient access-method panics into ordinary sync errors.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
aw-sync/src/main.rs Adds the config override, loopback-gated credential loading, explicit port precedence, IPv6 host normalization, and authenticated client construction.
aw-sync/src/util.rs Replaces port-only lookup with validated port and API-key parsing, adds loopback classification and URL-host formatting, and covers these behaviors with focused tests.
aw-sync/src/accessmethod.rs Replaces panic and unwrap paths in the AwClient access adapter with propagated errors.
aw-sync/README.md Documents using the same custom server config path for aw-server and aw-sync.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    CLI[aw-sync CLI options] --> Guard{Host is loopback?}
    Guard -->|Yes| Config[Read selected aw-server config]
    Guard -->|No| Defaults[Use default port without API key]
    Config --> Client[Construct AwClient]
    Defaults --> Client
    Client -->|HTTP with optional Bearer key| Server[aw-server]
    Server -->|HTTP failure| Error[Propagate sync error]
Loading

Reviews (5): Last reviewed commit: "fix(aw-sync): format IPv6 hosts for clie..." | Re-trigger Greptile

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Addressed the Greptile security finding in 9173349.

The original code always read the local API key and forwarded it to whatever --host was specified — a credential leak if --host points to a remote server. Fixed with a simple guard: the key is only attached when the target host is one of 127.0.0.1, ::1, or localhost.

let is_localhost = matches!(opts.host.as_str(), "127.0.0.1" | "::1" | "localhost");
let api_key = if is_localhost {
    util::get_server_api_key(opts.testing)?
} else {
    None
};

For the non-localhost case (--host REMOTE), api_key is None so AwClient sends no Authorization header — same behavior as before this PR. A remote aw-server would need its own separate auth mechanism if required.

@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 60.86957% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.19%. Comparing base (656f3c9) to head (7b8015d).
⚠️ Report is 80 commits behind head on master.

Files with missing lines Patch % Lines
aw-sync/src/main.rs 0.00% 9 Missing ⚠️
aw-sync/src/accessmethod.rs 0.00% 7 Missing ⚠️
aw-sync/src/util.rs 93.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #640      +/-   ##
==========================================
+ Coverage   70.81%   76.19%   +5.38%     
==========================================
  Files          51       63      +12     
  Lines        2916     5126    +2210     
==========================================
+ Hits         2065     3906    +1841     
- Misses        851     1220     +369     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

@greptileai review

Comment thread aw-sync/src/main.rs Outdated
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Addressed the remaining Greptile summary finding in 63296f0 and added regression coverage.

  • Added aw-sync -c/--config, matching aw-server's selected config override.
  • Port and API key now come from that same selected file; explicit --port still wins.
  • Tightened the security boundary: for a non-loopback --host, aw-sync does not read local server config at all, so it cannot attach the local credential. It also retains the normal fallback port unless --port is explicit.
  • Added tests for custom config parsing, missing-config defaults, and exact loopback classification.

Verification:

  • cargo test -p aw-sync --all-targets --no-fail-fast → 11 passed
  • cargo check -p aw-sync --all-targets → passed
  • cargo fmt --all -- --check → passed
  • pre-commit cargo clippy → passed

@greptileai review

@ErikBjare

Copy link
Copy Markdown
Member

@greptileai review

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

✅ Review Adjudication — Ready for Merge

All 3 Greptile threads resolved. Greptile convergence cap hit at attempt 3 (3/5 score), so re-triggering is stopped — the remaining findings were assessed and dismissed.

Dismissed (false positives):

  • P1: Local key sent to remote hosts () — false positive. The code explicitly guards against this: is_loopback_host(&opts.host) is checked before loading the config; non-loopback hosts receive ServerConfig::default_for() with api_key: None. The local credential is never sent to remote targets.

  • P1: Custom config key is ignored (util.rs) — false positive. get_server_config takes config_override: Option<&Path> and uses the provided path when Some. The reads_port_and_api_key_from_config_override test verifies this explicitly.

Previously fixed:

  • P1: Loopback aliases lose authentication — fixed in eb1ac62, thread resolved.

CI status: All jobs green (Qt + Tauri, all platforms). Greptile: 3/5 with no unaddressed findings (score reflects iteration count, not unresolved issues).

Converged after 3 rounds. Acceptable to ship.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Found and fixed the new Greptile summary finding in 7b8015d.

Bare IPv6 input (--host ::1) was correctly classified as loopback and loaded the API key, but AwClient then built the invalid URL http://::1:<port>. The CLI now brackets bare IPv6 literals only at the URL-construction boundary, so classification still sees ::1 while the client receives [::1]. Added focused coverage for loopback and non-loopback IPv6 formatting.

Verified locally:

  • cargo test -p aw-sync --all-targets --no-fail-fast
  • cargo check -p aw-sync --all-targets
  • cargo fmt --all -- --check
  • pre-commit, including workspace clippy

Fresh CI is running. This supersedes my earlier merge-ready adjudication: the summary-level IPv6 finding was real, not resolved by eb1ac62 alone.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

@greptileai review

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

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.

@ErikBjare
ErikBjare merged commit a8bf1e4 into ActivityWatch:master Jul 31, 2026
8 checks passed
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Confirmed — your YES action landed: #640 was merged at a8bf1e434825155e1e5ec51ee8ba52b8ed0a533a, with final CI green and Greptile 5/5. The source issue, #639, is closed as completed, and I marked the local task done. Thanks.

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.

aw-sync: doesn't read/forward API key when auth is configured, panics on 401

2 participants