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: hide 5xx error message from client #6745

Conversation

liangliang4ward
Copy link
Contributor

Description

hidding 5xx error message from client

Fixes #6699

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

apisix/plugins/authz-casbin.lua Outdated Show resolved Hide resolved
apisix/plugins/authz-keycloak.lua Outdated Show resolved Hide resolved
apisix/plugins/authz-casbin.lua Outdated Show resolved Hide resolved
apisix/plugins/authz-keycloak.lua Outdated Show resolved Hide resolved
apisix/plugins/authz-keycloak.lua Outdated Show resolved Hide resolved
@spacewander spacewander added the wait for update wait for the author's response in this issue/PR label Apr 1, 2022
@leslie-tsang
Copy link
Member

Hello there, I will take over this PR as it lacks of feedback. :)

@leslie-tsang leslie-tsang removed the wait for update wait for the author's response in this issue/PR label Apr 15, 2022
@liangliang4ward
Copy link
Contributor Author

Hello there, I will take over this PR as it lacks of feedback. :)

I'm sorry about that, I'm already wait for reply.

now I see you commit , I learn something.

@leslie-tsang
Copy link
Member

I'm sorry about that, I'm already wait for reply.

now I see you commit , I learn something.

Ohhh, I see, Glad to have you back, do you like to continue ?

@liangliang4ward
Copy link
Contributor Author

I'm sorry about that, I'm already wait for reply.
now I see you commit , I learn something.

Ohhh, I see, Glad to have you back, do you like to continue ?

let me do this continue;
I thought that recording ERROR logs might result in two error logs for the test case. Because it's already been recorded once.

Now that I see your commit, I think logging WARN would be a better way.

@leslie-tsang
Copy link
Member

let me do this continue; I thought that recording ERROR logs might result in two error logs for the test case. Because it's already been recorded once.

Now that I see your commit, I think logging WARN would be a better way.

Ok, please check those review comments carefully and fix them according to the comments. :)

Would you take some time to finish it today ?

@liangliang4ward
Copy link
Contributor Author

let me do this continue; I thought that recording ERROR logs might result in two error logs for the test case. Because it's already been recorded once.
Now that I see your commit, I think logging WARN would be a better way.

Ok, please check those review comments carefully and fix them according to the comments. :)

Would you take some time to finish it today ?

ok

@leslie-tsang
Copy link
Member

Hello there, @liangliang4ward, There are still some comment need to be take care of. I will reopen them for you.

@liangliang4ward
Copy link
Contributor Author

@leslie-tsang hi, ci run failed. but not relate this PR code;

@leslie-tsang
Copy link
Member

@leslie-tsang hi, ci run failed. but not relate this PR code;

I will check it out. Thanks for you contribution.

@leslie-tsang
Copy link
Member

@leslie-tsang hi, ci run failed. but not relate this PR code;

I will check it out. Thanks for you contribution.

CI pass now.

I think we also need to check the error log?

Please add the test case to cover the change.

@leslie-tsang leslie-tsang requested a review from tokers April 18, 2022 00:54
@liangliang4ward
Copy link
Contributor Author

hi, I will close this pr, I find the master already modify some code relate with "authz-keycloak.lua" ,some logic is changed.

@tzssangglass
Copy link
Member

hi @liangliang4ward , if what you think already modify some code relate with "authz-keycloak.lua" is this PR: #6854, I think there is some misunderstanding.

#6854 does not conflict with this PR, so you can go on and reopen this PR.

@liangliang4ward
Copy link
Contributor Author

liangliang4ward commented Apr 18, 2022

some code I don't know how process;
like function "authz_keycloak_get_token_endpoint"
the master new code call this function return :

return 503, {message = err}  

and other code return:

 return 500, "Unable to determine token endpoint."

and refer from #6699
will be: return 503;

@tzssangglass
Copy link
Member

some code I don't know how process; like function "authz_keycloak_get_token_endpoint" the master new code call this function return :

return 503, {message = err}  

and other code return:

 return 500, "Unable to determine token endpoint."

and refer from #6699 will be: return 503;

we can just use 503? cc @spacewander

@spacewander
Copy link
Member

It's recommended to return 503 as people keep thinking the 500 status code is the error of APISIX.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: As a user, I want to hide 5xx error detail from the client, so that the detail is not leaked
7 participants