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

[3.4.25][BC Break] 403 instead of 401 for unauthorized users #31136

Closed
JarJak opened this issue Apr 16, 2019 · 11 comments
Closed

[3.4.25][BC Break] 403 instead of 401 for unauthorized users #31136

JarJak opened this issue Apr 16, 2019 · 11 comments

Comments

@JarJak
Copy link

JarJak commented Apr 16, 2019

Symfony version(s) affected: 3.4.25

Description
Symfony now returns 403 instead of 401 for unauthorized users.

How to reproduce

  1. Create controller with a security voter like $this->denyAccessUnlessGranted(...)
  2. Try to access this route with an unauthorized user
  3. Observe different status codes (401 in 3.4.24 vs 403 in 3.4.25)

Possible Solution
Undo this change, or at least provide instructions how to revert to old behavior, or at least mention it explicitly in the changelog / upgrade files.
Even if the change is good (IMO 403 is better, see http://www.dirv.me/blog/2011/07/18/understanding-403-forbidden/index.html) this is a BC Break for apps that react on 401 status but not on 403.

@curry684
Copy link
Contributor

It's most certainly more correct according to protocol.

@JarJak
Copy link
Author

JarJak commented Apr 16, 2019

But it's a BC break and not a security issue.

@curry684
Copy link
Contributor

I agree, I even mentioned it before that it's a BC break because of the behavioral change. I didn't foresee it would cause the other return code though, nor did any of the other reviewers apparently.

I'd say revert it in 3.4 an 4.2 and ship the change with documentation for 4.3.

@nicolas-grekas
Copy link
Member

A BC break wouldn't be more acceptable in 4.3 than in 3.4.25.
And any bug fix in a BC break for someone.
We have to decide where we stand between these two lines.

@JarJak
Copy link
Author

JarJak commented Apr 17, 2019

@nicolas-grekas it depends what you define by bug fix... this one was only about status code change from 401 to 403 and definitions of these codes are soft... it is more an improvement than a bug fix

@fabpot
Copy link
Member

fabpot commented Apr 17, 2019

I think it should have been merged in master instead.

@curry684
Copy link
Contributor

And any bug fix in a BC break for someone.

Yes we decided then to ignore the behavioral break per the "it's a break for everyone" argument, but nobody correctly foresaw the functional break. 403 instead of 401 could cause far greater breakage, ie. in external API clients etc.

@JarJak
Copy link
Author

JarJak commented Apr 17, 2019

@fabpot Can it be reverted in 3.4.26? Because now I won't be able to update a version if i.e. real security issue comes up. For now I've just added 3.4.25 in conflict part of composer.json.

@nicolas-grekas
Copy link
Member

Please send PRs

@JarJak
Copy link
Author

JarJak commented Apr 17, 2019

@nicolas-grekas It's just about reverting fd1408b on 3.4 branch. @curry684 @dimabory do you agree?

@chalasr
Copy link
Member

chalasr commented Apr 17, 2019

See #31142.

fabpot pushed a commit that referenced this issue Apr 17, 2019
…ied rule (dimabory)" (chalasr)

This PR was merged into the 3.4 branch.

Discussion
----------

Revert "bug #30423 [Security] Rework firewall's access denied rule (dimabory)"

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? |no
| Tests pass?   | yes
| Fixed tickets | #31136
| License       | MIT
| Doc PR        | n/a

Commits
-------

cd77f6f Revert "bug #30423 [Security] Rework firewall's access denied rule (dimabory)"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants