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

errors: add a catch-all handler #12

Merged
merged 3 commits into from
Nov 26, 2024
Merged

Conversation

paulRbr
Copy link
Member

@paulRbr paulRbr commented Nov 25, 2024

The main goal of this PR is in the first commit:

errors: add a catch-all handler to give feedback to the proxy user

This generic error handler helps to give context in case of target
server errors.

Indeed without this commit, the user of the proxy would see a 500 Internal server error. Now, with the current change the real error is
catched and a 502 HTTP error is responded to the user, with some
details in a json object {error: error.message}.

E.g. when targeting a server which has a bad SSL certificate, the user
will now receive this response:

HTTP/1.1 502 Bad Gateway
access-control-allow-origin: *
access-control-allow-methods: OPTIONS
access-control-allow-methods: GET
access-control-allow-methods: POST
access-control-allow-methods: PUT
access-control-allow-methods: PATCH
access-control-allow-methods: DELETE
access-control-allow-headers: Content-Type, Authorization, x-bump-proxy-token, x-requested-with
content-type: application/json
x-content-type-options: nosniff
Content-Length: 127

{"error":"SSL_connect returned=1 errno=0 peeraddr=212.95.74.75:443 state=error: certificate verify failed (hostname mismatch)"}

Other changes

  • Make sure to return correct content-type when JSON errors are returned
  • Refactoring of the test file to clarify the context / stubs

This generic error handler helps to give context in case of target
server errors.

Indeed without this commit, the user of the proxy would see a `500
Internal server error`. Now, with the current change the real error is
catched and a 502 HTTP error is responded to the user, with some
details in a json object `{error: error.message}`.

E.g. when targeting a server which has a bad SSL certificate, the user
will now receive this response:

```
HTTP/1.1 502 Bad Gateway
access-control-allow-origin: *
access-control-allow-methods: OPTIONS
access-control-allow-methods: GET
access-control-allow-methods: POST
access-control-allow-methods: PUT
access-control-allow-methods: PATCH
access-control-allow-methods: DELETE
access-control-allow-headers: Content-Type, Authorization, x-bump-proxy-token, x-requested-with
content-type: application/json
x-content-type-options: nosniff
Content-Length: 127

{"error":"SSL_connect returned=1 errno=0 peeraddr=212.95.74.75:443 state=error: certificate verify failed (hostname mismatch)"}
```
This commits moves all “valid token” requests in the same context. It
also moves the HTTP stubs inside the corresponding test context for
clarity.
@paulRbr paulRbr self-assigned this Nov 25, 2024
Copy link
Contributor

@hack3rvaillant hack3rvaillant left a comment

Choose a reason for hiding this comment

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

Nice. TIL about 502 HTTP error code

@paulRbr
Copy link
Member Author

paulRbr commented Nov 25, 2024

Always good to know all the HTTP status codes! And indeed a 502 here is perfect.

@paulRbr paulRbr merged commit de04a4a into bump-sh:main Nov 26, 2024
1 check passed
@hack3rvaillant
Copy link
Contributor

Well done @paulRbr

@paulRbr paulRbr deleted the proxy-generic-errors branch November 27, 2024 14:24
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.

2 participants