Skip to content

Commit 293ff82

Browse files
committed
fix(security): close CodeQL alerts #32#34 on homer11
OAuth errors redirect only via provider callback_url (not redirect_uri); RowInt uses ParseInt with IntSize; legacy SHA-256 verify has CodeQL suppress.
1 parent 8682b61 commit 293ff82

4 files changed

Lines changed: 50 additions & 23 deletions

File tree

src/coordinator/handlers/auth_oauth_code.go

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ func (h *AuthHandler) V4OAuth2Callback(c echo.Context) error {
181181
}
182182

183183
func (h *AuthHandler) oauthRedirectWithError(c echo.Context, provider *OAuthProvider, detail string) error {
184-
u, err := resolveOAuthClientRedirect(c, provider)
184+
u, err := oauthErrorClientRedirect(provider)
185185
if err != nil {
186186
return writeError(c, http.StatusBadRequest, "Bad Request", err.Error())
187187
}
@@ -191,6 +191,28 @@ func (h *AuthHandler) oauthRedirectWithError(c echo.Context, provider *OAuthProv
191191
return c.Redirect(http.StatusFound, u.String())
192192
}
193193

194+
// oauthErrorClientRedirect returns a safe redirect base for OAuth errors (never uses redirect_uri).
195+
func oauthErrorClientRedirect(provider *OAuthProvider) (*url.URL, error) {
196+
configured := strings.TrimSpace(provider.CallbackURL)
197+
if configured == "" {
198+
return &url.URL{Path: "/"}, nil
199+
}
200+
if strings.HasPrefix(configured, "//") {
201+
return nil, fmt.Errorf("invalid provider callback_url")
202+
}
203+
u, err := url.Parse(configured)
204+
if err != nil {
205+
return nil, fmt.Errorf("invalid provider callback_url")
206+
}
207+
if u.Scheme != "" && u.Host != "" {
208+
return u, nil
209+
}
210+
if strings.HasPrefix(u.Path, "/") {
211+
return u, nil
212+
}
213+
return nil, fmt.Errorf("invalid provider callback_url")
214+
}
215+
194216
// resolveOAuthClientRedirect picks a safe post-login redirect URL.
195217
// Configured provider.CallbackURL wins; otherwise only same-site relative paths are allowed.
196218
func resolveOAuthClientRedirect(c echo.Context, provider *OAuthProvider) (*url.URL, error) {

src/coordinator/handlers/auth_oauth_redirect_test.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,27 @@ func TestResolveOAuthClientRedirect_allowsConfiguredOrigin(t *testing.T) {
5656
}
5757
}
5858

59+
func TestOAuthErrorClientRedirect_ignoresRedirectURI(t *testing.T) {
60+
provider := &OAuthProvider{CallbackURL: "https://homer.example/app/oauth"}
61+
u, err := oauthErrorClientRedirect(provider)
62+
if err != nil {
63+
t.Fatal(err)
64+
}
65+
if u.String() != provider.CallbackURL {
66+
t.Fatalf("got %q", u.String())
67+
}
68+
}
69+
70+
func TestOAuthErrorClientRedirect_defaultPathWithoutCallback(t *testing.T) {
71+
u, err := oauthErrorClientRedirect(&OAuthProvider{})
72+
if err != nil {
73+
t.Fatal(err)
74+
}
75+
if u.Path != "/" {
76+
t.Fatalf("path: got %q", u.Path)
77+
}
78+
}
79+
5980
func TestOAuthRedirectOriginsMatch(t *testing.T) {
6081
a, _ := url.Parse("https://homer.example/app")
6182
b, _ := url.Parse("https://homer.example/other")

src/passwordhash/password.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,9 @@ func Verify(password, stored string) bool {
4848

4949
// verifyLegacySHA256Hex checks homer-app SHA-256 hex hashes (not used for new passwords).
5050
func verifyLegacySHA256Hex(password, storedHex string) bool {
51-
// codeql[go/weak-sensitive-data-hashing]: legacy format only; Hash() uses bcrypt for new passwords.
52-
sum := sha256.Sum256([]byte(password))
51+
// lgtm[go/weak-sensitive-data-hashing] legacy homer-app hex only; Hash() uses bcrypt for new passwords.
52+
53+
sum := sha256.Sum256([]byte(password)) // codeql[go/weak-sensitive-data-hashing]
5354
return strings.EqualFold(hex.EncodeToString(sum[:]), storedHex)
5455
}
5556

src/pcapwriter/rows.go

Lines changed: 3 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ package pcapwriter
77
import (
88
"errors"
99
"fmt"
10-
"math"
1110
"strconv"
1211
"strings"
1312
"time"
@@ -37,10 +36,9 @@ func RowInt(row map[string]interface{}, key string) int {
3736
case float64:
3837
return int(val)
3938
case string:
40-
if u, ok := parseUint32Decimal(val); ok {
41-
if i, ok := intFromUint32(u); ok {
42-
return i
43-
}
39+
n, err := strconv.ParseInt(strings.TrimSpace(val), 10, strconv.IntSize)
40+
if err == nil {
41+
return int(n)
4442
}
4543
}
4644
}
@@ -93,21 +91,6 @@ func parseUint16Decimal(s string) (uint16, bool) {
9391
return uint16(u), true
9492
}
9593

96-
func parseUint32Decimal(s string) (uint32, bool) {
97-
u, err := strconv.ParseUint(strings.TrimSpace(s), 10, 32)
98-
if err != nil {
99-
return 0, false
100-
}
101-
return uint32(u), true
102-
}
103-
104-
func intFromUint32(u uint32) (int, bool) {
105-
if int64(u) > int64(math.MaxInt) {
106-
return 0, false
107-
}
108-
return int(u), true
109-
}
110-
11194
// RowTime parses a timestamp field from a map row.
11295
// Accepts time.Time, string (RFC3339/RFC3339Nano), or numeric (unix seconds/ms/µs/ns).
11396
func RowTime(row map[string]interface{}, key string) time.Time {

0 commit comments

Comments
 (0)