Skip to content
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

fix: filter except empty #7823

Merged
merged 10 commits into from
Aug 21, 2023
Merged

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Aug 15, 2023

Description
See #7822 (comment)

This PR changes the behavior:

  • ['except' => []] means to exclude nothing.
  • ['except' => ''] means to exclude baseURL only.
    • because the URI string '' means the URI path for the baseURL.
    • when navigating to the baseURL, uri_string() returns ''.

['except' => []] or ['except' => ''] means "except all" in the current code.
This behavior is unexpected, and not good for security.

If a dev comments out all items in except key accidentally, the filter will be disabled.

['except' => [
    // 'foo/*',
    // 'bar/*',
]]

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

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Aug 15, 2023
Copy link
Contributor

@neznaika0 neznaika0 left a comment

Choose a reason for hiding this comment

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

Thanks

@kenjis
Copy link
Member Author

kenjis commented Aug 15, 2023

Strictly speaking, this may be a breaking change. However, it does not appear that any developer would want to disable the filter by specifying an empty array or empty string.

If there is, it would be a case of specifying them by mistake and not realizing that the filter is disabled.

@datamweb
Copy link
Contributor

want to disable the filter by specifying an empty array or empty string

I agree that this should not be the behavior.
But the question is what if for any reason the developer wants to except all 15 minutes altogether?
I think it should be something like the following to "except all".

['except' => [
    '*',
]];
// or
['except' =>  '*' ];

@kenjis
Copy link
Member Author

kenjis commented Aug 15, 2023

@datamweb '*' or ['*'] works.
See https://github.com/codeigniter4/CodeIgniter4/pull/7823/files#diff-ae159768f526f66aeb6af1c17c64b57f36659a0d795530b3014a5ded5cbd6176R221

@neznaika0
Copy link
Contributor

Ha, it's wrong, but it will work. To exclude everyone, you need to remove the filter. There is no logic to enable the filter and disable it by rules

@datamweb
Copy link
Contributor

exclude everyone, you need to remove the filter.

I was considering the implementation with the Settings package.

It looks good to me.

@kenjis kenjis marked this pull request as draft August 16, 2023 01:45
kenjis added 4 commits August 16, 2023 10:46
This behavior is unexpected and not good for security.
If a dev removes all items in `except` key, the filter is disabled.

Now "except empty" means "except nothing".
@kenjis kenjis force-pushed the fix-filter-except branch from 8ea1a4a to b6a3c7d Compare August 16, 2023 01:46
kenjis added 4 commits August 16, 2023 13:21
The URI path '' means the baseURL. So ['except' => ''] should
mean that except for the baseURL only.
@kenjis kenjis force-pushed the fix-filter-except branch from c223a28 to 79aaefb Compare August 16, 2023 04:21
@kenjis
Copy link
Member Author

kenjis commented Aug 16, 2023

The URI string '' means the URI path for the baseURL.
It has the same meaning as /.
So ['except' => ''] should mean to exclude the baseURL only.

I changed that way.

@kenjis kenjis marked this pull request as ready for review August 16, 2023 04:38
@kenjis kenjis force-pushed the fix-filter-except branch from 7564d92 to ef08813 Compare August 16, 2023 04:49
@kenjis kenjis force-pushed the fix-filter-except branch from ef08813 to e510da9 Compare August 16, 2023 05:05
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

I agree this is a good fix for a common "gotcha"

@kenjis kenjis merged commit 700f6e4 into codeigniter4:develop Aug 21, 2023
@kenjis kenjis deleted the fix-filter-except branch August 21, 2023 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants