From 47112a7d5512c2d07cd7141c3f05510c9383679a Mon Sep 17 00:00:00 2001 From: Doug Martin Date: Fri, 24 Jul 2026 14:36:22 -0400 Subject: [PATCH 1/3] feat: add environment aliases and consolidate portal parsing [REPORT-87] Add prod/staging/dev environment aliases so one word sets a portal and the report server it is paired with: "cc-data login staging" cannot end up talking to the production server. The aliases work on --portal and --server, expanding independently so "--portal staging --server dev" is meaningful, and on the --portal flag of logout, reports list, reports jobs, and the MCP portal arguments. Aliases are refused, not expanded, wherever a portal names something on disk (a dataset ref, default_portal). Expanding there would give one dataset folder two names; taking the alias literally would file data under a portal that can never hold a credential, which reads downstream as a NOT_AUTHENTICATED that logging in appears to fix and does not. The rule the whole change follows: an environment alias names a network target, a portal that names something on disk is always a hostname. Adding a fourth meaning to the portal string exposed that four different functions already turned user input into a portal, each with different acceptance rules, and that one call site used none of them. Replace them with a config.Portal type no other package can build from a string, and two parsers: ParsePortalTarget for a portal to talk to (aliases expand, a near-miss alias is refused) and ParsePortalIdentity for a portal that names something on disk (aliases refused, single-label hosts preserved). auth.ResolvePortalTarget adds the one exception both need, accepting a host a stored credential vouches for, so a credential written by an older build can still be listed, used, revoked, and refreshed. dataset.Ref, the creds store, api.ForPortal and auth.LoginOptions all take config.Portal, so a portal cannot reach a credential key, a dataset folder, or an auth URL unparsed. Fix a path traversal that predates this branch: "cc-data dataset create ../wildfire" resolved outside the configured data root, reaching MkdirAll and, via dataset delete, os.RemoveAll, and was reachable through the MCP dataset_create tool. url.Parse reports ".." as the host of "https://../x" and the folder encoding passed it through. The parsers now require a host to be shaped like one, which is also what makes a portal a single path component. Also fixed along the way: - A named environment supplies its server for a portal no environment claims, so "cc-data login dev --portal localhost:3005" stops exchanging its code at the production report server. The help text and researcher guide stated a precedence the code did not implement. - default_portal is normalized once at load. Left raw, "https://learn.concord.org" and "learn.concord.org" named the same portal but two different folders, and the auto-named "dataset create" path filed data under the URL-shaped one where dataset list could not see it. - dataset show, delete and purge reach a dataset whose portal the parser refuses when the folder exists, so nothing dataset list shows is removable only with rm -rf. Scoped to the alias refusal, never a syntax refusal, so it cannot become a traversal. - Portal hosts exclude underscores and IPv6 literals, the two shapes the folder encoding could not represent reversibly. A dataset under "host_3000" was reported by dataset list as belonging to "host:3000". - auth status gains a SERVER column: a run belongs to a portal and to the server behind it, and a credential stored before the server was recorded shows "-". - An unreadable credential store no longer reports as a misspelled portal. Tests: a portal matrix crossing every input shape against both parsers, a property test that whatever either parser accepts stays a single path component inside the data root, an exact folder round-trip test, and a login precedence table covering every spelling of a portal against every server configuration. --- README.md | 13 +- cmd/auth.go | 22 +- cmd/auth_test.go | 46 ++++ cmd/common.go | 13 +- cmd/dataset.go | 14 +- cmd/dataset_show.go | 2 +- cmd/login.go | 197 ++++++++++++--- cmd/login_test.go | 335 +++++++++++++++++++++++-- cmd/logout.go | 7 +- cmd/repl_engine_test.go | 3 +- cmd/reports.go | 23 +- cmd/reports_test.go | 43 ++++ cmd/uninstall.go | 5 +- docs/researcher-guide.md | 40 ++- internal/api/portal.go | 3 +- internal/api/portal_test.go | 6 +- internal/auth/integration_test.go | 19 +- internal/auth/login.go | 25 +- internal/auth/logout.go | 3 +- internal/auth/pkce_test.go | 2 +- internal/auth/portal.go | 57 +++++ internal/auth/portal_test.go | 97 ++++++++ internal/auth/status.go | 7 +- internal/claude/skill/SKILL.md | 13 +- internal/config/config.go | 37 ++- internal/config/config_test.go | 128 ++++++++++ internal/config/portal.go | 378 +++++++++++++++++++++++++--- internal/config/portal_test.go | 398 +++++++++++++++++++++++++++--- internal/creds/creds.go | 14 +- internal/creds/creds_test.go | 18 +- internal/dataset/dataset.go | 5 +- internal/dataset/dataset_test.go | 20 +- internal/dataset/merge_test.go | 3 +- internal/dataset/ref.go | 104 ++++++-- internal/dataset/ref_test.go | 141 ++++++++++- internal/dataset/summary.go | 2 +- internal/dataset/summary_test.go | 6 +- internal/duck/engine_test.go | 15 +- internal/fetch/report_test.go | 3 +- internal/mcpserver/server.go | 24 +- internal/mcpserver/server_test.go | 5 +- internal/mcpserver/tools.go | 20 +- 42 files changed, 2057 insertions(+), 259 deletions(-) create mode 100644 cmd/auth_test.go create mode 100644 cmd/reports_test.go create mode 100644 internal/auth/portal.go create mode 100644 internal/auth/portal_test.go 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..88d537f --- /dev/null +++ b/cmd/auth_test.go @@ -0,0 +1,46 @@ +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 "-" rather than blank. + if !strings.Contains(lines[2], " - ") { + t.Errorf("check=%v: a serverless credential should show -: %q", check, lines[2]) + } + } +} diff --git a/cmd/common.go b/cmd/common.go index b4f606e..a8676ab 100644 --- a/cmd/common.go +++ b/cmd/common.go @@ -26,7 +26,18 @@ func loadRuntime() (*config.Config, string, error) { // resolveRef parses a dataset ref against the configured default portal. 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 the commands that inspect or remove a +// dataset already on disk, which must be able to reach anything dataset list +// shows, including a folder under a portal the parser would otherwise refuse. +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..285ce40 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 { @@ -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_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/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/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..e94e1a7 --- /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 +// config.ResolvePortal: the default_portal fallback layered on top of it. The +// accepted portal spellings themselves are config.TestResolvePortal'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..7b0b209 --- /dev/null +++ b/internal/auth/portal.go @@ -0,0 +1,57 @@ +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 host 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. +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..66d39c7 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 diff --git a/internal/config/config.go b/internal/config/config.go index 4fd4b84..8644a0b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -78,9 +78,40 @@ 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: this refusal fails every command, including the ones + // that never read default_portal, so the error has to say what to edit. + return nil, fmt.Errorf("%s: %w", 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 +126,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,8 +183,7 @@ 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)) if isLoopback(host) { return u.Scheme + "://" + u.Host, nil diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 37e9a49..45dcbf2 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,6 +20,12 @@ 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, ""}, @@ -101,6 +109,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..04c572a 100644 --- a/internal/config/portal.go +++ b/internal/config/portal.go @@ -4,66 +4,384 @@ 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, +// 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 for dev portals. -func NormalizePortal(portal string) (string, error) { - p := strings.TrimSpace(portal) +func parseHost(v string) (Portal, error) { + p := strings.TrimSpace(v) if p == "" { - return "", fmt.Errorf("portal is empty") + return Portal{}, fmt.Errorf("portal is empty") } if !strings.Contains(p, "://") { p = "https://" + p } u, err := url.Parse(p) if err != nil { - return "", fmt.Errorf("invalid portal %q: %w", portal, err) + return Portal{}, 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 Portal{}, fmt.Errorf("invalid portal %q: no host", v) + } + if err := checkHostSyntax(host); err != nil { + return Portal{}, fmt.Errorf("invalid portal %q: %w", v, err) + } + return Portal{host: host}, nil +} + +// hostSyntax is a hostname or dotted IPv4 literal: it must start and end +// alphanumeric, so "." and ".." and anything leading with a separator are out. +// Underscore is excluded on purpose: Portal.Folder encodes the port separator as +// "_", so a host containing one would decode back 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) } - return host, nil + 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, an IP literal, and loopback, so only +// the spellings that cannot be a real portal are turned away. +func checkPortalShape(p Portal) error { + h := splitHostPort(p.host) + if strings.Contains(h, ".") || 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 } -// 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 +// 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, } -// 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, ":", "_") +// 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 } -// FirebaseSource maps a portal host to its firebase project. -func FirebaseSource(host string) string { - if host == ProductionPortal { - return firebaseProd +// 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 } -// splitHostPort returns the host without its port, and whether a port was present. -func splitHostPort(host string) (string, bool) { +func hasHTTPSScheme(v string) bool { + return strings.HasPrefix(strings.ToLower(strings.TrimSpace(v)), "https://") +} + +// 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..605157c 100644 --- a/internal/config/portal_test.go +++ b/internal/config/portal_test.go @@ -1,60 +1,390 @@ 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) + } + }) + } +} + +// 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) } - if got != c.want { - t.Fatalf("NormalizePortal(%q) = %q, want %q", c.in, got, c.want) + } + + for _, name := range []string{"", "learn.concord.org", "stagin"} { + if _, ok := LookupEnvironment(name); ok { + t.Errorf("LookupEnvironment(%q) should not match an alias", name) } } } -func TestNormalizePortalEmpty(t *testing.T) { - if _, err := NormalizePortal(" "); err == nil { - t.Fatal("empty portal should error") +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) + } } } -func TestPortalFolderEncoding(t *testing.T) { - if got := PortalFolder("localhost:8080"); got != "localhost_8080" { - t.Fatalf("PortalFolder(localhost:8080) = %q, want localhost_8080", got) +// 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 } - if got := PortalFolder("learn.concord.org"); got != "learn.concord.org" { - t.Fatalf("PortalFolder(learn.concord.org) = %q, want unchanged", got) + for name, env := range environments { + if !named[env] { + t.Errorf("environment %+v (spelled %q) is in no EnvironmentNames entry", env, name) + } + if _, ok := environmentsByPortal[env.Portal]; !ok { + t.Errorf("environment %+v (spelled %q) has no entry in environmentsByPortal", env, name) + } } } -func TestPortalOrigin(t *testing.T) { - if got := PortalOrigin("localhost:8080"); got != "https://localhost:8080" { - t.Fatalf("PortalOrigin = %q", got) +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 TestFirebaseSource(t *testing.T) { - if FirebaseSource("learn.concord.org") != "report-service-pro" { - t.Fatal("production portal should map to report-service-pro") +// 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 FirebaseSource("learn.portal.staging.concord.org") != "report-service-dev" { - t.Fatal("non-production portal should map to report-service-dev") + _, err := buildEnvironmentsByPortal(colliding) + if err == nil { + t.Fatal("two environments claiming one portal should be rejected") } - if FirebaseSource("localhost:8080") != "report-service-dev" { - t.Fatal("dev portal should map to report-service-dev") + 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 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 TestExpandServerAlias(t *testing.T) { + if got := ExpandServerAlias("staging"); got != StagingServer { + t.Errorf("ExpandServerAlias(staging) = %q", got) + } + // Portal and server aliases are independent: each expands only its own side. + if got := ExpandServerAlias("dev"); got != DevServer { + t.Errorf("ExpandServerAlias(dev) = %q", got) + } + // 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..8db7b57 100644 --- a/internal/dataset/ref.go +++ b/internal/dataset/ref.go @@ -3,6 +3,7 @@ package dataset import ( + "errors" "fmt" "path/filepath" "regexp" @@ -25,19 +26,85 @@ 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 a command that inspects or removes a +// dataset already on disk, making one exception ParseRefForConfig does not: a +// portal refused only for naming an environment 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 written before the alias refusal existed would be +// visible and untouchable, removable only with rm -rf. The exception is +// deliberately narrow: only *config.AliasPortalError is salvageable, never a +// syntax refusal, so a traversal cannot reach os.RemoveAll by claiming the +// folder exists. +func ParseRefForExisting(cfg *config.Config, dataRoot, raw string) (Ref, error) { + ref, err := ParseRefForConfig(cfg, raw) + if err == nil { + return ref, nil + } + var alias *config.AliasPortalError + if !errors.As(err, &alias) { + return Ref{}, err + } + defaultPortal, derr := cfg.DefaultPortalValue() + if derr != nil { + return Ref{}, err + } + _, name, serr := splitRef(raw, defaultPortal) + if serr != nil { + return Ref{}, err + } + candidate, cerr := buildRef(alias.Portal, name, raw) + if cerr != nil { + return Ref{}, err + } + if !Open(dataRoot, candidate).Exists() { + return Ref{}, err + } + return candidate, 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,38 +112,39 @@ 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 - } - host, err := config.NormalizePortal(scheme + portalPart) - if err != nil { - return Ref{}, err + portalPart = defaultPortal.Host() } + return scheme + portalPart, name, nil +} + +// buildRef validates the name half and assembles the ref. +func buildRef(portal config.Portal, name, raw string) (Ref, error) { if name == "" { return Ref{}, fmt.Errorf("dataset ref %q has no name", raw) } 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..c7455cb 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,12 @@ 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}, + {"..%2fwildfire", "", "", "", 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 +47,54 @@ 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 above +// protect: Ref.Dir feeds MkdirAll and, via dataset delete, os.RemoveAll. +func TestParseRefStaysInsideTheDataRoot(t *testing.T) { + root := filepath.Join("/data", "root") + for _, raw := range []string{"../wildfire", "../../wildfire", "./wildfire", "..%2f..%2fwildfire"} { + ref, err := ParseRef(raw, config.Portal{}) + if err != nil { + continue // refused outright, which is the preferred outcome + } + 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) + } + } +} + 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 +119,87 @@ 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 the inspect +// and remove commands make. dataset list builds its rows from folder names and +// never parses them, so a folder written under a portal the parser now refuses +// 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 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") + } + // 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..7c93611 100644 --- a/internal/mcpserver/server.go +++ b/internal/mcpserver/server.go @@ -76,18 +76,32 @@ func loadRuntime() (*config.Config, string, error) { return cfg, root, nil } -func openDataset(refStr string) (*dataset.Dataset, string, error) { +// openExistingDataset is openDataset for the tools that inspect or remove a +// dataset already on disk. dataset_list builds its rows from folder names, so +// these have to reach anything it shows, including a folder under a portal the +// parser would otherwise refuse. +func openExistingDataset(refStr string) (*dataset.Dataset, config.Portal, error) { + return openDatasetWith(refStr, dataset.ParseRefForExisting) +} + +func openDataset(refStr string) (*dataset.Dataset, config.Portal, error) { + return openDatasetWith(refStr, func(cfg *config.Config, _ string, raw string) (dataset.Ref, error) { + return dataset.ParseRefForConfig(cfg, raw) + }) +} + +func openDatasetWith(refStr string, parse func(*config.Config, string, string) (dataset.Ref, error)) (*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 := parse(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..967689c 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 } @@ -123,7 +122,7 @@ func registerTools(s *mcp.Server, opts Options) { mcp.AddTool(s, &mcp.Tool{Name: "dataset_show", Description: "Show a dataset's holdings and warnings.", Annotations: readOnly}, func(ctx context.Context, req *mcp.CallToolRequest, in datasetShowIn) (*mcp.CallToolResult, dataset.ShowJSON, error) { - d, _, err := openDataset(in.Ref) + d, _, err := openExistingDataset(in.Ref) if err != nil { return nil, dataset.ShowJSON{}, err } @@ -169,7 +168,7 @@ func registerTools(s *mcp.Server, opts Options) { if !in.Confirm { return nil, nil, fmt.Errorf("dataset_delete requires confirm:true") } - d, _, err := openDataset(in.Ref) + d, _, err := openExistingDataset(in.Ref) if err != nil { return nil, nil, err } @@ -184,7 +183,7 @@ func registerTools(s *mcp.Server, opts Options) { if !in.Confirm { return nil, nil, fmt.Errorf("dataset_purge requires confirm:true") } - d, _, err := openDataset(in.Ref) + d, _, err := openExistingDataset(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.ParseRefForConfig(cfg, refStr) if perr != nil { return nil, queryOut{}, perr } From ba6f72c3836eefdae93d6cba678a2b764b4a420d Mon Sep 17 00:00:00 2001 From: Doug Martin Date: Fri, 24 Jul 2026 14:48:15 -0400 Subject: [PATCH 2/3] refactor: return the real error from ParseRefForExisting [REPORT-87] Address Copilot review: the alias-salvage path collapsed every failure to the original alias error, masking a more specific one. Restructure so each error path returns its own error: an invalid dataset name reports as an invalid name (matching what a hostname portal gives for the same ref), a syntax refusal stands as the traversal guard, and the alias refusal is kept only for the "no such folder" case, where naming the hostname is the actual fix. The two branches that re-derived the default portal and re-split the ref were dead (both succeed before an AliasPortalError can arise) and are gone. Also fix a stale test comment naming config.ResolvePortal / config.TestResolvePortal, both removed in this branch; the resolver is auth.ResolvePortalTarget and the spellings are pinned by config.TestPortalMatrix. --- cmd/reports_test.go | 8 ++++---- internal/dataset/ref.go | 37 ++++++++++++++++++++---------------- internal/dataset/ref_test.go | 10 ++++++++++ 3 files changed, 35 insertions(+), 20 deletions(-) diff --git a/cmd/reports_test.go b/cmd/reports_test.go index e94e1a7..f17ae80 100644 --- a/cmd/reports_test.go +++ b/cmd/reports_test.go @@ -7,10 +7,10 @@ import ( ) // TestResolvePortal covers what reports list and reports jobs add over -// config.ResolvePortal: the default_portal fallback layered on top of it. The -// accepted portal spellings themselves are config.TestResolvePortal'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 +// 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 { diff --git a/internal/dataset/ref.go b/internal/dataset/ref.go index 8db7b57..f98d3a6 100644 --- a/internal/dataset/ref.go +++ b/internal/dataset/ref.go @@ -55,30 +55,35 @@ func ParseRefForConfig(cfg *config.Config, raw string) (Ref, error) { // syntax refusal, so a traversal cannot reach os.RemoveAll by claiming the // folder exists. func ParseRefForExisting(cfg *config.Config, dataRoot, raw string) (Ref, error) { - ref, err := ParseRefForConfig(cfg, raw) - if err == nil { - return ref, nil - } - var alias *config.AliasPortalError - if !errors.As(err, &alias) { + defaultPortal, err := cfg.DefaultPortalValue() + if err != nil { return Ref{}, err } - defaultPortal, derr := cfg.DefaultPortalValue() - if derr != nil { + portalValue, name, err := splitRef(raw, defaultPortal) + if err != nil { return Ref{}, err } - _, name, serr := splitRef(raw, defaultPortal) - if serr != nil { - return Ref{}, err + portal, portalErr := config.ParsePortalIdentity(portalValue) + if portalErr == nil { + return buildRef(portal, name, raw) } - candidate, cerr := buildRef(alias.Portal, name, raw) - if cerr != nil { - return Ref{}, err + // Only an alias refusal is salvageable; a syntax refusal is the traversal + // guard and must stand. + var alias *config.AliasPortalError + if !errors.As(portalErr, &alias) { + return Ref{}, portalErr } - if !Open(dataRoot, candidate).Exists() { + // Validate the name before consulting the disk, so an invalid name still + // reports as one rather than as the alias refusal. If the name is fine but no + // folder exists, the alias refusal stands: naming the hostname is the fix. + ref, err := buildRef(alias.Portal, name, raw) + if err != nil { return Ref{}, err } - return candidate, nil + if !Open(dataRoot, ref).Exists() { + return Ref{}, portalErr + } + return ref, nil } // ParseRef resolves "/" or a bare "" (under defaultPortal). diff --git a/internal/dataset/ref_test.go b/internal/dataset/ref_test.go index c7455cb..6f4bc46 100644 --- a/internal/dataset/ref_test.go +++ b/internal/dataset/ref_test.go @@ -160,6 +160,16 @@ func TestParseRefForExistingReachesRefusedFolder(t *testing.T) { if _, err := ParseRefForExisting(cfg, root, "staging/absent"); err == nil { t.Error("an alias portal 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 { From 322159e8001791ace81f8798c080b7005ad7d2d6 Mon Sep 17 00:00:00 2001 From: Doug Martin Date: Sun, 26 Jul 2026 21:35:16 -0400 Subject: [PATCH 3/3] fix: address PR review (emcelroy) on portal handling [REPORT-87] Findings from the branch review: 1. (Medium) Legacy folders under underscore/IPv6 hosts were listed but unreachable. ParseRefForExisting now salvages any portal refusal, not just the alias one, via config.AdoptExistingPortal, which accepts a value the strict parser rejects (alias, underscore, IPv6 literal) only when it is a single safe path component and a folder exists under it. Traversal refusals still stand. 2. (Medium-Low) The "already exists" exception now covers every command that names an existing dataset, not just show/delete/purge: rename, edit, reindex, get, and query (CLI and MCP) route through the salvaging parser. Only create stays strict. This removes the asymmetry rather than documenting around it. 3. (Low) Scoped ResolvePortalTarget's doc comment: its exception covers an UnshapedHostError host with a stored credential, not a credential stored under a literal alias name. Preferring the literal there would misroute a fresh "login staging" (which must expand), so that gap is left to "cc-data uninstall", which the comment now names. 5. (Low) ValidateServerURL lowercases the host in the returned origin, so a mixed-case server_url canonicalizes to the same origin the pairing produces: no more spurious "server_url was not used" notice, and the stored SERVER column renders evenly. 6. (Low) A bad default_portal error now names the remedy ("remove or fix default_portal to continue") in addition to the file. 7. (Tests) TestParseRefStaysInsideTheDataRoot now runs its containment assertion on accepted refs (it previously bailed on all inputs); the TestParseRef traversal row exercises name validation with a default portal set. Nits: dropped a dead colon check in checkPortalShape; buildRef rejects a zero portal (the single Ref choke point); softened the hostSyntax comment and fixed parseHost's ("port preserved" for any host, not just dev); pinned the auth status test to the SERVER column; reconciled SKILL.md (never read manifest.json directly vs. "may auto-read ... manifest"). Not changed: #4 (server_url precedence) is a release-note item with no changelog in-repo; #8 DefaultPortalValue caching left as a cheap re-parse to avoid a staleness footgun. --- cmd/auth_test.go | 8 ++-- cmd/common.go | 10 +++-- cmd/dataset.go | 4 +- cmd/dataset_reindex.go | 2 +- cmd/get.go | 2 +- cmd/query.go | 2 +- internal/auth/portal.go | 15 +++++++- internal/claude/skill/SKILL.md | 2 +- internal/config/config.go | 17 ++++++--- internal/config/config_test.go | 8 ++++ internal/config/portal.go | 67 ++++++++++++++++++++++++++-------- internal/config/portal_test.go | 34 +++++++++++++++++ internal/dataset/ref.go | 41 ++++++++++++--------- internal/dataset/ref_test.go | 66 ++++++++++++++++++++++++++++----- internal/mcpserver/server.go | 21 +++-------- internal/mcpserver/tools.go | 8 ++-- 16 files changed, 227 insertions(+), 80 deletions(-) diff --git a/cmd/auth_test.go b/cmd/auth_test.go index 88d537f..be53af3 100644 --- a/cmd/auth_test.go +++ b/cmd/auth_test.go @@ -38,9 +38,11 @@ func TestRenderStatusShowsServer(t *testing.T) { 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 "-" rather than blank. - if !strings.Contains(lines[2], " - ") { - t.Errorf("check=%v: a serverless credential should show -: %q", check, lines[2]) + // 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 a8676ab..f61c9f1 100644 --- a/cmd/common.go +++ b/cmd/common.go @@ -24,7 +24,8 @@ 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.ParseRefForConfig(cfg, raw) if err != nil { @@ -33,9 +34,10 @@ func resolveRef(cfg *config.Config, raw string) (dataset.Ref, error) { return ref, nil } -// resolveExistingRef is resolveRef for the commands that inspect or remove a -// dataset already on disk, which must be able to reach anything dataset list -// shows, including a folder under a portal the parser would otherwise refuse. +// 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 { diff --git a/cmd/dataset.go b/cmd/dataset.go index 285ce40..6f2eeee 100644 --- a/cmd/dataset.go +++ b/cmd/dataset.go @@ -82,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 } @@ -116,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 } 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/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/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/internal/auth/portal.go b/internal/auth/portal.go index 7b0b209..97990b5 100644 --- a/internal/auth/portal.go +++ b/internal/auth/portal.go @@ -11,14 +11,25 @@ import ( // 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 host the shape check turns away is still accepted -// when a credential is actually stored under it. +// 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 { diff --git a/internal/claude/skill/SKILL.md b/internal/claude/skill/SKILL.md index 66d39c7..3a45503 100644 --- a/internal/claude/skill/SKILL.md +++ b/internal/claude/skill/SKILL.md @@ -127,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 8644a0b..856ad6a 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -79,9 +79,10 @@ func Load() (*Config, error) { c.ServerURL = origin } if err := c.normalizeDefaultPortal(); err != nil { - // Name the file: this refusal fails every command, including the ones - // that never read default_portal, so the error has to say what to edit. - return nil, fmt.Errorf("%s: %w", path, err) + // 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 } @@ -184,9 +185,15 @@ func ValidateServerURL(raw string) (string, error) { return "", fmt.Errorf("server URL %q has no host", raw) } 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) @@ -194,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 45dcbf2..d781dff 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -32,6 +32,14 @@ func TestValidateServerURL(t *testing.T) { {"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) diff --git a/internal/config/portal.go b/internal/config/portal.go index 04c572a..b23cdf7 100644 --- a/internal/config/portal.go +++ b/internal/config/portal.go @@ -152,34 +152,69 @@ func MustPortal(host string) Portal { return p } -// parseHost reduces a portal value to hostname form: scheme stripped, -// lowercased, trailing slash removed, port preserved for dev portals. +// 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 Portal{}, fmt.Errorf("portal is empty") + return "", fmt.Errorf("portal is empty") } if !strings.Contains(p, "://") { p = "https://" + p } u, err := url.Parse(p) if err != nil { - return Portal{}, fmt.Errorf("invalid portal %q: %w", v, err) + return "", fmt.Errorf("invalid portal %q: %w", v, err) } host := strings.ToLower(u.Host) if host == "" { - return Portal{}, fmt.Errorf("invalid portal %q: no host", v) + return "", fmt.Errorf("invalid portal %q: no host", v) } - if err := checkHostSyntax(host); err != nil { - return Portal{}, fmt.Errorf("invalid portal %q: %w", v, err) + return host, nil +} + +// 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 } - return Portal{host: host}, nil + p := Portal{host: host} + folder := p.Folder() + if folder == "" || folder == "." || folder == ".." || strings.ContainsAny(folder, `/\`) { + return Portal{}, false + } + return p, true } -// hostSyntax is a hostname or dotted IPv4 literal: it must start and end -// alphanumeric, so "." and ".." and anything leading with a separator are out. -// Underscore is excluded on purpose: Portal.Folder encodes the port separator as -// "_", so a host containing one would decode back to a different 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 @@ -231,11 +266,13 @@ func (e *UnshapedHostError) Error() string { e.Portal.Host(), strings.Join(environmentNames, ", ")) } -// checkPortalShape passes a dotted name, an IP literal, and loopback, so only -// the spellings that cannot be a real portal are turned away. +// 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, ".") || strings.Contains(h, ":") || isLoopback(h) { + if strings.Contains(h, ".") || isLoopback(h) { return nil } return &UnshapedHostError{Portal: p} diff --git a/internal/config/portal_test.go b/internal/config/portal_test.go index 605157c..bcb5bfb 100644 --- a/internal/config/portal_test.go +++ b/internal/config/portal_test.go @@ -163,6 +163,40 @@ func TestPortalFolderStaysOneComponent(t *testing.T) { } } +// 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 diff --git a/internal/dataset/ref.go b/internal/dataset/ref.go index f98d3a6..02e5e83 100644 --- a/internal/dataset/ref.go +++ b/internal/dataset/ref.go @@ -3,7 +3,6 @@ package dataset import ( - "errors" "fmt" "path/filepath" "regexp" @@ -43,17 +42,20 @@ func ParseRefForConfig(cfg *config.Config, raw string) (Ref, error) { return ParseRef(raw, defaultPortal) } -// ParseRefForExisting resolves a ref for a command that inspects or removes a -// dataset already on disk, making one exception ParseRefForConfig does not: a -// portal refused only for naming an environment is accepted when a dataset +// 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 written before the alias refusal existed would be -// visible and untouchable, removable only with rm -rf. The exception is -// deliberately narrow: only *config.AliasPortalError is salvageable, never a -// syntax refusal, so a traversal cannot reach os.RemoveAll by claiming the -// folder exists. +// 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 { @@ -67,16 +69,14 @@ func ParseRefForExisting(cfg *config.Config, dataRoot, raw string) (Ref, error) if portalErr == nil { return buildRef(portal, name, raw) } - // Only an alias refusal is salvageable; a syntax refusal is the traversal - // guard and must stand. - var alias *config.AliasPortalError - if !errors.As(portalErr, &alias) { + 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 alias refusal. If the name is fine but no - // folder exists, the alias refusal stands: naming the hostname is the fix. - ref, err := buildRef(alias.Portal, name, raw) + // 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 } @@ -130,8 +130,15 @@ func splitRef(raw string, defaultPortal config.Portal) (portalValue, name string return scheme + portalPart, name, nil } -// buildRef validates the name half and assembles the ref. +// 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) } diff --git a/internal/dataset/ref_test.go b/internal/dataset/ref_test.go index 6f4bc46..19c6df7 100644 --- a/internal/dataset/ref_test.go +++ b/internal/dataset/ref_test.go @@ -34,7 +34,10 @@ func TestParseRef(t *testing.T) { {"portal/", "", "", "", true}, // empty name // The portal half can no longer carry a traversal into Ref.Dir. {"../wildfire", "", "", "", true}, - {"..%2fwildfire", "", "", "", 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, portal(c.def)) @@ -79,20 +82,40 @@ func TestParseRefRejectsEnvironmentAliases(t *testing.T) { } } -// TestParseRefStaysInsideTheDataRoot is the property the traversal rows above -// protect: Ref.Dir feeds MkdirAll and, via dataset delete, os.RemoveAll. +// 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") - for _, raw := range []string{"../wildfire", "../../wildfire", "./wildfire", "..%2f..%2fwildfire"} { - ref, err := ParseRef(raw, config.Portal{}) + 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 { - continue // refused outright, which is the preferred outcome + 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) { @@ -126,9 +149,10 @@ func TestRefDirEncoding(t *testing.T) { } } -// TestParseRefForExistingReachesRefusedFolder covers the exception the inspect -// and remove commands make. dataset list builds its rows from folder names and -// never parses them, so a folder written under a portal the parser now refuses +// 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() @@ -156,10 +180,34 @@ func TestParseRefForExistingReachesRefusedFolder(t *testing.T) { 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. diff --git a/internal/mcpserver/server.go b/internal/mcpserver/server.go index 7c93611..a2a4c59 100644 --- a/internal/mcpserver/server.go +++ b/internal/mcpserver/server.go @@ -76,26 +76,17 @@ func loadRuntime() (*config.Config, string, error) { return cfg, root, nil } -// openExistingDataset is openDataset for the tools that inspect or remove a -// dataset already on disk. dataset_list builds its rows from folder names, so -// these have to reach anything it shows, including a folder under a portal the -// parser would otherwise refuse. -func openExistingDataset(refStr string) (*dataset.Dataset, config.Portal, error) { - return openDatasetWith(refStr, dataset.ParseRefForExisting) -} - +// 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) { - return openDatasetWith(refStr, func(cfg *config.Config, _ string, raw string) (dataset.Ref, error) { - return dataset.ParseRefForConfig(cfg, raw) - }) -} - -func openDatasetWith(refStr string, parse func(*config.Config, string, string) (dataset.Ref, error)) (*dataset.Dataset, config.Portal, error) { cfg, root, err := loadRuntime() if err != nil { return nil, config.Portal{}, err } - ref, err := parse(cfg, root, refStr) + ref, err := dataset.ParseRefForExisting(cfg, root, refStr) if err != nil { return nil, config.Portal{}, err } diff --git a/internal/mcpserver/tools.go b/internal/mcpserver/tools.go index 967689c..49c424b 100644 --- a/internal/mcpserver/tools.go +++ b/internal/mcpserver/tools.go @@ -122,7 +122,7 @@ func registerTools(s *mcp.Server, opts Options) { mcp.AddTool(s, &mcp.Tool{Name: "dataset_show", Description: "Show a dataset's holdings and warnings.", Annotations: readOnly}, func(ctx context.Context, req *mcp.CallToolRequest, in datasetShowIn) (*mcp.CallToolResult, dataset.ShowJSON, error) { - d, _, err := openExistingDataset(in.Ref) + d, _, err := openDataset(in.Ref) if err != nil { return nil, dataset.ShowJSON{}, err } @@ -168,7 +168,7 @@ func registerTools(s *mcp.Server, opts Options) { if !in.Confirm { return nil, nil, fmt.Errorf("dataset_delete requires confirm:true") } - d, _, err := openExistingDataset(in.Ref) + d, _, err := openDataset(in.Ref) if err != nil { return nil, nil, err } @@ -183,7 +183,7 @@ func registerTools(s *mcp.Server, opts Options) { if !in.Confirm { return nil, nil, fmt.Errorf("dataset_purge requires confirm:true") } - d, _, err := openExistingDataset(in.Ref) + d, _, err := openDataset(in.Ref) if err != nil { return nil, nil, err } @@ -261,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.ParseRefForConfig(cfg, refStr) + ref, perr := dataset.ParseRefForExisting(cfg, root, refStr) if perr != nil { return nil, queryOut{}, perr }