Review PR Comments on Cross-Jurisdiction Routing

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
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.
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):
validateExchangeURLnow assertsexchange.Path == oauthTokenPath. The strip-then-re-append inexchangeSubjectTokenis no longer a hidden assumption — it's enforced. Added a refusal test case. - #4 (
expires_indiscarded):exchangeSubjectTokennow returnsexpiresIn; neweffectiveTokenTTL(expiresIn)=min(expiresIn − 1m buffer, cachedTokenTTL), falling back to the 4m cap when the server advertises nothing.storeTokentakes an explicit TTL and declines<=0(the live retry still succeeds, we just don't cache a too-short token). NewtokenExpiryBufferconst. AddedTestEffectiveTokenTTL+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")?
Done. Pushed to origin/fix/coreapi-cross-juris-transport.
Three commits, each its own fix:
4a937e7#3 — asserttoken_exchange_urlpath invalidateExchangeURLc41ab4c#4 — honor exchangeexpires_inwhen caching tokense1ed824#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.