[management] report pat id for tokens used more than 120 times per minute #4930
Conversation
WalkthroughPAT usage increment moved from before the rate-limiter check to after retrieving PAT info, switching the increment key from the raw token to the PAT ID. reportUsageBuckets now iterates token IDs and logs a debug/high-usage message when a token's count exceeds 60. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
management/server/http/middleware/pat_usage_tracker.go (1)
77-81: Consider log level and message clarity for high-usage alerts.The changes correctly capture and log the PAT ID for high usage detection. However, consider these refinements:
Log level: Debug logs may not be visible in production. For operational alerts about potential abuse or rate-limiting concerns, consider using
log.Infoforlog.Warnfinstead.Log message wording: The message says "token %s" but logs the PAT ID, not the raw token value. Consider updating to:
"High PAT usage detected: PAT ID %s used %d times in the last minute"for clarity.Hardcoded threshold: The value
120is hardcoded. Consider making this configurable via a constant or configuration parameter to allow different thresholds for different environments.Apply this diff to improve log level and message clarity:
- if count > 120 { - log.Debugf("High PAT usage detected: token %s used %d times in the last minute", id, count) + if count > 120 { + log.Warnf("High PAT usage detected: PAT ID %s used %d times in the last minute", id, count) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
management/server/http/middleware/auth_middleware.go(1 hunks)management/server/http/middleware/pat_usage_tracker.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
management/server/http/middleware/auth_middleware.go (1)
route/route.go (1)
ID(48-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (26)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: release_ui
- GitHub Check: release
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
🔇 Additional comments (1)
management/server/http/middleware/auth_middleware.go (1)
185-188: Confirm that expired PAT tracking is intentional for abuse detection monitoring.The code tracks all PAT access attempts, including expired ones (lines 186-188), which appears intentional for detecting abuse patterns. This is separate from
MarkPATUsed(line 194), which is only called for valid, non-expired tokens. No other usage tracking calls exist in the codebase, so the implementation is consistent.If the intent is to track all access attempts for monitoring, the current placement is correct. If you want to track only valid PAT usage, move the
IncrementUsageblock after the expiration check (line 192).
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
management/server/http/middleware/pat_usage_tracker.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (386)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (amd64)
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: release_ui
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
| for id, count := range snapshot { | ||
| t.histogram.Record(t.ctx, count) | ||
| if count > 60 { | ||
| log.Debugf("High PAT usage detected: token %s used %d times in the last minute", id, count) | ||
| } |
There was a problem hiding this comment.
Align high‑usage threshold and log wording with PR intent
Two points here:
- Threshold mismatch with PR title/intent
The PR title mentions “tokens used more than 120 times per minute”, but the code logs high usage atcount > 60. That’s a behavioral mismatch and could lead to unexpected alert volume or missed true “>120/min” cases. Consider updating and centralizing the threshold, e.g.:
- if count > 60 {
- log.Debugf("High PAT usage detected: token %s used %d times in the last minute", id, count)
+ const highPATUsageThresholdPerMinute = 120
+ if count > highPATUsageThresholdPerMinute {
+ log.Debugf("High PAT usage detected: PAT ID %s used %d times in the last minute", id, count)
}If “more than 120” should be interpreted as >= 120 or > 120, please adjust the comparison accordingly.
- Log message clarity
The variable is now a PAT ID, but the message still says “token %s”. Renaming it to “PAT ID %s” (or similar) will avoid confusion when reading logs and makes it clear we’re not logging the raw secret.
Committable suggestion skipped: line range outside the PR's diff.
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.