diff --git a/README.md b/README.md index 51bca4a..401d6ba 100644 --- a/README.md +++ b/README.md @@ -44,17 +44,18 @@ A dataset is a named, single-portal, user-managed workspace meant to combine man - Writes are atomic and paged downloads are resumable: pages append to a per-download segment with the cursor saved, then merge into the store as a new version with an atomic swap. An interrupted `get` picks up from its last cursor, a truncated pull is honestly marked incomplete, and queries only ever see complete, duplicate-free data. - Datasets are purely local. `cc-data dataset delete` and `purge` only touch your disk. -Every `get`/`query`/`repl` names its dataset as `/`; a bare `` resolves under the optional `default_portal` config. +Every `get`/`query`/`repl` names its dataset as `/`; a bare `` resolves under the optional `default_portal` config. Both are hostnames: the environment aliases (`prod`/`staging`/`dev`) expand on `--portal`/`--server`, and are refused in a dataset ref and in `default_portal`, with the hostname to use named in the error. The portal there is also the on-disk folder, so expanding would give one dataset two names and taking it literally would file data under a portal that can never hold a credential. One parser owns every portal value, and it is what guarantees a portal is a single path component: `../wildfire` is refused rather than resolving outside your data root. ## Commands (sketch) ``` cc-data init # install the Claude skill + prompt for login -cc-data login --portal # browser loopback login; token stored per portal -cc-data logout # revoke the current token server-side +cc-data login [prod|staging|dev] # browser loopback login; the environment sets portal + server +cc-data login --portal # or name the portal directly; token stored per portal +cc-data logout --portal # revoke and remove that portal's stored token cc-data auth status # stored credentials per portal + default_portal -cc-data reports list --portal -cc-data reports jobs --portal # list a run's post-processing outputs +cc-data reports list --portal +cc-data reports jobs --portal # list a run's post-processing outputs cc-data dataset create|list|show|rename|edit|delete|purge|reindex ... # list/show take --json (and show --full) cc-data get report --dataset / [--job ] cc-data get answers --dataset / @@ -77,7 +78,7 @@ Every command has real per-command help; `cc-data get answers --help` is the aut - A presigned attachment URL (from `get attachments --url`) is a short-lived, credential-free capability to one student's file; don't paste it into shared or persistent channels. Prefer downloading into the dataset over `--url`. - **Manual token entry:** on a headless or SSH host, `cc-data login --token -` reads the token from stdin (piped, or an echo-off prompt on a TTY) — the recommended manual form. The bare `--token ` form works but is discouraged: flag values land in shell history and process lists. - **Dataset-folder trust boundary:** the DuckDB sandbox confines `query`/`repl` to the named dataset folders. On the bundled DuckDB the sandbox resolves symlinks, so this is a *writable-files* boundary, not a symlink escape: anyone who can write into a dataset folder can plant files your queries then read as trusted, so a dataset folder inherits the trust of whoever can write to it. -- **`--server` trust boundary:** `cc-data login --server ` drives the entire login and token exchange against ``. The CLI enforces an allowlist (a `concord.org`/`concordqa.org` host or subdomain, or a loopback host; http only for loopback) so a social-engineered `--server` cannot capture your login. Widening it is deliberately a code change. +- **`--server` trust boundary:** `cc-data login --server ` drives the entire login and token exchange against ``. The CLI enforces an allowlist (a `concord.org`/`concordqa.org` host or subdomain, or a loopback host; http only for loopback) so a social-engineered `--server` cannot capture your login. Widening it is deliberately a code change. The environment aliases (`prod`/`staging`/`dev`) expand to origins that are then checked against the same allowlist, so naming an environment is not a way around it. - Claude never drives the browser login. On an expired or missing token the CLI emits a structured `NOT_AUTHENTICATED` error and a human runs `cc-data login`. ## Development diff --git a/cmd/auth.go b/cmd/auth.go index 268518a..146b610 100644 --- a/cmd/auth.go +++ b/cmd/auth.go @@ -53,23 +53,35 @@ func renderStatus(res auth.StatusResult, check bool) { return } tw := tabwriter.NewWriter(out, 0, 2, 2, ' ', 0) + // Portal and server sit next to each other: a run belongs to a portal *and* + // the report server behind it, so the pair is what identifies a login. if check { - fmt.Fprintln(tw, "PORTAL\tBACKEND\tSTORED\tVALID\tLABEL\tLAST USED\tREPORT ACCESS") + fmt.Fprintln(tw, "PORTAL\tSERVER\tBACKEND\tSTORED\tVALID\tLABEL\tLAST USED\tREPORT ACCESS") } else { - fmt.Fprintln(tw, "PORTAL\tBACKEND\tSTORED") + fmt.Fprintln(tw, "PORTAL\tSERVER\tBACKEND\tSTORED") } for _, p := range res.Portals { if check { - fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\t%s\t%s\n", - p.Portal, p.Backend, fmtTime(p.StoredAt), + fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\n", + p.Portal, serverText(p.Server), p.Backend, fmtTime(p.StoredAt), validText(p), labelText(p.Label), lastUsedText(p), reportAccessText(p)) } else { - fmt.Fprintf(tw, "%s\t%s\t%s\n", p.Portal, p.Backend, fmtTime(p.StoredAt)) + fmt.Fprintf(tw, "%s\t%s\t%s\t%s\n", p.Portal, serverText(p.Server), p.Backend, fmtTime(p.StoredAt)) } } tw.Flush() } +// serverText renders a credential's recorded report server. A credential stored +// before the server was recorded has none, which is what makes its fetches fail; +// showing that as "-" is the point of the column. +func serverText(server string) string { + if server == "" { + return "-" + } + return server +} + func fmtTime(t time.Time) string { if t.IsZero() { return "-" diff --git a/cmd/auth_test.go b/cmd/auth_test.go new file mode 100644 index 0000000..be53af3 --- /dev/null +++ b/cmd/auth_test.go @@ -0,0 +1,48 @@ +package cmd + +import ( + "bytes" + "strings" + "testing" + "time" + + "github.com/concord-consortium/cc-data-cli/internal/auth" + "github.com/concord-consortium/cc-data-cli/internal/config" + "github.com/concord-consortium/cc-data-cli/internal/output" +) + +// TestRenderStatusShowsServer pins the report server in the table, not only in +// --json. A run id belongs to a portal and to the server behind it, so this is +// the column that answers "am I pointed at the environment that has my run?". +func TestRenderStatusShowsServer(t *testing.T) { + res := auth.StatusResult{ + Portals: []auth.PortalStatus{ + {Portal: config.StagingPortal, Server: config.StagingServer, Backend: "keychain", StoredAt: time.Unix(0, 0)}, + {Portal: config.DevPortal, Backend: "file", StoredAt: time.Unix(0, 0)}, + }, + } + for _, check := range []bool{false, true} { + var buf bytes.Buffer + restore := output.SetStreams(&buf, &buf) + renderStatus(res, check) + restore() + + got := buf.String() + lines := strings.Split(strings.TrimSpace(got), "\n") + if len(lines) != 3 { + t.Fatalf("check=%v: want a header and two rows, got:\n%s", check, got) + } + if !strings.Contains(lines[0], "SERVER") { + t.Errorf("check=%v: header has no SERVER column: %q", check, lines[0]) + } + if !strings.Contains(lines[1], config.StagingServer) { + t.Errorf("check=%v: staging row omits its server: %q", check, lines[1]) + } + // A credential with no recorded server reads as "-" in the SERVER column + // (the second field), not merely somewhere on the line. + fields := strings.Fields(lines[2]) + if len(fields) < 2 || fields[1] != "-" { + t.Errorf("check=%v: a serverless credential should show - in the SERVER column: %q", check, lines[2]) + } + } +} diff --git a/cmd/common.go b/cmd/common.go index b4f606e..f61c9f1 100644 --- a/cmd/common.go +++ b/cmd/common.go @@ -24,9 +24,22 @@ func loadRuntime() (*config.Config, string, error) { return cfg, root, nil } -// resolveRef parses a dataset ref against the configured default portal. +// resolveRef parses a dataset ref against the configured default portal. Only +// create uses it: a new dataset must name a portal the parser fully accepts. func resolveRef(cfg *config.Config, raw string) (dataset.Ref, error) { - ref, err := dataset.ParseRef(raw, cfg.DefaultPortal) + ref, err := dataset.ParseRefForConfig(cfg, raw) + if err != nil { + return dataset.Ref{}, output.Usagef("%v", err) + } + return ref, nil +} + +// resolveExistingRef is resolveRef for every command that names a dataset +// already on disk (show, delete, purge, rename, edit, reindex, get, query). It +// must reach anything dataset list shows, including a folder under a portal the +// parser would otherwise refuse, so the fix for such a folder is not rm -rf. +func resolveExistingRef(cfg *config.Config, dataRoot, raw string) (dataset.Ref, error) { + ref, err := dataset.ParseRefForExisting(cfg, dataRoot, raw) if err != nil { return dataset.Ref{}, output.Usagef("%v", err) } diff --git a/cmd/dataset.go b/cmd/dataset.go index 3d1b1be..6f2eeee 100644 --- a/cmd/dataset.go +++ b/cmd/dataset.go @@ -44,14 +44,18 @@ func newDatasetCreateCmd() *cobra.Command { return err } } else { - if cfg.DefaultPortal == "" { + defaultPortal, err := cfg.DefaultPortalValue() + if err != nil { + return output.Usagef("%v", err) + } + if defaultPortal.IsZero() { return output.Usagef("no dataset name given and no default_portal configured") } - name, err := dataset.AutoName(root, cfg.DefaultPortal, description) + name, err := dataset.AutoName(root, defaultPortal, description) if err != nil { return err } - ref = dataset.Ref{Portal: cfg.DefaultPortal, Name: name} + ref = dataset.Ref{Portal: defaultPortal, Name: name} } echoRef(ref) if _, err := dataset.Create(root, ref, description); err != nil { @@ -78,7 +82,7 @@ func newDatasetRenameCmd() *cobra.Command { if err != nil { return err } - ref, err := resolveRef(cfg, args[0]) + ref, err := resolveExistingRef(cfg, root, args[0]) if err != nil { return err } @@ -112,7 +116,7 @@ func newDatasetEditCmd() *cobra.Command { if err != nil { return err } - ref, err := resolveRef(cfg, args[0]) + ref, err := resolveExistingRef(cfg, root, args[0]) if err != nil { return err } @@ -143,7 +147,7 @@ func newDatasetDeleteCmd() *cobra.Command { if err != nil { return err } - ref, err := resolveRef(cfg, args[0]) + ref, err := resolveExistingRef(cfg, root, args[0]) if err != nil { return err } @@ -178,7 +182,7 @@ func newDatasetPurgeCmd() *cobra.Command { if err != nil { return err } - ref, err := resolveRef(cfg, args[0]) + ref, err := resolveExistingRef(cfg, root, args[0]) if err != nil { return err } diff --git a/cmd/dataset_reindex.go b/cmd/dataset_reindex.go index c9abc85..0fea502 100644 --- a/cmd/dataset_reindex.go +++ b/cmd/dataset_reindex.go @@ -16,7 +16,7 @@ func newReindexCmd() *cobra.Command { if err != nil { return err } - ref, err := resolveRef(cfg, args[0]) + ref, err := resolveExistingRef(cfg, root, args[0]) if err != nil { return err } diff --git a/cmd/dataset_show.go b/cmd/dataset_show.go index 100b731..1be1505 100644 --- a/cmd/dataset_show.go +++ b/cmd/dataset_show.go @@ -21,7 +21,7 @@ func newDatasetShowCmd() *cobra.Command { if err != nil { return err } - ref, err := resolveRef(cfg, args[0]) + ref, err := resolveExistingRef(cfg, root, args[0]) if err != nil { return err } diff --git a/cmd/get.go b/cmd/get.go index 89713bc..76f2151 100644 --- a/cmd/get.go +++ b/cmd/get.go @@ -25,7 +25,7 @@ func openDatasetForFetch(datasetRef string) (*dataset.Dataset, *api.Client, erro if err != nil { return nil, nil, err } - ref, err := resolveRef(cfg, datasetRef) + ref, err := resolveExistingRef(cfg, root, datasetRef) if err != nil { return nil, nil, err } diff --git a/cmd/login.go b/cmd/login.go index 3169f2e..e7321a6 100644 --- a/cmd/login.go +++ b/cmd/login.go @@ -17,79 +17,208 @@ import ( func newLoginCmd() *cobra.Command { var portal, token, server string cmd := &cobra.Command{ - Use: "login [--portal ]", + Use: "login [environment] [flags]", Short: "Log in to a portal via the PKCE loopback flow", - Long: `Log in to a portal and store an API token. + Long: fmt.Sprintf(`Log in to a portal and store an API token. -If --portal is omitted, the configured default_portal is used, falling back to -the production portal (learn.concord.org). +The optional environment argument sets the portal and its paired report server +in one word, so "cc-data login staging" points the staging portal at the staging +report server. The environments are %s. The same names work +on --portal and --server, and they expand independently, so +"--portal staging --server dev" is a staging portal against a local dev server. +A full hostname or URL still works on either flag, and each flag overrides its +own side of the environment argument. + +With no environment and no --portal, the configured default_portal is used, +falling back to the production portal (%s). + +The report server is chosen in this order: --server, the server paired with the +resolved portal, the server of the environment named as an argument, the +configured server_url, then the built-in default (%s). +A known portal always brings its own paired server, and naming an environment +covers a portal that has none, so server_url applies only to a portal reached +without either; when one of them overrides it, the login says which server it +used. The default flow opens a browser to complete a PKCE loopback login. For headless or SSH sessions, pass --token - to read a token from stdin (piped, or an echo-off prompt on a TTY); this is the recommended manual form. The bare --token form works but is discouraged: flag values land in shell history -and process lists.`, - Args: cobra.NoArgs, +and process lists.`, strings.Join(config.EnvironmentNames(), " / "), config.ProductionPortal, config.DefaultServerURL), + Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { cfg, err := config.Load() if err != nil { return err } - host, err := resolveLoginPortal(cfg, portal) + env, err := resolveEnvironmentArg(args) if err != nil { return err } - - serverOrigin, err := resolveLoginServer(cfg, server) + target, err := resolveLoginTarget(cfg, env, portal, server) if err != nil { return err } + // Say this before resolveToken: a manually pasted token is issued by + // one specific report server, so which server the login targets has + // to be on screen before the paste, not after it. + if target.ignoredServerURL != "" { + fmt.Fprintf(output.Stderr(), + "Using %s, %s; the configured server_url %s was not used (pass --server to override).\n", + target.server, target.serverReason(), target.ignoredServerURL) + } + rawToken, err := resolveToken(cmd, token) if err != nil { return err } return auth.Login(context.Background(), auth.LoginOptions{ - Portal: host, - Server: serverOrigin, - Token: rawToken, - Progress: output.Stderr(), + Portal: target.portal, + PortalOrigin: target.portalOrigin, + Server: target.server, + Token: rawToken, + Progress: output.Stderr(), }) }, } - cmd.Flags().StringVar(&portal, "portal", "", fmt.Sprintf("portal to log in to (default: default_portal, else %s)", config.ProductionPortal)) + cmd.Flags().StringVar(&portal, "portal", "", fmt.Sprintf("portal to log in to: an environment alias or a hostname (default: default_portal, else %s)", config.ProductionPortal)) cmd.Flags().StringVar(&token, "token", "", "store this token instead of the browser flow; use - to read from stdin (recommended for manual pastes)") - cmd.Flags().StringVar(&server, "server", "", "report server origin (default: config or https://report-server.concord.org; staging: https://report-server.concordqa.org)") + cmd.Flags().StringVar(&server, "server", "", fmt.Sprintf("report server: an environment alias or an origin (default: the environment's paired server, else config, else %s)", config.DefaultServerURL)) return cmd } -// resolveLoginPortal normalizes the --portal flag, else the configured -// default_portal, else the production portal, in that order. -func resolveLoginPortal(cfg *config.Config, flagVal string) (string, error) { - raw := flagVal - if raw == "" { - raw = cfg.DefaultPortal +// resolveEnvironmentArg resolves the optional positional environment argument. +// A value that is not a known alias is a usage error rather than a portal host: +// the positional slot exists only for the aliases, and silently treating a typo +// as a hostname would send a login somewhere unexpected. +func resolveEnvironmentArg(args []string) (*config.Environment, error) { + if len(args) == 0 { + return nil, nil + } + env, ok := config.LookupEnvironment(args[0]) + if !ok { + return nil, output.Usagef("unknown environment %q: expected one of %s (use --portal for a hostname)", + args[0], strings.Join(config.EnvironmentNames(), ", ")) + } + return &env, nil +} + +// loginTarget is the resolved portal host, the origin the auth flow sends that +// portal as, and the report-server origin for a login. +type loginTarget struct { + portal config.Portal + portalOrigin string + server string + // serverFrom says why the server was chosen, so the notice below can name + // the reason instead of claiming a pairing that may not exist. + serverFrom serverSource + // ignoredServerURL is a configured server_url the chosen server outranked. A + // login that quietly targets a server other than the one the config names is + // exactly the mismatch that later reads as "run not found", so the caller + // says so instead of letting it pass in silence. + ignoredServerURL string +} + +// serverSource is why a login's report server was chosen, for the notice that +// reports an outranked server_url. +type serverSource int + +const ( + serverFromNothing serverSource = iota + // serverFromPairing: the resolved portal is a known portal, which brings its + // own server. + serverFromPairing + // serverFromEnvironment: the portal is one no environment claims, and an + // environment named as an argument supplied the server instead. + serverFromEnvironment +) + +// resolveLoginTarget applies the precedence: an explicit flag wins over the +// environment argument, which wins over the configured defaults. --portal and +// --server expand independently so they can name different environments. +// +// The server is whatever --server states, else the one paired with the resolved +// portal host, else the one belonging to an environment named as an argument, +// else the configured server_url. The pairing outranks the environment argument +// so that "staging", "--portal staging", and +// "--portal learn.portal.staging.concord.org" run through one code path and land +// on the same server, reporting an overridden server_url identically; server_url +// is left to the portals neither the pairing nor a named environment covers. +func resolveLoginTarget(cfg *config.Config, env *config.Environment, portalFlag, serverFlag string) (loginTarget, error) { + var rawPortal, rawServer string + if env != nil { + rawPortal = env.Portal.Host() + } + if portalFlag != "" { + rawPortal = portalFlag } - if raw == "" { - raw = config.ProductionPortal + if serverFlag != "" { + rawServer = config.ExpandServerAlias(serverFlag) } - host, err := config.NormalizePortal(raw) + if rawPortal == "" { + rawPortal = cfg.DefaultPortal + } + if rawPortal == "" { + rawPortal = config.ProductionPortal + } + + host, portalOrigin, err := auth.ResolvePortalTarget(rawPortal) + if err != nil { + return loginTarget{}, output.Usagef("%v", err) + } + var ignoredServerURL string + from := serverFromNothing + if rawServer == "" { + var paired string + if paired, from = pairedServer(host, env); from != serverFromNothing { + rawServer = paired + if cfg.ServerURL != "" && cfg.ServerURL != rawServer { + ignoredServerURL = cfg.ServerURL + } + } else { + rawServer = cfg.ServerOrigin() + } + } + // Validate after expansion so an alias cannot widen the origin allowlist. + origin, err := config.ValidateServerURL(rawServer) if err != nil { - return "", output.Usagef("%v", err) + return loginTarget{}, output.Usagef("%v", err) } - return host, nil + return loginTarget{ + portal: host, + portalOrigin: portalOrigin, + server: origin, + serverFrom: from, + ignoredServerURL: ignoredServerURL, + }, nil } -func resolveLoginServer(cfg *config.Config, flagVal string) (string, error) { - if flagVal != "" { - origin, err := config.ValidateServerURL(flagVal) - if err != nil { - return "", output.Usagef("%v", err) - } - return origin, nil +// pairedServer returns the report server a login should use when --server was +// not given: the one paired with the resolved portal, else the one belonging to +// an environment named as an argument. That second case is what keeps +// "cc-data login dev --portal localhost:3005" a dev login; a portal no +// environment claims would otherwise fall through to the production default +// while the command line said dev. +func pairedServer(portal config.Portal, env *config.Environment) (string, serverSource) { + if pe, ok := config.EnvironmentForPortal(portal); ok { + return pe.Server, serverFromPairing + } + if env != nil { + return env.Server, serverFromEnvironment + } + return "", serverFromNothing +} + +// serverReason renders why the login used the server it did, for the notice. +// Naming a pairing that does not exist would send someone hunting the alias +// table for an entry they will not find. +func (t loginTarget) serverReason() string { + if t.serverFrom == serverFromEnvironment { + return "the report server for the environment you named" } - return cfg.ServerOrigin(), nil + return "the report server paired with " + t.portal.Host() } // resolveToken returns the raw token: "" for the PKCE flow, the flag value, or diff --git a/cmd/login_test.go b/cmd/login_test.go index ae49f56..a2c83d0 100644 --- a/cmd/login_test.go +++ b/cmd/login_test.go @@ -7,32 +7,337 @@ import ( "github.com/concord-consortium/cc-data-cli/internal/config" ) -func TestResolveLoginPortalDefaults(t *testing.T) { - // No flag, no default_portal -> production portal. - got, err := resolveLoginPortal(&config.Config{}, "") - if err != nil { - t.Fatal(err) +// testEnv builds the positional environment argument for a case. +func testEnv(t *testing.T, name string) *config.Environment { + t.Helper() + e, ok := config.LookupEnvironment(name) + if !ok { + t.Fatalf("no environment named %q", name) + } + return &e +} + +func TestResolveLoginTarget(t *testing.T) { + staging := &config.Config{DefaultPortal: config.StagingPortal} + cases := []struct { + name string + cfg *config.Config + env *config.Environment + portalFlag string + serverFlag string + wantPortal string + wantServer string + // wantOrigin defaults to https + wantPortal; set it only where the + // scheme is the point of the case. + wantOrigin string + // wantIgnored is the server_url the pairing outranked, "" where nothing + // was overridden. Every spelling of a portal has to agree on this as + // well as on the server, so it is pinned case by case rather than only + // where an override happens. + wantIgnored string + }{ + { + name: "no argument, no config: production defaults", + cfg: &config.Config{}, + wantPortal: config.ProductionPortal, + wantServer: config.DefaultServerURL, + }, + { + // The paired server follows the resolved host, so a default_portal + // spelled out in full still lands on that portal's own server. + name: "no argument: configured default_portal pairs its server", + cfg: staging, + wantPortal: config.StagingPortal, + wantServer: config.StagingServer, + }, + { + name: "a full staging hostname pairs the staging server", + cfg: &config.Config{}, + portalFlag: config.StagingPortal, + wantPortal: config.StagingPortal, + wantServer: config.StagingServer, + }, + { + name: "a full staging URL pairs the staging server", + cfg: &config.Config{}, + portalFlag: "https://learn.portal.staging.concord.org/", + wantPortal: config.StagingPortal, + wantServer: config.StagingServer, + }, + { + // An unrecognized portal has nothing to pair with and falls back. + name: "an unknown portal falls back to the default server", + cfg: &config.Config{}, + portalFlag: "learn.example.concord.org", + wantPortal: "learn.example.concord.org", + wantServer: config.DefaultServerURL, + }, + { + // ...unless an environment was named outright, which is what keeps a + // dev portal on a non-default port from silently exchanging its code + // at the production report server. + name: "a named environment serves a portal it does not claim", + cfg: &config.Config{}, + env: testEnv(t, "dev"), + portalFlag: "localhost:3005", + wantPortal: "localhost:3005", + wantServer: config.DevServer, + wantOrigin: "http://localhost:3005", + }, + { + name: "a named environment serves an unclaimed hostname", + cfg: &config.Config{}, + env: testEnv(t, "staging"), + portalFlag: "learn.example.concord.org", + wantPortal: "learn.example.concord.org", + wantServer: config.StagingServer, + }, + { + // The named environment outranks server_url the same way a pairing + // does, so the override is reported rather than passed over. + name: "a named environment beats server_url for an unclaimed portal", + cfg: &config.Config{ServerURL: config.StagingServer}, + env: testEnv(t, "prod"), + portalFlag: "learn.example.concord.org", + wantPortal: "learn.example.concord.org", + wantServer: config.DefaultServerURL, + wantIgnored: config.StagingServer, + }, + { + // A known portal brings its own server, outranking server_url... + name: "the pairing beats configured server_url", + cfg: &config.Config{ServerURL: config.StagingServer}, + portalFlag: config.ProductionPortal, + wantPortal: config.ProductionPortal, + wantServer: config.DefaultServerURL, + wantIgnored: config.StagingServer, + }, + { + // ...in the alias spelling of that same portal... + name: "the pairing beats server_url for a --portal alias too", + cfg: &config.Config{ServerURL: config.StagingServer}, + portalFlag: "prod", + wantPortal: config.ProductionPortal, + wantServer: config.DefaultServerURL, + wantIgnored: config.StagingServer, + }, + { + // ...and for an environment named outright. All three spellings of + // a portal must land on the same server in every configuration, and + // report the override the same way: the positional form is the one + // most people type, so it cannot be the one that stays silent. + name: "the pairing beats server_url for a named environment", + cfg: &config.Config{ServerURL: config.StagingServer}, + env: testEnv(t, "prod"), + wantPortal: config.ProductionPortal, + wantServer: config.DefaultServerURL, + wantIgnored: config.StagingServer, + }, + { + // server_url keeps the job no environment covers: a portal that has + // no paired server of its own. + name: "configured server_url serves a portal no environment claims", + cfg: &config.Config{ServerURL: config.StagingServer}, + portalFlag: "learn.example.concord.org", + wantPortal: "learn.example.concord.org", + wantServer: config.StagingServer, + }, + { + name: "positional staging pairs portal and server", + cfg: &config.Config{}, + env: testEnv(t, "staging"), + wantPortal: config.StagingPortal, + wantServer: config.StagingServer, + }, + { + name: "positional prod matches the bare-login default", + cfg: &config.Config{}, + env: testEnv(t, "prod"), + wantPortal: config.ProductionPortal, + wantServer: config.DefaultServerURL, + }, + { + name: "positional dev uses the loopback pair", + cfg: &config.Config{}, + env: testEnv(t, "dev"), + wantPortal: config.DevPortal, + wantServer: config.DevServer, + wantOrigin: "http://" + config.DevPortal, + }, + { + // An explicit https on a loopback portal survives to the auth URL; + // the stored portal identity stays the bare host either way. + name: "an https loopback portal keeps its scheme", + cfg: &config.Config{ServerURL: config.DevServer}, + portalFlag: "https://localhost:3001", + wantPortal: "localhost:3001", + wantServer: config.DevServer, + wantOrigin: "https://localhost:3001", + }, + { + name: "positional wins over default_portal", + cfg: &config.Config{DefaultPortal: config.ProductionPortal}, + env: testEnv(t, "staging"), + wantPortal: config.StagingPortal, + wantServer: config.StagingServer, + }, + { + name: "--portal alias carries its paired server", + cfg: &config.Config{}, + portalFlag: "staging", + wantPortal: config.StagingPortal, + wantServer: config.StagingServer, + }, + { + name: "--server alias sets only the server", + cfg: &config.Config{}, + serverFlag: "staging", + wantPortal: config.ProductionPortal, + wantServer: config.StagingServer, + }, + { + name: "mixed --portal staging --server dev", + cfg: &config.Config{}, + portalFlag: "staging", + serverFlag: "dev", + wantPortal: config.StagingPortal, + wantServer: config.DevServer, + }, + { + name: "flags override the positional environment", + cfg: &config.Config{}, + env: testEnv(t, "prod"), + portalFlag: "staging", + serverFlag: "dev", + wantPortal: config.StagingPortal, + wantServer: config.DevServer, + }, + { + name: "--portal overrides the positional pair, server included", + cfg: &config.Config{}, + env: testEnv(t, "prod"), + portalFlag: "staging", + wantPortal: config.StagingPortal, + wantServer: config.StagingServer, + }, + { + name: "full hostname and URL still work", + cfg: &config.Config{}, + portalFlag: "https://learn.portal.staging.concord.org/", + serverFlag: "https://report-server.concordqa.org", + wantPortal: config.StagingPortal, + wantServer: config.StagingServer, + }, + { + name: "a literal --server wins over the --portal alias pair", + cfg: &config.Config{}, + portalFlag: "staging", + serverFlag: "https://report-server.concord.org", + wantPortal: config.StagingPortal, + wantServer: config.DefaultServerURL, + }, } - if got != config.ProductionPortal { - t.Fatalf("empty flag + no default should use production %q, got %q", config.ProductionPortal, got) + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got, err := resolveLoginTarget(c.cfg, c.env, c.portalFlag, c.serverFlag) + if err != nil { + t.Fatal(err) + } + if got.portal.Host() != c.wantPortal { + t.Errorf("portal = %q, want %q", got.portal.Host(), c.wantPortal) + } + if got.server != c.wantServer { + t.Errorf("server = %q, want %q", got.server, c.wantServer) + } + wantOrigin := c.wantOrigin + if wantOrigin == "" { + wantOrigin = "https://" + c.wantPortal + } + if got.portalOrigin != wantOrigin { + t.Errorf("portalOrigin = %q, want %q", got.portalOrigin, wantOrigin) + } + if got.ignoredServerURL != c.wantIgnored { + t.Errorf("ignoredServerURL = %q, want %q", got.ignoredServerURL, c.wantIgnored) + } + }) } +} - // No flag, but a configured default_portal -> that portal. - got, err = resolveLoginPortal(&config.Config{DefaultPortal: "learn.portal.staging.concord.org"}, "") +// TestResolveLoginTargetReportsNoSpuriousOverride covers the cases that must +// stay silent about server_url. The cases that do report an override are pinned +// by wantIgnored in the table above, once per spelling of the portal. +func TestResolveLoginTargetReportsNoSpuriousOverride(t *testing.T) { + silent := []struct { + name string + cfg *config.Config + env *config.Environment + portalFlag string + serverFlag string + }{ + // Nothing was configured, so nothing was overridden. + {"no server_url", &config.Config{}, nil, config.ProductionPortal, ""}, + {"no server_url, positional", &config.Config{}, testEnv(t, "prod"), "", ""}, + // server_url already agrees with the pairing. + {"server_url matches the pairing", &config.Config{ServerURL: config.StagingServer}, nil, config.StagingPortal, ""}, + {"server_url matches the pairing, positional", &config.Config{ServerURL: config.StagingServer}, testEnv(t, "staging"), "", ""}, + // server_url was used, not overridden: this portal has no environment. + {"server_url is the one used", &config.Config{ServerURL: config.StagingServer}, nil, "learn.example.concord.org", ""}, + // An explicit --server is the user's own choice, not an override. + {"explicit --server", &config.Config{ServerURL: config.StagingServer}, nil, config.ProductionPortal, config.DefaultServerURL}, + {"explicit --server, positional", &config.Config{ServerURL: config.StagingServer}, testEnv(t, "prod"), "", config.DefaultServerURL}, + } + for _, c := range silent { + t.Run(c.name, func(t *testing.T) { + got, err := resolveLoginTarget(c.cfg, c.env, c.portalFlag, c.serverFlag) + if err != nil { + t.Fatal(err) + } + if got.ignoredServerURL != "" { + t.Errorf("ignoredServerURL = %q, want no override reported", got.ignoredServerURL) + } + }) + } +} + +// TestResolveLoginTargetRejectsAliasTypo pins that a near-miss alias on --portal +// is an error rather than a portal host of its own; the positional slot already +// refused it, and the flag must not be the way around that. +func TestResolveLoginTargetRejectsAliasTypo(t *testing.T) { + if _, err := resolveLoginTarget(&config.Config{}, nil, "stagng", ""); err == nil { + t.Fatal("a misspelled alias on --portal should be rejected") + } +} + +// TestResolveLoginTargetRejectsDisallowedServer checks that a literal --server +// is still allowlist-checked after alias expansion. The complementary guarantee, +// that no alias can expand to a disallowed origin in the first place, comes from +// config.TestEnvironmentPairsAreValid. +func TestResolveLoginTargetRejectsDisallowedServer(t *testing.T) { + if _, err := resolveLoginTarget(&config.Config{}, nil, "", "https://evil.example.com"); err == nil { + t.Fatal("a non-allowlisted server origin should be rejected") + } +} + +func TestResolveEnvironmentArg(t *testing.T) { + got, err := resolveEnvironmentArg(nil) if err != nil { t.Fatal(err) } - if got != "learn.portal.staging.concord.org" { - t.Fatalf("empty flag should use default_portal, got %q", got) + if got != nil { + t.Fatalf("no argument should resolve to no environment, got %+v", got) } - // Explicit flag wins over default_portal. - got, err = resolveLoginPortal(&config.Config{DefaultPortal: "learn.portal.staging.concord.org"}, "learn.concord.org") + got, err = resolveEnvironmentArg([]string{"STAGING"}) if err != nil { t.Fatal(err) } - if got != "learn.concord.org" { - t.Fatalf("flag should win, got %q", got) + if got == nil || got.Portal.Host() != config.StagingPortal { + t.Fatalf("alias should match case-insensitively, got %+v", got) + } + + // A hostname in the positional slot is a typo, not a portal. + if _, err := resolveEnvironmentArg([]string{"learn.concord.org"}); err == nil { + t.Fatal("an unknown environment should be a usage error") } } diff --git a/cmd/logout.go b/cmd/logout.go index 3e4bda8..1827974 100644 --- a/cmd/logout.go +++ b/cmd/logout.go @@ -5,7 +5,6 @@ import ( "github.com/concord-consortium/cc-data-cli/internal/api" "github.com/concord-consortium/cc-data-cli/internal/auth" - "github.com/concord-consortium/cc-data-cli/internal/config" "github.com/concord-consortium/cc-data-cli/internal/output" "github.com/spf13/cobra" ) @@ -13,14 +12,14 @@ import ( func newLogoutCmd() *cobra.Command { var portal string cmd := &cobra.Command{ - Use: "logout --portal ", + Use: "logout --portal ", Short: "Revoke and remove a portal's stored token", Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { if portal == "" { return output.Usagef("--portal is required") } - host, err := config.NormalizePortal(portal) + host, _, err := auth.ResolvePortalTarget(portal) if err != nil { return output.Usagef("%v", err) } @@ -30,6 +29,6 @@ func newLogoutCmd() *cobra.Command { return nil }, } - cmd.Flags().StringVar(&portal, "portal", "", "portal to log out of") + cmd.Flags().StringVar(&portal, "portal", "", "portal to log out of: an environment alias or a hostname") return cmd } diff --git a/cmd/query.go b/cmd/query.go index 379cc25..2dc8d83 100644 --- a/cmd/query.go +++ b/cmd/query.go @@ -85,7 +85,7 @@ func resolveDatasetSpecs(refs []string) ([]duck.DatasetSpec, error) { if i := strings.Index(raw, "="); i >= 0 { alias, refStr = raw[:i], raw[i+1:] } - ref, err := resolveRef(cfg, refStr) + ref, err := resolveExistingRef(cfg, root, refStr) if err != nil { return nil, err } diff --git a/cmd/repl_engine_test.go b/cmd/repl_engine_test.go index fff97b1..8739d7e 100644 --- a/cmd/repl_engine_test.go +++ b/cmd/repl_engine_test.go @@ -10,6 +10,7 @@ import ( "strings" "testing" + "github.com/concord-consortium/cc-data-cli/internal/config" "github.com/concord-consortium/cc-data-cli/internal/dataset" "github.com/concord-consortium/cc-data-cli/internal/duck" "github.com/concord-consortium/cc-data-cli/internal/fsutil" @@ -17,7 +18,7 @@ import ( func TestReplExecuteAndDotTables(t *testing.T) { root := t.TempDir() - d, err := dataset.Create(root, dataset.Ref{Portal: "learn.concord.org", Name: "ds"}, "") + d, err := dataset.Create(root, dataset.Ref{Portal: config.MustPortal("learn.concord.org"), Name: "ds"}, "") if err != nil { t.Fatal(err) } diff --git a/cmd/reports.go b/cmd/reports.go index a0278b4..d59de5c 100644 --- a/cmd/reports.go +++ b/cmd/reports.go @@ -8,6 +8,7 @@ import ( "text/tabwriter" "github.com/concord-consortium/cc-data-cli/internal/api" + "github.com/concord-consortium/cc-data-cli/internal/auth" "github.com/concord-consortium/cc-data-cli/internal/config" "github.com/concord-consortium/cc-data-cli/internal/output" "github.com/concord-consortium/cc-data-cli/internal/reportview" @@ -23,18 +24,22 @@ func newReportsCmd() *cobra.Command { return cmd } -// resolvePortal normalizes the --portal flag or the configured default. -func resolvePortal(cfg *config.Config, flagVal string) (string, error) { +// resolvePortal resolves the --portal flag or the configured default to a +// portal host. These commands have no --server flag (the server comes from the +// stored credential), so only the portal side of an environment alias applies. +// A portal only a stored credential vouches for is reachable here for the same +// reason it is at logout: its runs would otherwise be impossible to list. +func resolvePortal(cfg *config.Config, flagVal string) (config.Portal, error) { raw := flagVal if raw == "" { raw = cfg.DefaultPortal } if raw == "" { - return "", output.Usagef("--portal is required (no default_portal configured)") + return config.Portal{}, output.Usagef("--portal is required (no default_portal configured)") } - host, err := config.NormalizePortal(raw) + host, _, err := auth.ResolvePortalTarget(raw) if err != nil { - return "", output.Usagef("%v", err) + return config.Portal{}, output.Usagef("%v", err) } return host, nil } @@ -43,7 +48,7 @@ func newReportsListCmd() *cobra.Command { var portal string var asJSON bool cmd := &cobra.Command{ - Use: "list --portal ", + Use: "list --portal ", Short: "List the user's report runs", Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { @@ -70,7 +75,7 @@ func newReportsListCmd() *cobra.Command { return nil }, } - cmd.Flags().StringVar(&portal, "portal", "", "portal to list runs for") + cmd.Flags().StringVar(&portal, "portal", "", "portal to list runs for: an environment alias or a hostname") cmd.Flags().BoolVar(&asJSON, "json", false, "emit JSON instead of a table") return cmd } @@ -79,7 +84,7 @@ func newReportsJobsCmd() *cobra.Command { var portal string var asJSON bool cmd := &cobra.Command{ - Use: "jobs --portal ", + Use: "jobs --portal ", Short: "List a run's post-processing jobs", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { @@ -110,7 +115,7 @@ func newReportsJobsCmd() *cobra.Command { return nil }, } - cmd.Flags().StringVar(&portal, "portal", "", "portal the run belongs to") + cmd.Flags().StringVar(&portal, "portal", "", "portal the run belongs to: an environment alias or a hostname") cmd.Flags().BoolVar(&asJSON, "json", false, "emit JSON instead of a table") return cmd } diff --git a/cmd/reports_test.go b/cmd/reports_test.go new file mode 100644 index 0000000..f17ae80 --- /dev/null +++ b/cmd/reports_test.go @@ -0,0 +1,43 @@ +package cmd + +import ( + "testing" + + "github.com/concord-consortium/cc-data-cli/internal/config" +) + +// TestResolvePortal covers what reports list and reports jobs add over +// auth.ResolvePortalTarget: the default_portal fallback layered on top of it. +// The accepted portal spellings themselves are config.TestPortalMatrix's to pin, +// so only one alias case appears here, as a check that expansion is reached at +// all. There is no --server on these commands; the server comes from the stored +// credential. +func TestResolvePortal(t *testing.T) { + cases := []struct { + name string + cfg *config.Config + flagVal string + want string + }{ + {"the flag is alias-expanded", &config.Config{}, "staging", config.StagingPortal}, + {"flag wins over default_portal", &config.Config{DefaultPortal: config.ProductionPortal}, "staging", config.StagingPortal}, + {"empty flag falls back to default_portal", &config.Config{DefaultPortal: config.ProductionPortal}, "", config.ProductionPortal}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got, err := resolvePortal(c.cfg, c.flagVal) + if err != nil { + t.Fatal(err) + } + if got.Host() != c.want { + t.Fatalf("resolvePortal(%q) = %q, want %q", c.flagVal, got.Host(), c.want) + } + }) + } +} + +func TestResolvePortalRequiresAPortal(t *testing.T) { + if _, err := resolvePortal(&config.Config{}, ""); err == nil { + t.Fatal("no flag and no default_portal should be a usage error") + } +} diff --git a/cmd/uninstall.go b/cmd/uninstall.go index b128ffe..8ce71ff 100644 --- a/cmd/uninstall.go +++ b/cmd/uninstall.go @@ -83,7 +83,10 @@ func removeCredentials(errOut io.Writer) error { // uninstall for minutes per portal (the API client has no overall // timeout, only per-attempt deadlines across its retry budget). ctx, cancel := context.WithTimeout(context.Background(), revokeTimeout) - lerr := auth.Logout(ctx, info.Portal, errOut) + // The host comes back from our own credential store, so it is adopted + // rather than re-parsed: uninstall has to be able to revoke everything + // that is stored, including a legacy single-label portal. + lerr := auth.Logout(ctx, config.AdoptStoredPortal(info.Portal), errOut) cancel() if lerr != nil { fmt.Fprintf(errOut, "warning: could not revoke token for %s: %v (it may still be active; revoke it in the token UI)\n", info.Portal, lerr) diff --git a/docs/researcher-guide.md b/docs/researcher-guide.md index a8d5f40..85e3b9d 100644 --- a/docs/researcher-guide.md +++ b/docs/researcher-guide.md @@ -110,9 +110,26 @@ in your config folder). You stay logged in until the token expires or you `cc-data logout`. - With no options, login targets the **production portal, `learn.concord.org`**. -- To use a different portal, pass `--portal`, e.g. - `cc-data login --portal learn.portal.staging.concord.org`. Each portal is a - separate login with its own data. +- To use a different portal, name an environment: `cc-data login staging`. The + environments researchers use are `prod` and `staging` (`prod` also accepts + `production`, and `staging` also accepts `stage`); there is a third, `dev`, + that points at a portal and report server running on your own machine, for + people developing `cc-data` itself. Naming an environment this way sets both + the portal and the report server that goes with it, so a staging login does + not end up pointed at the production server unless you explicitly say + `--server`. Each portal is a separate login with its own data. +- `--portal` still takes a full hostname when you need one that has no short + name, e.g. `cc-data login --portal learn.portal.staging.concord.org`, and the + environment names work there too (`--portal staging`). Every spelling of a + known portal brings the same paired report server with it. +- Which report server a login uses is decided in this order: an explicit + `--server`, then the server paired with the portal you landed on, then the + server of the environment you named, then the `server_url` in your config, + then the built-in production default. A known portal always brings its own + paired server, and naming an environment covers a portal that has none (so + `cc-data login dev --portal localhost:3005` still talks to the dev report + server), which leaves `server_url` for a portal you reach without either; when + something outranks it, the login prints a line saying which server it used. - Logins are per portal: you can be logged into several at once, and each dataset belongs to exactly one of them. @@ -203,8 +220,8 @@ cc-data reports list This lists your report runs with their `run_id`, `slug`, and state (add `--json` to include the `report_type`). (Add `--portal ` for a non-production -portal.) Through Claude, the same thing is just asking "what report runs do I -have?" +portal; `--portal staging` works as well as the full hostname.) Through Claude, +the same thing is just asking "what report runs do I have?" ### A complete session @@ -258,7 +275,12 @@ dataset named `wildfire` on `learn.concord.org` lives in - A dataset is identified by `/`, e.g. `learn.concord.org/wildfire`. (If you set a default portal, a bare `wildfire` - works too.) + works too.) The portal here is always a full hostname, never an environment + name: it is also the folder your data lives in. `staging/wildfire` is refused + with the hostname to use, rather than quietly making a dataset under a portal + called `staging` that you could never log in to. For the same reason the + `default_portal` setting takes a full hostname, and `cc-data` refuses to start + if you put an environment name there. - Into a dataset you can pull four kinds of data for any run: - **report**: the report CSV (answers reports, and log/action reports). - **answers**: the raw student answer records. @@ -339,7 +361,7 @@ report types listed above. ## 6. What kinds of questions can I ask? Here's the range of questions the data supports, grouped by the kind of data they -draw on. You can ask any of these of Claude in plain English — **you don't need to +draw on. You can ask any of these of Claude in plain English: **you don't need to know SQL.** The SQL snippets shown throughout this section are just there to illustrate what Claude writes and runs for you under the hood; you can read them to see what's happening, or copy them into `cc-data query --dataset "..."` if @@ -475,8 +497,8 @@ write the cross-dataset query for you. You're almost certainly pointed at a different portal or report server than the one that has the run. Run IDs are specific to a portal *and* to the report server behind it. Check what your login targets with `cc-data auth status`, and re-run - `cc-data login` with the right `--portal` (and `--server`, if you use a - non-default report server) if it's wrong. + `cc-data login` for the right environment if it's wrong: `cc-data login staging` + sets the portal and its report server together, which is the usual fix for this. - **"dataset is busy" / "download busy".** Another `get` or query is already running against that dataset. Wait for it to finish and retry; `cc-data` serializes access so a dataset is never left half-written. diff --git a/internal/api/portal.go b/internal/api/portal.go index 05c33cb..182027c 100644 --- a/internal/api/portal.go +++ b/internal/api/portal.go @@ -1,6 +1,7 @@ package api import ( + "github.com/concord-consortium/cc-data-cli/internal/config" "github.com/concord-consortium/cc-data-cli/internal/creds" "github.com/concord-consortium/cc-data-cli/internal/output" ) @@ -8,7 +9,7 @@ import ( // ForPortal builds a client whose base URL is the portal credential's recorded // minting server, never the global config server_url, so a bearer token is only // ever sent to the origin that issued it. -func ForPortal(portal string) (*Client, error) { +func ForPortal(portal config.Portal) (*Client, error) { var store creds.Store token, server, err := store.Get(portal) if err != nil { diff --git a/internal/api/portal_test.go b/internal/api/portal_test.go index 7e293c8..20bae21 100644 --- a/internal/api/portal_test.go +++ b/internal/api/portal_test.go @@ -40,11 +40,11 @@ func TestOriginRuleUsesCredentialServer(t *testing.T) { } var store creds.Store - if err := store.Save("learn.concord.org", "ccd_portalA", credServer.URL); err != nil { + if err := store.Save(config.MustPortal("learn.concord.org"), "ccd_portalA", credServer.URL); err != nil { t.Fatal(err) } - cl, err := ForPortal("learn.concord.org") + cl, err := ForPortal(config.MustPortal("learn.concord.org")) if err != nil { t.Fatal(err) } @@ -66,7 +66,7 @@ func TestForPortalNotAuthedWhenAbsent(t *testing.T) { t.Setenv("USERPROFILE", home) } keyring.MockInit() - _, err := ForPortal("missing.concord.org") + _, err := ForPortal(config.MustPortal("missing.concord.org")) cliErr := err if cliErr == nil { t.Fatal("expected NOT_AUTHENTICATED") diff --git a/internal/auth/integration_test.go b/internal/auth/integration_test.go index 4dadafa..360085e 100644 --- a/internal/auth/integration_test.go +++ b/internal/auth/integration_test.go @@ -13,6 +13,7 @@ import ( "sync" "testing" + "github.com/concord-consortium/cc-data-cli/internal/config" "github.com/concord-consortium/cc-data-cli/internal/creds" "github.com/zalando/go-keyring" ) @@ -186,7 +187,7 @@ func TestFullAuthFlowCurrentServer(t *testing.T) { var progress strings.Builder err := Login(context.Background(), LoginOptions{ - Portal: "learn.concord.org", + Portal: config.MustPortal("learn.concord.org"), Server: fs.URL, Progress: &progress, }) @@ -199,7 +200,7 @@ func TestFullAuthFlowCurrentServer(t *testing.T) { // Stored credential should point at the fake server. var store creds.Store - _, server, err := store.Get("learn.concord.org") + _, server, err := store.Get(config.MustPortal("learn.concord.org")) if err != nil { t.Fatal(err) } @@ -220,10 +221,10 @@ func TestFullAuthFlowCurrentServer(t *testing.T) { } // logout revokes and removes local. - if err := Logout(context.Background(), "learn.concord.org", &progress); err != nil { + if err := Logout(context.Background(), config.MustPortal("learn.concord.org"), &progress); err != nil { t.Fatalf("logout: %v", err) } - if _, _, err := store.Get("learn.concord.org"); err == nil { + if _, _, err := store.Get(config.MustPortal("learn.concord.org")); err == nil { t.Fatal("credential should be gone after logout") } } @@ -235,7 +236,7 @@ func TestAuthFlowOlderServerDegradation(t *testing.T) { defer browserStub(t)() var progress strings.Builder - if err := Login(context.Background(), LoginOptions{Portal: "learn.concord.org", Server: fs.URL, Progress: &progress}); err != nil { + if err := Login(context.Background(), LoginOptions{Portal: config.MustPortal("learn.concord.org"), Server: fs.URL, Progress: &progress}); err != nil { t.Fatalf("login: %v", err) } @@ -250,14 +251,14 @@ func TestAuthFlowOlderServerDegradation(t *testing.T) { } // logout on the older server warns but still deletes locally and succeeds. - if err := Logout(context.Background(), "learn.concord.org", &progress); err != nil { + if err := Logout(context.Background(), config.MustPortal("learn.concord.org"), &progress); err != nil { t.Fatalf("older-server logout should succeed: %v", err) } if !strings.Contains(progress.String(), "may still be active") { t.Fatalf("expected older-server logout warning, got: %s", progress.String()) } var store creds.Store - if _, _, err := store.Get("learn.concord.org"); err == nil { + if _, _, err := store.Get(config.MustPortal("learn.concord.org")); err == nil { t.Fatal("credential should be gone after older-server logout") } } @@ -269,11 +270,11 @@ func TestLogoutAlreadyRevoked(t *testing.T) { // Store a credential whose token the server does not know (already revoked). var store creds.Store - if err := store.Save("learn.concord.org", "ccd_dead", fs.URL); err != nil { + if err := store.Save(config.MustPortal("learn.concord.org"), "ccd_dead", fs.URL); err != nil { t.Fatal(err) } var progress strings.Builder - if err := Logout(context.Background(), "learn.concord.org", &progress); err != nil { + if err := Logout(context.Background(), config.MustPortal("learn.concord.org"), &progress); err != nil { t.Fatalf("logout with dead token should still succeed: %v", err) } if !strings.Contains(progress.String(), "nothing needed revoking") { diff --git a/internal/auth/login.go b/internal/auth/login.go index f8830e9..1addb88 100644 --- a/internal/auth/login.go +++ b/internal/auth/login.go @@ -16,14 +16,17 @@ import ( // DefaultLoginTimeout matches the server's grant TTL. const DefaultLoginTimeout = 5 * time.Minute -// LoginOptions parameterizes a login. Portal is a normalized host and Server a +// LoginOptions parameterizes a login. Portal is a parsed portal and Server a // validated origin; a non-empty Token skips the PKCE flow and stores directly. +// PortalOrigin is the scheme-bearing form of Portal for the auth URL; when empty +// it is derived from Portal. type LoginOptions struct { - Portal string - Server string - Token string - Timeout time.Duration - Progress io.Writer + Portal config.Portal + PortalOrigin string + Server string + Token string + Timeout time.Duration + Progress io.Writer } // Login runs the PKCE loopback flow (or stores a manual token) and saves the @@ -70,7 +73,11 @@ func pkceFlow(ctx context.Context, opts LoginOptions) (string, error) { } defer lb.Close() - authURL := buildAuthURL(opts.Server, opts.Portal, lb.RedirectURI(), state, challenge) + portalOrigin := opts.PortalOrigin + if portalOrigin == "" { + portalOrigin = opts.Portal.Origin() + } + authURL := buildAuthURL(opts.Server, portalOrigin, lb.RedirectURI(), state, challenge) RedirectBrowserOutput(opts.Progress) fmt.Fprintf(opts.Progress, "Opening browser to log in. If it does not open, visit:\n%s\n", authURL) _ = OpenBrowser(authURL) @@ -90,9 +97,9 @@ func pkceFlow(ctx context.Context, opts LoginOptions) (string, error) { return token, nil } -func buildAuthURL(server, portalHost, redirectURI, state, challenge string) string { +func buildAuthURL(server, portalOrigin, redirectURI, state, challenge string) string { q := url.Values{} - q.Set("portal", config.PortalOrigin(portalHost)) + q.Set("portal", portalOrigin) q.Set("redirect_uri", redirectURI) q.Set("state", state) q.Set("code_challenge", challenge) diff --git a/internal/auth/logout.go b/internal/auth/logout.go index 5d066e4..d4a54fb 100644 --- a/internal/auth/logout.go +++ b/internal/auth/logout.go @@ -7,6 +7,7 @@ import ( "io" "github.com/concord-consortium/cc-data-cli/internal/api" + "github.com/concord-consortium/cc-data-cli/internal/config" "github.com/concord-consortium/cc-data-cli/internal/creds" ) @@ -14,7 +15,7 @@ import ( // (already invalid) or a 404 (older server without the revoke route) still // removes the local credential and exits successfully; only an unexpected error // aborts before local deletion. -func Logout(ctx context.Context, portal string, progress io.Writer) error { +func Logout(ctx context.Context, portal config.Portal, progress io.Writer) error { var store creds.Store token, server, err := store.Get(portal) if errors.Is(err, creds.ErrNotFound) { diff --git a/internal/auth/pkce_test.go b/internal/auth/pkce_test.go index 22fc166..78cd848 100644 --- a/internal/auth/pkce_test.go +++ b/internal/auth/pkce_test.go @@ -44,7 +44,7 @@ func TestGenerateStateUnique(t *testing.T) { } func TestBuildAuthURL(t *testing.T) { - u := buildAuthURL("https://report-server.concord.org", "learn.concord.org", "http://127.0.0.1:5000/callback", "st", "ch") + u := buildAuthURL("https://report-server.concord.org", "https://learn.concord.org", "http://127.0.0.1:5000/callback", "st", "ch") for _, needle := range []string{ "portal=https%3A%2F%2Flearn.concord.org", "redirect_uri=http%3A%2F%2F127.0.0.1%3A5000%2Fcallback", diff --git a/internal/auth/portal.go b/internal/auth/portal.go new file mode 100644 index 0000000..97990b5 --- /dev/null +++ b/internal/auth/portal.go @@ -0,0 +1,68 @@ +package auth + +import ( + "errors" + "fmt" + + "github.com/concord-consortium/cc-data-cli/internal/config" + "github.com/concord-consortium/cc-data-cli/internal/creds" +) + +// ResolvePortalTarget parses a portal the CLI is about to talk to, and is the +// one entry point every portal-scoped command uses: login, logout, the reports +// commands, and the MCP portal arguments. It adds a single exception to +// config.ParsePortalTarget: a *config.UnshapedHostError host (a single-label +// non-loopback name the shape check turns away) is still accepted when a +// credential is actually stored under it. +// +// An older build accepted single-label hosts at login and auth status still +// lists what it stored, so refusing them outright would leave such a credential +// visible with no way to revoke it, its runs impossible to list, and, because +// login refuses it too, no way to refresh the token once it expires. A portal +// nothing is stored under stays a typo. +// +// The exception does NOT cover a credential stored under a literal environment +// alias name (e.g. "staging", which a pre-alias build stored as-is): on the +// target path ParsePortalTarget expands the alias before any shape check, so the +// input never reaches here as an unshaped host, and "logout --portal staging" +// resolves to the expanded hostname instead. Such a credential is still +// revocable with "cc-data uninstall", which adopts every stored host directly. +// Preferring the literal here would misroute a fresh "login staging", which must +// expand, so the gap is left to uninstall rather than papered over in the shared +// resolver. +func ResolvePortalTarget(v string) (config.Portal, string, error) { + portal, origin, err := config.ParsePortalTarget(v) + if err == nil { + return portal, origin, nil + } + var unshaped *config.UnshapedHostError + if !errors.As(err, &unshaped) { + return config.Portal{}, "", err + } + stored, listErr := hasStoredCredential(unshaped.Portal) + if stored { + return unshaped.Portal, unshaped.Portal.Origin(), nil + } + if listErr != nil { + // A store that cannot be read is not a typo. Reporting only the shape + // error here would send the user to fix their spelling when the real + // problem is that the credential listing failed. + return config.Portal{}, "", fmt.Errorf("%w (could not read stored credentials: %v)", err, listErr) + } + return config.Portal{}, "", err +} + +// hasStoredCredential reports whether a portal has a credential on disk. It +// reads the offline listing only, so it never prompts for a keyring unlock. +func hasStoredCredential(portal config.Portal) (bool, error) { + infos, err := creds.Store{}.List() + if err != nil { + return false, err + } + for _, info := range infos { + if info.Portal == portal.Host() { + return true, nil + } + } + return false, nil +} diff --git a/internal/auth/portal_test.go b/internal/auth/portal_test.go new file mode 100644 index 0000000..edf51e6 --- /dev/null +++ b/internal/auth/portal_test.go @@ -0,0 +1,97 @@ +package auth + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/concord-consortium/cc-data-cli/internal/config" + "github.com/concord-consortium/cc-data-cli/internal/creds" +) + +// TestResolvePortalTarget spot-checks that expansion is reached at all; the +// accepted spellings themselves are config.TestPortalMatrix's to pin. +func TestResolvePortalTarget(t *testing.T) { + setupCredHome(t) + + cases := []struct{ in, want, wantOrigin string }{ + {"staging", config.StagingPortal, "https://" + config.StagingPortal}, + {config.ProductionPortal, config.ProductionPortal, "https://" + config.ProductionPortal}, + {"https://learn.concord.org/", config.ProductionPortal, "https://" + config.ProductionPortal}, + {"localhost:3000", config.DevPortal, "http://" + config.DevPortal}, + {"https://localhost:3001", "localhost:3001", "https://localhost:3001"}, + } + for _, c := range cases { + got, origin, err := ResolvePortalTarget(c.in) + if err != nil { + t.Errorf("ResolvePortalTarget(%q) error: %v", c.in, err) + continue + } + if got.Host() != c.want || origin != c.wantOrigin { + t.Errorf("ResolvePortalTarget(%q) = (%q, %q), want (%q, %q)", c.in, got.Host(), origin, c.want, c.wantOrigin) + } + } + + // With nothing stored under it, a single-label host is a typo'd alias. + if _, _, err := ResolvePortalTarget("stagng"); err == nil { + t.Error("a misspelled alias with no stored credential should be rejected") + } +} + +// TestResolvePortalTargetReachesLegacyCredential covers the exception this +// resolver makes to the portal shape check. An older build stored credentials +// under single-label hosts; auth status still lists them, so refusing them would +// leave one visible with no way to revoke it, its runs impossible to list, and no +// way to refresh the token once it expires. +func TestResolvePortalTargetReachesLegacyCredential(t *testing.T) { + setupCredHome(t) + + legacy := config.AdoptStoredPortal("myportal") + var store creds.Store + if err := store.Save(legacy, "ccd_abc", config.DefaultServerURL); err != nil { + t.Fatal(err) + } + got, origin, err := ResolvePortalTarget("myportal") + if err != nil { + t.Fatalf("a stored single-label portal should be reachable: %v", err) + } + if got != legacy { + t.Errorf("ResolvePortalTarget = %q, want %q", got.Host(), "myportal") + } + if origin != "https://myportal" { + t.Errorf("origin = %q, want %q", origin, "https://myportal") + } + + // The exception is only for what is actually stored: a near-miss of the + // stored host is still a typo. + if _, _, err := ResolvePortalTarget("myportl"); err == nil { + t.Error("an unstored single-label portal should still be rejected") + } +} + +// TestResolvePortalArgReportsUnreadableStore keeps a broken credential store +// from reading as a misspelled portal. The shape error alone would send the user +// to fix their spelling when the real problem is that the listing failed. +func TestResolvePortalTargetReportsUnreadableStore(t *testing.T) { + setupCredHome(t) + + dir, err := config.ConfigDir() + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "credentials.json"), []byte("{not json"), 0o600); err != nil { + t.Fatal(err) + } + + _, _, err = ResolvePortalTarget("myportal") + if err == nil { + t.Fatal("an unreadable store should not resolve a single-label portal") + } + if !strings.Contains(err.Error(), "could not read stored credentials") { + t.Errorf("error should name the listing failure: %v", err) + } +} diff --git a/internal/auth/status.go b/internal/auth/status.go index 41a760e..c149512 100644 --- a/internal/auth/status.go +++ b/internal/auth/status.go @@ -55,14 +55,17 @@ func Status(ctx context.Context, check bool) (StatusResult, error) { StoredAt: info.StoredAt, } if check { - checkPortal(ctx, info.Portal, &ps) + // The host comes back from our own credential store, so it is adopted + // rather than re-parsed: a legacy single-label portal still has to be + // checkable here. + checkPortal(ctx, config.AdoptStoredPortal(info.Portal), &ps) } res.Portals = append(res.Portals, ps) } return res, nil } -func checkPortal(ctx context.Context, portal string, ps *PortalStatus) { +func checkPortal(ctx context.Context, portal config.Portal, ps *PortalStatus) { ps.Checked = true client, err := api.ForPortal(portal) if err != nil { diff --git a/internal/claude/skill/SKILL.md b/internal/claude/skill/SKILL.md index 892c6a3..3a45503 100644 --- a/internal/claude/skill/SKILL.md +++ b/internal/claude/skill/SKILL.md @@ -17,13 +17,22 @@ guessing flags. warnings. - List datasets with `cc-data dataset list --json`. - A dataset ref is `/` (e.g. `learn.concord.org/wildfire`); a bare - `` resolves under the configured default portal. + `` resolves under the configured default portal. Dataset refs take a + hostname only: the environment aliases below are **refused** here rather than + expanded, so `staging/wildfire` is an error naming the hostname to use. If a + `get` returns `NOT_AUTHENTICATED`, check the ref's portal is a hostname before + relaying a login. ## Auth - If a command fails with `{"error":"NOT_AUTHENTICATED",...}`, relay to the user: - run `cc-data login --portal `. Never drive the browser login yourself. + run `cc-data login --portal `, or `cc-data login ` for one + of the environments (`prod`, `staging`, `dev`), which sets the portal and its + paired report server together. Never drive the browser login yourself. - Auth is per portal. `cc-data auth status --check` shows validity and metadata. +- The environment names also work wherever a `portal` is passed: the `--portal` + flag on `logout`/`reports list`/`reports jobs`, and the `portal` argument of + the `reports_list` / `reports_jobs` MCP tools. ## Fetching data @@ -118,6 +127,6 @@ provenance, e.g. ## Sensitive data Datasets hold sensitive student data. You may auto-read the -`dataset show --json` summary and manifest; do not dump raw JSONL stores into the +`dataset show --json` summary; do not dump raw JSONL stores into the conversation by default. Suggest `cc-data dataset purge ` when data is no longer needed rather than archiving it to shared drives. diff --git a/internal/config/config.go b/internal/config/config.go index 4fd4b84..856ad6a 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -78,9 +78,41 @@ func Load() (*Config, error) { // returns a raw value that still carries an /api path. c.ServerURL = origin } + if err := c.normalizeDefaultPortal(); err != nil { + // Name the file and the remedy: this refusal fails every command, + // including the ones that never read default_portal, so the error has to + // say both what to edit and that removing the key is a valid fix. + return nil, fmt.Errorf("%s: %w (remove or fix default_portal to continue)", path, err) + } return &c, nil } +// normalizeDefaultPortal parses default_portal and stores it back in normalized +// form, so every consumer sees one value. Left raw, "https://learn.concord.org" +// and "learn.concord.org" name the same portal but two different dataset +// folders, and the URL-shaped one is not even a single path component. +func (c *Config) normalizeDefaultPortal() error { + if c.DefaultPortal == "" { + return nil + } + p, err := ParsePortalIdentity(c.DefaultPortal) + if err != nil { + return fmt.Errorf("default_portal: %w", err) + } + c.DefaultPortal = p.Host() + return nil +} + +// DefaultPortalValue returns the configured default portal, zero when unset. +// Load and Save have already accepted the value, so a parse failure here means +// the Config was built in memory with an unparsed portal. +func (c *Config) DefaultPortalValue() (Portal, error) { + if c.DefaultPortal == "" { + return Portal{}, nil + } + return ParsePortalIdentity(c.DefaultPortal) +} + // Save writes the config atomically at 0600. func (c *Config) Save() error { if c.Version == 0 { @@ -95,6 +127,9 @@ func (c *Config) Save() error { } c.ServerURL = origin } + if err := c.normalizeDefaultPortal(); err != nil { + return err + } dir, err := ConfigDir() if err != nil { return err @@ -149,11 +184,16 @@ func ValidateServerURL(raw string) (string, error) { if u.Host == "" { return "", fmt.Errorf("server URL %q has no host", raw) } - host, _ := splitHostPort(u.Host) - host = strings.ToLower(host) + host := strings.ToLower(splitHostPort(u.Host)) + // Lowercase the returned origin's host too (the port is numeric, so + // lowercasing the whole authority is safe). Otherwise a mixed-case server_url + // would be stored and compared in its original case, spuriously reading as + // "different" from the canonical lowercase origins and rendering unevenly in + // the SERVER column. + origin := u.Scheme + "://" + strings.ToLower(u.Host) if isLoopback(host) { - return u.Scheme + "://" + u.Host, nil + return origin, nil } if u.Scheme != "https" { return "", fmt.Errorf("server URL %q must use https (http is accepted only for loopback)", raw) @@ -161,7 +201,7 @@ func ValidateServerURL(raw string) (string, error) { if !isAllowedServerHost(host) { return "", fmt.Errorf("server host %q is not allowed: must be concord.org, concordqa.org, a subdomain of either, or loopback", host) } - return u.Scheme + "://" + u.Host, nil + return origin, nil } func isLoopback(host string) bool { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 37e9a49..d781dff 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,7 +1,9 @@ package config import ( + "os" "path/filepath" + "strings" "testing" ) @@ -18,12 +20,26 @@ func TestValidateServerURL(t *testing.T) { {"http://localhost:4000", false, "http://localhost:4000"}, {"https://localhost:4000", false, "https://localhost:4000"}, {"http://127.0.0.1:4000", false, "http://127.0.0.1:4000"}, + // A bracketed IPv6 loopback is loopback with or without a port. + {"http://[::1]:4000", false, "http://[::1]:4000"}, + {"http://[::1]", false, "http://[::1]"}, + // Only a matched pair of brackets is unwrapped, so a stray one cannot + // be trimmed into a host that reads as loopback. + {"http://localhost]", true, ""}, {"https://evil-concord.org", true, ""}, {"https://concord.org.evil.com", true, ""}, {"https://notconcord.org", true, ""}, {"http://report-server.concord.org", true, ""}, // http not allowed off loopback {"ftp://report-server.concord.org", true, ""}, {"https://", true, ""}, + // The returned origin's host is lowercased, so a mixed-case server_url + // canonicalizes to the same origin the pairing produces (otherwise the + // login notice reports it as "not used" against the server it names, and + // the SERVER column renders unevenly). Scheme case is normalized by + // url.Parse; a trailing slash and an /api path are already stripped. + {"https://REPORT-SERVER.CONCORD.ORG", false, "https://report-server.concord.org"}, + {"https://Report-Server.Concord.org/api", false, "https://report-server.concord.org"}, + {"HTTP://LOCALHOST:4000", false, "http://localhost:4000"}, } for _, c := range cases { got, err := ValidateServerURL(c.in) @@ -101,6 +117,126 @@ func TestConfigRoundTripAndHomeExpansion(t *testing.T) { } } +// TestSaveRejectsAliasDefaultPortal keeps Save from writing a config Load would +// refuse, which would leave every command failing on a file the CLI wrote +// itself. +func TestSaveRejectsAliasDefaultPortal(t *testing.T) { + home := t.TempDir() + homeDir = func() (string, error) { return home, nil } + t.Cleanup(func() { homeDir = defaultHomeDir }) + + c := &Config{DefaultPortal: "staging"} + err := c.Save() + if err == nil { + t.Fatal("saving an alias default_portal should error") + } + if !strings.Contains(err.Error(), "environment alias") { + t.Fatalf("error should explain the alias: %v", err) + } + path, err := Path() + if err != nil { + t.Fatal(err) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Fatal("a rejected Save should not have written config.json") + } +} + +// TestLoadRejectsAliasDefaultPortal pins the refusal to treat an environment +// alias as a portal host. default_portal is also the literal portal of a bare +// dataset ref and the on-disk folder identity, so accepting "staging" here would +// store a credential under a host by that name and build the auth URL +// "https://staging". +func TestLoadRejectsAliasDefaultPortal(t *testing.T) { + home := t.TempDir() + homeDir = func() (string, error) { return home, nil } + t.Cleanup(func() { homeDir = defaultHomeDir }) + + dir := filepath.Join(home, ".config", "cc-data") + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + for _, alias := range []string{"staging", "prod", "dev", "STAGING"} { + body := []byte(`{"version":1,"default_portal":"` + alias + `"}`) + if err := os.WriteFile(filepath.Join(dir, "config.json"), body, 0o600); err != nil { + t.Fatal(err) + } + _, err := Load() + if err == nil { + t.Errorf("default_portal %q should be rejected as an alias", alias) + continue + } + if !strings.Contains(err.Error(), "environment alias") { + t.Errorf("default_portal %q error should explain the alias: %v", alias, err) + } + // This refusal fails every command, so the error has to name the file + // the user has to edit to recover. + path, perr := Path() + if perr != nil { + t.Fatal(perr) + } + if !strings.Contains(err.Error(), path) { + t.Errorf("default_portal %q error should name %s: %v", alias, path, err) + } + } + + // A full hostname is still accepted. + body := []byte(`{"version":1,"default_portal":"` + StagingPortal + `"}`) + if err := os.WriteFile(filepath.Join(dir, "config.json"), body, 0o600); err != nil { + t.Fatal(err) + } + c, err := Load() + if err != nil { + t.Fatalf("a full hostname default_portal should load: %v", err) + } + if c.DefaultPortal != StagingPortal { + t.Fatalf("default_portal = %q", c.DefaultPortal) + } +} + +// TestLoadNormalizesDefaultPortal pins that the value is canonicalized once, at +// load. Left raw, "https://learn.concord.org" and "learn.concord.org" name the +// same portal but two different dataset folders, and the auto-named +// "dataset create" path filed data under the URL-shaped one, where dataset list +// could not see it. +func TestLoadNormalizesDefaultPortal(t *testing.T) { + home := t.TempDir() + homeDir = func() (string, error) { return home, nil } + t.Cleanup(func() { homeDir = defaultHomeDir }) + + dir := filepath.Join(home, ".config", "cc-data") + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + for _, raw := range []string{"https://learn.concord.org", "https://learn.concord.org/", "Learn.Concord.Org"} { + body := []byte(`{"version":1,"default_portal":"` + raw + `"}`) + if err := os.WriteFile(filepath.Join(dir, "config.json"), body, 0o600); err != nil { + t.Fatal(err) + } + c, err := Load() + if err != nil { + t.Errorf("default_portal %q should load: %v", raw, err) + continue + } + if c.DefaultPortal != ProductionPortal { + t.Errorf("default_portal %q loaded as %q, want %q", raw, c.DefaultPortal, ProductionPortal) + } + p, err := c.DefaultPortalValue() + if err != nil || p.Host() != ProductionPortal { + t.Errorf("DefaultPortalValue for %q = (%q, %v)", raw, p.Host(), err) + } + } + + // A value that cannot be a folder is refused outright, naming the file. + body := []byte(`{"version":1,"default_portal":"../../escaped"}`) + if err := os.WriteFile(filepath.Join(dir, "config.json"), body, 0o600); err != nil { + t.Fatal(err) + } + if _, err := Load(); err == nil { + t.Error("a traversal default_portal should be rejected") + } +} + func TestLoadAbsentReturnsDefault(t *testing.T) { home := t.TempDir() homeDir = func() (string, error) { return home, nil } diff --git a/internal/config/portal.go b/internal/config/portal.go index 6d96609..b23cdf7 100644 --- a/internal/config/portal.go +++ b/internal/config/portal.go @@ -4,22 +4,173 @@ import ( "fmt" "net" "net/url" + "regexp" "strings" ) -// ProductionPortal is the one portal whose firebase source is report-service-pro; -// every other portal maps to report-service-dev. +// ProductionPortal is the portal a login targets when nothing names another. const ProductionPortal = "learn.concord.org" +// The portal hosts and report-server origins behind the environment aliases. +// A portal is stored in normalized (scheme-less) host form; Portal.Origin adds +// the scheme, so the loopback dev portal needs no scheme here. const ( - firebaseProd = "report-service-pro" - firebaseDev = "report-service-dev" + StagingPortal = "learn.portal.staging.concord.org" + DevPortal = "localhost:3000" + + StagingServer = "https://report-server.concordqa.org" + DevServer = "http://localhost:4000" ) -// NormalizePortal reduces a portal value to hostname form: scheme stripped, -// lowercased, trailing slash removed, port preserved for dev portals. -func NormalizePortal(portal string) (string, error) { - p := strings.TrimSpace(portal) +// Portal is a portal host that has been through one of this package's parsers. +// No other package can build one from a bare string, so a portal value cannot +// reach a credential key, a dataset folder, or an auth URL without having been +// parsed exactly once. Identity is the host alone, so Portals compare with == +// and work as map keys. +type Portal struct{ host string } + +// Host is the normalized portal host: lowercased, scheme-less, port preserved. +func (p Portal) Host() string { return p.host } + +func (p Portal) String() string { return p.host } + +// IsZero reports the absence of a portal, which is how an unset default_portal +// travels. +func (p Portal) IsZero() bool { return p.host == "" } + +// Origin re-expands the host to an origin, the form the server's /auth/cli +// portal query param requires. A loopback dev portal gets http, the same +// exception ValidateServerURL makes for loopback server origins; every other +// portal is https. Deriving the scheme here rather than carrying it in the host +// keeps the credential key, the manifest portal, and Folder on a bare host. +func (p Portal) Origin() string { + if isLoopbackHostPort(p.host) { + return "http://" + p.host + } + return "https://" + p.host +} + +// Folder encodes the host into one filesystem-safe path component, so a dev +// portal's port (localhost:8080) does not carry an illegal ':' into a path. +// Applied on every platform for portability; the real host is kept in the +// credentials and manifest. That this is a single component, never a traversal, +// is guaranteed upstream by checkHostSyntax rather than by escaping here. +func (p Portal) Folder() string { return strings.ReplaceAll(p.host, ":", "_") } + +// ParsePortalTarget parses a portal the CLI is about to talk to: an environment +// alias, a hostname, or a URL. Aliases expand, and the result must look like a +// portal rather than a near-miss alias. It also returns the origin the auth flow +// should send for it, which differs from Origin only for a loopback portal named +// with an explicit https://: a local portal may be served over either scheme, so +// "https://localhost:3001" is honored rather than downgraded, while a scheme on +// any other portal is ignored so "http://learn.concord.org" cannot downgrade a +// real portal login. +// +// A single-label non-loopback host is refused with *UnshapedHostError, which +// carries the parsed portal so a caller that can vouch for it (auth, holding a +// credential stored under that host) can accept it anyway. +// +// The alias lookup happens on the parsed host rather than the raw input, so +// every spelling that normalizes to an alias expands: checking the raw string +// first would let "https://staging" slip past as a literal host. +func ParsePortalTarget(v string) (Portal, string, error) { + p, err := parseHost(v) + if err != nil { + return Portal{}, "", err + } + if env, ok := LookupEnvironment(p.host); ok { + p = env.Portal + } + if err := checkPortalShape(p); err != nil { + return Portal{}, "", err + } + if isLoopbackHostPort(p.host) && hasHTTPSScheme(v) { + return p, "https://" + p.host, nil + } + return p, p.Origin(), nil +} + +// ParsePortalIdentity parses a portal that names something on disk: a dataset +// ref's portal, or default_portal. An alias is refused rather than expanded. +// Expanding it would give one dataset folder two names, and leaving it literal +// would file data under a portal that can never hold a credential, which reads +// downstream as a NOT_AUTHENTICATED that logging in does not fix. +// +// The single-label shape check that ParsePortalTarget applies is deliberately +// not applied here: a portal on disk is an identity that must stay whatever it +// was written as, and an older build stored credentials under single-label +// hosts. +// +// The alias lookup happens on the parsed host rather than the raw input, so a +// scheme cannot smuggle one through: "https://staging" normalizes to "staging" +// and is refused exactly as the bare spelling is. The refusal is +// *AliasPortalError, which carries the literal portal, so a caller holding a +// folder that already exists under it can still reach it. +func ParsePortalIdentity(v string) (Portal, error) { + p, err := parseHost(v) + if err != nil { + return Portal{}, err + } + if env, ok := LookupEnvironment(p.host); ok { + return Portal{}, &AliasPortalError{Portal: p, Env: env} + } + return p, nil +} + +// AliasPortalError is an identity that normalizes to an environment alias name. +// It is a policy refusal, not a safety one: the portal it carries is a perfectly +// good path component, it just names an environment rather than a host, so a +// caller that can vouch for it (an existing dataset folder) may accept it. +type AliasPortalError struct { + Portal Portal + Env Environment +} + +func (e *AliasPortalError) Error() string { + return fmt.Sprintf("portal %q is an environment alias; use the full hostname %q instead", + e.Portal.Host(), e.Env.Portal.Host()) +} + +// AdoptStoredPortal takes a portal host cc-data itself wrote (a credential key, +// a dataset folder) back as a Portal without re-parsing it. This is the one +// deliberate way around the parsers: the value is already an on-disk identity, +// and refusing it now would strand the data and credentials it names. +func AdoptStoredPortal(host string) Portal { return Portal{host: host} } + +// MustPortal parses a portal host that is already in normalized form, panicking +// if it is not. It is a parser, not a way around one: an unsafe value cannot +// survive it. Use it for constants and test fixtures; use ParsePortalIdentity +// for anything a user typed. +func MustPortal(host string) Portal { + p, err := ParsePortalIdentity(host) + if err != nil { + panic(fmt.Sprintf("portal %q does not parse: %v", host, err)) + } + if p.host != host { + panic(fmt.Sprintf("portal %q is not in normalized form (got %q)", host, p.host)) + } + return p +} + +// parseHost reduces a portal value to hostname form (scheme stripped, +// lowercased, trailing slash removed, port preserved) and enforces the host +// shape that keeps a portal usable as a single filesystem path component. +func parseHost(v string) (Portal, error) { + host, err := normalizeHost(v) + if err != nil { + return Portal{}, err + } + if err := checkHostSyntax(host); err != nil { + return Portal{}, fmt.Errorf("invalid portal %q: %w", v, err) + } + return Portal{host: host}, nil +} + +// normalizeHost is parseHost without the shape check: scheme stripped, +// lowercased, port preserved. It is the reduction an older, laxer build applied, +// so AdoptExistingPortal can reproduce the folder name that build wrote. +func normalizeHost(v string) (string, error) { + p := strings.TrimSpace(v) if p == "" { return "", fmt.Errorf("portal is empty") } @@ -28,42 +179,246 @@ func NormalizePortal(portal string) (string, error) { } u, err := url.Parse(p) if err != nil { - return "", fmt.Errorf("invalid portal %q: %w", portal, err) + return "", fmt.Errorf("invalid portal %q: %w", v, err) } host := strings.ToLower(u.Host) if host == "" { - return "", fmt.Errorf("invalid portal %q: no host", portal) + return "", fmt.Errorf("invalid portal %q: no host", v) } return host, nil } -// PortalOrigin re-expands a normalized hostname to an https origin, the form the -// server's /auth/cli portal query param requires. -func PortalOrigin(host string) string { - return "https://" + host +// AdoptExistingPortal returns the portal a value names for the purpose of +// reaching a dataset already on disk, for a value ParsePortalIdentity refused. +// It accepts what that parser rejects for being an alias or for a host shape an +// earlier, laxer build allowed (an underscore, an IPv6 literal), since folders +// under such portals can exist today. It never accepts a value that is not a +// single safe path component, so the traversal guard is preserved; ok is false +// then, and for anything that cannot be a host at all. +func AdoptExistingPortal(v string) (Portal, bool) { + host, err := normalizeHost(v) + if err != nil { + return Portal{}, false + } + p := Portal{host: host} + folder := p.Folder() + if folder == "" || folder == "." || folder == ".." || strings.ContainsAny(folder, `/\`) { + return Portal{}, false + } + return p, true } -// PortalFolder encodes a portal host into a filesystem-safe folder name, so a -// dev portal's port (localhost:8080) does not carry an illegal ':' into a path -// component. Applied on every platform for portability; the real host is kept in -// the credentials and manifest. -func PortalFolder(host string) string { - return strings.ReplaceAll(host, ":", "_") +// hostSyntax is a permissive host-shape check, not a strict hostname grammar: it +// requires the host to start and end alphanumeric, so "." and ".." and anything +// leading with a separator are out, but it tolerates odd interior runs like +// "a..b" (harmless: still one reversible path component, which is the property +// that matters). Underscore is excluded on purpose: Portal.Folder encodes the +// port separator as "_", so a host containing one would decode to a different +// host. +var hostSyntax = regexp.MustCompile(`^[a-z0-9]([a-z0-9.-]*[a-z0-9])?$`) + +// checkHostSyntax rejects anything not shaped like a host. This is what makes a +// portal safe to use as a path component: url.Parse happily reports "." and ".." +// as the host of "https://../../escaped", and Folder passes them through, so a +// dataset ref like "../wildfire" would otherwise resolve outside the data root +// and reach MkdirAll and os.RemoveAll. Checking here covers every caller at +// once, which is the point of there being one parser. +func checkHostSyntax(host string) error { + h := host + if hostOnly, port, err := net.SplitHostPort(host); err == nil { + // A trailing colon parses as an empty port. Left in, it would key a + // credential and name a folder distinct from the same host without it, + // and reach the auth URL as the malformed origin "http://localhost:". + if port == "" { + return fmt.Errorf("%q has an empty port", host) + } + h = hostOnly + } else { + h = unbracket(host) + } + if h == "" { + return fmt.Errorf("no host") + } + if strings.HasPrefix(host, "[") { + // A bracketed IPv6 literal survives Portal.Folder only as "[__1]", which + // decodes back to something else again. Loopback development is served by + // localhost and 127.0.0.1, so the literal buys nothing worth an + // unrepresentable folder name. Server origins are unaffected: they are + // URLs, never paths, and ValidateServerURL still accepts "http://[::1]". + return fmt.Errorf("%q is an IPv6 literal; use localhost or 127.0.0.1", host) + } + if !hostSyntax.MatchString(h) { + return fmt.Errorf("%q is not a hostname", host) + } + return nil +} + +// UnshapedHostError is a portal that parses as a host but is a single label that +// is not loopback. Once the environment aliases exist, "stagng" is a misspelled +// alias far more often than a portal, and nothing downstream would catch it: the +// value would key a credential, name a dataset folder, and reach the auth URL as +// "https://stagng". The parsed portal travels on the error so auth can still +// accept one that a stored credential vouches for. +type UnshapedHostError struct{ Portal Portal } + +func (e *UnshapedHostError) Error() string { + return fmt.Sprintf("unknown environment %q: expected one of %s (use a full hostname for a portal)", + e.Portal.Host(), strings.Join(environmentNames, ", ")) +} + +// checkPortalShape passes a dotted name and loopback, so only the spellings that +// cannot be a real portal (a single non-loopback label) are turned away. It runs +// after checkHostSyntax, which has already rejected IPv6 literals, so a colon can +// no longer survive in the host here. +func checkPortalShape(p Portal) error { + h := splitHostPort(p.host) + if strings.Contains(h, ".") || isLoopback(h) { + return nil + } + return &UnshapedHostError{Portal: p} +} + +// Environment is one environment alias: a portal and the report server it is +// operationally paired with. Naming the pair in one place is what keeps a +// staging portal from being pointed at the production server by accident. +type Environment struct { + Portal Portal + Server string +} + +// The three environments. Each is spelled several ways below; the pair itself +// is defined once here so the spellings cannot drift apart. +var ( + // mustHost runs as this package initializes, so a table entry that is + // misspelled or not already in normalized form fails the first time any + // command or test binary runs rather than at login. + envProduction = Environment{Portal: mustHost(ProductionPortal), Server: DefaultServerURL} + envStaging = Environment{Portal: mustHost(StagingPortal), Server: StagingServer} + envDevelopment = Environment{Portal: mustHost(DevPortal), Server: DevServer} +) + +// mustHost is MustPortal without the alias check, which the table cannot use +// because the alias check reads the table. +func mustHost(host string) Portal { + p, err := parseHost(host) + if err != nil { + panic(fmt.Sprintf("environment portal %q does not parse: %v", host, err)) + } + if p.host != host { + panic(fmt.Sprintf("environment portal %q is not in normalized form (got %q)", host, p.host)) + } + return p +} + +// environments maps every accepted spelling to its portal/server pair. Names +// are matched case-insensitively. Both the short and the long form of each +// environment are accepted so nobody has to remember which one this CLI picked. +var environments = map[string]Environment{ + "prod": envProduction, + "production": envProduction, + "stage": envStaging, + "staging": envStaging, + "dev": envDevelopment, + "development": envDevelopment, } -// FirebaseSource maps a portal host to its firebase project. -func FirebaseSource(host string) string { - if host == ProductionPortal { - return firebaseProd +// environmentNames lists the canonical (short) alias names in presentation +// order, for help text and error messages; map iteration order would shuffle +// them, and listing every synonym would bury the three real choices. +var environmentNames = []string{"prod", "staging", "dev"} + +// EnvironmentNames returns the canonical alias names in a stable order. The +// longer spellings are accepted by LookupEnvironment but deliberately left out +// of help text. +func EnvironmentNames() []string { + return append([]string(nil), environmentNames...) +} + +// LookupEnvironment resolves an alias name to its portal/server pair. Any value +// that is not an alias (a full host or URL) reports false. +func LookupEnvironment(name string) (Environment, bool) { + env, ok := environments[strings.ToLower(strings.TrimSpace(name))] + return env, ok +} + +// environmentsByPortal indexes the same pairs by portal, so a portal spelled out +// in full can find the server it belongs with. It is derived from environments +// rather than restated, so a new environment cannot end up with aliases that +// resolve but a hostname that pairs with nothing. A collision panics as this +// package initializes, which is the first thing any command or test binary does, +// rather than letting a login pick a different server on different runs. +var environmentsByPortal = func() map[Portal]Environment { + m, err := buildEnvironmentsByPortal(environments) + if err != nil { + panic(err) + } + return m +}() + +// buildEnvironmentsByPortal inverts an alias table onto its portals. Two +// environments sharing a portal but not a server would make the winner depend on +// map iteration order, so that is an error rather than a silent pick. It takes +// the table as an argument so the collision can be exercised directly; the +// package-level index cannot be, since a colliding table would panic before any +// test runs. +func buildEnvironmentsByPortal(envs map[string]Environment) (map[Portal]Environment, error) { + m := make(map[Portal]Environment, len(envs)) + for _, env := range envs { + if prior, ok := m[env.Portal]; ok && prior != env { + return nil, fmt.Errorf("portal %q is claimed by two environments with different servers (%q and %q)", + env.Portal.Host(), prior.Server, env.Server) + } + m[env.Portal] = env + } + return m, nil +} + +// EnvironmentForPortal finds the environment a portal belongs to. This is the +// reverse of LookupEnvironment: it keys on the portal rather than the alias +// spelling, so "--portal learn.portal.staging.concord.org" can be paired with the +// staging report server exactly as "--portal staging" is. +func EnvironmentForPortal(p Portal) (Environment, bool) { + env, ok := environmentsByPortal[p] + return env, ok +} + +// ExpandServerAlias expands an environment alias to its report-server origin, +// returning any other value unchanged. Portal and server aliases expand +// independently, which is what makes a mixed "--portal staging --server dev" +// meaningful. The result still goes through +// ValidateServerURL, so an alias is not a way around the origin allowlist. +func ExpandServerAlias(v string) string { + if env, ok := LookupEnvironment(v); ok { + return env.Server } - return firebaseDev + return v +} + +func hasHTTPSScheme(v string) bool { + return strings.HasPrefix(strings.ToLower(strings.TrimSpace(v)), "https://") } -// splitHostPort returns the host without its port, and whether a port was present. -func splitHostPort(host string) (string, bool) { +// isLoopbackHostPort reports whether a host that may carry a port is loopback. +func isLoopbackHostPort(host string) bool { + return isLoopback(strings.ToLower(splitHostPort(host))) +} + +// splitHostPort returns the host without its port. A bracketed IPv6 literal +// loses its brackets either way, so "[::1]" and "[::1]:3000" both reduce to the +// "::1" the loopback check compares against. Only a properly bracketed literal +// is unwrapped: a stray bracket leaves the host as it was found rather than +// trimming it into something that looks loopback to the caller. +func splitHostPort(host string) string { h, _, err := net.SplitHostPort(host) if err != nil { - return host, false + return unbracket(host) + } + return h +} + +func unbracket(host string) string { + if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") { + return host[1 : len(host)-1] } - return h, true + return host } diff --git a/internal/config/portal_test.go b/internal/config/portal_test.go index 4a23512..bcb5bfb 100644 --- a/internal/config/portal_test.go +++ b/internal/config/portal_test.go @@ -1,60 +1,424 @@ package config -import "testing" +import ( + "errors" + "path/filepath" + "strings" + "testing" +) -func TestNormalizePortal(t *testing.T) { +// The portal matrix. Every shape a portal value can arrive in, crossed with the +// two parsers, one row per shape. This table is the point: the bugs this package +// has shipped were all a call site reaching a parser that handled its shape +// differently than the author assumed, and a matrix is the only form in which +// "what does X do to Y" is answerable by reading rather than by searching. +// +// target is what ParsePortalTarget returns: a portal you are about to talk to, +// so aliases expand and a near-miss alias is refused. identity is what +// ParsePortalIdentity returns: a portal that names something on disk, so aliases +// are refused and single-label hosts survive. "" in either column means the +// parser must reject the value. +var portalMatrix = []struct { + name string + in string + + target string // expected host, "" to expect an error + targetOrigin string // expected auth-flow origin, checked when target is set + identity string // expected host, "" to expect an error +}{ + // Aliases: a target expands them, an identity refuses them. An identity is + // also a folder name, so expanding would give one dataset two names and + // leaving it literal would file data under a portal that can never hold a + // credential. + {"short alias", "staging", StagingPortal, "https://" + StagingPortal, ""}, + {"long alias", "production", ProductionPortal, "https://" + ProductionPortal, ""}, + {"alias, mixed case", "STAGING", StagingPortal, "https://" + StagingPortal, ""}, + {"alias, padded", " dev ", DevPortal, "http://" + DevPortal, ""}, + {"loopback alias", "dev", DevPortal, "http://" + DevPortal, ""}, + // A scheme must not smuggle an alias past either parser: the lookup runs on + // the normalized host, not the raw string. + {"alias with a scheme", "https://staging", StagingPortal, "https://" + StagingPortal, ""}, + {"alias with an http scheme", "http://prod", ProductionPortal, "https://" + ProductionPortal, ""}, + {"alias with scheme, mixed case", "HTTPS://STAGING", StagingPortal, "https://" + StagingPortal, ""}, + {"alias with a scheme and a slash", "https://staging/", StagingPortal, "https://" + StagingPortal, ""}, + // An explicit https on the loopback alias is still honored. + {"loopback alias with https", "https://dev", DevPortal, "https://" + DevPortal, ""}, + + // Hostnames and URLs normalize identically on both sides. + {"hostname", ProductionPortal, ProductionPortal, "https://" + ProductionPortal, ProductionPortal}, + {"hostname, mixed case", "Learn.Concord.Org", ProductionPortal, "https://" + ProductionPortal, ProductionPortal}, + {"URL", "https://learn.concord.org", ProductionPortal, "https://" + ProductionPortal, ProductionPortal}, + {"URL with trailing slash", "https://learn.concord.org/", ProductionPortal, "https://" + ProductionPortal, ProductionPortal}, + {"URL with path", "https://learn.concord.org/foo", ProductionPortal, "https://" + ProductionPortal, ProductionPortal}, + {"unknown but well-formed host", "learn.example.concord.org", "learn.example.concord.org", "https://learn.example.concord.org", "learn.example.concord.org"}, + + // http on a real portal is ignored rather than honored, so naming + // "http://learn.concord.org" cannot downgrade a real login. + {"http on a real portal", "http://learn.concord.org", ProductionPortal, "https://" + ProductionPortal, ProductionPortal}, + // A loopback portal may legitimately be served either way, so an explicit + // https survives to the auth URL. + {"loopback with port", "localhost:3000", DevPortal, "http://" + DevPortal, DevPortal}, + {"explicit https loopback", "https://localhost:3001", "localhost:3001", "https://localhost:3001", "localhost:3001"}, + {"explicit https loopback, mixed case scheme", "HTTPS://127.0.0.1:3001", "127.0.0.1:3001", "https://127.0.0.1:3001", "127.0.0.1:3001"}, + // An IPv6 literal and an underscore host are both refused because + // Portal.Folder could not encode them reversibly; localhost and 127.0.0.1 + // cover every loopback case that matters. + {"IPv6 loopback", "[::1]:3000", "", "", ""}, + {"IPv6 loopback, no port", "[::1]", "", "", ""}, + {"underscore in a host", "host_3000", "", "", ""}, + {"underscore in a label", "a_b.concord.org", "", "", ""}, + {"non-loopback IP", "192.168.1.10:3000", "192.168.1.10:3000", "https://192.168.1.10:3000", "192.168.1.10:3000"}, + + // A single label that is not loopback is a misspelled alias far more often + // than a portal, so a target refuses it. An identity keeps it: an older + // build stored credentials under such hosts, and the folders they name must + // stay reachable. + {"near-miss alias", "stagng", "", "", "stagng"}, + {"legacy single-label host", "myportal", "", "", "myportal"}, + {"single label, loopback", "localhost", "localhost", "http://localhost", "localhost"}, + + // Path traversal. url.Parse reports ".." as the host of "https://../x", and + // Folder passes a host straight into a path component, so both parsers have + // to refuse these or a dataset ref lands outside the data root. + {"dot-dot", "..", "", "", ""}, + {"dot", ".", "", "", ""}, + {"traversal", "../../escaped", "", "", ""}, + {"traversal via URL", "https://../x", "", "", ""}, + {"leading dot", ".hidden.org", "", "", ""}, + // url.Parse keeps only the host, which then falls to the single-label rule. + {"slash-bearing", "a/b", "", "", "a"}, + + // A trailing colon parses as an empty port, which would key a credential and + // name a folder distinct from the same host without it. + {"empty port", "localhost:", "", "", ""}, + {"empty port on a hostname", "learn.concord.org:", "", "", ""}, + + // Nothing at all. + {"empty", "", "", "", ""}, + {"whitespace", " ", "", "", ""}, +} + +func TestPortalMatrix(t *testing.T) { + for _, c := range portalMatrix { + t.Run(c.name, func(t *testing.T) { + portal, origin, err := ParsePortalTarget(c.in) + switch { + case c.target == "" && err == nil: + t.Errorf("ParsePortalTarget(%q) = %q, want an error", c.in, portal.Host()) + case c.target != "" && err != nil: + t.Errorf("ParsePortalTarget(%q) error: %v", c.in, err) + case c.target != "": + if portal.Host() != c.target { + t.Errorf("ParsePortalTarget(%q) = %q, want %q", c.in, portal.Host(), c.target) + } + if origin != c.targetOrigin { + t.Errorf("ParsePortalTarget(%q) origin = %q, want %q", c.in, origin, c.targetOrigin) + } + } + + identity, err := ParsePortalIdentity(c.in) + switch { + case c.identity == "" && err == nil: + t.Errorf("ParsePortalIdentity(%q) = %q, want an error", c.in, identity.Host()) + case c.identity != "" && err != nil: + t.Errorf("ParsePortalIdentity(%q) error: %v", c.in, err) + case c.identity != "": + if identity.Host() != c.identity { + t.Errorf("ParsePortalIdentity(%q) = %q, want %q", c.in, identity.Host(), c.identity) + } + } + }) + } +} + +// TestPortalFolderStaysOneComponent is the property the matrix's traversal rows +// exist to protect: whatever a parser accepts must be a single path component, +// because Ref.Dir joins it straight into the data root and dataset delete calls +// os.RemoveAll on the result. +func TestPortalFolderStaysOneComponent(t *testing.T) { + root := filepath.Join("/data", "root") + check := func(t *testing.T, in string, p Portal) { + t.Helper() + folder := p.Folder() + if strings.ContainsAny(folder, `/\`) || folder == "." || folder == ".." { + t.Errorf("portal from %q has folder %q, not a single path component", in, folder) + } + joined := filepath.Join(root, folder, "datasets", "x") + if !strings.HasPrefix(filepath.Clean(joined), filepath.Clean(root)+string(filepath.Separator)) { + t.Errorf("portal from %q resolves outside the data root: %q", in, joined) + } + } + // Whatever the parsers actually return, not what the table says they should: + // asserting on the expected strings would hold even if a parser returned + // something else entirely. + for _, c := range portalMatrix { + t.Run(c.name, func(t *testing.T) { + if p, _, err := ParsePortalTarget(c.in); err == nil { + check(t, c.in, p) + } + if p, err := ParsePortalIdentity(c.in); err == nil { + check(t, c.in, p) + } + }) + } +} + +// TestAdoptExistingPortal pins the salvage helper the existing-dataset parsers +// use: it accepts what ParsePortalIdentity refuses for being an alias or a host +// shape an earlier build allowed, but never a value that is not a single safe +// path component, so the traversal guard is preserved. +func TestAdoptExistingPortal(t *testing.T) { + accepted := map[string]string{ + "staging": "staging", // an environment alias name + "a_b.concord.org": "a_b.concord.org", // an underscore host + "HOST_3000": "host_3000", // lowercased + "https://a_b.local": "a_b.local", // scheme stripped + "[::1]:3000": "[::1]:3000", // an IPv6 literal, still one folder + "learn.concord.org": "learn.concord.org", // an ordinary host + } + for in, wantHost := range accepted { + p, ok := AdoptExistingPortal(in) + if !ok { + t.Errorf("AdoptExistingPortal(%q) should be accepted", in) + continue + } + if p.Host() != wantHost { + t.Errorf("AdoptExistingPortal(%q) host = %q, want %q", in, p.Host(), wantHost) + } + // Whatever it accepts must be a single path component. + if f := p.Folder(); f == "" || f == "." || f == ".." || strings.ContainsAny(f, `/\`) { + t.Errorf("AdoptExistingPortal(%q) folder %q is not one safe component", in, f) + } + } + for _, in := range []string{"", " ", "..", ".", "../escaped", "https://../x"} { + if _, ok := AdoptExistingPortal(in); ok { + t.Errorf("AdoptExistingPortal(%q) should be refused", in) + } + } +} + +// TestUnshapedHostErrorCarriesThePortal pins the one escape hatch: a shape +// refusal hands back the parsed portal so auth can accept a host that a stored +// credential vouches for, and it names the environments, since a typo'd alias is +// what lands here. +func TestUnshapedHostErrorCarriesThePortal(t *testing.T) { + _, _, err := ParsePortalTarget("stagng") + var unshaped *UnshapedHostError + if !errors.As(err, &unshaped) { + t.Fatalf("want *UnshapedHostError, got %T: %v", err, err) + } + if unshaped.Portal.Host() != "stagng" { + t.Errorf("error carries portal %q, want %q", unshaped.Portal.Host(), "stagng") + } + if !strings.Contains(err.Error(), "expected one of") { + t.Errorf("error should list the environments: %v", err) + } +} + +// TestIdentityRejectionNamesTheHostname keeps the alias refusal actionable: the +// message has to carry the hostname to use, since the whole failure mode is a +// researcher typing the environment name they read in the guide. +func TestIdentityRejectionNamesTheHostname(t *testing.T) { + for _, in := range []string{"staging", "https://staging", "STAGING"} { + _, err := ParsePortalIdentity(in) + if err == nil { + t.Fatalf("%q should be refused as an identity", in) + } + if !strings.Contains(err.Error(), StagingPortal) { + t.Errorf("%q error should name the hostname to use: %v", in, err) + } + // The refusal is typed and carries the literal portal, which is what + // lets an existing dataset folder under it stay reachable. + var alias *AliasPortalError + if !errors.As(err, &alias) { + t.Fatalf("%q: want *AliasPortalError, got %T", in, err) + } + if alias.Portal.Host() != "staging" || alias.Env.Portal.Host() != StagingPortal { + t.Errorf("%q carries portal %q / env %q", in, alias.Portal.Host(), alias.Env.Portal.Host()) + } + } +} + +func TestPortalOriginAndFolder(t *testing.T) { + cases := []struct{ host, origin, folder string }{ + {ProductionPortal, "https://" + ProductionPortal, ProductionPortal}, + {"localhost:3000", "http://localhost:3000", "localhost_3000"}, + // Adopted only: the parsers refuse an IPv6 literal, but a host stored by + // an older build still has to render. + {"[::1]:3000", "http://[::1]:3000", "[__1]_3000"}, + {"portal.example.org:8443", "https://portal.example.org:8443", "portal.example.org_8443"}, + } + for _, c := range cases { + p := AdoptStoredPortal(c.host) + if got := p.Origin(); got != c.origin { + t.Errorf("Portal(%q).Origin() = %q, want %q", c.host, got, c.origin) + } + if got := p.Folder(); got != c.folder { + t.Errorf("Portal(%q).Folder() = %q, want %q", c.host, got, c.folder) + } + } +} + +func TestMustPortalRejectsUnnormalized(t *testing.T) { + defer func() { + if recover() == nil { + t.Fatal("MustPortal should panic on a value that is not already normalized") + } + }() + MustPortal("https://learn.concord.org/") +} + +func TestLookupEnvironment(t *testing.T) { cases := []struct { - in string - want string + name string + wantPortal string + wantServer string }{ - {"https://learn.concord.org/", "learn.concord.org"}, - {"https://learn.concord.org", "learn.concord.org"}, - {"learn.concord.org", "learn.concord.org"}, - {"HTTPS://Learn.Concord.ORG/", "learn.concord.org"}, - {"http://localhost:8080", "localhost:8080"}, - {"localhost:8080", "localhost:8080"}, - {"learn.portal.staging.concord.org", "learn.portal.staging.concord.org"}, + // Both the short and the long spelling of each environment resolve to + // the same pair; neither form is privileged. + {"prod", ProductionPortal, DefaultServerURL}, + {"production", ProductionPortal, DefaultServerURL}, + {"stage", StagingPortal, StagingServer}, + {"staging", StagingPortal, StagingServer}, + {"dev", DevPortal, DevServer}, + {"development", DevPortal, DevServer}, + {" Staging ", StagingPortal, StagingServer}, } for _, c := range cases { - got, err := NormalizePortal(c.in) - if err != nil { - t.Fatalf("NormalizePortal(%q) error: %v", c.in, err) + env, ok := LookupEnvironment(c.name) + if !ok { + t.Fatalf("LookupEnvironment(%q) not found", c.name) + } + if env.Portal.Host() != c.wantPortal || env.Server != c.wantServer { + t.Errorf("LookupEnvironment(%q) = %+v, want portal %q server %q", c.name, env, c.wantPortal, c.wantServer) + } + } + + for _, name := range []string{"", "learn.concord.org", "stagin"} { + if _, ok := LookupEnvironment(name); ok { + t.Errorf("LookupEnvironment(%q) should not match an alias", name) + } + } +} + +func TestEnvironmentNamesAreResolvable(t *testing.T) { + for _, name := range EnvironmentNames() { + if _, ok := LookupEnvironment(name); !ok { + t.Errorf("EnvironmentNames listed %q but LookupEnvironment does not know it", name) + } + } +} + +// TestEveryEnvironmentIsNamed walks the alias table rather than the curated +// name list, so an environment added to environments but never given a +// canonical name fails here instead of shipping invisible: nothing would print +// it in help or error text, and the only way to reach it would be to already +// know the spelling. +func TestEveryEnvironmentIsNamed(t *testing.T) { + named := map[Environment]bool{} + for _, name := range EnvironmentNames() { + env, ok := LookupEnvironment(name) + if !ok { + continue // TestEnvironmentNamesAreResolvable reports this. + } + named[env] = true + } + for name, env := range environments { + if !named[env] { + t.Errorf("environment %+v (spelled %q) is in no EnvironmentNames entry", env, name) } - if got != c.want { - t.Fatalf("NormalizePortal(%q) = %q, want %q", c.in, got, c.want) + if _, ok := environmentsByPortal[env.Portal]; !ok { + t.Errorf("environment %+v (spelled %q) has no entry in environmentsByPortal", env, name) } } } -func TestNormalizePortalEmpty(t *testing.T) { - if _, err := NormalizePortal(" "); err == nil { - t.Fatal("empty portal should error") +func TestEnvironmentPairsAreValid(t *testing.T) { + // Every alias must survive the same validation the flags apply, so a typo in + // the table fails here rather than at login time. The portal side is checked + // as the package initializes (mustHost), so this covers the server side. + for name, env := range environments { + origin, err := ValidateServerURL(env.Server) + if err != nil { + t.Errorf("environment %q server %q is not an allowed origin: %v", name, env.Server, err) + } + if origin != env.Server { + t.Errorf("environment %q server %q is not canonical (got %q)", name, env.Server, origin) + } } } -func TestPortalFolderEncoding(t *testing.T) { - if got := PortalFolder("localhost:8080"); got != "localhost_8080" { - t.Fatalf("PortalFolder(localhost:8080) = %q, want localhost_8080", got) +// TestBuildEnvironmentsByPortalRejectsCollisions pins that no two environments +// may claim the same portal with different servers. The index is built by +// ranging over a map, so such a pair would make the winner depend on iteration +// order and send the same "--portal " login to different servers on +// different runs. The real table is checked as this package initializes, which +// panics rather than failing a test, so the collision itself is exercised here +// against a table built for it. +func TestBuildEnvironmentsByPortalRejectsCollisions(t *testing.T) { + if _, err := buildEnvironmentsByPortal(environments); err != nil { + t.Fatalf("the shipped environments should build: %v", err) + } + + staging := MustPortal(StagingPortal) + colliding := map[string]Environment{ + "one": {Portal: staging, Server: StagingServer}, + "two": {Portal: staging, Server: DefaultServerURL}, } - if got := PortalFolder("learn.concord.org"); got != "learn.concord.org" { - t.Fatalf("PortalFolder(learn.concord.org) = %q, want unchanged", got) + _, err := buildEnvironmentsByPortal(colliding) + if err == nil { + t.Fatal("two environments claiming one portal should be rejected") + } + if !strings.Contains(err.Error(), StagingPortal) { + t.Errorf("the error should name the contested portal: %v", err) + } + + // Two spellings of the same pair are not a collision; that is how every + // environment's short and long names coexist. + agreeing := map[string]Environment{ + "stage": {Portal: staging, Server: StagingServer}, + "staging": {Portal: staging, Server: StagingServer}, + } + if _, err := buildEnvironmentsByPortal(agreeing); err != nil { + t.Errorf("two spellings of one environment should build: %v", err) } } -func TestPortalOrigin(t *testing.T) { - if got := PortalOrigin("localhost:8080"); got != "https://localhost:8080" { - t.Fatalf("PortalOrigin = %q", got) +func TestEnvironmentForPortal(t *testing.T) { + // A portal spelled out in full finds the same pair its alias would. + for _, name := range EnvironmentNames() { + want, _ := LookupEnvironment(name) + got, ok := EnvironmentForPortal(want.Portal) + if !ok { + t.Errorf("EnvironmentForPortal(%q) not found", want.Portal.Host()) + continue + } + if got != want { + t.Errorf("EnvironmentForPortal(%q) = %+v, want %+v", want.Portal.Host(), got, want) + } + } + + if _, ok := EnvironmentForPortal(MustPortal("learn.example.concord.org")); ok { + t.Error("an unknown portal should not resolve to an environment") + } + // The reverse index is keyed by portal, not by alias name. + if _, ok := EnvironmentForPortal(AdoptStoredPortal("staging")); ok { + t.Error("an alias name is not a portal host") } } -func TestFirebaseSource(t *testing.T) { - if FirebaseSource("learn.concord.org") != "report-service-pro" { - t.Fatal("production portal should map to report-service-pro") +func TestExpandServerAlias(t *testing.T) { + if got := ExpandServerAlias("staging"); got != StagingServer { + t.Errorf("ExpandServerAlias(staging) = %q", got) } - if FirebaseSource("learn.portal.staging.concord.org") != "report-service-dev" { - t.Fatal("non-production portal should map to report-service-dev") + // Portal and server aliases are independent: each expands only its own side. + if got := ExpandServerAlias("dev"); got != DevServer { + t.Errorf("ExpandServerAlias(dev) = %q", got) } - if FirebaseSource("localhost:8080") != "report-service-dev" { - t.Fatal("dev portal should map to report-service-dev") + // Non-aliases pass through untouched, so full origins keep working. + for _, v := range []string{"", "https://report-server.concord.org"} { + if got := ExpandServerAlias(v); got != v { + t.Errorf("ExpandServerAlias(%q) = %q, want unchanged", v, got) + } } } diff --git a/internal/creds/creds.go b/internal/creds/creds.go index d4f140a..923e312 100644 --- a/internal/creds/creds.go +++ b/internal/creds/creds.go @@ -94,7 +94,8 @@ func writeFile(cf *CredFile) error { // Save stores a token for a portal, preferring the OS keychain and falling back // to the inline credentials file with a one-line stderr note. -func (Store) Save(portal, token, server string) error { +func (Store) Save(p config.Portal, token, server string) error { + portal := p.Host() cf, err := readFile() if err != nil { return err @@ -125,7 +126,8 @@ func (Store) Save(portal, token, server string) error { } // Token returns the stored token for a portal. -func (Store) Token(portal string) (string, error) { +func (Store) Token(p config.Portal) (string, error) { + portal := p.Host() cf, err := readFile() if err != nil { return "", err @@ -141,7 +143,8 @@ func (Store) Token(portal string) (string, error) { } // Get returns the token and the recorded minting server origin for a portal. -func (s Store) Get(portal string) (token, server string, err error) { +func (s Store) Get(p config.Portal) (token, server string, err error) { + portal := p.Host() cf, err := readFile() if err != nil { return "", "", err @@ -150,7 +153,7 @@ func (s Store) Get(portal string) (token, server string, err error) { if !ok { return "", "", ErrNotFound } - token, err = s.Token(portal) + token, err = s.Token(p) if err != nil { return "", "", err } @@ -158,7 +161,8 @@ func (s Store) Get(portal string) (token, server string, err error) { } // Delete removes a portal's credential from the keychain and the metadata file. -func (Store) Delete(portal string) error { +func (Store) Delete(p config.Portal) error { + portal := p.Host() cf, err := readFile() if err != nil { return err diff --git a/internal/creds/creds_test.go b/internal/creds/creds_test.go index 506d14f..b1c44de 100644 --- a/internal/creds/creds_test.go +++ b/internal/creds/creds_test.go @@ -32,10 +32,10 @@ func TestSaveTokenRoundTripKeyring(t *testing.T) { keyring.MockInit() var s Store - if err := s.Save("learn.concord.org", "ccd_abc", "https://report-server.concord.org"); err != nil { + if err := s.Save(config.MustPortal("learn.concord.org"), "ccd_abc", "https://report-server.concord.org"); err != nil { t.Fatal(err) } - got, err := s.Token("learn.concord.org") + got, err := s.Token(config.MustPortal("learn.concord.org")) if err != nil { t.Fatal(err) } @@ -62,10 +62,10 @@ func TestSaveFallbackToFile(t *testing.T) { t.Cleanup(func() { keyringSet = origSet }) var s Store - if err := s.Save("localhost:8080", "ccd_file", "http://localhost:4000"); err != nil { + if err := s.Save(config.MustPortal("localhost:8080"), "ccd_file", "http://localhost:4000"); err != nil { t.Fatal(err) } - got, err := s.Token("localhost:8080") + got, err := s.Token(config.MustPortal("localhost:8080")) if err != nil { t.Fatal(err) } @@ -85,13 +85,13 @@ func TestDelete(t *testing.T) { setupHome(t) keyring.MockInit() var s Store - if err := s.Save("a.concord.org", "ccd_a", "https://report-server.concord.org"); err != nil { + if err := s.Save(config.MustPortal("a.concord.org"), "ccd_a", "https://report-server.concord.org"); err != nil { t.Fatal(err) } - if err := s.Delete("a.concord.org"); err != nil { + if err := s.Delete(config.MustPortal("a.concord.org")); err != nil { t.Fatal(err) } - if _, err := s.Token("a.concord.org"); !errors.Is(err, ErrNotFound) { + if _, err := s.Token(config.MustPortal("a.concord.org")); !errors.Is(err, ErrNotFound) { t.Fatalf("expected ErrNotFound after delete, got %v", err) } } @@ -101,7 +101,7 @@ func TestListOrdering(t *testing.T) { keyring.MockInit() var s Store for _, p := range []string{"c.concord.org", "a.concord.org", "b.concord.org"} { - if err := s.Save(p, "ccd_"+p, "https://report-server.concord.org"); err != nil { + if err := s.Save(config.MustPortal(p), "ccd_"+p, "https://report-server.concord.org"); err != nil { t.Fatal(err) } } @@ -127,7 +127,7 @@ func TestCredFilePermission(t *testing.T) { setupHome(t) keyring.MockInit() var s Store - if err := s.Save("learn.concord.org", "ccd_abc", "https://report-server.concord.org"); err != nil { + if err := s.Save(config.MustPortal("learn.concord.org"), "ccd_abc", "https://report-server.concord.org"); err != nil { t.Fatal(err) } dir, _ := config.ConfigDir() diff --git a/internal/dataset/dataset.go b/internal/dataset/dataset.go index 43247c4..272a22e 100644 --- a/internal/dataset/dataset.go +++ b/internal/dataset/dataset.go @@ -9,6 +9,7 @@ import ( "strings" "time" + "github.com/concord-consortium/cc-data-cli/internal/config" "github.com/concord-consortium/cc-data-cli/internal/store" ) @@ -322,13 +323,13 @@ func isArtifactFile(name string) bool { // AutoName generates a {date}_{slug} name (slug from description) or a // {date}_{n} counter scanning existing names. -func AutoName(dataRoot, portalHost, description string) (string, error) { +func AutoName(dataRoot string, portal config.Portal, description string) (string, error) { date := clock().UTC().Format("2006-01-02") if slug := kebab(description); slug != "" { return date + "_" + slug, nil } existing := map[string]bool{} - dir := PortalDatasetsDir(dataRoot, portalHost) + dir := PortalDatasetsDir(dataRoot, portal) if entries, err := os.ReadDir(dir); err == nil { for _, e := range entries { if e.IsDir() { diff --git a/internal/dataset/dataset_test.go b/internal/dataset/dataset_test.go index 8343277..dab2af2 100644 --- a/internal/dataset/dataset_test.go +++ b/internal/dataset/dataset_test.go @@ -6,13 +6,15 @@ import ( "path/filepath" "testing" "time" + + "github.com/concord-consortium/cc-data-cli/internal/config" ) func fixedClock() time.Time { return time.Date(2026, 7, 17, 0, 0, 0, 0, time.UTC) } func TestCRUD(t *testing.T) { root := t.TempDir() - ref := Ref{Portal: "learn.concord.org", Name: "wildfire"} + ref := Ref{Portal: config.MustPortal("learn.concord.org"), Name: "wildfire"} d, err := Create(root, ref, "Wildfire study") if err != nil { @@ -62,7 +64,7 @@ func TestCRUD(t *testing.T) { // the portal folder, and a listing sees nothing. func TestDeleteLeavesNoTombstoneResidue(t *testing.T) { root := t.TempDir() - ref := Ref{Portal: "learn.concord.org", Name: "gone"} + ref := Ref{Portal: config.MustPortal("learn.concord.org"), Name: "gone"} d, err := Create(root, ref, "") if err != nil { t.Fatal(err) @@ -104,7 +106,7 @@ func TestDeleteLeavesNoTombstoneResidue(t *testing.T) { // would already have destroyed them by the time the commit failed.) func TestPurgeKeepsArtifactsWhenManifestCommitFails(t *testing.T) { root := t.TempDir() - d, err := Create(root, Ref{Portal: "learn.concord.org", Name: "ds"}, "") + d, err := Create(root, Ref{Portal: config.MustPortal("learn.concord.org"), Name: "ds"}, "") if err != nil { t.Fatal(err) } @@ -128,7 +130,7 @@ func TestPurgeKeepsArtifactsWhenManifestCommitFails(t *testing.T) { func TestCreateRejectsReservedName(t *testing.T) { root := t.TempDir() - _, err := Create(root, Ref{Portal: "learn.concord.org", Name: "main"}, "") + _, err := Create(root, Ref{Portal: config.MustPortal("learn.concord.org"), Name: "main"}, "") if err == nil { t.Fatal("create main should be rejected") } @@ -136,7 +138,7 @@ func TestCreateRejectsReservedName(t *testing.T) { func TestPurgeKeepsShell(t *testing.T) { root := t.TempDir() - ref := Ref{Portal: "learn.concord.org", Name: "ds"} + ref := Ref{Portal: config.MustPortal("learn.concord.org"), Name: "ds"} d, err := Create(root, ref, "desc") if err != nil { t.Fatal(err) @@ -176,7 +178,7 @@ func TestPurgeKeepsShell(t *testing.T) { func TestMutationBusyWhenActivityHeld(t *testing.T) { root := t.TempDir() - ref := Ref{Portal: "learn.concord.org", Name: "ds"} + ref := Ref{Portal: config.MustPortal("learn.concord.org"), Name: "ds"} d, err := Create(root, ref, "") if err != nil { t.Fatal(err) @@ -206,7 +208,7 @@ func TestAutoName(t *testing.T) { clock = fixedClock defer func() { clock = defaultClock }() root := t.TempDir() - name, err := AutoName(root, "learn.concord.org", "Wildfire Study!") + name, err := AutoName(root, config.MustPortal("learn.concord.org"), "Wildfire Study!") if err != nil { t.Fatal(err) } @@ -218,8 +220,8 @@ func TestAutoName(t *testing.T) { } // No description: counter. - os.MkdirAll(filepath.Join(PortalDatasetsDir(root, "learn.concord.org"), "2026-07-17_1"), 0o700) - name2, _ := AutoName(root, "learn.concord.org", "") + os.MkdirAll(filepath.Join(PortalDatasetsDir(root, config.MustPortal("learn.concord.org")), "2026-07-17_1"), 0o700) + name2, _ := AutoName(root, config.MustPortal("learn.concord.org"), "") if name2 != "2026-07-17_2" { t.Fatalf("counter name = %q", name2) } diff --git a/internal/dataset/merge_test.go b/internal/dataset/merge_test.go index 5b21dd8..0805d12 100644 --- a/internal/dataset/merge_test.go +++ b/internal/dataset/merge_test.go @@ -9,13 +9,14 @@ import ( "testing" "time" + "github.com/concord-consortium/cc-data-cli/internal/config" "github.com/concord-consortium/cc-data-cli/internal/store" ) func newDataset(t *testing.T) *Dataset { t.Helper() root := t.TempDir() - d, err := Create(root, Ref{Portal: "learn.concord.org", Name: "ds"}, "") + d, err := Create(root, Ref{Portal: config.MustPortal("learn.concord.org"), Name: "ds"}, "") if err != nil { t.Fatal(err) } diff --git a/internal/dataset/ref.go b/internal/dataset/ref.go index 3dcc26d..02e5e83 100644 --- a/internal/dataset/ref.go +++ b/internal/dataset/ref.go @@ -25,19 +25,91 @@ var reservedSchemaNames = map[string]bool{ // Ref is a resolved dataset reference. type Ref struct { - Portal string // real host + Portal config.Portal Name string } -func (r Ref) String() string { return r.Portal + "/" + r.Name } +func (r Ref) String() string { return r.Portal.Host() + "/" + r.Name } + +// ParseRefForConfig resolves a ref against the configured default portal. Every +// caller goes through this rather than reading cfg.DefaultPortal directly, so +// the raw config string never reaches a Ref unparsed. +func ParseRefForConfig(cfg *config.Config, raw string) (Ref, error) { + defaultPortal, err := cfg.DefaultPortalValue() + if err != nil { + return Ref{}, err + } + return ParseRef(raw, defaultPortal) +} + +// ParseRefForExisting resolves a ref for any command that names a dataset +// already on disk (everything but create), making one exception ParseRefForConfig +// does not: a portal ParsePortalIdentity refuses is accepted when a dataset +// actually exists under it. +// +// dataset list builds its rows from folder names and never parses them, so +// without this a folder an earlier, laxer build wrote could be visible and +// untouchable, removable only with rm -rf. That covers two kinds of stranded +// folder: an environment-alias name, and a host shape 0.1.0's NormalizePortal +// accepted but this build rejects (an underscore, an IPv6 literal). Both route +// through config.AdoptExistingPortal, which refuses anything that is not a single +// safe path component, so a traversal can never reach os.RemoveAll by claiming +// the folder exists. When no folder exists the original strict refusal stands, +// since for a genuinely new dataset the fix is to name the hostname. +func ParseRefForExisting(cfg *config.Config, dataRoot, raw string) (Ref, error) { + defaultPortal, err := cfg.DefaultPortalValue() + if err != nil { + return Ref{}, err + } + portalValue, name, err := splitRef(raw, defaultPortal) + if err != nil { + return Ref{}, err + } + portal, portalErr := config.ParsePortalIdentity(portalValue) + if portalErr == nil { + return buildRef(portal, name, raw) + } + adopted, ok := config.AdoptExistingPortal(portalValue) + if !ok { + return Ref{}, portalErr + } + // Validate the name before consulting the disk, so an invalid name still + // reports as one rather than as the portal refusal. If the name is fine but no + // folder exists, the strict refusal stands. + ref, err := buildRef(adopted, name, raw) + if err != nil { + return Ref{}, err + } + if !Open(dataRoot, ref).Exists() { + return Ref{}, portalErr + } + return ref, nil +} // ParseRef resolves "/" or a bare "" (under defaultPortal). -// The name is validated with ValidateName so no ref can carry path-traversal -// segments into filesystem sinks (os.RemoveAll, MkdirAll) or MCP dataset_create. -func ParseRef(raw, defaultPortal string) (Ref, error) { +// Neither half can carry path-traversal segments into filesystem sinks +// (os.RemoveAll, MkdirAll) or MCP dataset_create: the name is validated with +// ValidateName, and the portal by config.ParsePortalIdentity, which is also what +// refuses an environment alias here. +func ParseRef(raw string, defaultPortal config.Portal) (Ref, error) { + portalValue, name, err := splitRef(raw, defaultPortal) + if err != nil { + return Ref{}, err + } + portal, err := config.ParsePortalIdentity(portalValue) + if err != nil { + return Ref{}, err + } + return buildRef(portal, name, raw) +} + +// splitRef splits a ref into the portal value to parse and the dataset name, +// without deciding whether either is acceptable. ParseRefForExisting reuses it +// to recover the name after the portal has been refused. +func splitRef(raw string, defaultPortal config.Portal) (portalValue, name string, err error) { raw = strings.TrimSpace(raw) if raw == "" { - return Ref{}, fmt.Errorf("dataset ref is empty") + return "", "", fmt.Errorf("dataset ref is empty") } // Strip an optional scheme so the first-slash split names the dataset, not // the "//" of a scheme. @@ -45,19 +117,27 @@ func ParseRef(raw, defaultPortal string) (Ref, error) { if i := strings.Index(raw, "://"); i >= 0 { scheme, raw = raw[:i+3], raw[i+3:] } - var portalPart, name string + var portalPart string if i := strings.Index(raw, "/"); i >= 0 { portalPart, name = raw[:i], raw[i+1:] } else { name = raw - if defaultPortal == "" { - return Ref{}, fmt.Errorf("no portal in ref %q and no default_portal configured", raw) + if defaultPortal.IsZero() { + return "", "", fmt.Errorf("no portal in ref %q and no default_portal configured", raw) } - portalPart = defaultPortal + portalPart = defaultPortal.Host() } - host, err := config.NormalizePortal(scheme + portalPart) - if err != nil { - return Ref{}, err + return scheme + portalPart, name, nil +} + +// buildRef validates both halves and assembles the ref. It is the single choke +// point every Ref passes through, so it also rejects a zero portal: a Ref{} +// portal would make Ref.Dir resolve to /datasets/, a location +// dataset list never scans. Every caller supplies a parsed or adopted portal, so +// this only fires on a programming error. +func buildRef(portal config.Portal, name, raw string) (Ref, error) { + if portal.IsZero() { + return Ref{}, fmt.Errorf("dataset ref %q has no portal", raw) } if name == "" { return Ref{}, fmt.Errorf("dataset ref %q has no name", raw) @@ -65,18 +145,18 @@ func ParseRef(raw, defaultPortal string) (Ref, error) { if err := ValidateName(name); err != nil { return Ref{}, err } - return Ref{Portal: host, Name: name}, nil + return Ref{Portal: portal, Name: name}, nil } // Dir returns the dataset directory under a data root, using the filesystem // folder encoding for the portal host. func (r Ref) Dir(dataRoot string) string { - return filepath.Join(dataRoot, config.PortalFolder(r.Portal), "datasets", r.Name) + return filepath.Join(dataRoot, r.Portal.Folder(), "datasets", r.Name) } // PortalDatasetsDir returns the datasets directory for a portal under a data root. -func PortalDatasetsDir(dataRoot, portalHost string) string { - return filepath.Join(dataRoot, config.PortalFolder(portalHost), "datasets") +func PortalDatasetsDir(dataRoot string, portal config.Portal) string { + return filepath.Join(dataRoot, portal.Folder(), "datasets") } // ValidateName enforces the dataset name alphabet and the reserved-schema diff --git a/internal/dataset/ref_test.go b/internal/dataset/ref_test.go index ecbd7ba..19c6df7 100644 --- a/internal/dataset/ref_test.go +++ b/internal/dataset/ref_test.go @@ -1,10 +1,22 @@ package dataset import ( + "os" + "path/filepath" "strings" "testing" + + "github.com/concord-consortium/cc-data-cli/internal/config" ) +// portal builds a default-portal fixture; the zero Portal means "none configured". +func portal(host string) config.Portal { + if host == "" { + return config.Portal{} + } + return config.MustPortal(host) +} + func TestParseRef(t *testing.T) { cases := []struct { raw string @@ -20,9 +32,15 @@ func TestParseRef(t *testing.T) { {"localhost:8080/ds", "", "localhost:8080", "ds", false}, {"", "x", "", "", true}, {"portal/", "", "", "", true}, // empty name + // The portal half can no longer carry a traversal into Ref.Dir. + {"../wildfire", "", "", "", true}, + // With a default portal set, "..%2f..%2fwildfire" resolves the portal and + // is refused by name validation (the %2f keeps it one slash-free token), so + // this exercises the name guard rather than the missing-default path. + {"..%2f..%2fwildfire", "learn.concord.org", "", "", true}, } for _, c := range cases { - ref, err := ParseRef(c.raw, c.def) + ref, err := ParseRef(c.raw, portal(c.def)) if c.wantErr { if err == nil { t.Fatalf("ParseRef(%q,%q) should error", c.raw, c.def) @@ -32,12 +50,74 @@ func TestParseRef(t *testing.T) { if err != nil { t.Fatalf("ParseRef(%q,%q) error: %v", c.raw, c.def, err) } - if ref.Portal != c.portal || ref.Name != c.name { + if ref.Portal.Host() != c.portal || ref.Name != c.name { t.Fatalf("ParseRef(%q) = %+v, want %s/%s", c.raw, ref, c.portal, c.name) } } } +// TestParseRefRejectsEnvironmentAliases pins that a ref refuses an environment +// alias rather than expanding it or taking it literally. Expanding would give one +// folder two names; taking it literally files the data under a portal that can +// never hold a credential, which reads downstream as a NOT_AUTHENTICATED that +// "cc-data login staging" appears to fix and does not. +func TestParseRefRejectsEnvironmentAliases(t *testing.T) { + for _, alias := range []string{"staging", "prod", "dev", "production", "STAGING"} { + _, err := ParseRef(alias+"/foo", config.Portal{}) + if err == nil { + t.Errorf("ParseRef(%q/foo) should be refused as an alias", alias) + continue + } + if !strings.Contains(err.Error(), "environment alias") { + t.Errorf("ParseRef(%q/foo) error should explain the alias: %v", alias, err) + } + } + // A full hostname is of course still fine. + ref, err := ParseRef(config.StagingPortal+"/foo", config.Portal{}) + if err != nil { + t.Fatal(err) + } + if ref.Portal.Host() != config.StagingPortal { + t.Errorf("ref portal = %q", ref.Portal.Host()) + } +} + +// TestParseRefStaysInsideTheDataRoot is the property the traversal rows protect: +// Ref.Dir feeds MkdirAll and, via dataset delete, os.RemoveAll. It checks two +// things that are otherwise easy to conflate: that an accepted ref's Dir stays +// under the root (the assertion, run on real accepted refs), and that the +// traversal inputs are refused before they can reach Dir at all. +func TestParseRefStaysInsideTheDataRoot(t *testing.T) { + root := filepath.Join("/data", "root") + def := config.MustPortal("learn.concord.org") + + // Accepted refs must resolve strictly inside the root. This is the case the + // old version never reached, because every input it tried was refused. + accepted := 0 + for _, raw := range []string{"wildfire", "learn.concord.org/wildfire", "localhost:8080/ds", "a-b.concord.org/x"} { + ref, err := ParseRef(raw, def) + if err != nil { + t.Errorf("ParseRef(%q) should be accepted: %v", raw, err) + continue + } + accepted++ + dir := filepath.Clean(ref.Dir(root)) + if !strings.HasPrefix(dir, filepath.Clean(root)+string(filepath.Separator)) { + t.Errorf("ParseRef(%q).Dir escapes the data root: %s", raw, dir) + } + } + if accepted == 0 { + t.Fatal("no accepted ref reached the containment assertion") + } + + // Traversal inputs never produce a ref, so they never reach Dir. + for _, raw := range []string{"../wildfire", "../../wildfire", "./wildfire"} { + if _, err := ParseRef(raw, def); err == nil { + t.Errorf("ParseRef(%q) must be refused", raw) + } + } +} + func TestValidateName(t *testing.T) { valid := []string{"a", "wildfire", "2026-07-16_wildfire", "a_b", "a-b", strings.Repeat("a", 63)} for _, n := range valid { @@ -62,9 +142,122 @@ func TestValidateName(t *testing.T) { } func TestRefDirEncoding(t *testing.T) { - ref := Ref{Portal: "localhost:8080", Name: "ds"} + ref := Ref{Portal: config.MustPortal("localhost:8080"), Name: "ds"} dir := ref.Dir("/root") if !strings.Contains(dir, "localhost_8080") { t.Fatalf("dir should encode the port: %s", dir) } } + +// TestParseRefForExistingReachesRefusedFolder covers the exception every command +// that names an existing dataset makes. dataset list builds its rows from folder +// names and never parses them, so a folder written under a portal this build now +// refuses (an environment-alias name, or a host shape an earlier build allowed) +// would otherwise be visible and removable only with rm -rf. +func TestParseRefForExistingReachesRefusedFolder(t *testing.T) { + root := t.TempDir() + cfg := &config.Config{} + + // A folder that could only have been created by an earlier build. + stranded := Ref{Portal: config.AdoptStoredPortal("staging"), Name: "wildfire"} + if err := os.MkdirAll(stranded.Dir(root), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(stranded.Dir(root), "manifest.json"), []byte(`{"name":"wildfire"}`), 0o600); err != nil { + t.Fatal(err) + } + + // The ordinary parser still refuses it, so nothing new can be created there. + if _, err := ParseRefForConfig(cfg, "staging/wildfire"); err == nil { + t.Error("ParseRefForConfig should still refuse an alias portal") + } + // The existing-dataset parser reaches it, so it can be shown and deleted. + ref, err := ParseRefForExisting(cfg, root, "staging/wildfire") + if err != nil { + t.Fatalf("a stranded dataset should be reachable: %v", err) + } + if ref.Portal.Host() != "staging" || ref.Name != "wildfire" { + t.Errorf("ParseRefForExisting = %+v", ref) + } + + // The same salvage reaches a folder under a host shape 0.1.0's laxer + // NormalizePortal accepted but this build rejects (an underscore). Without it, + // such a folder would be listed but removable only with rm -rf. + underscored := Ref{Portal: config.AdoptStoredPortal("a_b.concord.org"), Name: "wildfire"} + if err := os.MkdirAll(underscored.Dir(root), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(underscored.Dir(root), "manifest.json"), []byte(`{"name":"wildfire"}`), 0o600); err != nil { + t.Fatal(err) + } + if _, err := ParseRefForConfig(cfg, "a_b.concord.org/wildfire"); err == nil { + t.Error("ParseRefForConfig should still refuse an underscored host") + } + uref, err := ParseRefForExisting(cfg, root, "a_b.concord.org/wildfire") + if err != nil { + t.Fatalf("a stranded underscored-host dataset should be reachable: %v", err) + } + if uref.Portal.Host() != "a_b.concord.org" || uref.Name != "wildfire" { + t.Errorf("ParseRefForExisting = %+v", uref) + } + + // The exception is only for what is actually on disk. + if _, err := ParseRefForExisting(cfg, root, "staging/absent"); err == nil { + t.Error("an alias portal with no folder should still be refused") + } + if _, err := ParseRefForExisting(cfg, root, "a_b.concord.org/absent"); err == nil { + t.Error("an underscored host with no folder should still be refused") + } + + // An invalid name reports as an invalid name, not as the alias refusal, so + // the message matches what a hostname portal would give for the same ref. + _, err = ParseRefForExisting(cfg, root, "staging/MAIN") + if err == nil { + t.Fatal("an invalid dataset name should be refused") + } + if !strings.Contains(err.Error(), "invalid dataset name") { + t.Errorf("staging/MAIN should report the name error, got: %v", err) + } + // And never for a syntax refusal: that is the traversal guard, not a policy. + for _, raw := range []string{"../wildfire", "../../wildfire"} { + if _, err := ParseRefForExisting(cfg, root, raw); err == nil { + t.Errorf("ParseRefForExisting(%q) must stay refused", raw) + } + } +} + +// TestPortalFolderRoundTripsExactly pins the property that lets the folder name +// serve as the only on-disk record of a dataset's portal: for every host the +// parser accepts, encoding it to a folder and decoding it back returns the same +// host. config.hostSyntax excludes underscores and IPv6 literals precisely so +// this holds; without that, a dataset under "host_3000" would be reported by +// dataset list as belonging to "host:3000", a different portal. +func TestPortalFolderRoundTripsExactly(t *testing.T) { + hosts := []string{ + "learn.concord.org", + "learn.portal.staging.concord.org", + "localhost", + "localhost:3000", + "127.0.0.1:3000", + "portal.example.org:8443", + "my-portal.concord.org", + } + for _, host := range hosts { + p, err := config.ParsePortalIdentity(host) + if err != nil { + t.Errorf("ParsePortalIdentity(%q) should be accepted: %v", host, err) + continue + } + if back := decodePortalFolder(p.Folder()); back != host { + t.Errorf("%q -> folder %q -> %q, want the original host", host, p.Folder(), back) + } + } + + // The hosts that could not round-trip are refused outright, so the property + // above has no exceptions to carve out. + for _, host := range []string{"host_3000", "a_b.concord.org", "[::1]", "[::1]:3000"} { + if _, err := config.ParsePortalIdentity(host); err == nil { + t.Errorf("ParsePortalIdentity(%q) should be refused: it cannot round-trip", host) + } + } +} diff --git a/internal/dataset/summary.go b/internal/dataset/summary.go index 750807a..5dcd4b5 100644 --- a/internal/dataset/summary.go +++ b/internal/dataset/summary.go @@ -69,7 +69,7 @@ func (d *Dataset) BuildShowJSON(full bool) (*ShowJSON, error) { Ref: d.Ref.String(), Name: m.Name, Description: m.Description, - Portal: d.Ref.Portal, + Portal: d.Ref.Portal.Host(), CreatedAt: m.CreatedAt, Totals: manifestTotals(m), SizeBytes: size, diff --git a/internal/dataset/summary_test.go b/internal/dataset/summary_test.go index 526af53..aec4f1c 100644 --- a/internal/dataset/summary_test.go +++ b/internal/dataset/summary_test.go @@ -4,6 +4,8 @@ import ( "os" "strings" "testing" + + "github.com/concord-consortium/cc-data-cli/internal/config" ) func TestShowJSONSchema(t *testing.T) { @@ -68,8 +70,8 @@ func TestListJSON(t *testing.T) { clock = fixedClock defer func() { clock = defaultClock }() root := t.TempDir() - Create(root, Ref{Portal: "learn.concord.org", Name: "a"}, "first") - Create(root, Ref{Portal: "localhost:8080", Name: "b"}, "second") + Create(root, Ref{Portal: config.MustPortal("learn.concord.org"), Name: "a"}, "first") + Create(root, Ref{Portal: config.MustPortal("localhost:8080"), Name: "b"}, "second") list, err := BuildListJSON(root) if err != nil { diff --git a/internal/duck/engine_test.go b/internal/duck/engine_test.go index 67cea5d..9dfb469 100644 --- a/internal/duck/engine_test.go +++ b/internal/duck/engine_test.go @@ -11,6 +11,7 @@ import ( "testing" "time" + "github.com/concord-consortium/cc-data-cli/internal/config" "github.com/concord-consortium/cc-data-cli/internal/dataset" "github.com/concord-consortium/cc-data-cli/internal/store" ) @@ -54,7 +55,7 @@ func addReportCSV(t *testing.T, d *dataset.Dataset, run int, reportType, content func newDS(t *testing.T, name string) *dataset.Dataset { t.Helper() root := t.TempDir() - d, err := dataset.Create(root, dataset.Ref{Portal: "learn.concord.org", Name: name}, "") + d, err := dataset.Create(root, dataset.Ref{Portal: config.MustPortal("learn.concord.org"), Name: name}, "") if err != nil { t.Fatal(err) } @@ -204,9 +205,9 @@ func TestEngineMultiDatasetSchemas(t *testing.T) { func TestEngineSchemaCollisionRequiresAlias(t *testing.T) { root := t.TempDir() - d1, _ := dataset.Create(root, dataset.Ref{Portal: "a.concord.org", Name: "same"}, "") + d1, _ := dataset.Create(root, dataset.Ref{Portal: config.MustPortal("a.concord.org"), Name: "same"}, "") root2 := t.TempDir() - d2, _ := dataset.Create(root2, dataset.Ref{Portal: "b.concord.org", Name: "same"}, "") + d2, _ := dataset.Create(root2, dataset.Ref{Portal: config.MustPortal("b.concord.org"), Name: "same"}, "") _, err := Open(context.Background(), []DatasetSpec{{DS: d1}, {DS: d2}}, nil, io.Discard) if err == nil { @@ -238,7 +239,7 @@ func TestEngineDataRootWithQuote(t *testing.T) { if err := os.MkdirAll(base, 0o700); err != nil { t.Skipf("cannot create quote dir: %v", err) } - d, err := dataset.Create(base, dataset.Ref{Portal: "learn.concord.org", Name: "ds"}, "") + d, err := dataset.Create(base, dataset.Ref{Portal: config.MustPortal("learn.concord.org"), Name: "ds"}, "") if err != nil { t.Fatal(err) } @@ -305,8 +306,8 @@ func TestResolveSchemasLegacyMainRequiresAlias(t *testing.T) { // A legacy dataset named "main" (constructed via Open, bypassing the create // validator) must require an alias in a multi-dataset registration. specs := []DatasetSpec{ - {DS: dataset.Open(root, dataset.Ref{Portal: "learn.concord.org", Name: "wildfire"})}, - {DS: dataset.Open(root, dataset.Ref{Portal: "learn.concord.org", Name: "main"})}, + {DS: dataset.Open(root, dataset.Ref{Portal: config.MustPortal("learn.concord.org"), Name: "wildfire"})}, + {DS: dataset.Open(root, dataset.Ref{Portal: config.MustPortal("learn.concord.org"), Name: "main"})}, } if _, err := resolveSchemas(specs); err == nil { t.Fatal("a legacy dataset named main should require an alias") @@ -321,7 +322,7 @@ func TestResolveSchemasLegacyMainRequiresAlias(t *testing.T) { t.Fatalf("aliased schema = %q", schemas[1]) } // A single dataset named main uses the default schema and is fine. - if _, err := resolveSchemas([]DatasetSpec{{DS: dataset.Open(root, dataset.Ref{Portal: "p", Name: "main"})}}); err != nil { + if _, err := resolveSchemas([]DatasetSpec{{DS: dataset.Open(root, dataset.Ref{Portal: config.MustPortal("p"), Name: "main"})}}); err != nil { t.Fatalf("single main dataset should be allowed: %v", err) } } diff --git a/internal/fetch/report_test.go b/internal/fetch/report_test.go index e73d8c2..8426240 100644 --- a/internal/fetch/report_test.go +++ b/internal/fetch/report_test.go @@ -16,6 +16,7 @@ import ( "time" "github.com/concord-consortium/cc-data-cli/internal/api" + "github.com/concord-consortium/cc-data-cli/internal/config" "github.com/concord-consortium/cc-data-cli/internal/dataset" "github.com/concord-consortium/cc-data-cli/internal/output" "github.com/concord-consortium/cc-data-cli/internal/store" @@ -33,7 +34,7 @@ func newTestDataset(t *testing.T) *dataset.Dataset { } keyring.MockInit() root := t.TempDir() - d, err := dataset.Create(root, dataset.Ref{Portal: "learn.concord.org", Name: "ds"}, "") + d, err := dataset.Create(root, dataset.Ref{Portal: config.MustPortal("learn.concord.org"), Name: "ds"}, "") if err != nil { t.Fatal(err) } diff --git a/internal/mcpserver/server.go b/internal/mcpserver/server.go index 19a0a39..a2a4c59 100644 --- a/internal/mcpserver/server.go +++ b/internal/mcpserver/server.go @@ -76,18 +76,23 @@ func loadRuntime() (*config.Config, string, error) { return cfg, root, nil } -func openDataset(refStr string) (*dataset.Dataset, string, error) { +// openDataset resolves a ref that names a dataset already on disk. Every MCP +// tool that opens a dataset (all but dataset_create) goes through it, so it uses +// the salvaging parser: dataset_list builds its rows from folder names, and this +// has to reach anything it shows, including a folder under a portal the strict +// parser would refuse. +func openDataset(refStr string) (*dataset.Dataset, config.Portal, error) { cfg, root, err := loadRuntime() if err != nil { - return nil, "", err + return nil, config.Portal{}, err } - ref, err := dataset.ParseRef(refStr, cfg.DefaultPortal) + ref, err := dataset.ParseRefForExisting(cfg, root, refStr) if err != nil { - return nil, "", err + return nil, config.Portal{}, err } d := dataset.Open(root, ref) if !d.Exists() { - return nil, "", &output.CLIError{Code: "NOT_FOUND", Message: "dataset " + ref.String() + " does not exist"} + return nil, config.Portal{}, &output.CLIError{Code: "NOT_FOUND", Message: "dataset " + ref.String() + " does not exist"} } return d, ref.Portal, nil } diff --git a/internal/mcpserver/server_test.go b/internal/mcpserver/server_test.go index 3f29096..3185822 100644 --- a/internal/mcpserver/server_test.go +++ b/internal/mcpserver/server_test.go @@ -6,6 +6,7 @@ import ( "runtime" "testing" + "github.com/concord-consortium/cc-data-cli/internal/config" "github.com/concord-consortium/cc-data-cli/internal/dataset" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/zalando/go-keyring" @@ -155,7 +156,7 @@ func TestMCPDatasetShowParity(t *testing.T) { _, out := callJSON(t, cs, "dataset_show", map[string]any{"ref": "learn.concord.org/ds"}) // The tool payload must equal the CLI's BuildShowJSON for the same dataset. - d := dataset.Open(root, dataset.Ref{Portal: "learn.concord.org", Name: "ds"}) + d := dataset.Open(root, dataset.Ref{Portal: config.MustPortal("learn.concord.org"), Name: "ds"}) cliJSON, _ := d.BuildShowJSON(false) cliBytes, _ := json.Marshal(cliJSON) toolBytes, _ := json.Marshal(out) @@ -173,7 +174,7 @@ func TestMCPDatasetShowParity(t *testing.T) { func TestMCPQueryTruncation(t *testing.T) { root := setupEnv(t) cs := connect(t) - if _, err := dataset.Create(root, dataset.Ref{Portal: "learn.concord.org", Name: "ds"}, ""); err != nil { + if _, err := dataset.Create(root, dataset.Ref{Portal: config.MustPortal("learn.concord.org"), Name: "ds"}, ""); err != nil { t.Fatal(err) } res, err := cs.CallTool(context.Background(), &mcp.CallToolParams{ diff --git a/internal/mcpserver/tools.go b/internal/mcpserver/tools.go index 7731fad..49c424b 100644 --- a/internal/mcpserver/tools.go +++ b/internal/mcpserver/tools.go @@ -7,7 +7,6 @@ import ( "github.com/concord-consortium/cc-data-cli/internal/api" "github.com/concord-consortium/cc-data-cli/internal/auth" - "github.com/concord-consortium/cc-data-cli/internal/config" "github.com/concord-consortium/cc-data-cli/internal/dataset" "github.com/concord-consortium/cc-data-cli/internal/duck" "github.com/concord-consortium/cc-data-cli/internal/fetch" @@ -36,7 +35,7 @@ func registerTools(s *mcp.Server, opts Options) { return nil, res, err }) - mcp.AddTool(s, &mcp.Tool{Name: "reports_list", Description: "List the user's report runs for a portal.", Annotations: readOnly}, + mcp.AddTool(s, &mcp.Tool{Name: "reports_list", Description: "List the user's report runs for a portal. The portal may be a hostname or an environment alias (prod / staging / dev).", Annotations: readOnly}, func(ctx context.Context, req *mcp.CallToolRequest, in portalIn) (*mcp.CallToolResult, reportview.RunsPayload, error) { client, err := portalClient(in.Portal) if err != nil { @@ -49,7 +48,7 @@ func registerTools(s *mcp.Server, opts Options) { return nil, reportview.Runs(runs), nil }) - mcp.AddTool(s, &mcp.Tool{Name: "reports_jobs", Description: "List a run's post-processing jobs.", Annotations: readOnly}, + mcp.AddTool(s, &mcp.Tool{Name: "reports_jobs", Description: "List a run's post-processing jobs. The portal may be a hostname or an environment alias (prod / staging / dev).", Annotations: readOnly}, func(ctx context.Context, req *mcp.CallToolRequest, in reportsJobsIn) (*mcp.CallToolResult, reportview.JobsPayload, error) { client, err := portalClient(in.Portal) if err != nil { @@ -98,7 +97,7 @@ func registerTools(s *mcp.Server, opts Options) { if err != nil { return nil, nil, err } - ref, err := dataset.ParseRef(in.Ref, cfg.DefaultPortal) + ref, err := dataset.ParseRefForConfig(cfg, in.Ref) if err != nil { return nil, nil, err } @@ -233,8 +232,11 @@ func fetchResult(result any, err error) (*mcp.CallToolResult, mapOut, error) { return nil, mapOut{}, nil } +// portalClient resolves a portal argument to an authenticated client, accepting +// the same environment aliases and hostnames the CLI's --portal does, including +// a portal only a stored credential vouches for. func portalClient(portal string) (*api.Client, error) { - host, err := config.NormalizePortal(portal) + host, _, err := auth.ResolvePortalTarget(portal) if err != nil { return nil, err } @@ -259,7 +261,7 @@ func queryHandler(opts Options) func(context.Context, *mcp.CallToolRequest, quer if i := strings.Index(raw, "="); i >= 0 { alias, refStr = raw[:i], raw[i+1:] } - ref, perr := dataset.ParseRef(refStr, cfg.DefaultPortal) + ref, perr := dataset.ParseRefForExisting(cfg, root, refStr) if perr != nil { return nil, queryOut{}, perr }