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: prevent fetchr to throw a 500 whenever a resource does not implement an operation #546

Merged
merged 3 commits into from
Nov 22, 2024

Conversation

pablopalacios
Copy link
Contributor

@pablopalacios pablopalacios commented Nov 18, 2024

We are getting some alerts on our systems whenever some bot our security scanner try to hit a fetchr resource using an operation that is not registered.

The most common case is when a resource only implements the create method (which requires a POST request). The bots try to hit this resource with a GET request, which makes fetchr middleware to think that this is a a read operation. Since the resource does not implement it, fetchr throws an error. Before this change this error wouldn't include a status code which would default to a 500. But since this is a client error and we are talking about not implemented methods, I thought that a 405 would be more appropriate.

I really thought that this would be a trivial change (I actually tried it a long time ago, but I gave up and just decided to handle it in our services directly). The main issue is that testCrud is used for both server and client tests. Even though the interface is the same, the errors are different: in the client, fetchr is mainly handling an http error and FetchrError.message includes the whole response body. But in the server (since there is no response yet), FetchrError.message is just the error message itself. This is why you also see some refactoring here for testCrud so we can do this differentiation in the test.

The other issue that I found while solving this one is that we don't have a consistent way of throwing errors in the server. Sometimes it's handled in the middleware before triggering a fetchr request. In this case, we call express next function with a HttpError generated by fumble. But sometimes, we let the request go through fetchr and, if any error happens, we throw an FetchrError and handle the response within fetchr middleware. This behavior is quite visible now on the functional test where in the first case we get as response { "message": "error" } and in the second case a { "output": { "message": "error" }, "meta": {} }.
This is not an issue to fetchr client since it can handle all kind of errors consistently. But it is an issue if one wants to consistently monitor/track errors on the server: the first case of errors must be handled in a custom express error middleware (as I added in the functional tests) and the second case must be handled within a fetchr statsCollector function.

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

@pablopalacios
Copy link
Contributor Author

Alt Text

@redonkulus

Copy link
Contributor

@redonkulus redonkulus left a comment

Choose a reason for hiding this comment

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

Changes makes sense to me, thanks!

@redonkulus redonkulus merged commit 4024cab into yahoo:main Nov 22, 2024
2 checks passed
@redonkulus
Copy link
Contributor

Released in https://github.com/yahoo/fetchr/releases/tag/v0.7.14

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