feat!: scope session cookies, bound response decompression, and drop the FlightRadar24 alias - #118
Merged
Merged
Conversation
The session jar stored cookies in a flat name/value object and replayed all of them on every request, so a cookie set by www.flightradar24.com was also sent to cdn./api./data-live./data-cloud., and Path, Secure and expiry were ignored entirely. `Set-Cookie` was also split on every `=`, which truncated any value containing one: `_frPl` arrived as `sess.token.with` instead of `sess.token.with==padding`, and that truncated value is what getAirportDetails sends as `token` and getFlights as `enc`. Parse the attributes properly and key the jar by name/domain/path. `Max-Age=0` now deletes rather than storing an empty value, and a Map keeps a cookie named `__proto__` off Object.prototype.
The lockfile already resolved a clean undici, but consumers of a published library resolve the declared range instead, and `^6.13.0` admits 6.13.0-6.27.0 — 15 advisories including request smuggling and an unbounded decompression chain. The Python dependencies had no bounds at all, which also left them as the only three packages in the dependency graph with no resolved version, so Dependabot could not match advisories against them.
Five defects in the new jar, all reproduced before fixing:
- A malformed `Max-Age` (bare or empty) read as 0 and deleted the cookie,
because Number("") is 0. RFC 6265 says ignore a non-integer value. A
bare `Max-Age` from FR24 would have dropped the login token silently.
- `Domain=com` passed the suffix check, so one response could plant a
cookie replayed to every .com host the caller later requested — the
cross-host leak this change exists to close. Reject dotless domains.
- getCookie() returned the first jar entry by insertion order, so a
re-issued token under a different scope never superseded the old one
and authenticated calls kept sending a dead `token`/`enc`. Take the
newest instead.
- Cookies were attributed to the requested URL, but fetch follows
redirects, so `Set-Cookie` belongs to the final URL. Key off that.
- Two in-scope cookies of the same name resolved by insertion order
rather than specificity. Sort so the longer path wins.
The negative scoping assertions passed vacuously if an interceptor never
fired; they now assert it did.
The pyproject comment claimed the Brotli floor mitigated decompression
bombs. It does not: 1.2.0 only adds the bounded API, and request.py
still calls the unbounded brotli.decompress. Claim removed rather than
left overstating what the bump buys.
Review follow-ups: - `index.d.ts` still described the pre-`url` return shape, so TypeScript consumers could not see the field the jar now keys off. tsd passed because nothing exercised it. - The redirect attribution had no test at all — the subtlest change in the branch. MockAgent does not follow redirects, so this needs a real local server. Verified the test fails when the fix is reverted. - A `Secure` cookie arriving over plaintext is no longer stored; we already refused to send one, so accepting it was incoherent. - `storedAt` is initialised with the record, leaving the jar as the only thing that assigns it. Kept the hand-rolled Set-Cookie parser rather than delegating to undici's `getSetCookies`, which was the tempting simplification: it drops a negative `Max-Age` instead of deleting, accepts an empty cookie name, and widens a relative `Path` to `/`. Three regressions to remove two bugs is a bad trade, so those three cases now have tests pinning the behaviour.
The server bound only 127.0.0.1 while the redirect targeted `localhost`. Node 18 resolves that to ::1 first, so the hop was refused. Node 22 happened to pick the v4 address, which is why this passed locally and failed in CI. Bind dual-stack instead, and drop keep-alive sockets in the teardown — server.close() waits for the undici agent's connections, so the failure was followed by an afterEach timeout that obscured it. Verified on node 18.14.2, 20.20.2 and 22.12.0: 76 passing on each, and the test still fails on 18 when the fix under test is reverted.
A compressed body was trusted to expand to any size. brotli reaches ratios high enough that ~500 bytes on the wire expands to 300 MB, and ~1.7 KB to 1 GB — enough to kill the process on a small host. Python now decompresses through `brotli.Decompressor.process` with a max-output argument and `zlib.decompressobj` with `max_length`, so the cap is enforced by the decompressor instead of being checked afterwards. That distinction is the whole fix: a first attempt checked the size between input chunks, which still materialised a 1 GB body (peak RSS 1936 MB) before reporting a breach it had already suffered. With the output cap, a 1 GB and a 4 GB bomb both peak at 253 MB — cost tracks the limit, not the payload. A test pins that with tracemalloc and fails against the chunk-checking version. Node reads `response.body` as a stream with a byte budget, since undici decompresses in the transport. Both ports raise DecompressionLimitError and default to 64 MiB, far above FR24's largest payload. Note the Brotli>=1.2.0 floor from an earlier commit does not do this on its own: 1.2.0 only makes the bounded API available.
- Every action is pinned to a commit SHA. Tags are mutable, so a compromised maintainer account can repoint one and the next run executes it. `pypa/gh-action-pypi-publish@release/v1` was the worst case: a *branch*, in the job holding `id-token: write` for PyPI. - The docs job installed mkdocs unpinned while holding `contents: write`, so a malicious release would run with a token that can push to the repository. Versions now come from a pinned file. - The npm audit never ran. The job defaults into ./nodejs and the step also did `cd nodejs`, so it failed with "No such file or directory" on every run while continue-on-error reported success — which is how the vulnerable undici floor sat unnoticed. Fixed and enforced. - The Python audit now blocks on the dependencies users actually install, resolved in a clean venv, and keeps the dev toolchain informational so a vulnerable linter cannot block every PR.
Both packages, since publish.yml verifies they match.
Pages was switched to build_type=workflow, which stops it rebuilding on a push to the gh-pages branch. The job still ran `mkdocs gh-deploy`, so the next docs change would have pushed the branch, reported success and never appeared on the site — the same silent-success shape as the audit step this branch already fixed. Build the site and hand it to actions/deploy-pages instead. That drops `contents: write` to `contents: read`, so a compromised docs dependency no longer runs with a token that can push to the repository, which was the point of pinning docs/requirements.txt in the first place. Verified `mkdocs build` against the pinned requirements: exits 0 and writes to site/, which is what the upload step expects. Ignored that directory, since the build now runs locally too.
Review of the budget work found that guarding size had quietly traded away integrity. `_decompress_gzip` returned whatever it had inflated: a body cut to 3/4 of its compressed bytes came back as 76 KB of partial JSON with no error, where gzip.decompress raises EOFError. `_decompress_brotli` did the same, returning 0 bytes. Both now raise when the stream ends mid-message. That also restores a fallback this had broken. get_content() reads a raised error as "curl_cffi already decoded this" and returns the raw bytes; since _decompress_brotli(b'[]') returned b"" instead of raising, an already-decoded body became empty and json.loads blew up on it. `_decompress_gzip` also stopped at the first member, so a concatenated gzip body — legal Content-Encoding, emitted by some proxies — silently lost everything after it. It now drains unused_data. On the Node side, Response.json()/text() ran the spec UTF-8 decode, which strips a byte-order mark; reading the body as a Buffer does not, so a BOM-prefixed JSON body started failing to parse. Stripped by hand. Also moved the docs pin file out of docs_dir: mkdocs copies non-page files into the built site, so it was being published at /requirements.txt.
The budget added earlier never ran in production. curl_cffi decompresses in the transport, so by the time `.content` exists the body has already expanded: `_decompress_brotli` was handed already-decoded bytes, raised, and fell through to the "transport already decoded" path. Measured against a local server — 27 compressed bytes arrived as 200 008 — and against FR24 itself, where every real request logs that fallback at DEBUG. The tests passed only because they called the helpers directly with compressed input, which is a state production never reaches. Read the response with `stream=True` and `iter_content()` instead, and apply the budget to the chunks as they arrive. A 512 MB bomb from 841 bytes on the wire is now refused with no growth in RSS, which is what the Node port already did by streaming `response.body`. Also closes the two smaller gaps from the same review: - `max_response_bytes` is now a per-request argument, matching Node's `maxResponseBytes`; it was a module constant with no way to override. - `get_content()` handling an already-decoded body under a live `Content-Encoding` header — the path every FR24 response takes — had no test. It does now, along with one asserting the response is closed when the body is refused. Renamed to MAX_RESPONSE_BYTES for parity with the Node constant, since the budget covers the response, not just what this code decompresses.
…bomb Streaming was the wrong call, and the measurements say so plainly. In curl_cffi 0.16, `stream=True`: - Turns `timeout` from a wall-clock cap into a ">=1 byte/sec" liveness check. A tarpit trickling one byte every 400ms held a `timeout=2` request for 23.8s and would hold it indefinitely — including every worker in the get_flight_details fan-out. That is the same denial-of-service the byte budget exists to prevent. - Stops the session reusing connections: 5 requests opened 5 TCP+TLS connections instead of 1. A fresh handshake per request is precisely the fingerprint Cloudflare bot management scores against, which undermines the TLS impersonation this library is built around. - Leaves `Response.content` empty, so `get_response_object()` and `CloudflareError.response` — both public — silently return nothing. The challenge body is the first thing anyone inspects when debugging a block. Reverted to a buffered read. Confirmed restored: timeout raises at 2.0s, 5 requests share 1 connection, and the response body is readable again. The budget now bounds what reaches the parser rather than what the transport allocates. It cannot undo the peak — libcurl expanded the body before we saw it — but it does stop the larger second cost of parsing a body that size into Python objects. The module note states that limit honestly instead of implying the Node port's guarantee, and a test pins the no-streaming decision so the trade-off cannot be undone by accident. Also validates max_response_bytes, which turned a typo into a blanket failure on every response, and restores the encoding-table wiring test.
The budget only ever bounded what reached the parser: libcurl decoded the body first, so the peak had already happened. Disabling CURLOPT_HTTP_CONTENT_DECODING hands this module the compressed bytes instead, which puts the cap back inside the decompressor — the guarantee the Node port has — while keeping the buffered read that a previous attempt at this had to give up. Measured end to end: a 512 MB bomb from 841 bytes on the wire is refused with RSS growing by the budget, and a 2 GB bomb costs 6 MB more than the 512 MB one, so the cost tracks the limit rather than the payload. `timeout` stays a wall-clock cap (2.0s) and the session still reuses connections (1 for 5 requests) — the two things streaming cost. The option is applied per request, not once at construction. Set once it works exactly once: `Session.request` resets the handle, so requests 2..n arrive pre-expanded with no budget in reach. Every double-based test passed through that because each built a fresh client; there is now one asserting the option is set on all three of three requests, and the local-server tests reuse a client so the lapse cannot come back quietly. Owning the decoding means decoding everything `Accept-Encoding` advertises, so `deflate` is implemented rather than left to a transport that no longer does it — `Core.html_headers` asks for it, and get_airlines() would have been served raw bytes. Both shapes are accepted: RFC 9110 says zlib-wrapped, plenty of servers send raw. Real FR24 traffic now decodes here instead of falling through the "transport already decoded" path: 0 fallbacks across 4 calls, where before every single request took it. That also makes the truncation and budget checks in those helpers live code rather than a dead branch with passing tests.
Taking decoding from libcurl means owning every shape the header arrives in, and the first pass did not. - The lookup was exact and case-sensitive, so `GZIP` or `gzip, br` fell through to identity and returned compressed bytes as content; callers got a UnicodeDecodeError out of json.loads with nothing logged. The token is normalised now, and an encoding with no decoder warns instead of passing bytes along as if they were readable. - Worse, `zstd` was reachable: with no explicit header, curl_cffi's chrome136 profile asks for "gzip, deflate, br, zstd" (verified), and nothing here decodes zstd. Every request now advertises exactly what `__content_encodings` implements. - `get_response_object().content` and `CloudflareError.response` were handing back the compressed body — both public, and a challenge page is the first thing anyone reads when debugging a block. The body is decoded during the request now and written back, so they read as they did before this branch. - `MAXFILESIZE_LARGE` bounds the download itself, so the received size is limited rather than merely checked once libcurl has buffered everything. Surfaced as DecompressionLimitError, because the retry policy treats raw curl errors as transient and retrying an oversized body is futile. On the Node side, `getCookie` and `__cookiesFor` disagreed about which same-named cookie wins — newest vs longest path — so a re-issued token could go out in the query string while the Cookie header on any /user/... request carried the stale one, and `Core.userLogoutUrl` is exactly that path. Both take the newest now. The cookie maps are also null-prototype: a cookie named `toString` read as an inherited function rather than as absent.
Review cleanups, no behaviour change on the happy path. `max_response_bytes` governed two different limits: the bytes on the wire and the bytes after expansion. Compression grows incompressible data, so a body that expands to just under the budget can arrive slightly over it and be rejected by a bound that was not meant for it. `max_download_bytes` is now separate, defaulting to the other. `supported_encodings` is derived from the decoder table instead of written out, so advertising an encoding with no decoder — the zstd bug from the previous commit — is no longer expressible. The post-hoc length check is labelled as the backstop it is: libcurl aborts first via MAXFILESIZE, so it is unreachable unless a transport ignores that option, and the stub-based test said so. Renamed to match, with a pointer to the socket-level test that proves the real bound. The module note claimed streaming cost connection reuse without qualifying it; the standalone path already opens a connection per call, so that cost lands on the session path only. The timeout degradation ruled streaming out on its own. On the Node side, the cookie map merged into the Cookie header was the last one built from a plain object literal, so it reintroduced the inherited prototype the other two had dropped.
All seven reproduced before fixing, and re-measured after. Python. The expansion budget stopped applying to bodies that reach no decoder: an identity 5 MiB body passed with max_response_bytes=1024, because the only check on the received bytes had moved to the download bound when the two knobs were split. Checked after decoding now, so the budget means the same thing whatever encoding arrived. Raw deflate carries neither header nor checksum, so a body that is not deflate can inflate to plausible bytes and be returned as content. Requiring the whole input to be consumed is the only tell available and takes 3000 fuzzed JSON/HTML bodies from 6 silent passes to 1. It applies to raw only: the zlib wrapper has an adler32 and validates itself, and demanding full consumption there rejected legitimate bodies that arrived with trailing padding. Node. Both error paths threw before the body was read, so undici had to destroy the socket: 10 sequential 500s opened 8 new connections against 2 for 200s. Every Cloudflare block and every 5xx therefore cost a fresh TLS handshake, compounding under RetryPolicy — the same handshake-per-request pattern this branch rejected stream=True over. The body is read before the checks now, and 10 errors open none. That read also ran outside the abort timer, so timeout only covered time-to-headers. A server dripping 20 bytes at 400ms resolved after 8032ms against timeout 500 — precisely the degradation cited for rejecting streaming on the Python side. The read is inside the timer now: TimeoutError at 504ms. Draining the body for that first fix left response.bodyUsed true, so the challenge page CloudflareError exposes was no longer readable. The error carries it directly. Also: the path-length tie-break in __cookiesFor was unreachable, since storedAt is unique per cookie; dropped, with the same-name collapse documented as the deliberate limit it is. engines.node moves to >=18.17, which is what undici 6.28 actually requires. And the three size checks now say which case each one covers, so the next reader does not remove the wrong "duplicate".
Both were capabilities lost when this module took content decoding over, and both surfaced as an unreadable body rather than an error. Padding after a gzip trailer restarted a member and raised, which `__decode_body` then swallowed as "the transport already decoded this" and returned the still-compressed bytes — so gzip'd JSON plus three NUL bytes died in json.loads. libcurl stopped at the stream end and ignored the tail, and the deflate helper already tolerated exactly this for the zlib shape, so the gzip helper was the inconsistency. Another member is only started when the tail actually looks like one. Stacked encodings missed the table entirely: `Content-Encoding: gzip, br` found no single-token decoder, warned, and handed back compressed bytes. The header is split now and the decoders applied in reverse, so gzip, "gzip, br" and "gzip, deflate, br" all round-trip, whitespace and case included. Also, a Set-Cookie whose Domain does not cover the request host was kept as host-only instead of discarded, so a cookie scoped to some other domain was replayed on every later request to that host. RFC 6265 5.3.6 says ignore it. And a module comment still pointed at `_new_curl_handle`, which the per-request rework replaced with `_keep_body_encoded`.
…sponses `Object.create(null)` on the result's `cookies` map broke `cookies.hasOwnProperty(name)` in consumer code — a silent breaking change under a type that still says `Record<string, string>`, shipping as a patch. That map is public; the prototype hardening belongs on the jar's internal maps, which is where it stays. The Cloudflare challenge page skipped the BOM strip every other body path goes through, so `err.body` could start with U+FEFF and defeat a JSON.parse or a prefix comparison. A stacked `Content-Encoding` that failed halfway returned the half-decoded intermediate as if it were the body, and picked its log level from the stage that had succeeded rather than the one that failed. The recovery is a claim about the body as received, so that is what it returns now. And cookies on an error response were dropped: `Session.request` only banked them on success, so a Cloudflare 403 — the response that hands out `cf_clearance` — left the jar empty and RetryPolicy replayed the identical blocked request. The errors carry their `Set-Cookie` headers now and the session stores them before rethrowing.
Both are regressions from the commit that started banking cookies off failed responses, and both were reproduced before fixing. `rawCookies` was an enumerable own property on the error, so `JSON.stringify(err)` and any log line dumping it printed the session credentials the response had just handed out — in a branch whose whole point is keeping cookies away from where they do not belong. Defined non-enumerable now: `Object.keys` and `JSON.stringify` no longer see it, while the jar still does. The catch also credited the cookie to the requested URL rather than the one that answered, so a cookie set after a redirect was replayed to a host that never set it. That is the same defect the success path was fixed for earlier in this branch; the final URL travels with the error now. Note, unchanged and pre-existing: `util.inspect(err)` still reveals a Set-Cookie through `err.response`, because exposing the response is what that field is for. Left alone deliberately rather than widened into another change.
The shim existed so `from FlightRadar24 import FlightRadar24API` kept working after the package was renamed to match the PyPI distribution and the Node package. It has been emitting a DeprecationWarning saying it would be removed in a future release; this is that release. Gone with it: the module itself, its backwards-compatibility tests, its entry in the wheel's package list, and the extra path in the flake8 step. Verified against a built wheel — it now ships `FlightRadarAPI` alone, and `import FlightRadar24` raises ModuleNotFoundError. Version moves to 1.6.0 in both packages, since publish.yml requires them to match. Note for the release: removing a public import path is not a minor change. Anyone still on `FlightRadar24` breaks on upgrade, which semver calls a major bump. 1.6.0 is what was asked for and nothing is irreversible until the tag is cut, but 2.0.0 is the number that matches what this does.
JeanExtreme002
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates both the Node.js and Python FlightRadarAPI SDKs to (1) fix session cookie handling in the Node port by properly parsing and scoping cookies, (2) enforce a hard cap on decoded response size to mitigate decompression-bomb style payloads, (3) raise dependency floors and CI security posture, and (4) remove the deprecated Python FlightRadar24 import alias.
Changes:
- Node: replace the flat cookie bag with a scoped cookie jar (domain/path/secure/expiry) and bank cookies even from failed responses.
- Node + Python: enforce a bounded response-body budget (default 64 MiB) with per-request override; add
DecompressionLimitError. - CI/deps/docs: bump dependency floors, pin GitHub Actions by SHA, fix/enforce audits, and harden docs deployment permissions; remove Python
FlightRadar24shim.
Reviewed changes
Copilot reviewed 22 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| python/tests/test_request_transport.py | Adds extensive tests for bounded decompression, encoding robustness, and transport-level limits. |
| python/tests/test_legacy_import.py | Removes legacy-alias tests now that FlightRadar24 import path is dropped. |
| python/tests/_request_doubles.py | Extends request doubles with a curl handle stub to verify per-request curl options. |
| python/pyproject.toml | Adds dependency minimum versions and removes FlightRadar24 from wheel packages. |
| python/FlightRadarAPI/request.py | Implements bounded decoding (gzip/deflate/br), per-request curl options, and size limits. |
| python/FlightRadarAPI/errors.py | Introduces DecompressionLimitError. |
| python/FlightRadarAPI/init.py | Bumps version to 1.6.0 and exports DecompressionLimitError. |
| python/FlightRadar24/init.py | Removes deprecated Python import-alias module. |
| nodejs/tests/testRequestTransport.js | Adds cookie-jar scoping tests, response budget tests, and error-path shape assertions. |
| nodejs/tests/testFeedRetry.js | Updates cookie setup to use the new jar store path. |
| nodejs/package.json | Bumps version to 1.6.0, raises Node engine floor, bumps undici floor. |
| nodejs/package-lock.json | Lockfile updates reflecting the new Node/package versions and undici floor. |
| nodejs/FlightRadarAPI/request.js | Implements bounded body reads, cookie jar parsing/scoping, error cookie banking, and new return shape. |
| nodejs/FlightRadarAPI/index.js | Exports DecompressionLimitError from the main package entrypoint. |
| nodejs/FlightRadarAPI/index.d.ts | Updates TS types for new request return shape and errors. |
| nodejs/FlightRadarAPI/errors.js | Adds DecompressionLimitError and extends CloudflareError to carry body. |
| .gitignore | Ignores MkDocs build output (site/). |
| .github/workflows/python-package.yml | Pins actions, updates lint targets, and enforces runtime-dependency auditing. |
| .github/workflows/publish.yml | Pins actions and hardens PyPI publish action pinning. |
| .github/workflows/node-package.yml | Pins actions and fixes/enforces npm audit execution. |
| .github/workflows/lint-pr-title.yml | Pins semantic PR title action to a commit SHA. |
| .github/workflows/labeler.yml | Pins github-script action to a commit SHA. |
| .github/workflows/deploy-docs.yml | Switches to Pages artifact deploy, reduces repo permissions, pins actions, and adds concurrency. |
| .github/workflows/delete-pr-branch.yml | Pins github-script action to a commit SHA. |
| .github/docs-requirements.txt | Adds pinned docs build requirements for deterministic/supply-chain safer builds. |
Files not reviewed (1)
- nodejs/package-lock.json: Generated file
Suppressed comments (1)
nodejs/FlightRadarAPI/request.js:616
- The JSDoc return type for
Session.request()is stale (it inherits the modulerequest()shape, includingrawCookiesandurl). Keeping it outdated will mislead consumers using the class directly.
* @param {string} url
* @param {object} [options={}]
* @return {Promise<{content: *, statusCode: number, cookies: object}>}
*/
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+107
to
+130
| output = b"" | ||
| remaining = data | ||
|
|
||
| while remaining: | ||
| # A new object per member: `Content-Encoding: gzip` may carry several, | ||
| # and one decompressor stops at the first trailer. | ||
| decompressor = zlib.decompressobj(_GZIP_WBITS) | ||
| output += decompressor.decompress(remaining, limit + 1 - len(output)) | ||
|
|
||
| if len(output) > limit or decompressor.unconsumed_tail: | ||
| raise DecompressionLimitError( | ||
| f"gzip body expands past the {limit} byte decompression limit." | ||
| ) | ||
|
|
||
| if not decompressor.eof: | ||
| raise zlib.error("gzip stream ended mid-member") | ||
|
|
||
| # Another member only when the tail looks like one. Trailing padding is | ||
| # not an error: libcurl stopped at the stream end and ignored it, and | ||
| # the deflate helper tolerates the same thing. | ||
| tail = decompressor.unused_data | ||
| remaining = tail if tail.startswith(_GZIP_MAGIC) else b"" | ||
|
|
||
| return output |
Comment on lines
+1
to
+3
| # Pinned because this workflow runs with `contents: write`: an automatic | ||
| # upgrade to a compromised release would execute with a token that can push | ||
| # to the repository. Bump deliberately, not implicitly. |
From a Copilot review of the PR, all three verified before acting.
`_decompress_gzip` accumulated with `output += ...`, which copies
everything decoded so far on every member: 1270ms for a 500-member body
against 25ms. The review suggested a bytearray, but measuring says that
is slower than the status quo on the single-member path nearly every
response takes (23.1ms vs 15.2ms on 32 MiB), because converting back to
bytes pays another full copy. Collecting the members and joining once
wins both cases — 11.9ms and 19.0ms — and returns the sole member
untouched when there is only one.
The JSDoc on `request()` and the three methods wrapping it still
described the old `{content, statusCode, cookies}` return, four places
in all, after `rawCookies` and `url` were added.
And the docs pin file justified itself with `contents: write`, which
that workflow no longer has — the deploy moved to the Pages artifact
earlier in this branch. The reason for pinning still stands, so the
wording says what it actually is: those packages execute during
`mkdocs build`.
The brotli helper's docstring claimed peak memory stays near the limit. Measured against a 7 MiB body under an 8 MiB cap, it was 3.0x the output: growing a bytearray copies each piece in, and converting back to bytes copies the whole thing again. Collecting the pieces and joining — the shape the gzip helper already uses — brings that to 1.0x, because the single-piece response almost every reply produces is returned without being copied at all. With the 64 MiB default that is the difference between a ~190 MiB peak and a ~64 MiB one, which matters to anyone sizing the limit for a small container. Re-checked the behaviours this must not disturb: empty body, ordinary round-trip, bomb refused, truncated stream still raising at 50/75/90% of a real stream, and the "transport already decoded this" fallback. A test now pins the memory shape, since a correctness test cannot see it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a live cookie-handling bug in the Node SDK, gives both ports a bound on how far a response body may expand, raises dependency floors past known advisories, hardens CI, and drops the deprecated
FlightRadar24import alias.Released as 1.6.0 in both packages.
1. The Node session jar ignored every cookie attribute
The jar kept cookies in a flat
{name: value}object and replayed all of them on every request. Probing two origins against the old code:Three separate defects:
Set-Cookiewas split on every=, so_frPl=sess.token.with==paddingwas stored assess.token.with. That truncated value is whatgetAirportDetailssends astokenandgetFlightsasenc— this was corrupting live requests, not sitting latent.www.flightradar24.comwas attached tocdn.,api.,data-live.anddata-cloud.too. All siblings, so not a third-party leak, butDomain/Path/Securewere not honoured at all.Max-Age=0— a deletion instruction — stored the cookie.Now: attributes are parsed properly, the jar is a
Mapkeyed by name/domain/path,Max-Age/Expiresdelete on the way in and are re-checked on the way out,Securecookies stay off plaintext, and aDomainthat does not cover the host discards the cookie rather than narrowing it (RFC 6265 §5.3.6). Cookies are credited to the host that ended a redirect chain, on both the success and error paths.Cookies from a failed response are now banked too: a Cloudflare 403 is the response that hands out
cf_clearance, and discarding it madeRetryPolicyreplay the identical blocked request. They ride on the error as a non-enumerable property, soJSON.stringify(err)does not print session credentials.Scope note: this fixes what the jar sends.
getAirportDetailsandgetFlightsstill read_frPlviagetCookie()and forward it by hand as thetoken/encquery parameter toapi.anddata-cloud.— that is FR24's API shape and is unchanged here.The Python port was already correct on all of this; curl_cffi's jar scopes by host and preserved the full value under the identical probe.
2. A compressed body could expand without limit
Measured: ~500 bytes on the wire expands to 300 MB, ~1.7 KB to 1 GB — enough to kill a process on a small host. Both ports now refuse a body past a 64 MiB budget (
max_response_bytes, overridable per request).Node streams
response.bodyagainst the budget, because undici decompresses in the transport. Python takes content decoding away from libcurl (CURLOPT_HTTP_CONTENT_DECODING = 0) so the cap sits inside the decompressor, and bounds the download itself withMAXFILESIZE_LARGE. A 512 MB bomb from 841 bytes is refused with RSS growing by the budget; a 2 GB bomb costs 6 MB more than the 512 MB one, so the cost tracks the limit rather than the payload.Two things that reaching this required, both worth knowing:
Session.requestresets the handle — and requests 2..n arrive pre-expanded with no budget in reach.deflate(both the RFC's zlib-wrapped shape and the raw one servers actually send), stacked encodings (gzip, brundone in reverse), case-insensitive and whitespace-padded tokens, padding after a gzip trailer, and multi-member gzip.Accept-Encodingnow advertises exactly what is implemented, derived from the decoder table so advertising something undecodable is not expressible — curl_cffi's chrome136 profile otherwise asks forzstd, which nothing here can read.Truncated bodies raise rather than passing as whole, and
get_response_object()/CloudflareError.responsestill expose a decoded body.3. Dependency floors
undici^6.13.0→^6.28.0. The lockfile already resolved a clean version, but consumers of a published library resolve the declared range, and the old floor admitted 15 advisories including request smuggling and an unbounded decompression chain.engines.nodemoves to>=18.17to match what undici 6.28 actually requires.curl_cffi>=0.15.0closes GHSA-qw2m-4pqf-rmpp (redirect-based SSRF, CVSS 7.4).Brotli>=1.2.0is required for the bounded-decompression API used above.beautifulsoup4>=4.12.0has no advisory behind it and is a baseline only.These three were also the only 3 of 262 packages in the dependency graph with no resolved version, because the requirements were unbounded — so Dependabot could not match advisories against them and had never reported a pip finding. Adding floors turns Python monitoring on.
4. CI hardening
pypa/gh-action-pypi-publish@release/v1was the worst case — a branch, in the job holdingid-token: writefor PyPI../nodejsand the step also didcd nodejs, so every run failed withcd: nodejs: No such file or directorywhilecontinue-on-errorreported success. That is how the vulnerableundicifloor went unnoticed. Fixed and now enforced.actions/deploy-pages, which dropscontents: writetocontents: read; its dependencies are pinned, so a compromised docs package no longer runs with a token that can push to the repo.5. Breaking changes
FlightRadar24is gone. The alias existed sofrom FlightRadar24 import FlightRadar24APIkept working after the rename, and had been emitting aDeprecationWarningsaying it would be removed. Import fromFlightRadarAPIinstead. Verified against a built wheel: it shipsFlightRadarAPIalone, andimport FlightRadar24raisesModuleNotFoundError.engines.node>=18→>=18.17, so Node 18.0–18.16 withengine-strictwill refuse to install.Semver would call removing a public import path a major bump. 1.6.0 is a deliberate choice, not an oversight: the alias had been formally deprecated with a warning, and the affected
enginesrange is narrow.New public API:
DecompressionLimitError(both ports),max_response_bytes/max_download_bytes(Python),CloudflareError.bodyandrawCookies/urlon the request result (Node).Verification
flake8, mypy, eslint and tsd clean. The Node suite runs on all three matrix versions locally, after a redirect test that passed on 22 and failed on 18. Behaviour was checked against local servers and real FR24 rather than through doubles alone — several defects here were found only that way, because the doubles fed code paths production never reaches.
Known limitations, accepted rather than fixed
Domain=com, butDomain=co.ukwould still pass for a caller on a host under that suffix. Closing it properly means a PSL dependency, hard to justify for an SDK talking to five fixed FR24 hosts.Cookieheader is built from a flat map.util.inspect(err)can reveal aSet-Cookiethrougherr.response. Pre-existing, and exposing the response is what that field is for.accesstokenheader survives a cross-origin redirect in both ports. Cookies are correctly stripped; the custom header is not. Needs an FR24 open-redirect or DNS/MITM to matter.getAirlineLogoandgetAirport. The host is a fixed literal, so this is path confusion rather than SSRF.A note on how this PR is shaped
Roughly half the diff is the decompression budget, hardening against a threat that requires FR24, its CDN, or the network path to be compromised. The defect that was actively corrupting user tokens is a small fraction of it, and shipped many review rounds later than it needed to. Speculative hardening should not share a PR with an active production bug; extracting it once entangled was riskier than shipping it. Worth avoiding next time.