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

CommonClient: handle Firefox removing the password part of userinfo #2773

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Berserker66
Copy link
Member

@Berserker66 Berserker66 commented Jan 28, 2024

What is this fixing or adding?

Firefox's mistake
More details in https://discord.com/channels/731205301247803413/1201031114400210984

How was this tested?

by copying a link Firefox imagined into existence.

If this makes graphical changes, please attach screenshots.

@ThePhar
Copy link
Member

ThePhar commented Jan 28, 2024

Opened an appropriate bug report for Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1876952 as well.

ThePhar
ThePhar previously approved these changes Jan 28, 2024
@ThePhar ThePhar added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. affects: webhost Issues/PRs that touch webhost and may need additional validation. affects: core Issues/PRs that touch core and may need additional validation. labels Jan 28, 2024
@ThePhar ThePhar dismissed their stale review January 28, 2024 14:08

Believe we are not following spec based on report from Firefox. Withdrawing approval to review other solutions.

@black-sliver
Copy link
Member

my suggestion would be to do

    if server_url.username:
        ctx.username = server_url.username
        ctx.password = server_url.password or ""

instead to meet the spec used by firefox/browsers.

@Berserker66
Copy link
Member Author

my suggestion would be to do

    if server_url.username:
        ctx.username = server_url.username
        ctx.password = server_url.password or ""

instead to meet the spec used by firefox/browsers.

this will still crash when passed into the websockets library, which specifically checks for this and raises an exception. In the discord thread I suggested raising an issue with said library as another way.

@black-sliver
Copy link
Member

this will still crash when passed into the websockets library

Hm, I don't have a good setup to test this without first implementing archipelago:// on linux, so I'll let you and phar decide, I guess.

@Exempt-Medic Exempt-Medic added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Mar 28, 2024
@ThePhar
Copy link
Member

ThePhar commented Apr 21, 2024

Is this something we still want to handle like this? Unfortunately seems browsers want to follow a different URI standard than Python does.

@Berserker66
Copy link
Member Author

I'd still prefer to do things technically correct, but browsers overwriting html with their own imagination is not something we can do much against.

@black-sliver
Copy link
Member

It looks like Firefox undid their change. I have not checked other browsers. There seem to be a bunch of regressions with other URIs. https://bugzilla.mozilla.org/show_bug.cgi?id=1603699 seems to be the upstream "request" to break it (again). The last 4 posts are kinda interesting. Should we just wait and see where it goes? Should we "fix" python websockets?

@Berserker66
Copy link
Member Author

I guess we'll need to detect Firefox user agent in the long run and serve them their own html, I'm sure they'll love that. :P

@black-sliver
Copy link
Member

I think WHATWG is not Firefox-exclusive, so others may follow if they haven't already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. affects: webhost Issues/PRs that touch webhost and may need additional validation. is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants