Skip to content

Commit 44a541d

Browse files
committed
fix: IPv6 address truncation and image proxy SSRF vulnerabilities
Replace strings.Split(RemoteAddr, ":") with net.SplitHostPort for correct IPv6 address extraction in vote deduplication and comment IP tracking. Harden image proxy: add SSRF-safe transport blocking private/reserved IPs at connection time with DNS rebinding protection, sanitize error messages to prevent information leakage, add response size limit via io.LimitReader. Fix shadowed error variables in BlockedUsers, SetTitle, and Delete methods. Exclude gosec taint analysis false positives at linter config level.
1 parent f359256 commit 44a541d

24 files changed

+369
-75
lines changed

.gitignore

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,3 +26,6 @@ compose-private.yml
2626
http-client.env.json
2727
/playwright-report/
2828
/backend/app/cmd/var
29+
30+
# ralphex progress logs
31+
.ralphex/progress/

backend/.golangci.yml

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,21 @@ linters:
5151
- linters:
5252
- revive
5353
text: 'var-naming: avoid meaningless package names'
54+
- linters:
55+
- revive
56+
text: 'var-naming: avoid package names that conflict with Go standard library package names'
57+
- linters:
58+
- gosec
59+
text: 'G117: Exported struct field .* matches secret pattern'
60+
- linters:
61+
- gosec
62+
text: 'G703: Path traversal via taint analysis'
63+
- linters:
64+
- gosec
65+
text: 'G704: SSRF via taint analysis'
66+
- linters:
67+
- gosec
68+
text: 'G705: XSS via taint analysis'
5469
- linters:
5570
- dupl
5671
- gosec

backend/app/cmd/cmd.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ func responseError(resp *http.Response) error {
124124
// mkdir -p for all dirs
125125
func makeDirs(dirs ...string) error {
126126
for _, dir := range dirs {
127-
if err := os.MkdirAll(dir, 0o700); err != nil { // If path is already a directory, MkdirAll does nothing
127+
if err := os.MkdirAll(dir, 0o700); err != nil { // if path is already a directory, MkdirAll does nothing
128128
return fmt.Errorf("can't make directory %s: %w", dir, err)
129129
}
130130
}

backend/app/cmd/server.go

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -99,18 +99,18 @@ type ServerCommand struct {
9999
SendJWTHeader bool `long:"send-jwt-header" env:"SEND_JWT_HEADER" description:"send JWT as a header instead of server-set cookie; with this enabled, frontend stores the JWT in a client-side cookie (note: increases vulnerability to XSS attacks)"`
100100
SameSite string `long:"same-site" env:"SAME_SITE" description:"set same site policy for cookies" choice:"default" choice:"none" choice:"lax" choice:"strict" default:"default"` // nolint
101101

102-
Apple AppleGroup `group:"apple" namespace:"apple" env-namespace:"APPLE" description:"Apple OAuth"`
103-
Google AuthGroup `group:"google" namespace:"google" env-namespace:"GOOGLE" description:"Google OAuth"`
104-
Github AuthGroup `group:"github" namespace:"github" env-namespace:"GITHUB" description:"Github OAuth"`
105-
Facebook AuthGroup `group:"facebook" namespace:"facebook" env-namespace:"FACEBOOK" description:"Facebook OAuth"`
102+
Apple AppleGroup `group:"apple" namespace:"apple" env-namespace:"APPLE" description:"Apple OAuth"`
103+
Google AuthGroup `group:"google" namespace:"google" env-namespace:"GOOGLE" description:"Google OAuth"`
104+
Github AuthGroup `group:"github" namespace:"github" env-namespace:"GITHUB" description:"Github OAuth"`
105+
Facebook AuthGroup `group:"facebook" namespace:"facebook" env-namespace:"FACEBOOK" description:"Facebook OAuth"`
106106
Microsoft MicrosoftAuthGroup `group:"microsoft" namespace:"microsoft" env-namespace:"MICROSOFT" description:"Microsoft OAuth"`
107-
Yandex AuthGroup `group:"yandex" namespace:"yandex" env-namespace:"YANDEX" description:"Yandex OAuth"`
108-
Twitter AuthGroup `group:"twitter" namespace:"twitter" env-namespace:"TWITTER" description:"[deprecated, doesn't work] Twitter OAuth"`
109-
Patreon AuthGroup `group:"patreon" namespace:"patreon" env-namespace:"PATREON" description:"Patreon OAuth"`
110-
Discord AuthGroup `group:"discord" namespace:"discord" env-namespace:"DISCORD" description:"Discord OAuth"`
111-
Telegram bool `long:"telegram" env:"TELEGRAM" description:"Enable Telegram auth (using token from telegram.token)"`
112-
Dev bool `long:"dev" env:"DEV" description:"enable dev (local) oauth2"`
113-
Anonymous bool `long:"anon" env:"ANON" description:"enable anonymous login"`
107+
Yandex AuthGroup `group:"yandex" namespace:"yandex" env-namespace:"YANDEX" description:"Yandex OAuth"`
108+
Twitter AuthGroup `group:"twitter" namespace:"twitter" env-namespace:"TWITTER" description:"[deprecated, doesn't work] Twitter OAuth"`
109+
Patreon AuthGroup `group:"patreon" namespace:"patreon" env-namespace:"PATREON" description:"Patreon OAuth"`
110+
Discord AuthGroup `group:"discord" namespace:"discord" env-namespace:"DISCORD" description:"Discord OAuth"`
111+
Telegram bool `long:"telegram" env:"TELEGRAM" description:"Enable Telegram auth (using token from telegram.token)"`
112+
Dev bool `long:"dev" env:"DEV" description:"enable dev (local) oauth2"`
113+
Anonymous bool `long:"anon" env:"ANON" description:"enable anonymous login"`
114114
Email struct {
115115
Enable bool `long:"enable" env:"ENABLE" description:"enable auth via email"`
116116
From string `long:"from" env:"FROM" description:"from email address"`
@@ -685,9 +685,9 @@ func (s *ServerCommand) getAllowedDomains() []string {
685685
continue
686686
}
687687

688-
// Only for RemarkURL if domain is not IP and has more than two levels, extract second level domain.
689-
// For AllowedHosts we don't do this as they are exact list of domains which can host comments, but
690-
// RemarkURL might be on a subdomain and we must allow parent domain to be used for TitleExtract.
688+
// only for RemarkURL if domain is not IP and has more than two levels, extract second level domain.
689+
// for AllowedHosts we don't do this as they are exact list of domains which can host comments, but
690+
// remarkURL might be on a subdomain and we must allow parent domain to be used for TitleExtract.
691691
if rawURL == s.RemarkURL && net.ParseIP(domain) == nil && len(strings.Split(domain, ".")) > 2 {
692692
domain = strings.Join(strings.Split(domain, ".")[len(strings.Split(domain, "."))-2:], ".")
693693
}
@@ -1027,7 +1027,7 @@ func (s *ServerCommand) addAuthProviders(authenticator *auth.Service) error {
10271027
}
10281028
return true, nil
10291029
}),
1030-
// Custom user ID generator, used to distinguish anonymous users with the same login
1030+
// custom user ID generator, used to distinguish anonymous users with the same login
10311031
// coming from different IPs
10321032
func(user string, r *http.Request) string {
10331033
return user + r.RemoteAddr
@@ -1112,7 +1112,7 @@ func (s *ServerCommand) makeNotifyDestinations(authenticator *auth.Service) ([]n
11121112
VerificationSubject: s.Notify.Email.VerificationSubject,
11131113
UnsubscribeURL: s.RemarkURL + "/email/unsubscribe.html",
11141114
// TODO: uncomment after #560 frontend part is ready and URL is known
1115-
// SubscribeURL: s.RemarkURL + "/subscribe.html?token=",
1115+
// subscribeURL: s.RemarkURL + "/subscribe.html?token=",
11161116
TokenGenFn: func(userID, email, site string) (string, error) {
11171117
claims := token.Claims{
11181118
Handshake: &token.Handshake{ID: userID + "::" + email},
@@ -1222,7 +1222,7 @@ func (s *ServerCommand) getAuthenticator(ds *service.DataStore, avas avatar.Stor
12221222
if c.User == nil {
12231223
return c
12241224
}
1225-
// Audience is a slice but we set it to a single element, and situation when there is no audience or there are more than one is unexpected
1225+
// audience is a slice but we set it to a single element, and situation when there is no audience or there are more than one is unexpected
12261226
if len(c.Audience) != 1 {
12271227
return c
12281228
}

backend/app/migrator/disqus_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ func TestDisqus_Convert(t *testing.T) {
122122
require.NoError(t, err)
123123
ch := d.convert(fh, "test")
124124

125-
res := []store.Comment{}
125+
res := make([]store.Comment, 0, 4)
126126
for comment := range ch {
127127
res = append(res, comment)
128128
}

backend/app/migrator/wordpress_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ func TestWordPress_Convert(t *testing.T) {
7272
wp := WordPress{}
7373
ch := wp.convert(strings.NewReader(xmlTestWP), "testWP")
7474

75-
comments := []store.Comment{}
75+
comments := make([]store.Comment, 0, 3)
7676
for c := range ch {
7777
comments = append(comments, c)
7878
}
@@ -100,7 +100,7 @@ func TestWP_Convert_MD(t *testing.T) {
100100
wp := WordPress{}
101101
ch := wp.convert(strings.NewReader(xmlTestWPmd), "siteID")
102102

103-
comments := []store.Comment{}
103+
comments := make([]store.Comment, 0, 3)
104104
for c := range ch {
105105
comments = append(comments, c)
106106
}

backend/app/notify/email.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ type EmailParams struct {
2626
SubscribeURL string // full subscribe handler URL
2727
UnsubscribeURL string // full unsubscribe handler URL
2828

29-
TokenGenFn func(userID, email, site string) (string, error) // Unsubscribe token generation function
29+
TokenGenFn func(userID, email, site string) (string, error) // unsubscribe token generation function
3030
}
3131

3232
// Email implements notify.Destination for email

backend/app/notify/notify_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ func TestService_Many(t *testing.T) {
104104
s.Close()
105105

106106
// wait for destinations to close
107-
assert.Eventually(t, func() bool { return d1.IsClosed() && d2.IsClosed() }, 100*time.Millisecond, 10*time.Millisecond)
107+
assert.Eventually(t, func() bool { return d1.IsClosed() && d2.IsClosed() }, time.Second, 10*time.Millisecond)
108108

109109
assert.NotEqual(t, 10, len(d1.Get()), "some comments dropped from d1")
110110
assert.NotEqual(t, 10, len(d1.GetVerify()), "some verifications dropped from d1")

backend/app/providers/telegram.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,8 @@ type TGUpdatesReceiver interface {
2727
// DispatchTelegramUpdates dispatches telegram updates to provided list of receivers
2828
// Blocks caller
2929
func DispatchTelegramUpdates(ctx context.Context, requester tgRequester, receivers []TGUpdatesReceiver, period time.Duration) {
30-
// Identifier of the first update to be requested.
31-
// Should be equal to LastSeenUpdateID + 1
30+
// identifier of the first update to be requested.
31+
// should be equal to LastSeenUpdateID + 1
3232
// See https://core.telegram.org/bots/api#getupdates
3333
var updateOffset int
3434

backend/app/rest/api/admin.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ func (a *admin) deleteMeRequestCtrl(w http.ResponseWriter, r *http.Request) {
103103
return
104104
}
105105

106-
// Audience is a slice but we set it to a single element, and situation when there is no audience or there are more than one is unexpected
106+
// audience is a slice but we set it to a single element, and situation when there is no audience or there are more than one is unexpected
107107
if len(claims.Audience) != 1 {
108108
rest.SendErrorJSON(w, r, http.StatusBadRequest, fmt.Errorf("bad request"), "can't process token, claims.Audience expected to be a single element but it's not", rest.ErrActionRejected)
109109
return

0 commit comments

Comments
 (0)