aio-libs / aio-libs/yarl

C and pure Python quoters disagree when % is in safe and requote is on

Ouverte
#1,869 0 commentaires 0 réactions 0 personnes assignées Voir sur GitHub
bug
Langage dominant
Python
Étoiles
1.5k
Forks
215
Merge moyen
1 j 2 min
PR mergées (30 j)
13

Description

### Please confirm the following

- [x] I agree to follow the [aio-libs Code of Conduct][CoC]

[CoC]: ../../.github/blob/master/CODE_OF_CONDUCT.md

- [x] I have checked the [current issues][issue search] for duplicates.

[issue search]: ../search?q=is%3Aissue&type=issues

- [x] I understand this is open source software provided for free and that I might not receive a timely response.
- [x] I am positive I am **NOT** reporting a (potential) security
vulnerability, to the best of my knowledge. *(These must be shared by
submitting [this report form][vulnerability report form] instead, if
any hesitation exists.)*

[vulnerability report form]: ../security/advisories/new

- [x] I am willing to submit a pull request with reporoducers as xfailing test cases or even entire fix. *(Assign this issue to me.)*

### Describe the bug

`_Quoter` has two backends, the Cython `_quoting_c` and the pure Python
`_quoting_py`, and `yarl/_quoting.py` picks between them depending on the build
and on `YARL_NO_EXTENSIONS`. When `%` is included in `safe` and `requote` is
left at its default of `True`, the two backends return different strings.

The cause looks like the fast path in `_do_quote_or_skip`
(`yarl/_quoting_c.pyx`):

```cython
# If everything in the string is in the safe
# table and all ASCII, we can skip quoting
while idx:
idx -= 1
ch = PyUnicode_READ(kind, data, idx)
if ch >= 128 or not bit_at(self._safe_table, ch):
must_quote = 1
break

if not must_quote:
return val
```

The skip condition asks only whether every character is ASCII and in the safe
table; it does not take `_requote` into account. Putting `%` in `safe` sets that
bit, so a string like `%41` consists entirely of safe-table characters,
`must_quote` stays `0`, and `_do_quote` never runs. The pure Python backend has
no equivalent short-circuit, so it still performs the requote pass.

With `%` absent from `safe`, or with `requote=False`, the two backends agree.

I want to be clear about the scope: **this is not reachable through the public
API and I am not reporting it as a security issue.** `_Quoter` is private, none
of the quoters in `_quoters.py` put `%` in `safe`, and no test does either. You
may reasonably decide `safe="%"` is not a supported configuration and close
this.

The reason I thought it was still worth writing up is that
`PATH_SAFE_UNQUOTER = _Unquoter(ignore="/%", unsafe="+")` already exists, so a
quoter-side analogue that wants to leave `%` alone seems like a natural thing
for someone to add later. If that ever happened, the C and pure Python builds
would produce different output with nothing in the suite catching it, since the
existing parametrized tests never exercise that configuration.

### To Reproduce

```python
from yarl._quoting_c import _Quoter as CQuoter
from yarl._quoting_py import _Quoter as PyQuoter

for value in ["%41", "%4a", "%", "%zz", "%2f%2F"]:
c = CQuoter(safe="%")(value)
py = PyQuoter(safe="%")(value)
print(f"{value!r:10} C={c!r:10} py={py!r:10} {'DIFFER' if c != py else ''}")
```

### Expected behavior

The two backends return the same string for the same input and constructor
arguments, as they do for every other configuration I tried.

I do not have a view on which of the two is the correct answer here.
`safe="%"` and `requote=True` pull in opposite directions, one asking for `%` to
be left alone and the other asking for `%XX` to be interpreted, so the intended
resolution is a decision for you rather than something I can read off the code.

### Logs/tracebacks

```python-traceback
No exception is raised. The output of the reproducer above:

'%41' C='%41' py='A' DIFFER
'%4a' C='%4a' py='J' DIFFER
'%' C='%' py='%25' DIFFER
'%zz' C='%zz' py='%25zz' DIFFER
'%2f%2F' C='%2f%2F' py='%2F%2F' DIFFER

With `safe=""` the same script prints no `DIFFER` rows:

'%41' C='A' py='A'
'%4a' C='J' py='J'
'%' C='%25' py='%25'
'%zz' C='%25zz' py='%25zz'
'%2f%2F' C='%2F%2F' py='%2F%2F'

`safe="@:"` and `requote=False` behave like the `safe=""` control above.

For what it is worth, I compared the two backends over a wider space while
looking at this: percent-escape edge cases, the code point range in bands
including surrogates and astral characters, and a sweep of the constructor
arguments. Every divergence found was this one case, `%` in `safe` with
`requote=True`. I did not find any divergence in the unquoters.
```

### Python Version

```console
$ python -V
Python 3.13.13

Observed in a container built from the official `python:3.13-slim` image.
```

### multidict Version

```console
Not installed. The reproducer imports only yarl._quoting_c and yarl._quoting_py,
so multidict was not required.
```

### propcache Version

```console
Not installed, for the same reason as multidict
```

### yarl Version

```console
Built from master at 06abbc731ac269df9ba6ed36e6836e173b873dd6, with the Cython accelerator compiled, so that both backends could be imported in one process.
```

### OS

Debian GNU/Linux 13 (trixie), x86_64, inside the official python:3.13-slim container image.

### Additional context

Nothing about this looks platform specific: the difference is in which code path
runs, not in anything the platform decides. I have only actually run it on the
one machine above, though, so that is an inference from reading rather than
something I have measured across systems.

No proxy or network is involved; the reproducer only imports the two quoting
modules and calls them directly.

Guide de contribution

Ouvrir le guide de contribution

Évaluation

Cette issue n'a pas encore été évaluée.

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.