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 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/prometheusGocritical

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.LHSProject's fix

gin-gonic/ginGomajor

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 raceProject's fix

go-chi/chiGomajor

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 timeProject's fix

caddyserver/caddyGomajor

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 errorProject's fix

psf/requestsPythonmajor

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 stringsProject's fix

pallets/flaskPythoncritical

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 keyProject's fix

pallets/werkzeugPythoncritical

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 URLProject's fix

encode/httpxPythonmajor

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 dotsProject's fix

fastapi/fastapiPythonmajor

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 itProject's fix

honojs/honoTypeScriptmajor

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 servedProject's fix

vitejs/viteTypeScriptmajor

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

axios/axiosJavaScriptmajor

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 sessionProject's fix

expressjs/expressJavaScriptmajor

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 ambiguitiesProject's fix

google/guavaJavamajor

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 -1Project's fix

apache/commons-langJavamajor

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

apache/commons-ioJavamajor

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

keycloak/keycloakJavamajor

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 existProject's fix

spring-projects/spring-frameworkJavamajor

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

tokio-rs/tokioRustmajor

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_addProject's fix

hyperium/hyperRustmajor

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 appendProject's fix

sinatra/sinatraRubyminor

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 stripProject's fix

rack/rackRubycritical

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

rails/railsRubymajor

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 caseProject's fix

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

Guide · 8 min readA code review checklist for human and AI-assisted reviewA practical code review checklist for correctness, security, concurrency, errors, migrations, API contracts, tests and performance, and what AI review covers.Read the guide Guide · 6 min readDiff-only versus codebase-aware review: what a diff hidesWhy 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

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 Book a demo