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

Block invalid poptoken #1660

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions fe1-web/src/core/objects/ScannablePopToken.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ export class ScannablePopToken {
throw new ProtocolError("undefined 'pop_token' when creating 'ScannedPopToken'");
}

const tokenMatcher = new RegExp('^[A-Za-z0-9_-]{43}=$');
if (!tokenMatcher.test(scannedPopToken.pop_token)) {
throw new ProtocolError('Invalid pop token format');
}

this.pop_token = scannedPopToken.pop_token;
}

Expand Down
33 changes: 33 additions & 0 deletions fe1-web/src/core/objects/__tests__/ScannablePopToken.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { mockPopToken } from '__tests__/utils';
import { OmitMethods } from 'core/types';

import { ProtocolError } from '../ProtocolError';
import { ScannablePopToken } from '../ScannablePopToken';

describe('ScannablePopToken object', () => {
Expand All @@ -22,6 +23,38 @@ describe('ScannablePopToken object', () => {
).toThrow(Error);
});

it('throws if the pop_token is not in a valid format', () => {
const validPopTokens = [
'QRf_GkuQI6-TviY6ZmW3sMZQzDYc66SWqcgfgKyiY00=',
'-r4fYiL7-uAnt4WIQ2-XopjzwCVBKyTECNr420juktI=',
'abcdefghijklwxyzABNOPQRSTUVWXYZ0123456789-_=',
];

const invalidPopTokens = [
'', // empty string
'ockPublicKey2_fFcHDaVHcCcY8IBfHE7auXJ7h4ms=', // size not multiple of 44
'mockP!blicKey2_fFcHDaVHcCcY8IBfHE7auXJ7h4ms=', // invalid character
];

validPopTokens.forEach((popToken) => {
expect(
() =>
new ScannablePopToken({
pop_token: popToken,
}),
).not.toThrow(Error);
});

invalidPopTokens.forEach((popToken) => {
expect(
() =>
new ScannablePopToken({
pop_token: popToken,
}),
).toThrow(ProtocolError);
});
});

it('can encode in json', () => {
const token = new ScannablePopToken({
pop_token: mockPopToken.publicKey.toString(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
TouchableOpacity,
TouchableWithoutFeedback,
} from 'react-native-gesture-handler';
import { useToast } from 'react-native-toast-notifications';
import { useSelector } from 'react-redux';

import {
Expand All @@ -26,6 +27,7 @@ import { DigitalCashParamList } from 'core/navigation/typing/DigitalCashParamLis
import { Hash, PublicKey } from 'core/objects';
import { ScannablePopToken } from 'core/objects/ScannablePopToken';
import { Color, Icon, ModalStyles, Spacing, Typography } from 'core/styles';
import { FOUR_SECONDS } from 'resources/const';
import STRINGS from 'resources/strings';

import { DigitalCashHooks } from '../../hooks';
Expand Down Expand Up @@ -290,6 +292,8 @@ export const SendReceiveHeaderRight = () => {
const route = useRoute<NavigationProps['route']>();
const laoId = DigitalCashHooks.useCurrentLaoId();

const toast = useToast();

const { rollCallId: serializedRollCallId, isCoinbase } = route.params;
const rollCallId = useMemo(
() => (serializedRollCallId ? new Hash(serializedRollCallId) : undefined),
Expand All @@ -300,6 +304,22 @@ export const SendReceiveHeaderRight = () => {

const popToken = useMemo(() => rollCallToken?.token.publicKey.valueOf() || '', [rollCallToken]);

const serializedPopToken = useMemo(() => {
try {
return ScannablePopToken.encodePopToken({ pop_token: popToken });
} catch {
return null;
}
Comment on lines +307 to +312
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

}, [popToken]);

if (serializedPopToken === null) {
toast.show(STRINGS.digital_cash_error_rollcall_not_defined, {
type: 'warning',
placement: 'bottom',
duration: FOUR_SECONDS,
});
return null;
}
if (isCoinbase) {
return null;
}
Expand Down Expand Up @@ -329,7 +349,7 @@ export const SendReceiveHeaderRight = () => {

<View>
<QRCode
value={ScannablePopToken.encodePopToken({ pop_token: popToken })}
value={serializedPopToken}
overlayText={STRINGS.digital_cash_wallet_qrcode_text}
/>
</View>
Expand Down
13 changes: 9 additions & 4 deletions fe1-web/src/features/rollCall/components/RollCallOpen.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,14 @@ const RollCallOpen = ({
.catch((err) => console.error(`Could not generate token: ${err}`));
}, [hasWalletBeenInitialized, generateToken, laoId, rollCall]);

const serializedPopToken = useMemo(() => {
try {
return ScannablePopToken.encodePopToken({ pop_token: popToken });
} catch {
return undefined;
}
}, [popToken]);

return (
<ScreenWrapper toolbarItems={toolbarItems}>
<Text style={Typography.paragraph}>
Expand Down Expand Up @@ -181,10 +189,7 @@ const RollCallOpen = ({
<>
<Text style={Typography.paragraph}>{STRINGS.roll_call_open_attendee}</Text>
<View>
<QRCode
value={ScannablePopToken.encodePopToken({ pop_token: popToken })}
overlayText={STRINGS.roll_call_qrcode_text}
/>
<QRCode value={serializedPopToken} overlayText={STRINGS.roll_call_qrcode_text} />
<Text
style={[
Typography.paragraph,
Expand Down
Copy link
Contributor

Choose a reason for hiding this comment

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

At some time in the past we had something like that already ..

const tokenMatcher = new RegExp('^[A-Za-z0-9_-]{43}=$');

It might be nice to put this validation into the constructor of ScannablePopToken?

Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,6 @@ const RollCallOpened = () => {
(popToken: string) => {
// if the token is already part of attendeePopTokens, do not trigger a state update
// and return false indicating the pop token was not added since it's a duplicate

if (attendeePopTokens.current.includes(popToken)) {
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,8 @@ describe('RollCallOpened', () => {
it('shows toast when scanning attendees', async () => {
renderRollCallOpened();

fakeQrReaderScan(mockPublicKey2.valueOf());
fakeQrReaderScan(mockPublicKey3.valueOf());
fakeQrReaderScan(ScannablePopToken.encodePopToken({ pop_token: mockPublicKey2.valueOf() }));
fakeQrReaderScan(ScannablePopToken.encodePopToken({ pop_token: mockPublicKey2.valueOf() }));

expect(mockToastShow).toHaveBeenCalledTimes(2);
});
Expand Down Expand Up @@ -241,4 +241,48 @@ describe('RollCallOpened', () => {
// counter should be at 2
expect(toJSON()).toMatchSnapshot();
});

it('accepts only valid pop tokens', async () => {
const validPopTokens = [
mockPublicKey2.valueOf(),
mockPublicKey3.valueOf(),
'abcdefghijklwxyzABNOPQRSTUVWXYZ0123456789-_=',
];

const invalidPopTokens = [
'', // empty string
'ockPublicKey2_fFcHDaVHcCcY8IBfHE7auXJ7h4ms=', // size not multiple of 4
'mockP!blicKey2_fFcHDaVHcCcY8IBfHE7auXJ7h4ms=', // invalid character
];

renderRollCallOpened();

for (const validPopToken of validPopTokens) {
fakeQrReaderScan(ScannablePopToken.encodePopToken({ pop_token: validPopToken }));
expect(mockToastShow).toHaveBeenCalledWith(
expect.anything(),
expect.objectContaining({ type: 'success' }),
);
mockToastShow.mockReset();
}

for (const invalidPopToken of invalidPopTokens) {
fakeQrReaderScan(JSON.stringify({ pop_token: invalidPopToken }));
expect(mockToastShow).toHaveBeenCalledWith(
expect.anything(),
expect.objectContaining({ type: 'danger' }),
);
mockToastShow.mockReset();
}

// should have correct json format
for (const validPopToken of validPopTokens) {
fakeQrReaderScan(validPopToken);
expect(mockToastShow).toHaveBeenCalledWith(
expect.anything(),
expect.objectContaining({ type: 'danger' }),
);
mockToastShow.mockReset();
}
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ exports[`EventRollCall render correctly no duplicate attendees opened roll calls
"width": "100%",
}
}
value="{"pop_token":""}"
value=""
viewBox="0 0 256 256"
/>
<View
Expand Down Expand Up @@ -1490,7 +1490,7 @@ exports[`EventRollCall render correctly no duplicate attendees re-opened roll ca
"width": "100%",
}
}
value="{"pop_token":""}"
value=""
viewBox="0 0 256 256"
/>
<View
Expand Down Expand Up @@ -3846,7 +3846,7 @@ exports[`EventRollCall render correctly non organizers opened roll calls 1`] = `
"width": "100%",
}
}
value="{"pop_token":""}"
value=""
viewBox="0 0 256 256"
/>
<View
Expand Down Expand Up @@ -4889,7 +4889,7 @@ exports[`EventRollCall render correctly non organizers re-opened roll calls 1`]
"width": "100%",
}
}
value="{"pop_token":""}"
value=""
viewBox="0 0 256 256"
/>
<View
Expand Down Expand Up @@ -8845,7 +8845,6 @@ exports[`EventRollCall render correctly organizers re-opened roll calls 1`] = `
}
}
testID="roll_call_close_button"

>
<View
style={
Expand Down
2 changes: 2 additions & 0 deletions fe1-web/src/resources/strings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,8 @@ namespace STRINGS {
'Issue to every attendee of this roll call';

export const digital_cash_infinity = '∞';
export const digital_cash_error_rollcall_not_defined =
'RollcallId not defined, cannot generate PoP token';

/* --- Witness Feature Strings --- */
export const witness_message_witness = 'Witness Message';
Expand Down