Open
Conversation
Fixes Applied:
Issue 8 — parseState() panic (DoS)
- backend/pkg/server/services/auth.go:614-622 — moved the len(stateJSON) <= signatureLen bounds check before the slice stateJSON[:signatureLen], eliminating the panic on malformed short input.
Issue 11 — Authorization string typos (broken ACL)
- backend/pkg/server/services/users.go — removed trailing ' from all 7 privilege strings ("users.view'" → "users.view", and same for create, edit, delete). These typos caused every privilege check to silently fail.
- backend/pkg/server/services/roles.go — same fix for "roles.view'".
Issue 5 — OAuth GET callback missing state validation (CSRF)
- backend/pkg/server/services/auth.go:327-332 — AuthLoginGetCallback now validates the state query param against the session cookie value, matching the POST handler's behavior.
Issue 6 — Session expiry not enforced in middleware
- backend/pkg/server/auth/auth_middleware.go:102-109 — tryUserCookieAuthentication now checks time.Now().Unix() > exp and returns authResultFail for expired sessions, not just the /info route.
Issue 7 — Missing SameSite cookie attribute
- backend/pkg/server/services/auth.go — added SameSite: http.SameSiteLaxMode to all four sessions.Options{} blocks and the http.Cookie in setCallbackCookie.
Issue 1 (partial) — password_change_required not enforced server-side
- backend/pkg/server/response/errors.go — added ErrAuthPasswordChangeRequired (HTTP 403).
- backend/pkg/server/services/auth.go:125-129 — AuthLogin now returns 403 when user.PasswordChangeRequired == true, blocking login until the password is changed.
Issue 9 — Browser tool HTTP client has no timeout
- backend/pkg/tools/browser.go:425-428 — added Timeout: 30 * time.Second to the http.Client in callScraper, preventing indefinite hangs.
asdek
requested changes
Feb 20, 2026
Contributor
asdek
left a comment
asdek
left a comment
There was a problem hiding this comment.
thank you for this PR but there are some cases to fix from your side to accept it
| return | ||
| } | ||
|
|
||
| if user.PasswordChangeRequired { |
Contributor
There was a problem hiding this comment.
this case will handled in the UI with asking user about changing password, otherwise first login after deploy will failed
could you remove this case and ErrAuthPasswordChangeRequired from error codes?
| if !ok { | ||
| return authResultFail, errors.New("token claim invalid") | ||
| } | ||
| if time.Now().Unix() > expVal { |
Contributor
There was a problem hiding this comment.
I think, this code is unreachable because user session should invalidate earlier while checking but we can keep it for double check.
could you check it on your side?
| TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, | ||
| }} | ||
| client := &http.Client{ | ||
| Timeout: 30 * time.Second, |
Contributor
There was a problem hiding this comment.
maybe it's too small timeout, scraper has timeout 60 seconds in the service and this can caught unexpectedly error.
could you change it for 65 seconds at least?
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.
PR Summary: 🔒 Security Hardening and Critical Bug Fixes
Overview
This PR addresses several security vulnerabilities and critical bugs identified in the authentication service, session management, and external tool integrations. The fixes range from preventing DoS attacks to correcting broken access control logic (ACL).
Key Changes
🛡️ Security Fixes
stateJSONbefore attempting to slice the signature. This prevents a runtime panic when processing malformed or intentionally short input.stateparameter against the stored session cookie, closing a CSRF gap in the GET handler.SameSite=Laxfor all session and callback cookies to mitigate CSRF risks.PasswordChangeRequiredset totrue, ensuring the policy is enforced before any session is granted.🛠️ Critical Bug Fixes
"users.view'"→"users.view"). These typos previously caused all permission checks to fail silently.expclaim. Sessions are now correctly invalidated as soon as they expire, even outside the/inforoute.Impact
Checklist
SameSiteattribute.