Feat/echo v5#51
Merged
Merged
Conversation
- Update go.mod: github.com/labstack/echo/v4 -> v5, remove labstack/gommon
- Remove github.com/swaggo/echo-swagger (no v5 support); replace with
hand-rolled Swagger UI handler using swaggo/files/v2 and swag.ReadDoc()
- Mass import path replacement across 55 Go files (v4 -> v5)
- Fix echo.Context interface -> *echo.Context struct pointer (v5 change)
- Fix HTTPErrorHandler signature: func(err, ctx) -> func(ctx, err)
- Fix NewHTTPError 3-arg calls -> 2-arg with .Wrap(err) (v5 API change)
- Fix e.Routes() -> e.Router().Routes() (v5 API change)
- Fix e.Shutdown() -> echo.StartConfig with GracefulTimeout (v5 API change)
- Fix e.Debug field removed -> use config-based debug flag
- Fix c.Response().Status/Committed -> echo.UnwrapResponse() (v5 change)
- Fix RecoverConfig: remove LogLevel/LogErrorFunc/DisableErrorHandler fields
- Fix gommon/random -> math/rand for RequestID fallback generation
- Fix Validator interface: Validate(interface{}) -> Validate(any)
https://claude.ai/code/session_012HXUTcemrXNF2PbH2kSQ7m
echo-swagger/v2 now supports Echo v5 (github.com/swaggo/echo-swagger/v2 v2.0.1). Replacing the custom swagger handler with echoSwagger.WrapHandler also removes the last indirect pulls of echo/v4 and labstack/gommon. https://claude.ai/code/session_012HXUTcemrXNF2PbH2kSQ7m
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 13 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpgrades the web stack from Echo v4 → v5 across the codebase, converting handlers/middleware/utility APIs to pointer-based Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ServerMain as Main goroutine
participant EchoServer as Echo (server goroutine)
participant ShutdownCtx as Shutdown context
Client->>EchoServer: HTTP requests
ServerMain->>EchoServer: sc.Start(ctx, e) (start server goroutine)
Note right of EchoServer: Server runs until Shutdown requested
ServerMain->>ServerMain: wait for OS signal
ServerMain->>ShutdownCtx: create 10s shutdown ctx
ServerMain->>EchoServer: request graceful shutdown
EchoServer-->>ServerMain: closes serverDone when shutdown completes
ServerMain->>EchoServer: OnShutdownError logs (non-fatal)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
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 (5)
internal/middlewares/rbac_audit_middleware.go (2)
107-111:⚠️ Potential issue | 🟠 MajorDon't reuse the request context in the async audit write.
ctxcomes fromc.Request().Context(). For incoming server requests, that context is canceled when the handler returns, soauditRepo.Create(ctx, log)can be aborted right as the request finishes. That makes audit persistence best-effort instead of reliable. Use a detached context with its own timeout, or enqueue the record to a worker. (pkg.go.dev)🛠️ Safer pattern
go func(log *models.AuditLog) { auditCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() if err := auditRepo.Create(auditCtx, log); err != nil { logger.Errorf("Failed to save audit log: %v", err) } }(log)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/middlewares/rbac_audit_middleware.go` around lines 107 - 111, The current goroutine reuses the incoming request context (ctx) which may be canceled when the handler returns, causing auditRepo.Create(ctx, log) to be aborted; change the async write to use a detached context with its own timeout (e.g., context.WithTimeout(context.Background(), 5*time.Second)) or enqueue the log to a background worker instead, and call auditRepo.Create(auditCtx, log) inside the goroutine while ensuring the cancel function is deferred and errors are logged via logger.Errorf; update the goroutine surrounding auditRepo.Create, referencing auditRepo.Create, ctx, and log.
80-90:⚠️ Potential issue | 🟠 MajorUse
echo.ResolveResponseStatusto determine HTTP status code.The current manual status resolution does not follow Echo v5's response status precedence: when a response is committed, its status is final and errors on unwind must not override it. Additionally, the code only handles
*echo.HTTPErrorbut misses other errors implementing theHTTPStatusCoderinterface. Echo v5.1.0 providesResolveResponseStatuswhich correctly implements all precedence rules: committed responses take priority, then response-suggested status, then error status.🛠️ Suggested fix
- // Add response status - statusCode := http.StatusOK - if echoResp != nil { - statusCode = echoResp.Status - if statusCode == 0 { - statusCode = http.StatusOK - } - } - if err != nil { - if he, ok := err.(*echo.HTTPError); ok { - statusCode = he.Code - } - } + // Add response status + _, statusCode := echo.ResolveResponseStatus(c.Response(), err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/middlewares/rbac_audit_middleware.go` around lines 80 - 90, Replace the manual status resolution logic (using statusCode, echoResp and type-asserting *echo.HTTPError) with Echo's canonical resolver: call echo.ResolveResponseStatus(...) passing the current Response, echoResp and err to compute the effective status (this handles committed responses, response-suggested status and any HTTPStatusCoder errors). Ensure the echo package is imported and remove the custom *echo.HTTPError type assertion so the middleware uses the returned status from echo.ResolveResponseStatus to set statusCode.internal/applications/user/controller/user_controller.go (2)
101-102:⚠️ Potential issue | 🟡 MinorPre-existing bug: error from
dtoMapper.Mapis not checked.The error returned from
dtoMapper.Mapon line 101 is not checked before returning success on line 102. If mapping fails, the response may contain incomplete or zero-valued data.🐛 Suggested fix
err = dtoMapper.Map(&pokemonGetResponseDto, result) + if err != nil { + return response.Error(eCtx, http.StatusInternalServerError, err) + } return response.Success(eCtx, pokemonGetResponseDto)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/applications/user/controller/user_controller.go` around lines 101 - 102, The call to dtoMapper.Map(&pokemonGetResponseDto, result) assigns an error to err but that error is not checked before returning response.Success(eCtx, pokemonGetResponseDto); update the code in user_controller.go to check err after calling dtoMapper.Map (handle the error from dtoMapper.Map), and if non-nil return an error response (e.g., response.Error or appropriate failure using eCtx) instead of response.Success; reference dtoMapper.Map, pokemonGetResponseDto, err, response.Success and eCtx when making the change so the mapping failure yields a proper error response.
35-36:⚠️ Potential issue | 🟠 MajorPre-existing bug: error from
strconv.Atoiis silently ignored.The error from
strconv.Atoion line 35 is assigned toerr, but line 36 immediately reassignserrwith the result of the service call, causing the conversion error to be silently ignored. If the ID is not a valid integer,idStringwill be 0 and the subsequent service call may return unexpected results.🐛 Suggested fix
idString, err := strconv.Atoi(userGetReq.ID) + if err != nil { + return response.Error(eCtx, http.StatusBadRequest, err) + } user, err := c.service.GetById(eCtx.Request().Context(), uint32(idString))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/applications/user/controller/user_controller.go` around lines 35 - 36, The strconv.Atoi call of userGetReq.ID assigns err but it gets overwritten by c.service.GetById, so conversion errors are ignored; fix by checking the error returned from strconv.Atoi immediately after the call (e.g., if err != nil { return/respond with a bad-request error referencing invalid userGetReq.ID }) before calling c.service.GetById, and avoid reusing the same err without handling; ensure you pass the validated idString (converted to uint32) into c.service.GetById using the existing eCtx.Request().Context().internal/applications/rbac/controllers/policy_controller.go (1)
78-79:⚠️ Potential issue | 🟠 MajorPre-existing bug: potential nil pointer dereference on
req.TenantID.Line 79 dereferences
*req.TenantID, but the condition on line 78 only checksreq.Role != nil. Ifreq.Roleis non-nil butreq.TenantIDis nil, this will panic.🐛 Suggested fix
- } else if req.Role != nil { - casbinPolicies, err := c.policyService.GetPoliciesByRole(ctx.Request().Context(), *req.Role, *req.TenantID) + } else if req.Role != nil && req.TenantID != nil { + casbinPolicies, err := c.policyService.GetPoliciesByRole(ctx.Request().Context(), *req.Role, *req.TenantID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/applications/rbac/controllers/policy_controller.go` around lines 78 - 79, The code dereferences req.TenantID when only req.Role is checked, risking a nil pointer; update the conditional that calls policyService.GetPoliciesByRole so it verifies req.TenantID is non-nil before using *req.TenantID (e.g., change the branch to check both req.Role != nil && req.TenantID != nil), and if TenantID is nil return a clear error/validation response instead of dereferencing; locate the usage in policy_controller.go around the call to c.policyService.GetPoliciesByRole to apply this fix.
🧹 Nitpick comments (5)
config/config.go (1)
214-215:SetDebugModenow behaves like a no-op setter; consider renaming/deprecating.The current name implies state mutation, but this implementation only logs. A clearer API will reduce future misuse.
♻️ Suggested refactor
-func SetDebugMode(_ *echo.Echo, debug bool) { - logger.Debugf("Debug mode set to %v", debug) -} +// Deprecated: no longer mutates Echo runtime state. +func SetDebugMode(_ *echo.Echo, debug bool) { + logger.Debugf("Debug mode set to %v", debug) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/config.go` around lines 214 - 215, SetDebugMode currently only logs the value and doesn't mutate any state; either make it actually set debug mode on the Echo instance or rename/deprecate it to avoid confusion. Fix by one of two minimal options: (A) implement state change inside SetDebugMode by calling echoServer.HideBanner = !debug and/or echoServer.Debug = debug (or set your global logger level) so the function truly sets debug on the provided *echo.Echo, and update callers accordingly; or (B) rename the function to LogDebugMode/DebugModeLog, add a deprecation comment to SetDebugMode, and update all call sites to the new name so the API clearly reflects it is only logging. Reference: the SetDebugMode function in config.go.pkg/errors/generic_handler.go (1)
14-21: Silent failure when UnwrapResponse fails.When
echo.UnwrapResponse()returns an error, the status is not set and the handler silently returnsnil. Consider logging or handling this edge case:💡 Suggested improvement
func (g *GenericHandler) Handle(err error, ctx *echo.Context) error { - if resp, unwrapErr := echo.UnwrapResponse(ctx.Response()); unwrapErr == nil { resp.Status = 500 + } else { + // Log or handle unwrap failure if needed } // addition code for body or header here return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/errors/generic_handler.go` around lines 14 - 21, The GenericHandler.Handle currently ignores errors from echo.UnwrapResponse(ctx.Response()), causing silent failures; update Handle (method GenericHandler.Handle) to handle the unwrapErr path: if echo.UnwrapResponse returns an error, set an HTTP 500 on the original ctx.Response() (or call ctx.Response().WriteHeader(500)/SetStatus if available) and log the unwrapErr via your logger (or return a wrapped error) instead of returning nil, while preserving the existing success path that sets resp.Status = 500 when unwrapErr == nil.internal/middlewares/registry.go (1)
30-32: Recovery middleware behavior change may warrant attention.The Echo v5
RecoverConfigno longer supportsLogLevel,LogErrorFunc, orDisableErrorHandler. The previous configuration included custom stack trace logging andDisableErrorHandler: true.With the current minimal config, panics will now be passed to the
HTTPErrorHandler(default behavior) instead of being handled internally. Verify this aligns with your error handling strategy inpkg/errors/handler.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/middlewares/registry.go` around lines 30 - 32, Echo v5 removed RecoverConfig options so current middleware.RecoverWithConfig(middleware.RecoverConfig{DisablePrintStack: !mainConfig.App().Debug}) will forward panics to the HTTPErrorHandler; update code to either (A) implement a custom recover middleware (replace middleware.RecoverWithConfig) that captures panics, logs stack traces when mainConfig.App().Debug is true and writes a safe response (restoring prior DisableErrorHandler behavior), or (B) ensure pkg/errors/handler.go's HTTPErrorHandler properly handles panic values (inspect error types/values and produce an appropriate response and logs). Locate middleware.RecoverWithConfig/RecoverConfig usage in registry.go and adjust accordingly, or modify HTTPErrorHandler in pkg/errors/handler.go to handle panics from RecoverWithConfig.internal/applications/auth/controller/auth_controller.go (1)
152-152: Remove debug print statement.
fmt.Println("AuthController.Me called")appears to be leftover debug code. Consider removing it or using the structured logger instead.♻️ Suggested fix
func (c *AuthController) Me(eCtx *echo.Context) error { - fmt.Println("AuthController.Me called") // Get auth context from middleware🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/applications/auth/controller/auth_controller.go` at line 152, Remove the stray debug print in the AuthController.Me handler: replace or delete the fmt.Println("AuthController.Me called") call inside the Me method of AuthController; if logging is desired use the existing structured logger used elsewhere in the controller (e.g., c.logger or AuthController's logger) to emit a debug-level message instead of fmt.Println, ensuring consistency with the app's logging approach.internal/applications/rbac/controllers/audit_controller.go (1)
112-113: Pre-existing risk: unchecked type assertions may panic.The type assertions
log.PolicyBefore.(map[string]interface{})andlog.PolicyAfter.(map[string]interface{})will panic if the underlying value isnilor a different type. This is not introduced by this PR but is worth noting.Consider using the comma-ok idiom for safety:
🛡️ Suggested defensive check
- PolicyBefore: log.PolicyBefore.(map[string]interface{}), - PolicyAfter: log.PolicyAfter.(map[string]interface{}), + PolicyBefore: toMapOrEmpty(log.PolicyBefore), + PolicyAfter: toMapOrEmpty(log.PolicyAfter),Where
toMapOrEmptyis a helper that safely handles nil or type mismatch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/applications/rbac/controllers/audit_controller.go` around lines 112 - 113, The direct type assertions on log.PolicyBefore and log.PolicyAfter can panic if the value is nil or the wrong type; update the code that builds the response (the lines referencing log.PolicyBefore and log.PolicyAfter) to perform safe assertions using the comma-ok idiom or a helper like toMapOrEmpty that returns an empty map on nil/mismatch, and then use that safe value for PolicyBefore and PolicyAfter to prevent runtime panics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/server/web_server.go`:
- Around line 15-17: The Render method signature on TemplateRenderer is using
the old parameter order; update the method to match Echo v5 by changing func (t
*TemplateRenderer) Render(...) to func (t *TemplateRenderer) Render(c
*echo.Context, w io.Writer, templateName string, data any) error, adjust imports
to include io and use templateName and data when calling
t.templates.ExecuteTemplate (writing to w instead of http.ResponseWriter), and
ensure the context parameter is the first argument named c; keep function name
TemplateRenderer.Render and the call to t.templates.ExecuteTemplate as the
template executor to render into w.
In `@pkg/errors/bun_handler.go`:
- Around line 20-33: The handler currently unwraps the response with
echo.UnwrapResponse(ctx.Response()) and returns nil for sql.ErrNoRows and
sql.ErrTxDone even if resp == nil, which swallows the error; update the switch
arms in bun_handler.go so you only set resp.Status and return nil when resp !=
nil (keep logger.Debugf for ErrNoRows), and if resp == nil do not return nil but
return the original err (i.e., let the error propagate to fallback handlers) for
both errors checked via errors.Is(err, sql.ErrNoRows) and errors.Is(err,
sql.ErrTxDone).
In `@pkg/logger/logger.go`:
- Around line 146-149: Replace the use of echo.UnwrapResponse(...).Status (which
can be zero) with echo.ResolveResponseStatus(eCtx.Response()) when computing
statusCode in the logging path so uncommitted responses are normalized to 200;
update the code that sets the local variable statusCode (and any logic that
reads it) to call echo.ResolveResponseStatus(eCtx.Response()) instead of
echo.UnwrapResponse(eCtx.Response()).Status to avoid emitting 0 as the HTTP
status.
---
Outside diff comments:
In `@internal/applications/rbac/controllers/policy_controller.go`:
- Around line 78-79: The code dereferences req.TenantID when only req.Role is
checked, risking a nil pointer; update the conditional that calls
policyService.GetPoliciesByRole so it verifies req.TenantID is non-nil before
using *req.TenantID (e.g., change the branch to check both req.Role != nil &&
req.TenantID != nil), and if TenantID is nil return a clear error/validation
response instead of dereferencing; locate the usage in policy_controller.go
around the call to c.policyService.GetPoliciesByRole to apply this fix.
In `@internal/applications/user/controller/user_controller.go`:
- Around line 101-102: The call to dtoMapper.Map(&pokemonGetResponseDto, result)
assigns an error to err but that error is not checked before returning
response.Success(eCtx, pokemonGetResponseDto); update the code in
user_controller.go to check err after calling dtoMapper.Map (handle the error
from dtoMapper.Map), and if non-nil return an error response (e.g.,
response.Error or appropriate failure using eCtx) instead of response.Success;
reference dtoMapper.Map, pokemonGetResponseDto, err, response.Success and eCtx
when making the change so the mapping failure yields a proper error response.
- Around line 35-36: The strconv.Atoi call of userGetReq.ID assigns err but it
gets overwritten by c.service.GetById, so conversion errors are ignored; fix by
checking the error returned from strconv.Atoi immediately after the call (e.g.,
if err != nil { return/respond with a bad-request error referencing invalid
userGetReq.ID }) before calling c.service.GetById, and avoid reusing the same
err without handling; ensure you pass the validated idString (converted to
uint32) into c.service.GetById using the existing eCtx.Request().Context().
In `@internal/middlewares/rbac_audit_middleware.go`:
- Around line 107-111: The current goroutine reuses the incoming request context
(ctx) which may be canceled when the handler returns, causing
auditRepo.Create(ctx, log) to be aborted; change the async write to use a
detached context with its own timeout (e.g.,
context.WithTimeout(context.Background(), 5*time.Second)) or enqueue the log to
a background worker instead, and call auditRepo.Create(auditCtx, log) inside the
goroutine while ensuring the cancel function is deferred and errors are logged
via logger.Errorf; update the goroutine surrounding auditRepo.Create,
referencing auditRepo.Create, ctx, and log.
- Around line 80-90: Replace the manual status resolution logic (using
statusCode, echoResp and type-asserting *echo.HTTPError) with Echo's canonical
resolver: call echo.ResolveResponseStatus(...) passing the current Response,
echoResp and err to compute the effective status (this handles committed
responses, response-suggested status and any HTTPStatusCoder errors). Ensure the
echo package is imported and remove the custom *echo.HTTPError type assertion so
the middleware uses the returned status from echo.ResolveResponseStatus to set
statusCode.
---
Nitpick comments:
In `@config/config.go`:
- Around line 214-215: SetDebugMode currently only logs the value and doesn't
mutate any state; either make it actually set debug mode on the Echo instance or
rename/deprecate it to avoid confusion. Fix by one of two minimal options: (A)
implement state change inside SetDebugMode by calling echoServer.HideBanner =
!debug and/or echoServer.Debug = debug (or set your global logger level) so the
function truly sets debug on the provided *echo.Echo, and update callers
accordingly; or (B) rename the function to LogDebugMode/DebugModeLog, add a
deprecation comment to SetDebugMode, and update all call sites to the new name
so the API clearly reflects it is only logging. Reference: the SetDebugMode
function in config.go.
In `@internal/applications/auth/controller/auth_controller.go`:
- Line 152: Remove the stray debug print in the AuthController.Me handler:
replace or delete the fmt.Println("AuthController.Me called") call inside the Me
method of AuthController; if logging is desired use the existing structured
logger used elsewhere in the controller (e.g., c.logger or AuthController's
logger) to emit a debug-level message instead of fmt.Println, ensuring
consistency with the app's logging approach.
In `@internal/applications/rbac/controllers/audit_controller.go`:
- Around line 112-113: The direct type assertions on log.PolicyBefore and
log.PolicyAfter can panic if the value is nil or the wrong type; update the code
that builds the response (the lines referencing log.PolicyBefore and
log.PolicyAfter) to perform safe assertions using the comma-ok idiom or a helper
like toMapOrEmpty that returns an empty map on nil/mismatch, and then use that
safe value for PolicyBefore and PolicyAfter to prevent runtime panics.
In `@internal/middlewares/registry.go`:
- Around line 30-32: Echo v5 removed RecoverConfig options so current
middleware.RecoverWithConfig(middleware.RecoverConfig{DisablePrintStack:
!mainConfig.App().Debug}) will forward panics to the HTTPErrorHandler; update
code to either (A) implement a custom recover middleware (replace
middleware.RecoverWithConfig) that captures panics, logs stack traces when
mainConfig.App().Debug is true and writes a safe response (restoring prior
DisableErrorHandler behavior), or (B) ensure pkg/errors/handler.go's
HTTPErrorHandler properly handles panic values (inspect error types/values and
produce an appropriate response and logs). Locate
middleware.RecoverWithConfig/RecoverConfig usage in registry.go and adjust
accordingly, or modify HTTPErrorHandler in pkg/errors/handler.go to handle
panics from RecoverWithConfig.
In `@pkg/errors/generic_handler.go`:
- Around line 14-21: The GenericHandler.Handle currently ignores errors from
echo.UnwrapResponse(ctx.Response()), causing silent failures; update Handle
(method GenericHandler.Handle) to handle the unwrapErr path: if
echo.UnwrapResponse returns an error, set an HTTP 500 on the original
ctx.Response() (or call ctx.Response().WriteHeader(500)/SetStatus if available)
and log the unwrapErr via your logger (or return a wrapped error) instead of
returning nil, while preserving the existing success path that sets resp.Status
= 500 when unwrapErr == nil.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94b0e480-4ca1-4401-a229-ab46edd01c1f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (56)
cmd/main.gocmd/server/rest_server.gocmd/server/web_server.goconfig/config.gogo.modinternal/applications/auth/controller/auth_controller.gointernal/applications/auth/controller/auth_controller_test.gointernal/applications/auth/controller/auth_routes.gointernal/applications/auth/registry.gointernal/applications/auth/service/auth_service_test.gointernal/applications/health/controller/health_controller.gointernal/applications/health/registry.gointernal/applications/notification/controller/notification_controller.gointernal/applications/notification/controller/notification_routes.gointernal/applications/notification/registry.gointernal/applications/rbac/controllers/audit_controller.gointernal/applications/rbac/controllers/enforcement_controller.gointernal/applications/rbac/controllers/policy_controller.gointernal/applications/rbac/controllers/role_controller.gointernal/applications/rbac/controllers/user_role_controller.gointernal/applications/rbac/registry.gointernal/applications/user/controller/user_controller.gointernal/applications/user/controller/user_routes.gointernal/applications/user/registry.gointernal/middlewares/cors_middleware.gointernal/middlewares/log_middleware.gointernal/middlewares/rbac_audit_middleware.gointernal/middlewares/rbac_enforcement_middleware.gointernal/middlewares/registry.gointernal/middlewares/req_context_middleware.gointernal/middlewares/req_injector_middleware.gointernal/middlewares/requestid_middleware.gointernal/middlewares/tenant_context_middleware.gointernal/middlewares/timeout_middleware.gointernal/middlewares/validator_middleware.gointernal/middlewares/version_middleware.gointernal/middlewares/version_middleware_test.gopkg/authenticator/auth.gopkg/authenticator/auth_middleware.gopkg/authenticator/jwt.gopkg/authenticator/jwt_test.gopkg/authenticator/jwt_token.gopkg/errors/bun_handler.gopkg/errors/echo_handler.gopkg/errors/generic_handler.gopkg/errors/handler.gopkg/errors/oops_handler.gopkg/errors/registry.gopkg/logger/logger.gopkg/logger/logger_test.gopkg/utils/response/response_json_builder.gopkg/validator/helper.gopkg/validator/mocks/mock_context_validator.gopkg/validator/translator.gopkg/versioning/version.gopkg/versioning/version_test.go
Inline comments: - web_server.go: fix TemplateRenderer.Render signature to match Echo v5 Renderer interface (c *echo.Context, w io.Writer, templateName string, data any) - bun_handler.go: propagate error when echo.UnwrapResponse returns nil resp instead of silently returning nil and swallowing the error - logger.go: use echo.ResolveResponseStatus instead of UnwrapResponse to normalise uncommitted responses to 200 (avoids logging status=0) Outside diff comments: - policy_controller.go: guard *req.TenantID dereference behind nil check to prevent panic when Role is set but TenantID is nil - user_controller.go: check strconv.Atoi error before calling GetById; check dtoMapper.Map error in GetPokemon before returning success response - rbac_audit_middleware.go: replace manual status code resolution with echo.ResolveResponseStatus(c.Response(), err); use detached context.WithTimeout(context.Background(), 5s) in audit goroutine to avoid cancellation when the request context is done Nitpick comments: - auth_controller.go: remove fmt.Println debug print from Me handler and drop now-unused fmt import - audit_controller.go: replace direct type assertions on PolicyBefore/PolicyAfter with safe toMapOrEmpty helper to prevent runtime panics on nil/type mismatch - generic_handler.go: fall back to WriteHeader(500) when echo.UnwrapResponse fails instead of returning nil without setting any status https://claude.ai/code/session_012HXUTcemrXNF2PbH2kSQ7m
Contributor
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 (3)
internal/applications/rbac/controllers/audit_controller.go (2)
230-242:⚠️ Potential issue | 🔴 CriticalBlock path traversal in export filename handling.
Line 230 accepts
req.FileNameand Line 241 joins it directly into a writable path. A crafted filename (for example with../) can escape./exports/auditand overwrite arbitrary files.🔐 Proposed fix
import ( "fmt" "net/http" "os" "path/filepath" + "strings" "time" @@ fileName := req.FileName if fileName == "" { fileName = fmt.Sprintf("audit_logs_%s.%s", time.Now().Format("20060102_150405"), req.Format) } + if strings.Contains(fileName, "/") || strings.Contains(fileName, "\\") { + return response.Error(ctx, http.StatusBadRequest, echo.NewHTTPError(http.StatusBadRequest, "invalid file name")) + } + fileName = filepath.Base(fileName) + if fileName == "." || fileName == ".." || fileName == "" { + return response.Error(ctx, http.StatusBadRequest, echo.NewHTTPError(http.StatusBadRequest, "invalid file name")) + } @@ filePath := filepath.Join(exportDir, fileName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/applications/rbac/controllers/audit_controller.go` around lines 230 - 242, The code currently trusts req.FileName and directly joins it into exportDir (see variables fileName and exportDir and the filepath.Join call), which allows path traversal; fix by validating/sanitizing req.FileName before using it: reject or sanitize inputs that are absolute paths, contain ".." or path separators (use filepath.IsAbs and check for string.Contains any of string(os.PathSeparator) or "/" and "\"), or better replace the name with filepath.Base and then validate it against an allowlist regex (e.g., only alphanumerics, dots, underscores, hyphens) and if it fails fallback to the generated audit_logs_<timestamp>.<ext> name; only after this sanitized/validated fileName use filepath.Join(exportDir, fileName) to create filePath.
315-316:⚠️ Potential issue | 🔴 CriticalAvoid panic-prone type assertions in recent mutations response.
Line 315 and Line 316 directly assert
map[string]interface{}from JSON-backedinterface{}fields; nil or non-map values will panic at runtime.🛠️ Proposed fix
- PolicyBefore: log.PolicyBefore.(map[string]interface{}), - PolicyAfter: log.PolicyAfter.(map[string]interface{}), + PolicyBefore: toMapOrEmpty(log.PolicyBefore), + PolicyAfter: toMapOrEmpty(log.PolicyAfter),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/applications/rbac/controllers/audit_controller.go` around lines 315 - 316, The direct assertions PolicyBefore: log.PolicyBefore.(map[string]interface{}) and PolicyAfter: log.PolicyAfter.(map[string]interface{}) are panic-prone; replace them with safe checks using the comma-ok idiom or a type switch on log.PolicyBefore and log.PolicyAfter (e.g., attempt map[string]interface{} with ok, handle nil/non-map by assigning nil or an empty map, and optionally log the unexpected type). Update the code that builds the recent mutations response (the usage of the fields PolicyBefore and PolicyAfter) to use these safe-converted values instead of raw assertions so it never panics on non-map or nil values.internal/applications/auth/controller/auth_controller.go (1)
56-84:⚠️ Potential issue | 🟠 MajorAlign the Swagger
@Routerpaths with the versioned routes.
RegisterRoutesV1mountsRegister,RefreshToken,Logout, andMeon the same versioned group asLogin, but these annotations drop that prefix. The generated Swagger will point consumers to URLs that do not exist.🛠️ Proposed fix
-// `@Router` /auth/register [post] +// `@Router` /202601/auth/register [post] ... -// `@Router` /auth/refresh [post] +// `@Router` /202601/auth/refresh [post] ... -// `@Router` /auth/logout [post] +// `@Router` /202601/auth/logout [post] ... -// `@Router` /auth/me [get] +// `@Router` /202601/auth/me [get]Also applies to: 86-118, 120-136, 138-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/applications/auth/controller/auth_controller.go` around lines 56 - 84, Swagger `@Router` annotations for controller methods (Register, RefreshToken, Logout, Me) don't include the API version prefix used by RegisterRoutesV1, causing generated docs to point to nonexistent endpoints; update each method's `@Router` line (e.g., in Register, RefreshToken, Logout, Me) to include the same versioned path prefix used by RegisterRoutesV1 (for example change /auth/register to /v1/auth/register or the exact prefix used by RegisterRoutesV1) so the annotations match the actual mounted routes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/applications/auth/controller/auth_controller.go`:
- Around line 56-84: Swagger `@Router` annotations for controller methods
(Register, RefreshToken, Logout, Me) don't include the API version prefix used
by RegisterRoutesV1, causing generated docs to point to nonexistent endpoints;
update each method's `@Router` line (e.g., in Register, RefreshToken, Logout, Me)
to include the same versioned path prefix used by RegisterRoutesV1 (for example
change /auth/register to /v1/auth/register or the exact prefix used by
RegisterRoutesV1) so the annotations match the actual mounted routes.
In `@internal/applications/rbac/controllers/audit_controller.go`:
- Around line 230-242: The code currently trusts req.FileName and directly joins
it into exportDir (see variables fileName and exportDir and the filepath.Join
call), which allows path traversal; fix by validating/sanitizing req.FileName
before using it: reject or sanitize inputs that are absolute paths, contain ".."
or path separators (use filepath.IsAbs and check for string.Contains any of
string(os.PathSeparator) or "/" and "\"), or better replace the name with
filepath.Base and then validate it against an allowlist regex (e.g., only
alphanumerics, dots, underscores, hyphens) and if it fails fallback to the
generated audit_logs_<timestamp>.<ext> name; only after this sanitized/validated
fileName use filepath.Join(exportDir, fileName) to create filePath.
- Around line 315-316: The direct assertions PolicyBefore:
log.PolicyBefore.(map[string]interface{}) and PolicyAfter:
log.PolicyAfter.(map[string]interface{}) are panic-prone; replace them with safe
checks using the comma-ok idiom or a type switch on log.PolicyBefore and
log.PolicyAfter (e.g., attempt map[string]interface{} with ok, handle
nil/non-map by assigning nil or an empty map, and optionally log the unexpected
type). Update the code that builds the recent mutations response (the usage of
the fields PolicyBefore and PolicyAfter) to use these safe-converted values
instead of raw assertions so it never panics on non-map or nil values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 385416ba-9706-4e21-9413-48802f545262
📒 Files selected for processing (9)
cmd/server/web_server.gointernal/applications/auth/controller/auth_controller.gointernal/applications/rbac/controllers/audit_controller.gointernal/applications/rbac/controllers/policy_controller.gointernal/applications/user/controller/user_controller.gointernal/middlewares/rbac_audit_middleware.gopkg/errors/bun_handler.gopkg/errors/generic_handler.gopkg/logger/logger.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/errors/bun_handler.go
- pkg/errors/generic_handler.go
- cmd/server/web_server.go
- pkg/logger/logger.go
- internal/middlewares/rbac_audit_middleware.go
- internal/applications/rbac/controllers/policy_controller.go
- internal/applications/user/controller/user_controller.go
- auth_controller.go: fix @router annotations for Register, RefreshToken, Logout, and Me to include the /202601 version prefix, matching the actual mount paths from RegisterRoutesV1 (base path is /ichi-go/api per @basepath) - audit_controller.go: fix path traversal in export filename handling; introduce sanitizeExportFileName that rejects absolute paths, "..", path separators, and non-allowlisted characters (only [a-zA-Z0-9._-] permitted), falling back to a timestamped default name on any violation - audit_controller.go: replace direct type assertions on PolicyBefore/PolicyAfter in GetRecentMutations response loop with toMapOrEmpty to prevent runtime panics on nil or unexpected types https://claude.ai/code/session_012HXUTcemrXNF2PbH2kSQ7m
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/applications/rbac/controllers/audit_controller.go`:
- Around line 129-135: toMapOrEmpty currently hides non-map audit payloads by
returning an empty map; change it so unexpected types are preserved or surfaced
instead of collapsed. Modify toMapOrEmpty to return the original value (or a
clear wrapper like map[string]interface{}{"__raw": v}) when v is not
map[string]interface{}, and add a logged warning (via the audit logger) or
return an error to callers (update callers of toMapOrEmpty to handle
interface{}/error) so deserialization mismatches are visible rather than
silently dropped; preserve the function name (toMapOrEmpty) in the refactor so
you can locate usage sites easily.
- Around line 143-156: The fallback filename in sanitizeExportFileName currently
uses time.Now().Format with second precision causing collisions; update the
logic that sets the fallback (the fallback variable in sanitizeExportFileName)
to produce a collision-resistant name—either append higher-resolution time (e.g.
nanoseconds), a short random/UUID suffix, or reserve a unique file by using
os.CreateTemp (or similar) and return that reserved name/path to ensure two
exports cannot clash. Ensure the rest of sanitizeExportFileName still
validates/returns base when provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 185ff37e-29a9-4e00-918f-7e839ef5c5b6
📒 Files selected for processing (2)
internal/applications/auth/controller/auth_controller.gointernal/applications/rbac/controllers/audit_controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/applications/auth/controller/auth_controller.go
- audit_controller.go: toMapOrEmpty now returns {"__raw": v} for unexpected
non-nil types instead of silently returning an empty map, and logs a Warnf
so deserialization mismatches are visible; nil still maps to empty map as
nil policy is an expected/normal case
- audit_controller.go: sanitizeExportFileName fallback name now uses
nanosecond precision (now.Nanosecond() as 9-digit suffix) to prevent
filename collisions when multiple exports are triggered within the same second
https://claude.ai/code/session_012HXUTcemrXNF2PbH2kSQ7m
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
2 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.
hand-rolled Swagger UI handler using swaggo/files/v2 and swag.ReadDoc()
https://claude.ai/code/session_012HXUTcemrXNF2PbH2kSQ7m
Summary by CodeRabbit