Fix error that invite link doesn't work inside chat #2246
Conversation
… because assert inside then clause?
… into 2240-invite-links-not-working-reliably
Passing run #2794 ↗︎
Details:
Review all test suite changes for PR #2246 ↗︎ |
|||||||||||||||
| @@ -44,7 +44,7 @@ export function notifyAndArchiveProposal ({ state, proposalHash, proposal, contr | |||
| export function buildInvitationUrl (groupId: string, groupName: string, inviteSecret: string, creatorID?: string): string { | |||
| const rootGetters = sbp('state/vuex/getters') | |||
| const creatorUsername = creatorID && rootGetters.usernameFromID(creatorID) | |||
| return `${location.origin}/app/join#?${(new URLSearchParams({ | |||
| return `${location.origin}/app/join?${(new URLSearchParams({ | |||
There was a problem hiding this comment.
@corrideat Need your review on this
There was a problem hiding this comment.
Actually, the hash symbol # was not needed since we saved all the parameters inside query which lies after ?.
There was a problem hiding this comment.
That's wrong, the # is needed, see f8cac66#diff-ab2099fc20649c2e08f92b0ca5b846b5073fc274c7d740b8e1baec7bcd26b92aR98
There was a problem hiding this comment.
(also, if for some reason it works without a #, that's a bug: the query must not contain secrets)
There was a problem hiding this comment.
@Silver-IT This PR needs to be closed while keeping this URL using a #.
The reason we're using a hash here is because the hash prevents the server from learning about what groups the user is joining, and also prevents the invite secret from being shared with the server.
There was a problem hiding this comment.
Why is it bug? I have updated all the related codes, and works fine all time.
The invite link is created, there is a secret inside. Why is it fine to contain secret in hash, and why not in query?
I answered both of these questions in my reply above.
The reason we're using a hash here is because the hash prevents the server from learning about what groups the user is joining, and also >>>> prevents the invite secret from being shared with the server. <<<
If the invite secret is shared with the server then it's not so secret anymore and the server could use it to join the group.
Regarding:
Why # and ? are there all together?
I'm not sure, perhaps it's to make it easier for using with URLSearchParams?
There was a problem hiding this comment.
If the invite secret is shared with the server then it's not so secret anymore and the server could use it to join the group.
So do you mean the first one is good in terms of secret and the second one is bad?
http://localhost:8000/app/join#?groupId=z9brRu3VXVfvXAHFxjuzphoq8mSsPt4Y8fVREeM5g9mPoSVEhFUG&groupName=Dreamers&secret=%5B%22edwards25519sha512batch%22%2Cnull%2C%224nGfJcEZjoVk7MOnPAZlKUBJeQbQthkK2YPAb%2BTXJwxpEIZfXdqJpTuQssNu0MrZUDbGsIrn5bOJdIODk5zRKw%3D%3D%22%5D
http://localhost:8000/app/join?groupId=z9brRu3VXVfvXAHFxjuzphoq8mSsPt4Y8fVREeM5g9mPoSVEhFUG&groupName=Dreamers&secret=%5B%22edwards25519sha512batch%22%2Cnull%2C%224nGfJcEZjoVk7MOnPAZlKUBJeQbQthkK2YPAb%2BTXJwxpEIZfXdqJpTuQssNu0MrZUDbGsIrn5bOJdIODk5zRKw%3D%3D%22%5D
And it's just the format change of invite link. There is no more changes. I can't understand what you mean by secret problem. What is the different on the server side when the user uses these two formats?
There was a problem hiding this comment.
So do you mean the first one is good in terms of secret and the second one is bad?
Yes.
What is the different on the server side when the user uses these two formats?
The first one sends all of that information to the server and the second one doesn't.
There was a problem hiding this comment.
Now, I think I understood what you mean, @taoeffect. So we should use hash instead of query for the security reason.
| @@ -44,7 +44,7 @@ export function notifyAndArchiveProposal ({ state, proposalHash, proposal, contr | |||
| export function buildInvitationUrl (groupId: string, groupName: string, inviteSecret: string, creatorID?: string): string { | |||
| const rootGetters = sbp('state/vuex/getters') | |||
| const creatorUsername = creatorID && rootGetters.usernameFromID(creatorID) | |||
| return `${location.origin}/app/join#?${(new URLSearchParams({ | |||
| return `${location.origin}/app/join?${(new URLSearchParams({ | |||
There was a problem hiding this comment.
@Silver-IT This PR needs to be closed while keeping this URL using a #.
The reason we're using a hash here is because the hash prevents the server from learning about what groups the user is joining, and also prevents the invite secret from being shared with the server.
There was a problem hiding this comment.
Nice! Thanks for the fixing the hash issue!
One issue:
Running "exec:flow" (exec) task
Error ------------------------------ frontend/views/containers/chatroom/chat-mentions/RenderMessageWithMarkdown.js:42:35
Cannot assign `url.hash` to `routerOptions.route['hash']` because property `hash` is missing in object literal [1].
[prop-missing]
frontend/views/containers/chatroom/chat-mentions/RenderMessageWithMarkdown.js:42:35
42| routerOptions.route['hash'] = url.hash
^^^^^^
References:
frontend/views/containers/chatroom/chat-mentions/RenderMessageWithMarkdown.js:40:35
40| routerOptions.route = { path, query }
^^^^^^^^^^^^^^^ [1]
Found 1 error
The Flow check failed!
Summary of Changes