(Fix) Strict numeric validation for announce fields#5530
Open
skavinski-lab wants to merge 3 commits into
HDInnovations:developmentHDInnovations/UNIT3D:developmentfrom
skavinski-lab:fix/announce-numeric-validationskavinski-lab/UNIT3D:fix/announce-numeric-validationCopy head branch name to clipboard
Open
(Fix) Strict numeric validation for announce fields#5530skavinski-lab wants to merge 3 commits intoHDInnovations:developmentHDInnovations/UNIT3D:developmentfrom skavinski-lab:fix/announce-numeric-validationskavinski-lab/UNIT3D:fix/announce-numeric-validationCopy head branch name to clipboard
skavinski-lab wants to merge 3 commits into
HDInnovations:developmentHDInnovations/UNIT3D:developmentfrom
skavinski-lab:fix/announce-numeric-validationskavinski-lab/UNIT3D:fix/announce-numeric-validationCopy head branch name to clipboard
Conversation
is_numeric() and (int) casts accepted floats and scientific notation (e.g. "1e19"), and no upper bound existed. A client could credit itself ~9.2 exabytes of upload in a single announce, and out-of-range floats overflowed the (int) cast into platform-dependent values that poison the unsigned bigint peer/history upsert batches. Require plain non-negative decimal integers (ctype_digit) capped at 1 PiB (MAX_ANNOUNCE_VALUE) for port, uploaded, downloaded, left, numwant and corrupt.
Laravel's test client sends accept-language/referer/accept-charset headers by default, which trips tracker error 122 (Abnormal access blocked) before field validation is reached. Mirror the existing test by nulling those headers, and assert on the specific error 134 message so the rejection tests prove the validation fired rather than any generic failure.
Laravel's TrimStrings middleware strips the padding before the controller runs, so the value is valid by validation time.
Collaborator
|
Hi, just a heads up - would really appreciate it if you refrain from using AI prose when communicating with us maintainers. It's just so tiring to read. |
Author
Understand. I'll keep such more concise. My English is not the best, so it also helps there. |
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
2 participants
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.
The announce endpoint validated
uploaded,downloaded,left,numwantandcorruptwith onlyis_numeric($value) || $value < 0. Two problems:uploaded=9000000000000000000passes (it's underPHP_INT_MAX), and the delta over the stored peer base is credited straight intousers.uploadedbyProcessAnnounce. One request with a valid passkey grants a huge amount of upload credit.is_numeric('1e19')is true and'1e19' < 0is false, but(int) '1e19'overflows to a negative value on 64-bit. That negative flows into the Redis peer/history batches and gets upserted intounsignedBigIntegercolumns, which fails the wholeAutoUpsertPeers/AutoUpsertHistoriestransaction.Fix: require plain non-negative decimal integers with
ctype_digit(), capped at a newMAX_ANNOUNCE_VALUEconstant of 1 PiB, well above any real per-session client total. Applied toport,uploaded,downloaded,left,numwantandcorrupt. The now-redundantctype_digit()in the port check is dropped since validation runs before it.Tests: added cases for oversized values, scientific notation, negatives, floats, whitespace padding, hex,
MAX_ANNOUNCE_VALUE + 1, and a passing boundary at exactly 1 PiB.Conforming clients send decimal integers in range, so nothing changes for them. Malformed or out-of-range values now get failure 134 instead of being silently miscast.