Skip to content
This repository has been archived by the owner on Feb 24, 2024. It is now read-only.

safer error handling #2322

Merged
merged 4 commits into from
Sep 21, 2022
Merged

safer error handling #2322

merged 4 commits into from
Sep 21, 2022

Conversation

briskt
Copy link
Contributor

@briskt briskt commented Sep 16, 2022

The defaultErrorHandler presently compares the environment against "production" to determine whether it's safe to dump debugging details. When running in staging (or qa, etc.), it is preferable to not have debugging detail since it could be hosted on a public server. This PR reverses the logic so the detailed error message is only used when the environment is "development".

@briskt briskt requested a review from a team as a code owner September 16, 2022 09:39
@sio4
Copy link
Member

sio4 commented Sep 16, 2022

Hi @schparky,

Currently, the main branch is not configured properly. I am working on the cleanup of v1 release and the main will be used for the next version. Did you make the change based on the main branch? Please use v1 branch.

@sio4 sio4 changed the base branch from main to v1 September 16, 2022 09:47
@sio4
Copy link
Member

sio4 commented Sep 16, 2022

Ah, it looks like you started from the correct branch. I changed the target.

Still failing. I will take a look at this issue.

@sio4 sio4 self-assigned this Sep 16, 2022
@sio4 sio4 added the s: triage Some tests need to be run to confirm the issue label Sep 16, 2022
@sio4 sio4 changed the base branch from v1 to main September 16, 2022 10:58
@sio4 sio4 changed the base branch from main to v1 September 16, 2022 10:58
@sio4 sio4 changed the base branch from v1 to update-workflow September 16, 2022 11:06
@sio4
Copy link
Member

sio4 commented Sep 16, 2022

I tested two things.

  1. filed my own PR against v1 branch --> the tests worked fine
  2. retry the failed test for this PR after changing merge target --> still failing

The symptom: actions/checkout@v3 keep trying to merge this PR against the last commit of main.

  HEAD is now at 7cc1cdc Merge 13d320b2917516df9d7a8674c9f9b7975c75a0c6 into 89ed84d0d3bf2db8474659131231a0f5117eba27

89ed84d is the latest commit in the main branch.

I tested after delete the last commit 89ed84d from the main branch to see if the action noticed it and merged the PR into the last commit of the main branch, but the action still keep trying to merge into the deleted sha.

@sio4 sio4 closed this Sep 16, 2022
@sio4 sio4 reopened this Sep 16, 2022
@sio4 sio4 changed the base branch from update-workflow to v1 September 16, 2022 11:40
@sio4 sio4 closed this Sep 16, 2022
@sio4 sio4 reopened this Sep 16, 2022
@sio4
Copy link
Member

sio4 commented Sep 21, 2022

This issue was originally discussed in the Slack channel and I think it could be fine. Even though it changes the behavior of error handling for each environment, the direction of this change is narrowing down the scope of a safe place, it could be better.

  • The previous behavior: shows the simple error page for production but shows debugging pages for all others
  • The changed behavior: shows the simple error page for all other environments, but only shows debugging page when the environment is exactly set as development

This is better from the security point of view and allows users have other custom environment names for their various production-ish environments.

I basically agree that those debugging messages SHOULD only be allowed for a specific narrow environment so != development looks better than == production to me. However, regardless the other values are mentioned in the doc or not, behavior change should be handled carefully so I just ask you to consider them if you are fine. 

[1] https://gophers.slack.com/archives/C3MSAFD40/p1663318963883059

Copy link
Member

@sio4 sio4 left a comment

Choose a reason for hiding this comment

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

LGTM

@sio4 sio4 added enhancement New feature or request and removed s: triage Some tests need to be run to confirm the issue labels Sep 21, 2022
@sio4 sio4 merged commit 5a97f9c into gobuffalo:v1 Sep 21, 2022
@briskt briskt deleted the feature/safer-error-handling branch September 21, 2022 12:45
@briskt
Copy link
Contributor Author

briskt commented Sep 21, 2022

Thank you, @sio4

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants