Repository navigation
Fix --status to validate against the configured sync server - #64
Open
tobiashochguertel wants to merge 2 commits into
Open
tobiashochguertel wants to merge 2 commits into
tobiashochguertel wants to merge 2 commits into
Conversation
--status validated the session against creds.server_url instead of config.sync.server_url, so after re-registering on a different server the check silently probed the stale credentials URL. login/register also re-prompted for a server URL even when sync was already enabled. - status: validate against config.sync.server_url, warn on mismatch, and print a re-auth hint on failure - login/register: fall back to the configured server URL when --server is omitted and sync is enabled
There was a problem hiding this comment.
🟡 Changes recommended
Status checks can disclose saved session credentials to a different configured server.
1 open issue
What changed in this PR
Aligns authentication commands with the configured sync server.
Changes:
- Validates status against the configured URL, with mismatch warnings and login guidance.
- Defaults login and registration to the configured server when sync is enabled.
- Adds unit tests for server URL selection.
| File | Description |
|---|---|
| crates/taskbook-client/src/auth.rs | Updates server selection, status checks, messaging, and tests. |
Review effort: Balanced
Comment on lines
+217
to
+226
| if creds.server_url != config.sync.server_url { | ||
| println!( | ||
| "{}", | ||
| format!( | ||
| "Warning: credentials were saved for {} (config points to {})", | ||
| creds.server_url, config.sync.server_url | ||
| ) | ||
| .yellow() | ||
| ); | ||
| } |
Author
There was a problem hiding this comment.
Good catch — fixed in bf12160. On a server mismatch --status now prints the warning, marks the session "not checked" and exits without sending the stored token anywhere; the user is directed to tb --login. URLs are compared with the same trailing-slash normalization ApiClient::new applies (same_server_url), with unit coverage.
--status sent the saved bearer token to the configured server even when the credentials were issued by a different one. Compare normalized URLs and skip the authenticated request on mismatch, directing the user to re-authenticate. Co-Authored-By: Tobias Hochgürtel <[email protected]>
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
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.
Problem
tb --statusprintsServer:fromconfig.sync.server_url, but the session check callsApiClient::new(&creds.server_url, ...). If the configured server changed after credentials were saved (e.g. re-registered on a different server, or the config file was edited),--statusprobes the stale credentials URL while displaying the configured one — the two silently disagree.Relatedly,
tb --loginandtb --registeralways prompt for a server URL when--serveris omitted, even when sync is already enabled andconfig.sync.server_urlis set.Fix
status: validate the session againstconfig.sync.server_url(the user's intent), warn when the saved credentials were issued for a different server, and print a re-auth hint (tb --login) when the session check fails.login/register: when--serveris omitted and sync is already enabled, useconfig.sync.server_urland printUsing server: …instead of prompting.configured_server_url()helper with unit tests.Verification
rustfmt --checkclean.auth.rstypechecks and its 3 new unit tests pass (flag wins over configured server, configured server used when sync enabled, no fallback when sync disabled). The local toolchain predates the dep set's edition2024 requirement, so the tests were run against stubs matching this crate'sConfig/Credentials/ApiClientsignatures — CI covers the full build.Happy to adjust the messaging or split this if you'd prefer a smaller change.