search

etalab / transport-site

Rendre disponible, valoriser et améliorer les données transports
https://transport.data.gouv.fr
184 stars 28 forks source link

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

Closed ptitfred closed 1 week ago

ptitfred commented 3 weeks ago

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 commented 3 weeks ago

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

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

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

ptitfred commented 3 weeks ago

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

content-disposition: attachment; filename=Navette_SaintTropez(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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

Les points suivants me chaffouinent :

ptitfred commented 2 weeks ago
  • 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 commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

Voir https://github.com/elixir-mint/mint/issues/301

thbar commented 2 weeks ago

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

> :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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

ptitfred commented 2 weeks ago

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.

ptitfred commented 1 week ago

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 commented 1 week ago

@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 !