Skip to content

fix(#1129): allow consecutive hyphens in GitHub org names #6954

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shairevivo wants to merge 1 commit into from
Closed

fix(#1129): allow consecutive hyphens in GitHub org names #6954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions internal/cli/admin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,6 @@ func validateOrgName(org string) error {
if strings.HasPrefix(org, "-") || strings.HasSuffix(org, "-") {
return fmt.Errorf("organization name cannot start or end with a hyphen")
}
if strings.Contains(org, "--") {
return fmt.Errorf("organization name cannot contain consecutive hyphens")
}
for _, c := range org {
if !((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '-') {
return fmt.Errorf("organization name contains invalid character: %c", c)
Expand All @@ -128,8 +125,8 @@ func validateOrgName(org string) error {
}

// githubOwnerPattern matches valid GitHub usernames and org names
// (alphanumeric and single hyphens only, no dots or underscores).
var githubOwnerPattern = regexp.MustCompile(`^[a-zA-Z0-9](-?[a-zA-Z0-9])*$`)
// (alphanumeric and hyphens only, no dots or underscores).
var githubOwnerPattern = mintcore.GitHubOrgPattern

// githubRepoPattern matches valid GitHub repository names
// (alphanumeric, hyphens, dots, and underscores). Dot-prefixed repos such as
Expand Down
2 changes: 1 addition & 1 deletion internal/cli/admin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ func TestAnalyzeCmd_RequiresOrg(t *testing.T) {
}

func TestValidateOrgName_Valid(t *testing.T) {
valid := []string{"my-org", "org123", "A", "abc-def-ghi", "ORG"}
valid := []string{"my-org", "my--org", "org123", "A", "abc-def-ghi", "ORG"}
for _, name := range valid {
t.Run(name, func(t *testing.T) {
assert.NoError(t, validateOrgName(name))
Expand Down
6 changes: 3 additions & 3 deletions internal/cli/inference_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,10 +282,10 @@ func TestParseOrgOrRepo_OrgMode(t *testing.T) {
}

func TestParseOrgOrRepo_RepoMode(t *testing.T) {
org, repo, err := parseOrgOrRepo("acme/widget")
org, repo, err := parseOrgOrRepo("acme--corp/widget")
require.NoError(t, err)
assert.Equal(t, "acme", org)
assert.Equal(t, "acme/widget", repo)
assert.Equal(t, "acme--corp", org)
assert.Equal(t, "acme--corp/widget", repo)
}

func TestParseOrgOrRepo_Invalid(t *testing.T) {
Expand Down
7 changes: 3 additions & 4 deletions internal/dispatch/gcf/mintsrc/mintcore/patterns.go.embed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
// timestamps (exp, iat). Used by both STSVerifier and JWKSVerifier.
const maxClockSkew = 30 * time.Second

// GitHubOrgPattern validates GitHub org/user names: alphanumeric or single
// GitHubOrgPattern validates GitHub org/user names: alphanumeric or
// hyphens, cannot start or end with a hyphen, max 39 characters.
var GitHubOrgPattern = regexp.MustCompile(`^[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,37}[a-zA-Z0-9])?$`)

Expand All @@ -22,10 +22,9 @@ var RepoNamePattern = regexp.MustCompile(`^[a-zA-Z0-9_.][a-zA-Z0-9._-]{0,99}$`)
// RolePattern restricts role to safe lowercase identifiers.
var RolePattern = regexp.MustCompile(`^[a-z][a-z0-9_-]*$`)

// ValidateOrgName checks that an org name matches GitHubOrgPattern and
// does not contain double-hyphens (which would be ambiguous in secret names).
// ValidateOrgName checks that an org name matches GitHubOrgPattern.
func ValidateOrgName(org string) error {
if !GitHubOrgPattern.MatchString(org) || strings.Contains(org, "--") {
if !GitHubOrgPattern.MatchString(org) {
return fmt.Errorf("invalid org name %q", org)
}
return nil
Expand Down
6 changes: 3 additions & 3 deletions internal/dispatch/gcf/provisioner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,7 @@ func (p *Provisioner) Provision(ctx context.Context) (map[string]string, error)
}
seen := make(map[string]bool)
for _, org := range p.cfg.GitHubOrgs {
if !mintcore.GitHubOrgPattern.MatchString(org) || strings.Contains(org, "--") {
if err := mintcore.ValidateOrgName(org); err != nil {
return nil, fmt.Errorf("invalid GitHub org name: %q", org)
}
lower := strings.ToLower(org)
Expand Down Expand Up @@ -1522,7 +1522,7 @@ func (p *Provisioner) provisionRepoWIFProvider(ctx context.Context) (wifProvider
return "", "", fmt.Errorf("repo must be in owner/repo format, got %q", p.cfg.Repo)
}
partsLower := [2]string{strings.ToLower(parts[0]), strings.ToLower(parts[1])}
if !mintcore.GitHubOrgPattern.MatchString(partsLower[0]) || strings.Contains(partsLower[0], "--") {
if err := mintcore.ValidateOrgName(partsLower[0]); err != nil {
return "", "", fmt.Errorf("invalid repo owner %q: must be a valid GitHub org/user name", parts[0])
}
if !githubRepoSlugPattern.MatchString(partsLower[1]) {
Expand Down Expand Up @@ -1577,7 +1577,7 @@ func (p *Provisioner) ProvisionWIF(ctx context.Context) (wifProvider string, err
orgs := make([]string, len(p.cfg.GitHubOrgs))
seen := make(map[string]bool)
for i, org := range p.cfg.GitHubOrgs {
if !mintcore.GitHubOrgPattern.MatchString(org) || strings.Contains(org, "--") {
if err := mintcore.ValidateOrgName(org); err != nil {
return "", fmt.Errorf("invalid GitHub org name: %q", org)
}
lower := strings.ToLower(org)
Expand Down
23 changes: 11 additions & 12 deletions internal/dispatch/gcf/provisioner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ func TestProvisioner_Provision_FullFlow(t *testing.T) {
State: "ACTIVE",
URI: "https://fullsend-mint-abc123.run.app",
EnvVars: map[string]string{
"ALLOWED_ORGS": "test-org",
"ALLOWED_ORGS": "test--org",
"ROLE_APP_IDS": `{"coder":"12345"}`,
"ALLOWED_ROLES": "coder",
"ALLOWED_WORKFLOW_FILES": "*",
Expand All @@ -259,7 +259,7 @@ func TestProvisioner_Provision_FullFlow(t *testing.T) {

p := newTestProvisioner(Config{
ProjectID: "my-project",
GitHubOrgs: []string{"test-org"},
GitHubOrgs: []string{"test--org"},
AgentPEMs: singleRolePEMs(),
AgentAppIDs: singleRoleAppIDs(),
FunctionSourceDir: fakeFunctionSourceDir(t),
Expand Down Expand Up @@ -298,7 +298,7 @@ func TestProvisioner_Provision_FullFlow(t *testing.T) {
assert.Equal(t, "my-project", fake.projectIAMBindings[0].ProjectID)
assert.Equal(t, "roles/aiplatform.user", fake.projectIAMBindings[0].Role)
assert.Contains(t, fake.projectIAMBindings[0].Member, "principalSet://iam.googleapis.com/")
assert.Contains(t, fake.projectIAMBindings[0].Member, "attribute.repository/test-org/.fullsend")
assert.Contains(t, fake.projectIAMBindings[0].Member, "attribute.repository/test--org/.fullsend")

// Verify PEMs were zeroed.
for role, pem := range p.cfg.AgentPEMs {
Expand Down Expand Up @@ -1905,7 +1905,7 @@ func TestProvisionWIF_HappyPath(t *testing.T) {
fake := newFakeGCFClient()
p := NewProvisioner(Config{
ProjectID: "my-project",
GitHubOrgs: []string{"acme"},
GitHubOrgs: []string{"acme--corp"},
}, fake)

wifProvider, err := p.ProvisionWIF(context.Background())
Expand All @@ -1918,7 +1918,7 @@ func TestProvisionWIF_HappyPath(t *testing.T) {
assert.Contains(t, fake.calls, "CreateWIFProvider")
assert.Contains(t, fake.calls, "SetProjectIAMBinding")

assert.Equal(t, "assertion.repository_owner == 'acme'", fake.lastWIFProviderConfig.AttributeCondition)
assert.Equal(t, "assertion.repository_owner == 'acme--corp'", fake.lastWIFProviderConfig.AttributeCondition)
}

func TestProvisionWIF_MissingProjectID(t *testing.T) {
Expand Down Expand Up @@ -2076,19 +2076,19 @@ func TestProvisionWIF_RepoScoped(t *testing.T) {
fake := newFakeGCFClient()
p := NewProvisioner(Config{
ProjectID: "my-project",
GitHubOrgs: []string{"acme"},
Repo: "acme/widget",
GitHubOrgs: []string{"acme--corp"},
Repo: "acme--corp/widget",
}, fake)

wifPath, err := p.ProvisionWIF(context.Background())
require.NoError(t, err)

assert.Equal(t, "gh-acme-widget", fake.lastWIFProviderID)
assert.Equal(t, "assertion.repository == 'acme/widget'", fake.lastWIFProviderConfig.AttributeCondition)
assert.Contains(t, wifPath, "gh-acme-widget")
assert.Equal(t, "gh-acme--corp-widget", fake.lastWIFProviderID)
assert.Equal(t, "assertion.repository == 'acme--corp/widget'", fake.lastWIFProviderConfig.AttributeCondition)
assert.Contains(t, wifPath, "gh-acme--corp-widget")

require.Len(t, fake.projectIAMBindings, 1)
assert.Contains(t, fake.projectIAMBindings[0].Member, "attribute.repository/acme/widget")
assert.Contains(t, fake.projectIAMBindings[0].Member, "attribute.repository/acme--corp/widget")

assert.NotContains(t, fake.calls, "GetWIFProvider")
}
Expand Down Expand Up @@ -2186,7 +2186,6 @@ func TestProvisionWIF_RepoScoped_RejectsInvalidRepo(t *testing.T) {
{"dot as repo", "owner/.", "cannot be"},
{"dotdot as repo", "owner/..", "cannot be"},
{"dot as owner", "./repo", "invalid repo owner"},
{"double-hyphen in owner", "org--name/repo", "invalid repo owner"},
{"git suffix", "owner/repo.git", "cannot end with"},
}
for _, tt := range tests {
Expand Down
5 changes: 4 additions & 1 deletion internal/mintcore/foreign_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,10 @@ func TestValidateTargetOrg(t *testing.T) {
if err := validateTargetOrg("halfsend-01"); err != nil {
t.Fatalf("valid org: %v", err)
}
if err := validateTargetOrg("bad--org"); err == nil {
if err := validateTargetOrg("half--send"); err != nil {
t.Fatalf("valid org with consecutive hyphens: %v", err)
}
if err := validateTargetOrg("-bad-org"); err == nil {
t.Fatal("expected invalid org")
}
}
Expand Down
7 changes: 3 additions & 4 deletions internal/mintcore/patterns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
// timestamps (exp, iat). Used by both STSVerifier and JWKSVerifier.
const maxClockSkew = 30 * time.Second

// GitHubOrgPattern validates GitHub org/user names: alphanumeric or single
// GitHubOrgPattern validates GitHub org/user names: alphanumeric or
// hyphens, cannot start or end with a hyphen, max 39 characters.
var GitHubOrgPattern = regexp.MustCompile(`^[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,37}[a-zA-Z0-9])?$`)

Expand All @@ -22,10 +22,9 @@ var RepoNamePattern = regexp.MustCompile(`^[a-zA-Z0-9_.][a-zA-Z0-9._-]{0,99}$`)
// RolePattern restricts role to safe lowercase identifiers.
var RolePattern = regexp.MustCompile(`^[a-z][a-z0-9_-]*$`)

// ValidateOrgName checks that an org name matches GitHubOrgPattern and
// does not contain double-hyphens (which would be ambiguous in secret names).
// ValidateOrgName checks that an org name matches GitHubOrgPattern.
func ValidateOrgName(org string) error {
if !GitHubOrgPattern.MatchString(org) || strings.Contains(org, "--") {
if !GitHubOrgPattern.MatchString(org) {
Comment thread
shairevivo marked this conversation as resolved.
Resolved
return fmt.Errorf("invalid org name %q", org)
}
return nil
Expand Down
Loading