Block invalid poptoken #1660
Block invalid poptoken #1660
Conversation
dayan9265
added
fe1-web
prod-ready
There was a problem hiding this comment.
At some time in the past we had something like that already ..
It might be nice to put this validation into the constructor of ScannablePopToken?
| if (!popToken.match(/^[a-zA-Z0-9_-]+={0,3}$/) || popToken.length % 4 !== 0) { | ||
| throw new Error('Invalid popToken format'); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think it is better if this is moved to at least another function or the ScannablePopToken.fromJson which is used line 144
|
It was indeed a better idea to put the verification inside ScannablePopToken, so I moved it. (I let the tests inside the scanner, since we want to make sure that wrong tokens are refused) |
|
Okay it seems that there's now a risk when calling the ScannablePopToken generator with empty strings (such as in the RollCallOpen), I saw there is an another place where it could be the case: @Tyratox since this is code that you had written, do you know if it can cause an issue? |
|
Hmm true. While not ideal, this is the reality and I think the best solution for now would be to add a check like const serializedPopToken = useMemo(() => {
try {
return ScannablePopToken.encodePopToken({ pop_token: popToken })
} catch {
return null;
}
}, [popToken]);
if serializedPopToken == null {
return <SomeNiceErrorMessage/>;
}While |
| popToken !== '' | ||
| ? ScannablePopToken.encodePopToken({ pop_token: popToken }) | ||
| : undefined |
There was a problem hiding this comment.
although I don't see how, this technically could throw.
Would it make sense to do this operation beforehand and wrap it like you did in digital cash ?
There was a problem hiding this comment.
this could throw since the popToken is initially empty, and it is changed through a promise (generateToken), so for a brief moment the popToken will be an empty string
I will wrap it earlier 👍
| const serializedPopToken = useMemo(() => { | ||
| try { | ||
| return ScannablePopToken.encodePopToken({ pop_token: popToken }); | ||
| } catch { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Why don't you return an empty string there directly then (in catch)?
It seems that you return null to convert it in an empty string after if it's null.
There was a problem hiding this comment.
The idea was that if the popToken is null, it displays nothing (the return null after the toast show). In reality the || '' condition will never be used (I have removed it now)
|
[PoP - Be1-Go] Kudos, SonarCloud Quality Gate passed!
|
|
[PoP - Be2-Scala] Kudos, SonarCloud Quality Gate passed!
|
|
[PoP - Fe2-Android] Kudos, SonarCloud Quality Gate passed!
|
|
[PoP - Fe1-Web] SonarCloud Quality Gate failed. |
This PR blocks invalid pop token when scanning (this avoid bugs with wrong channel created, and then when trying to connect to these channel it fails, which could lead to the app thinking it is offline)
PS: the updated snap test is due to the current state of the release not being correct