Skip to content

extract_option does not raise exception on leftover options #601

Description

@blightbow

Describe the bug

Client(**kwds) and the request methods (get, post, ...) accept unhandled keyword arguments and silently discard them. This means a typo in an option fails open with no feedback.

The most glaring variant of this problem is for dns_options. Writing dns= or dns_option= instead of dns_options= silently fails to apply custom DNS resolution, so a caller who is using add_resolve to pin a hostname to a validated address connects to whatever the system resolver returns instead. (example: a silently failing SSRF guard against DNS rebinding)

To Reproduce
Given the following code:

(click to expand: reproduction script)
#!/usr/bin/python3
"""Unknown keyword arguments are silently ignored by Client and by request methods."""

import asyncio
from datetime import timedelta
from ipaddress import ip_address

from wreq import Client, DnsOptions, Emulation

TARGET = "https://example.com"


async def case1_dns_options_typo():
    """A typo in `dns_options` silently disables DNS pinning."""
    opts = DnsOptions()
    opts.add_resolve("example.com", [ip_address("127.0.0.1")])

    # Correct spelling: the pin applies and the connection is refused.
    try:
        await Client(emulation=Emulation.Chrome149, dns_options=opts).get(
            TARGET, timeout=timedelta(seconds=10)
        )
        print("  dns_options=  -> request SUCCEEDED (pin was not applied?)")
    except Exception as e:
        print(f"  dns_options=  -> pin applied, connection refused ({type(e).__name__})")

    # One character different: silently ignored, request reaches the real host.
    try:
        r = await Client(emulation=Emulation.Chrome149, dns_option=opts).get(
            TARGET, timeout=timedelta(seconds=10)
        )
        print(f"  dns_option=   -> request SUCCEEDED to {r.remote_addr}  <-- pin silently dropped")
    except Exception as e:
        print(f"  dns_option=   -> raised {type(e).__name__}: {e}")


async def case2_client_constructor():
    """A wholly unknown kwarg on the constructor is accepted."""
    Client(emulation=Emulation.Chrome149, complete_nonsense=1)
    print("  Client(complete_nonsense=1)          -> accepted, no error")


async def case3_request_method():
    """A wholly unknown kwarg on a request method is accepted."""
    client = Client(emulation=Emulation.Chrome149)
    r = await client.get(TARGET, complete_nonsense=1, timeout=timedelta(seconds=15))
    print(f"  client.get(complete_nonsense=1)      -> accepted, status {r.status}")

async def main():
    import wreq

    print(f"wreq {getattr(wreq, '__version__', '0.12.1')}\n")
    print("case 1: dns_options typo (security relevant)")
    await case1_dns_options_typo()
    print("\ncase 2: unknown kwarg on constructor")
    await case2_client_constructor()
    print("\ncase 3: unknown kwarg on request method")
    await case3_request_method()


asyncio.run(main())

Result:

wreq 0.12.1

case 1: dns_options typo (security relevant)
  dns_options=  -> pin applied, connection refused (ConnectionError)
  dns_option=   -> request SUCCEEDED to 104.20.23.154:443  <-- pin silently dropped

case 2: unknown kwarg on constructor
  Client(complete_nonsense=1)          -> accepted, no error

case 3: unknown kwarg on request method
  client.get(complete_nonsense=1)      -> accepted, status 200 OK

Expected behavior
requests / httpx raise TypeError for unexpected keyword arguments. The least astonishing behavior would be to mirror these, barring a compelling technical reason.

Desktop (please complete the following information)

  • OS: MacOS
  • Browser N/A
  • Version 26.5.1

Additional context
I considered submitting a PR for this, but this would be a breaking change and worth some discussion. The breakage would be exposing a bug in the most common case, but a breaking change nonetheless.

There's also a risk of this impacting the fix in flight for #591 if I submit a PR without coordination.

A fix would look something like this in wreq-python/src/macros.rs#extract_option:

# existing (unchanged)
macro_rules! extract_option {
    ($ob:expr, $params:expr, $field:ident) => {
        if let Ok(value) = $ob.get_item(pyo3::intern!($ob.py(), stringify!($field))) {
            $params.$field = value.extract()?;
        }
    };
}

# new handler for the existing call sites
macro_rules! extract_options_safe {
    ($ob:expr, $params:expr, [$($field:ident),* $(,)?]) => {{
        $( extract_option!($ob, $params, $field); )*
        reject_unknown_keys($ob, &[$(stringify!($field)),*])?;
    }};
}

Metadata

Metadata

Assignees

No one assigned

    Labels

    C-featureCategory: feature. This is requesting a new feature.C-refactorCategory: refactor. This would improve the clarity of internal code.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions