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

keep track of event ID and timestamp of decrypted messages #555

Merged
merged 2 commits into from
Oct 25, 2017

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Oct 12, 2017

This is to avoid false positives when detecting replay attacks.

fixes: element-hq/element-web#3712

Signed-off-by: Hubert Chathi [email protected]

@richvdh PTAL

This is to avoid false positives when detecting replay attacks.

fixes: element-hq/element-web#3712

Signed-off-by: Hubert Chathi <[email protected]>
@uhoreg
Copy link
Member Author

uhoreg commented Oct 12, 2017

suggestions welcome for how to format lines 833 and 834 of src/crypto/OlmDevice.js to avoid the lint error

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Looks great. A few comments below.

const messageIndexKey = senderKey + "|" + sessionId + "|" + res.message_index;
if (messageIndexKey in self._inboundGroupSessionMessageIndexes) {
if (messageIndexKey in self._inboundGroupSessionMessageIndexes
&& (argumentsLength <= 4 // Compatibility for older old versions.
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't worry about this

//
// Keys are strings of form "<senderKey>|<session_id>|<message_index>"
// Values are true.
// Values are objects containing the event ID and timestamp.
Copy link
Member

Choose a reason for hiding this comment

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

could you specify the keys here?

"values are objects of the format {id: <event id>, timestamp: <ts>}"

if (messageIndexKey in self._inboundGroupSessionMessageIndexes
&& (argumentsLength <= 4 // Compatibility for older old versions.
|| self._inboundGroupSessionMessageIndexes[messageIndexKey].id !== eventId
|| self._inboundGroupSessionMessageIndexes[messageIndexKey].timestamp !== timestamp)) {
Copy link
Member

Choose a reason for hiding this comment

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

to fix the lint, pull out a local var as @krombel suggested in riot-dev. tmp is a rubbish name though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know why I called it "suggestion" here 😎

@uhoreg
Copy link
Member Author

uhoreg commented Oct 25, 2017

@richvdh this PR is ready for re-review

@richvdh richvdh self-assigned this Oct 25, 2017
@richvdh
Copy link
Member

richvdh commented Oct 25, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants