fix: six bugs found while auditing the library - #6
Merged
Merged
Conversation
Each of these was reproduced before being fixed, and each has a regression test. The options hash was consumed rather than read. #from and #to called Hash#delete on the caller's hash, so a second call with the same hash lost :from and :to and fell through to language detection, and a frozen hash -- which is what a hash of settings kept in a constant is -- raised FrozenError on the first call. The request copies the hash now. The chunker measured three different things against one limit. #next_chunk? added the escaped size of the incoming value to a running total of raw String#size, #update_chunk accumulated the raw size, and #validate_value_size compared the raw size. Since CGI.escape inflates Cyrillic sixfold, a chunk of Russian text ran several times over the limit, and a single value whose request size was five times the limit passed validation. Everything is measured as escaped now, which is what actually goes over the wire. A detected source language never matched the target. #detect_language returns a String while :to is usually a Symbol, so `from == to` could not fire and the text was paid for and translated into its own language. The two are compared case-insensitively as strings now. A short response from the API shifted nils into the results, which surfaced as a NoMethodError on nil.strip two layers away in Spacing. Request::Error is raised at the call site instead. Scalars other than strings crashed. nil and Integer raised NoMethodError on #empty?, and Symbol raised TypeError inside Ox. The emptiness check no longer assumes a String, the tokenizer ignores non-strings, and such values are passed through untouched. nil keeps collapsing to "" inside a structure, as before. The count limit was off by one. #next_chunk? tested `>` after the value had been added, so a chunk held count_limit + 1 texts. The existing test had the off-by-one baked into its expectation.
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.
Each of these was reproduced against the real library before being fixed, and each has a regression test. Version bumped to 2.1.0 — all are fixes, but
DeepLDiff::Request::Erroris a new public constant.1. The options hash was consumed, not read
request.rb:27,31calledHash#deleteon the caller's hash.A frozen hash — which is what a hash of settings kept in a constant is — raised
FrozenErroron the very first call. The request copies the hash in its initializer now.2. The chunker measured three different things against one limit
next_chunk?CGI.escape(value).size— escapedupdate_chunkvalue.size— raw, accumulatedvalidate_value_sizevalue.size— rawSo the escaped size of the incoming value was added to a running total of raw sizes.
CGI.escapeinflates Cyrillic sixfold:Chunking is the only guard against DeepL's request-size limit, and it did not work for any non-Latin text. Everything is measured escaped now — that is what goes over the wire.
3. A detected source language never matched the target
detect_languagereturns aString;:tois usually aSymbol.from == tocould not fire:Compared case-insensitively as strings now. The cache key is deliberately left alone so existing cached entries stay valid.
4. A short API response surfaced two layers away
updates.shiftreturnednil, which reachedSpacing.restoreasNoMethodError: undefined method 'strip' for nil. Now:Raised at the call site, before a
nilcan be written to the cache.5. Non-string scalars crashed
The emptiness check no longer assumes a String, the tokenizer ignores non-strings, and such values pass through untouched.
nilkeeps collapsing to""inside a structure, exactly as before — that behavior is covered by an existing test and stays.6. The count limit was off by one
tail.texts.size > count_limitwas tested after the value was added, so a chunk heldcount_limit + 1texts:The existing test had the off-by-one baked into its expectation; it is corrected here.
Test plan
bundle exec rake test— 46 runs, 75 assertions, 0 failures, on Ruby 4.0.5 and 3.2.4 (was 34 runs).bundle exec rubocop— no offenses.https://claude.ai/code/session_01Pda49PcgziFVnibkKKjXRE