fix(v2ex): percent-encode caller values in API query strings - #565
Closed
SEPURI-SAI-KRISHNA wants to merge 1 commit into
Closed
fix(v2ex): percent-encode caller values in API query strings#565SEPURI-SAI-KRISHNA wants to merge 1 commit into
SEPURI-SAI-KRISHNA wants to merge 1 commit into
Conversation
Owner
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.
Problem
agent_reach/channels/v2ex.pyinterpolates caller-supplied values straight into query strings:Nothing is percent-encoded, so five separate things go wrong. Reproduced on
mainby stubbing_get_jsonand parsing what would have been sent:node_name="python&page=99"?node_name=python&page=99&page=1pagetwice — pagination silently hijackednode_name="foo#bar"?node_name=foo#bar&page=1node_name=foo,pagenever sent — it lands in the fragmentnode_name="c++"?node_name=c++&page=1node_name="c "—+decodes as spacenode_name="hello world"?node_name=hello world&page=1username="张三"UnicodeEncodeError: 'ascii' codec can't encode charactersThe last one is the worst: any non-ASCII value raises
UnicodeEncodeErrorstraight out ofurllib— an unhandled crash rather than a channel error. That's easy to hit on a Chinese-language site from a Chinese-first project.These values reach the channel from agents and from parsed URLs, so they are not guaranteed to be tidy slugs.
The same raw interpolation is in two returned display URLs (
/member/<username>,/t/<topic_id>) and in the search advisory URL thatsearch()hands the user to click.Fix
One
_api_url()helper that builds every V2EX API URL throughurllib.parse.urlencode, plusurllib.parse.quoteon the two path-segment fallbacks and the search advisory link.After the change every value round-trips exactly through
parse_qs,page=1always survives, and the non-ASCII call reaches the server and returns a normal HTTP 404 instead of crashing.get_topicis typedtopic_id: intbut nothing enforces it; routing it throughurlencodemeans a stray string can no longer truncate the query, while plain ints serialise unchanged (id=123).Tests
15 new cases in
tests/test_v2ex_channel.py, following the existing module-level style and stubbing_get_jsonso nothing touches the network:test_get_node_topics_percent_encodes_node_name/test_get_user_percent_encodes_username— parametrised over all five hostile values, asserting the value round-trips and no fragment appearstest_get_topic_percent_encodes_ids_in_both_requests/test_get_topic_replies_request_keeps_page_parameter— both requestsget_topicmakestest_get_topic_accepts_plain_int_unchanged— regression guard for the normal pathtest_search_advisory_url_is_encoded,test_fallback_display_urls_are_encodedpytest -q: 457 passed (was 442).ruff checkadds no new findings.Scope
Confined to URL construction. It does not touch
_get_json, so it does not overlap the TLS-EOF transport work in #531 or #536 — both of those rewrite only the fetch layer and neither encodes query values. This should merge cleanly alongside either.