Skip to content

An unrecognised env name silently resolves to production in api.BaseURL #496

Description

@LukasWodka

What happens

api.BaseURL maps an env name to a backend host with no error case
(internal/api/client.go:86):

switch strings.ToLower(env) {
case EnvDev: return "https://dev-api.tracebloc.io"
case EnvStg: return "https://stg-api.tracebloc.io"
default:     return "https://api.tracebloc.io"
}

So every unrecognised value — a typo, an alias, a future env name, a plausible-looking
staging instead of stg — resolves to production. The doc comment says
"Unknown / empty → prod", so this was a choice rather than an oversight, which is why
this is a discussion issue and not a bug report.

The distinction being lost

"Unset" and "set to something I do not recognise" are different inputs, and they
currently produce the same host.

api.ResolveEnv defaulting to prod when nothing is configured is reasonable — no
config means the real thing. But it returns strings.ToLower(explicit) for whatever
it is given, and BaseURL then silently accepts it. A user who typed an env name
intended a specific backend; falling back to prod gives them the one place where
being wrong is most expensive.

Why the existing validator does not catch it

api.IsKnownEnv exists (client.go:115) and is correct. It has exactly one caller:
internal/cli/auth.go:63, inside login.

IsKnownEnv  ->  auth.go:63 (login) only
ResolveEnv  ->  auth.go:58, auth.go:314, client.go:136

client.go:136 returns api.ResolveEnv("") with no validation, and that value
reaches BaseURL. So the validator sits on the path a caller takes when it is
already being careful, and the silent default sits on the path everything else
takes.
Any caller that configures a session without going through login inherits
the fallback.

That shape is the actual finding. A guard that only runs on the careful path is
close to no guard: the careless path is the one that needs it.

Suggested change, and the trade-off

Have BaseURL (or a new BaseURLFor) return an error for an env that is neither
empty nor known, and keep empty → prod. Concretely:

  • empty / unset → prod, unchanged. This is the real default and should stay.
  • known (dev, stg, prod) → its host, unchanged.
  • anything else → an error naming the value and listing the known set.

The cost is that BaseURL gains an error return and its callers have to handle it.
Given there is currently one non-test caller (client.go:140), that is small today
and only gets larger.

A cheaper half-measure, if the signature change is unwelcome: call IsKnownEnv
wherever a session is constructed rather than only in login. That closes the same
hole without touching BaseURL, at the cost of relying on every future
session-construction site remembering to call it — which is the property that failed
here in the first place.

Also worth a look: aliases

If names like staging / production are ones users plausibly type, an explicit
alias table mapping them to stg / prod would be friendlier than either an error
or a silent prod fallback. Rejecting a typo is good; rejecting a reasonable synonym
is a papercut. That is a product call, not a correctness one.

How this surfaced

A caller that wrote a CLI config directly, rather than running login, passed its own
environment name and every request went to prod with a non-prod token. The caller was
wrong to assume its name matched the CLI's vocabulary — that half is fixed on the
caller's side. This issue is about the other half: the CLI resolved an env name it did
not recognise to the most dangerous available default and said nothing.

No repro against a customer environment is needed: BaseURL("staging") returning
https://api.tracebloc.io is the whole of it.

Metadata

Metadata

Assignees

No one assigned

    Labels

    work-type:tech-debtRefactor / cleanup, no behavior change

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions