Enhance Badger with CI updates, dependency management, and refactoring#2
Open
marcschaeferger wants to merge 15 commits into
Open
Enhance Badger with CI updates, dependency management, and refactoring#2marcschaeferger wants to merge 15 commits into
marcschaeferger wants to merge 15 commits into
Conversation
…stem and add github ci action Signed-off-by: Marc Schäfer <git@marcschaeferger.de>
…es and add test coverage Signed-off-by: Marc Schäfer <git@marcschaeferger.de>
Signed-off-by: Marc Schäfer <git@marcschaeferger.de>
…angci config Signed-off-by: Marc Schäfer <git@marcschaeferger.de>
Signed-off-by: Marc Schäfer <git@marcschaeferger.de>
…eckout step Signed-off-by: Marc Schäfer <git@marcschaeferger.de>
… - temp Signed-off-by: Marc Schäfer <git@marcschaeferger.de>
…ddleware Signed-off-by: Marc Schäfer <git@marcschaeferger.de>
…e quality Signed-off-by: Marc Schäfer <git@marcschaeferger.de>
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR refactors the Badger forward-auth middleware (config validation, trusted-IP parsing, session exchange and verification flows), adds tests for initialization and request behavior, and updates project tooling: CI, linting, Dependabot, Makefile targets, Go version bump, README, and .gitignore. ChangesForward-auth middleware refactor and testing
Build infrastructure and project documentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Go |  |
Jun 7, 2026 3:24p.m. | Review ↗ |
| Code coverage |  |
Jun 7, 2026 3:24p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
![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)
badger.go (1)
327-340:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
RequestSchemewill be empty for server-handled requests.
req.URL.Schemeis not populated for incoming requests received by a Go HTTP server. Line 304 correctly usesgetScheme(req)to derive the scheme from TLS state, but here the raw (empty) scheme is sent.🐛 Proposed fix
+ scheme := getScheme(req) return VerifyBody{ Sessions: cookies, OriginalRequestURL: originalRequestURL, - RequestScheme: &req.URL.Scheme, + RequestScheme: &scheme, RequestHost: &req.Host,🤖 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 `@badger.go` around lines 327 - 340, In the VerifyBody return construction (the block that builds and returns VerifyBody), RequestScheme is incorrectly set to &req.URL.Scheme which is empty for server-handled requests; compute the scheme using the existing helper getScheme(req) (e.g. scheme := getScheme(req)) and set RequestScheme to a pointer to that scheme (RequestScheme: &scheme) so the TLS-derived scheme is used; update the VerifyBody builder (function that returns VerifyBody) to use getScheme(req) for RequestScheme instead of req.URL.Scheme.
♻️ Duplicate comments (1)
badger.go (1)
261-265:⚠️ Potential issue | 🟠 MajorSame timeout concern applies here.
This call shares the
http.DefaultClienttimeout risk flagged inServeHTTP. Using a configured client with timeout will address both locations.🤖 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 `@badger.go` around lines 261 - 265, The call to http.DefaultClient.Do(httpReq) in the handler (same pattern as ServeHTTP) uses the global client without a timeout; replace usage of http.DefaultClient.Do with the configured HTTP client that includes a timeout (the same client used in ServeHTTP), e.g., call the injected/stack-level client (not http.DefaultClient) when sending httpReq so errors/timeouts are handled consistently and avoid hanging requests.
🧹 Nitpick comments (2)
badger.go (1)
406-419: ⚡ Quick winStruct definition duplicated; consider extracting a named type.
The parameter type replicates
VerifyResponse.Dataverbatim. Changes to the response model require updating both locations.Extract a named type for the data payload:
type VerifyResponseData struct { HeaderAuthChallenged bool `json:"headerAuthChallenged"` // ... remaining fields } type VerifyResponse struct { Data VerifyResponseData `json:"data"` }Then use
*VerifyResponseDataas the parameter type here.🤖 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 `@badger.go` around lines 406 - 419, The anonymous struct used as the second parameter of Badger.setUserHeaders duplicates VerifyResponse.Data; extract a shared named type (e.g., type VerifyResponseData struct { HeaderAuthChallenged bool ... PangolinVersion *string }) and update the VerifyResponse struct to use VerifyResponseData for its Data field, then change setUserHeaders to accept *VerifyResponseData instead of the inline struct and update any callsites to pass the typed value.Makefile (1)
15-16: ⚡ Quick winConsider documenting the expected golangci-lint version.
The CI workflow uses golangci-lint v2.12.2, but the
linttarget uses whatever version is installed locally. This could lead to inconsistent results. Consider adding a comment or variable to document the expected version.📝 Suggested improvement
+# Use golangci-lint v2.12.2 to match CI (install via: go install github.com/golangci/golangci-lint/cmd/golangci-lint@v2.12.2) lint: golangci-lint run🤖 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 `@Makefile` around lines 15 - 16, The Makefile's lint target currently runs golangci-lint without documenting the expected version; add a brief comment above the lint target mentioning the CI-locked version (v2.12.2) and optionally introduce a variable like GOLANGCI_LINT_VERSION or GOLANGCI_LINT to document or pin the expected binary/version used by the lint target (referencing the lint target and golangci-lint command) so local runs match CI.
🤖 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:
- Around line 60-64: The CI step named "Run golangci-lint" currently has
continue-on-error: true which allows lint failures to pass; remove that setting
(or set it to false) so the job fails on lint errors before merging, and if
there are known legitimate issues either fix them or add targeted exclusions in
.golangci.yml (adjust rules or linters) to keep the workflow strict; locate the
workflow job by the step name "Run golangci-lint" and the uses line
golangci/golangci-lint-action@82606bf257cbaff209d206a39f5134f0cfbfd2ee to make
the change.
In `@badger.go`:
- Around line 212-216: The code uses http.DefaultClient.Do in badger.go which
has no timeout; declare a package-level variable (e.g., httpClient :=
&http.Client{Timeout: 10 * time.Second}) and replace
http.DefaultClient.Do(httpReq) with httpClient.Do(httpReq); ensure the package
imports time if not already and keep the existing gosec nolint comment on the Do
call if needed.
---
Outside diff comments:
In `@badger.go`:
- Around line 327-340: In the VerifyBody return construction (the block that
builds and returns VerifyBody), RequestScheme is incorrectly set to
&req.URL.Scheme which is empty for server-handled requests; compute the scheme
using the existing helper getScheme(req) (e.g. scheme := getScheme(req)) and set
RequestScheme to a pointer to that scheme (RequestScheme: &scheme) so the
TLS-derived scheme is used; update the VerifyBody builder (function that returns
VerifyBody) to use getScheme(req) for RequestScheme instead of req.URL.Scheme.
---
Duplicate comments:
In `@badger.go`:
- Around line 261-265: The call to http.DefaultClient.Do(httpReq) in the handler
(same pattern as ServeHTTP) uses the global client without a timeout; replace
usage of http.DefaultClient.Do with the configured HTTP client that includes a
timeout (the same client used in ServeHTTP), e.g., call the injected/stack-level
client (not http.DefaultClient) when sending httpReq so errors/timeouts are
handled consistently and avoid hanging requests.
---
Nitpick comments:
In `@badger.go`:
- Around line 406-419: The anonymous struct used as the second parameter of
Badger.setUserHeaders duplicates VerifyResponse.Data; extract a shared named
type (e.g., type VerifyResponseData struct { HeaderAuthChallenged bool ...
PangolinVersion *string }) and update the VerifyResponse struct to use
VerifyResponseData for its Data field, then change setUserHeaders to accept
*VerifyResponseData instead of the inline struct and update any callsites to
pass the typed value.
In `@Makefile`:
- Around line 15-16: The Makefile's lint target currently runs golangci-lint
without documenting the expected version; add a brief comment above the lint
target mentioning the CI-locked version (v2.12.2) and optionally introduce a
variable like GOLANGCI_LINT_VERSION or GOLANGCI_LINT to document or pin the
expected binary/version used by the lint target (referencing the lint target and
golangci-lint command) so local runs match CI.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8eed73c4-1f89-499d-9fe2-4a2a1d8efb91
📒 Files selected for processing (11)
.github/dependabot.yml.github/workflows/ci.yml.gitignore.go-version.golangci.ymlMakefileREADME.mdbadger.gobadger_test.gogo.modips/ips_test.go
…oved accessibility Signed-off-by: Marc Schäfer <git@marcschaeferger.de>
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)
badger.go (1)
327-340:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
RequestSchemewill always be empty for server-side requests.For incoming HTTP requests,
req.URL.Schemeis not populated by Go's HTTP server - only the path and query are set. This causesRequestSchemeto always be an empty string, inconsistent withOriginalRequestURLwhich correctly derives the scheme viagetScheme(req).🐛 Proposed fix
+ scheme := getScheme(req) return VerifyBody{ Sessions: cookies, OriginalRequestURL: originalRequestURL, - RequestScheme: &req.URL.Scheme, + RequestScheme: &scheme, RequestHost: &req.Host,🤖 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 `@badger.go` around lines 327 - 340, The VerifyBody construction sets RequestScheme to &req.URL.Scheme which is empty for server-side requests; change it to use the actual scheme from getScheme(req) (e.g. scheme := getScheme(req) and set RequestScheme to &scheme) so RequestScheme matches OriginalRequestURL; update the VerifyBody initialization (where RequestScheme is assigned) to reference that scheme variable instead of req.URL.Scheme.
🧹 Nitpick comments (1)
badger.go (1)
407-419: 💤 Low valueConsider passing
*VerifyResponseinstead of duplicating the anonymous struct.The anonymous struct parameter duplicates the exact structure of
VerifyResponse.Data. This creates a maintenance burden where both definitions must stay in sync.♻️ Suggested simplification
Pass the result directly and access
.Data:-func setUserHeaders(req *http.Request, data *struct { - HeaderAuthChallenged bool `json:"headerAuthChallenged"` - Valid bool `json:"valid"` - RedirectURL *string `json:"redirectUrl"` - UserID *string `json:"userId,omitempty"` - DontStripSession bool `json:"dontStripSession,omitempty"` - Username *string `json:"username,omitempty"` - Email *string `json:"email,omitempty"` - Name *string `json:"name,omitempty"` - Role *string `json:"role,omitempty"` - ResponseHeaders map[string]string `json:"responseHeaders,omitempty"` - PangolinVersion *string `json:"pangolinVersion,omitempty"` -}) { - if data.UserID != nil { +func setUserHeaders(req *http.Request, result *VerifyResponse) { + if result.Data.UserID != nil {And update the call site:
- setUserHeaders(req, &result.Data) + setUserHeaders(req, &result)🤖 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 `@badger.go` around lines 407 - 419, Replace the anonymous struct parameter of setUserHeaders with the existing VerifyResponse (or a pointer to it) so you don't duplicate VerifyResponse.Data; change setUserHeaders(req *http.Request, data *VerifyResponse) (or data VerifyResponse) and inside the function reference data.Data (or data.Data.ResponseHeaders, data.Data.UserID, etc.) and update all call sites that pass the anonymous literal to pass the VerifyResponse value (or its pointer) instead; ensure you remove the anonymous type declaration and import/adjust any field dereferences accordingly in setUserHeaders and callers.
🤖 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 `@badger.go`:
- Around line 327-340: The VerifyBody construction sets RequestScheme to
&req.URL.Scheme which is empty for server-side requests; change it to use the
actual scheme from getScheme(req) (e.g. scheme := getScheme(req) and set
RequestScheme to &scheme) so RequestScheme matches OriginalRequestURL; update
the VerifyBody initialization (where RequestScheme is assigned) to reference
that scheme variable instead of req.URL.Scheme.
---
Nitpick comments:
In `@badger.go`:
- Around line 407-419: Replace the anonymous struct parameter of setUserHeaders
with the existing VerifyResponse (or a pointer to it) so you don't duplicate
VerifyResponse.Data; change setUserHeaders(req *http.Request, data
*VerifyResponse) (or data VerifyResponse) and inside the function reference
data.Data (or data.Data.ResponseHeaders, data.Data.UserID, etc.) and update all
call sites that pass the anonymous literal to pass the VerifyResponse value (or
its pointer) instead; ensure you remove the anonymous type declaration and
import/adjust any field dereferences accordingly in setUserHeaders and callers.
…ession handling Signed-off-by: Marc Schäfer <git@marcschaeferger.de>
…checks Signed-off-by: Marc Schäfer <git@marcschaeferger.de>
Signed-off-by: Marc Schäfer <git@marcschaeferger.de>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 19: The workflow defines an unused environment variable YAEGI_VERSION
which is never referenced by the jobs; either remove the YAEGI_VERSION entry
from the workflow or forward it into the Makefile invocation used by the yaegi
job (the step that runs make yaegi-test) so the Makefile receives the same value
(e.g., invoke make yaegi-test with YAEGI_VERSION set from the workflow env).
Update the yaegi job's step that runs make yaegi-test or delete the top-level
YAEGI_VERSION to eliminate the duplicate/confusing source of truth.
- Line 44: Replace the explicit disabling of Go module caching and heavy clone
depth: remove or change the `cache: false` entry for the `actions/setup-go` step
so the action can use the repo's go.mod/go.sum-based cache (i.e., allow the
action's default/module cache instead of `false`), and reduce `fetch-depth: 0`
to `1` so CI still obtains the files needed for `git diff -- go.mod go.sum`
without fetching full history; target the `actions/setup-go` step and the
`fetch-depth` key in the workflow where `cache: false` is currently set.
In `@Makefile`:
- Line 15: Update the misleading Makefile comment that references the undefined
variable $(GOLANGCI_LINT_VERSION): change the comment line containing "Lint
using golangci-lint (CI-locked version: $(GOLANGCI_LINT_VERSION))" to a neutral
description (e.g., "Lint using golangci-lint (version is pinned in CI
workflow)") so it no longer suggests the variable is defined in the Makefile;
target the comment text in the Makefile near the golangci-lint mention and
remove or replace the $(GOLANGCI_LINT_VERSION) token.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bb56be5b-152e-40af-8bda-6e7c2dabe009
📒 Files selected for processing (4)
.github/workflows/ci.yml.gitignoreMakefilebadger.go
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- badger.go
… in redirect page Signed-off-by: Marc Schäfer <git@marcschaeferger.de>
Signed-off-by: Marc Schäfer <git@marcschaeferger.de>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces significant improvements to project automation, code quality enforcement, and codebase maintainability. The main changes include adding a comprehensive CI workflow, introducing and configuring
golangci-lint, updating the Go version, refactoring and renaming the main code file, and enhancing documentation. These updates streamline development, ensure higher code quality, and clarify project usage.Automation and CI/CD:
ci.yml) to automate testing, linting, vulnerability scanning, and metadata validation for every push and pull request. This ensures all code changes are automatically checked for quality and security..github/dependabot.ymlto group and limit dependency update pull requests for both Go modules and GitHub Actions, improving dependency management automation.Code Quality and Linting:
.golangci.ymlwith a detailed configuration forgolangci-lint, enabling a wide range of linters for correctness, maintainability, and security, and customizing rules for project-specific needs.Makefilewith targets for formatting, linting, testing, vulnerability checking, and reproducing the CI checks locally, making it easier for developers to maintain code quality.Codebase Refactoring and Upgrades:
main.gotobadger.goand refactored the main middleware logic for improved readability and maintainability. This includes extracting helper methods, improving error handling, and clarifying configuration validation. [1] [2] [3] [4] [5] [6] [7] [8] [9].go-versionto use the latest language features and improvements.Documentation:
README.mdwith project badges, clearer explanations of Badger’s modes, and improved installation instructions, helping users understand and adopt the middleware more easily.Summary by CodeRabbit
New Features
Documentation
Tests
Chores