Skip to content

fix: remove service dependency from sanitize_filename() helper function #9560

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

Jump to bottom
Merged
merged 2 commits into from
May 18, 2025
Merged

fix: remove service dependency from sanitize_filename() helper function #9560

merged 2 commits into from
May 18, 2025

Conversation

michalsn
Copy link
Member

Description
This PR fixes a bug where the sanitize_filename() helper function could not be used in CLI requests.

The sanitize_filename() function in the Security helper now contains the logic that was previously in the Security::sanitizeFilename() method. The method has been updated to delegate to the helper function, reversing the previous dependency. This change allows the function to be used in CLI requests without relying on the service.

Fixes: #9555

Checklist:

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

@ddevsr ddevsr added the bug Verified issues on the current code behavior or pull requests that will fix them label May 15, 2025
paulbalandan
paulbalandan reviewed May 15, 2025
View reviewed changes
Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

Should the method be deprecated as it is just an exact copy?

@michalsn
Copy link
Member Author

I was thinking about this because if you look at the methods that are available and the user guide itself, IMO, this method is added a bit by force.

On the other hand, it is listed in SecurityInterface, so I am not sure if we should deprecate it.

@paulbalandan
Copy link
Member

In my opinion, if the Security component is meant for HTTP requests and sanitizeFilename() can be used for both HTTP and CLI requests (as its logic has no specific dependence on HTTP requests), I think its presence as an API method is a misuse. Maybe it was added before as a utility method. I'm in favor of deprecating it both in the class and interface. But, let's hear what others think of this.

@michalsn
Copy link
Member Author

With no additional feedback, I've marked sanitizeFilename as deprecated in both the class and the interface.

@michalsn michalsn merged commit 153c738 into codeigniter4:develop May 18, 2025
55 of 59 checks passed
@michalsn
Copy link
Member Author

Thank you @samsonasik and @paulbalandan

@michalsn michalsn deleted the fix-security branch May 22, 2025 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samsonasik samsonasik samsonasik approved these changes

@paulbalandan paulbalandan paulbalandan approved these changes

Assignees
No one assigned
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: Cannot call Security Service from Command Line
4 participants
@michalsn @paulbalandan @samsonasik @ddevsr