Skip to content

Use cryptographically secure random for new document ids #1162

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.

Sign up for GitHub

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

Open
probicheaux wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use cryptographically secure random for new document ids #1162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 6 additions & 2 deletions google/cloud/firestore_v1/base_collection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
import datetime

_AUTO_ID_CHARS = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789"
system_random = random.SystemRandom()


class BaseCollectionReference(Generic[QueryType]):
Expand Down Expand Up @@ -623,8 +624,11 @@ def _auto_id() -> str:
str: A 20 character string composed of digits, uppercase and
lowercase and letters.
"""

return "".join(random.choice(_AUTO_ID_CHARS) for _ in range(20))
try:
return "".join(system_random.choice(_AUTO_ID_CHARS) for _ in range(20))
# Very old Unix systems don't have os.urandom (/dev/urandom), in which case use random.choice
except NotImplementedError:
return "".join(random.choice(_AUTO_ID_CHARS) for _ in range(20))
Comment on lines +627 to +631
Copy link

@gemini-code-assist gemini-code-assist bot Feb 5, 2026

Choose a reason for hiding this comment

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

critical

The existing unit test test__auto_id in tests/unit/v1/test_base_collection.py mocks random.choice. With this change, the test will likely fail as the code now primarily calls system_random.choice. The tests need to be updated to cover both the primary path (using system_random.choice) and the fallback path (using random.choice when NotImplementedError is raised).

Copy link
Author

@probicheaux probicheaux Feb 5, 2026

Choose a reason for hiding this comment

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

Fixed

Comment on lines +627 to +631
Copy link

@gemini-code-assist gemini-code-assist bot Feb 5, 2026

Choose a reason for hiding this comment

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

medium

While the current implementation is correct, it contains duplicated code for generating the random string. To improve maintainability, you could refactor this to select the appropriate random function first, and then use it to generate the string.

    try:
        # Use a cryptographically secure random number generator if available.
        # The .random() method will raise NotImplementedError on systems without os.urandom().
        system_random.random()
        choice_func = system_random.choice
    except NotImplementedError:
        # Fallback to the default pseudo-random generator on very old systems.
        choice_func = random.choice

    return "".join(choice_func(_AUTO_ID_CHARS) for _ in range(20))

Copy link
Author

@probicheaux probicheaux Feb 5, 2026

Choose a reason for hiding this comment

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

this is slower

Copy link

@PranavMin PranavMin Feb 5, 2026

Choose a reason for hiding this comment

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

I agree, this is slower



def _item_to_document_ref(collection_reference, item):
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/v1/test_base_collection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ def test_basecollectionreference_pipeline(mock_query):
assert pipeline == mock_query._build_pipeline.return_value


@mock.patch("random.choice")
@mock.patch("random.SystemRandom.choice")
def test__auto_id(mock_rand_choice):
from google.cloud.firestore_v1.base_collection import _AUTO_ID_CHARS, _auto_id

Expand Down
Loading