diff --git a/internal/cli/admin.go b/internal/cli/admin.go index a1ef00d039..74b731ff9e 100644 --- a/internal/cli/admin.go +++ b/internal/cli/admin.go @@ -1130,7 +1130,7 @@ func runPerRepoInstall(ctx context.Context, c perRepoInstallConfig) error { "FULLSEND_GCP_PROJECT_ID": inferenceProject, "FULLSEND_GCP_WIF_PROVIDER": inferenceWIFProvider, } - if err := applyPerRepoScaffold(ctx, client, printer, owner, repo, vendorFiles, repoVars, repoSecrets, c.Direct); err != nil { + if err := applyPerRepoScaffold(ctx, client, printer, owner, repo, vendorFiles, repoVars, repoSecrets, scaffoldOptions{direct: c.Direct}); err != nil { return err } } @@ -1210,11 +1210,18 @@ func (a *gcfProvisionerAdapter) DeleteWIFProvider(ctx context.Context, repo stri return a.provisioner.DeleteWIFProvider(ctx, providerID) } +// scaffoldOptions holds optional behavioral modifiers for applyPerRepoScaffold, +// keeping the function signature stable as new options are added. +type scaffoldOptions struct { + direct bool // push directly to the default branch instead of creating a PR + signOffTrailer string // e.g. "Signed-off-by: Name "; appended to the commit message when non-empty +} + // applyPerRepoScaffold commits scaffold files to the repo's default branch // and configures the repository variables and secrets needed for fullsend. func applyPerRepoScaffold(ctx context.Context, client forge.Client, printer *ui.Printer, owner, repo string, files []forge.TreeFile, - repoVars, repoSecrets map[string]string, direct bool) error { + repoVars, repoSecrets map[string]string, opts scaffoldOptions) error { targetRepo, err := client.GetRepo(ctx, owner, repo) if err != nil { @@ -1227,7 +1234,10 @@ func applyPerRepoScaffold(ctx context.Context, client forge.Client, printer *ui. // BuildScaffoldPRMetadata will use the guard variable to distinguish // fresh installs from upgrades without version information. meta := repos.BuildScaffoldPRMetadata(ctx, client, owner, repo, "") - if direct { + if opts.signOffTrailer != "" { + meta.CommitMsg += "\n\n" + opts.signOffTrailer + } + if opts.direct { printer.StepStart(fmt.Sprintf("Committing scaffold files to %s/%s (%s branch)", owner, repo, targetRepo.DefaultBranch)) } else { @@ -1236,7 +1246,7 @@ func applyPerRepoScaffold(ctx context.Context, client forge.Client, printer *ui. } if _, err := layers.CommitScaffoldFiles(ctx, client, printer, owner, repo, targetRepo.DefaultBranch, - meta, files, direct, os.Stdin); err != nil { + meta, files, opts.direct, os.Stdin); err != nil { return err } diff --git a/internal/cli/admin_test.go b/internal/cli/admin_test.go index 3906e1afd7..d75839f0ae 100644 --- a/internal/cli/admin_test.go +++ b/internal/cli/admin_test.go @@ -2657,7 +2657,7 @@ func TestApplyPerRepoScaffold(t *testing.T) { } err := applyPerRepoScaffold(context.Background(), client, printer, - "acme", "widget", files, repoVars, repoSecrets, false) + "acme", "widget", files, repoVars, repoSecrets, scaffoldOptions{}) require.NoError(t, err) require.Len(t, client.CommittedFilesToBranch, 1) @@ -2683,6 +2683,47 @@ func TestApplyPerRepoScaffold(t *testing.T) { assert.Contains(t, secretNames, "FULLSEND_GCP_WIF_PROVIDER") } +func TestApplyPerRepoScaffold_WithSignOff(t *testing.T) { + client := forge.NewFakeClient() + client.AuthenticatedUser = "acme" + client.Repos = []forge.Repository{{FullName: "acme/widget", DefaultBranch: "main"}} + var buf bytes.Buffer + printer := ui.New(&buf) + + files := []forge.TreeFile{ + {Path: ".fullsend/config.yaml", Content: []byte("config"), Mode: "100644"}, + } + trailer := "Signed-off-by: Test User " + + err := applyPerRepoScaffold(context.Background(), client, printer, + "acme", "widget", files, nil, nil, scaffoldOptions{direct: true, signOffTrailer: trailer}) + require.NoError(t, err) + + require.NotEmpty(t, client.CommittedFiles) + commitMsg := client.CommittedFiles[0].Message + assert.Contains(t, commitMsg, "chore: initialize fullsend per-repo installation") + assert.Contains(t, commitMsg, "Signed-off-by: Test User ") +} + +func TestApplyPerRepoScaffold_WithoutSignOff(t *testing.T) { + client := forge.NewFakeClient() + client.AuthenticatedUser = "acme" + client.Repos = []forge.Repository{{FullName: "acme/widget", DefaultBranch: "main"}} + printer := ui.New(&bytes.Buffer{}) + + files := []forge.TreeFile{ + {Path: ".fullsend/config.yaml", Content: []byte("config"), Mode: "100644"}, + } + + err := applyPerRepoScaffold(context.Background(), client, printer, + "acme", "widget", files, nil, nil, scaffoldOptions{direct: true}) + require.NoError(t, err) + + require.NotEmpty(t, client.CommittedFiles) + commitMsg := client.CommittedFiles[0].Message + assert.Equal(t, "chore: initialize fullsend per-repo installation", commitMsg) +} + func TestApplyPerRepoScaffold_GetRepoError(t *testing.T) { client := forge.NewFakeClient() client.AuthenticatedUser = "acme" @@ -2690,7 +2731,7 @@ func TestApplyPerRepoScaffold_GetRepoError(t *testing.T) { printer := ui.New(&bytes.Buffer{}) err := applyPerRepoScaffold(context.Background(), client, printer, - "acme", "widget", nil, nil, nil, false) + "acme", "widget", nil, nil, nil, scaffoldOptions{}) require.Error(t, err) assert.Contains(t, err.Error(), "getting repo info") } @@ -2707,7 +2748,7 @@ func TestApplyPerRepoScaffold_CommitFilesError(t *testing.T) { } err := applyPerRepoScaffold(context.Background(), client, printer, - "acme", "widget", files, nil, nil, true) + "acme", "widget", files, nil, nil, scaffoldOptions{direct: true}) require.Error(t, err) assert.Contains(t, err.Error(), "committing scaffold files") assert.Empty(t, client.CreatedBranches, "should not attempt fallback for generic error") @@ -2731,7 +2772,7 @@ func TestApplyPerRepoScaffold_Idempotent(t *testing.T) { } err := applyPerRepoScaffold(context.Background(), client, printer, - "acme", "widget", files, map[string]string{"K": "V"}, map[string]string{"S": "secret"}, false) + "acme", "widget", files, map[string]string{"K": "V"}, map[string]string{"S": "secret"}, scaffoldOptions{}) require.NoError(t, err) assert.Contains(t, buf.String(), "up to date") assert.Len(t, client.Variables, 1, "variables should still be set even when files are unchanged") @@ -2753,7 +2794,7 @@ func TestApplyPerRepoScaffold_DefaultPR_NoChanges(t *testing.T) { } err := applyPerRepoScaffold(context.Background(), client, printer, - "acme", "widget", files, map[string]string{"K": "V"}, nil, false) + "acme", "widget", files, map[string]string{"K": "V"}, nil, scaffoldOptions{}) require.NoError(t, err) assert.Contains(t, buf.String(), "up to date") } @@ -2770,7 +2811,7 @@ func TestApplyPerRepoScaffold_NonMainBranch(t *testing.T) { } err := applyPerRepoScaffold(context.Background(), client, printer, - "acme", "widget", files, nil, nil, true) + "acme", "widget", files, nil, nil, scaffoldOptions{direct: true}) require.NoError(t, err) assert.Contains(t, buf.String(), "acme/widget (develop branch)") assert.Contains(t, buf.String(), "Pushed 1 file to develop") @@ -2786,7 +2827,7 @@ func TestApplyPerRepoScaffold_CreateVariableError(t *testing.T) { printer := ui.New(&bytes.Buffer{}) err := applyPerRepoScaffold(context.Background(), client, printer, - "acme", "widget", nil, map[string]string{"K": "V"}, nil, false) + "acme", "widget", nil, map[string]string{"K": "V"}, nil, scaffoldOptions{}) require.Error(t, err) assert.Contains(t, err.Error(), "setting repo variable") } @@ -2801,7 +2842,7 @@ func TestApplyPerRepoScaffold_CreateSecretError(t *testing.T) { printer := ui.New(&bytes.Buffer{}) err := applyPerRepoScaffold(context.Background(), client, printer, - "acme", "widget", nil, nil, map[string]string{"S": "V"}, false) + "acme", "widget", nil, nil, map[string]string{"S": "V"}, scaffoldOptions{}) require.Error(t, err) assert.Contains(t, err.Error(), "setting repo secret") } @@ -2821,7 +2862,7 @@ func TestApplyPerRepoScaffold_ProtectedBranchFallback(t *testing.T) { repoSecrets := map[string]string{"S": "secret"} err := applyPerRepoScaffold(context.Background(), client, printer, - "acme", "widget", files, repoVars, repoSecrets, true) + "acme", "widget", files, repoVars, repoSecrets, scaffoldOptions{direct: true}) require.NoError(t, err) require.Len(t, client.CreatedBranches, 1) @@ -2854,7 +2895,7 @@ func TestApplyPerRepoScaffold_ProtectedBranch_ExistingBranch(t *testing.T) { } err := applyPerRepoScaffold(context.Background(), client, printer, - "acme", "widget", files, nil, nil, true) + "acme", "widget", files, nil, nil, scaffoldOptions{direct: true}) require.NoError(t, err) require.Len(t, client.CommittedFilesToBranch, 1, "should proceed despite branch existing") @@ -2875,7 +2916,7 @@ func TestApplyPerRepoScaffold_ProtectedBranch_StillSetsVarsAndSecrets(t *testing repoSecrets := map[string]string{"FULLSEND_GCP_PROJECT_ID": "my-project"} err := applyPerRepoScaffold(context.Background(), client, printer, - "acme", "widget", files, repoVars, repoSecrets, true) + "acme", "widget", files, repoVars, repoSecrets, scaffoldOptions{direct: true}) require.NoError(t, err) assert.Len(t, client.Variables, 1, "variables should be set even with PR fallback") @@ -2895,7 +2936,7 @@ func TestApplyPerRepoScaffold_ProtectedBranch_CreateBranchFails(t *testing.T) { } err := applyPerRepoScaffold(context.Background(), client, printer, - "acme", "widget", files, nil, nil, true) + "acme", "widget", files, nil, nil, scaffoldOptions{direct: true}) require.Error(t, err) assert.Contains(t, err.Error(), "creating scaffold branch") } @@ -2913,7 +2954,7 @@ func TestApplyPerRepoScaffold_ProtectedBranch_CommitToBranchFails(t *testing.T) } err := applyPerRepoScaffold(context.Background(), client, printer, - "acme", "widget", files, nil, nil, true) + "acme", "widget", files, nil, nil, scaffoldOptions{direct: true}) require.Error(t, err) assert.Contains(t, err.Error(), "committing scaffold files to branch") } @@ -2931,7 +2972,7 @@ func TestApplyPerRepoScaffold_ProtectedBranch_ScaffoldBranchAlsoProtected(t *tes } err := applyPerRepoScaffold(context.Background(), client, printer, - "acme", "widget", files, nil, nil, true) + "acme", "widget", files, nil, nil, scaffoldOptions{direct: true}) require.Error(t, err) assert.Contains(t, err.Error(), "is protected") assert.Contains(t, err.Error(), "configure branch protection") @@ -2950,7 +2991,7 @@ func TestApplyPerRepoScaffold_ProtectedBranch_CreatePRFails(t *testing.T) { } err := applyPerRepoScaffold(context.Background(), client, printer, - "acme", "widget", files, nil, nil, true) + "acme", "widget", files, nil, nil, scaffoldOptions{direct: true}) require.Error(t, err) assert.Contains(t, err.Error(), "creating scaffold PR") } @@ -2969,7 +3010,7 @@ func TestApplyPerRepoScaffold_ProtectedBranch_DuplicatePR(t *testing.T) { } err := applyPerRepoScaffold(context.Background(), client, printer, - "acme", "widget", files, nil, nil, true) + "acme", "widget", files, nil, nil, scaffoldOptions{direct: true}) require.NoError(t, err) output := buf.String() @@ -3136,7 +3177,7 @@ func TestApplyPerRepoScaffold_ProtectedBranch_BranchUpToDate(t *testing.T) { } err := applyPerRepoScaffold(context.Background(), client, printer, - "acme", "widget", files, nil, nil, true) + "acme", "widget", files, nil, nil, scaffoldOptions{direct: true}) require.NoError(t, err) assert.Contains(t, buf.String(), "up to date") diff --git a/internal/cli/github.go b/internal/cli/github.go index f25360daf9..f280f5f333 100644 --- a/internal/cli/github.go +++ b/internal/cli/github.go @@ -71,6 +71,7 @@ type githubSetupConfig struct { runtime string configPreset string // --config: local path or HTTPS URL to a preset configHash string // --config-hash: SHA-256 hex digest for preset validation + signoff bool // --signoff: add Signed-off-by trailer to scaffold commits // changedFlags records which flags were explicitly set on the // command line (populated by RunE before calling the setup @@ -116,7 +117,7 @@ values (mint URL, WIF provider, project ID) are provided as flags.`, _, _, isRepoTarget := parseTarget(cfg.target) if !isRepoTarget { - for _, name := range []string{"config", "config-hash"} { + for _, name := range []string{"config", "config-hash", "signoff"} { if cmd.Flags().Changed(name) { return fmt.Errorf("--%s is only valid for per-repo setup (fullsend github setup )", name) } @@ -191,6 +192,7 @@ values (mint URL, WIF provider, project ID) are provided as flags.`, addVendorFlags(cmd, &cfg.vendor, &cfg.fullsendBinary, &cfg.fullsendSource) cmd.Flags().StringVar(&cfg.configPreset, "config", "", "local file path or HTTPS URL to a vendor preset (committed as .fullsend/config.base.yaml)") cmd.Flags().StringVar(&cfg.configHash, "config-hash", "", "SHA-256 hex digest to validate the preset content") + cmd.Flags().BoolVar(&cfg.signoff, "signoff", false, "add Signed-off-by trailer to scaffold commits (requires GitHub user identity)") return cmd } @@ -402,12 +404,41 @@ func runGitHubSetupPerRepo(ctx context.Context, client forge.Client, printer *ui repoSecrets["FULLSEND_GCP_WIF_PROVIDER"] = cfg.inferenceWIFProvider } + // Resolve Signed-off-by trailer when --signoff is set. + // + // Identity resolution runs before the dry-run early return so that + // --dry-run --signoff validates the token's identity up front instead + // of silently skipping the check. + // + // Unlike sync-scaffold (which gracefully degrades when identity is + // unavailable), setup uses an explicit opt-in flag and hard-fails. + // The user explicitly requested DCO sign-off; silently omitting the + // trailer would cause the DCO check to fail with a confusing error. + var signOffTrailer string + if cfg.signoff { + id, idErr := client.GetAuthenticatedUserIdentity(ctx) + if idErr != nil { + return fmt.Errorf("--signoff requires a GitHub user identity (name and email) — this is not available for GitHub App tokens: %w", idErr) + } + if id.Name == "" || id.Email == "" { + return fmt.Errorf("--signoff requires a GitHub user identity with both name and email set (got name=%q, email=%q)", id.Name, id.Email) + } + trailer, trailerErr := id.SignOffTrailer() + if trailerErr != nil { + return fmt.Errorf("--signoff: %w", trailerErr) + } + signOffTrailer = trailer + } + if cfg.dryRun { printer.StepInfo("Dry run — no changes will be made") printer.Blank() for _, f := range files { printer.StepDone(fmt.Sprintf("Would commit: %s (%d bytes)", f.Path, len(f.Content))) } + if signOffTrailer != "" { + printer.StepDone(fmt.Sprintf("Would add trailer: %s", signOffTrailer)) + } printer.Blank() printer.StepInfo("Would set repository variables:") for _, name := range maputil.SortedKeys(repoVars) { @@ -441,7 +472,7 @@ func runGitHubSetupPerRepo(ctx context.Context, client forge.Client, printer *ui } } - if err := applyPerRepoScaffold(ctx, client, printer, owner, repo, files, repoVars, repoSecrets, cfg.direct); err != nil { + if err := applyPerRepoScaffold(ctx, client, printer, owner, repo, files, repoVars, repoSecrets, scaffoldOptions{direct: cfg.direct, signOffTrailer: signOffTrailer}); err != nil { return err } diff --git a/internal/cli/github_test.go b/internal/cli/github_test.go index f61bd96390..9907df8a0d 100644 --- a/internal/cli/github_test.go +++ b/internal/cli/github_test.go @@ -1,6 +1,7 @@ package cli import ( + "bytes" "context" "fmt" "strings" @@ -100,6 +101,10 @@ func TestGitHubSetupCmd_Flags(t *testing.T) { inferenceWIFFlag := cmd.Flags().Lookup("inference-wif-provider") require.NotNil(t, inferenceWIFFlag, "expected --inference-wif-provider flag") + + signoffFlag := cmd.Flags().Lookup("signoff") + require.NotNil(t, signoffFlag, "expected --signoff flag") + assert.Equal(t, "false", signoffFlag.DefValue) } func TestGitHubSetupCmd_UsesDefaultMintURL(t *testing.T) { @@ -947,6 +952,134 @@ func TestRunGitHubSetupPerRepo(t *testing.T) { assert.Contains(t, secretNames, "FULLSEND_GCP_WIF_PROVIDER") } +// newSignoffTestSetup returns a pre-configured fake client and base config +// for signoff tests. Override fields on the returned values as needed. +func newSignoffTestSetup(t *testing.T) (*forge.FakeClient, githubSetupConfig) { + t.Helper() + t.Setenv("GH_TOKEN", "test-token") + + client := forge.NewFakeClient() + client.AuthenticatedUser = "acme" + client.AuthenticatedUserIdentity = &forge.UserIdentity{ + Name: "Test User", + Email: "test@example.com", + } + client.Repos = []forge.Repository{{FullName: "acme/widget", DefaultBranch: "main"}} + client.TokenScopes = []string{"repo", "workflow"} + + cfg := githubSetupConfig{ + target: "acme/widget", + mintURL: "https://mint-test-abc123.run.app", + inferenceProject: "my-project", + inferenceWIFProvider: "projects/123456789/locations/global/workloadIdentityPools/fullsend-pool/providers/github-oidc", + inferenceRegion: "global", + agents: strings.Join(config.PerRepoDefaultRoles(), ","), + } + return client, cfg +} + +func TestRunGitHubSetupPerRepo_SignoffAddsTrailer(t *testing.T) { + client, cfg := newSignoffTestSetup(t) + cfg.signoff = true + printer := ui.New(&discardWriter{}) + + err := runGitHubSetupPerRepo(context.Background(), client, printer, cfg) + require.NoError(t, err) + + // Verify the commit message contains the Signed-off-by trailer. + require.NotEmpty(t, client.CommittedFilesToBranch) + commitMsg := client.CommittedFilesToBranch[0].Message + assert.Contains(t, commitMsg, "Signed-off-by: Test User ") +} + +func TestRunGitHubSetupPerRepo_WithoutSignoffOmitsTrailer(t *testing.T) { + client, cfg := newSignoffTestSetup(t) + cfg.signoff = false + printer := ui.New(&discardWriter{}) + + err := runGitHubSetupPerRepo(context.Background(), client, printer, cfg) + require.NoError(t, err) + + // Verify the commit message does NOT contain a Signed-off-by trailer. + require.NotEmpty(t, client.CommittedFilesToBranch) + commitMsg := client.CommittedFilesToBranch[0].Message + assert.NotContains(t, commitMsg, "Signed-off-by") +} + +func TestRunGitHubSetupPerRepo_SignoffMissingIdentity(t *testing.T) { + client, cfg := newSignoffTestSetup(t) + client.AuthenticatedUserIdentity = nil // simulates a bot token + cfg.signoff = true + printer := ui.New(&discardWriter{}) + + err := runGitHubSetupPerRepo(context.Background(), client, printer, cfg) + require.Error(t, err) + assert.Contains(t, err.Error(), "--signoff requires a GitHub user identity") +} + +func TestRunGitHubSetupPerRepo_SignoffDirect(t *testing.T) { + client, cfg := newSignoffTestSetup(t) + cfg.signoff = true + cfg.direct = true + printer := ui.New(&discardWriter{}) + + err := runGitHubSetupPerRepo(context.Background(), client, printer, cfg) + require.NoError(t, err) + + // Direct mode commits to the default branch. + require.NotEmpty(t, client.CommittedFiles) + commitMsg := client.CommittedFiles[0].Message + assert.Contains(t, commitMsg, "Signed-off-by: Test User ") +} + +func TestGitHubSetupCmd_SignoffRejectedForOrgTarget(t *testing.T) { + cmd := newRootCmd() + cmd.SetArgs([]string{"github", "setup", "acme", "--signoff"}) + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "--signoff is only valid for per-repo setup") +} + +func TestRunGitHubSetupPerRepo_SignoffEmptyIdentityFields(t *testing.T) { + client, cfg := newSignoffTestSetup(t) + client.AuthenticatedUserIdentity = &forge.UserIdentity{Name: "", Email: ""} + cfg.signoff = true + printer := ui.New(&discardWriter{}) + + err := runGitHubSetupPerRepo(context.Background(), client, printer, cfg) + require.Error(t, err) + assert.Contains(t, err.Error(), "--signoff requires a GitHub user identity with both name and email set") +} + +func TestRunGitHubSetupPerRepo_DryRunSignoffShowsTrailer(t *testing.T) { + client, cfg := newSignoffTestSetup(t) + cfg.signoff = true + cfg.dryRun = true + var buf bytes.Buffer + printer := ui.New(&buf) + + err := runGitHubSetupPerRepo(context.Background(), client, printer, cfg) + require.NoError(t, err) + + // Dry run should display the trailer that would be added. + assert.Contains(t, buf.String(), "Signed-off-by: Test User ") + // Nothing should actually be committed. + assert.Empty(t, client.CommittedFiles) + assert.Empty(t, client.CommittedFilesToBranch) +} + +func TestRunGitHubSetupPerRepo_DryRunSignoffMissingIdentity(t *testing.T) { + client, cfg := newSignoffTestSetup(t) + client.AuthenticatedUserIdentity = nil + cfg.signoff = true + cfg.dryRun = true + printer := ui.New(&discardWriter{}) + + err := runGitHubSetupPerRepo(context.Background(), client, printer, cfg) + require.Error(t, err) + assert.Contains(t, err.Error(), "--signoff requires a GitHub user identity") +} + func TestGitHubSetCmd_OrgTargetDefaultsToConfigRepo(t *testing.T) { client := forge.NewFakeClient() printer := ui.New(&discardWriter{}) diff --git a/internal/forge/forge.go b/internal/forge/forge.go index 38b10d77b0..22463caa1c 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -6,6 +6,8 @@ package forge import ( "context" "errors" + "fmt" + "strings" ) // ConfigRepoName is the conventional name for the org-level fullsend @@ -252,6 +254,29 @@ type UserIdentity struct { Email string // primary or noreply email } +// SignOffTrailer returns a "Signed-off-by: Name " string for this +// identity. Newline characters are stripped from both fields to prevent +// trailer injection via crafted profile names. Returns an error if name +// or email is empty after sanitization. +func (id *UserIdentity) SignOffTrailer() (string, error) { + return FormatSignOffTrailer(id.Name, id.Email) +} + +// FormatSignOffTrailer builds a "Signed-off-by: name " string. +// Newline characters (\n, \r) and angle brackets (< and >) are stripped +// from both fields to prevent trailer injection via crafted forge profile +// names and malformed trailers. Returns an error if name or email is +// empty after sanitization. +func FormatSignOffTrailer(name, email string) (string, error) { + sanitize := strings.NewReplacer("\n", "", "\r", "", "<", "", ">", "") + name = strings.TrimSpace(sanitize.Replace(name)) + email = strings.TrimSpace(sanitize.Replace(email)) + if name == "" || email == "" { + return "", fmt.Errorf("sign-off identity must have non-empty name and email after sanitization (got name=%q, email=%q)", name, email) + } + return fmt.Sprintf("Signed-off-by: %s <%s>", name, email), nil +} + // TreeFile represents a file to be committed via the Git Trees API. // Mode controls file permissions: "100644" for regular files, // "100755" for executable files (e.g., shell scripts). diff --git a/internal/forge/signoff_test.go b/internal/forge/signoff_test.go new file mode 100644 index 0000000000..7b36532bab --- /dev/null +++ b/internal/forge/signoff_test.go @@ -0,0 +1,61 @@ +package forge + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFormatSignOffTrailer(t *testing.T) { + got, err := FormatSignOffTrailer("Alice Smith", "alice@example.com") + require.NoError(t, err) + assert.Equal(t, "Signed-off-by: Alice Smith ", got) +} + +func TestFormatSignOffTrailer_StripsNewlines(t *testing.T) { + got, err := FormatSignOffTrailer("Evil\nUser", "evil@example.com\r") + require.NoError(t, err) + assert.Equal(t, "Signed-off-by: EvilUser ", got) +} + +func TestFormatSignOffTrailer_StripsAngleBracketsFromName(t *testing.T) { + got, err := FormatSignOffTrailer("Evil>User", "evil@example.com") + require.NoError(t, err) + assert.Equal(t, "Signed-off-by: EvilUser ", got) + + got, err = FormatSignOffTrailer("User ", "user@example.com") + require.NoError(t, err) + assert.Equal(t, "Signed-off-by: User injected ", got) +} + +func TestFormatSignOffTrailer_StripsAngleBracketsFromEmail(t *testing.T) { + got, err := FormatSignOffTrailer("User", "") + require.NoError(t, err) + assert.Equal(t, "Signed-off-by: User ", got) +} + +func TestFormatSignOffTrailer_ErrorsOnEmptyNameAfterSanitization(t *testing.T) { + _, err := FormatSignOffTrailer("\n\r", "user@example.com") + require.Error(t, err) + assert.Contains(t, err.Error(), "non-empty name and email after sanitization") +} + +func TestFormatSignOffTrailer_ErrorsOnEmptyEmailAfterSanitization(t *testing.T) { + _, err := FormatSignOffTrailer("User", "<>") + require.Error(t, err) + assert.Contains(t, err.Error(), "non-empty name and email after sanitization") +} + +func TestFormatSignOffTrailer_ErrorsOnBothEmptyAfterSanitization(t *testing.T) { + _, err := FormatSignOffTrailer("<>", "\n\r") + require.Error(t, err) + assert.Contains(t, err.Error(), "non-empty name and email after sanitization") +} + +func TestUserIdentity_SignOffTrailer(t *testing.T) { + id := &UserIdentity{Name: "Test User", Email: "test@example.com"} + got, err := id.SignOffTrailer() + require.NoError(t, err) + assert.Equal(t, "Signed-off-by: Test User ", got) +} diff --git a/internal/layers/workflows.go b/internal/layers/workflows.go index fd19708f92..ccaef53cab 100644 --- a/internal/layers/workflows.go +++ b/internal/layers/workflows.go @@ -65,9 +65,13 @@ func (l *WorkflowsLayer) WithDirect(direct bool) *WorkflowsLayer { // WithSignOff configures a Signed-off-by trailer to append to commit // messages. This is used for human-driven CLI operations where DCO // sign-off is required. Pass an empty string to disable. +// If the identity is empty after sanitization, the trailer is silently +// omitted (callers in best-effort paths already guard for this). func (l *WorkflowsLayer) WithSignOff(name, email string) *WorkflowsLayer { if name != "" && email != "" { - l.signOffTrailer = fmt.Sprintf("Signed-off-by: %s <%s>", name, email) + if trailer, err := forge.FormatSignOffTrailer(name, email); err == nil { + l.signOffTrailer = trailer + } } return l }