fix: stop reporting expected client errors to Sentry

[reneaaron]

Problem

The OAuth handlers reported every authorize/token rejection to Sentry — malformed requests, unknown clients, redirect-URI mismatches, disallowed scopes and aborted connections. These are 4xx-class, caller-driven outcomes, not server faults, and made up the large majority of this project's Sentry volume.

Approach

Rather than one central filter (which risked swallowing real errors — errors.StatusCodes/Descriptions include server_error 500 and temporarily_unavailable 503), each call site decides explicitly, with local knowledge:

• AuthorizationHandler / TokenHandler report only errors that aren't a known go-oauth2 protocol error with a 4xx status — so 5xx OAuth errors and any unrecognised error (e.g. a DB failure) are still captured: if code, ok := oauthErrors.StatusCodes[err]; !ok || code >= 500.
• preRedirectErrorHandler no longer captures; the error is reported once downstream in AuthorizationHandler instead of twice.
• checkRedirectUriDomain and AuthorizeScopeHandler return the standard OAuth errors (invalid_request / invalid_scope) and log the detail, instead of bespoke errors that bypass classification.
• FetchClientHandler reports only genuine DB failures, not ErrRecordNotFound for an unknown client id.
• InternalErrorHandler skips context.Canceled (client aborted the request).
• Keeps the gateway context.Canceled early-return.

No behavioural change for clients — the same HTTP status/redirect outcomes — only what reaches Sentry changes. Genuine server errors (DB failures, JWT injection failures, admin client CRUD, 5xx OAuth errors) are still reported.

Verification

• go build ./... ✅
• go vet ./internal/... ✅
• Rebased on current main (preserves isUriValid, AllowPublicAccess, ContextKey).

Summary by CodeRabbit

• Bug Fixes
  • Enhanced error handling for client lookups to return appropriate HTTP status codes (404 for missing clients, 500 for server errors)
  • Added proper handling for canceled request contexts to prevent error responses
  • Improved OAuth validation error responses to use standard error types
  • Refined error reporting to capture only relevant server errors

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR improves error handling across OAuth and client flows. Request handling now checks for client-side context cancellation before proceeding. OAuth handlers return standard library error types instead of custom formatted errors, Sentry reporting becomes conditional (skipping 4xx errors and context cancellations), and client error matching uses proper type-checking semantics.

Changes

Error Handling and Cancellation Improvements

Layer / File(s)	Summary
Request context cancellation handling internal/gateway/server.go	ServeHTTP now captures the request context into a local variable and adds an early exit when the context is canceled (context.Canceled), preventing further processing of aborted requests.
OAuth error standardization and conditional Sentry reporting internal/tokens/oauth.go, internal/tokens/tokens.go	Imports updated to use context and errors packages. AuthorizationHandler and TokenHandler now conditionally report errors to Sentry based on OAuth status classification (client errors/4xx are not reported). InternalErrorHandler skips Sentry for context.Canceled. preRedirectErrorHandler and AuthorizeScopeHandler now return standard OAuth library error types (oauthErrors.ErrInvalidScope, oauthErrors.ErrInvalidRequest) with logrus warnings instead of formatted errors and Sentry capture. Redirect URI validation logs warnings and returns the standard OAuth error.
Client error handling with errors.Is internal/clients/clients.go	FetchClientHandler now uses errors.Is(err, gorm.ErrRecordNotFound) for proper error type matching to determine 404 vs. 500 status, replacing direct equality comparison.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 Error flows now handled with proper grace,
Context cancellations find their place,
OAuth types no longer need to flee,
Sentry learns when errors critical be,
Four files refactored, error paths align!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix: stop reporting expected client errors to Sentry' accurately summarizes the main objective of the changeset—reducing Sentry noise by not reporting expected 4xx errors from OAuth handlers and client misconfigurations.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/reduce-sentry-client-error-noise

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

internal/gateway/server.go (1)

37-52: ⚡ Quick win

Check cancellation before the token lookup.

ctx.Err() is only consulted after origin.CheckTokenFunc(...), so a request that is already canceled still does token-store work before this handler exits. Move the guard ahead of Line 47 and pass ctx through so the abort path stays cheap.

Suggested change

 func (origin *OriginServer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	ctx := r.Context()
@@
-	tokenInfo, err := origin.CheckTokenFunc(r.Context(), token)
+	if ctx.Err() == context.Canceled {
+		return
+	}
+
+	tokenInfo, err := origin.CheckTokenFunc(ctx, token)
 
 	// Client has aborted the request
 	if ctx.Err() == context.Canceled {
 		return
 	}

🤖 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/gateway/server.go` around lines 37 - 52, Move the
request-cancellation guard to run immediately after obtaining ctx to avoid doing
token-store work for already-aborted requests: after "ctx := r.Context()" check
"if ctx.Err() == context.Canceled { return }" before calling
origin.CheckTokenFunc; also change the CheckTokenFunc call to pass the local ctx
variable (origin.CheckTokenFunc(ctx, token)) so the cancelable context is used
for token lookup. Ensure the early-return remains in place and the rest of the
logic (public access proxy, token extraction) still runs in the correct order.

🤖 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 `@internal/tokens/oauth.go`:
- Around line 101-107: The warning currently logs full redirectURI and baseURI
which can leak sensitive path/query/userinfo; update the log in the branch that
checks parsedClientUri.Scheme != parsedRedirect.Scheme ||
!isUriValid(clientHost, redirectHost) so it only records parsed scheme and host
values (e.g., parsedClientUri.Scheme, clientHost, parsedRedirect.Scheme,
redirectHost) using logrus.WithField/WithFields and then return
oauthErrors.ErrInvalidRequest; do not include redirectURI or baseURI in the log.

---

Nitpick comments:
In `@internal/gateway/server.go`:
- Around line 37-52: Move the request-cancellation guard to run immediately
after obtaining ctx to avoid doing token-store work for already-aborted
requests: after "ctx := r.Context()" check "if ctx.Err() == context.Canceled {
return }" before calling origin.CheckTokenFunc; also change the CheckTokenFunc
call to pass the local ctx variable (origin.CheckTokenFunc(ctx, token)) so the
cancelable context is used for token lookup. Ensure the early-return remains in
place and the rest of the logic (public access proxy, token extraction) still
runs in the correct order.

🪄 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: 8b75a284-676f-430a-8425-a890c2597d3b

📥 Commits

Reviewing files that changed from the base of the PR and between 75080f9 and aa642f5.

📒 Files selected for processing (4)

• internal/clients/clients.go
• internal/gateway/server.go
• internal/tokens/oauth.go
• internal/tokens/tokens.go