[banghamid]

Description
In system/HTTP/Files/UploadedFile.php, new destination directories were created with 0777 permissions in setPath(), and moved files were given 0666 & ~umask() permissions in move().

Granting write permissions to group and other users violates the principle of least privilege (CWE-732: Incorrect Permission Assignment for Critical Resource) and poses a security risk on environments with permissive umasks or shared systems.

This pull request:

1. Hardens newly created upload directory permissions from 0777 to 0755.
2. Hardens moved uploaded file permissions from 0666 & ~umask() to 0644 & ~umask().
3. Ensures fclose() is only called when fopen() returns a valid resource for the directory index.html marker.
4. Updates permission assertion in tests/system/HTTP/Files/FileMovingTest.php.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value (without duplication)
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[michalsn]

I would keep the current permission handling. Both operations already respect the process umask: with 0022, they produce 0755 directories and 0644 files. Under that mask, the updated test also passes without the change.

With 0002, group write access may be intentional, allowing PHP and background workers to share files. Removing it can break valid deployments - group write access alone does not establish a CWE-732 vulnerability.

This hardening does not justify a compatibility break in a patch release and would introduce an inconsistency with File::move(), which still uses 0666 & ~umask().

The fclose() guard can be considered separately, though our default error handler settings already throws an ErrorException when fopen() fails.

[banghamid]

Makes sense, I overlooked the 0002 umask scenario where group write is actually intended (like shared workers). Also didn't realize File::move() still uses 0666 & ~umask().

Given that and the default error handler already throwing on fopen() failure, I think it's best to just close this PR. Thanks @michalsn for taking a look!

[banghamid]

Good point on the 0002 umask case, didn't consider setups sharing files across PHP/workers via group write. Keeping it aligned with File::move() makes sense too.

Would you still want a small PR with just the is_resource($file) check before fclose() (in case someone runs with a custom error handler), or better to just close this one?

[paulbalandan]

Feel free to open a new PR with the resource check as a refactor PR, though I would not use is_resource since it can still return false.

[michalsn]

I think we're fine for now. If someone runs into this issue in practice, we can revisit it.