DEV Community

Cover image for I made a mistake implementing a React Hook and got a denial of service from my backend
DrKnoxy
DrKnoxy

Posted on

I made a mistake implementing a React Hook and got a denial of service from my backend

This seemingly simple usage of React's useEffect hook on a Firebase endpoint accidentally ran through my 50k reads/day quota in minutes.

function Page() {
  const [meetings, setMeetings] = useState([]);
  useEffect(() => {
    return firebase.firestore().collection('/meetings').onSnapshot(query => {
      setMeeting( query.docs.map(m => m.data()) );
    });
  });

  return (
    <ul>
      {meetings.map(m => <li>{m.title}</li>}
    </ul>
  )
}

The effect isn't dependent on any state changing so I omitted the second parameter. What I failed to realize was that everytime setMeeting was called the body of the function would be executed again, causing a loop in the data fetching.

So yeah. Firebase's free tier offers a 50k reads/day quota that I exceeded in a few minutes of development work. It was a pain to trace down too. Once I realized that the Net tab in Chrome devtools was pounding out requests to firebase I had to hurry over to the perf tab and move into "offline mode". Then I had the time to take a look at the payload of one of the requests and figure out what data was being requested.

The fix is simply to add an empty square brace to indicate that this should only be run once, kind of like only componentDidMount and componentWillUnmount (the return from the firebase call is a listener we want to unmount).

useEffect(() => {
  // return firebase...
}, []) // this guy

After I fixed it though I paused and thought about the underlying problems.

  1. It is really easy to overlook the second parameter in an effect hook.

  2. There is no server side rate-limiting implementation for firebase / firestore. So any malicious user, or a bug in the code, can take down a free tier or charge a ton of money to a paying user. There wasn't even a great way to visualize what endpoint was being hammered, or when, by the quota management tool in Google's console.

  3. When your usage quota is exceeded in Firebase you can't even access your admin panel.

  4. Error handling doesn't catch this kind of thing.

Happy Hacking,


References

  • Photo by Andrew Gaines on Unsplash
  • Firebase is a Backend as a Service providing a generous free-tier for a realtime data storage solution

Top comments (14)

Collapse
 
17cupsofcoffee profile image
Joe Clay

There's a good explanation of why this is the default in the docs here: reactjs.org/docs/hooks-effect.html...

That said, I agree that it seems easy to shoot yourself in the foot with this behavior if you're not careful... Maybe in debug mode it should warn/error if an effect gets itself into a loop?

Collapse
 
dance2die profile image
Sung M. Kim

Thanks for sharing the failing experience and the lesson learned, DrKnoxy 👋

Hooks are great and sometimes being explicit about reading life cycle method names (cDM, cDU, cWU, etc).

From what I've learned React used to tell us what it was doing but the trend is that we devs are now responsible to tell what React should do

e.g.) componentDidMount -> React tells us what we are doing and we had to figure out how to use that lifecycle
useEffect -> We tell React what we want to do, and when.

So we are given a dangerous weapon and be careful of how to use it as mentioned in your post 😀

Collapse
 
itaditya profile image
Aditya Agarwal

I think both are same in this regard. React let's us hook arbitrary code between it's render cycle. Before we assigned a function to cDM etc. and now we do it by passing a callback to useEffect

Collapse
 
aodev profile image
AoDev

This type of problem is because separation of concerns was broken. It's not the role of a UI component to deal with data fetching.
React's developers seem to have forgotten that React is supposed to be a view library. They are adding more and more "features" unrelated to that purpose. People misuse all these hooks / functionalities and lose sight of clean architecture. They will have more and more problems like you had.

Collapse
 
itaditya profile image
Aditya Agarwal

Right now, React team's stand is that View is a very arbitrary term. What exactly constitutes a view?

I think the moment we are adding event listeners to our markup we are transitioning from simple markup template to adding business logic etc.

The good thing is we can still have separation of concerns. That's up to us. Separating components between Presentation and Container components is pretty common in React projects.

All in all, I think it's great React is making more things to make common scenarios easy to build. It's the first time a framework is attempting to solve data loading problem that causes jank or hundreds of loaders.

Collapse
 
aodev profile image
AoDev • Edited

If the business use cases can not be tested without the UI, then the separation of concerns is broken. That's why a "view" has nothing of "arbitrary" and has a well defined purpose: present data.

Taking the code given in the post, to test the business use case of "receiving some data about meetings", the UI code needs to be run. That's wrong. React or no React.

And React is not a framework, it's a library. But you can create a framework using it, like some people have already done.

Thread Thread
 
itaditya profile image
Aditya Agarwal

And React is not a framework, it's a library.

Regarding React being lib or framework, please check this thread. Ryan Florence says it's a framework. Actually there is no meaning in debating on this point.

If the business use cases can not be tested without the UI, then the separation of concerns is broken.

But in React, we render components and test them by checking if data is injected properly and things behaves a certain way based on various user events. That's the approach react-testing-library preaches.

The React community finds that MVC approach doesn't hold good on the frontend. Rather than divide by language (Controller- Js, View- Html) we divide by components. So one component has the responsibility of the UI and other logic of that piece. I have found this to be a better approach.

One thing though, if the end-result of some operation doesn't affect the view but only have side-effects on server or web storage then we can abstract them into functions (separate from React) and those can be simply tested like any other function

Collapse
 
krusenas profile image
Karolis

I have seen this mistake in projects where lifecycle hooks get the same outcome, for example ComponentWillUpdate would be fetching some additional data and it would result in a loop with 1K requests per second :)

Collapse
 
jay97 profile image
Jamal Al

Although im not a big fan of hooks, it's a nice read :)

Collapse
 
theodesp profile image
Theofanis Despoudis

Oooops 🔁

Collapse
 
cutiko profile image
Erick Navarro

It seems the Firestore query should have used get()

Collapse
 
adamwknox profile image
DrKnoxy

What good is a real time connection if you only read from it once?

Collapse
 
cutiko profile image
Erick Navarro

What is good from a 300km/h max speed car if you only drive it at 200km/h. Use case are real, technical choices are contingent.

Collapse
 
zeerorg profile image
Rishabh Gupta

This is going to become one of those errors which we accidentally do time and time again. Though there should be a runtime error raised when we try to use useEffect hook this way.