Skip to content
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 39 additions & 2 deletions src/middleware/jwt.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"log"
"net/http"
"strings"
"sync"
"time"

"github.com/gin-gonic/gin"
Expand Down Expand Up @@ -221,11 +222,41 @@ func IsValidAddr(addr string) bool {
return strings.Contains(rest, "@")
}

// fmsgIDClient is a dedicated HTTP client with a bounded timeout so that a
// slow or hung fmsgid never blocks an API request goroutine indefinitely
// (which would otherwise hold the inbound HTTP connection open and exhaust
// the browser's per-host connection limit).
var fmsgIDClient = &http.Client{Timeout: 5 * time.Second}

// fmsgIDCacheTTL is how long a positive fmsgid lookup is cached. Tokens are
// re-validated every time, but the relatively expensive network round-trip to
// fmsgid is short-circuited for this window. Negative results are not cached.
const fmsgIDCacheTTL = 30 * time.Second

type fmsgIDEntry struct {
expires time.Time
code int
acceptingNew bool
}

var fmsgIDCache sync.Map // map[string]fmsgIDEntry, key = addr

Copilot AI Apr 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inline comment on fmsgIDCache says the key is addr, but the implementation keys by idURL + "|" + addr. Please update the comment to match reality (and ideally clarify the key format) to avoid misleading future maintainers.

Suggested change
var fmsgIDCache sync.Map // map[string]fmsgIDEntry, key = addr
var fmsgIDCache sync.Map // map[string]fmsgIDEntry, key = idURL + "|" + addr

Copilot uses AI. Check for mistakes.

// checkFmsgID queries the fmsgid service for a user address.
// Returns (statusCode, acceptingNew, error).
// Returns (statusCode, acceptingNew, error). Successful 200 responses are
// cached for fmsgIDCacheTTL to avoid hammering fmsgid when a browser fires
// many concurrent requests with the same JWT.
func checkFmsgID(idURL, addr string) (int, bool, error) {
cacheKey := idURL + "|" + addr
if v, ok := fmsgIDCache.Load(cacheKey); ok {
entry := v.(fmsgIDEntry)
Comment on lines 260 to +262

Copilot AI Apr 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cacheKey := idURL + "|" + addr relies on a string delimiter, which is easy to get wrong and can theoretically collide. Using a comparable struct key (e.g., {idURL, addr}) avoids delimiter issues and also makes it clearer what dimensions are being cached.

Copilot uses AI. Check for mistakes.
if time.Now().Before(entry.expires) {
return entry.code, entry.acceptingNew, nil
}
fmsgIDCache.Delete(cacheKey)
}
Comment on lines +243 to +282

Copilot AI Apr 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fmsgIDCache entries are only deleted on cache hits after expiry. If an address is looked up once and never again, the expired entry will remain in the sync.Map indefinitely, so the cache can grow without bound over the process lifetime. Consider adding periodic cleanup (e.g., a ticker that Ranges and deletes expired entries, similar to the cleanup loop in middleware/ratelimit.go) or switching to a bounded LRU/TTL cache implementation.

Copilot uses AI. Check for mistakes.
Comment on lines +256 to +282

Copilot AI Apr 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says caching avoids hammering fmsgid "when a browser fires many concurrent requests", but this implementation does not deduplicate concurrent cache misses—multiple goroutines can still call fmsgid at once before the first response is stored. If reducing burst load is a goal, consider adding per-key request coalescing (e.g., singleflight.Group) so concurrent lookups share one upstream request.

Copilot uses AI. Check for mistakes.

url := strings.TrimRight(idURL, "/") + "/fmsgid/" + addr
resp, err := http.Get(url) //nolint:gosec // URL constructed from trusted config + validated addr
resp, err := fmsgIDClient.Get(url) //nolint:gosec // URL constructed from trusted config + validated addr
if err != nil {
Comment on lines 290 to 292

Copilot AI Apr 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addr is concatenated directly into the URL path. IsValidAddr currently allows characters like '/', '?', or '#' (it only checks leading '@' and another '@'), so a crafted sub could change the requested path/query on the fmsgid service. Please build the URL using url.JoinPath (or escape the path segment) and/or tighten IsValidAddr to the exact allowed character set so the gosec suppression is justified.

Copilot uses AI. Check for mistakes.
return 0, false, err
}
Expand All @@ -244,5 +275,11 @@ func checkFmsgID(idURL, addr string) (int, bool, error) {
if err := decodeJSON(resp.Body, &result); err != nil {
return http.StatusOK, true, nil // assume accepting if parse fails
}

fmsgIDCache.Store(cacheKey, fmsgIDEntry{
expires: time.Now().Add(fmsgIDCacheTTL),
code: http.StatusOK,
acceptingNew: result.AcceptingNew,
})
return http.StatusOK, result.AcceptingNew, nil
}

Copilot AI Apr 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR introduces new caching behavior for fmsgid lookups, but there isn't a test asserting the cache is actually used (e.g., that a second request with the same addr/idURL does not trigger a second upstream HTTP call, and that non-200 responses are not cached). Since src/middleware/jwt_test.go already covers fmsgid interactions, consider adding a test that counts requests or makes the second upstream call fail to verify caching.

Copilot uses AI. Check for mistakes.
Loading