Handle basic auth without token validation #26
Conversation
📝 WalkthroughWalkthroughAdds HasAuthPrefix(token, scheme) and updates auth handling: Bearer and Basic detection now use HasAuthPrefix across queue routing, queue creation, and Discord rate logic; routing hash behavior adjusted for Bearer tokens; new unit tests for routing and the utility were added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant QueueManager
participant Util as HasAuthPrefix
Client->>QueueManager: GetRequestRoutingInfo(req)
QueueManager->>Util: HasAuthPrefix(auth, "Bearer")
alt Bearer
QueueManager-->>Client: queueType=Bearer, routingHash=HashCRC64("Bearer ...")
else Not Bearer
QueueManager->>Util: HasAuthPrefix(auth, "Basic")
alt Basic or empty
QueueManager-->>Client: queueType=NoAuth, routingHash=HashCRC64(path)
else Non-empty non-Basic
QueueManager-->>Client: queueType=Bot, routingHash=HashCRC64(path)
end
end
sequenceDiagram
autonumber
participant App
participant Queue as NewRequestQueue
participant Util as HasAuthPrefix
participant Discord as GetBotUser
App->>Queue: NewRequestQueue(token)
Queue->>Util: HasAuthPrefix(token, "Bearer")
alt Bearer
Queue-->>App: queueType=Bearer
else Not Bearer
Queue->>Util: HasAuthPrefix(token, "Basic")
alt Basic
Queue-->>App: queueType=NoAuth
else token empty
Queue-->>App: queueType=NoAuth
else non-empty non-Basic
Queue->>Discord: GetBotUser(token)
alt success
Queue-->>App: queueType=Bot
else error
Queue-->>App: error
end
end
end
sequenceDiagram
autonumber
participant Caller
participant DiscordLib as lib/discord.go
participant Util as HasAuthPrefix
Caller->>DiscordLib: call with token
DiscordLib->>Util: HasAuthPrefix(token, "Bearer")
alt Bearer
DiscordLib-->>Caller: short-circuit (50, nil)
else
DiscordLib->>Util: HasAuthPrefix(token, "Basic")
alt Basic
DiscordLib-->>Caller: short-circuit (50, nil)
else
DiscordLib-->>Caller: continue existing logic
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests✅ Unit Test PR creation complete.
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/queue_manager.go (1)
295-299: Normalize NoAuth queue key (prevent retaining Basic credentials).Basic auth is classified as NoAuth by GetRequestRoutingInfo, but fulfillRequest passes the raw Authorization token into getOrCreateBotQueue — this creates per‑Basic-token queues and stores plaintext Basic tokens as map keys. Unify NoAuth to a single key before creating/getting queues.
File: lib/queue_manager.go Lines: 295-299
- if queueType == Bearer { - q, err = m.getOrCreateBearerQueue(token) - } else { - q, err = m.getOrCreateBotQueue(token) - } + key := token + if queueType == NoAuth { + key = "" // unify Basic and empty-Authorization into the NoAuth queue + } + if queueType == Bearer { + q, err = m.getOrCreateBearerQueue(key) + } else { + q, err = m.getOrCreateBotQueue(key) + }lib/queue_manager_test.go (1)
58-58: Add invariance tests for Bearer scheme (case/whitespace) to prevent routing splits.These guard against upstream variations like “bearer” vs “Bearer” and multi‑space separators.
@@ } +func TestGetRequestRoutingInfoBearerAuthInvariance(t *testing.T) { + t.Parallel() + manager := &QueueManager{} + + req1, err := http.NewRequest("GET", "https://discord.com/api/v10/users/@me", nil) + if err != nil { + t.Fatalf("failed to create request: %v", err) + } + h1, _, qt1 := manager.GetRequestRoutingInfo(req1, "Bearer some-token") + if qt1 != Bearer { + t.Fatalf("expected queue type %v, got %v", Bearer, qt1) + } + + req2, err := http.NewRequest("GET", "https://discord.com/api/v10/users/@me", nil) + if err != nil { + t.Fatalf("failed to create request: %v", err) + } + h2, _, qt2 := manager.GetRequestRoutingInfo(req2, "bearer some-token") + if qt2 != Bearer { + t.Fatalf("expected queue type %v, got %v", Bearer, qt2) + } + + if h1 != h2 { + t.Fatalf("expected normalized hashing for bearer tokens; got %d vs %d", h1, h2) + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
lib/discord.go(1 hunks)lib/queue.go(1 hunks)lib/queue_manager.go(1 hunks)lib/queue_manager_test.go(1 hunks)lib/util.go(2 hunks)lib/util_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
lib/discord.go (1)
lib/util.go (1)
HasAuthPrefix(52-67)
lib/queue_manager_test.go (2)
lib/queue_manager.go (4)
QueueManager(32-45)NoAuth(21-21)Bearer(22-22)Bot(20-20)lib/util.go (1)
HashCRC64(13-17)
lib/util_test.go (1)
lib/util.go (2)
HashCRC64(13-17)HasAuthPrefix(52-67)
lib/queue.go (2)
lib/util.go (1)
HasAuthPrefix(52-67)lib/discord.go (1)
GetBotUser(189-217)
lib/queue_manager.go (1)
lib/util.go (2)
HashCRC64(13-17)HasAuthPrefix(52-67)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (4)
lib/discord.go (1)
140-146: Short‑circuiting Basic and Bearer is correct; reuse via HasAuthPrefix is good.This aligns with the PR goal to bypass lookups for non‑bot traffic. Consider adding a focused unit test for GetBotGlobalLimit to assert 50 for Basic and Bearer to prevent regressions.
lib/queue.go (1)
49-57: Good: Basic tokens no longer trigger bot lookups; Bearer handled via helper.This matches the intent to bypass token validation for Basic/Bearer.
Ensure queue_manager normalization (proposed in its review) is applied; otherwise Basic will still create per‑token queues keyed by credentials.
lib/util.go (1)
35-39: Incorrect assumption — don't decode Basic credentials in GetBotId.GetBotId (lib/util.go:30–46) currently strips only "Bot " and "Bearer " then base64-decodes the first segment; it does NOT remove "Basic " (so the original claim that Basic is already stripped is incorrect).
- Do not attempt to decode Basic credentials; if header has a Basic prefix, return a non-sensitive tag (e.g. "BasicAuth") and avoid returning decoded "user:pass".
- If parsing JWTs, strip auth prefixes by prefix-only checks (case-insensitive) and decode the first segment using base64.RawURLEncoding/URLEncoding (JWT uses URL-safe base64).
- Search found no call sites for GetBotId — confirm where/if it’s used before changing behavior.
lib/queue_manager_test.go (1)
9-9: Confirm zero-value QueueManager is safe in GetRequestRoutingInfolib/queue_manager_test.go (lines 9, 26, 43) construct &QueueManager{} — verify GetRequestRoutingInfo does not dereference uninitialized fields (locks, caches, maps) or perform side effects; if it does, initialize via a constructor (e.g. NewQueueManager) or update the tests to set required fields.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/util_test.go (1)
25-35: Trim the 50k loop to reduce test time without losing signal.50k iterations are unnecessary for catching statefulness. 10k (or even 5k) is sufficient and faster.
- for i := 0; i < 50000; i++ { + for i := 0; i < 10000; i++ { HashCRC64(strconv.Itoa(i)) }
♻️ Duplicate comments (1)
lib/util_test.go (1)
44-47: Thanks for adding the tab and scheme‑only cases.These close the previously noted coverage gap for delimiter handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
lib/util_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/util_test.go (1)
lib/util.go (2)
HashCRC64(13-17)HasAuthPrefix(52-67)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (1)
lib/util_test.go (1)
47-47: Confirm intent: should “Basic ” (no credentials) be considered a Basic header?HasAuthPrefix returns true for a lone scheme + delimiter. If that classification affects bypass paths (e.g., token lookups/routing), confirm this is desired; otherwise, require at least one non‑whitespace char after the delimiter.
| func TestHasAuthPrefix(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| token string | ||
| scheme string | ||
| expected bool | ||
| }{ | ||
| {name: "basic with space", token: "Basic Zm9vOmJhcg==", scheme: "Basic", expected: true}, | ||
| {name: "basic lowercase", token: "basic Zm9vOmJhcg==", scheme: "Basic", expected: true}, | ||
| {name: "basic with tab", token: "Basic\tZm9vOmJhcg==", scheme: "Basic", expected: true}, | ||
| {name: "basic only scheme", token: "Basic ", scheme: "Basic", expected: true}, | ||
| {name: "missing space", token: "BasicZm9v", scheme: "Basic", expected: false}, | ||
| {name: "different scheme", token: "Bot foo", scheme: "Basic", expected: false}, | ||
| {name: "bearer", token: "Bearer token", scheme: "Bearer", expected: true}, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Add a few more edge cases to lock in prefix semantics.
Recommend covering empty token, no delimiter, double‑space, and tab‑only delimiter for both Basic and Bearer.
tests := []struct {
name string
token string
scheme string
expected bool
}{
{name: "basic with space", token: "Basic Zm9vOmJhcg==", scheme: "Basic", expected: true},
{name: "basic lowercase", token: "basic Zm9vOmJhcg==", scheme: "Basic", expected: true},
{name: "basic with tab", token: "Basic\tZm9vOmJhcg==", scheme: "Basic", expected: true},
{name: "basic only scheme", token: "Basic ", scheme: "Basic", expected: true},
+ {name: "basic only scheme with tab", token: "Basic\t", scheme: "Basic", expected: true},
+ {name: "basic without delimiter", token: "Basic", scheme: "Basic", expected: false},
+ {name: "basic with double space", token: "Basic Zm9v", scheme: "Basic", expected: true},
+ {name: "empty token", token: "", scheme: "Basic", expected: false},
{name: "missing space", token: "BasicZm9v", scheme: "Basic", expected: false},
{name: "different scheme", token: "Bot foo", scheme: "Basic", expected: false},
{name: "bearer", token: "Bearer token", scheme: "Bearer", expected: true},
+ {name: "bearer with tab", token: "Bearer\ttoken", scheme: "Bearer", expected: true},
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestHasAuthPrefix(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| token string | |
| scheme string | |
| expected bool | |
| }{ | |
| {name: "basic with space", token: "Basic Zm9vOmJhcg==", scheme: "Basic", expected: true}, | |
| {name: "basic lowercase", token: "basic Zm9vOmJhcg==", scheme: "Basic", expected: true}, | |
| {name: "basic with tab", token: "Basic\tZm9vOmJhcg==", scheme: "Basic", expected: true}, | |
| {name: "basic only scheme", token: "Basic ", scheme: "Basic", expected: true}, | |
| {name: "missing space", token: "BasicZm9v", scheme: "Basic", expected: false}, | |
| {name: "different scheme", token: "Bot foo", scheme: "Basic", expected: false}, | |
| {name: "bearer", token: "Bearer token", scheme: "Bearer", expected: true}, | |
| } | |
| func TestHasAuthPrefix(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| token string | |
| scheme string | |
| expected bool | |
| }{ | |
| {name: "basic with space", token: "Basic Zm9vOmJhcg==", scheme: "Basic", expected: true}, | |
| {name: "basic lowercase", token: "basic Zm9vOmJhcg==", scheme: "Basic", expected: true}, | |
| {name: "basic with tab", token: "Basic\tZm9vOmJhcg==", scheme: "Basic", expected: true}, | |
| {name: "basic only scheme", token: "Basic ", scheme: "Basic", expected: true}, | |
| {name: "basic only scheme with tab", token: "Basic\t", scheme: "Basic", expected: true}, | |
| {name: "basic without delimiter", token: "Basic", scheme: "Basic", expected: false}, | |
| {name: "basic with double space", token: "Basic Zm9v", scheme: "Basic", expected: true}, | |
| {name: "empty token", token: "", scheme: "Basic", expected: false}, | |
| {name: "missing space", token: "BasicZm9v", scheme: "Basic", expected: false}, | |
| {name: "different scheme", token: "Bot foo", scheme: "Basic", expected: false}, | |
| {name: "bearer", token: "Bearer token", scheme: "Bearer", expected: true}, | |
| {name: "bearer with tab", token: "Bearer\ttoken", scheme: "Bearer", expected: true}, | |
| } |
🤖 Prompt for AI Agents
In lib/util_test.go around lines 37 to 51, the test table for TestHasAuthPrefix
is missing edge cases that assert exact prefix semantics; add tests for empty
token (""), no delimiter (e.g., "BasicZm9v" and "Bearertoken"), double-space
delimiter (e.g., "Basic Zm9v" and "Bearer token"), and tab-only delimiter
(e.g., "Basic\t" and "Bearer\t") for both Basic and Bearer schemes, setting
expected true only when the scheme is present followed by at least one
whitespace delimiter and possibly an empty credential when appropriate, and
false for no-delimiter cases and malformed spacing as described.
| for _, tc := range tests { | ||
| if result := HasAuthPrefix(tc.token, tc.scheme); result != tc.expected { | ||
| t.Errorf("%s: expected %v, got %v", tc.name, tc.expected, result) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Use subtests for clearer reporting.
Subtests make failures easier to pinpoint and enable parallelism if needed.
- for _, tc := range tests {
- if result := HasAuthPrefix(tc.token, tc.scheme); result != tc.expected {
- t.Errorf("%s: expected %v, got %v", tc.name, tc.expected, result)
- }
- }
+ for _, tc := range tests {
+ tc := tc
+ t.Run(tc.name, func(t *testing.T) {
+ if result := HasAuthPrefix(tc.token, tc.scheme); result != tc.expected {
+ t.Fatalf("expected %v, got %v (token=%q, scheme=%q)", tc.expected, result, tc.token, tc.scheme)
+ }
+ })
+ }🤖 Prompt for AI Agents
In lib/util_test.go around lines 53 to 58, the table-driven loop uses a plain
for loop which makes failures harder to pinpoint; convert each iteration to a
subtest by calling t.Run(tc.name, func(t *testing.T) { tc := tc; /* optional:
t.Parallel() */ if result := HasAuthPrefix(tc.token, tc.scheme); result !=
tc.expected { t.Errorf("%s: expected %v, got %v", tc.name, tc.expected, result)
} }) so each case reports separately (remember to capture tc into the closure
and add t.Parallel() only if the test is safe to run concurrently).
|
@codex deep analyze this pull request |
|
Codex Review: Didn't find any major issues. Nice work! About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback". |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
1 similar comment
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Caution CodeRabbit failed during planning: Script execution failed: /bin/bash: -c: line 2: syntax error: unexpected end of file |
2 similar comments
|
Caution CodeRabbit failed during planning: Script execution failed: /bin/bash: -c: line 2: syntax error: unexpected end of file |
|
Caution CodeRabbit failed during planning: Script execution failed: /bin/bash: -c: line 2: syntax error: unexpected end of file |
Handle basic auth without token validation (germanoeich#26)
* Handle basic auth without token validation * Update lib/util_test.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update lib/util_test.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68c9938bc248832fbc69728d16243d20