fix: harden git credential error handling#1676
Conversation
github-actions
Bot
added
the
size/M
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds credential redaction to git credential errors, wraps keychain failures with structured causes, documents lock ordering, raises cleanup timeout, and centralizes Drive cleanup deletion through a retry-aware helper. ChangesCredential Error Redaction
Drive Cleanup Retry Verification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1676 +/- ##
==========================================
+ Coverage 74.73% 74.74% +0.01%
==========================================
Files 811 811
Lines 81965 82011 +46
==========================================
+ Hits 61258 61302 +44
- Misses 16138 16139 +1
- Partials 4569 4570 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@d2e8fe37ff65ca9bd441dda916cb380946f7040e🧩 Skill updatenpx skills add larksuite/cli#fix/gitcred-credential-safety -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/apps/git_credential_test.go (1)
1062-1083: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the typed classification as well as the redaction.
This test proves the message is scrubbed, but it would still pass if
parseIssueCredentialDatachanged category/subtype on these error paths. Please lock in the expectederrs.ProblemOf(err)metadata too, and preserve/capture cause where that branch wraps one. As per coding guidelines**/*_test.go: Error-path tests must assert typed metadata viaerrs.ProblemOf(category/subtype/param) and cause preservation, not message substrings alone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/apps/git_credential_test.go` around lines 1062 - 1083, The test currently verifies redaction in the error message but does not lock in the typed error metadata for the `parseIssueCredentialData` error path. Update this `git_credential_test.go` case to assert the full `errs.ProblemOf(err)` classification for the returned error, including the expected category, subtype, and any param values, and also verify cause preservation where that branch wraps an underlying error. Keep the existing redaction checks, but make the test fail if `parseIssueCredentialData` changes its typed metadata or drops the wrapped cause.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/apps/gitcred/gitcred_test.go`:
- Around line 1428-1453: The keychain error-path tests in NewSecretStore.Set and
NewSecretStore.Remove only check the wrapped ConfigError type and cause, so they
can miss a metadata regression. Update these assertions to also verify
errs.ProblemOf(err) for the expected config classification on both branches,
alongside the existing cause-preservation checks, using the Set/Remove failure
paths in gitcred_test.go to locate the test cases.
In `@shortcuts/apps/gitcred/helper.go`:
- Around line 216-223: In the Git credential helper flow, stop unconditionally
wrapping the error returned by lockApp; that code path is downgrading
already-typed errs.InternalError values from the lower layer. Update the error
handling so the app-ID wrapper via errs.NewInternalError is applied only when
lockApp returns an untyped lockfile error, and pass through existing typed
errors unchanged before calling writeCredentialError.
---
Outside diff comments:
In `@shortcuts/apps/git_credential_test.go`:
- Around line 1062-1083: The test currently verifies redaction in the error
message but does not lock in the typed error metadata for the
`parseIssueCredentialData` error path. Update this `git_credential_test.go` case
to assert the full `errs.ProblemOf(err)` classification for the returned error,
including the expected category, subtype, and any param values, and also verify
cause preservation where that branch wraps an underlying error. Keep the
existing redaction checks, but make the test fail if `parseIssueCredentialData`
changes its typed metadata or drops the wrapped cause.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69d7bab9-b543-4c31-b6c5-7aec835ede7a
📒 Files selected for processing (6)
shortcuts/apps/git_credential.goshortcuts/apps/git_credential_test.goshortcuts/apps/gitcred/gitcred_test.goshortcuts/apps/gitcred/helper.goshortcuts/apps/gitcred/keychain.goshortcuts/apps/gitcred/lock.go
a991ae8 to
a73f81f
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/apps/git_credential_test.go (1)
1067-1076: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlso assert the preserved typed metadata (category/subtype), not just
Hint.This is the boundary that redacts a server message while it must "preserve typed error metadata." The test confirms a typed
Problemexists and checksHint, but never asserts thecategory/subtype, so a classification downgrade during redaction would slip through. Add subtype/category assertions onpto lock the preserved classification.As per coding guidelines (
**/*_test.go): "Error-path tests must assert typed metadata viaerrs.ProblemOf(category/subtype/param) and cause preservation, not message substrings alone."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/apps/git_credential_test.go` around lines 1067 - 1076, The test in gitCredentialError handling currently checks only that errs.ProblemOf returns a Problem and that Hint/Message are preserved; extend it to assert the preserved typed metadata on p as well. Add explicit assertions for the category and subtype fields returned by errs.ProblemOf in this test so redaction cannot silently downgrade classification, while keeping the existing Hint and message checks in place.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@shortcuts/apps/git_credential_test.go`:
- Around line 1067-1076: The test in gitCredentialError handling currently
checks only that errs.ProblemOf returns a Problem and that Hint/Message are
preserved; extend it to assert the preserved typed metadata on p as well. Add
explicit assertions for the category and subtype fields returned by
errs.ProblemOf in this test so redaction cannot silently downgrade
classification, while keeping the existing Hint and message checks in place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5c0877e-b903-40bf-b878-8ae88ee78b0b
📒 Files selected for processing (6)
shortcuts/apps/git_credential.goshortcuts/apps/git_credential_test.goshortcuts/apps/gitcred/gitcred_test.goshortcuts/apps/gitcred/helper.goshortcuts/apps/gitcred/keychain.goshortcuts/apps/gitcred/lock.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/apps/gitcred/lock.go
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/apps/gitcred/keychain.go
- shortcuts/apps/git_credential.go
- shortcuts/apps/gitcred/helper.go
0cc49f4 to
0a8303b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/apps/git_credential_test.go`:
- Around line 1090-1096: The leak-check in the git credential test only covers
the formatted password assignment string, so it can miss a raw password leak.
Tighten the assertions around the existing field scan over p.Message and p.Hint
by also checking that the plain samplePassword value itself is absent, alongside
the existing PAT and credential-form checks, so the test catches any regression
where the raw secret appears without the key=value wrapper.
In `@shortcuts/apps/gitcred/gitcred_test.go`:
- Around line 1599-1643: The test setup in
TestManagerGetWrapsUntypedLockAppError is using the default config directory
when calling lockApp(record.AppID), which can fail for unrelated reasons before
the intended ErrHeld path is exercised. Set an isolated config dir at the start
of the test with t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) so lockApp,
NewManager, and Get operate against a clean per-test state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 693f3104-8095-4188-9cf0-e767275096c7
📒 Files selected for processing (15)
shortcuts/apps/git_credential.goshortcuts/apps/git_credential_test.goshortcuts/apps/gitcred/gitcred_test.goshortcuts/apps/gitcred/helper.goshortcuts/apps/gitcred/keychain.goshortcuts/apps/gitcred/lock.gotests/cli_e2e/core.gotests/cli_e2e/drive/drive_add_comment_workflow_test.gotests/cli_e2e/drive/drive_preview_workflow_test.gotests/cli_e2e/drive/drive_status_workflow_test.gotests/cli_e2e/drive/drive_sync_workflow_test.gotests/cli_e2e/drive/drive_upload_workflow_test.gotests/cli_e2e/drive/drive_version_workflow_test.gotests/cli_e2e/drive/helpers.gotests/cli_e2e/drive/helpers_test.go
✅ Files skipped from review due to trivial changes (2)
- tests/cli_e2e/drive/drive_sync_workflow_test.go
- shortcuts/apps/gitcred/lock.go
🚧 Files skipped from review as they are similar to previous changes (11)
- tests/cli_e2e/drive/drive_version_workflow_test.go
- tests/cli_e2e/drive/drive_upload_workflow_test.go
- tests/cli_e2e/drive/drive_add_comment_workflow_test.go
- tests/cli_e2e/core.go
- shortcuts/apps/gitcred/keychain.go
- tests/cli_e2e/drive/drive_preview_workflow_test.go
- tests/cli_e2e/drive/drive_status_workflow_test.go
- tests/cli_e2e/drive/helpers_test.go
- shortcuts/apps/git_credential.go
- tests/cli_e2e/drive/helpers.go
- shortcuts/apps/gitcred/helper.go
a8d1252 to
0cc49f4
Compare
0cc49f4 to
d2e8fe3
Compare
2 participants
Summary
Improve the safety of the app Git credential flow by ensuring credential-shaped values from API, keychain, and helper refresh errors are not surfaced in user-visible output.
Changes
Test Plan
make unit-testlark-cli apps +git-credential-init,+git-credential-list, Git credential helper, andgit ls-remoteflow works as expectedgo test -race ./shortcuts/apps/gitcred ./shortcuts/apps -count=1go vet ./shortcuts/apps/gitcred ./shortcuts/appsgofmt -lon changed Go files produced no outputgo run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/mainRelated Issues
Summary by CodeRabbit