Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

WIP: use window.crypto.getRandomValues #2404

Merged
merged 4 commits into from Mar 27, 2017

Conversation

garbados
Copy link
Contributor

This is a WIP solution to #2299

I found this pastebin describing a method of creating UUIDs using window.crypto.getRandomValues and added it to two locations. I haven't added tests because I haven't found testing guidelines (ex: what files detail tests for which environments, server or client) but I would feel irresponsible merging this without them.

@garbados
Copy link
Contributor Author

garbados commented Mar 16, 2017

The pastebin method, though lacking a known author besides "A guest, on December 7, 2016", makes sense to me, and matches the spec as I understand it. Still it irks my paranoia, since we're fundamentally discussing a security issue.

@ianb
Copy link
Contributor

ianb commented Mar 16, 2017

The function in shot.js may be harder to change – that file is used on both the server and client, so window shouldn't be used. OTOH, looking at how that function is used there's no real security concern, and Math.random would be fine for that case. Also I notice while the function in shot.js is distractingly called makeUuid() it actually returns a random string. I think you could just leave that function alone.

The function that does matter in terms of security is in randomString.

For makeUuid.js you could still use the string substitution method (which reads kind of nicely), and just use var r = u[index]; index++

@garbados
Copy link
Contributor Author

Understood. I'll make those changes. Thanks for the feedback!

@garbados
Copy link
Contributor Author

OK, I restored shared/shot.js to match the version on master, and modified makeUuid.js to use getRandomValues with the pre-existing 'xxxxx...'.replace approach. How's it look?

@ianb
Copy link
Contributor

ianb commented Mar 27, 2017

Oops, I saw the "WIP" in the title and didn't notice your updates! Reviewing now...

@ianb ianb merged commit 4d3c7cb into mozilla-services:master Mar 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants