feat: add support for deny by default access controls#852
Open
steveiliop56 wants to merge 6 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduce a PolicyEngine and rule types for ACL checks, add ACLS config, refactor AccessControlsService to use model.Config, wire label provider and policy engine in bootstrap, update ProxyController to evaluate rules via PolicyEngine, and update utils and tests accordingly. ChangesPolicy & ACL rewrite
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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
🧪 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 |
dosubot
Bot
added
the
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
internal/controller/proxy_controller_test.go (1)
25-25: ⚡ Quick winAdd tests for the new deny/block policy.
The shared
cfgfromtest.CreateTestConfigs(t)doesn't setAuth.ACLS.Policy, so the entire deny-by-default code path introduced in this PR is never executed byTestProxyController. The existing happy-path/path-allow/ip-bypass/user-allow cases all bypasspolicyResulteither via explicit filters or via the bypass/auth-disabled short circuits.Consider adding at least:
- A test where
cfg.Auth.ACLS.Policy = "block"and a request hits a domain with no matchingAppsentry — assert it's denied at the IP allow step.- A test where
cfg.Auth.ACLS.Policy = "block"and a request hits a domain with an explicitIP.Allow(orUsers.Allow) — assert it succeeds.- A test with an invalid policy string (e.g.,
"deny","") — assert it logs a warning and behaves as the documented default ("allow"). This will also catch the fallback regression flagged inNewAccessControlsService.Also applies to: 367-367
🤖 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/controller/proxy_controller_test.go` at line 25, The tests for TestProxyController never exercise the new deny-by-default policy because test.CreateTestConfigs(t) doesn't set cfg.Auth.ACLS.Policy; add new unit tests that set cfg.Auth.ACLS.Policy = "block" and exercise the code paths around policyResult: (1) a request to a domain with no matching Apps entry should be denied at the IP allow step, (2) a request to a domain with an explicit IP.Allow or Users.Allow should succeed, and (3) a test with an invalid policy string (e.g., "deny" or "") should assert a warning is logged and behavior falls back to documented default ("allow") to catch the NewAccessControlsService fallback regression. Ensure tests reference and assert on the same controller flow exercised by TestProxyController so policyResult is evaluated rather than bypassed.internal/controller/proxy_controller.go (1)
53-72: ⚡ Quick winRemove unused
authfield and constructor parameter fromProxyController.After the refactor, all authorization checks in
proxyHandlerdelegate tocontroller.acls(IP bypass, auth-enabled status, user allowed, groups). Theauthfield on the struct and correspondingauthparameter inNewProxyControllerare never referenced. Remove them to keep the dependency surface minimal and make intent clearer. Update the wiring ininternal/bootstrap/router_bootstrap.go:47and test setup accordingly.🤖 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/controller/proxy_controller.go` around lines 53 - 72, The ProxyController struct and constructor still include an unused auth dependency: remove the auth field from the ProxyController struct and remove the auth parameter (type *service.AuthService) from NewProxyController signature and its struct literal, then update all call sites that invoke NewProxyController to stop passing the auth argument (including bootstrap/router wiring and tests) so the code compiles with the reduced dependency surface; ensure any imports or test fixtures related only to auth are cleaned up.
🤖 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/model/config.go`:
- Around line 230-232: The ACLSConfig description string is inconsistent and has
a typo: change "deny-by-defaut" to "deny-by-default" and make the allowed values
consistent with accessControlPolicyFromString by either (preferred) switching
the internal parser/constants in access_controls_service.go (and any related
constants) to accept the YAML value "deny" instead of "block", or conversely
update this description to list "allow and block"; specifically modify
ACLSConfig (type ACLSConfig) description text and update
accessControlPolicyFromString (and its associated policy constants) so the
accepted policy string matches the user-facing value ("deny") throughout the
codebase.
In `@internal/service/access_controls_service.go`:
- Around line 213-215: The current append usage can mutate the shared backing
arrays for service.config.Auth.IP.Block/Allow; to fix, create new slices before
concatenation so you never write into the shared slice: allocate a fresh slice
(e.g., with make or by starting from a nil-typed slice) and copy or append the
global entries then the per-app entries to produce blockedIps and allowedIPs;
update the code that constructs blockedIps and allowedIPs (the lines using
append with service.config.Auth.IP.Block and acls.IP.Block, and similarly for
.Allow) to use the safe allocation/copy approach (or slices.Concat if you target
Go ≥1.22).
- Around line 41-68: NewAccessControlsService sets service.policy incorrectly
when accessControlPolicyFromString returns ok==false: it first assigns
service.policy = PolicyAllow then later logs and overwrites service.policy with
the empty/local variable policy, leaving an invalid value. Fix by using the
validated policy variable (or set policy = PolicyAllow when !ok) before any
logging and before assigning into service.policy; specifically adjust
NewAccessControlsService to set policy = PolicyAllow when
accessControlPolicyFromString returns ok==false (and keep the warning log), then
use that policy for the subsequent if/else logging and final service.policy
assignment so service.policy and the logged policy remain consistent
(references: NewAccessControlsService, accessControlPolicyFromString,
service.policy, policy, PolicyAllow).
- Around line 106-125: IsUserAllowed currently treats an existing ACL with empty
users filters as allowing access because utils.CheckFilter("", username) returns
true; change IsUserAllowed (and similarly handle the OAuth whitelist if desired)
to detect when both acls.Users.Block and acls.Users.Allow are empty and route
that case through service.policyResult(true) instead of calling
utils.CheckFilter, so the global policy (policyResult) is respected when an ACL
exists but contains no user filters; use the symbols IsUserAllowed,
acls.Users.Block, acls.Users.Allow, utils.CheckFilter, and service.policyResult
to locate and modify the logic.
---
Nitpick comments:
In `@internal/controller/proxy_controller_test.go`:
- Line 25: The tests for TestProxyController never exercise the new
deny-by-default policy because test.CreateTestConfigs(t) doesn't set
cfg.Auth.ACLS.Policy; add new unit tests that set cfg.Auth.ACLS.Policy = "block"
and exercise the code paths around policyResult: (1) a request to a domain with
no matching Apps entry should be denied at the IP allow step, (2) a request to a
domain with an explicit IP.Allow or Users.Allow should succeed, and (3) a test
with an invalid policy string (e.g., "deny" or "") should assert a warning is
logged and behavior falls back to documented default ("allow") to catch the
NewAccessControlsService fallback regression. Ensure tests reference and assert
on the same controller flow exercised by TestProxyController so policyResult is
evaluated rather than bypassed.
In `@internal/controller/proxy_controller.go`:
- Around line 53-72: The ProxyController struct and constructor still include an
unused auth dependency: remove the auth field from the ProxyController struct
and remove the auth parameter (type *service.AuthService) from
NewProxyController signature and its struct literal, then update all call sites
that invoke NewProxyController to stop passing the auth argument (including
bootstrap/router wiring and tests) so the code compiles with the reduced
dependency surface; ensure any imports or test fixtures related only to auth are
cleaned up.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d27ab4c6-72fd-4514-9274-045ba1cf85e8
📒 Files selected for processing (7)
internal/bootstrap/service_bootstrap.gointernal/controller/proxy_controller.gointernal/controller/proxy_controller_test.gointernal/model/config.gointernal/service/access_controls_service.gointernal/service/auth_service.gointernal/test/test.go
💤 Files with no reviewable changes (1)
- internal/service/auth_service.go
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/service/access_controls_service.go (1)
71-82: 💤 Low valuePrefer the explicit local-copy pattern when storing a pointer to the loop value.
appAcls = &configtakes the address of the range loop variable. Although the immediatebreakmakes this functionally safe, the project convention is to materialize an explicit local copy to make intent obvious and avoid loop-variable capture confusion.♻️ Proposed refactor
var appAcls *model.App for app, config := range service.config.Apps { + appConfig := config if config.Config.Domain == domain { service.log.App.Debug().Str("name", app).Msg("Found matching container by domain") - appAcls = &config + appAcls = &appConfig break // If we find a match by domain, we can stop searching } if strings.SplitN(domain, ".", 2)[0] == app { service.log.App.Debug().Str("name", app).Msg("Found matching container by app name") - appAcls = &config + appAcls = &appConfig break // If we find a match by app name, we can stop searching } }Based on learnings: "prefer the explicit local copy pattern (e.g.,
result := config; return &result) rather than returning&<range-variable>directly … the project prefers the explicit copy for clarity and to avoid reviewer confusion about loop-variable capture semantics."🤖 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/service/access_controls_service.go` around lines 71 - 82, The code takes the address of the range loop variable (config) when assigning appAcls (appAcls = &config); change this to the explicit local-copy pattern to avoid loop-variable capture confusion: create a new local variable (e.g., cfg := config) and assign appAcls = &cfg instead for both matches (the domain check and the app-name check) while keeping the break logic and using the existing service.config.Apps, appAcls, and config identifiers to locate the spots to change.
🤖 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/service/access_controls_service.go`:
- Around line 173-205: The Path.Block branch in
AccessControlsService.IsAuthEnabled currently uses a negation (if
!regex.MatchString(uri) { return false }) which inverts the intended behavior;
remove the negation so that matching the block regex disables auth (i.e., if
regex.MatchString(uri) { return false }). Also avoid compiling regexes on every
call to IsAuthEnabled: precompile and cache the regex objects for
acls.Path.Block and acls.Path.Allow (either during service initialization or by
adding compiled fields on the model.App/ACL struct) and use those cached
*regexp.Regexp instances in IsAuthEnabled; preserve existing error logging via
service.log.App.Error() when compilation fails during initialization or when
setting the ACL.
---
Nitpick comments:
In `@internal/service/access_controls_service.go`:
- Around line 71-82: The code takes the address of the range loop variable
(config) when assigning appAcls (appAcls = &config); change this to the explicit
local-copy pattern to avoid loop-variable capture confusion: create a new local
variable (e.g., cfg := config) and assign appAcls = &cfg instead for both
matches (the domain check and the app-name check) while keeping the break logic
and using the existing service.config.Apps, appAcls, and config identifiers to
locate the spots to change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a7e25ccc-eb0b-4e59-b3b4-8d0fc39ae11f
📒 Files selected for processing (3)
internal/model/config.gointernal/service/access_controls_service.gointernal/test/test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/test/test.go
- internal/model/config.go
|
Whilst I can't help with code reviews, I 'm more than happy to build locally and help test this if that's of any use to you @steveiliop56 |
Member
Author
|
@arcoast it would be better to wait for an alpha/beta release. But if you like, you can test locally using the development mode and let me know if everything works correctly. Any feedback is appreciated! |
|
I don't mind, whatever is most useful for you. If you do want me to test then let me know what the labels/envvars required. |
Member
Author
|
This change is quite simple, all you need to do is set |
Rycochet
previously approved these changes
May 16, 2026
Rycochet
reviewed
May 16, 2026
Comment on lines
+66
to
+75
| app.log.App.Debug().Msg("Using Docker label provider") | ||
|
|
||
| dockerService, err := service.NewDockerService(app.log, app.ctx, &app.wg) | ||
|
|
||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to initialize docker service: %w", err) | ||
| } | ||
|
|
||
| app.services.dockerService = dockerService | ||
| return dockerService, nil |
Collaborator
There was a problem hiding this comment.
Technically there might not be a docker provider available either - more defensive to check for it and report if it's set to auto?
dosubot
Bot
added
size:XL
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/proxy_controller.go (1)
114-133:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't collapse rule evaluation failures into plain allow/deny results.
These checks now only consume a
bool, so malformed ACL path/IP rules can no longer reachhandleError(...)as configuration errors. That turns bad ACLs into ordinary auth decisions and makes misconfiguration much harder to detect in production.🤖 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/controller/proxy_controller.go` around lines 114 - 133, The rule evaluation calls (policyEngine.Evaluate) are treating malformed rules as simple booleans, hiding config errors; update the three evaluations for service.RuleIPBypassed, service.RuleAuthEnabled, and service.RuleIPAllowed to capture and check an error return (e.g., allowed, err := controller.policyEngine.Evaluate(...)); if err != nil call controller.handleError(c, err) and return; only proceed to controller.setHeaders and c.JSON or the !allowed branch when err is nil and the boolean result is definitive. Ensure you propagate errors from the policy engine API (or change Evaluate to return (bool, error) if needed) and keep existing handlers (controller.setHeaders, controller.log.App.Debug, c.JSON) unchanged when no error occurs.
♻️ Duplicate comments (1)
internal/service/access_controls_rules.go (1)
189-192:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid mutating shared ACL/config slice backing arrays during merge.
The current
appendmerge can write into existing backing arrays (ctx.ACLs.IP.*) if capacity allows, causing cross-request leakage/corruption of filter state.Safe merge without aliasing
- // Merge the global and app IP filter - blockedIps := append(ctx.ACLs.IP.Block, rule.Config.Auth.IP.Block...) - allowedIPs := append(ctx.ACLs.IP.Allow, rule.Config.Auth.IP.Allow...) + // Merge the global and app IP filter (copy into fresh slices) + blockedIps := make([]string, 0, len(ctx.ACLs.IP.Block)+len(rule.Config.Auth.IP.Block)) + blockedIps = append(blockedIps, ctx.ACLs.IP.Block...) + blockedIps = append(blockedIps, rule.Config.Auth.IP.Block...) + + allowedIPs := make([]string, 0, len(ctx.ACLs.IP.Allow)+len(rule.Config.Auth.IP.Allow)) + allowedIPs = append(allowedIPs, ctx.ACLs.IP.Allow...) + allowedIPs = append(allowedIPs, rule.Config.Auth.IP.Allow...)🤖 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/service/access_controls_rules.go` around lines 189 - 192, The merge of IP lists using append (creating blockedIps and allowedIPs from ctx.ACLs.IP.Block/Allow and rule.Config.Auth.IP.Block/Allow) can mutate the backing arrays of the shared ctx.ACLs slices; allocate a fresh slice and copy both source slices into it instead of appending into an existing backing array—e.g. create a new slice with capacity equal to len(ctx.ACLs.IP.Block)+len(rule.Config.Auth.IP.Block) (or start from append([]string{}, ctx.ACLs.IP.Block...) and then append the rule slice), and do the same for allowedIPs; update the variables blockedIps and allowedIPs accordingly so ctx.ACLs.IP.* is never mutated.
🧹 Nitpick comments (4)
internal/utils/security_utils.go (1)
84-93: ⚡ Quick winRegex filter should not fall through to comma-separated check.
When a regex filter is detected (wrapped in
/...) but doesn't match, the code falls through to the comma-separated comparison. This creates an edge case where ifinputis literally the regex string itself (e.g., input"/^admin$/"against filter"/^admin$/"), it would match unexpectedly.Additionally, filters like
/or//would compile an empty regex that matches everything.♻️ Proposed fix
if strings.HasPrefix(filter, "/") && strings.HasSuffix(filter, "/") { + if len(filter) < 3 { + return false, fmt.Errorf("invalid regex filter: empty pattern") + } re, err := regexp.Compile(filter[1 : len(filter)-1]) if err != nil { return false, fmt.Errorf("invalid regex filter: %w", err) } - if re.MatchString(input) { - return true, nil - } + return re.MatchString(input), nil }🤖 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/utils/security_utils.go` around lines 84 - 93, When a filter is wrapped in slashes (the code branch that checks strings.HasPrefix(filter, "/") && strings.HasSuffix(filter, "/")), ensure you treat it exclusively as a regex: validate that the inner pattern is non-empty (e.g., len(filter) > 2 and filter[1:len(filter)-1] != "") to avoid compiling "/" or "//" into an empty regex, compile the pattern and if compilation fails return the error, and if compilation succeeds but re.MatchString(input) is false return (false, nil) immediately instead of falling through to the comma-separated logic; update the existing handling around variables filter, input and the regexp.Compile call to enforce these checks.internal/service/access_control_rules_test.go (1)
12-181: ⚡ Quick winAdd nil-context/user-context regression cases for all rules.
Given the rule evaluators are security-critical request-path logic, add table rows for
ctx == nilandctx.UserContext == nil(where applicable) to assert deterministicEffect*outcomes and prevent panic regressions.Also applies to: 183-309, 311-409, 411-518, 520-636, 638-702
🤖 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/service/access_control_rules_test.go` around lines 12 - 181, Add regression table rows to TestUserAllowedRule (and the other rule tests mentioned) to cover nil inputs: create two cases — one with ctx == nil and one with ctx != nil but ctx.UserContext == nil — and assert the rule's Evaluate method (UserAllowedRule.Evaluate) returns a deterministic Effect (use EffectAbstain) for both; repeat the same two-row additions for each corresponding test function covering the other rule types so Evaluate handles nil ACLContext and nil UserContext without panicking.internal/service/access_controls_service.go (1)
37-43: ⚡ Quick winUse an explicit local copy when returning a pointer from
for range.Returning
&configfrom the range variable works but is easy to misread and has loop-variable semantics pitfalls. Prefer a named local copy before taking the address.Suggested pattern
- appAcls = &config + result := config + appAcls = &result @@ - appAcls = &config + result := config + appAcls = &resultBased on learnings: prefer the explicit local copy pattern instead of returning
&<range-variable>directly for clarity and to avoid loop-variable capture confusion.🤖 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/service/access_controls_service.go` around lines 37 - 43, The code takes the address of the for-range variable `config` when assigning to `appAcls` (e.g., appAcls = &config) which can be confusing and error-prone due to loop-variable semantics; fix it by creating an explicit local copy (e.g., copy := config) inside the loop before assigning its address to `appAcls`, and do the same in both match branches (the domain-match branch and the app-name branch where you call service.log.App.Debug and then set appAcls) so the pointer refers to a stable, clearly named local variable.internal/controller/proxy_controller_test.go (1)
367-405: ⚡ Quick winAdd at least one controller-level case for the
denyACL policy.These tests wire the new policy engine correctly, but they still only exercise the default policy. Since the PR's main behavior is deny-by-default, please cover one unauthenticated request and one explicitly authorized request with the policy set to
deny.🤖 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/controller/proxy_controller_test.go` around lines 367 - 405, Add controller-level test cases that exercise the deny-by-default ACL: in the tests loop that creates the Gin router and calls controller.NewProxyController(log, runtime, group, aclsService, authService, policyEngine), configure the policy engine to use the deny default and add two cases—one unauthenticated request expected to be denied (HTTP 403/401) and one request explicitly authorized via the ACLs/policy (e.g., using aclsService or injecting an allow rule into policyEngine) expected to succeed; ensure the new cases are added to the existing tests slice and use the same setup (router, middlewares, recorder) so they run with the controller instance created by NewProxyController.
🤖 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/bootstrap/service_bootstrap.go`:
- Around line 51-82: The getLabelProvider currently falls back to Docker for any
unrecognized app.config.LabelProvider; change it to explicitly accept only
"none", "kubernetes", "docker", or "auto" and return an error for any other
value. Update the selection logic in getLabelProvider: compute useKubernetes as
before, compute useDocker as app.config.LabelProvider == "docker" ||
(app.config.LabelProvider == "auto" && !inCluster), and if neither matches
return fmt.Errorf("unknown label provider: %q", app.config.LabelProvider). Keep
the existing initialization calls (service.NewKubernetesService and
service.NewDockerService) and assignments to
app.services.kubernetesService/app.services.dockerService unchanged.
- Line 25: The code passes &labelProvider (a pointer-to-interface) into
NewAccessControlsService, which leads to a non-nil pointer that may hold a nil
interface and causes a panic in GetAccessControls; update the handling so
callers and service check the actual interface value: either (A) change
getLabelProvider() to return a concrete pointer type (avoid
pointer-to-interface) and propagate that through NewAccessControlsService and
the service struct, or (B) keep the current signature but change the nil checks
in GetAccessControls (and any callers) to verify both pointer and the underlying
interface—i.e., ensure you test service.labelProvider != nil &&
*service.labelProvider != nil before calling
(*service.labelProvider).GetLabels(); adjust NewAccessControlsService and the
labelProvider variable usage accordingly to eliminate the pointer-to-interface
anti-pattern.
In `@internal/service/access_controls_rules.go`:
- Around line 27-35: The Evaluate method on UserAllowedRule (and other rule
Evaluate methods) dereferences ctx and nested fields like ctx.UserContext and
ctx.IP without full nil guards, risking nil-pointer panics; add explicit nil
checks before any nested access (e.g., verify ctx != nil, then ctx.UserContext
!= nil, and ctx.ACLs != nil) in UserAllowedRule.Evaluate and the other mentioned
Evaluate implementations so you only call utils.CheckFilter or access
ctx.IP/ctx.UserContext when those pointers are non-nil; return EffectAbstain or
an appropriate effect early when a required nested field is nil to short-circuit
safely.
In `@internal/service/access_controls_service.go`:
- Around line 34-45: The current loop over service.config.Apps can pick an
app-name fallback before finding an exact domain match because map iteration is
unordered; change the logic in the function using service.config.Apps (and
assigning to appAcls) to perform two passes: first iterate all entries and check
if config.Config.Domain == domain and set appAcls (logging via
service.log.App.Debug().Str("name", app).Msg(...)) and break on exact match, and
only if no exact match found do a second pass that checks strings.SplitN(domain,
".", 2)[0] == app to set the app-name fallback; ensure appAcls is only set by
the fallback when the exact pass found nothing.
---
Outside diff comments:
In `@internal/controller/proxy_controller.go`:
- Around line 114-133: The rule evaluation calls (policyEngine.Evaluate) are
treating malformed rules as simple booleans, hiding config errors; update the
three evaluations for service.RuleIPBypassed, service.RuleAuthEnabled, and
service.RuleIPAllowed to capture and check an error return (e.g., allowed, err
:= controller.policyEngine.Evaluate(...)); if err != nil call
controller.handleError(c, err) and return; only proceed to controller.setHeaders
and c.JSON or the !allowed branch when err is nil and the boolean result is
definitive. Ensure you propagate errors from the policy engine API (or change
Evaluate to return (bool, error) if needed) and keep existing handlers
(controller.setHeaders, controller.log.App.Debug, c.JSON) unchanged when no
error occurs.
---
Duplicate comments:
In `@internal/service/access_controls_rules.go`:
- Around line 189-192: The merge of IP lists using append (creating blockedIps
and allowedIPs from ctx.ACLs.IP.Block/Allow and rule.Config.Auth.IP.Block/Allow)
can mutate the backing arrays of the shared ctx.ACLs slices; allocate a fresh
slice and copy both source slices into it instead of appending into an existing
backing array—e.g. create a new slice with capacity equal to
len(ctx.ACLs.IP.Block)+len(rule.Config.Auth.IP.Block) (or start from
append([]string{}, ctx.ACLs.IP.Block...) and then append the rule slice), and do
the same for allowedIPs; update the variables blockedIps and allowedIPs
accordingly so ctx.ACLs.IP.* is never mutated.
---
Nitpick comments:
In `@internal/controller/proxy_controller_test.go`:
- Around line 367-405: Add controller-level test cases that exercise the
deny-by-default ACL: in the tests loop that creates the Gin router and calls
controller.NewProxyController(log, runtime, group, aclsService, authService,
policyEngine), configure the policy engine to use the deny default and add two
cases—one unauthenticated request expected to be denied (HTTP 403/401) and one
request explicitly authorized via the ACLs/policy (e.g., using aclsService or
injecting an allow rule into policyEngine) expected to succeed; ensure the new
cases are added to the existing tests slice and use the same setup (router,
middlewares, recorder) so they run with the controller instance created by
NewProxyController.
In `@internal/service/access_control_rules_test.go`:
- Around line 12-181: Add regression table rows to TestUserAllowedRule (and the
other rule tests mentioned) to cover nil inputs: create two cases — one with ctx
== nil and one with ctx != nil but ctx.UserContext == nil — and assert the
rule's Evaluate method (UserAllowedRule.Evaluate) returns a deterministic Effect
(use EffectAbstain) for both; repeat the same two-row additions for each
corresponding test function covering the other rule types so Evaluate handles
nil ACLContext and nil UserContext without panicking.
In `@internal/service/access_controls_service.go`:
- Around line 37-43: The code takes the address of the for-range variable
`config` when assigning to `appAcls` (e.g., appAcls = &config) which can be
confusing and error-prone due to loop-variable semantics; fix it by creating an
explicit local copy (e.g., copy := config) inside the loop before assigning its
address to `appAcls`, and do the same in both match branches (the domain-match
branch and the app-name branch where you call service.log.App.Debug and then set
appAcls) so the pointer refers to a stable, clearly named local variable.
In `@internal/utils/security_utils.go`:
- Around line 84-93: When a filter is wrapped in slashes (the code branch that
checks strings.HasPrefix(filter, "/") && strings.HasSuffix(filter, "/")), ensure
you treat it exclusively as a regex: validate that the inner pattern is
non-empty (e.g., len(filter) > 2 and filter[1:len(filter)-1] != "") to avoid
compiling "/" or "//" into an empty regex, compile the pattern and if
compilation fails return the error, and if compilation succeeds but
re.MatchString(input) is false return (false, nil) immediately instead of
falling through to the comma-separated logic; update the existing handling
around variables filter, input and the regexp.Compile call to enforce these
checks.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 282799e8-3593-4bd5-9e7f-b5a47b9b4912
📒 Files selected for processing (12)
internal/bootstrap/app_bootstrap.gointernal/bootstrap/router_bootstrap.gointernal/bootstrap/service_bootstrap.gointernal/controller/proxy_controller.gointernal/controller/proxy_controller_test.gointernal/service/access_control_rules_test.gointernal/service/access_controls_rules.gointernal/service/access_controls_service.gointernal/service/auth_service.gointernal/service/policy_engine.gointernal/utils/security_utils.gointernal/utils/security_utils_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/service/auth_service.go
| return fmt.Errorf("failed to initialize label provider: %w", err) | ||
| } | ||
|
|
||
| accessControlsService := service.NewAccessControlsService(app.log, app.config, &labelProvider) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd '^access_controls_service\.go$' internal/service | head -n1)"
echo "Inspecting: $file"
sed -n '1,260p' "$file"
echo
echo "Relevant nil-handling checks:"
rg -n -C3 'NewAccessControlsService|labelProvider|== nil|\*.*labelProvider|GetAccessControls|GetLabels' "$file"Repository: tinyauthapp/tinyauth
Length of output: 2787
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find and inspect service_bootstrap.go around the setupServices function
file="internal/bootstrap/service_bootstrap.go"
echo "=== Checking file structure ==="
wc -l "$file"
echo ""
echo "=== Full setupServices and getLabelProvider functions ==="
rg -n 'func (app \*App) setupServices|func (app \*App) getLabelProvider' -A 50 "$file"Repository: tinyauthapp/tinyauth
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the entire bootstrap file
cat -n internal/bootstrap/service_bootstrap.goRepository: tinyauthapp/tinyauth
Length of output: 4083
Fix pointer-to-interface nil handling in AccessControlsService—will panic at runtime.
When getLabelProvider() returns nil for "none" (line 19, 53), line 25 passes &labelProvider to NewAccessControlsService. This creates a non-nil pointer to a nil interface. In GetAccessControls() (line 60), the check if service.labelProvider != nil only validates the pointer, not the interface value. Calling (*service.labelProvider).GetLabels() on a nil interface causes a panic.
The fix is to check both: replace the condition with if service.labelProvider != nil && *service.labelProvider != nil OR change getLabelProvider() to return a pointer type to avoid the anti-pattern entirely.
🤖 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/service_bootstrap.go` at line 25, The code passes
&labelProvider (a pointer-to-interface) into NewAccessControlsService, which
leads to a non-nil pointer that may hold a nil interface and causes a panic in
GetAccessControls; update the handling so callers and service check the actual
interface value: either (A) change getLabelProvider() to return a concrete
pointer type (avoid pointer-to-interface) and propagate that through
NewAccessControlsService and the service struct, or (B) keep the current
signature but change the nil checks in GetAccessControls (and any callers) to
verify both pointer and the underlying interface—i.e., ensure you test
service.labelProvider != nil && *service.labelProvider != nil before calling
(*service.labelProvider).GetLabels(); adjust NewAccessControlsService and the
labelProvider variable usage accordingly to eliminate the pointer-to-interface
anti-pattern.
Comment on lines
+51
to
+82
| func (app *BootstrapApp) getLabelProvider() (service.LabelProvider, error) { | ||
| if app.config.LabelProvider == "none" { | ||
| return nil, nil | ||
| } | ||
|
|
||
| useKubernetes := app.config.LabelProvider == "kubernetes" || | ||
| (app.config.LabelProvider == "auto" && os.Getenv("KUBERNETES_SERVICE_HOST") != "") | ||
|
|
||
| var labelProvider service.LabelProvider | ||
|
|
||
| if useKubernetes { | ||
| app.log.App.Debug().Msg("Using Kubernetes label provider") | ||
|
|
||
| kubernetesService, err := service.NewKubernetesService(app.log, app.ctx, &app.wg) | ||
|
|
||
| if err != nil { | ||
| return fmt.Errorf("failed to initialize kubernetes service: %w", err) | ||
| return nil, fmt.Errorf("failed to initialize kubernetes service: %w", err) | ||
| } | ||
|
|
||
| app.services.kubernetesService = kubernetesService | ||
| labelProvider = kubernetesService | ||
| } else { | ||
| app.log.App.Debug().Msg("Using Docker label provider") | ||
| return kubernetesService, nil | ||
| } | ||
|
|
||
| dockerService, err := service.NewDockerService(app.log, app.ctx, &app.wg) | ||
| app.log.App.Debug().Msg("Using Docker label provider") | ||
|
|
||
| if err != nil { | ||
| return fmt.Errorf("failed to initialize docker service: %w", err) | ||
| } | ||
| dockerService, err := service.NewDockerService(app.log, app.ctx, &app.wg) | ||
|
|
||
| app.services.dockerService = dockerService | ||
| labelProvider = dockerService | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to initialize docker service: %w", err) | ||
| } | ||
|
|
||
| accessControlsService := service.NewAccessControlsService(app.log, &labelProvider, app.config.Apps) | ||
| app.services.accessControlService = accessControlsService | ||
|
|
||
| oauthBrokerService := service.NewOAuthBrokerService(app.log, app.runtime.OAuthProviders, app.ctx) | ||
| app.services.oauthBrokerService = oauthBrokerService | ||
|
|
||
| authService := service.NewAuthService(app.log, app.config, app.runtime, app.ctx, &app.wg, app.services.ldapService, app.queries, app.services.oauthBrokerService) | ||
| app.services.authService = authService | ||
| app.services.dockerService = dockerService | ||
| return dockerService, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Reject unknown LabelProvider values instead of silently using Docker.
Any value other than "none", "kubernetes", or auto-in-cluster currently falls through to Docker. A typo in config will boot against the wrong label backend and change ACL resolution instead of failing fast.
🤖 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/service_bootstrap.go` around lines 51 - 82, The
getLabelProvider currently falls back to Docker for any unrecognized
app.config.LabelProvider; change it to explicitly accept only "none",
"kubernetes", "docker", or "auto" and return an error for any other value.
Update the selection logic in getLabelProvider: compute useKubernetes as before,
compute useDocker as app.config.LabelProvider == "docker" ||
(app.config.LabelProvider == "auto" && !inCluster), and if neither matches
return fmt.Errorf("unknown label provider: %q", app.config.LabelProvider). Keep
the existing initialization calls (service.NewKubernetesService and
service.NewDockerService) and assignments to
app.services.kubernetesService/app.services.dockerService unchanged.
Comment on lines
+27
to
+35
| func (rule *UserAllowedRule) Evaluate(ctx *ACLContext) Effect { | ||
| if ctx.ACLs == nil { | ||
| return EffectAbstain | ||
| } | ||
|
|
||
| if ctx.UserContext.Provider == model.ProviderOAuth { | ||
| rule.Log.App.Debug().Msg("User is an OAuth user, checking OAuth whitelist") | ||
| match, err := utils.CheckFilter(ctx.ACLs.OAuth.Whitelist, ctx.UserContext.OAuth.Email) | ||
| if err != nil { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its size
wc -l internal/service/access_controls_rules.goRepository: tinyauthapp/tinyauth
Length of output: 110
🏁 Script executed:
# Examine the primary range (27-35)
sed -n '27,35p' internal/service/access_controls_rules.goRepository: tinyauthapp/tinyauth
Length of output: 416
🏁 Script executed:
# Examine the secondary ranges
echo "=== Lines 82-90 ===" && sed -n '82,90p' internal/service/access_controls_rules.go
echo "=== Lines 116-123 ===" && sed -n '116,123p' internal/service/access_controls_rules.go
echo "=== Lines 145-147 ===" && sed -n '145,147p' internal/service/access_controls_rules.go
echo "=== Lines 184-186 ===" && sed -n '184,186p' internal/service/access_controls_rules.go
echo "=== Lines 230-232 ===" && sed -n '230,232p' internal/service/access_controls_rules.goRepository: tinyauthapp/tinyauth
Length of output: 988
🏁 Script executed:
# Get full context around each range to understand the nil-check pattern
echo "=== Lines 20-40 ===" && sed -n '20,40p' internal/service/access_controls_rules.go
echo "=== Lines 75-95 ===" && sed -n '75,95p' internal/service/access_controls_rules.go
echo "=== Lines 110-130 ===" && sed -n '110,130p' internal/service/access_controls_rules.go
echo "=== Lines 140-155 ===" && sed -n '140,155p' internal/service/access_controls_rules.go
echo "=== Lines 178-195 ===" && sed -n '178,195p' internal/service/access_controls_rules.go
echo "=== Lines 225-240 ===" && sed -n '225,240p' internal/service/access_controls_rules.goRepository: tinyauthapp/tinyauth
Length of output: 3187
Add nil guards before dereferencing context fields.
Evaluate methods dereference ctx / ctx.UserContext / ctx.IP on reachable paths without comprehensive nil checks, risking nil pointer panics (e.g., Line 32, Line 87, Line 121, Line 191, Line 235). Several methods check only ctx.ACLs == nil while still accessing ctx.UserContext or ctx itself. Short-circuit safely by guarding all nested field accesses.
Suggested hardening pattern
func (rule *UserAllowedRule) Evaluate(ctx *ACLContext) Effect {
- if ctx.ACLs == nil {
+ if ctx == nil || ctx.ACLs == nil || ctx.UserContext == nil {
return EffectAbstain
}
@@
func (rule *OAuthGroupRule) Evaluate(ctx *ACLContext) Effect {
- if ctx.ACLs == nil {
+ if ctx == nil || ctx.ACLs == nil || ctx.UserContext == nil {
return EffectAbstain
}
@@
func (rule *LDAPGroupRule) Evaluate(ctx *ACLContext) Effect {
- if ctx == nil {
+ if ctx == nil || ctx.ACLs == nil || ctx.UserContext == nil {
return EffectAbstain
}
@@
func (rule *AuthEnabledRule) Evaluate(ctx *ACLContext) Effect {
- if ctx.ACLs == nil {
+ if ctx == nil || ctx.ACLs == nil {
return EffectDeny
}
@@
func (rule *IPAllowedRule) Evaluate(ctx *ACLContext) Effect {
- if ctx.ACLs == nil {
+ if ctx == nil || ctx.ACLs == nil {
return EffectAbstain
}
@@
func (rule *IPBypassedRule) Evaluate(ctx *ACLContext) Effect {
- if ctx.ACLs == nil {
+ if ctx == nil || ctx.ACLs == nil {
return EffectDeny
}Also applies to: 82-90, 116-123, 145-147, 184-186, 230-232
🤖 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/service/access_controls_rules.go` around lines 27 - 35, The Evaluate
method on UserAllowedRule (and other rule Evaluate methods) dereferences ctx and
nested fields like ctx.UserContext and ctx.IP without full nil guards, risking
nil-pointer panics; add explicit nil checks before any nested access (e.g.,
verify ctx != nil, then ctx.UserContext != nil, and ctx.ACLs != nil) in
UserAllowedRule.Evaluate and the other mentioned Evaluate implementations so you
only call utils.CheckFilter or access ctx.IP/ctx.UserContext when those pointers
are non-nil; return EffectAbstain or an appropriate effect early when a required
nested field is nil to short-circuit safely.
Comment on lines
+34
to
45
| for app, config := range service.config.Apps { | ||
| if config.Config.Domain == domain { | ||
| acls.log.App.Debug().Str("name", app).Msg("Found matching container by domain") | ||
| service.log.App.Debug().Str("name", app).Msg("Found matching container by domain") | ||
| appAcls = &config | ||
| break // If we find a match by domain, we can stop searching | ||
| } | ||
|
|
||
| if strings.SplitN(domain, ".", 2)[0] == app { | ||
| acls.log.App.Debug().Str("name", app).Msg("Found matching container by app name") | ||
| service.log.App.Debug().Str("name", app).Msg("Found matching container by app name") | ||
| appAcls = &config | ||
| break // If we find a match by app name, we can stop searching | ||
| } |
Contributor
There was a problem hiding this comment.
Make exact-domain match deterministic before app-name fallback.
service.config.Apps is a map; current single-pass logic can return an app-name fallback before discovering an exact-domain match, depending on iteration order. Split this into two passes (exact domain first, fallback second) to avoid nondeterministic ACL selection.
🤖 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/service/access_controls_service.go` around lines 34 - 45, The
current loop over service.config.Apps can pick an app-name fallback before
finding an exact domain match because map iteration is unordered; change the
logic in the function using service.config.Apps (and assigning to appAcls) to
perform two passes: first iterate all entries and check if config.Config.Domain
== domain and set appAcls (logging via service.log.App.Debug().Str("name",
app).Msg(...)) and break on exact match, and only if no exact match found do a
second pass that checks strings.SplitN(domain, ".", 2)[0] == app to set the
app-name fallback; ensure appAcls is only set by the fallback when the exact
pass found nothing.
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
4 participants
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.
Solves #850
Need help on this PR? Tag
@codesmithwith what you need.Summary by CodeRabbit
New Features
Refactor
Tests