Review PR Comments on Cross-Jurisdiction Routing

Claude Code·Opus 4.8[1m]·toothbrush·3mo ago·8min·3 Checkpoints·5 file changes·+85/-13·44.1K tokens

for PR https://github.com/entireio/cli/pull/1457, let's have a look at the comments posted on the trail https://entire.io/gh/entireio/cli/trails/599/fix-coreapi-handle-cross-jurisdiction-routing-for-control, use entire trail to find the comments, once you've switched to the right branch

3mo ago·2m

Bottom line: On branch fix/coreapi-cross-juris-transport (trail 599). 5 open review findings, all in internal/coreapi/cross_juris_transport.go — 3 medium, 2 low. The two highest-value ones are the same bug (default-port normalization) flagged in two places.

Medium

1. Default-port host comparison — isSafeOrigin/validateExchangeURL (line 365) Exact string equality on url.URL.Host. Go omits default ports (example.com) but keeps explicit ones (example.com:443). A server returning token_exchange_url with :443 won't match a request to the bare host → legitimate cross-juris auth fails. Fix: normalize default ports / use origin-equality. (conf 0.8)

2. Same bug in followMisdirected (line 371) homeCore.Host vs federation-manifest hosts, exact equality, same :443-vs-bare-host failure → legitimate 421 redirect rejected. Same fix. (conf 0.85)

3. Brittle path coupling in exchangeSubjectToken (line 515) strings.TrimSuffix(hint.TokenExchangeURL, oauthTokenPath) strips /oauth/token, then PostOAuthToken re-appends it — but validateExchangeURL only checks scheme+host, not path. If a hint ever supplies a non-/oauth/token path, the trim is a no-op and the POST goes to a wrong URL. Not exploitable today (same-origin enforced, server controls its path). Fix: assert path == oauthTokenPath, or pass the validated origin.

Low

4. expires_in discarded (line 519) PostOAuthToken returns expiresIn but it's dropped (token, _, err); cache always uses fixed cachedTokenTTL (4m). If server issues a token shorter than 4m, cache could serve a stale token → unexpected 401. Suggest min(expiresIn - buffer, cachedTokenTTL).

5. Doc/const contradiction (lines 67–68) Struct doc says TTL "matches the foreign-session token's lifetime (5 minutes)" but cachedTokenTTL is 4m (with a correct buffer comment at line 103). Just fix/drop the misleading sentence.

Findings 1 and 2 are clearly the most worth fixing (real functional failure, not just hardening). Want me to implement any/all of these?

i disagree - 1 & 2 concern strings that we control, so i don't want to build machinery to normalise strings we have set.

3,4,5 though are probably worth addressing.

3mo ago·2m

Done. Lint clean, internal/coreapi tests pass.

Bottom line: Addressed findings 3, 4, 5 in internal/coreapi/cross_juris_transport.go; left 1 & 2 alone per your call. Not committed.

What changed:

  • #3 (path coupling): validateExchangeURL now asserts exchange.Path == oauthTokenPath. The strip-then-re-append in exchangeSubjectToken is no longer a hidden assumption — it's enforced. Added a refusal test case.
  • #4 (expires_in discarded): exchangeSubjectToken now returns expiresIn; new effectiveTokenTTL(expiresIn) = min(expiresIn − 1m buffer, cachedTokenTTL), falling back to the 4m cap when the server advertises nothing. storeToken takes an explicit TTL and declines <=0 (the live retry still succeeds, we just don't cache a too-short token). New tokenExpiryBuffer const. Added TestEffectiveTokenTTL + TestStoreTokenDeclinesNonPositiveTTL.
  • #5 (doc/const contradiction): Rewrote the lines 67–68 comment to describe the real TTL behavior instead of the wrong "5 minutes".

Note: I ran the package tests, not the full mise run test:ci suite. Want me to run the full suite, and/or dismiss findings 1 & 2 on the trail with a rationale ("server-controlled strings, not worth normalization machinery")?

push.

3mo ago

commit each fix separately then push.

3mo ago·3m

Done. Pushed to origin/fix/coreapi-cross-juris-transport.

Three commits, each its own fix:

  • 4a937e7 #3 — assert token_exchange_url path in validateExchangeURL
  • c41ab4c #4 — honor exchange expires_in when caching tokens
  • e1ed824 #5 — correct the cache-TTL doc comment

Lint clean, internal/coreapi tests pass at each commit. Findings 1 & 2 left untouched per your call. I did not run the full test:ci suite — say the word if you want it.