Add automatic request retry on CSRF failures.

[jdeanwallace]

This PR wraps all unsafe HTTP requests (made from the frontend) with a function that retries the request once more if it fails due to a stale CSRF token.

Desired behaviour

1. Frontend gets loaded in the browser
2. Backend restarts for whatever reason. This changes the CSRF token.
3. Frontend makes a unsafe HTTP request (i.e. PUT, POST, DELETE) to the backend.
4. The request fails with a 403 Forbidden HTTP error because the frontend has an outdated CSRF token.
5. The frontend refreshes its CSRF token
6. The original request (in step 3) is retried once.

Caveat

Seeing as the retry behaviour is applied to the individual controller functions, a race-condition is created when calling these controllers in "parallel" and passing them to Promise.all. Currently this race-condition can only happen in the video-dialog component and if the backend restarted before clicking "Apply". Here's a video to demonstrate.

Fixes #495

---

This change is 

[jotaen4tinypilot]

The race condition is quite interesting, but also tricky as you mentioned. If the “time budget” allows us to keep working on it a bit more, I think it would be worthwhile to fix this, because it’s an inherent problem even if it’s just one place right now.

I’ve fiddled around on a separate branch and got something to work: https://github.com/tiny-pilot/tinypilot/compare/auto-refresh-csrf-token...auto-refresh-csrf-token-fix?expand=1

It’s not nicely coded, more of a proof-of-concept: to avoid multiple requests kicking of the refresh simultaneously, we could mark the token as “refresh-in-progress” and stall the auto-retry until we managed to obtain a fresh token. The idea is basically like a thread lock for the token refresh. I’m not certain that’s the ultimate solution that covers all edge cases, but it seemed to work and maybe is a viable approach.

Two other things:

• Generally, I think it would make sense to encapsulate the CSRF-related logic in a separate file / module (like csrf.js), to streamline the controllers module a bit.
• Would we still need the refreshCsrfToken() call in <update-dialog> in line 294, or would that problem become obsolete due to the new automatism?

Reviewable status: 0 of 1 LGTMs obtained

---

app/static/js/controllers.js, line 29 at r1 (raw file):

 * If the HTTP request fails due to a stale CSRF token, then refresh the CSRF
 * token and retry the original request once more.
 * @param {string} resource The path to the resource you want to fetch. See the

(optional) I personally would be okay if we just said something along the lines of “this method has the same API as the global fetch function” instead of reiterating the signature, since it’s clear that this function is just meant to be a transparent wrapper or decorator for an existing function.

---

app/static/js/controllers.js, line 36 at r1 (raw file):

 * @returns {Promise}
 */
async function fetchWithRetry(resource, init) {

Maybe fetchWithCsrfRetry to make it clearer what is retried?

---

app/static/js/controllers.js, line 39 at r1 (raw file):

  let response = await fetch(resource, init);
  // Only retry a request when the original request has a CSRF token header.
  // This is to avoid programmers from getting lazy and never including a CSRF

Wouldn’t the request fail anyway if the programmer forgot to include the token? That being said, I think it’s still good to have that check, because it’s most likely a developer’s mistake otherwise.

To make that issue easier to spot, maybe something like:

if (!csrfTokenHeader) {
  console.error('Cannot trigger CSRF refresh if token is not included initially.');
  return response;
}
if (response.status !== 403) {
  return response;
}

---

app/static/js/controllers.js, line 42 at r1 (raw file):

  // token header in their requests.
  let csrfTokenHeader = init?.headers?.["X-CSRFToken"] || null;
  if (!csrfTokenHeader || response.status != 403) {

!= should better be !== to avoid JS’s type coercion (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equality_comparisons_and_sameness).

[jdeanwallace]

I’ve fiddled around on a separate branch and got something to work: https://github.com/tiny-pilot/tinypilot/compare/auto-refresh-csrf-token...auto-refresh-csrf-token-fix?expand=1

Wow, thanks for going through the trouble of coding up a PoC. I've taken your solution and expanded on it a bit more. Instead of using time-based polling retries, I used a "global" Promise as the lock. This allows the retries to be event-based and trigger immediately after the CSRF token refresh has completed. Let me know what you think?

it would make sense to encapsulate the CSRF-related logic in a separate file / module (like csrf.js)

Good idea. Done.

Would we still need the refreshCsrfToken() call in in line 294, or would that problem become obsolete due to the new automatism?

Yeah I think calling refreshCsrfToken explicitly is no longer necessary. Good catch.

Here's a demo video of TinyPilot working after these changes have been applied.

Reviewable status: 0 of 1 LGTMs obtained

---

app/static/js/controllers.js, line 29 at r1 (raw file):

Previously, jotaen4tinypilot wrote…

(optional) I personally would be okay if we just said something along the lines of “this method has the same API as the global fetch function” instead of reiterating the signature, since it’s clear that this function is just meant to be a transparent wrapper or decorator for an existing function.

Done.

---

app/static/js/controllers.js, line 36 at r1 (raw file):

Previously, jotaen4tinypilot wrote…

Maybe fetchWithCsrfRetry to make it clearer what is retried?

Yes, I like that. Thanks.

---

app/static/js/controllers.js, line 39 at r1 (raw file):

Wouldn’t the request fail anyway if the programmer forgot to include the token?

Yes it would initially fail, but then if this check didn't exist it would insert the missing token automatically and eventually succeed. This would result in the code making an extra request each time.

To make that issue easier to spot, maybe something like...

Yes, that is more clear. Thanks.

---

app/static/js/controllers.js, line 42 at r1 (raw file):

Previously, jotaen4tinypilot wrote…

!= should better be !== to avoid JS’s type coercion (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equality_comparisons_and_sameness).

Done.

[jotaen4tinypilot]

Let me know what you think?

I like it, this seems clear and straightforward.

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained

---

app/static/js/controllers.js, line 39 at r1 (raw file):

Yes it would initially fail, …

Ah okay, that makes sense! 👍

---

app/static/js/csrf.js, line 1 at r3 (raw file):

let csrfRefreshLock = null;

Could we add a brief comment here to explain what the two possible values are? (null or Promise)

---

app/static/js/csrf.js, line 38 at r3 (raw file):

 */
export async function fetchWithCsrfRetry(resource, init) {
  let response = await fetch(resource, init);

I think this can be const, since we are not altering the response variable.

[jotaen4tinypilot]

Reviewable status: complete! 1 of 1 LGTMs obtained

[jdeanwallace]

Reviewable status: complete! 1 of 1 LGTMs obtained

---

app/static/js/csrf.js, line 1 at r3 (raw file):

Previously, jotaen4tinypilot wrote…

Could we add a brief comment here to explain what the two possible values are? (null or Promise)

Done.

---

app/static/js/csrf.js, line 38 at r3 (raw file):

Previously, jotaen4tinypilot wrote…

I think this can be const, since we are not altering the response variable.

Done.