Skip to content
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

Forward query string when redirecting from /git-pull/ endpoint #303

Closed
wants to merge 2 commits into from

Conversation

frankier
Copy link

Fixes: #290

I've been having trouble with the redirect on BinderHub when 3rd party cookies are disabled. This fowards the token query string argument which seems to fix the issue.

@welcome
Copy link

welcome bot commented Apr 12, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@consideRatio
Copy link
Member

consideRatio commented Apr 12, 2023

I'd like to review and help this PR, but I'm not confident on the situation you look to resolve based on the PR description.

Can you describe example links where nbgitpullers behavior will changed with this PR?

@frankier
Copy link
Author

In Chrome/Chromium you should encounter this on BinderHub, when you disable 3rd party cookies. Do you happen to have a spare BinderHub nbgitpuller link handy?

2023-04-12T12-33-04

@frankier frankier marked this pull request as draft April 13, 2023 06:14
@manics
Copy link
Member

manics commented Apr 13, 2023

Would you say this is primarily a mybinder.org issue that could affect other applications/extensions?

Can you open your browser debug/log console, and show us all the network requests and logs for a failing redirect?

@frankier
Copy link
Author

I'm having some trouble generating working example links here because the link generator is still broken for me.

For me, the link generator is still not working. See also: #297

@frankier
Copy link
Author

frankier commented Apr 13, 2023

As requested...

2023-04-13T13-48-57

The above links are finally fixed now. It might be enough to use an incognito window for testing. For me, one works and one does not.

P.S.: Click them while they're warm(!) ;)

P.P.S: I think this might affect other BinderHub instances? I'm not sure about other cases.

@frankier
Copy link
Author

Hi! Were either of you able to replicate from my description?

@frankier frankier marked this pull request as ready for review May 9, 2023 10:47
@frankier
Copy link
Author

I've resolved conflicts now. Please let me know if anything is unclear with this PR.

@manics
Copy link
Member

manics commented Aug 26, 2024

@frankier frankier closed this Aug 26, 2024
@frankier
Copy link
Author

Well I'm not working on this anymore so I can't say exactly what went wrong. No problem if it works for you and you don't want to pass the token query string through redirects along and rely on the cookie. Maybe someone will be able to post a replicable bug later on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parts of Repo are forbidden
3 participants