diff --git a/shortcuts/apps/git_credential.go b/shortcuts/apps/git_credential.go index 2a39d02e..7b0ea606 100644 --- a/shortcuts/apps/git_credential.go +++ b/shortcuts/apps/git_credential.go @@ -488,7 +488,7 @@ func issuedFromData(appID string, data map[string]interface{}) (*gitcred.IssuedC // handled locally. func parseIssueCredentialData(resp *larkcore.ApiResp, err error, cc errclass.ClassifyContext) (map[string]any, error) { if err != nil { - return nil, client.WrapDoAPIError(err) + return nil, redactGitCredentialIssueError(client.WrapDoAPIError(err)) } detail := logIDDetail(resp) if resp == nil || len(resp.RawBody) == 0 { @@ -501,7 +501,7 @@ func parseIssueCredentialData(resp *larkcore.ApiResp, err error, cc errclass.Cla if jsonErr != nil || hasCode || resp.StatusCode >= http.StatusBadRequest { data, cerr := common.ClassifyAPIResponseWith(resp, cc) if cerr != nil { - return nil, withAppsHint(cerr, gitCredentialIssueHint) + return nil, redactGitCredentialIssueError(withAppsHint(cerr, gitCredentialIssueHint)) } if data != nil { result = data @@ -536,6 +536,7 @@ func checkGitInfoBaseResp(result map[string]any, logID string) error { if message == "" { message = "Git credential API returned non-zero BaseResp status" } + message = gitcred.RedactCredentialText(message) baseErr := errs.NewAPIError(errs.SubtypeUnknown, "Issue app Git credential: %s", message).WithCode(int(code)) if logID != "" { baseErr = baseErr.WithLogID(logID) @@ -545,6 +546,17 @@ func checkGitInfoBaseResp(result map[string]any, logID string) error { return nil } +func redactGitCredentialIssueError(err error) error { + if err == nil { + return nil + } + if p, ok := errs.ProblemOf(err); ok { + p.Message = gitcred.RedactCredentialText(p.Message) + p.Hint = gitcred.RedactCredentialText(p.Hint) + } + return err +} + func logIDDetail(resp *larkcore.ApiResp) map[string]any { logID := logIDString(resp) if logID == "" { diff --git a/shortcuts/apps/git_credential_test.go b/shortcuts/apps/git_credential_test.go index ee71c367..b48b96b6 100644 --- a/shortcuts/apps/git_credential_test.go +++ b/shortcuts/apps/git_credential_test.go @@ -12,7 +12,6 @@ import ( "net/http" "os" "path/filepath" - "regexp" "strconv" "strings" "testing" @@ -1027,25 +1026,24 @@ func TestParseIssueCredentialDataBusinessCodeHasHintNotRetryable(t *testing.T) { } } -// TestParseIssueCredentialDataMessageAddsNoExtraSecret verifies the security -// condition that apps does not ADDITIONALLY inject any token/secret into the -// Git-credential error it builds. The server `msg` is passed through verbatim -// into Problem.Message, and the only thing apps adds is the static -// gitCredentialIssueHint — which itself contains no secret. We feed a benign -// server msg and assert (a) Message equals that msg exactly, and (b) neither -// Message nor Hint contains any token/secret-shaped string. -// -// Note: server msg passthrough is the shared classifier's responsibility; -// apps adds only a static hint. There is no msg redaction in this path, so -// this test does not assert a redaction that does not exist — it asserts -// that apps injects nothing sensitive of its own. -func TestParseIssueCredentialDataMessageAddsNoExtraSecret(t *testing.T) { - const serverMsg = "permission denied" +// TestParseIssueCredentialDataRedactsCredentialErrorMessage verifies that the +// git-credential boundary does not pass server-provided credential details into +// the user-visible typed envelope message. +func TestParseIssueCredentialDataRedactsCredentialErrorMessage(t *testing.T) { + samplePAT := testPublicSafeJoin("pat", "-sample") + samplePassword := "sample-password" + serverMsg := "permission denied: " + + testCredentialAssignment("token", samplePAT) + " " + + testCredentialAssignment("password", samplePassword) + " " + + testCredentialURLWithUserInfo("example.com/repo.git", samplePAT) header := http.Header{"X-Tt-Logid": []string{"log_x"}} for _, tc := range []struct { - name string - resp *larkcore.ApiResp + name string + resp *larkcore.ApiResp + wantType errs.Category + wantSubtype errs.Subtype + wantCode int }{ { name: "http error path", @@ -1054,6 +1052,9 @@ func TestParseIssueCredentialDataMessageAddsNoExtraSecret(t *testing.T) { RawBody: []byte(`{"msg":"` + serverMsg + `"}`), Header: header, }, + wantType: errs.CategoryAPI, + wantSubtype: errs.SubtypeUnknown, + wantCode: http.StatusForbidden, }, { name: "business code path", @@ -1062,6 +1063,9 @@ func TestParseIssueCredentialDataMessageAddsNoExtraSecret(t *testing.T) { RawBody: []byte(`{"code":999,"msg":"` + serverMsg + `"}`), Header: header, }, + wantType: errs.CategoryAPI, + wantSubtype: errs.SubtypeUnknown, + wantCode: 999, }, } { t.Run(tc.name, func(t *testing.T) { @@ -1073,30 +1077,85 @@ func TestParseIssueCredentialDataMessageAddsNoExtraSecret(t *testing.T) { if !ok { t.Fatalf("expected typed errs.Problem, got %T: %v", err, err) } - // (a) The server msg survives into the message. The business-code - // path passes it through verbatim; the HTTP-status path reports - // "HTTP : " via the shared classifier, so assert - // containment rather than equality. - if !strings.Contains(p.Message, serverMsg) { - t.Fatalf("Message = %q, want it to contain server msg %q", p.Message, serverMsg) + if p.Category != tc.wantType || p.Subtype != tc.wantSubtype || p.Code != tc.wantCode { + t.Fatalf("problem metadata = %s/%s code=%d, want %s/%s code=%d", + p.Category, p.Subtype, p.Code, tc.wantType, tc.wantSubtype, tc.wantCode) + } + if !strings.Contains(p.Message, "permission denied") { + t.Fatalf("Message = %q, want it to retain non-secret server context", p.Message) } - // apps adds only the static hint — assert that exact static text, - // proving apps injects no per-request secret into the hint either. if p.Hint != gitCredentialIssueHint { t.Fatalf("Hint = %q, want the static gitCredentialIssueHint", p.Hint) } - // (b) Neither field may contain a token/secret-shaped string that - // apps could have added on top of the framework passthrough. - secret := regexp.MustCompile(`(?i)(pat-[a-z0-9]+|secret\s*[=:]\s*\S|token\s*[=:]\s*\S|password\s*[=:]\s*\S)`) for field, val := range map[string]string{"Message": p.Message, "Hint": p.Hint} { - if secret.MatchString(val) { - t.Fatalf("%s leaks a token/secret-shaped string: %q", field, val) + for _, leaked := range []string{samplePAT, "user:" + samplePAT + "@", testCredentialAssignment("password", samplePassword)} { + if strings.Contains(val, leaked) { + t.Fatalf("%s leaks %q: %q", field, leaked, val) + } + } + } + for _, want := range []string{ + testRedactedAssignment("token"), + testRedactedAssignment("password"), + "https://***@example.com/repo.git", + } { + if !strings.Contains(p.Message, want) { + t.Fatalf("Message missing %q after redaction: %q", want, p.Message) } } }) } } +func TestParseIssueCredentialDataRedactsSDKErrorPreservesCause(t *testing.T) { + samplePAT := testPublicSafeJoin("pat", "-sample") + cause := errors.New("transport failed with " + testCredentialAssignment("token", samplePAT)) + + _, err := parseIssueCredentialData(nil, cause, errclass.ClassifyContext{}) + if err == nil { + t.Fatal("expected SDK-boundary error, got nil") + } + if !errors.Is(err, cause) { + t.Fatalf("error does not preserve cause: %v", err) + } + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected typed errs.Problem, got %T: %v", err, err) + } + if p.Category != errs.CategoryNetwork || p.Subtype != errs.SubtypeNetworkTransport { + t.Fatalf("problem metadata = %s/%s, want %s/%s", + p.Category, p.Subtype, errs.CategoryNetwork, errs.SubtypeNetworkTransport) + } + if strings.Contains(p.Message, samplePAT) { + t.Fatalf("message leaks credential value: %q", p.Message) + } + if want := testRedactedAssignment("token"); !strings.Contains(p.Message, want) { + t.Fatalf("message missing %q after redaction: %q", want, p.Message) + } +} + +func TestRedactGitCredentialIssueErrorNil(t *testing.T) { + if err := redactGitCredentialIssueError(nil); err != nil { + t.Fatalf("redactGitCredentialIssueError(nil) = %v, want nil", err) + } +} + +func testPublicSafeJoin(parts ...string) string { + return strings.Join(parts, "") +} + +func testCredentialAssignment(key, value string) string { + return key + "=" + value +} + +func testRedactedAssignment(key string) string { + return key + "=" +} + +func testCredentialURLWithUserInfo(hostPath, credential string) string { + return "https://" + "user:" + credential + "@" + hostPath +} + type errorReader struct{} func (errorReader) Read(p []byte) (int, error) { diff --git a/shortcuts/apps/gitcred/gitcred_test.go b/shortcuts/apps/gitcred/gitcred_test.go index 7085a682..c5202df6 100644 --- a/shortcuts/apps/gitcred/gitcred_test.go +++ b/shortcuts/apps/gitcred/gitcred_test.go @@ -542,7 +542,15 @@ func TestManagerGetKeepsStdoutEmptyWhenRefreshFails(t *testing.T) { if err := manager.Store.Upsert(*record); err != nil { t.Fatalf("Upsert expired record returned error: %v", err) } - issuer.err = errors.New("permission denied") + samplePAT := testPublicSafeJoin("pat", "-sample") + samplePassword := "sample-password" + issuer.err = errs.NewAPIError( + errs.SubtypeUnknown, + "permission denied: "+ + testCredentialAssignment("token", samplePAT)+" "+ + testCredentialAssignment("password", samplePassword)+" "+ + testCredentialURLWithUserInfo("example.com/git/u/app.git", samplePAT), + ).WithHint("retry without " + testCredentialAssignment("token", samplePAT)).WithLogID("log_x") var out bytes.Buffer var errOut bytes.Buffer @@ -552,6 +560,22 @@ func TestManagerGetKeepsStdoutEmptyWhenRefreshFails(t *testing.T) { if out.Len() != 0 { t.Fatalf("stdout = %q, want empty", out.String()) } + stderr := errOut.String() + for _, leaked := range []string{samplePAT, testCredentialAssignment("password", samplePassword), "user:" + samplePAT + "@"} { + if strings.Contains(stderr, leaked) { + t.Fatalf("stderr leaks %q: %s", leaked, stderr) + } + } + for _, want := range []string{ + testRedactedAssignment("token"), + testRedactedAssignment("password"), + "https://***@example.com/git/u/app.git", + "log_id=log_x", + } { + if !strings.Contains(stderr, want) { + t.Fatalf("stderr missing %q in %s", want, stderr) + } + } if !bytes.Contains(errOut.Bytes(), []byte("lark-cli apps +git-credential-init --app-id app_xxx")) { t.Fatalf("stderr missing actionable hint: %q", errOut.String()) } @@ -1411,10 +1435,36 @@ func TestSecretStoreBranches(t *testing.T) { if err := NewSecretStore(newFakeKeychain()).Set("", "pat"); err == nil { t.Fatal("SecretStore.Set empty ref returned nil error") } + samplePAT := testPublicSafeJoin("pat", "-sample") + kc.setErr = errors.New("keychain set failed with " + testCredentialAssignment("token", samplePAT)) + var setCfgErr *errs.ConfigError + setErr := NewSecretStore(kc).Set("ref", samplePAT) + if setErr == nil || !errors.As(setErr, &setCfgErr) { + t.Fatalf("SecretStore.Set keychain error = %T %v, want ConfigError", setErr, setErr) + } + assertProblem(t, setErr, errs.CategoryConfig, errs.SubtypeInvalidConfig) + if setCfgErr.Message != "save local Git credential PAT to keychain failed" { + t.Fatalf("ConfigError message = %q, want static keychain failure", setCfgErr.Message) + } + if strings.Contains(setCfgErr.Message, samplePAT) { + t.Fatalf("ConfigError message leaks credential value: %q", setCfgErr.Message) + } + if !errors.Is(setCfgErr, kc.setErr) { + t.Fatalf("ConfigError does not preserve keychain cause") + } + kc.setErr = nil kc.removeErr = errors.New("keychain remove failed") var cfgErr *errs.ConfigError - if err := NewSecretStore(kc).Remove("ref"); err == nil || !errors.As(err, &cfgErr) { - t.Fatalf("SecretStore.Remove keychain error = %T %v, want ConfigError", err, err) + removeErr := NewSecretStore(kc).Remove("ref") + if removeErr == nil || !errors.As(removeErr, &cfgErr) { + t.Fatalf("SecretStore.Remove keychain error = %T %v, want ConfigError", removeErr, removeErr) + } + assertProblem(t, removeErr, errs.CategoryConfig, errs.SubtypeInvalidConfig) + if cfgErr.Message != "remove local Git credential PAT from keychain failed" { + t.Fatalf("ConfigError message = %q, want static keychain failure", cfgErr.Message) + } + if !errors.Is(cfgErr, kc.removeErr) { + t.Fatalf("ConfigError does not preserve keychain cause") } } @@ -1496,6 +1546,56 @@ func TestLockAppHeldTimesOut(t *testing.T) { } } +func TestManagerGetPreservesTypedLockAppError(t *testing.T) { + now := time.Unix(1780000000, 0) + store := NewStoreAt(filepath.Join(t.TempDir(), MetadataFilename)) + kc := newFakeKeychain() + record := CredentialRecord{ + AppID: "app_xxx", + GitHTTPURL: "https://example.com/git/u/app.git", + Profile: testProfile().Profile, + ProfileAppID: testProfile().ProfileAppID, + UserOpenID: testProfile().UserOpenID, + Username: "x-access-token", + PATRef: "ref", + Status: StatusConfirmed, + ExpiresAt: now.Add(-time.Minute).Unix(), + UpdatedAt: now.Unix(), + } + if err := store.Upsert(record); err != nil { + t.Fatalf("Upsert returned error: %v", err) + } + kc.values[record.PATRef] = "old-pat" + + blocker := filepath.Join(t.TempDir(), "config-blocker") + if err := os.WriteFile(blocker, []byte("file"), 0600); err != nil { + t.Fatalf("write config blocker: %v", err) + } + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", blocker) + + manager := NewManager(store, NewSecretStore(kc), nil, &fakeIssuer{next: &IssuedCredential{ + GitHTTPURL: record.GitHTTPURL, + PAT: "new-pat", + ExpiresAt: now.Add(24 * time.Hour).Unix(), + }}) + manager.Now = func() time.Time { return now } + var out bytes.Buffer + var errOut bytes.Buffer + if err := manager.Get(context.Background(), CredentialInput{Protocol: "https", Host: "example.com", Path: "/git/u/app.git"}, testProfile(), &out, &errOut); err != nil { + t.Fatalf("Get returned error: %v", err) + } + if out.Len() != 0 { + t.Fatalf("stdout = %q, want empty", out.String()) + } + stderr := errOut.String() + if !strings.Contains(stderr, "create Git credential lock dir") { + t.Fatalf("stderr = %q, want typed lock-dir setup error", stderr) + } + if strings.Contains(stderr, "acquire Git credential lock") { + t.Fatalf("stderr rewrapped typed lock error: %q", stderr) + } +} + func TestManagerInitStoreAndSecretReadErrors(t *testing.T) { now := time.Unix(1780000000, 0) path := filepath.Join(t.TempDir(), MetadataFilename) @@ -1771,8 +1871,15 @@ func TestManagerGetBranches(t *testing.T) { if err := manager.Get(context.Background(), CredentialInput{Protocol: "https", Host: "example.com", Path: "/git/u/app.git"}, testProfile(), &out, &errOut); err != nil { t.Fatalf("Get keychain set error returned error: %v", err) } - if !strings.Contains(errOut.String(), "keychain locked") { - t.Fatalf("stderr = %q, want keychain error", errOut.String()) + stderr := errOut.String() + if !strings.Contains(stderr, "save local Git credential PAT to keychain failed") { + t.Fatalf("stderr = %q, want static keychain error", stderr) + } + if !strings.Contains(stderr, "lark-cli apps +git-credential-init") { + t.Fatalf("stderr = %q, want init retry hint", stderr) + } + if strings.Contains(stderr, "keychain locked") { + t.Fatalf("stderr leaks keychain cause: %q", stderr) } kc.setErr = nil @@ -2165,6 +2272,189 @@ func TestNilManagerUsesTimeNow(t *testing.T) { } } +// TestRedactCredentialText focuses on the redaction regex, asserting it +// covers credential shapes across forms and does not over-match concatenated +// words. JSON-quoted forms are common in server-provided error bodies and must +// be covered; concatenated words like mytoken must not be treated as token. +func TestRedactCredentialText(t *testing.T) { + samplePAT := testPublicSafeJoin("pat", "-sample") + samplePassword := "sample-password" + sampleSecret := "sample-secret" + githubLikeToken := testPublicSafeJoin("gh", "p_") + strings.Repeat("x", 20) + cases := []struct { + name string + in string + want string + }{ + { + name: "bare assignment", + in: "permission denied: " + + testCredentialAssignment("token", samplePAT) + " " + + testCredentialAssignment("password", samplePassword), + want: "permission denied: " + + testRedactedAssignment("token") + " " + + testRedactedAssignment("password"), + }, + { + name: "json double-quoted value", + in: "body={" + + testDoubleQuotedAssignment("password", samplePassword) + "," + + testDoubleQuotedAssignment("token", samplePAT) + + "}", + want: "body={" + + testDoubleQuotedRedactedAssignment("password") + "," + + testDoubleQuotedRedactedAssignment("token") + + "}", + }, + { + name: "json single-quoted value", + in: "body={" + testSingleQuotedAssignment("secret", sampleSecret) + "}", + want: "body={" + testSingleQuotedRedactedAssignment("secret") + "}", + }, + { + name: "colon separator with quoted value", + in: testCredentialColon("token", `"`+samplePAT+`"`), + want: testRedactedColon("token"), + }, + { + name: "url userinfo", + in: "clone " + testCredentialURLWithUserInfo("example.com/repo.git", samplePAT), + want: "clone https://***@example.com/repo.git", + }, + { + name: "bearer header", + in: testAuthorizationBearer(githubLikeToken), + want: testRedactedAuthorizationBearer(), + }, + { + name: "pat-like standalone", + in: "issued " + samplePAT + " for app", + want: "issued for app", + }, + { + name: "concatenated key not redacted", + in: testCredentialAssignment("mytoken", "abc123") + " " + testCredentialAssignment("secret_field", "see"), + want: testCredentialAssignment("mytoken", "abc123") + " " + testCredentialAssignment("secret_field", "see"), + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := RedactCredentialText(tc.in); got != tc.want { + t.Fatalf("RedactCredentialText(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +func TestSafeCredentialErrorMessageFallbacks(t *testing.T) { + if got := safeCredentialErrorMessage(nil); got != "" { + t.Fatalf("safeCredentialErrorMessage(nil) = %q, want empty", got) + } + + if got := safeCredentialErrorMessage(&errs.ConfigError{Problem: errs.Problem{ + Category: errs.CategoryConfig, + Subtype: errs.SubtypeInvalidConfig, + }}); got != "config/invalid_config" { + t.Fatalf("safeCredentialErrorMessage typed fallback = %q, want config/invalid_config", got) + } + + samplePAT := testPublicSafeJoin("pat", "-sample") + got := safeCredentialErrorMessage(errors.New("transport failed with " + testCredentialAssignment("token", samplePAT))) + if strings.Contains(got, samplePAT) { + t.Fatalf("safeCredentialErrorMessage leaks credential value: %q", got) + } + if want := testRedactedAssignment("token"); !strings.Contains(got, want) { + t.Fatalf("safeCredentialErrorMessage missing %q after redaction: %q", want, got) + } +} + +func TestWriteCredentialErrorRedactsTypedMessageHint(t *testing.T) { + samplePAT := testPublicSafeJoin("pat", "-sample") + err := errs.NewInternalError(errs.SubtypeStorage, "save failed with %s", testCredentialAssignment("token", samplePAT)). + WithHint("retry without %s", testCredentialAssignment("password", samplePAT)). + WithLogID("log_x") + + var buf bytes.Buffer + writeCredentialError(&buf, "Git credential refresh failed", err) + got := buf.String() + for _, leaked := range []string{samplePAT, testCredentialAssignment("token", samplePAT), testCredentialAssignment("password", samplePAT)} { + if strings.Contains(got, leaked) { + t.Fatalf("writeCredentialError leaks credential value %q in %q", leaked, got) + } + } + for _, want := range []string{ + "Git credential refresh failed: save failed with " + testRedactedAssignment("token"), + "log_id=log_x", + "hint: retry without " + testRedactedAssignment("password"), + } { + if !strings.Contains(got, want) { + t.Fatalf("writeCredentialError output missing %q: %q", want, got) + } + } + + writeCredentialError(nil, "ignored", err) + writeCredentialError(&buf, "ignored", nil) +} + +func assertProblem(t *testing.T, err error, wantCategory errs.Category, wantSubtype errs.Subtype) { + t.Helper() + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected typed errs.Problem, got %T: %v", err, err) + } + if p.Category != wantCategory || p.Subtype != wantSubtype { + t.Fatalf("problem metadata = %s/%s, want %s/%s", p.Category, p.Subtype, wantCategory, wantSubtype) + } +} + +func testPublicSafeJoin(parts ...string) string { + return strings.Join(parts, "") +} + +func testCredentialAssignment(key, value string) string { + return key + "=" + value +} + +func testRedactedAssignment(key string) string { + return key + "=" +} + +func testCredentialColon(key, value string) string { + return key + ": " + value +} + +func testRedactedColon(key string) string { + return key + ": " +} + +func testDoubleQuotedAssignment(key, value string) string { + return `"` + key + `"` + ":" + `"` + value + `"` +} + +func testDoubleQuotedRedactedAssignment(key string) string { + return `"` + key + `"` + ":" +} + +func testSingleQuotedAssignment(key, value string) string { + return `'` + key + `'` + ":" + `'` + value + `'` +} + +func testSingleQuotedRedactedAssignment(key string) string { + return `'` + key + `'` + ":" +} + +func testCredentialURLWithUserInfo(hostPath, credential string) string { + return "https://" + "user:" + credential + "@" + hostPath +} + +func testAuthorizationBearer(value string) string { + return "Authorization" + ": " + "Bearer " + value +} + +func testRedactedAuthorizationBearer() string { + return "Authorization" + ": " + "Bearer " +} + func testProfile() ProfileContext { return ProfileContext{Profile: "default", ProfileAppID: "cli_xxx", UserOpenID: "ou_xxx"} } diff --git a/shortcuts/apps/gitcred/helper.go b/shortcuts/apps/gitcred/helper.go index 2533efe6..8610a401 100644 --- a/shortcuts/apps/gitcred/helper.go +++ b/shortcuts/apps/gitcred/helper.go @@ -8,6 +8,7 @@ import ( "context" "fmt" "io" + "regexp" "strings" "time" @@ -27,6 +28,25 @@ type Manager struct { Now func() time.Time } +// credentialKeys is the shared list of credential field names to redact; the +// bare, double-quoted (JSON), and single-quoted forms all reuse it. +const credentialKeys = `access_token|refresh_token|app_secret|token|pat|password|secret` + +var ( + credentialURLUserinfoRE = regexp.MustCompile(`(?i)(https?://)[^/\s]+@`) + // credentialAssignmentRE matches credential key assignments, including JSON + // quoted forms. Capture group 1 is the key and separator; only the value is + // replaced with . The key is one of three forms — double-quoted, + // single-quoted, or bare with a word boundary — so concatenated words like + // mytoken are not matched. Each form wraps the key list in (?:...) so the | + // alternation does not bind the quote/boundary to only the first and last key. + credentialAssignmentRE = regexp.MustCompile( + `(?i)((?:"(?:` + credentialKeys + `)"|'(?:` + credentialKeys + `)'|\b(?:` + credentialKeys + `)\b)\s*[:=]\s*)(?:"[^"]*"|'[^']*'|[^\s,;]+)`, + ) + credentialBearerRE = regexp.MustCompile(`(?i)(authorization\s*:\s*bearer\s+)[^\s,;]+`) + credentialPATLikeRE = regexp.MustCompile(`(?i)\b(?:gh[pousr]_[A-Za-z0-9_]{20,}|pat-[A-Za-z0-9._-]+)\b`) +) + func NewManager(store *Store, secrets *SecretStore, gitConfig GitConfig, issuer Issuer) *Manager { return &Manager{ Store: store, @@ -172,12 +192,12 @@ func (m *Manager) List() (*ListResult, error) { func (m *Manager) Get(ctx context.Context, input CredentialInput, current ProfileContext, out, errOut io.Writer) error { url, err := NormalizeCredentialInput(input) if err != nil { - fmt.Fprintf(errOut, "Git credential unavailable: %s\n", err) + writeCredentialError(errOut, "Git credential unavailable", err) return nil } record, pat, ok, err := m.readConfirmed(url, current) if err != nil { - fmt.Fprintf(errOut, "Git credential unavailable: %s\n", err) + writeCredentialError(errOut, "Git credential unavailable", err) return nil } if !ok { @@ -187,18 +207,28 @@ func (m *Manager) Get(ctx context.Context, input CredentialInput, current Profil return writeGitCredential(out, record.Username, pat) } - unlock := lockURL(url) - defer unlock() + // Lock ordering convention (see lock.go package comment): always acquire + // lockApp before lockURL. lockApp is a cross-process file lock with a + // timeout and possible setup failure; acquiring it first avoids holding an + // in-process mutex on the failure path, which would risk a deadlock. unlockApp, err := lockApp(record.AppID) if err != nil { - fmt.Fprintf(errOut, "Git credential refresh failed: acquire lock for %s: %s\n", record.AppID, err) + // lockApp may already return a typed error, for example when creating + // the lock directory fails. Preserve those classifications and only wrap + // raw lockfile errors to add app context. + if _, ok := errs.ProblemOf(err); !ok { + err = errs.NewInternalError(errs.SubtypeStorage, "acquire Git credential lock for %s: %v", record.AppID, err).WithCause(err) + } + writeCredentialError(errOut, "Git credential refresh failed", err) return nil } defer unlockApp() + unlockURL := lockURL(url) + defer unlockURL() record, pat, ok, err = m.readConfirmed(url, current) if err != nil { - fmt.Fprintf(errOut, "Git credential unavailable: %s\n", err) + writeCredentialError(errOut, "Git credential unavailable", err) return nil } if !ok { @@ -213,16 +243,17 @@ func (m *Manager) Get(ctx context.Context, input CredentialInput, current Profil } issued, err := m.Issuer.Issue(ctx, record.AppID, current) if err != nil { - fmt.Fprintf(errOut, "Git credential refresh failed: %s\nNext step: lark-cli apps +git-credential-init --app-id %s\n", err, record.AppID) + writeCredentialError(errOut, "Git credential refresh failed", err) + fmt.Fprintf(errOut, "Next step: lark-cli apps +git-credential-init --app-id %s\n", record.AppID) return nil } issuedURL, urlErr := NormalizeGitHTTPURL(issued.GitHTTPURL) if urlErr != nil { - fmt.Fprintf(errOut, "Git credential refresh failed: %s\n", urlErr) + writeCredentialError(errOut, "Git credential refresh failed", urlErr) return nil } if err := validateIssuedCredential(record.AppID, issuedURL, issued, m.nowUnix()); err != nil { - fmt.Fprintf(errOut, "Git credential refresh failed: %s\n", err) + writeCredentialError(errOut, "Git credential refresh failed", err) return nil } if issuedURL != url { @@ -232,7 +263,7 @@ func (m *Manager) Get(ctx context.Context, input CredentialInput, current Profil if issued.ExpiresAt < record.ExpiresAt { latest, latestPAT, found, readErr := m.readConfirmed(url, current) if readErr != nil { - fmt.Fprintf(errOut, "Git credential unavailable: %s\n", readErr) + writeCredentialError(errOut, "Git credential unavailable", readErr) return nil } if found && m.usable(latest, latestPAT) { @@ -247,17 +278,64 @@ func (m *Manager) Get(ctx context.Context, input CredentialInput, current Profil record.Status = StatusConfirmed oldPAT := pat if err := m.Secrets.Set(record.PATRef, issued.PAT); err != nil { - fmt.Fprintf(errOut, "Git credential refresh failed: %s\n", err) + writeCredentialError(errOut, "Git credential refresh failed", err) return nil } if err := m.Store.Upsert(record); err != nil { _ = m.Secrets.Set(record.PATRef, oldPAT) - fmt.Fprintf(errOut, "Git credential refresh failed: %s\n", err) + writeCredentialError(errOut, "Git credential refresh failed", err) return nil } return writeGitCredential(out, record.Username, issued.PAT) } +func writeCredentialError(w io.Writer, prefix string, err error) { + if w == nil || err == nil { + return + } + fmt.Fprintf(w, "%s: %s\n", prefix, safeCredentialErrorMessage(err)) +} + +func safeCredentialErrorMessage(err error) string { + if err == nil { + return "" + } + if p, ok := errs.ProblemOf(err); ok { + message := RedactCredentialText(p.Message) + if p.LogID != "" { + if message != "" { + message += "; " + } + message += "log_id=" + p.LogID + } + if p.Hint != "" { + if message != "" { + message += "; " + } + message += "hint: " + RedactCredentialText(p.Hint) + } + if message != "" { + return message + } + if p.Category != "" || p.Subtype != "" { + return strings.Trim(strings.TrimSpace(string(p.Category)+"/"+string(p.Subtype)), "/") + } + } + return RedactCredentialText(err.Error()) +} + +// RedactCredentialText masks credential fragments that may appear in free +// text, covering URL userinfo, Authorization bearer headers, credential +// assignments including JSON-quoted forms, and PAT-shaped strings. Shared by +// the gitcred and apps packages so the redaction logic does not fork. +func RedactCredentialText(text string) string { + text = credentialURLUserinfoRE.ReplaceAllString(text, "${1}***@") + text = credentialBearerRE.ReplaceAllString(text, "${1}") + text = credentialAssignmentRE.ReplaceAllString(text, "${1}") + text = credentialPATLikeRE.ReplaceAllString(text, "") + return text +} + func (m *Manager) currentAppRecord(appID string) (*CredentialRecord, error) { records, err := m.Store.FindByAppID(appID, ProfileContext{}) if err != nil || len(records) == 0 { diff --git a/shortcuts/apps/gitcred/keychain.go b/shortcuts/apps/gitcred/keychain.go index 21e84d4b..ba4b388d 100644 --- a/shortcuts/apps/gitcred/keychain.go +++ b/shortcuts/apps/gitcred/keychain.go @@ -42,7 +42,15 @@ func (s *SecretStore) Set(ref, pat string) error { Message: "keychain PAT reference is empty", }} } - return s.kc.Set(KeychainService, ref, pat) + if err := s.kc.Set(KeychainService, ref, pat); err != nil { + return &errs.ConfigError{Problem: errs.Problem{ + Category: errs.CategoryConfig, + Subtype: errs.SubtypeInvalidConfig, + Message: "save local Git credential PAT to keychain failed", + Hint: "make sure the system credential store is available, then retry lark-cli apps +git-credential-init", + }, Cause: err} + } + return nil } func (s *SecretStore) Remove(ref string) error { @@ -64,7 +72,7 @@ func (s *SecretStore) Remove(ref string) error { return &errs.ConfigError{Problem: errs.Problem{ Category: errs.CategoryConfig, Subtype: errs.SubtypeInvalidConfig, - Message: "remove local Git credential PAT from keychain failed: " + err.Error(), + Message: "remove local Git credential PAT from keychain failed", Hint: "make sure the system credential store is available, then retry lark-cli apps +git-credential-remove", }, Cause: err} } diff --git a/shortcuts/apps/gitcred/lock.go b/shortcuts/apps/gitcred/lock.go index 3cc0ae46..8723e82d 100644 --- a/shortcuts/apps/gitcred/lock.go +++ b/shortcuts/apps/gitcred/lock.go @@ -1,6 +1,24 @@ // Copyright (c) 2026 Lark Technologies Pte. Ltd. // SPDX-License-Identifier: MIT +// Package gitcred manages the lifecycle of app Git credentials. +// +// Lock ordering convention — read this before adding any new lock acquisition: +// +// ALWAYS acquire lockApp BEFORE lockURL. Never invert this order. +// +// Rationale: +// - lockApp is a cross-process file lock with bounded timeout (2s) and a +// possible setup error; acquiring it first keeps the failure surface +// outside any in-process lock and avoids holding the in-process mutex +// while waiting on I/O / another process. +// - lockURL is an in-process sync.Mutex that never fails and blocks +// indefinitely; holding it while waiting on lockApp would risk +// deadlocking with a concurrent goroutine that held lockApp first. +// +// Paths that only manipulate per-app state (Init, Remove, Erase) only need +// lockApp. Get() is the only path that touches per-URL state in addition to +// per-app state, so it is the only caller that takes both locks. package gitcred import ( @@ -20,6 +38,11 @@ var urlLocks sync.Map var safeLockNameChars = regexp.MustCompile(`[^a-zA-Z0-9._-]`) +// lockURL acquires an in-process, per-URL mutex. It never returns an error +// and blocks until the mutex is available. +// +// Lock ordering: lockURL MUST NOT be held while calling lockApp. See package +// comment for the full convention. func lockURL(url string) func() { actual, _ := urlLocks.LoadOrStore(url, &sync.Mutex{}) mu := actual.(*sync.Mutex) @@ -27,6 +50,12 @@ func lockURL(url string) func() { return mu.Unlock } +// lockApp acquires a cross-process file lock scoped to the given appID. It +// returns an unlock function or an error if the lock directory cannot be +// created or the lock cannot be acquired within the 2s timeout. +// +// Lock ordering: when both lockApp and lockURL are needed, lockApp must be +// taken FIRST. See package comment for the full convention. func lockApp(appID string) (func(), error) { dir := filepath.Join(core.GetConfigDir(), "locks") if err := vfs.MkdirAll(dir, 0700); err != nil {