Second review round found two blocking defects, both verified against a stub
proxy rather than inferred.
HttpsProxyAgent was used for every target, and it always issues CONNECT:
PROXY RECEIVED: "CONNECT example.com:80 HTTP/1.1"
Squid's stock ACL permits CONNECT only to 443, so every http:// monitor would
have broken the moment HTTP_PROXY was set — in exactly the hardened networks
this feature targets. Plain-http targets now use http-proxy-agent, which
sends an absolute-URI request. The agent cache keys on target scheme.
A refused CONNECT is replayed onto the socket, so the proxy's own status was
parsed as the target's: a proxy ACL denial reported "the site returned 403",
and a monitor whose customUpCodes included 403 or 407 was marked UP while the
target was never reached. Proxy-tunnel rejections are now detected via the
agent's proxyConnect event and reported as a network error naming the proxy.
The listener is per request and removed in a finally block, so a failure
cannot be attributed to a concurrent check.
Also from review:
- The fallback for an invalid PROXY_MONITOR_TYPES no longer fires when every
entry was a deliberate but unproxyable type ("ping,port"). Falling back
there would have proxied http and pagespeed, which the operator never
nominated. Typos still fall back; unproxyable lists stay empty and warn.
- Dropped the now-redundant strict/insecure agent split: TLS is applied per
request, and Node already separates socket pools by TLS options, so the
split only doubled tunnels to the proxy.
- Renamed to ProxyPolicy.ts to match the PascalCase peers in the directory.
- The warning no longer echoes the raw config value.
Adds 9 tests covering scheme selection, tunnel rejection, and the narrowed
fallback. Verified by reverting: the tunnel-failure test fails without the fix.
Review found that the proxy path silently dropped ignoreTlsErrors.
rejectUnauthorized was passed to the HttpsProxyAgent constructor, where it
governs the socket to the proxy rather than the tunnelled handshake with the
target. Verified directly: the value lands in connectOpts, while the target
handshake reads got's per-request https options, which were unset and so
defaulted to strict.
The consequence was worse than a no-op. A monitor with ignoreTlsErrors
against a self-signed endpoint works today; configuring a proxy would have
made it fail verification, flip to down and fire a false incident with
notifications. Stating the option per request fixes it, and the agent cache
keeps its TLS split so a relaxed agent is never shared with a strict monitor.
Also from review:
- A fully-invalid PROXY_MONITOR_TYPES disabled proxying entirely, which in a
restricted-egress deployment would take every HTTP monitor down at once —
the exact failure the parser was meant to prevent. It now falls back to the
defaults and warns loudly. "none" remains the way to disable deliberately.
- The "none" sentinel only matched when it was the sole entry, so "none,http"
half-enabled proxying. It is now honoured wherever it appears.
- "unknown" is a real MonitorType but never a deliberate choice, so it is
reported as a typo rather than as an unproxyable type.
- Dropped the unused warning field from ProxyResolution.
- Documented that lowercase proxy variables are honoured and take precedence,
which matters on base images that already set them.
Adds 10 tests, including the request-level TLS assertions the previous suite
lacked. Verified by reverting the fix: both TLS tests fail without it.
Deployments with restricted egress could not run checks at all: got does not
read HTTP_PROXY/HTTPS_PROXY (unlike curl or axios), and NODE_USE_ENV_PROXY
only affects Node's built-in fetch, which got does not use. Even a correctly
built proxy agent was discarded, because HttpProvider overwrote
options.agent on every request.
Proxy selection is resolved per check and applied where the agent is chosen.
NO_PROXY handling is delegated to proxy-from-env rather than reimplemented,
so wildcards, host lists and port-qualified entries behave exactly as
operators expect from curl.
Infrastructure monitors are excluded by default. Hardware and pagespeed
checks both delegate to HttpProvider, so a naive global proxy would also
capture Capture agents — which normally sit on the internal network, where
proxying them fails or hairpins. PROXY_MONITOR_TYPES is the escape hatch for
deployments whose agents genuinely are behind the proxy, and accepts "none"
to disable proxying without unsetting the environment variables. Values that
are not real monitor types, or that open raw sockets and cannot be proxied
(ping, port, docker, grpc, websocket, dns, game), are dropped and logged so
a typo cannot silently disable proxying.
Agents are cached by proxy URL and TLS mode. Keying on TLS matters: sharing
one agent between a strict monitor and an ignoreTlsErrors monitor would leak
the relaxed setting across monitors. Caching also preserves pooling, so 50
monitors share tunnels instead of dialling the proxy per check.
Both new dependencies were already present transitively (76K total).
Notification providers still call out directly and are unaffected; that is a
known follow-up for the same class of deployment.
When customUpCodes mark an HTTPError as up, parse the response body, apply advancedMatcher.validate, and set status to statusUp && matchResult.ok. Return parsed payload and extracted values to match the success path.
Adds a customUpCodes field to the monitor schema to allow users to specify which HTTP status codes should be treated as UP instead of DOWN. Includes core logic in HttpProvider and unit tests.
Refs #3657
gorkem-bwl
Alex Holliday
harsh-aghara