From 77630da5263186abd76e7981789455622f299212 Mon Sep 17 00:00:00 2001 From: unknown Date: Sun, 18 Oct 2026 15:29:50 +0530 Subject: [PATCH] fix: ignore trailing # comments in tokens/clients/groups files Only whole-line comments were skipped, so `tok acme rw # ci runner` scoped the token to org "runner" and `prompt-admins rw # platform team` issued tokens with org "team". A field starting with # now ends the line, and a second org (or second expiry) is rejected instead of the last one silently winning. Fixes #2 Co-Authored-By: Claude Opus 5.5 --- authn/tokens.go | 12 +++++++++- authn/tokens_test.go | 42 +++++++++++++++++++++++++++++++++++ cmd/priompt-auth/main.go | 32 ++++++++++++++++++++------ cmd/priompt-auth/main_test.go | 30 +++++++++++++++++++++++++ 4 files changed, 108 insertions(+), 8 deletions(-) diff --git a/authn/tokens.go b/authn/tokens.go index 8c52c75..01846c8 100644 --- a/authn/tokens.go +++ b/authn/tokens.go @@ -30,7 +30,8 @@ func GenToken() (string, error) { // or RFC3339 timestamp; the keyword `rw` grants write (default is read-only, // `ro` is the explicit no-op). Fields after the token are order-independent: // `rw`/`ro` is recognized by keyword, a date/timestamp as the expiry, anything -// else as the org. Blank lines and # comments are ignored. An empty file name +// else as the org; a second org or expiry is an error. Blank lines, # comment +// lines, and a trailing `# …` after the token are ignored. An empty file name // with no PRIOMPT_TOKEN yields an empty map = auth disabled. To rotate: add the // new token, give the old one a near-future expiry, drop it once it lapses. func LoadTokens(file string) (map[string]Token, error) { @@ -53,18 +54,27 @@ func LoadTokens(file string) (map[string]Token, error) { fields := strings.Fields(line) tok := Token{} for _, f := range fields[1:] { + if strings.HasPrefix(f, "#") { + break // trailing comment + } switch { case f == "rw": tok.Write = true case f == "ro": // explicit read-only (the default); no-op case isExpiry(f): + if !tok.Expires.IsZero() { + return nil, fmt.Errorf("token line has two expiries (%q); a trailing comment needs a leading #", f) + } exp, err := parseExpiry(f) if err != nil { return nil, err } tok.Expires = exp default: + if tok.Org != "" { + return nil, fmt.Errorf("token line has two orgs (%q, %q); a trailing comment needs a leading #", tok.Org, f) + } tok.Org = f } } diff --git a/authn/tokens_test.go b/authn/tokens_test.go index 6f23c7f..b8056ad 100644 --- a/authn/tokens_test.go +++ b/authn/tokens_test.go @@ -62,3 +62,45 @@ func TestLoadTokensMissingFile(t *testing.T) { t.Errorf("no file and no env token = auth disabled, got %v err=%v", tokens, err) } } + +// A trailing # comment is not part of the line: `tok acme rw # ci runner` must +// stay scoped to acme, not be re-scoped to org "runner" (issue #2). +func TestLoadTokensTrailingComment(t *testing.T) { + t.Setenv("PRIOMPT_TOKEN", "") + file := filepath.Join(t.TempDir(), "tokens.txt") + content := "scoped acme rw # ci runner\nadmin # break-glass, all orgs\nhash acme #nospace\n" + if err := os.WriteFile(file, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + tokens, err := LoadTokens(file) + if err != nil { + t.Fatal(err) + } + if tok := tokens["scoped"]; tok.Org != "acme" || !tok.Write { + t.Errorf("trailing comment changed scope: want org=acme rw, got %+v", tok) + } + if tok := tokens["admin"]; tok.Org != "" || tok.Write { + t.Errorf("comment-only tail: want admin read-only, got %+v", tok) + } + if tok := tokens["hash"]; tok.Org != "acme" { + t.Errorf("#comment without a space: want org=acme, got %+v", tok) + } +} + +// Two org-looking fields (or two expiries) are ambiguous: reject the line +// instead of letting the last one silently win. +func TestLoadTokensRejectsExtraFields(t *testing.T) { + t.Setenv("PRIOMPT_TOKEN", "") + for _, line := range []string{ + "tok acme rw ci runner", + "tok acme 2026-01-01 2027-01-01", + } { + file := filepath.Join(t.TempDir(), "tokens.txt") + if err := os.WriteFile(file, []byte(line+"\n"), 0o600); err != nil { + t.Fatal(err) + } + if tokens, err := LoadTokens(file); err == nil { + t.Errorf("%q: want an error, got %+v", line, tokens) + } + } +} diff --git a/cmd/priompt-auth/main.go b/cmd/priompt-auth/main.go index 774bca3..07ce2fe 100644 --- a/cmd/priompt-auth/main.go +++ b/cmd/priompt-auth/main.go @@ -279,9 +279,11 @@ func loadClients(file string) map[string]client { if len(fields) < 2 || !strings.HasPrefix(fields[1], "sha256:") { log.Fatalf("%s: want `client_id sha256: [org] [rw]`, got %q", file, strings.Join(fields, " ")) } - c := client{secretHash: strings.TrimPrefix(fields[1], "sha256:")} - c.grant = parseGrant(fields[2:]) - out[fields[0]] = c + g, err := parseGrant(fields[2:]) + if err != nil { + log.Fatalf("%s: client %s: %v", file, fields[0], err) + } + out[fields[0]] = client{secretHash: strings.TrimPrefix(fields[1], "sha256:"), grant: g} } return out } @@ -290,12 +292,18 @@ func loadClients(file string) map[string]client { func loadGroups(file string) []groupRule { var out []groupRule for _, fields := range configLines(file) { - out = append(out, groupRule{group: fields[0], grant: parseGrant(fields[1:])}) + g, err := parseGrant(fields[1:]) + if err != nil { + log.Fatalf("%s: group %s: %v", file, fields[0], err) + } + out = append(out, groupRule{group: fields[0], grant: g}) } return out } -func parseGrant(fields []string) grant { +// parseGrant reads `[org] [rw|ro]`; a second org-looking field is an error +// rather than a silent last-wins. +func parseGrant(fields []string) (grant, error) { var g grant for _, f := range fields { switch f { @@ -303,10 +311,13 @@ func parseGrant(fields []string) grant { g.rw = true case "ro": // explicit read-only (the default); no-op default: + if g.org != "" { + return grant{}, fmt.Errorf("two orgs (%q, %q); a trailing comment needs a leading #", g.org, f) + } g.org = f } } - return g + return g, nil } func configLines(file string) [][]string { @@ -320,7 +331,14 @@ func configLines(file string) [][]string { if line == "" || strings.HasPrefix(line, "#") { continue } - out = append(out, strings.Fields(line)) + fields := strings.Fields(line) + for i, f := range fields { + if strings.HasPrefix(f, "#") { // trailing comment + fields = fields[:i] + break + } + } + out = append(out, fields) } return out } diff --git a/cmd/priompt-auth/main_test.go b/cmd/priompt-auth/main_test.go index 68b6015..337fe4d 100644 --- a/cmd/priompt-auth/main_test.go +++ b/cmd/priompt-auth/main_test.go @@ -15,6 +15,8 @@ import ( "net/http" "net/http/httptest" "net/url" + "os" + "path/filepath" "strings" "testing" "time" @@ -192,3 +194,31 @@ func TestOIDCExchange(t *testing.T) { t.Errorf("expired ID token must 401, got %d", code) } } + +// Trailing # comments in clients.txt / groups.txt are not fields: the README's +// own `prompt-admins rw # platform team` must stay an admin grant (issue #2). +func TestConfigTrailingComments(t *testing.T) { + dir := t.TempDir() + groups := filepath.Join(dir, "groups.txt") + os.WriteFile(groups, []byte("# header\nprompt-admins rw # platform team\nacme-readers acme # read-only\n"), 0o600) + g := loadGroups(groups) + if len(g) != 2 || g[0].grant != (grant{rw: true}) || g[1].grant != (grant{org: "acme"}) { + t.Errorf("trailing comments parsed as fields: %+v", g) + } + + clients := filepath.Join(dir, "clients.txt") + os.WriteFile(clients, []byte("ci-bot sha256:abcd acme rw # ci runner\n"), 0o600) + if c := loadClients(clients)["ci-bot"]; c.grant != (grant{org: "acme", rw: true}) || c.secretHash != "abcd" { + t.Errorf("trailing comment changed the client grant: %+v", c) + } +} + +// Two org-looking fields are ambiguous; reject instead of last-wins. +func TestParseGrantRejectsExtraFields(t *testing.T) { + if g, err := parseGrant([]string{"acme", "rw", "platform", "team"}); err == nil { + t.Errorf("want an error for two orgs, got %+v", g) + } + if g, err := parseGrant([]string{"acme", "rw"}); err != nil || g != (grant{org: "acme", rw: true}) { + t.Errorf("want acme rw, got %+v err=%v", g, err) + } +}