fix(bfabric): canonicalise base_url without a trailing slash - #596
Draft
leoschwarz wants to merge 7 commits into
Draft
fix(bfabric): canonicalise base_url without a trailing slash#596leoschwarz wants to merge 7 commits into
leoschwarz wants to merge 7 commits into
Conversation
`_validate_base_url` appended a trailing slash that essentially nothing
wanted: 16 call sites stripped it right back off to build a URL, and the
two SOAP engines forgot to, emitting `.../bfabric//workunit?wsdl` on every
`Bfabric.connect()`. The CLI's `normalize_base_url` had already settled on
the slash-free form, which is also what gets persisted to `~/.bfabricpy.yml`.
Flip the canonical form and canonicalise once at each public boundary --
the `connect_*` classmethods and `WebappClient.create` now build the
`BfabricClientConfig` first and read `base_url` back from it -- so the
downstream strips become dead rather than merely relocated. Going through
the validator is also stronger than `rstrip`: it lowercases the host, drops
a default port, and rejects a non-HTTP URL up front.
`entities/core/uri.py` had to flip alongside. It hardcoded the instance
component as `.../bfabric/`, and `EntityReader` compares that for string
equality against `config.base_url` at three sites, so changing only the
config would have made every `read_uris`/`query` raise "Unsupported
B-Fabric instance". `EntityUri` strings are unaffected -- `as_uri()`
appends its own slash before `urljoin`.
Two strips survive deliberately. `api_to_rest_url` is exported from
`bfabric.transfer` and its strip is load-bearing for the `.../api/` case,
where `endswith("/api")` is False without it. The one in
`compute_token_cache_path` is gone instead: every caller passes a
canonical value and the resulting key is byte-identical to today's, so no
cached token is orphaned -- a test now pins exactly that.
The trailing slash was originally introduced in #341 so that
`urljoin(base_url, ...)` in `Entity.web_url` would not drop the `/bfabric`
path segment. That call site has since been replaced by the `EntityUri`
machinery, and both remaining `urljoin` callers append their own slash, so
the reason no longer applies.
Closes #576
…stem The slash-free canonical form was a convention nothing checked: a caller could hand a raw string to `pkce_login` or `compute_token_cache_path` and nothing would object. That is not hypothetical -- `bfabric-cli auth register` was passing a raw `--base-url` straight into `register_client`, previously masked by a defensive `rstrip`. Add `bfabric.config.CanonicalBaseUrl`, a `str` subclass that validates and canonicalises in `__new__`, mirroring the existing `EntityUri` pattern. It stays a `str`, so interpolation, comparison and dict keys are unaffected, but annotating a parameter with it lets basedpyright reject a value that never passed through canonicalisation -- which `Annotated[str, AfterValidator(...)]` cannot, being indistinguishable from `str`. Public entry points (`connect_*`, `WebappClient.create`) keep taking `str` and canonicalise once; private helpers and the entity layer require the type. That collapses the previous construct-then-read-back dance into `base_url = CanonicalBaseUrl(base_url)`, with the config built where it belongs. `EntityUriComponents.bfabric_instance` moves from pydantic's `HttpUrl` to the same type, removing the third spelling of one concept. The `str()` coercion when building a `GroupKey` existed only because `HttpUrl` is not a `str`, and sat on exactly the seam that diverged in #576; the retype also clears a grandfathered `reportArgumentType` from the baseline. Two fixes fall out of typing the boundary: `validate_token` compared the server's `caller` against the configured `supported_bfabric_instances` by exact string equality, so a trailing slash on either side rejected a valid token. Both sides are now canonicalised. This is legacy API, so the fix is local and the settings stay `str`. Typing `normalize_base_url`'s return exposed that the CLI writes its config through unsafe `yaml.dump`, which serialised the subclass as `!!python/object/new:` -- a file the `safe_load` on read cannot parse. The writer now uses `safe_dump` (matching the reader) and the CLI writes plain strings, so a non-YAML type fails loudly instead of corrupting `~/.bfabricpy.yml`. The baseline loses two entries and gains none.
#596 fixed the canonical form of base_url and canonicalises it once per public boundary, but the resulting invariant was convention only: nothing stopped a caller handing a raw string to pkce_login or compute_token_cache_path. That was not hypothetical -- `auth register` was doing exactly that, masked by a defensive rstrip until #596 removed it. The failure modes are asymmetric. A stray slash in a URL yields `//`, which servers tolerate, which is why #576 went unnoticed for months. A stray slash in compute_token_cache_path changes the SHA-256, so the cache misses and the user is told to log in again while their token sits on disk. bfabric.BaseUrl is a str subclass that validates and canonicalises in __new__, so basedpyright rejects an unvalidated string at the boundary while every f-string, dict key and httpx call keeps working. It replaces three spellings of one concept: base_url: str, bfabric_instance: str, and EntityUriComponents.bfabric_instance: HttpUrl -- the last of which forced the str() coercion in uri.py that sat on the exact seam #576 broke. Verified the type checker actually catches it before migrating: annotating one function surfaced two real call sites, so neither the position-independent baselines nor the neighbouring inline ignores absorb the error. Two consequences worth noting: - yaml.dump would serialise a str subclass as `!!python/object/new:`, silently writing a config that no longer loads. config_writer now uses safe_dump, which rejects it loudly instead; the two CLI write paths coerce to str. - BaseUrl raises a plain ValueError rather than a pydantic ValidationError, so the CLI can print it directly. Pydantic still wraps it on a model field. Also fixes a latent bug in validate_token, which compared the server's caller against the configured instances with no canonicalisation on either side.
…l-form # Conflicts: # bfabric/docs/getting_started/configuration.md # bfabric/src/bfabric/oauth/_webapp_client.py # tests/bfabric/oauth/test_device_code.py # tests/bfabric/oauth/test_token_cache.py # tests/bfabric/oauth/test_token_exchange.py
validate_token canonicalises the token's caller and the configured supported_bfabric_instances before comparing them, but the three sibling checks against that same list were left as raw string comparisons. An operator whose settings spell one instance with a trailing slash and another without gets a startup failure, and a rest-proxy client that passes a trailing slash is told the configured instance is unknown. Type the instance fields as BaseUrl so pydantic canonicalises them on parse -- including the feeder_user_credentials keys, which are used as a lookup key for the very value the membership check just accepted -- and canonicalise the request-supplied bfabric_instance before matching it. A malformed URL is reported as an unknown instance rather than leaking a parse error to the caller. The new settings tests also caught that default_bfabric_instance: null, documented as making the request parameter mandatory, was rejected by its own validator, so that mode could never actually be configured.
…l-form # Conflicts: # bfabric/docs/changelog.md # bfabric/docs/getting_started/configuration.md # bfabric/src/bfabric/bfabric.py # bfabric/src/bfabric/engine/engine_zeep.py # bfabric/src/bfabric/oauth/_url_token.py # bfabric_asgi_auth/docs/changelog.md # bfabric_rest_proxy/docs/changelog.md
… boundaries Four hand-rolled canonicalisations survived the first pass, all in the class the BaseUrl type exists to eliminate: - UrlTokenContext.base_url stripped the slash off `iss` by hand and returned a bare str. - api_to_rest_url stripped it again, and its only caller cast an already-canonical BaseUrl back to str to feed it. - asgi-auth's SessionData kept the token's caller verbatim, so BfabricUser.instance still compared unequal to the same instance spelled without a slash -- the exact mismatch this branch fixes in core and the proxy. - KNOWN_INSTANCES held plain strs, forcing a BaseUrl re-wrap at each use. Typing SessionData.bfabric_instance surfaced a related hole: validate_token now raises ValueError for a caller that is not an http URL, and the asgi-auth strategy only caught ValidationError, so a malformed caller in an otherwise valid token became a 500 instead of a reported validation failure. ValueError subsumes pydantic's ValidationError, so widening the tuple covers both. The session is dumped in json mode so no str subclass reaches the cookie serializer. Also corrects the changelog claim that BfabricClientConfig now requires BaseUrl(...) -- a plain string still validates at runtime; only a type checker asks for the wrapper. The set-comprehension in validate_token is deliberate, not redundant: settings is a structural protocol, so its list is not guaranteed to hold BaseUrl already. Its test parametrises exactly that case; comment added so the next reader does not "simplify" it away.
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.
BfabricClientConfig.base_urlto be canonicalised without a trailing slash. Config files andconnect_*arguments still accept one.bfabric.BaseUrl, astrsubclass holding a validated slash-free instance URL, so an unvalidated base URL is a type error rather than a stray//or a token-cache miss. It behaves as astreverywhere, so interpolation, comparison and dict keys are unaffected.BfabricClientConfig.base_url,Entity.bfabric_instanceandEntityUriComponents.bfabric_instance(was a pydanticHttpUrl) to this type, and require it forEntityReader'sbfabric_instancearguments. A plain string is still accepted wherever pydantic validates one; only a type checker now asks forBaseUrl(...).connect_oauth/connect_pkce/connect_device_code/connect_patandWebappClient.createto canonicalisebase_url, so a mixed-case host or a default port is normalised too and a non-HTTP URL is rejected up front with a plainValueError.UrlTokenContext.base_urlandbfabric.transfer.api_to_rest_urlto return aBaseUrlinstead of stripping the slash by hand.show.htmllinks printed bybfabric_readandbfabric-cli api read, to no longer contain a doubled slash.validate_tokento canonicalise both the token'scallerand the configuredsupported_bfabric_instancesbefore comparing, so a trailing slash on either side no longer rejects a valid token.TokenValidationSettings/WebappIntegrationSettingsto canonicalisevalidation_bfabric_instance,supported_bfabric_instancesand thefeeder_user_credentialskeys on parse, so a settings file that spells the same instance both ways no longer fails its own membership checks. A non-http instance URL is now rejected there too.bfabric_instancerequest parameter before matching it againstsupported_bfabric_instances, so a caller's trailing slash no longer makes a configured instance look unknown.default_bfabric_instance: nullin the REST proxy, documented as making thebfabric_instanceparameter mandatory, to no longer fail settings validation.bfabric-asgi-authto canonicalise the session'sbfabric_instance, soBfabricUser.instanceandget_bfabric_client().config.base_urlcompare equal to a configured instance spelled either way.bfabric-asgi-authto report a token naming a non-http instance as a failed validation instead of letting an unhandledValueErrorbecome a 500.bfabric-cli auth registerto canonicalise itsbase_urlargument like the otherauthcommands.bfabric-cli auth login/auth patto write the config withyaml.safe_dump, matching thesafe_loadused to read it back.bfabricPy-testsmay assert the old trailing-slash form ofbase_url.Closes #576
🤖 Prepared with assistance from Claude Opus 5 via Claude Code.