Skip to content

Ban uniqid() from codebase #57588

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

Closed
derrabus opened this issue Jun 28, 2024 · 4 comments · Fixed by #57856
Closed

Ban uniqid() from codebase #57588

derrabus opened this issue Jun 28, 2024 · 4 comments · Fixed by #57856
Labels
RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@derrabus
Copy link
Member

derrabus commented Jun 28, 2024

Description

Following #57542, I'd like to review our usages of the uniqid() function. It is currently considered for deprecation and removal.

I'm not sure if this the RFC for the deprecation will actually pass, but I agree with the motives behind that proposal. We should stop using that function.

We have quite a few usages in our codebase that can be clustered by the following concerns:

Generate a collision-free temporary file.

return tempnam(sys_get_temp_dir(), uniqid('symfony', true));

The tempnam() function should be good enough for this purpose. And if it's not, maybe we can add a safer replacement to the Filesystem component?

Generate a collision-free random value for tests

It might be even better to work with a constant value in such situations.

Salt for a random string

return \sprintf('__internal_%s', hash('xxh128', uniqid(mt_rand(), true)));

We might as well just use random_bytes() in such situations.

Generate a collision-free identifier

A UUIDv7 would probably be better suited in these cases. That would mean that we would need to pull symfony/uid as a dependency in several packages, but that seems reasonable to me.

Example

No response

@derrabus derrabus added the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Jun 28, 2024
@GromNaN
Copy link
Member

GromNaN commented Jun 28, 2024

Salt for a random string
We might as well just use random_bytes() in such situations.

For predictability and performance, it is better to use a simple sequence of ints. This is what we ended up implementing in the Twig compiler.

@TimWolla
Copy link
Contributor

It is currently considered for deprecation and removal.

Correction: It is considered for “deprecation”. The RFC very intentionally proposes an indefinite deprecation period, with the removal - should that ever be desired - left for a future vote for a major PHP version that is at least 5 years in the future and has a version number of at least 10.

The tempnam() function should be good enough for this purpose. And if it's not, maybe we can add a safer replacement to the Filesystem component?

tempnam() guarantees creating a unique file by means of locking and assistance by the operating system. If you also require the generated filenames to be hard to guess, then PHP 8.4 has you covered. In that case you probably should also ensure you set an appropriate umask() before creating the file.

We might as well just use random_bytes() in such situations.

If the counter, as suggested by Jerome, works for you then I would recommend using a counter.

If you need the values to be unpredictable for a given request only, then you could combine the counter with a secure prefix generated with random_bytes(): Without knowing the secure prefix you don't know the generated identifiers, but the generation of each identifier super cheap.

If for some reason you require each identifier to be unpredictable on its own (e.g. to not leak the amount of identifiers generated within the request), but don't need cryptographically secure randomness (e.g. because the identifiers are regenerated for each request), then you could use Random\Engine\Xoshiro256StarStar + Random\Randomizer::getBytesFromString(). The Xoshiro256** engine is the fastest engine available.


Let me know if I can give any further recommendations or advice for specific situations. Really happy about this issue cleaning up the use of uniqid() independent of the RFC results.

@derrabus
Copy link
Member Author

For predictability and performance, it is better to use a simple sequence of ints. This is what we ended up implementing in the compiler.

In a compiler especially, randomness is even harmful. A compiler should be deterministic because its results should be reproducible. Yes, a simple monotonic sequence is the better strategy in this case. Thank you for this remark.


Correction: It is considered for “deprecation”. The RFC very intentionally proposes an indefinite deprecation period, with the removal - should that ever be desired - left for a future vote for a major PHP version that is at least 5 years in the future and has a version number of at least 10.

Thank you for clarifying this. From my POV, it doesn't really matter when the function is removed. The motivation for the deprecation is absolutely valid and we should not use that function anymore.

Also, thank you for all your input, @TimWolla. It's certainly helpful for everyone working on this ticket.

@TimWolla
Copy link
Contributor

Thank you for clarifying this. From my POV, it doesn't really matter when the function is removed. The motivation for the deprecation is absolutely valid and we should not use that function anymore.

The last time I attempted to deprecate a widely-used function for similar reasons, there were reactions that this would be too large of an impact on the ecosystem, ultimately resulting in the vote being declined and the function not being deprecated.

That's why I find it important to clarify that the RFC proposes to mark the function as deprecated to indicated that it is unsafe and should no longer be used in newly written new / greenfield projects, but not change anything else / not to remove it to give users the time they need to migrate to a better alternative. Not every project is as organized and foresighted as Symfony is.

nicolas-grekas added a commit that referenced this issue Jul 1, 2024
…` calls (derrabus)

This PR was merged into the 7.2 branch.

Discussion
----------

[PsrHttpMessageBridge] Remove `uniqid()` from `tempnam()` calls

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | Part of #57588
| License       | MIT

PHP's `tempnam()` function should already make sure we get a collision-free random temporary file. We should not need additional randomness from `uniqid()`.

Commits
-------

1a05a66 Remove uniqid() from tempnam() calls
nicolas-grekas added a commit that referenced this issue Jul 1, 2024
This PR was merged into the 7.2 branch.

Discussion
----------

Remove useless `uniqid` in tempnam calls in tests

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | Part of #57588
| License       | MIT

For creating a temporary directory, I use `tempnam` then remove the file and create the directory with the same name.

Commits
-------

d5edc93 Remove useless uniqid in tempnam calls
nicolas-grekas added a commit that referenced this issue Jul 1, 2024
…put in case of error (GromNaN)

This PR was merged into the 7.2 branch.

Discussion
----------

[TwigBridge] Remove random file name for `lint:twig` output in case of error

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | Part of #57588
| License       | MIT

In the `lint:twig`, `uniqid` is used to generate a file name when linting a template from STDIN.
- I don't find any reason to use a unique file name. The file name is never stored when the template is tokenized and compiled. The cache is not used.
- This meaningless file name is displayed in the output in case of error, which is better replaced by the text "Standard Input".

Before:
```
$ bin/console lint:twig - < vendor/symfony/twig-bridge/Resources/views/Email/zurb_2/notification/body.html.twig
  ERROR  in sf_6681e2dd64a2e2.02015003 (line -1)
  >> The "inky_to_html" filter is part of the InkyExtension, which is not installed/enabled; try running "composer require twig/inky-extra".
 [WARNING] 0 Twig files have valid syntax and 1 contain errors.
```

After:
```
$ bin/console lint:twig - < vendor/symfony/twig-bridge/Resources/views/Email/zurb_2/notification/body.html.twig
  ERROR  in Standard Input (line -1)
  >> The "inky_to_html" filter is part of the InkyExtension, which is not installed/enabled; try running "composer require twig/inky-extra".
 [WARNING] 0 Twig files have valid syntax and 1 contain errors.
```

Note: the `line -1` is due to the lack of context provided to  `Twig\Extra\TwigExtraBundle\MissingExtensionSuggestor::suggestFilter()`

For reference, this was introduced by #10843

Commits
-------

a97acbd Remove random file name for lint:twig output in case of error
fabpot added a commit that referenced this issue Jul 1, 2024
…ult_domain` expression result (GromNaN)

This PR was merged into the 7.2 branch.

Discussion
----------

[TwigBridge]  Use constant var name to cache `trans_default_domain` expression result

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | Part of #57588
| License       | MIT

When `trans_default_domain` is used with an expression, the result of the expression is cached into a variable and this variable is stored in the `Scope` to be used for each following usage of the `|trans` filter.

This var name doesn't need to be random:
- there is only 1 value in the scope, more than 1 variable at a time is never necessary.
- if `trans_default_domain` is called a second time, the same variable can be reassigned.

The only benefit of using a random var name would be to prevent usage in the template. The name `__internal_trans_default_domain` self-explains that it is not meant to be used.

Commits
-------

ab116bb Use constant var name to cache trans_default_domain expression result
fabpot added a commit that referenced this issue Jul 6, 2024
…ests (xabbuh)

This PR was merged into the 7.2 branch.

Discussion
----------

[Config][Lock][SecurityBundle]  do not use uniqid() in tests

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | part of #57588
| License       | MIT

Commits
-------

1b3d6bf do not use uniqid() in tests
fabpot added a commit that referenced this issue Jul 6, 2024
… uniqid() in tests (xabbuh)

This PR was merged into the 7.2 branch.

Discussion
----------

[Filesystem][Messenger][PsrHttpMessageBridge] do not use uniqid() in tests

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | part of #57588
| License       | MIT

updates the remaining tests that were not covered by #57665

Commits
-------

ee7ffbd do not use uniqid() in tests
nicolas-grekas added a commit that referenced this issue Jul 18, 2024
…bbuh)

This PR was merged into the 7.2 branch.

Discussion
----------

do not use `uniqid()` for generating dev tool tokens

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | part of #57588
| License       | MIT

Commits
-------

5ad7ab9 do not use uniqid() for generating dev tool tokens
xabbuh added a commit that referenced this issue Jul 23, 2024
…temporary file (derrabus)

This PR was merged into the 7.2 branch.

Discussion
----------

[Cache] Use tempnam() instead of uniquid() to produce a temporary file

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  |no
| Deprecations? | no
| Issues        | Part of #57588
| License       | MIT

Commits
-------

3465a2b [Cache] Use tempnam() instead of uniquid()
nicolas-grekas added a commit that referenced this issue Jul 26, 2024
…porary files (xabbuh)

This PR was merged into the 7.2 branch.

Discussion
----------

[DomCrawler][Filesystem] stop using uniqid() to create temporary files

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | part of #57588
| License       | MIT

Commits
-------

ffb21c1 stop using uniqid() to create temporary files
@fabpot fabpot closed this as completed in 7368685 Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants