Historisation des ressources : support content-disposition en latin1 #3988

ptitfred · 2024-06-11T13:58:07Z

Les headers HTTP sont encodés en latin1 (iso-8859-1), les chaînes de caractères elixir sont encodés en UTF-8. L‘on pourrait croire que la bibliothèque HTTP cliente décoderait de latin1 vers utf8 et fournirait des chaînes de caractères systématiquement mais ça ne semble pas être le cas. HTTPoison produit le même résultat pour l‘URL de la ressource mentionnée dans l‘issue.

Quand le contenu d‘un header ne rentre pas dans le subset ASCII (commun à latin1 et utf8), Req renvoie un binary que l‘utilisateur de la bibliothèque devra lui-même décoder. Cette pull request se propose de faire cela pour l‘historisation de ressources.

Ma compréhension (partielle) est qu‘il s‘agit d‘un bug (ou a minima un défaut de Req et HTTPoison). Si d‘autres que moi pensent que c‘est en effet un défaut je logguerai une issue sur Req ou HTTPoison (ou les deux).

Closes #3984.

Les headers HTTP sont encodés en latin1 (iso-8859-1), les chaînes de caractères elixir sont encodés en UTF-8. L‘on pourrait croire que la bibliothèque HTTP cliente décoderait de latin1 vers utf8 et fournirait des chaînes de caractères systématiquement mais ça ne semble pas être le cas. HTTPoison produit le même résultat pour l‘URL de la ressource mentionnée dans l‘issue. Quand le contenu d‘un header ne rentre pas dans le subset ASCII (commun à latin1 et utf8), Req renvoie un binary que l‘utilisateur de la bibliothèque devra lui-même décoder. Cette pull request se propose de faire cela pour l‘historisation de ressources. Ma compréhension (partielle) est qu‘il s‘agit d‘un bug (ou a minima un défaut de Req et HTTPoison). Si d‘autres que moi pensent que c‘est en effet un défaut je logguerai une issue sur Req ou HTTPoison (ou les deux). Closes #3984.

AntoineAugusti · 2024-06-11T14:14:02Z

Étonnant tout ça. Mon curl local semble ne pas apprécier ce header non plus.

content-disposition: attachment; filename=Navette_Saint_Tropez_(horaires_?t?_2024).zip

Quel est le bon comportement ? Est-ce que les headers de réponse HTTP renvoyés sont réellement valides ?

apps/transport/lib/jobs/resource_history_job.ex

ptitfred · 2024-06-11T14:53:20Z

Étonnant tout ça. Mon curl local semble ne pas apprécier ce header non plus.

content-disposition: attachment; filename=Navette_Saint_Tropez_(horaires_?t?_2024).zip

Quel est le bon comportement ? Est-ce que les headers de réponse HTTP renvoyés sont réellement valides ?

Tu me mets le doute. J'avais débugué avec httpie rapidement au début sans observer de défaut de décodage via cet outil, mais pas testé avec curl.

Je vais revérifier avec d'autres clients HTTP.

AntoineAugusti · 2024-06-11T15:34:24Z

J'ai un serveur qui renvoie un ❤️ en valeur de header HTTP et ça passe avec HTTPoison mais pas pour le content-disposition. Étonnant.

iex> HTTPoison.get!("https://pysae.com/api/v2/groups/st-tropez/gtfs/pub").headers
[
  {"Date", "Tue, 11 Jun 2024 15:31:44 GMT"},
  {"Content-Type", "application/zip"},
  {"Transfer-Encoding", "chunked"},
  {"Connection", "keep-alive"},
  {"cache-control", "max-age=10368000"},
  {"content-disposition",
   <<97, 116, 116, 97, 99, 104, 109, 101, 110, 116, 59, 32, 102, 105, 108,
     101, 110, 97, 109, 101, 61, 78, 97, 118, 101, 116, 116, 101, 95, 83, 97,
     105, 110, 116, 95, 84, 114, 111, 112, ...>>},
  {"Strict-Transport-Security", "max-age=15724800; includeSubDomains"}
]
iex> HTTPoison.get!("https://ansart-augusti.fr").headers
[
  {"Server", "nginx"},
  {"Date", "Tue, 11 Jun 2024 15:32:22 GMT"},
  {"Content-Type", "text/html"},
  {"Content-Length", "178"},
  {"Connection", "keep-alive"},
  {"Location", "https://www.ansart-augusti.fr/"},
  {"X-XSS-Protection", "1; mode=block"},
  {"X-Content-Type-Options", "nosniff"},
  {"Referrer-Policy", "no-referrer-when-downgrade"},
  {"Content-Security-Policy",
   "default-src 'self' http: https: data: blob: 'unsafe-inline'; frame-ancestors 'self';"},
  {"Permissions-Policy", "interest-cohort=()"},
  {"x-message", "❤️"}
]

ptitfred · 2024-06-11T15:46:22Z

La version web de httpie me montre le défaut d'encodage. Je me demande si le résultat dans mon terminal n'est pas un artefact (positif) de mon terminal lui-même. A noter que je reproduis ton observation via curl.

Si je devais parier je dirais qu'il y a de l'encodage Windows-1252 dans cette histoire...

ptitfred · 2024-06-11T15:56:18Z

En relisant https://datatracker.ietf.org/doc/html/rfc5987 qui rappelle que par défaut les en-têtes sont encodés en latin1, il est possible de choisir un encodage différent par header selon un syntaxe relativement récente, mais qui n'est pas employée par pysae dans notre cas.

AntoineAugusti · 2024-06-11T16:18:30Z

Il est possible que le nom de fichier soit repris à partir du nom de fichier de l'utilisateur ayant déposé quelque chose sur la plateforme, depuis un poste Windows.

Peux-tu contacter Pysae et partager ce bug ?

ptitfred · 2024-06-11T16:20:37Z

xh m'affiche ceci:

$ xh https://pysae.com/api/v2/groups/st-tropez/gtfs/pub
HTTP/2.0 200 OK
cache-control: max-age=10368000
content-disposition: "attachment; filename=Navette_Saint_Tropez_(horaires_\xe9t\xe9_2024).zip"
content-encoding: gzip
content-type: application/zip
date: Tue, 11 Jun 2024 16:19:06 GMT
strict-transport-security: max-age=15724800; includeSubDomains
vary: Accept-Encoding

+-----------------------------------------+
| NOTE: binary data not shown in terminal |
+-----------------------------------------+

Ce qui confirme que l'encodage est curieux.

Je ne peux plus faire confiance à httpie...

ptitfred · 2024-06-11T16:20:52Z

Il est possible que le nom de fichier soit repris à partir du nom de fichier de l'utilisateur ayant déposé quelque chose sur la plateforme, depuis un poste Windows.

Peux-tu contacter Pysae et partager ce bug ?

je m'en occupe

ptitfred · 2024-06-12T10:07:59Z

Les points suivants me chaffouinent :

firefox respecte le header et se débrouille pour le décode correctement.
je n'ai pas encore mis le doigt sur l'encodage réel utilisé mais pour l'instant (et notamment pour le caractère fautif) je ne trouve aucun encodage qui ne suis pas compatible avec latin1 dans notre cas ; ce qui explique que le patch ci-dessus devrait fonctionner. Pour m'en assurer il me faudrait tester de bout en bout le job, ce que je n'ai pas réussi à faire en local pour l'instant. La table resource_history ne contient pas de nouvelle entrée après exécution du job.
ne devrait-on pas résister à ce scenario et fallback avec un nom par défaut ?

ptitfred · 2024-06-12T11:07:00Z

Pour m'en assurer il me faudrait tester de bout en bout le job, ce que je n'ai pas réussi à faire en local pour l'instant. La table resource_history ne contient pas de nouvelle entrée après exécution du job.

Il me manquait le bucket en local ; c'est corrigé et l'historisation passe avec ce changement.

Je n'explique pas encore pleinement le bien fondé du patch. Je dois creuser dans l'implémentation HTTP sous-jacente pour le justifier.

AntoineAugusti · 2024-06-12T11:35:34Z

@ptitfred

ne devrait-on pas résister à ce scenario et fallback avec un nom par défaut ?

Le bug actuel survient quand on cherche à stocker la payload en JSON, qui contient une allowlist de K/V de headers HTTP et là Jason a du mal à bien encoder la valeur mal encodée.

La survenance du bug montre un problème potentiel pour les réutilisateurs. Idéalement le producteur devrait résoudre le bug.

Par contre difficile de choisir entre fixer le bug (encodage pour tout ce qui passe) ou avoir une meilleure détection d'un tel problème pour gérer. Il est aussi possible que certains producteurs ne corrigent jamais ou très lentement, ce qui bloque une fonctionnalité importante (historisation > métadonnées > validation)

ptitfred · 2024-06-12T12:00:17Z

Mise à jour de ma compréhension du problème:

La présence éventuelle de binary pour certaines valeurs de header pose problème en aval (lors du logging notamment, mais j'imagine aussi pour l'encodage en JSON). En pratique cette PR prend en charge la présence d'un binary en le décodant comme étant du latin1 (ce qui n'est pas 100% rigoureux ; il est possible que certains binary soient encodés différemment).

ptitfred · 2024-06-17T13:49:08Z

Voir elixir-mint/mint#301

thbar

Sujet pas simple 😄

J'ai fini par tomber sur ce chouette article https://www.bortzmeyer.org/6266.html et ouvrir un bug report (postmanlabs/httpbin#724).

Vu que la version actuelle du fix touche 100% des headers sur 100% des historisations, le rapport ennuis sur bénéfices est embêtant (même après correctif je trouve), ça nécessiterait pour être sûr de ne rien casser sur cette partie assez critique des tests sur le volume (ce qui a déjà été fait et peut se faire, mais prend un peu de temps).

Et je me suis demandé aussi quelle est la proportion de serveurs qui vont servir du HTTP2 actuellement aussi, où le cas est géré assez différemment (les headers en UTF-8 ne posent pas de souci si j'ai compris, mais on peut rencontrer d'autres choses).

Toutefois, vu qu'on a qu'un seul cas, et que @etalab/transport-bizdev a des contacts chez Pysae, je préconise qu'on voit avec un biz-dev pour demander une correction chez eux (passer été en ete), et suivre ainsi ce que j'ai lu ici:

https://tools.ietf.org/html/rfc6266#appendix-D

To successfully interoperate with existing and future user agents,
senders of the Content-Disposition header field are advised to:

SNIP

Avoid using non-ASCII characters in the filename parameter.
Although most existing implementations will decode them as
ISO-8859-1, some will apply heuristics to detect UTF-8, and thus
might fail on certain names.

thbar · 2024-06-17T22:16:07Z

apps/transport/lib/jobs/resource_history_job.ex

  end

+  defp decode_header(as_latin1) when is_binary(as_latin1) do


is_binary va retourner true dans tous les cas qui nous intéressent, et comme le dit @AntoineAugusti plus bas, ça va donc matcher tout le temps si je lis bien.

On a en effet:

> is_binary(<<233>>) true > is_binary("é") true

Une String est toujours un binary, mais tous les binaries ne sont pas des String, car (doc):

Strings in Elixir are UTF-8 encoded binaries.

On peut vérifier si un binary est un string avec:

https://hexdocs.pm/elixir/1.17.0/String.html#valid?/2

Là dans le cas présent, ça veut dire que:

la translitération va être appelée sur tous les headers, tout le temps

et si on avait un header en UTF-8 (String) valide avec des accents (ce qui peut arriver en HTTP 2), le code de translitération va causer un souci, en réencodant "é" (UTF-8) en "Ã©" (EDIT: j'ai créé un test-case plus haut)

apps/transport/lib/jobs/resource_history_job.ex

thbar · 2024-06-18T07:29:50Z

Et sinon, je propose un hot-fix beaucoup plus ciblé, si Pysae ne peut pas bouger:

Uniquement sur le header concerné
Uniquement sur les caractères qui nous intéressent, avec:

> :binary.replace("attachment" <> <<102, 105, 108, 101, 110, 97, 109, 101, 61, 233, 46, 122, 105, 112>>, << 233 >>, "é")
"attachmentfilename=é.zip"

(et pareil pour le è).

C'est au scotch, mais beaucoup plus facile à recetter et moins d'impact.

ptitfred · 2024-06-18T09:49:37Z

Je vais me ranger à la position prudente et remplacer les caractères inattendus le temps que ça soit corrigé chez l'AOM. Je les ai contactés par le formulaire en ligne, je vais tenter par le contact direct si ça répond pas.

ptitfred · 2024-06-18T09:55:33Z

Cependant,

Pour l'instant je n'ai lu aucun argument donnant tort à Pysae. Ils seraient en droit de répondre que leur réponse HTTP respecte la spec. Les headers HTTP1.1 sont en latin1, ils renvoient des caractères latin1. A ce titre je continue de penser que mon fix est le bon et que les client HTTP devraient faire ce décodage à notre place systématiquement.

La posture de mint "ça devrait être du ASCII-7 partout" couplée à "HTTP2 est en UTF-8, la lib est récente, on va pas supporter de vieux comportements" me déçoit fortement :/

thbar · 2024-06-18T10:11:08Z

Pour l'instant je n'ai lu aucun argument donnant tort à Pysae. Ils seraient en droit de répondre que leur réponse HTTP respecte la spec. Les headers HTTP1.1 sont en latin1, ils renvoient des caractères latin1.

Ah oui je ne leur donne pas du tout tort non plus, je précise, même si je ne l'avais pas écrit.

thbar · 2024-06-18T10:12:18Z

La posture de mint "ça devrait être du ASCII-7 partout" couplée à "HTTP2 est en UTF-8, la lib est récente, on va pas supporter de vieux comportements" me déçoit fortement :/

Si j'ai compris le fait de ne pas mettre de caractères non ASCII-7 est également une recommandation forte, donc ils y ont été à l'économie, sur un écosystème de petite taille.

Mais je n'ai pas encore d'avis clair là dessus, je creuserai à l'occasion par curiosité !

ptitfred · 2024-06-18T12:33:36Z

Mes derniers changements (replace) ne fonctionnent pas avec le job (contrairement au transcodage avec :unicode).

ptitfred · 2024-06-18T16:05:35Z

Mes derniers changements (replace) ne fonctionnent pas avec le job (contrairement au transcodage avec :unicode).

parce que je n'ai pas fait un "replace all"

avec l'option :global ça passe.

apps/transport/lib/jobs/resource_history_job.ex

AntoineAugusti

LGTM, en espérant qu'on puisse retirer ce fix un jour.

Peut-être tenter de contacter de nouveau Pysae en trouvant un contact tech sur LinkedIn ?

ptitfred · 2024-06-19T09:59:53Z

Peut-être tenter de contacter de nouveau Pysae en trouvant un contact tech sur LinkedIn ?

Je peux tenter le contact de Stéphane comme suggéré par Thibaut.

Plus je regarde ce bug et moins je pense que Pysae est en tort. Ils sont peut-être à contre courant mais difficile de leur reprocher de respecter la RFC et je ne m'attends pas forcément à une réponse positive de leur part.

thbar

Le fix est beaucoup plus clean, GG le fait de vérifier d'abord avec String.valid? pour ne pas corrompre les headers accentués déjà en UTF-8!

Après relecture et petite vérification, je me suis aperçu de plusieurs choses:

on a des coquilles dans nos fixtures: le header content-disposition n'autorise pas attachment, filename=this.zip (avec une virgule), la bonne version est attachment; filename="this.zip" avec un point-virgule (on peut laisser les double-quotes de côté je pense et le serveur Pysae ne le respecte pas, mais par contre il faut ;, voir https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition)
%Req.Response{headers: %{"content-disposition" => ["attachment", "filename=éè.zip"]}} ne correspond pas à la réalité de ce que retourne Req sur un cas correct. Une seule chaîne est retournée (dans le cas ce cet exemple, ça sera "attachment; filename=éè.zip"). Je propose de modifier les fixtures pour que ça colle à la réalité.
la version actuelle du code applique le traitement (chemin de code, avec ou sans transcodage) à tous les @headers_to_keep, je crois que le cas ne se produit que pour content-disposition (vu que ça contient un nom de fichier etc), ou bien il faudrait ne faire ce hotfix que pour le header spécifique, ou accepter un petit risque (probablement mineur à mon avis) d'impact sur des headers non concernés, et le fait que ça consomme un peu + de processing mais ça à cette échelle c'est pas gênant
je remarque dans l'output de curl sur le flux Pysae:

❯ curl -i https://pysae.com/api/v2/groups/st-tropez/gtfs/pub                                              
HTTP/2 200 
# SNIP
content-disposition: attachment; filename=Navette_Saint_Tropez_(horaires_?t?_2024).zip

le retour est marqué comme HTTP/2
le header content-disposition est encodé en latin-1

détail pour l'aspect latin-1

# merci Frédéric
❯ curl --dump-header st-tropez-headers.txt https://pysae.com/api/v2/groups/st-tropez/gtfs/pub -o /dev/null

# sur Mac
❯ file st-tropez-headers.txt 
st-tropez-headers.txt: ISO-8859 text, with CRLF line terminators

# caractères 0xe9 = "é" en ISO-8859-1 = Latin-1
❯ xxd st-tropez-headers.txt | grep horaires
000000b0: 5f28 686f 7261 6972 6573 5fe9 74e9 5f32  _(horaires_.t._2

Je ne sais pas actuellement statuer de façon certaine sur le côté obligatoire ou pas de UTF-8 pour les headers qui ne spécifient pas leur encoding en HTTP/2, mais ça m'intrigue et j'avais lu des choses dans ce sens.

thbar

Ça me paraît nickel, bravo !

(petite suggestion d'amélioration d'un dernier test mais c'est tout)

J'ai fait des petits tests sur de la data, je pense qu'on peut envoyer !

Merci pour le fix et le suivi

apps/transport/lib/jobs/resource_history_job.ex

thbar · 2024-06-21T05:35:44Z

@ptitfred je me suis permis de commit le dernier bout de test direct pour qu'on puisse envoyer.

Je te laisse merger quand tu veux !

ptitfred requested a review from a team as a code owner June 11, 2024 13:58

AntoineAugusti reviewed Jun 11, 2024

View reviewed changes

apps/transport/lib/jobs/resource_history_job.ex Outdated Show resolved Hide resolved

apps/transport/lib/jobs/resource_history_job.ex Outdated Show resolved Hide resolved

ptitfred marked this pull request as draft June 12, 2024 09:52

thbar reviewed Jun 18, 2024

View reviewed changes

ptitfred added 2 commits June 18, 2024 12:43

Merge remote-tracking branch 'origin/master' into fix-3984

b80bd36

Manually replace some latin1 characters

eecdab0

Global :binary.replace

fb8d41b

ptitfred added 2 commits June 18, 2024 18:05

Merge remote-tracking branch 'origin/master' into fix-3984

d694116

http headers: Transcode if invalid

52120ec

ptitfred commented Jun 18, 2024

View reviewed changes

apps/transport/lib/jobs/resource_history_job.ex Outdated Show resolved Hide resolved

ptitfred marked this pull request as ready for review June 19, 2024 08:20

ptitfred added 2 commits June 19, 2024 11:20

More test cases

afb38dc

Only cleanup the content-disposition header

044b406

AntoineAugusti approved these changes Jun 19, 2024

View reviewed changes

thbar reviewed Jun 19, 2024

View reviewed changes

ptitfred added 2 commits June 20, 2024 11:00

Merge remote-tracking branch 'origin/master' into fix-3984

e1773f8

content-disposition: more accurate test cases

14f8f37

ptitfred requested a review from thbar June 20, 2024 09:31

thbar self-assigned this Jun 20, 2024

thbar approved these changes Jun 21, 2024

View reviewed changes

apps/transport/lib/jobs/resource_history_job.ex Outdated Show resolved Hide resolved

thbar added 2 commits June 21, 2024 07:34

Amélioration test

35c28c9

Merge branch 'master' into fix-3984

799aea2

thbar changed the title ~~Historisation des ressources : support du latin1~~ Historisation des ressources : support content-disposition en latin1 Jun 21, 2024

ptitfred added this pull request to the merge queue Jun 24, 2024

Merged via the queue into master with commit 5b87a04 Jun 24, 2024
4 checks passed

ptitfred deleted the fix-3984 branch June 24, 2024 07:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Historisation des ressources : support content-disposition en latin1 #3988

Historisation des ressources : support content-disposition en latin1 #3988

ptitfred commented Jun 11, 2024

AntoineAugusti commented Jun 11, 2024 •

edited

Loading

ptitfred commented Jun 11, 2024

AntoineAugusti commented Jun 11, 2024

ptitfred commented Jun 11, 2024

ptitfred commented Jun 11, 2024

AntoineAugusti commented Jun 11, 2024

ptitfred commented Jun 11, 2024

ptitfred commented Jun 11, 2024

ptitfred commented Jun 12, 2024

ptitfred commented Jun 12, 2024

AntoineAugusti commented Jun 12, 2024

ptitfred commented Jun 12, 2024

ptitfred commented Jun 17, 2024

thbar left a comment

thbar Jun 17, 2024

thbar commented Jun 18, 2024

ptitfred commented Jun 18, 2024

ptitfred commented Jun 18, 2024

thbar commented Jun 18, 2024 •

edited

Loading

thbar commented Jun 18, 2024

ptitfred commented Jun 18, 2024

ptitfred commented Jun 18, 2024

AntoineAugusti left a comment

ptitfred commented Jun 19, 2024

thbar left a comment •

edited

Loading

thbar left a comment

thbar commented Jun 21, 2024

		end

		defp decode_header(as_latin1) when is_binary(as_latin1) do

Historisation des ressources : support content-disposition en latin1 #3988

Historisation des ressources : support content-disposition en latin1 #3988

Conversation

ptitfred commented Jun 11, 2024

AntoineAugusti commented Jun 11, 2024 • edited Loading

ptitfred commented Jun 11, 2024

AntoineAugusti commented Jun 11, 2024

ptitfred commented Jun 11, 2024

ptitfred commented Jun 11, 2024

AntoineAugusti commented Jun 11, 2024

ptitfred commented Jun 11, 2024

ptitfred commented Jun 11, 2024

ptitfred commented Jun 12, 2024

ptitfred commented Jun 12, 2024

AntoineAugusti commented Jun 12, 2024

ptitfred commented Jun 12, 2024

ptitfred commented Jun 17, 2024

thbar left a comment

Choose a reason for hiding this comment

thbar Jun 17, 2024

Choose a reason for hiding this comment

thbar commented Jun 18, 2024

ptitfred commented Jun 18, 2024

ptitfred commented Jun 18, 2024

thbar commented Jun 18, 2024 • edited Loading

thbar commented Jun 18, 2024

ptitfred commented Jun 18, 2024

ptitfred commented Jun 18, 2024

AntoineAugusti left a comment

Choose a reason for hiding this comment

ptitfred commented Jun 19, 2024

thbar left a comment • edited Loading

Choose a reason for hiding this comment

thbar left a comment

Choose a reason for hiding this comment

thbar commented Jun 21, 2024

AntoineAugusti commented Jun 11, 2024 •

edited

Loading

thbar commented Jun 18, 2024 •

edited

Loading

thbar left a comment •

edited

Loading