-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OPTIONS: ne pas retourner de code redirect (3xx) ou erreur #1359
Comments
Merci pour le ticket déjà bien détaillé ! Vous avez une solution de contournement ou c'est vraiment très bloquant chez vous ? |
Je vois deux contournements :
1/ je ne pense vraiment pas me lancer là-dedans 👹 et 2/ c'est triste 😢 surtout qu'on travaille sur le SEO en ce moment et que les slugs c'est bien mieux à ce niveau là (et pour les usagers au passage). Donc ça dépend de ce que tu appelles très bloquant :-) Bloquant mais pas très ? |
Idem
|
@maudetes on a identifié un autre cas où ça nous bloque (ecolabdata/ecospheres#221) donc je change ma priorisation à très bloquant 👹 😉 🙏 |
A priori le problème vient de la spécification de CORS plutôt que de Nginx ou Flask… Il y a quelques propositions ici sur la page de Firefox https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#preflighted_requests_and_redirects mais la spécification de CORS est en train de changer pour autoriser les redirections de ce que j'en comprends… Peut-être que la solution la plus simple dans ton cas c'est de ne pas envoyer le header |
Précision, la section sur le site de Firefox parle d'une redirection après la requête pre-flight ce qui n'est peut-être pas la même chose qu'une redirection en retour de la requête pre-flight, mais je ne vois vraiment pas commet ça pourrait fonctionner côté Python où on a forcément la redirection… Dans tous les cas, ne pas envoyer le header |
J'ai push une PR opendatateam/udata#3046 qui corrige le problème mais je ne suis pas du tout sûr de la validité de faire ça et si ça posera pas de problème autre part… |
Effectivement ça marcherait de ne pas envoyer le header Authorization mais ce n'est pas trivial car il faudrait indiquer pour chaque appel à l'API si on a besoin de l'Authorization ou non. Discriminer sur le verbe ne suffirait pas (eg Cool pour opendatateam/udata#3046 mais ça ne traite que le cas des redirections, on en aurait aussi besoin pour les 4xx, voire même les 5xx parce qu'on peut vouloir réagir de manière différente dans ces cas là (d'ailleurs dans ce cas, impossible de savoir à l'avance si on a besoin d'être authentifié ou non). Je pense que le plus simple pour vous est de le gérer au niveau Nginx. Moins de code à maintenir et réglé une fois pour toute. Vous pourriez même désactiver NB : quelques issues pas très intéressantes sur Flask-Cors |
@jordanguedj Tu as un avis sur ce problème ? On le fait côté Nginx ? |
Salut, j'ai déployé sur demo une nouvelle version avec les CORS corrigés. Est-ce que vous avez des pages spécifiques qui plantaient sur ecologie.data.gouv.fr que l'on pourrait tester sur demo.ecologie.data.gouv.fr et vérifier que ça fonctionne mieux ? |
Hello ! Plus de page qui plante depuis opendatateam/udata-front-kit#464, on contourne la plupart des problèmes en n'envoyant pas le header d'authorization quand pas besoin. C'est ok en démo sur les redirections
Ok 401 sur
Mais pas sur les autres codes, comme 404 (logique, vu la PR).
Je n'ai pas de endpoint pour tester les 5xx, mais ce serait bien qu'on puisse les gérer proprement dans la SPA et donc récupérer la "vraie" requête. Ce serait intéressant d'avoir un endpoint qui retourne arbitrairement une 500 d'ailleurs. |
Les requêtes preflight doivent contenir un header Mais tant que ça fonctionne pour vous, ça me va. Si vous rencontrez d'autres soucis n'hésitez pas à me les envoyer. (normalement le code fonctionne pour tous les codes d'erreurs 3xx, 4xx, 5xx…) https://demo.data.gouv.fr/api/2/topics/premier-slug/
https://demo.data.gouv.fr/api/1/me/
https://demo.data.gouv.fr/api/2/topics/not-a-topic/
|
Ah ok, c'est opendatateam/udata#3074 que tu as mis sur démo ? Looks nice, je vais faire quelques tests depuis notre SPA et je te dis. |
C'est KO même avec les contournements actuels sur démo. Tu peux tester sur demo.ecologie.data.gouv.fr et essayer de te logguer. Au retour oauth, la requête vers https://demo.data.gouv.fr/oauth/token plante. A priori la requête preflight est bien passée puisque c'est le POST qui pose problème.
Request:
Response:
|
Peut-être que le problème ne vient pas de là mais j'ai activé CORS de façon open uniquement sur les endpoints API, j'avais oublié les endpoints OAuth, je viens de l'activer je redéploie et je reteste et je te dis |
C'est déployé et le flow OAuth fonctionne :-) N'hésite pas à me dire si tu vois d'autres problèmes |
C'est toujours KO sur demo.ecologie.data.gouv.fr pour moi :-/ |
Tu peux essayer en navigation privée pour voir si c'est pas un problème de cache ? |
Oui j'ai fait ça, mais rien n'y fait. Testé sous Chrome et FF. |
Je ne sais pas si tu as changé qqchose mais le login marche maintenant :-) |
Oui en fait j'avais déployé mon fix sur dev à la place de demo… :-( |
Ca marche bien sans les contournements sur notre front (donc en envoyant toujours le header d'Authorization) : 308 et 404 ok 👌 |
Conditions de reproduction :
Authorization
Dans ce cas :
308
👍Authorization
308
à la réponse à la requête OPTIONS, pas seulement au GET ❌Un code de redirect n'est pas acceptable pour une requête preflight (voir Spécifications, étape 7 : ok status). Chrome affiche une erreur
Redirect is not allowed for a preflight request.
.Il faudrait donc retourner une réponse de type
2xx
(probablement204
) pour OPTIONS, même si la requête GET subséquente retournera bien un3xx
. Cf la conf simplifiée ici qui retourne toujours un204
.Ci-dessous un exemple pour https://demo.data.gouv.fr/api/2/topics/plantabilite-des-arbres/ qui redirige vers https://demo.data.gouv.fr/api/2/topics/calque-de-plantabilite/.
Requête :
Réponse :
The text was updated successfully, but these errors were encountered: