Harden v2.0.0: secure-by-default TLS, resource limits, redirect/XXE fixes#23
Conversation
…ixes Pre-release security and code review hardening across both HTTP adapters. Transport/TLS: - Verify TLS certificates by default; add explicit `insecure: true` opt-in (both adapters). Previously `hackney: [:insecure]` disabled verification. - Strip basic_auth on cross-origin redirects. - Match TLS-retry alerts structurally and decouple the retry from the redirect budget; fix the fabricated handshake mock. Resource limits: - Bound gzip decompression with streaming safeInflate and add a configurable `max_body_size` (default 100 MB) in both adapters -> :body_too_large. Resolver/contract: - Return an error tuple (not a crash) for a 3xx without a usable Location; case-insensitive Location/Content-Encoding lookups. - Error on redirect-limit exhaustion (:too_many_redirects) instead of an :ok tuple; preserve the original URL in error tuples. - Non-binary :contents returns :invalid_contents; tighten public @specs; stable :adapter_error reason for reason-less exceptions. Scrapers: - SitemapScraper parses with dtd: :none (XXE / billion-laughs / external-DTD hardening, OTP-version-independent). - PageScraper parses once, tolerates nil body, resolves canonical against <base href>, and compares hosts case-insensitively. Docs/hygiene: - Correct CVE attribution to the two advisories that actually affect hackney 1.21 (CVE-2026-47075, CVE-2026-47076) with NVD links; drop overstated cluster/CVE-free claims. - Add LICENSE; document insecure/max_body_size and SSRF posture; UA version from app spec; remove stale .travis.yml; bump CI actions and run both adapters in CI. Tests: - New adapter unit tests (mock Req.request/1, HTTPoison.get/3) covering TLS translation, body caps, and error mapping. - Migrate integration host to httpbingo.org with host-agnostic assertions. - mix test.adapters and mix test --include integration both pass.
There was a problem hiding this comment.
Pull request overview
Security-hardening pass for the upcoming v2.0.0 release, tightening transport defaults (TLS verification), redirect handling, resource limits, and XML/HTML parsing safety across both HTTP adapters (Req + HTTPoison), with expanded unit/integration coverage.
Changes:
- Secure-by-default transport: TLS verification on by default,
insecure: trueopt-out, stricter TLS retry matching, and basic-auth stripping on cross-origin redirects. - Resource safety: configurable
max_body_sizeplus bounded gzip decompression to mitigate zip bombs / memory exhaustion. - Parser & contract hardening: XXE/DTD refusal for sitemaps, case-insensitive header handling, safer redirect behavior and error tuples, and improved adapter error normalization.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| test/support/mocked_connections.exs | Updates TLS alert mock shape to match Mint/OTP. |
| test/support/integration_urls.exs | Centralizes integration hosts; switches default httpbin host to httpbingo. |
| test/sitemap_scraper_test.exs | Adds tests ensuring entity expansion / external DTD fetch do not occur. |
| test/resolver_test.exs | Adds tests for max body size, redirect contract, auth stripping, TLS retry alert shapes. |
| test/page_scraper_test.exs | Adds tests for host case-insensitivity, canonical resolution vs <base>, nil body tolerance. |
| test/integration/tls_integration_test.exs | Verifies TLS is rejected by default and allowed only with insecure: true. |
| test/integration/resolver_integration_test.exs | Makes redirect assertions host-agnostic; updates redirect-limit behavior expectation. |
| test/http/adapters/req_test.exs | New adapter option-translation and error-mapping tests for Req adapter. |
| test/http/adapters/httpoison_test.exs | New adapter option-translation tests for HTTPoison adapter. |
| test/funkspector_test.exs | Adds regression test for non-binary :contents handling. |
| test/document_test.exs | Ensures errors after redirects preserve the original requested URL. |
| ROADMAP.md | Updates adapter reference to Req as default transport. |
| README.md | Documents TLS default, insecure, max_body_size, and SSRF posture. |
| LICENSE | Adds missing MIT license file. |
| lib/funkspector/utils.ex | Clarifies URL validation is not an SSRF guard. |
| lib/funkspector/sitemap_scraper.ex | Disables DTD/entity parsing (dtd: :none) to harden against XXE/billion-laughs. |
| lib/funkspector/resolver.ex | Redirect contract changes; gzip bounded inflate; header casing normalization; TLS retry refinements; basic-auth stripping on cross-origin redirects. |
| lib/funkspector/page_scraper.ex | Single-pass HTML parse; nil-body tolerance; canonical/base resolution fixes; host comparisons case-insensitive; security warning docs. |
| lib/funkspector/http/adapters/req.ex | Implements secure-by-default TLS option layering; body-size checks; stable adapter error normalization. |
| lib/funkspector/http/adapters/httpoison.ex | Implements insecure translation, body-size checks, and updated docs. |
| lib/funkspector/http/adapter.ex | Updates adapter docs re: hackney CVE exposure wording. |
| lib/funkspector/document.ex | Preserves original URL in error tuples and expands error contract types. |
| lib/funkspector.ex | Adds unified error_reason type, new defaults (insecure, max_body_size), UA version from app spec, and invalid :contents handling. |
| CHANGELOG.md | Documents new options and breaking behavior changes for v2.0.0. |
| .travis.yml | Removes stale Travis config. |
| .github/workflows/ci.yml | Updates GitHub Actions versions and runs mix test.adapters. |
Comments suppressed due to low confidence (1)
lib/funkspector.ex:64
Funkspector.resolve/2currently returnsResolver.resolve/2verbatim, so on failures after redirects the returned URL will be the post-redirect URL. This contradicts the v2.0.0 contract noted in the changelog (“Error tuples report the original requested URL”).
@spec resolve(String.t(), map()) ::
{:ok, String.t(), Response.t()} | {:error, String.t(), error_reason()}
def resolve(url, options \\ %{}) do
options = Map.merge(default_options(), options)
Resolver.resolve(url, options)
end
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @spec resolve(String.t() | any(), map()) :: | ||
| {:ok, String.t(), Response.t()} | ||
| | {:error, String.t() | any(), Response.t() | Error.t() | :invalid_url} | ||
| | {:error, String.t() | any(), | ||
| Response.t() | Error.t() | :invalid_url | :too_many_redirects} |
There was a problem hiding this comment.
Resolved in 5f3946a. The gzip path now returns %Funkspector.Error{reason: :body_too_large} (unified with the adapter raw-body path), which is already covered by Error.t() in the spec union — so no separate atom is needed.
| if total > limit do | ||
| :too_large | ||
| else | ||
| safe_inflate(z, [], limit, [acc | output], total) | ||
| end |
There was a problem hiding this comment.
Verified empirically — this does not crash. [acc | output] with a binary output produces a valid improper iolist (e.g. [[[] | "abc"] | "def"]), and both IO.iodata_to_binary/1 and IO.iodata_length/1 handle binary tails correctly. That said, switched to the proper-list form [acc, output] in 5f3946a for clarity.
| if total > limit do | ||
| :too_large | ||
| else | ||
| {:ok, IO.iodata_to_binary([acc | output])} | ||
| end |
There was a problem hiding this comment.
Same as the {:continue, ...} branch above — the improper list is valid iodata, but both branches now use [acc, output] (5f3946a).
| defp request_headers_and_options(options) do | ||
| headers = request_headers(options) | ||
|
|
||
| options = | ||
| options | ||
| |> translate_insecure() | ||
| |> Map.delete(:user_agent) | ||
| |> Map.delete(:basic_auth) | ||
| |> Map.delete(:adapter) | ||
| |> Map.delete(:contents) | ||
| |> Map.delete(:connect_options) | ||
| |> Map.delete(:insecure) | ||
|
|
||
| {headers, options} |
There was a problem hiding this comment.
Fixed in 5f3946a — :max_body_size is now deleted before HTTPoison.get/3 (hackney silently ignored it, so no runtime error today, but it should not be forwarded). Added a regression test asserting it is not in the forwarded options.
| @spec page_scrape(String.t(), map()) :: | ||
| {:ok, Document.t()} | {:error, String.t() | any(), any()} | ||
| {:ok, Document.t()} | {:error, String.t(), error_reason()} | ||
| def page_scrape(url, options \\ %{}) do |
There was a problem hiding this comment.
Fixed in 5f3946a. These functions intentionally echo back invalid non-binary input in the {:error, url, :invalid_url} tuple (covered by tests), so the URL positions are loosened to String.t() | any(). The precise error_reason type is kept.
| @spec sitemap_scrape(String.t(), map()) :: | ||
| {:ok, Document.t()} | {:error, String.t() | any(), any()} | ||
| {:ok, Document.t()} | {:error, String.t(), error_reason()} | ||
| def sitemap_scrape(url, options \\ %{}) do |
There was a problem hiding this comment.
Fixed in 5f3946a (same change as page_scrape/2) — URL positions loosened to String.t() | any().
| @spec text_sitemap_scrape(String.t(), map()) :: | ||
| {:ok, Document.t()} | {:error, String.t() | any(), any()} | ||
| {:ok, Document.t()} | {:error, String.t(), error_reason()} | ||
| def text_sitemap_scrape(url, options \\ %{}) do |
There was a problem hiding this comment.
Fixed in 5f3946a (same change as page_scrape/2) — URL positions loosened to String.t() | any().
| `max_body_size` (default 100 MB) bounds both the raw response body and gzip | ||
| decompression, protecting against memory exhaustion and decompression bombs | ||
| when scraping untrusted URLs. Oversized responses return | ||
| `%Funkspector.Error{reason: :body_too_large}`. Set `:infinity` to disable: |
There was a problem hiding this comment.
Resolved in 5f3946a by unifying the code to the documented shape: the resolver gzip path now returns %Funkspector.Error{reason: :body_too_large} (matching the adapter raw-body path), so the docs are accurate.
| - `:max_body_size` option (default 100 MB) that bounds both the raw response body and gzip decompression; oversized responses return `%Funkspector.Error{reason: :body_too_large}`. Set `:infinity` to disable. | ||
| - Mix aliases: `mix test.httpoison` (run the suite against the HTTPoison adapter) and `mix test.adapters` (run both back-to-back). |
There was a problem hiding this comment.
Resolved in 5f3946a — the resolver now returns %Funkspector.Error{reason: :body_too_large} for the gzip-decompression cap too, matching this changelog entry.
…p forwarded option
- Unify the oversized-body error to %Funkspector.Error{reason: :body_too_large}
in the resolver gzip path (matches the adapter raw-body path and the docs),
instead of a bare atom.
- Loosen public @specs to allow non-binary URL input/echo (page_scrape etc.
return invalid non-string input unchanged in the error tuple); keep the
precise error_reason type. Drop redundant :body_too_large atom (now Error.t()).
- HTTPoison adapter: stop forwarding the funkspector-level :max_body_size key
to HTTPoison.get/3; add a regression test.
- Use a proper-list accumulator in the bounded gunzip (clearer; the prior
improper iolist was valid but flagged).
- Scope the CHANGELOG 'original URL' note to the scrape functions; resolve/2
intentionally reports the URL that failed.
|
@copilot can you take another look? |
I re-reviewed the latest commit ( |
Pre-release security & code review hardening for v2.0.0, across both HTTP adapters. All changes are covered by unit tests (Req + HTTPoison) and verified against live hosts;
mix test.adaptersandmix test --include integrationboth pass with 0 failures.Transport / TLS
hackney: [:insecure], which disabled verification on every HTTPS request. Added an explicitinsecure: trueopt-in for hosts with broken/self-signed certs.basic_authwhen a redirect crosses origin (credentials no longer leak to redirect targets).{:tls_alert, {alert, _}}shape) and decouple the retry from the redirect budget; replaced the fabricated handshake mock.Resource limits
safeInflate(real zip-bomb fix) plus a configurablemax_body_size(default 100 MB,:infinityto disable) in both adapters →%Funkspector.Error{reason: :body_too_large}.Resolver / API contract
Locationnow returns an error tuple instead of raisingFunctionClauseError;Location/Content-Encodinglookups are case-insensitive.{:error, url, :too_many_redirects}instead of{:ok, url, <last 3xx>}.:contents→:invalid_contents; tightened public@specs; stable{:adapter_error, msg}reason for reason-less exceptions.Scrapers
SitemapScraperparses withdtd: :none— OTP-version-independent hardening against XXE, billion-laughs, and external-DTD fetch on attacker-controlled sitemaps.PageScraperparses once, tolerates a nil body, resolves<link rel=canonical>against<base href>, and compares hosts case-insensitively.Docs / hygiene
LICENSE(was declared MIT but missing from the tarball); documentedinsecure/max_body_sizeand the SSRF posture; UA version now derived from the app spec; removed stale.travis.yml; bumped CI actions and made CI runmix test.adapters.Notes for review
max_body_sizedefault 100 MB is a new (generous) default;:infinitydisables.request_timeoutand raises on the unknown option.httpbingo.orgwith host-agnostic assertions.