Skip to content

Commit 6f7138f

Browse files
Copilotnomeguy
andcommitted
fix: address security and code quality issues from code review
Co-authored-by: nomeguy <85475922+nomeguy@users.noreply.github.com>
1 parent 688cca1 commit 6f7138f

File tree

2 files changed

+41
-16
lines changed

2 files changed

+41
-16
lines changed

internal/handler/casdoor_handler.go

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -40,26 +40,31 @@ type Replacement struct {
4040
func ForwardAuthHandler(c *gin.Context) {
4141
clientcode, err := c.Cookie("client-code")
4242
if err != nil {
43-
fmt.Println("no client code found in cookie")
43+
log.Println("no client code found in cookie")
4444
ForwardAuthHandlerWithoutState(c)
4545
return
4646
}
4747
clientstate, err := c.Cookie("client-state")
4848
if err != nil {
49-
fmt.Println("no state found in cookie")
49+
log.Println("no state found in cookie")
5050
ForwardAuthHandlerWithoutState(c)
5151
return
5252
}
5353
if err := checkCode(clientcode, clientstate); err != nil {
54-
fmt.Printf("invalid code and state %s\n", err.Error())
54+
log.Printf("invalid code and state: %s\n", err.Error())
5555
ForwardAuthHandlerWithoutState(c)
5656
return
5757
}
5858
ForwardAuthHandlerWithState(c)
5959
}
6060

6161
func ForwardAuthHandlerWithoutState(c *gin.Context) {
62-
body, _ := io.ReadAll(c.Request.Body)
62+
body, err := io.ReadAll(c.Request.Body)
63+
if err != nil {
64+
log.Printf("error reading request body: %s\n", err.Error())
65+
c.JSON(http.StatusBadRequest, gin.H{"error": "failed to read request body"})
66+
return
67+
}
6368
state := httpstate.NewState(c.Request.Method, c.Request.Header, body)
6469
stateNonce, err := stateStorage.SetState(state)
6570
if err != nil {
@@ -77,17 +82,24 @@ func ForwardAuthHandlerWithoutState(c *gin.Context) {
7782
}
7883

7984
func ForwardAuthHandlerWithState(c *gin.Context) {
80-
fmt.Println("client code checked")
85+
log.Println("client code checked")
8186

8287
var replacement Replacement
8388
replacement.ShouldReplaceBody = true
8489
replacement.ShouldReplaceHeader = true
8590

8691
stateString, _ := c.Cookie("client-state")
87-
stateNonce, _ := strconv.Atoi(stateString)
92+
stateNonce, err := strconv.Atoi(stateString)
93+
if err != nil {
94+
log.Printf("invalid state nonce %s: %s\n", stateString, err.Error())
95+
replacement.ShouldReplaceBody = false
96+
replacement.ShouldReplaceHeader = false
97+
c.JSON(200, replacement)
98+
return
99+
}
88100
state, err := stateStorage.PopState(stateNonce)
89101
if err != nil {
90-
fmt.Printf("no related state found, state nonce %s\n", stateString)
102+
log.Printf("no related state found, state nonce %s: %s\n", stateString, err.Error())
91103
replacement.ShouldReplaceBody = false
92104
replacement.ShouldReplaceHeader = false
93105
c.JSON(200, replacement)
@@ -105,19 +117,26 @@ func CasdoorCallbackHandler(c *gin.Context) {
105117
var splits = strings.Split(config.CurrentConfig.PluginEndpoint, "://")
106118
if len(splits) < 2 {
107119
c.JSON(500, gin.H{
108-
"error": "invalid webhook address in configuration" + stateString,
120+
"error": "invalid webhook address in configuration",
109121
})
110122
return
111123
}
112124
domain := splits[1]
113125
c.SetCookie("client-code", code, 3600, "/", domain, false, true)
114126
c.SetCookie("client-state", stateString, 3600, "/", domain, false, true)
115-
stateNonce, _ := strconv.Atoi(stateString)
127+
stateNonce, err := strconv.Atoi(stateString)
128+
if err != nil {
129+
log.Printf("invalid state parameter %s: %s\n", stateString, err.Error())
130+
c.JSON(500, gin.H{
131+
"error": "invalid state parameter",
132+
})
133+
return
134+
}
116135
state, err := stateStorage.GetState(stateNonce)
117136
if err != nil {
118-
fmt.Printf("no related state found, state nonce %s\n", stateString)
137+
log.Printf("no related state found, state nonce %s: %s\n", stateString, err.Error())
119138
c.JSON(500, gin.H{
120-
"error": "no related state found, state nonce " + stateString,
139+
"error": "no related state found",
121140
})
122141
return
123142
}
@@ -126,7 +145,6 @@ func CasdoorCallbackHandler(c *gin.Context) {
126145
uri := state.Header.Get("X-Forwarded-URI")
127146
url := fmt.Sprintf("%s://%s%s", scheme, host, uri)
128147
c.Redirect(307, url)
129-
130148
}
131149

132150
func checkCode(code, state string) error {

internal/httpstate/state_memory_storage.go

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,9 @@
1515
package httpstate
1616

1717
import (
18+
"crypto/rand"
19+
"encoding/binary"
1820
"fmt"
19-
"math/rand"
2021
"sync"
2122
)
2223

@@ -35,7 +36,15 @@ func (s *StateMemoryStorage) SetState(state *State) (int, error) {
3536
s.Lock()
3637
defer s.Unlock()
3738

38-
nonce := rand.Int()
39+
// Generate cryptographically secure random nonce
40+
var b [8]byte
41+
if _, err := rand.Read(b[:]); err != nil {
42+
return 0, fmt.Errorf("failed to generate random nonce: %w", err)
43+
}
44+
nonce := int(binary.BigEndian.Uint64(b[:]))
45+
if nonce < 0 {
46+
nonce = -nonce
47+
}
3948
s.content[nonce] = state
4049
return nonce, nil
4150
}
@@ -50,7 +59,6 @@ func (s *StateMemoryStorage) PopState(nonce int) (*State, error) {
5059

5160
delete(s.content, nonce)
5261
return state, nil
53-
5462
}
5563
func (s *StateMemoryStorage) GetState(nonce int) (*State, error) {
5664
s.Lock()
@@ -61,5 +69,4 @@ func (s *StateMemoryStorage) GetState(nonce int) (*State, error) {
6169
}
6270

6371
return state, nil
64-
6572
}

0 commit comments

Comments
 (0)