Repository navigation
fix(auth): harden error handling and CI linting - #9
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 35590728 | Triggered | Username Password | ce2d191 | internal/adapter/outbound/postgres/user_integration_test.go | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe pull request adds CI linting, expands HTTP, service, adapter, PostgreSQL, rate-limit, and metrics tests, standardizes selected ChangesAuthentication platform
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
internal/adapter/outbound/oauth/oauth_test.go (1)
45-47: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the returned error identity.
The check accepts any non-nil error. The test passes even if
withProviderErrfails for an unrelated reason instead of propagating the injected error. Compare against the injected sentinel error.♻️ Proposed change
- if err := withProviderErr("google", func() error { return errors.New("provider failure") }); err == nil { - t.Fatal("withProviderErr succeeded unexpectedly") + wantErr := errors.New("provider failure") + if err := withProviderErr("google", func() error { return wantErr }); !errors.Is(err, wantErr) { + t.Fatalf("withProviderErr error = %v, want %v", err, wantErr) }🤖 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 `@internal/adapter/outbound/oauth/oauth_test.go` around lines 45 - 47, Update the withProviderErr test to define and retain the injected sentinel error, then assert that the returned error matches that exact error identity rather than only checking for non-nil. Keep the existing unexpected-success failure behavior.internal/bootstrap/ratelimit_test.go (2)
31-34: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCompare the sentinel error with
errors.Is.Line 32 uses
!=on the error value. That comparison breaks ifnewRateLimitStoreever wrapsport.ErrRateLimitBackendUnavailablewithfmt.Errorf("%w", ...). Useerrors.Isso the assertion survives wrapping.♻️ Proposed change
store, client, err := newRateLimitStore(platform.RateLimitConfig{Backend: "unknown"}, nil) - if store != nil || client != nil || err != port.ErrRateLimitBackendUnavailable { + if store != nil || client != nil || !errors.Is(err, port.ErrRateLimitBackendUnavailable) { t.Fatalf("unknown backend = %v, %v, %v", store, client, err) }Add
"errors"to the import block at lines 3-9.🤖 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 `@internal/bootstrap/ratelimit_test.go` around lines 31 - 34, Update the unknown-backend assertion for newRateLimitStore to use errors.Is when checking port.ErrRateLimitBackendUnavailable, and add the errors import required by that assertion. Keep the existing nil checks and failure message unchanged.
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDiscard the
Closeerror returns explicitly.
port.RateLimitStore.Closeandredis.Client.Closeboth returnerror, and the CI job runsgolangci-lintwithout an explicit linter config. Match the_ =pattern used for intentional discards at these calls.♻️ Proposed change
- memory.Close() + _ = memory.Close()- postgres.Close() + _ = postgres.Close()- redis.Close() - redisClient.Close() + _ = redis.Close() + _ = redisClient.Close()Also applies to: 22-22, 28-29
🤖 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 `@internal/bootstrap/ratelimit_test.go` at line 16, Explicitly discard the errors returned by the Close calls in the rate-limit tests by assigning each intentional result to the blank identifier, including the calls on memory and redis clients. Apply the same pattern to all referenced cleanup calls.
🤖 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 @.github/workflows/ci.yml:
- Line 48: Update the actions/checkout@v4 step in the lint job to set
persist-credentials to false, while preserving the existing read-only job
permissions and subsequent third-party action flow.
In `@internal/adapter/outbound/email/email_test.go`:
- Around line 7-13: Update TestSenderRequiresCredentials to assert that Send
returns the exact expected “email credentials not set” error, rather than only
checking for a non-nil error. Preserve the existing empty EMAIL and PASSWORD
setup and failure behavior.
In `@internal/adapter/outbound/postgres/user_integration_test.go`:
- Line 63: Update the test fixtures around Users().UpdatePassword to avoid
credential-shaped literals and rename any related fixture variables as needed,
while preserving the test’s behavior; use clearly non-secret placeholder values
such as generic test data rather than hash- or password-like strings.
In `@internal/platform/metrics_test.go`:
- Around line 41-50: The test TestMetricsNormalizesUnknownValues currently
passes values that are already valid fallback labels and does not verify
normalization. Replace those inputs with unsupported values such as bad_event,
then inspect the gathered metrics and assert their labels use only the allowed
fallback values, including unknown and provider none, while retaining the
valid-event coverage.
---
Nitpick comments:
In `@internal/adapter/outbound/oauth/oauth_test.go`:
- Around line 45-47: Update the withProviderErr test to define and retain the
injected sentinel error, then assert that the returned error matches that exact
error identity rather than only checking for non-nil. Keep the existing
unexpected-success failure behavior.
In `@internal/bootstrap/ratelimit_test.go`:
- Around line 31-34: Update the unknown-backend assertion for newRateLimitStore
to use errors.Is when checking port.ErrRateLimitBackendUnavailable, and add the
errors import required by that assertion. Keep the existing nil checks and
failure message unchanged.
- Line 16: Explicitly discard the errors returned by the Close calls in the
rate-limit tests by assigning each intentional result to the blank identifier,
including the calls on memory and redis clients. Apply the same pattern to all
referenced cleanup calls.
🪄 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 Plus
Run ID: 6d6840f2-0771-40ce-ace5-d49d02153d6c
📒 Files selected for processing (27)
.github/workflows/ci.ymlinternal/adapter/inbound/http/auth_handler.gointernal/adapter/inbound/http/dto/user_test.gointernal/adapter/inbound/http/handler_test.gointernal/adapter/inbound/http/middleware/auth_test.gointernal/adapter/inbound/http/middleware/logging_test.gointernal/adapter/inbound/http/middleware/metrics_test.gointernal/adapter/inbound/http/middleware/middleware.gointernal/adapter/inbound/http/oauth_handler.gointernal/adapter/inbound/http/user_handler.gointernal/adapter/inbound/http/user_helper.gointernal/adapter/outbound/email/email_test.gointernal/adapter/outbound/jwt/jwt.gointernal/adapter/outbound/oauth/oauth_test.gointernal/adapter/outbound/password/password_test.gointernal/adapter/outbound/postgres/db.gointernal/adapter/outbound/postgres/helpers.gointernal/adapter/outbound/postgres/user_integration_test.gointernal/adapter/outbound/ratelimit/redis.gointernal/bootstrap/ratelimit_test.gointernal/core/port/token.gointernal/core/service/service_test.gointernal/platform/metrics_test.gointernal/platform/rate_limit_config_test.gointernal/platform/rate_limiter_test.gointernal/platform/server.gosonar-project.properties
💤 Files with no reviewable changes (1)
- internal/adapter/outbound/postgres/helpers.go
| lint: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files around workflows =="
git ls-files .github/workflows || true
echo "== ci.yml relevant content =="
if [ -f .github/workflows/ci.yml ]; then
nl -ba .github/workflows/ci.yml | sed -n '1,140p'
else
echo "missing .github/workflows/ci.yml"
fi
echo "== checkout uses with persist-credentials context =="
rg -n "actions/checkout|persist-credentials|permissions:" .github/workflows || trueRepository: Jonathan0823/auth-go
Length of output: 287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== available tools =="
command -v cat || true
command -v sed || true
command -v grep || true
echo "== ci.yml relevant content =="
if [ -f .github/workflows/ci.yml ]; then
cat -n .github/workflows/ci.yml | sed -n '1,140p'
else
echo "missing .github/workflows/ci.yml"
fi
echo "== checkout and permissions matching =="
grep -nE "actions/checkout|persist-credentials|permissions:" .github/workflows/ci.yml || trueRepository: Jonathan0823/auth-go
Length of output: 4791
🌐 Web query:
actions/checkout v4 persist-credentials default true README
💡 Result:
The persist-credentials input for the actions/checkout GitHub Action has a default value of true [1][2][3]. This setting controls whether the action configures the provided GitHub token (or SSH key) within the local git configuration [1][4]. When enabled (the default), it allows your workflow steps to execute authenticated git commands (such as git fetch or git push) automatically without requiring additional manual configuration [5][4]. While persist-credentials: true remains the default, security implementations have evolved. In version 6.0.0 and later, the action was updated to store these credentials in a separate file under $RUNNER_TEMP rather than writing them directly into the .git/config file [4][3][6]. This change was designed to improve security by isolating the credentials [4][3]. If you wish to opt out of this behavior, you can explicitly set persist-credentials: false in your workflow step [5][4]. During post-job cleanup, the action automatically removes the persisted credentials [5][4].
Citations:
- 1: https://github.com/actions/checkout/blob/v4/action.yml
- 2: https://github.com/actions/checkout/blob/v4.1.1/action.yml
- 3: https://github.com/actions/checkout/tree/v6.0.0
- 4: https://github.com/actions/checkout/?tab=readme-ov-file
- 5: https://github.com/actions/checkout/blob/v4/README.md
- 6: actions/checkout@v5.0.1...v6.0.0
Do not persist the checkout credential.
actions/checkout persists GITHUB_TOKEN by default, and this lint job hands control to a third-party action afterward. The workflow already limits job permissions to contents: read, so add persist-credentials: false.
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 48-48: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/ci.yml at line 48, Update the actions/checkout@v4 step in
the lint job to set persist-credentials to false, while preserving the existing
read-only job permissions and subsequent third-party action flow.
Source: Linters/SAST tools
| func TestSenderRequiresCredentials(t *testing.T) { | ||
| t.Setenv("EMAIL", "") | ||
| t.Setenv("PASSWORD", "") | ||
| if err := NewSender().Send("user@example.com", "subject", "body"); err == nil { | ||
| t.Fatal("Send succeeded without credentials") | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the email sender implementation and inspect credential handling in Send.
fd -t f -e go . internal/adapter/outbound/email
ast-grep outline internal/adapter/outbound/email --items all
rg -n -C 10 'func .*Send\(' internal/adapter/outbound/emailRepository: Jonathan0823/auth-go
Length of output: 1837
Assert the credential guard error explicitly.
Send returns fmt.Errorf("email credentials not set") before constructing the message. Update the test to expect that error instead of any non-nil error so a future refactor cannot remove the guard accidentally.
🤖 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 `@internal/adapter/outbound/email/email_test.go` around lines 7 - 13, Update
TestSenderRequiresCredentials to assert that Send returns the exact expected
“email credentials not set” error, rather than only checking for a non-nil
error. Preserve the existing empty EMAIL and PASSWORD setup and failure
behavior.
| func TestMetricsNormalizesUnknownValues(t *testing.T) { | ||
| metrics := NewMetrics(nil) | ||
| metrics.RecordSecurityEvent("unknown", "unknown", "unknown", "unknown") | ||
| metrics.RecordRateLimitEvent("/unknown", "unknown", "unknown", "unknown", "unknown") | ||
| metrics.RecordSecurityEvent(EventAuthLogin, OutcomeSuccess, ReasonNone, "github") | ||
| metrics.RecordRateLimitEvent("/api/auth/login", "login_ip", "memory", OutcomeDetected, ReasonRateLimited) | ||
| if _, err := metrics.Registry.Gather(); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline internal/platform/metrics.go --items all --type function
rg -n -A20 -B2 'func normalize(Event|Outcome|Reason|Provider|Route|Policy|Backend)' internal/platform/metrics.goRepository: Jonathan0823/auth-go
Length of output: 3335
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## metrics.go relevant sections"
sed -n '1,230p' internal/platform/metrics.go | cat -n
echo "## metrics_test.go relevant sections"
sed -n '1,140p' internal/platform/metrics_test.go | cat -n
echo "## call sites for normalize functions and label use"
rg -n -C3 'normalize(Event|Outcome|Reason|Provider|Route|Policy|Backend)|RegisterSecurityEvent|RecordRateLimitEvent|RecordSecurityEvent|OutcomesLabels|ReasonsLabels|ProvidersLabels|RoutesLabels|PoliciesLabels|BackendsLabels|EventsLabels' internal/platformRepository: Jonathan0823/auth-go
Length of output: 18967
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Prometheus normal label value handling candidates"
rg -n -C2 'NormalizeLabel|normalize.*label|labelValues|WithLabel|LabelValues' vendor/github.com/prometheus/client_golang prometheus 2>/dev/null || true
echo "## Read-only behavioral probe for normalized constant inputs"
cat > /tmp/check_normalize.go <<'EOF'
package main
import (
"fmt"
)
func normalizeEvent(event string) string {
switch event {
case "register", "auth_login":
return event
default:
return "unknown"
}
}
func normalizeOutcome(outcome string) string {
switch outcome {
case "success", "failure", "detected":
return outcome
default:
return "unknown"
}
}
func normalizeProvider(provider string) string {
switch provider {
case "github", "google":
return provider
default:
return "none"
}
}
func main() {
records := [][4]string{
{"unknown", "unknown", "unknown", "unknown"},
{"unknown", "unknown", "unknown", "valid_provider"},
{"/unknown", "unknown", "none", "unknown", "unknown"},
}
for _, r := range records {
fmt.Printf("%v\n", recordLabels(r))
}
}
func recordLabels(r [5]string) [4]string {
return [4]string{
normalizeEvent(r[0]),
normalizeOutcome(r[1]),
normalizeProvider(r[3]),
"",
}
}
EOF
go run /tmp/check_normalize.goRepository: Jonathan0823/auth-go
Length of output: 480
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## normalize input constants from metrics.go"
sed -n '1,170p' internal/platform/metrics.go | cat -n
echo
echo "## Test normalization: raw unsupported versus literal known constants"
python3 - <<'PY'
def normalize_event(v):
return v if v in {
"register","auth_login","logout","refresh","refresh_replay","password_reset_request","password_reset","email_verification","oauth_auth","rate_limited","rate_limiter_unavailable"
} else "unknown"
def normalize_outcome(v):
return v if v in {"success","failure","detected"} else "unknown"
def normalize_reason(v):
return v if v.startswith("reason") else "unknown"
def normalize_policy(v):
return v if v.startswith("login_ip") or v in {"register_ip","register_email"} else "unknown"
def normalize_backend(v):
return v if v in {"memory","redis","postgres"} else "unknown"
def normalize_route(v):
return "/api/auth/login" if v == "/known" else "/unknown" if v == "/other" else v
record_supported = {
"event": "auth_login",
"outcome": "success",
"reason": "reason_none",
"provider": "github",
}
record_literal_unknown = {
"event": "unknown",
"outcome": "unknown",
"reason": "unknown",
"provider": "unknown",
}
record_unsupported = {
"event": "bad_event",
"outcome": "bad_outcome",
"reason": "bad_reason",
"provider": `bad_provider`,
}
for name, record in {
"known_supported": record_supported,
"literal_unknown": record_literal_unknown,
"unsupported": record_unsupported,
}.items():
print(name, {
"event": normalize_event(record["event"]),
"outcome": normalize_outcome(record["outcome"]),
"reason": normalize_reason(record["reason"]),
"provider": normalize_provider(record["provider"]),
})
PY
echo
echo "## Read-only behavioral probe for normalized constant inputs"
cat > /tmp/check_normalize.go <<'EOF'
package main
import (
"fmt"
)
func normalizeEvent(v string) string {
if v == "auth_login" {
return v
}
if v == "unknown" {
return v
}
return "unknown"
}
func normalizeOutcome(outcome string) string {
if outcome == "success" || outcome == "failure" || outcome == "detected" {
return outcome
}
return "unknown"
}
func normalizeProvider(provider string) string {
if provider == "github" || provider == "google" {
return provider
}
return "none"
}
func main() {
for _, r := range [][4]string{
{"auth_login", "success", "bad_reason", "github"},
{"unknown", "unknown", "unknown", "unknown"},
{"bad_event", "bad_outcome", "bad_reason", "bad_provider"},
} {
fmt.Printf("%v -> event=%v outcome=%v reason=%v provider=%v\n",
r,
normalizeEvent(r[0]),
normalizeOutcome(r[1]),
normalizeReason(r[2]),
normalizeProvider(r[3]),
)
}
}
func normalizeReason(reason string) string {
if reason == "reason_none" {
return reason
}
return "unknown"
}
EOF
go run /tmp/check_normalize.goRepository: Jonathan0823/auth-go
Length of output: 6473
Assert the normalized label values for unsupported inputs.
TestMetricsNormalizesUnknownValues passes literal "unknown" inputs, which match normalizeEvent, normalizeOutcome, and normalizeReason before Gather. Use unsupported values such as bad_event and assert that gathered labels only contain the allowed fallback labels, like "unknown" or provider "none", to keep the bounded-label-cardinality test coverage.
🤖 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 `@internal/platform/metrics_test.go` around lines 41 - 50, The test
TestMetricsNormalizesUnknownValues currently passes values that are already
valid fallback labels and does not verify normalization. Replace those inputs
with unsupported values such as bad_event, then inspect the gathered metrics and
assert their labels use only the allowed fallback values, including unknown and
provider none, while retaining the valid-event coverage.
Related issue
Summary
c.Errorreturn values.r.Run(...)returns an error.golangci-lint.Scope
Testing
go test ./...go test -race ./...go vet ./...golangci-lint rungit diff --checkTest details
go test ./...go test -race ./...go vet ./...golangci-lint rungit diff --checkReviewer focus
internal/adapter/inbound/http/auth_handler.gointernal/adapter/inbound/http/oauth_handler.gointernal/adapter/inbound/http/user_handler.gointernal/platform/server.go.github/workflows/ci.ymlNotes
Jonathan0823/auth-go→masterSummary by CodeRabbit
Bug Fixes
Tests
Chores