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

Fix a few more regressions from #4235. #4266

Merged
merged 2 commits into from
Sep 23, 2020
Merged

Conversation

chrisbobbe
Copy link
Contributor

@chrisbobbe chrisbobbe commented Sep 23, 2020

Explanations in the commit messages.

For the second (doubled slashes in Apple auth endpoint), following Greg's comment at #4265 (comment) — chat.zulip.org seems perfectly happy to accept the double-slash version, like it did before cfebe3f 😆. At first I was confused about why it seemed to work both before and after my commit here. Anyway, that's probably why.

Like we did in c53def3 and a change to that file in a commit
shortly after, 865914f.
For zulip#4146, but also to fix a recent regression from 865914f, like
we did in b3af772.
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixes! Merging.

Comment on lines +622 to +630
const rawUpdateEvents = JSON.parse(decodedData);
const updateEvents: WebViewUpdateEvent[] = rawUpdateEvents.map(updateEvent => ({
...updateEvent,
// A URL object doesn't round-trip through JSON; we get the string
// representation. So, "revive" it back into a URL object.
...(updateEvent.auth
? { auth: { ...updateEvent.auth, realm: new URL(updateEvent.auth.realm) } }
: {}),
}));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, kind of ugly, isn't it. :-/ Seems fine for a workaround, and I don't have any immediate ideas for a way to do this (and the similar spot in handleInitialLoad) more cleanly without a fair amount more code; but we'll want to keep an eye on this area whenever adding more complexity to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, my thoughts exactly. 😦

@gnprice gnprice merged commit d56db67 into zulip:master Sep 23, 2020
@gnprice
Copy link
Member

gnprice commented Sep 23, 2020

chat.zulip.org seems perfectly happy to accept the double-slash version, like it did before cfebe3f . At first I was confused about why it seemed to work both before and after my commit here. Anyway, that's probably why.

Hmm! Now that you mention that, I do seem to recall finding that some classes of server API routes tolerated duplicate slashes, while others didn't, just because they went through different pieces of infrastructure code that handled that case differently. The web auth flows certainly use some distinctive pieces of infrastructure code, so I guess it fits that they might be in the group that tolerates such variations in the URL while our main API endpoints aren't.

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.

2 participants