fix(auth): Self-hosted instances dead-end with no usable credential - #86
Conversation
The 404 from /.well-known/oauth-authorization-server carried its own
recovery guidance ("is this a Flagsmith API URL?"). Hints don't live in
message strings, and this one is about to become conditional: an old
Flagsmith needs a different answer from a URL that isn't Flagsmith at all.
Wrap ErrNoDiscovery instead, and map it to hintAPIURL alongside the
existing hintSDKAPIURL.
beep boop
Answers what a Flagsmith supports before there is a credential to ask with, which is exactly when the CLI needs to know: the login flow has to tell an instance too old for OAuth apart from a URL that is not Flagsmith. Root-scoped, not under /api/v1, and unauthenticated — so a package function rather than a Client method, whose newRequest would need auth that does not exist yet at this point in the flow. beep boop
Off api.flagsmith.com the unscoped credential variables are ignored by design, so every hint naming one told self-hosted users to set something that would have no effect — including the "not logged in" hint they reach by following the previous one. The variable name depends on the instance, so the hints that carry one become functions of it. hintMasterKeyOrLogin names none and stays a const. beep boop
`flagsmith init` and `flagsmith login` against a Flagsmith older than 2.255.0 dead-ended on a 404 from the discovery endpoint, with no mention of the Master API key that would have worked. Probe /version only once discovery has already failed, so the happy path costs nothing. An image tag that is not a version says nothing about age, so anything unparseable keeps the generic hint rather than sending someone with a healthy instance chasing an upgrade. Anyone reaching this is self-hosted by definition, so the key variable is named host-scoped. beep boop
A self-hosted user who exported FLAGSMITH_API_KEY was told "not logged in" and hinted to set FLAGSMITH_API_KEY — the thing they had just done. Their key was fine; only its name was, and the CLI knew that and didn't say so. Say which variable is being ignored and which one to set instead. This no longer wraps ErrNotLoggedIn, so `init` stops offering a browser login over a credential the user explicitly set: silently discarding it is the bug. The redirected-host tests now assert on the withholding itself rather than the sentinel that used to stand in for it. beep boop
The scoping rule was only discoverable by hitting it, which is how it was reported. beep boop
FLAGSMITH_ENVIRONMENT_KEY is host-scoped like the Admin variables, but off its own surface — sdkApiUrl, which defaults to Edge and so is usually not even the same host as the API. The hints naming it had the same defect as the Admin ones: correct on SaaS, useless anywhere else. The ser.* context guard needed the SDK URL to name the variable, and ran before that URL was resolved, so it moves below the resolution. Both surface URLs are now published as soon as resolveContext knows them rather than on the way out of applyContext, which is also what lets the guard name them at all. beep boop
The Master API key variable is host-scoped, so a sentinel that spells FLAGSMITH_API_KEY into its own message names the wrong variable for every self-hosted instance — telling a user their FLAGSMITH_API_KEY holds a server-side key when what they set was FLAGSMITH_API_KEY_flagsmith_example_com. The rejections become predicates and loadCredential supplies the subject: the name envCredential actually read the value from. This is how config.ErrServerSideKey already reads against its file path. beep boop
|
Warning Review limit reached
Next review available in: 52 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThe CLI now supports host-scoped static credentials for self-hosted instances. It resolves separate Admin API and SDK URLs and generates credential hints from the active host. Authentication errors use source-neutral validation messages. OAuth discovery failures use a sentinel error. Browser login checks the server version and provides version-aware guidance for older instances. Tests cover credential resolution, dynamic hints, OAuth discovery, and version responses. Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: 🟡 Moderate · up to The change updates host-scoped credential resolution, but distinct IPv6 authorities can currently map to the same environment-variable name, potentially selecting the wrong credential and causing authentication failures. This concrete issue should be fixed before merging. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f5cbdf2f-c64c-4037-9535-89dd191c7973
📒 Files selected for processing (20)
README.mdgo.modinternal/api/version.gointernal/api/version_test.gointernal/auth/kind.gointernal/auth/oauth.gointernal/auth/oauth_test.gointernal/cmd/auth.gointernal/cmd/cmd_test.gointernal/cmd/context.gointernal/cmd/envcred.gointernal/cmd/envcred_test.gointernal/cmd/errors.gointernal/cmd/errors_test.gointernal/cmd/evaluate.gointernal/cmd/init.gointernal/cmd/login.gointernal/cmd/resolve.gointernal/cmd/root.gointernal/cmd/sentinel_test.go
Every non-200 was reported as "no authorization server metadata", so a rate-limited or briefly broken instance was described as one that has no OAuth support and hinted to check --api-url for a mistake that isn't there. A refusal that is not a 404 comes from a server that does have the document, and joins the other protocol surprises in this function as reportable. beep boop
An instance addressed by IPv6 literal scoped to FLAGSMITH_API_KEY_[__1]_8000, which no POSIX shell can export — so the variable could be neither set nor found, and the hints added here would have asked for it by name. The brackets are URL syntax rather than part of the host, so they are dropped instead of encoded. This does cost injectivity: `::` and `-` both land on `__`. Nothing reverses these names, and a name that cannot be typed is worse than one that cannot be reversed. beep boop
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
Can we check the config-load -> hint ordering? I proposed to fallback to the base but you might have something better in mind.
And suggesting to also add the scope encoding in the readme for clarity
A config file that fails to load aborts resolveContext before it publishes either surface URL, so a hint rendered from that error scoped its variable name to an empty host and asked for FLAGSMITH_ENVIRONMENT_KEY_ — a name that cannot be set and identifies no instance. Fall back to the unscoped name when no host is known. It is the right answer for the default host and the only answer available for a user whose instance URL was in the file that just failed to parse. Every test in the package shares the resolved-URL globals, so the one pinning this clears them: a real run is one command in its own process, which is the state that exposed this. beep boop
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cmd/envcred.go (1)
31-36: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftUse a one-to-one encoding for IPv6 hosts and ports.
http://[::1]:8000andhttp://[::1:8000]both produceFLAGSMITH_API_KEY___1_8000andFLAGSMITH_ENVIRONMENT_KEY___1_8000.envCredentialcan therefore select one authority’s credential for the other. Use distinct separators for the IPv6 host and port, and add collision tests for Admin API and SDK variable names ininternal/cmd/envcred_test.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ca1de01a-9777-42ec-8ea9-cdb280cafe55
📒 Files selected for processing (3)
internal/cmd/cmd_test.gointernal/cmd/envcred.gointernal/cmd/envcred_test.go
A single example of a host that needed no escaping left the reader to guess what happens to a hyphen, which is the one character whose encoding is not obvious. The example now carries a hyphen and a port, so it demonstrates every part of the rule it states. beep boop
5e4ce67 to
ae5d42f
Compare
2 participants
Closes #76.
Closes #77.
Accommodates auth-related errors for self-hosted use cases (host-scoped environment variables):
Additionally, adds a
/versionso people are offered to upgrade in case of missing OAuth metadata.