Misc fixes - #14
Open
rolandgeider wants to merge 12 commits into
Open
Misc fixes#14rolandgeider wants to merge 12 commits into
rolandgeider wants to merge 12 commits into
Conversation
_verify treated any JoseError as "the signing key may have rotated" and forced a fresh key-set fetch. Anyone can post a malformed or foreign-signed token, so an unauthenticated caller could drive one request to the IdP per attempt — the TTL cache never applied on the one path strangers reach. joserfc raises InvalidKeyIdError for an unknown key id and something else for a bad signature or a malformed token, and only the first can be fixed by fetching keys, so only it forces a refetch now. A kid is attacker-chosen though, so forced refetches are capped at one per minute, measured from the last forced one — measuring from the last fetch of any kind would let an ordinary refresh start the window and answer a rotation moments later with 401s. JwksCache also takes a lock, so concurrent misses share one fetch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The one-key sub-dicts ({"slot": {"id": ...}}) were a vestige of the full
objects trimmed in #13; every caller had to map ["slot_entry"]["id"] onto
the slot_entry_id parameter the follow-up tools take. The flat keys match
those parameter names, and the delete tools' responses, verbatim. A new
test pins the success shape, which nothing asserted on before.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Five conflicts, all of them two additions meeting in one place rather than two answers to one question: common.py — this branch's opt_uuid/opt_int/opt_decimal and master's _unit_id landed on the same lines above as_weight_unit. Both stay; the old as_weight_unit signature goes, master's name-or-id one stands. routines.py and workout_logs.py — import lists, both sides' names kept. update_slot_entry — master resolves the unit through as_repetition_unit / as_weight_unit with allow_id, this branch replaces the None dance on slot_id with opt_int. The two are independent: the resolved units stay, and opt_int(slot_id, "slot_id") is exactly what opt(as_int(slot_id, ...) if slot_id is not None else None) said. Verified after merging: an omitted slot_id is UNSET, "9" is 9, "seconds" is 3 and "3" is still 3. test_weight_units_and_rir.py and CHANGELOG — adjacent additions, both kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pydantic attaches the raw input dict to a ValidationError as `input_value` and truncates only its middle, so the tail of WGER_API_KEY survived into the message. Nothing caught that error, so it left main() as an uncaught traceback: the client's MCP log under stdio, the container log under http. A key shorter than the truncation window would have appeared in full. load_settings now restates a validation failure as ConfigError, built from the error messages and none of the inputs, and server.main turns it into the same one-line SystemExit it already gave for a bad --transport. The raise sits outside the except block on purpose: inside it, the original would hang off __context__ with the input dict still attached, which `raise ... from None` hides from the printed traceback but does not drop. The three credential fields become SecretStr as a second layer, covering anything that formats the settings object rather than the error — a log line, a traceback frame. That alone would not have fixed the leak, since the input dict holds what the env source read before any field was validated. Reading a secret in code now needs .get_secret_value(); the static-token length check needed it too, as str() on a SecretStr is the ten-character mask whatever the value, which would have rejected every token as too short. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
wger returns one date-sequence entry per calendar day, and a fit_in_week routine pads the rest of the week with entries whose `day` is null: four of the seven for a three-day split. get_workout_for_date read that day's id straight out and raised AttributeError. It now answers the way it already did for a rest day — planned: [] with is_rest_day true — while keeping the entry's own iteration and omitting the "outside the routine's range" note, since the date is inside it. The null never reached this code before because the generated client died parsing it first: wger's OpenAPI schema declares `day` and `label` required and non-nullable on both WorkoutDayData serializers, though the API has always returned null for both on padding entries. That is a separate fix in the server's serializers plus a regenerated client. The test builds the entry directly rather than through from_dict so it does not depend on which client version is installed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every transport failure was shaped as `wger is unreachable: {exc}`. httpx
raises most of them with an empty message — str() on a ReadTimeout is '' —
so the detail ended at the colon, naming neither the cause nor anything
the operator could act on. It also asserted the wrong thing: a read
timeout means the request arrived and the answer was late, which sends
someone checking the URL and the firewall for a problem that is a slow
query.
A read, write or pool timeout now says wger did not answer within the
budget and points at the query; a connect timeout says the connection was
never accepted; everything else keeps the unreachable wording but falls
back to the exception's class name when it carries no message.
The budget moves into REQUEST_TIMEOUT_SECONDS next to the client that
applies it, so the message quotes the value actually in force rather than
a number written twice.
Found while sweeping the tools against a dev server, where
search_ingredients times out on a remote database: twenty minutes went
into rediscovering what the message could have said outright.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A validly signed token without `sub` was accepted; its identity fell back to the username claim, or to the constant "unknown" when that was absent too. The outbound wger credential cache is keyed by that identity, so two such callers shared one entry and the second acted with the first caller's wger JWT. Typical IdPs always emit `sub`, so this needed an IdP that omits it plus the default empty allowlist. The middleware now rejects tokens whose `sub` is missing, empty or not a string with 401. The username claim keeps serving the allowlist and display but never keys the cache; preferred_username is by spec neither unique nor stable. Reported privately by github.com/WRG-11 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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
1 participant
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.
Follow-ups from a full read-through of
src/wger_mcp, looking for duplication, overengineering and YAGNI. No structural changes — the layering holds. Two user-visible fixes, one hardening fix, and four refactors that remove repeated definitions rather than lines.Breaking (response shape)
Both are documented under Unreleased in the changelog:
add_exercise_with_setsreturns the ids flat (slot_id,slot_entry_id, …) instead of one-key sub-dicts. The flat keys match the parameter names the follow-up tools take, verbatim.lookup_food_by_barcode/lookup_foods_by_barcodesno longer returnwger_ingredient_payload. It repeatedmacros_per_100gunder a second set of keys, shaped for acreate_ingredientcall that cannot exist: wger's REST/ingredient/is read-only (the generated client has noingredient_createand noIngredientRequest), and the tool was removed with the move to multi-user auth. Every lookup was paying the caller's context for it.docs/HANDOFF.mdkept the payload as the seed of that tool should the endpoint ever appear; the note now says why it went instead.Fixes
ALLOWED_HOSTS=a,bin.envaborted startup with a parse error, because only the process environment was rewritten to the JSON form pydantic-settings parses — the dotenv file is a source of its own.NoDecodeplus a before-validator now handles both sources identically, andload_settings()no longer mutatesos.environas a side effect. The JSON spelling keeps working.JoseErrorcounted as "the key may have rotated", so an unauthenticated caller could drive one request to the IdP per malformed token. OnlyInvalidKeyIdErrorforces a refetch now, capped at one per minute and measured from the last forced refetch — measuring from the last fetch of any kind would answer a rotation with 401s until the window ran out.JwksCachealso takes a lock, so concurrent misses share one fetch.lookup_food_by_barcodegains the one-shot 429 retry the batch variant always had, as a consequence of the two sharing a fetch path.Refactors
opt_int/opt_uuid/opt_decimalreplaceopt(as_int(x, "x") if x is not None else None)at 38 call sites across six modules.day_bounds/day_range_filtersreplace three copies of the inclusive date-range translation. The exclusive upper bound is the kind of detail a fourth copy gets wrong, and getting it wrong drops the final day.METRICStable drives analytics' buckets, projection and deltas, which spelled the same mapping out three times.weekly_summaryandexercise_historynow accumulate through_accumulateinstead of open-codingreps * weighta second and third time.Verification
264 tests pass,
ruff checkclean. New tests cover the JWKS cache (6 cases, including that a rotation is still picked up immediately), the OFF lookup (4),day_bounds(4) and env-file list parsing (2).The two refactors that could silently change output were checked by dumping the real results before and after: every analytics tool's response over the same logs is byte-identical, metric subsets included, and so are the date filters all three affected tools send.
🤖 Generated with Claude Code