Skip to content

Commit 2659cda

Browse files
vishrclaude
andauthored
fix: SMTPS support + align toolchain with echo (#62)
* fix: SMTPS support, sync.Pool copy, printf-vet warnings; align toolchain with echo email: stdlib smtp.SendMail speaks plain + STARTTLS only; port 465 (Resend, SendGrid, etc.) hangs on the handshake. Detect port 465 and dial TLS directly via tls.DialWithDialer. Expose an optional TLSConfig for callers needing a custom root pool or ServerName, and DialTimeout so misconfigured hosts fail fast. Public Send API is unchanged. New tests cover plain dial, timeout, and invalid addresses. random: sync.Pool must not be copied after first use; New was copying from a local var into the Random struct. Construct the pool directly on the struct literal to satisfy go vet and prevent mutex corruption under concurrent use. log: the internal (*Logger).log method overloaded three modes (Sprint, Sprintf, JSON) via a format-or-sentinel string, which go vet flagged with 14 "call has arguments but no formatting directives" warnings. Split into log(level, message, jsonBody) and logJSON(level, j). The new logJSON wrapper adds one stack frame, so log bumps runtime.Caller's skip when jsonBody is true to keep the reported file:line on the user's call site. TestCallerFile guards against future skip drift. toolchain: bump go directive to 1.23.0 and CI matrix to 1.23-1.26 to match labstack/echo. Refresh direct deps: testify 1.8.4 -> 1.11.1, go-colorable 0.1.13 -> 0.1.14, go-isatty 0.0.20 -> 0.0.21. Pin x/sys at v0.29.0 (latest that doesn't require Go 1.25+). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: refresh README with email/random packages and Go 1.23 floor Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(bytes): use 7EiB instead of 8EiB in tests — 2^63 overflow is non-portable Parse("8EiB") computes 2^63, which exceeds MaxInt64. The float-to-int conversion of out-of-range values is implementation-dependent per the Go spec, so the prior assertion `math.MaxInt64 == b-1` (relying on two's-complement wrap) only held on some toolchains. 7EiB fits within int64 and tests EiB parsing without depending on undefined behaviour. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(email): propagate EHLO errors; always clone TLSConfig; honest comments - Drive c.Hello explicitly before checking the STARTTLS extension. (*smtp.Client).Extension triggers a lazy hello() and swallows its error, which would silently treat a failed EHLO as "STARTTLS not advertised" and keep the connection in the clear even when the caller configured TLS. - Always Clone the caller's TLSConfig in dial() instead of partial- cloning only when ServerName is empty. Removes the "sometimes mutated" branch and the implicit "do not mutate TLSConfig concurrently with Send" contract ambiguity. - Reword the dial comment so STARTTLS-opportunistic-on-non-465 reads as the honest contract it is ("stays cleartext if server doesn't advertise"), not a guarantee of TLS. - Expand doc comments on TLSConfig and DialTimeout to state the clone behavior and timeout scope. - log: note that skip++ for jsonBody must stay in sync with logJSON. - Tests: bump TestDial_Timeout from 150ms -> 500ms to absorb CI scheduler jitter (upper bound bumped to 3s — still well-scoped). Document intent in TestDial_PlainAccepts by asserting STARTTLS is not advertised on the fake server. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 2888b9c commit 2659cda

10 files changed

Lines changed: 331 additions & 137 deletions

File tree

.github/workflows/gommon.yml

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,16 +23,16 @@ on:
2323

2424
env:
2525
# run static analysis only with the latest Go version
26-
LATEST_GO_VERSION: "1.21"
26+
LATEST_GO_VERSION: "1.26"
2727

2828
jobs:
2929
test:
3030
strategy:
3131
matrix:
3232
os: [ubuntu-latest, macos-latest, windows-latest]
3333
# Each major Go release is supported until there are two newer major releases. https://golang.org/doc/devel/release.html#policy
34-
# Echo tests with last four major releases
35-
go: ["1.18","1.19","1.20","1.21"]
34+
# Matches the go directive floor in go.mod.
35+
go: ["1.23","1.24","1.25","1.26"]
3636
name: ${{ matrix.os }} @ Go ${{ matrix.go }}
3737
runs-on: ${{ matrix.os }}
3838
steps:

README.md

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,17 @@
11
# Gommon [![GoDoc](http://img.shields.io/badge/go-documentation-blue.svg?style=flat-square)](http://godoc.org/github.com/labstack/gommon) [![Coverage Status](http://img.shields.io/coveralls/labstack/gommon.svg?style=flat-square)](https://coveralls.io/r/labstack/gommon)
22

3-
Common packages for Go
4-
- [Bytes](https://github.com/labstack/gommon/tree/master/bytes) - Format/parse bytes.
5-
- [Color](https://github.com/labstack/gommon/tree/master/color) - Style terminal text.
6-
- [Log](https://github.com/labstack/gommon/tree/master/log) - Simple logging.
3+
Common packages for Go.
4+
5+
Requires Go 1.23 or later.
6+
7+
- [Bytes](https://github.com/labstack/gommon/tree/master/bytes) — format/parse byte sizes (binary IEC and decimal SI).
8+
- [Color](https://github.com/labstack/gommon/tree/master/color) — style terminal text.
9+
- [Email](https://github.com/labstack/gommon/tree/master/email) — send email over SMTP; supports STARTTLS and implicit TLS (SMTPS, port 465).
10+
- [Log](https://github.com/labstack/gommon/tree/master/log) — simple leveled logger with text and JSON output.
11+
- [Random](https://github.com/labstack/gommon/tree/master/random) — cryptographically secure random strings over configurable charsets.
12+
13+
## Install
14+
15+
```sh
16+
go get github.com/labstack/gommon
17+
```

bytes/bytes_test.go

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -246,24 +246,26 @@ func TestBytesParse(t *testing.T) {
246246
assert.Equal(t, int64(10133099161583616), b)
247247
}
248248

249-
// EiB
250-
b, err = Parse("8EiB")
249+
// EiB — 7EiB stays within int64; 8EiB == 2^63 overflows and the
250+
// float-to-int conversion of out-of-range values is
251+
// implementation-dependent per the Go spec.
252+
b, err = Parse("7EiB")
251253
if assert.NoError(t, err) {
252-
assert.True(t, math.MaxInt64 == b-1)
254+
assert.Equal(t, int64(8070450532247928832), b)
253255
}
254-
b, err = Parse("8Ei")
256+
b, err = Parse("7Ei")
255257
if assert.NoError(t, err) {
256-
assert.True(t, math.MaxInt64 == b-1)
258+
assert.Equal(t, int64(8070450532247928832), b)
257259
}
258260

259261
// EiB with spaces
260-
b, err = Parse("8 EiB")
262+
b, err = Parse("7 EiB")
261263
if assert.NoError(t, err) {
262-
assert.True(t, math.MaxInt64 == b-1)
264+
assert.Equal(t, int64(8070450532247928832), b)
263265
}
264-
b, err = Parse("8 Ei")
266+
b, err = Parse("7 Ei")
265267
if assert.NoError(t, err) {
266-
assert.True(t, math.MaxInt64 == b-1)
268+
assert.Equal(t, int64(8070450532247928832), b)
267269
}
268270

269271
// KB

email/email.go

Lines changed: 76 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,19 @@ import (
1414

1515
type (
1616
Email struct {
17-
Auth smtp.Auth
18-
Header map[string]string
19-
Template *template.Template
17+
Auth smtp.Auth
18+
Header map[string]string
19+
Template *template.Template
20+
// TLSConfig, when non-nil, is used for both implicit TLS (SMTPS)
21+
// and STARTTLS. Callers that need a custom root pool or a
22+
// specific ServerName should set this. The config is cloned per
23+
// dial, so callers can reuse a single value across sends; they
24+
// must not mutate it concurrently with an in-flight Send.
25+
TLSConfig *tls.Config
26+
// DialTimeout caps the TCP (and, for SMTPS, TLS) connect phase.
27+
// It does not bound the full SMTP conversation after the client
28+
// is returned. Zero means no caller-imposed timeout.
29+
DialTimeout time.Duration
2030
smtpAddress string
2131
}
2232

@@ -124,22 +134,17 @@ func (e *Email) Send(m *Message) (err error) {
124134
m.buffer.WriteString(m.boundary)
125135
m.buffer.WriteString("--")
126136

127-
// Dial
128-
c, err := smtp.Dial(e.smtpAddress)
137+
// Dial. Port 465 is SMTPS (implicit TLS) per IANA and always uses
138+
// TLS. Other ports connect plaintext and opportunistically upgrade
139+
// to STARTTLS only if the server advertises it — if the server
140+
// doesn't, the connection stays in the clear. Operators that
141+
// require TLS must use port 465.
142+
c, err := e.dial()
129143
if err != nil {
130144
return
131145
}
132146
defer c.Quit()
133147

134-
// Check if TLS is required
135-
if ok, _ := c.Extension("STARTTLS"); ok {
136-
host, _, _ := net.SplitHostPort(e.smtpAddress)
137-
config := &tls.Config{ServerName: host}
138-
if err = c.StartTLS(config); err != nil {
139-
return err
140-
}
141-
}
142-
143148
// Authenticate
144149
if e.Auth != nil {
145150
if err = c.Auth(e.Auth); err != nil {
@@ -172,3 +177,60 @@ func (e *Email) Send(m *Message) (err error) {
172177
_, err = m.buffer.WriteTo(wc)
173178
return
174179
}
180+
181+
func (e *Email) dial() (*smtp.Client, error) {
182+
host, port, err := net.SplitHostPort(e.smtpAddress)
183+
if err != nil {
184+
return nil, err
185+
}
186+
187+
// Always clone so we never mutate the caller's TLSConfig.
188+
var tlsConfig *tls.Config
189+
if e.TLSConfig == nil {
190+
tlsConfig = &tls.Config{ServerName: host}
191+
} else {
192+
tlsConfig = e.TLSConfig.Clone()
193+
if tlsConfig.ServerName == "" {
194+
tlsConfig.ServerName = host
195+
}
196+
}
197+
198+
dialer := &net.Dialer{Timeout: e.DialTimeout}
199+
200+
if port == "465" {
201+
conn, err := tls.DialWithDialer(dialer, "tcp", e.smtpAddress, tlsConfig)
202+
if err != nil {
203+
return nil, err
204+
}
205+
c, err := smtp.NewClient(conn, host)
206+
if err != nil {
207+
conn.Close()
208+
return nil, err
209+
}
210+
return c, nil
211+
}
212+
213+
conn, err := dialer.Dial("tcp", e.smtpAddress)
214+
if err != nil {
215+
return nil, err
216+
}
217+
c, err := smtp.NewClient(conn, host)
218+
if err != nil {
219+
conn.Close()
220+
return nil, err
221+
}
222+
// Drive EHLO explicitly so we can surface its error. (*Client).Extension
223+
// triggers a lazy hello() and swallows its error, which would silently
224+
// treat a failed EHLO as "STARTTLS not advertised" and stay cleartext.
225+
if err := c.Hello("localhost"); err != nil {
226+
c.Close()
227+
return nil, err
228+
}
229+
if ok, _ := c.Extension("STARTTLS"); ok {
230+
if err := c.StartTLS(tlsConfig); err != nil {
231+
c.Close()
232+
return nil, err
233+
}
234+
}
235+
return c, nil
236+
}

email/email_test.go

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,93 @@
11
package email
2+
3+
import (
4+
"bufio"
5+
"net"
6+
"strings"
7+
"testing"
8+
"time"
9+
10+
"github.com/stretchr/testify/assert"
11+
)
12+
13+
// TestDial_PlainAccepts verifies non-465 ports dial plaintext and
14+
// complete the SMTP handshake against a minimal fake server.
15+
func TestDial_PlainAccepts(t *testing.T) {
16+
ln, err := net.Listen("tcp", "127.0.0.1:0")
17+
assert.NoError(t, err)
18+
defer ln.Close()
19+
20+
done := runFakeSMTP(ln)
21+
22+
e := New(ln.Addr().String())
23+
e.DialTimeout = 2 * time.Second
24+
25+
c, err := e.dial()
26+
assert.NoError(t, err)
27+
if c != nil {
28+
// Our fake server never advertises STARTTLS, so dial must have
29+
// stayed plaintext — document that intent.
30+
ok, _ := c.Extension("STARTTLS")
31+
assert.False(t, ok, "fake server should not advertise STARTTLS")
32+
_ = c.Quit()
33+
}
34+
<-done
35+
}
36+
37+
// TestDial_Timeout verifies DialTimeout caps dial wait on unreachable
38+
// hosts. Uses 203.0.113.1 (TEST-NET-3, RFC 5737) which is unroutable.
39+
// Timeout is 500ms to absorb CI scheduler jitter; the upper bound is
40+
// still well under a round-trip expectation.
41+
func TestDial_Timeout(t *testing.T) {
42+
e := New("203.0.113.1:465")
43+
e.DialTimeout = 500 * time.Millisecond
44+
45+
start := time.Now()
46+
_, err := e.dial()
47+
elapsed := time.Since(start)
48+
49+
assert.Error(t, err)
50+
assert.Less(t, elapsed, 3*time.Second)
51+
}
52+
53+
// TestDial_InvalidAddress verifies malformed addresses fail fast.
54+
func TestDial_InvalidAddress(t *testing.T) {
55+
e := New("not-a-valid-address")
56+
_, err := e.dial()
57+
assert.Error(t, err)
58+
}
59+
60+
// runFakeSMTP accepts one connection and drives a minimal SMTP dialog.
61+
// Returns a channel that closes when the connection is done.
62+
func runFakeSMTP(ln net.Listener) <-chan struct{} {
63+
done := make(chan struct{})
64+
go func() {
65+
defer close(done)
66+
conn, err := ln.Accept()
67+
if err != nil {
68+
return
69+
}
70+
defer conn.Close()
71+
conn.SetDeadline(time.Now().Add(5 * time.Second))
72+
73+
_, _ = conn.Write([]byte("220 fake.local ESMTP ready\r\n"))
74+
r := bufio.NewReader(conn)
75+
for {
76+
line, err := r.ReadString('\n')
77+
if err != nil {
78+
return
79+
}
80+
upper := strings.ToUpper(strings.TrimSpace(line))
81+
switch {
82+
case strings.HasPrefix(upper, "EHLO"), strings.HasPrefix(upper, "HELO"):
83+
_, _ = conn.Write([]byte("250-fake.local\r\n250 OK\r\n"))
84+
case strings.HasPrefix(upper, "QUIT"):
85+
_, _ = conn.Write([]byte("221 bye\r\n"))
86+
return
87+
default:
88+
_, _ = conn.Write([]byte("502 not implemented\r\n"))
89+
}
90+
}
91+
}()
92+
return done
93+
}

go.mod

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,18 @@
11
module github.com/labstack/gommon
22

3-
go 1.18
3+
go 1.23.0
44

55
require (
6-
github.com/mattn/go-colorable v0.1.13
7-
github.com/mattn/go-isatty v0.0.20
8-
github.com/stretchr/testify v1.8.4
6+
github.com/mattn/go-colorable v0.1.14
7+
github.com/mattn/go-isatty v0.0.21
8+
github.com/stretchr/testify v1.11.1
99
github.com/valyala/fasttemplate v1.2.2
1010
)
1111

1212
require (
1313
github.com/davecgh/go-spew v1.1.1 // indirect
1414
github.com/pmezard/go-difflib v1.0.0 // indirect
1515
github.com/valyala/bytebufferpool v1.0.0 // indirect
16-
golang.org/x/sys v0.15.0 // indirect
16+
golang.org/x/sys v0.29.0 // indirect
1717
gopkg.in/yaml.v3 v3.0.1 // indirect
1818
)

go.sum

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,19 @@
11
github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
22
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
3-
github.com/mattn/go-colorable v0.1.13 h1:fFA4WZxdEF4tXPZVKMLwD8oUnCTTo08duU7wxecdEvA=
4-
github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovkB8vQcUbaXHg=
5-
github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM=
6-
github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY=
7-
github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y=
3+
github.com/mattn/go-colorable v0.1.14 h1:9A9LHSqF/7dyVVX6g0U9cwm9pG3kP9gSzcuIPHPsaIE=
4+
github.com/mattn/go-colorable v0.1.14/go.mod h1:6LmQG8QLFO4G5z1gPvYEzlUgJ2wF+stgPZH1UqBm1s8=
5+
github.com/mattn/go-isatty v0.0.21 h1:xYae+lCNBP7QuW4PUnNG61ffM4hVIfm+zUzDuSzYLGs=
6+
github.com/mattn/go-isatty v0.0.21/go.mod h1:ZXfXG4SQHsB/w3ZeOYbR0PrPwLy+n6xiMrJlRFqopa4=
87
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
98
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
10-
github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
11-
github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
9+
github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U=
10+
github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U=
1211
github.com/valyala/bytebufferpool v1.0.0 h1:GqA5TC/0021Y/b9FG4Oi9Mr3q7XYx6KllzawFIhcdPw=
1312
github.com/valyala/bytebufferpool v1.0.0/go.mod h1:6bBcMArwyJ5K/AmCkWv1jt77kVWyCJ6HpOuEn7z0Csc=
1413
github.com/valyala/fasttemplate v1.2.2 h1:lxLXG0uE3Qnshl9QyaK6XJxMXlQZELvChBOCmQD0Loo=
1514
github.com/valyala/fasttemplate v1.2.2/go.mod h1:KHLXt3tVN2HBp8eijSv/kGJopbvo7S+qRAEEKiv+SiQ=
16-
golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
17-
golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
18-
golang.org/x/sys v0.15.0 h1:h48lPFYpsTvQJZF4EKyI4aLHaev3CxivZmv7yZig9pc=
19-
golang.org/x/sys v0.15.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
15+
golang.org/x/sys v0.29.0 h1:TPYlXGxvx1MGTn2GiZDhnjPA9wZzZeGKHHmKhHYvgaU=
16+
golang.org/x/sys v0.29.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
2017
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
2118
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
2219
gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=

0 commit comments

Comments
 (0)