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

[Security] getTargetPath of TargetPathTrait must return string or null #29408

Merged
merged 1 commit into from Dec 10, 2018

Conversation

Projects
None yet
7 participants
@gmponos
Contributor

gmponos commented Dec 1, 2018

Q A
Branch? 3.4
Bug fix? yes (possible bug)
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

Since the return type is string the default return value must be also string.

@ro0NL

ro0NL approved these changes Dec 1, 2018

little test would be nice :)

@gmponos

This comment has been minimized.

Contributor

gmponos commented Dec 1, 2018

The way tests are written adding a test does not add any value. If tests where using an actual session with a MockSessionStorage then it would have some value.

@chalasr chalasr added the Security label Dec 1, 2018

@chalasr chalasr added this to the 3.4 milestone Dec 1, 2018

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Dec 1, 2018

We can assert the default value is passed here

->with('_security.cool_firewall.target_path')

@gmponos gmponos changed the title from Default value of session target_path must be empty string to [Security] Default value of session target_path must be empty string Dec 1, 2018

@Kronhyx

Kronhyx approved these changes Dec 1, 2018

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 2, 2018

OR we should change the return type to string|null. Wouldn't it make more sense? Technically it would be safer at least (no BC break for sure).

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Dec 2, 2018

for reference, same topic was somewhat discussed here: #23409 (comment)

tend to like the empty string here as well (over null), as it's a valid relative target URI as well. So no hassle with null checks required.

@gmponos

This comment has been minimized.

Contributor

gmponos commented Dec 2, 2018

OR we should change the return type to string|null. Wouldn't it make more sense? Technically it would be safer at least (no BC break for sure).

Personally I have banned empty from my projects for being too loose.. So in this case I will still need to do $value === '' || $value === null

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 2, 2018

I will still need to do $value === '' || $value === null

what the code does is if ($value = ...) - no need for empty.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 2, 2018

and the BC break argument is not a minor one!

@gmponos

This comment has been minimized.

Contributor

gmponos commented Dec 2, 2018

what the code does is if ($value = ...) - no need for empty.

Let me elaborate:

I have a class that I use this trait. If we go with the solution of string|null then I will need to do this:

public function myFunct(){
    ....
    $value = $this->getTargetPath($session, $key);
    if ($value === null || $value === ''){
        return new RedirectResponse('myhomeurl');
    }
    
    return new RedirectResponse($value);
}

if we go just with the string then the if statement above only needs to check about $value === ''

and the BC break argument is not a minor one!

Never said it is minor.. except if you were not referring to me.. But since you mention it the BC it depends I believe in the way that you look at this.

If someone was depending on the docbloc then it is not a BC.. That is what the code should do and it is just a bug fix. If someone was depending on the actual implementation and he was actually checking about null (like the code I have above) then you could say that it is a BC.

My point of view tends towards to the direction that it is a bug. The docblock says string. If someone was depending on null he should commit PR and never the less he should check against emtpy string as well.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 2, 2018

The docblock is just a hint - it doesn't enforce anything.
If anyone does if (null !== $value = ...), their code will just break if we do this change.
That's what I mean here by BC break.

@gmponos

This comment has been minimized.

Contributor

gmponos commented Dec 2, 2018

Yea I know what you meant.. so let's not keep going in loops.. You said your PoV and I said mine.. In this specific case I believe there is no right or wrong so someone has to take a decision.

Wouldn't it make more sense?

Judging by the above I feel that you are not certain either... and also the fact that @ro0NL posted an accepted PR doing otherwise. So I will need you (@nicolas-grekas) to give me the final direction. 😄

Also I need to know what about tests.. If this is limited to a docblock change then no test changes is required since there is no way to actually reproduce this in the way they are written. Too much mocking that it will just make it silly. I would be happy to refactor tests as well on this PR using an actual Session with a Mock storage if you wish.

If we stay with the change as is in my thinking tests should be all about giving this input and expecting that output.. So changing with methods is not one of my favorite things to do. Same as above.. I can proceed with refactoring test that would give more meaning.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 2, 2018

My vote is for changing the type hint.
I'd suggest to update the PR accordingly and give some time to others to chime in.

@gmponos gmponos changed the title from [Security] Default value of session target_path must be empty string to [Security] getTargetPath of TargetPathTrait must return string or null Dec 2, 2018

@fabpot

fabpot approved these changes Dec 10, 2018

@fabpot fabpot force-pushed the gmponos:patch-2 branch from a5079a5 to 8d4b787 Dec 10, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Dec 10, 2018

Thank you @gmponos.

@fabpot fabpot merged commit 8d4b787 into symfony:3.4 Dec 10, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Dec 10, 2018

minor #29408 [Security] getTargetPath of TargetPathTrait must return …
…string or null (gmponos)

This PR was squashed before being merged into the 3.4 branch (closes #29408).

Discussion
----------

[Security] getTargetPath of TargetPathTrait must return string or null

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes (possible bug)
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT

Since the return type is string the default return value must be also string.

Commits
-------

8d4b787 [Security] getTargetPath of TargetPathTrait must return string or null
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment