# Real bugs, and what MergeHarbor said about them Each card is a bug that shipped in a well-known project. We put it back, MergeHarbor reviewed the change, and this is its comment, unedited apart from length. Compare it with the project's own fix. From the [benchmark run](https://mergeharbor.com/benchmarks) with the Standard tier and default settings. Each badge is the severity MergeHarbor gave its comment, which can differ from the benchmark's own label. prometheus/prometheus Go critical promql/parser/ast.go:499 ## Nil pointer dereference: `e.RHS.PositionRange()` inside `if e.RHS == nil` The branch at lines 496-501 is entered only when `e.RHS == nil`. The PR changed `e.LHS.PositionRange().End` to `e.RHS.PositionRange().End`, so this now calls a method on a nil interface and panics. The branch is for a duration expression with only an LHS, where the end position must come from LHS. The next branch (`e.LHS == nil`) correctly uses RHS, so this change looks like a copy-paste error. `DurationExpr.PositionRange` is reached through `mergeRanges`, `MatrixSelector`, `SubqueryExpr` and similar callers. Any… The bug: Nil pointer dereference: inside the \`e.RHS == nil\` branch the code calls e.RHS.PositionRange(); it should use e.LHS [Project's fix](https://github.com/prometheus/prometheus/commit/e4e65d1247bb6f92fb1fd1ded5d0ed3486136ed3) gin-gonic/gin Go major context.go:135 ## c.Keys read moved outside RLock in Copy(), creating a data race `cKeys := c.Keys` now runs before `c.mu.RLock()`. `Context.Set` lazily allocates the map (`c.Keys = make(map[string]any)`) while holding `c.mu.Lock()`. If another goroutine calls `Set` while `Copy()` runs, the unlocked read of the `c.Keys` field races with that write. `go test -race` will flag it. There is also a correctness effect. If `Copy` reads a nil `c.Keys` before `Set` allocates the map, `maps.Clone(nil)` returns nil, so keys that `Set` stored before `Copy` took the lock are silently missing from the copy… The bug: Data race: c.Keys is read before c.mu.RLock(), while Set writes the map under the lock; the copy can miss keys or race [Project's fix](https://github.com/gin-gonic/gin/commit/1bd0ecf2d3e08da37efa270bb7be20c1eddb0d68) go-chi/chi Go major middleware/route\_headers.go:81 ## Removed `return` after empty-routes shortcut causes the next handler to run twice When `len(hr) == 0`, `next.ServeHTTP(w, r)` is called. Without the `return`, execution continues. The `for` loop over the empty map does nothing. The code then looks up `hr["*"]`, finds nothing, and calls `next.ServeHTTP(w, r)` a second time (lines 100-103). The downstream handler therefore runs twice for every request when no routes are configured. This causes duplicate side effects, such as double writes to the body, a "superfluous WriteHeader" warning, and duplicated DB writes. Restore the `return`. The bug: Missing return after next.ServeHTTP in the empty-router branch: execution falls through and the next handler is invoked a second time [Project's fix](https://github.com/go-chi/chi/commit/05f1ef7bb50b8a8cb33a9dd3ba1c5b94bff0f723) caddyserver/caddy Go major modules/caddypki/ca.go:155 ## Removed error check after Root.Load() causes panic on load failure The `if err != nil { return err }` after `ca.Root.Load()` was removed, so `rootCertChain[0]` is now evaluated before any error check. If Load() fails (missing/invalid file, bad PEM, etc.), rootCertChain is nil/empty and indexing `[0]` panics with an index-out-of-range instead of returning a clean config error. The `if err != nil` at line 160 comes after the panic site, so it does not help. A misconfigured root certificate would crash provisioning rather than yielding a normal error. The bug: Error from ca.Root.Load() is not checked before rootCertChain\[0\]: a failed load panics with index out of range instead of returning the error [Project's fix](https://github.com/caddyserver/caddy/commit/9738f49b952a337cdaf42175a0e92b01974c0e3c) psf/requests Python major src/requests/utils.py:99 ## Removing empty-string filter makes Windows proxy bypass match every host The removed lines filtered empty entries out of the `ProxyOverride` split. Without the filter, a registry value with a trailing or doubled semicolon (e.g. `"localhost;192.168.*;"`, which is common on Windows) produces an empty string in the list. For that entry, `re.match("", host, re.I)` always matches (line 108), so `proxy_bypass_registry` returns True for every host. Every request then bypasses the configured proxy. This is a regression of the upstream fix with the comment "filter out empty strings to avoid… The bug: Empty entries from a trailing or doubled ';' in ProxyOverride become an empty regex that matches every host, so the proxy is bypassed for all hosts; filter out empty strings [Project's fix](https://github.com/psf/requests/commit/1604e20fc87ce212cf3a02474a6d7509b640cbbb) pallets/flask Python critical src/flask/sessions.py:321 ## Reintroduces wrong key order: sessions are signed with an old fallback key (GHSA-4grg-w6v8-c28g) The change puts `app.secret_key` first in the key list and then extends it with `SECRET_KEY_FALLBACKS`. itsdangerous treats the list as ordered oldest to newest. It signs with the last key (`secret_keys[-1]`) and tries keys in reverse order when verifying. The removed line `keys.append(app.secret_key) # itsdangerous expects current key at top` placed the current key last. With this change and a non-empty `SECRET_KEY_FALLBACKS`: - New sessions are signed with the last fallback key, not the current `SECRET_KEY`. A… The bug: Key order reversed: itsdangerous signs with the last key in the list, so placing secret\_key first and fallbacks after makes new sessions signed with a retired fallback key [Project's fix](https://github.com/pallets/flask/commit/fb54159861708558b5f5658ebdc14709d984361c) pallets/werkzeug Python critical src/werkzeug/routing/map.py:963 ## Relative URLs no longer built when subdomain and host matching are both disabled The rewrite adds `and (self.map.subdomain_matching or self.map.host_matching)` as a required condition. The original code returned a plain path when neither matching mode was enabled (`not (subdomain_matching or host_matching)`), as the comment on lines 961-962 still says. Now, with `Map(subdomain_matching=False)` and `host_matching=False`, the condition is always false. `build()` then falls through to line 973-974 and returns an absolute `//host/path` URL for every build. `subdomain_matching=False` is an… The bug: Wrong boolean logic: requiring subdomain\_matching or host\_matching to be enabled means a map with neither enabled never returns a plain path and always builds an external URL [Project's fix](https://github.com/pallets/werkzeug/commit/884437aef93e543ebaff3b9cdbe857ee47e7e00c) encode/httpx Python major httpx/\_urlparse.py:90 ## Unescaped dots in IPv4\_STYLE\_HOSTNAME regex match any character, rejecting valid hostnames The change turns `\.` into `.`, so the regex matches any character between digit groups. `encode_host` (line 290) sends every match to `ipaddress.IPv4Address(host)`, and on failure it raises `InvalidURL("Invalid IPv4 address")`. Hostnames such as `1a2b3c4d5` are not affected, because the pattern needs four digit groups separated by single characters. Hosts like `1-2-3-4`, `10x20x30x40` or `123_456_789_012` now match and are rejected as invalid IPv4 addresses. These are legal registered-name hosts, for example… The bug: Unescaped '.' in the IPv4 regex matches any character, so hosts like '1x2x3x4' are treated as IPv4 literals; escape the dots [Project's fix](https://github.com/encode/httpx/commit/e63b6594f2863b7c8274eb0991ebc6cad63661f7) fastapi/fastapi Python major fastapi/encoders.py:305 ## exclude\_defaults no longer propagated when encoding dict keys/values The dict branch of `jsonable_encoder` (lines 298-313) no longer passes `exclude_defaults` to the recursive calls for keys and values. The sibling recursion paths still pass it: the list/set/tuple branch (line 326), the dataclass branch (line 268) and the BaseModel branch (line 256). The default is `False`, so the flag is silently reset to `False` for everything nested in a dict. Concrete effects: - `jsonable_encoder({"a": SomeModel(...)}, exclude_defaults=True)` still emits the default-valued fields of… The bug: Recursive jsonable\_encoder calls for dict keys and values pass exclude\_unset and exclude\_none but drop exclude\_defaults, so nested models inside dicts ignore it [Project's fix](https://github.com/fastapi/fastapi/commit/aadfcce76380ab169fe172d5cda21722e53c4924) honojs/hono TypeScript major src/middleware/serve-static/index.ts:58 ## Removed GET/HEAD method guard: static files now served for every HTTP method The diff deletes the check `if (c.req.method !== 'GET' && c.req.method !== 'HEAD') return next()`. A grep of src/middleware/serve-static/ finds no other method handling (no `HEAD`/`method` matches). The middleware therefore calls `getContent` and returns `c.body(content)` with a 200 for POST, PUT, DELETE, PATCH and OPTIONS requests whenever a matching file exists. Routes like `app.post('/foo')` registered after `app.use('*', serveStatic())` are shadowed whenever a file with the same path exists. Because the… The bug: serveStatic no longer checks the request method: POST/PUT/DELETE requests are answered with static files instead of falling through to next(); only GET and HEAD should be served [Project's fix](https://github.com/honojs/hono/commit/588ec13fa68ea44fed1b5e1b503341db2bf09d70) vitejs/vite TypeScript major packages/vite/src/node/utils.ts:1449 ## hmrProxy now aliases and mutates the caller's hmr object instead of a copy `const hmrProxy = hmrConfig || {}` no longer copies `hmrConfig`. The loop below then runs `Object.defineProperty` on `hmrProxy`, which replaces the data properties (`protocol`, `host`, `port`, `clientPort`, `path`, `timeout`, `server`) on the user's own `server.hmr` object with getters. Those getters read from `serverConfig.ws`. The previous `{ ...hmrConfig }` avoided this. Consequences: - If the same `hmr` object is shared by several configs or servers (a common config constant, or a config resolved twice), the… The bug: hmrProxy aliases the user's hmr config object, so Object.defineProperty installs accessors on the shared/default config; copy it first ({ ...hmrConfig }) [Project's fix](https://github.com/vitejs/vite/commit/574d5e351905fe99891364d03c2440531378f7fd) axios/axios JavaScript major lib/helpers/Http2Sessions.js:108 ## Removing the `goaway` listener lets draining sessions be reused from the pool The PR deletes `session.once('goaway', removeSession)`. Only `close` and `error` now evict a session. `getSession` (lines ~29-36) reuses any entry where `!destroyed && !closed`. Node emits `goaway` before it closes or destroys the session. Any `getSession` call made in that window, including one from inside a `goaway` handler, can get the draining session back. Requests on it then fail or are refused. The deleted changelog entry (#11210) described exactly this fix, so this change reintroduces the bug it fixed… The bug: No 'goaway' listener: a session that received GOAWAY stays in the pool and is reused, so new requests fail; also the idle timer can re-run removal for an already removed session [Project's fix](https://github.com/axios/axios/commit/43968f294eaf79dabf0fcc2c05fd9add6a4faf12) expressjs/express JavaScript major lib/response.js:165 ## Reverts fix: res.send() again sets Content-Length when Transfer-Encoding is present The PR removes the `!this.get('Transfer-Encoding')` guard around Content-Length population in `res.send()`, and also deletes the matching History.md entry for #4893. With this change, a response where the app or middleware has set `Transfer-Encoding` (e.g. `chunked`) and then calls `res.send(body)` gets `Content-Length` set at line 181 as well. Sending both headers violates RFC 7230 §3.3.2 and can cause response-smuggling or parsing problems in proxies and clients. The 204/304/205 branches only strip… The bug: Content-Length is populated even when a Transfer-Encoding header is set; sending both is invalid HTTP (RFC 9112) and enables request/response smuggling ambiguities [Project's fix](https://github.com/expressjs/express/commit/18e5985b8a9d5e8423db0a9121f22bdaecd5b120) google/guava Java major guava/src/com/google/common/net/InetAddresses.java:384 ## Int overflow in tryParseDecimal for 2147483648 and 2147483649 returns a negative value instead of -1 The new guard `decimal > Integer.MAX_VALUE / 10` runs before `decimal *= 10`, but it does not stop overflow on the last digit. The check passes when decimal is exactly 214748364. The loop then computes 2147483640 and adds the digit. A final digit of 8 or 9 gives 2147483648 or 2147483649, which exceed Integer.MAX\_VALUE and wrap to a negative int. The old code used a `long` and a final range check, so it returned -1 for these inputs. The only caller, `bytesToInetAddress` (line 434), treats only `-1` as "not… The bug: Overflow check happens before the multiply-and-add, so values just above Integer.MAX\_VALUE (e.g. 2147483648) wrap to a negative int instead of returning -1 [Project's fix](https://github.com/google/guava/commit/6b3a8d7498a7fde122967ba37a023ba3dcb47792) apache/commons-lang Java major src/main/java/org/apache/commons/lang3/math/Fraction.java:331 ## Reverts the fix for negating Integer.MIN\_VALUE in getReducedFraction; valid inputs now throw The removed loop reduced common powers of two before sign normalization, so it avoided negating `Integer.MIN_VALUE`. The new code only halves once, and only when `denominator == Integer.MIN_VALUE`. It does not handle a numerator of `Integer.MIN_VALUE` with any other negative denominator. Examples: - `getReducedFraction(Integer.MIN_VALUE, -2)` used to reduce to `2^30/1`. It now reaches the `denominator < 0` branch and throws `ArithmeticException("overflow: can't negate")`, because \`numerator ==… The bug: Only a single factor of two is removed and only when the denominator is MIN\_VALUE; MIN\_VALUE numerators/denominators reach sign negation and overflow (ArithmeticException or wrong value) [Project's fix](https://github.com/apache/commons-lang/commit/e992a498d711fc4302d5ad89e80511db41db2f11) apache/commons-io Java major src/main/java/org/apache/commons/io/input/BoundedReader.java:134 ## skip() no longer enforces the bound and corrupts charsRead The new `skip` does `charsRead += n; return super.skip(n);`. This has four problems. 1. \*\*The bound is not enforced.\*\* The old code capped the skip at `maxCharsFromTargetReader - charsRead` and returned 0 once the limit was reached. The new code passes `n` straight to the underlying reader. A caller can therefore skip past `maxCharsFromTargetReader` and consume characters the wrapper promises never to expose. This breaks the class contract of returning EOF at the limit. 2. \*\*`charsRead` counts requested… The bug: skip() adds the requested n (not the number actually skipped) and does not clamp to the remaining bound, so it can skip past the limit and corrupt charsRead (int overflow for large n) [Project's fix](https://github.com/apache/commons-io/commit/e806e6b5926993ec6348a78542b1e1377a765e88) keycloak/keycloak Java major scim/core/src/main/java/org/keycloak/scim/resource/spi/BaseResourceTypeProvider.java:55 ## Removed guard lets callers without view permission probe resource existence The PR deletes the check that threw `ForbiddenException` (403) when `getModel(id)` returns null and the caller lacks query/view permission. `get()` now returns null for every missing id, which the SCIM layer reports as 404. For an existing id, line 59 throws 403 when the caller lacks VIEW. A caller without permissions therefore gets 404 for an id that doesn't exist and 403 for one that does. That is an enumeration oracle for user and group ids. The removed comment said exactly this was the intent. `getAll` (line… The bug: Missing resource returns null (404) before any permission check, while existing ones return 403, letting callers without view permission probe which ids exist [Project's fix](https://github.com/keycloak/keycloak/commit/0aa156ebd74ad90e7faccb77b7ca1844bfbec812) spring-projects/spring-framework Java major spring-core/src/main/java/org/springframework/util/StringUtils.java:1304 ## NPE on null collection elements in collectionToDelimitedString Replacing `String.valueOf(element)` with `element.toString()` throws a NullPointerException when the collection contains a null element. The `Collection` parameter allows nulls, for example `Arrays.asList("a", null)` or an `ArrayList`. The append loop at line 1310 uses `StringBuilder.append(Object)`, which writes "null" for a null element, so the old code handled nulls correctly. The sizing loop is now the only place that dereferences each element. This changes the public behaviour of… The bug: Length pre-computation calls element.toString() and throws NullPointerException for null elements; use String.valueOf(element) [Project's fix](https://github.com/spring-projects/spring-framework/commit/0d6cc122744df002e35f0788eab28bbd492c46ed) tokio-rs/tokio Rust major tokio/src/fs/file.rs:682 ## Unchecked `*offset += n` reintroduces overflow in SeekFrom::Current handling The PR removes the `checked_add` guard and the fallback two-step seek (`extra_seek` followed by `seek(pos)`). The offset is user-supplied (`SeekFrom::Current(i64)`), and `n` is the negative count of unread buffered bytes (`discard_read()`). If the user passes an offset near `i64::MIN` (e.g. `SeekFrom::Current(i64::MIN)`) while there is unread buffered data, `*offset += n` overflows. In debug builds this panics inside `start_seek`. In release builds it wraps to a large positive offset, so the file seeks to a… The bug: Adding the unread-buffer adjustment to a SeekFrom::Current offset overflows i64 for extreme offsets (e.g. i64::MIN): panic in debug, wrong seek in release; use checked\_add [Project's fix](https://github.com/tokio-rs/tokio/commit/2fc5972b8bd6f51d1ad6df3bcc1a1927d1702663) hyperium/hyper Rust major src/proto/h1/decode.rs:674 ## Trailer decoding drops duplicate header values (append changed to insert) `HeaderMap::insert` replaces any existing values for the name. `append` keeps them. In `decode_trailers`, a trailer block such as `X-Trace: first\r\nX-Trace: second` now yields only `second`. HTTP allows repeated field lines, which are semantically a comma-joined list. Dropping earlier values silently loses data. Other multi-value trailers such as `Set-Cookie` or `Server-Timing` would lose data the same way. It can also mask values that downstream code inspects. The headers array is sized to `count`, and… The bug: HeaderMap::insert replaces earlier values, so repeated trailer fields keep only the last one; use append [Project's fix](https://github.com/hyperium/hyper/commit/876effe10fd8f8ad4535ade1f84e88f199f2cf6b) sinatra/sinatra Ruby minor lib/sinatra/base.rb:714 ## ETag list matching no longer trims leading/trailing whitespace of the header value The old code was `split(',').map(&:strip)`, which stripped every element, including the first and last. The new `split(/\s*,\s*/)` only removes whitespace around the commas. Whitespace at the start of the first token and at the end of the last token is kept. Examples: - `If-None-Match: ' "abc"'` no longer matches `"abc"`. - `If-None-Match: '"abc" '` no longer matches `"abc"`. - `'"a", "abc" '` no longer matches the last entry. Most servers trim header values, but `env['HTTP_IF_NONE_MATCH']` and… The bug: Splitting the client-controlled If-None-Match/If-Match header with /\\s\*,\\s\*/ backtracks polynomially on long whitespace runs (ReDoS); split on ',' and strip [Project's fix](https://github.com/sinatra/sinatra/commit/3fe8c38dc405586f7ad8f2ac748aa53e9c3615bd) rack/rack Ruby critical lib/rack/static.rb:118 ## Reverts CVE-2026-34785 fix: Rack::Static prefix matching exposes unintended files This change removes the normalization in `initialize` (lines 96-98 of the base file). That code stored each URL together with a slash-terminated variant. It also changes `route_file` back to a raw string prefix match, `path.index(url) == 0`. The old check was `path == url || path.start_with?(url_slash)`, which only matches on a path-segment boundary. With the raw prefix match, `urls: ["/css"]` also routes `/css-secret/x`, `/cssfoo` and `/css.bak`. Rack::Files then serves those from the static root, so any sibling… The bug: path.index(url) == 0 is a plain string-prefix test, so urls: \['/assets'\] also routes '/assets-private/...' to the static file server; require an exact match or a trailing '/' [Project's fix](https://github.com/rack/rack/commit/6d434bbccdc2ffc3086dbce3930c3816945a35ec) rails/rails Ruby major activesupport/lib/active\_support/number\_helper/number\_to\_currency\_converter.rb:17 ## Removing the `options[:precision].nil?` guard makes negative currency raise TypeError when precision is nil The original line short-circuited when `options[:precision]` was nil. Now `10**options[:precision]` is evaluated for every negative number. `options` is built with `defaults.merge!(opts)`, so a caller passing `precision: nil` overrides the default. A locale that has no `currency.format.precision` has the same effect. In that case `10**nil` raises `TypeError: nil can't be coerced into Integer`. For example, `number_to_currency(-1.5, precision: nil)` worked before and now raises. The old behavior was to use the… The bug: 10\*\*options\[:precision\] raises TypeError when precision is nil (allowed for positive numbers); guard the nil case [Project's fix](https://github.com/rails/rails/commit/21c60f81a3c72635ba9b0fdb863608952d4766a8) ## Try it on your own pull request Paste a public GitHub pull request and see MergeHarbor's review in a few minutes. No sign-up, nothing posted. [Review a pull request](https://mergeharbor.com/try) ## Further reading [Guide · 8 min read A code review checklist for human and AI-assisted review A practical code review checklist for correctness, security, concurrency, errors, migrations, API contracts, tests and performance, and what AI review covers. Read the guide](https://mergeharbor.com/guides/code-review-checklist) [Guide · 6 min read Diff-only versus codebase-aware review: what a diff hides Why many serious bugs sit outside the lines a pull request changes, three concrete examples, and how MergeHarbor indexes, traces and verifies to find them. Read the guide](https://mergeharbor.com/guides/diff-only-vs-codebase-aware-review) ## Review every pull request with the context of your whole codebase Connect a code host in minutes. Reviews start right away, and codebase context is added as indexing completes. [Start free trial](https://app.mergeharbor.com/signup) [Book a demo](https://mergeharbor.com/contact?topic=sales&source=demo)