Skip to content

fix: race condition in Copy(), invalid AsciiJSON Unicode, Proxy-Authorization leak#4627

Open
barry3406 wants to merge 5 commits intogin-gonic:masterfrom
barry3406:fix/context-copy-race-and-asciijson-unicode
Open

fix: race condition in Copy(), invalid AsciiJSON Unicode, Proxy-Authorization leak#4627
barry3406 wants to merge 5 commits intogin-gonic:masterfrom
barry3406:fix/context-copy-race-and-asciijson-unicode

Conversation

@barry3406
Copy link
Copy Markdown

Summary

Three independent bug fixes found during code audit.

1. Race condition in Context.Copy() (context.go)

Copy() reads c.Keys before acquiring the read lock, then clones the stale reference under the lock:

cKeys := c.Keys      // reads WITHOUT lock
c.mu.RLock()
cp.Keys = maps.Clone(cKeys)  // clones potentially stale reference
c.mu.RUnlock()

If Set() is called concurrently between the read and the lock, cKeys may reference a map being modified. This is especially dangerous on the first Set() call where c.Keys transitions from nil to a new map.

Fix: Move the c.Keys read inside the RLock.

2. AsciiJSON produces invalid JSON for supplementary Unicode (render/json.go)

Characters above U+FFFF (emoji, math symbols) are escaped as \u1f600 (5+ hex digits), which is invalid JSON per RFC 8259. Strict JSON parsers reject this output.

Per the spec, supplementary plane characters must be encoded as UTF-16 surrogate pairs (e.g., U+1F600 → \uD83D\uDE00).

Fix: Detect runes > 0xFFFF and encode as surrogate pairs.

3. Proxy-Authorization header leaked in recovery panic logs (recovery.go)

secureRequestDump only masks Authorization but not Proxy-Authorization, which also carries credentials (used by gin's own BasicAuthForProxy middleware). When a panic occurs behind proxy auth, credentials are logged in plaintext.

Fix: Also sanitize Proxy-Authorization in the same check.

Copy() reads c.Keys before acquiring the read lock, then clones the
stale reference under the lock. If another goroutine calls Set()
between the read and the lock acquisition, the cKeys variable may
reference a map being concurrently modified.

This is particularly dangerous on the first Set() call where c.Keys
transitions from nil to a new map allocation.
Characters above U+FFFF (emoji, math symbols, etc.) were escaped as
\u1f600 (5+ hex digits) which is invalid JSON per RFC 8259. Strict
JSON parsers reject this output.

Per the spec, supplementary plane characters must be encoded as
UTF-16 surrogate pairs (e.g. U+1F600 becomes \uD83D\uDE00).
secureRequestDump only masks the Authorization header but not
Proxy-Authorization, which also carries credentials (used by gin's
own BasicAuthForProxy middleware). When a panic occurs behind proxy
auth, credentials are logged in plaintext.
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 9, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.37%. Comparing base (3dc1cd6) to head (473f9dd).
⚠️ Report is 275 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4627      +/-   ##
==========================================
- Coverage   99.21%   98.37%   -0.84%     
==========================================
  Files          42       48       +6     
  Lines        3182     3143      -39     
==========================================
- Hits         3157     3092      -65     
- Misses         17       42      +25     
- Partials        8        9       +1     
Flag Coverage Δ
?
--ldflags="-checklinkname=0" -tags sonic 98.36% <100.00%> (?)
-tags go_json 98.30% <100.00%> (?)
-tags nomsgpack 98.35% <100.00%> (?)
go-1.18 ?
go-1.19 ?
go-1.20 ?
go-1.21 ?
go-1.25 98.37% <100.00%> (?)
go-1.26 98.37% <100.00%> (?)
macos-latest 98.37% <100.00%> (-0.84%) ⬇️
ubuntu-latest 98.37% <100.00%> (-0.84%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Add TestRenderAsciiJSONSupplementaryUnicode to verify emoji (U+1F600)
  is encoded as UTF-16 surrogate pair (\uD83D\uDE00)
- Add Proxy-Authorization test case to TestSecureRequestDump to verify
  credentials are sanitized in recovery panic logs
Copy link
Copy Markdown
Contributor

@Nurysso Nurysso left a comment

Choose a reason for hiding this comment

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

LGTM for the most parts, but a few minor suggestions

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think a adding a nil check would be better

// - cKeys := c.Keys
    c.mu.RLock()
    if c.Keys != nil {
        cp.Keys = maps.Clone(c.Keys)
    }
    c.mu.RUnlock()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Few things I noticed

  • Case sensitivity issue: HTTP headers are case-insensitive, but the code only matches exact case. A header like authorization: or AUTHORIZATION: would leak. Use strings.EqualFold or convert to lowercase before comparison.
  • Compared to the original implementation, this adds extra complexity (SplitN) without a clear benefit. Since we already know the header name from the prefix check, a simple hardcoded replacement (as before) seems easier to read and maintain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants