Forcibly reload app from server when API is redirected

[TimQuelch]

This adds a wrapper around fetch so the application is reloaded when an API request is redirected. The motivation for this is as a workaround for the bug in #2793.

This issue occurs when using forward authentication with something like authentik with a proxy (e.g. caddy, traefik). The proxy checks with the auth service whether all requests to the host are valid, if they are they are forwarded onto the backend server (in this case actual server). If the request is invalid e.g. the cookie has expired or the user has not authenticated yet, then the proxy redirects the user to the authentication page. After authenticating they are redirected back to what they were originally requesting.

For a more standard application; after a authenticatoin cookie has expired the next time the page is reloaded it would redirect to the auth page in order for the user to re-authenticate. However actual uses a service worker pattern where the application code is cached and all requests are served by the worker. This means that reloading the page in the browser does not actually result in a http request to the backend server; which means that that request cannot be redirected to authenticate. This results in an invalid authentication session and API queries to the backend failling because they are redirecting to auth pages.

This PR adds a button in the advanced settings section that un-registers the service worker handling the routing and then reloads the page. This forces the page to be requeried from the backend server which is then able to be redirected to the authentication page.

I've tested this functionality on web, mobile browser, and also iphone PWA and it works as expected.

I've got a couple things I want to check on my implementation:

• This functionality isn't relevant on electron. I have followed the pattern that the ResetSync follows by disabling the button if we're on electron. I considered also completely hiding the setting when on electron. Which would be preferable?
• this page mentions that changes to global.Acutal are somewhat fragile. Basically what I've done is defined the new reload function only in the browser version and typed it as maybe undefined. We check if it is defined before trying to call it; this means that on electron it won't be called. Is this the correct way to go about this?

I couldn't think of a solution that was less of a workaround than this. Maybe this reloading could be triggered if an API query gets a 302 to a configurable path? e.g if /sync/sync gets redirected uselessly to auth.mydomain.com then we trigger reloading the whole app so that the user can re-auth? I feel I don't know the full consequences of this solution though. Does anyone have any thoughts on a more automatic solution that will work in a general and/or configurable way?

Edit: After feedback I re-implemented the solution to wrap the API fetch so that if any API call is redirected the application is reloaded.

[netlify]

✅ Deploy Preview for actualbudget ready!

Name	Link
🔨 Latest commit	2f0edf7
🔍 Latest deploy log	https://app.netlify.com/sites/actualbudget/deploys/66e3bb85d9bccf0008ba4c54
😎 Deploy Preview	https://deploy-preview-3286.demo.actualbudget.org
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[github-actions]

Bundle Stats — desktop-client

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count	Total bundle size	% Changed
9	5.16 MB → 5.16 MB (+470 B)	+0.01%

Changeset

File	Δ	Size
home/runner/work/actual/actual/packages/loot-core/src/client/actions/app.ts	📈 +77 B (+13.73%)	561 B → 638 B
src/browser-preload.browser.js	📈 +321 B (+8.76%)	3.58 kB → 3.89 kB
src/global-events.ts	📈 +72 B (+2.20%)	3.2 kB → 3.27 kB

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset	File Size	% Changed
static/js/index.js	3.19 MB → 3.19 MB (+470 B)	+0.01%

Smaller

No assets were smaller

Unchanged

Asset	File Size	% Changed
static/js/resize-observer.js	18.37 kB	0%
static/js/BackgroundImage.js	122.29 kB	0%
static/js/indexeddb-main-thread-worker-e59fee74.js	13.5 kB	0%
static/js/narrow.js	78.89 kB	0%
static/js/usePreviewTransactions.js	1.59 kB	0%
static/js/AppliedFilters.js	20.97 kB	0%
static/js/wide.js	224.91 kB	0%
static/js/ReportRouter.js	1.5 MB	0%

[github-actions]

Bundle Stats — loot-core

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count	Total bundle size	% Changed
1	1.19 MB → 1.19 MB (+208 B)	+0.02%

Changeset

File	Δ	Size
packages/loot-core/src/platform/server/fetch/index.web.ts	📈 +629 B (+1612.82%)	39 B → 668 B

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset	File Size	% Changed
kcab.worker.js	1.19 MB → 1.19 MB (+208 B)	+0.02%

Smaller

No assets were smaller

Unchanged

No assets were unchanged

[StefanFabian]

I found this because I have the same issue with Authelia and Actual.
Why did you decide against just handling every 302 on API requests and executing the 302?
I would assume that the user is in control of the server hosting Actual, so all 302 requests will probably happen for a reason, and IMO, that would be a cleaner way of handling that than having to do it manually.

[alexyao2015]

Could we do something like this? I run silverbullet as well and it seems like they were able to solve the issue by just setting location.href to the redirected url. This has the benefit of being able to automatically redirect the user rather than manually pressing a button.

https://github.com/silverbulletmd/silverbullet/blob/main/common/spaces/http_space_primitives.ts#L42-L65

[TimQuelch]

Reloading when an API query gets a 302 would work for my setup (and I'd guess many other people's), however there are probably many valid setups where a 302 is normal and the correct action is to have the API follow the redirect and return the redirected response normally.

Can we be confident that any time a 302 is returned that we need to reload? Maybe a configuration setting which has this automatic reloading as an optional feature?

[TimQuelch]

Maybe an option where you can configure hosts that if we get redirected to one of those hosts we reload?

In this case any time I get a redirect to auth.mydomain.com I want to reload

[alexyao2015]

I don't see why any config would be necessary. If you make a request to a trusted destination and the request is calling for you to be redirected, you should redirect. You don't even need to handle reloading I think. As long as your proxy sends a redirect call, it should just directly redirect you without prompting. That's how normal webpages without service workers would react anyways.

[alexyao2015]

Just glacing over the source I see this which appears to let you wrap the fetch command.

actual/packages/api/index.ts

Line 32 in 9c0e6a3

if (!globalThis.fetch) {

[StefanFabian]

Reloading when an API query gets a 302 would work for my setup (and I'd guess many other people's), however there are probably many valid setups where a 302 is normal and the correct action is to have the API follow the redirect and return the redirected response normally.

Can we be confident that any time a 302 is returned that we need to reload? Maybe a configuration setting which has this automatic reloading as an optional feature?

A whitelist would also be an option but as this is a common problem with reverse proxies + auth, I think having to first figure out what's wrong and then find the solution would be a bit annoying to users.

As I see it, there are two common behaviors that can happen when you get a 302:

1. Your browser sends an options request and the reply doesn't have the correct CORS headers

• Should you redirect to the page in that case? I think this will be the default case unless someone already configured their auth server to return Access-Control-Allow-Headers: * since actual will send the following access control request Access-Control-Request-Headers: x-actual-token with the options request.

2. The options request passes and the API follows the 302 successfully

• If it is just an API redirect, the data should still be valid JSON. For an auth server, the JSON parsing would fail and we could use that as an indication that we should redirect to the 302 page.
  Maybe first verify that the returned data is HTML?

Another point to consider is that some auth solutions pass the return URL to the auth page.
If we just set the location to the redirect URL, after authentication, it would redirect the user to the result of an API call, which doesn't make sense.
Is there a way to fetch the current URL without the service worker catching the request and serving it from the cache?
In that case, we could use that after determining that authentication is required.

[TimQuelch]

I'm happy to implement doing this automatically when an API request gets a 302 if maintainers agree that is appropriate. My initial design was erring on the side of not breaking existing setups.

If you make a request to a trusted destination and the request is calling for you to be redirected, you should redirect. You don't even need to handle reloading I think. As long as your proxy sends a redirect call, it should just directly redirect you without prompting. That's how normal webpages without service workers would react anyways.

The problem is only api requests get redirected. GET requests for the page itself get served by the service worker with cached code and therefore do not get redirected.

Another point to consider is that some auth solutions pass the return URL to the auth page.
If we just set the location to the redirect URL, after authentication, it would redirect the user to the result of an API call, which doesn't make sense.

I agree, we do not want to follow the 302 that originates from the API request because after authentication finishes we'd be redirected back to the api call; not the app. Instead we'd want to force a uncached GET the current page and follow the resulting redirect.

Is there a way to fetch the current URL without the service worker catching the request and serving it from the cache?
In that case, we could use that after determining that authentication is required.

The way I've figured out how to do that is by unregistering the navigation service worker and reloading. This is what I do in my reload function now. I'm open to other suggestions on how to do this. My understanding of service workers is fairly limited.

[jfdoming]

I'm happy to implement doing this automatically when an API request gets a 302 if maintainers agree that is appropriate. My initial design was erring on the side of not breaking existing setups.

If you make a request to a trusted destination and the request is calling for you to be redirected, you should redirect. You don't even need to handle reloading I think. As long as your proxy sends a redirect call, it should just directly redirect you without prompting. That's how normal webpages without service workers would react anyways.

The problem is only api requests get redirected. GET requests for the page itself get served by the service worker with cached code and therefore do not get redirected.

Another point to consider is that some auth solutions pass the return URL to the auth page.
If we just set the location to the redirect URL, after authentication, it would redirect the user to the result of an API call, which doesn't make sense.

I agree, we do not want to follow the 302 that originates from the API request because after authentication finishes we'd be redirected back to the api call; not the app. Instead we'd want to force a uncached GET the current page and follow the resulting redirect.

Is there a way to fetch the current URL without the service worker catching the request and serving it from the cache?
In that case, we could use that after determining that authentication is required.

The way I've figured out how to do that is by unregistering the navigation service worker and reloading. This is what I do in my reload function now. I'm open to other suggestions on how to do this. My understanding of service workers is fairly limited.

I think the approach needed here would be to change the service worker logic to stale-while-revalidate, which causes it to still fetch in the background if there is a connection. That way we can handle the background fetch and redirect if it returns 302. I'm on board with just redirecting unconditionally for now; if someone does start using a setup where the redirect isn't an auth request, we can help them at that time.

Another thing worth noting is that IIRC one of the first things the app does when it loads is make an API request to check if the server has been initialized. Assuming my memory is correct, maybe handling only API requests is fine?

[alexyao2015]

Your browser sends an options request and the reply doesn't have the correct CORS headers

In this case, nothing happens since the redirect url cannot be determined.

change the service worker logic to stale-while-revalidate

It's probably better to use network first. Stale while revalidate would work but only on the second request since it pulls from the cache first.

[alexyao2015]

Looking at the config, it seems like no runtime caching is being performed right now. Presumably, all you would need to do is handle the redirect that the service worker returns. A slight disadvantage is the return url would likely be an API call. I suppose if you unregister the service worker and remake the request, that will direct the user to the right place. I'm just not sure of the implications of unregistering the service worker.

https://github.com/actualbudget/actual/blob/master/packages/desktop-client/vite.config.mts#L155

[TimQuelch]

I've added functionality to automatically reload when an API call gets redirected. Please let me know if there's anything to improve

[alexyao2015]

Is the reload button on settings even needed still? Can that be removed?

[TimQuelch]

Yes. I've now removed the setting and updated the release notes.

[alexyao2015]

LGTM but I'll let someone with more familiarity take a look at the code. The PR description also need an update as well.

[AWare]

hi, this pr would be incredibly useful to me as I also run mine behind an authenticated gateway.
the implementation looks sensible, but I'm not a contributor.

if I can be of help here, I'd be happy to- but otherwise I'm just chiming in with thanks to @TimQuelch

[youngcw]

if I can be of help here, I'd be happy to

feel free to try it out and even put in a reviews, but you are right that a maintainer has to sign it off to merge in.

[TimQuelch]

At this point it is ready for review and just waiting for a maintainer to have the time to take a look.

I've been running this branch on my instance for several weeks now with no issues.

[alexyao2015]

Is there an easy way to run this in container or do you need to run an entire build process to get that?

[TimQuelch]

Is there an easy way to run this in container or do you need to run an entire build process to get that?

This is the dockerfile that I use to run the actual server with specific versions of actual. Modify the commit arg to the git commit you want to run. https://gist.github.com/TimQuelch/8f794cf187eeabb24d6eaee278e3ba38

It is somewhat based on the the actual server edge images https://github.com/actualbudget/actual-server/blob/master/docker/edge-alpine.Dockerfile

[matt-fidd]

@coderabbitai full review

[coderabbitai]

Actions performed

Full review triggered.

[coderabbitai]

Walkthrough

The pull request introduces several changes aimed at enhancing the application's ability to manage state and handle API interactions more effectively. Key updates include the addition of a reload method to the global Actual object for reloading the application, a new event listener for handling API fetch redirections, and modifications to the fetch function to improve error handling. Additionally, new TypeScript declarations have been added to support these functionalities.

Changes

Files	Change Summary
packages/desktop-client/src/browser-preload.browser.js	Added a reload method to global.Actual to reload the application after unregistering the service worker, ensuring fresh data is fetched from the server.
packages/desktop-client/src/global-events.ts	Added an event listener for api-fetch-redirected that calls the reloadApp action to refresh the application state when a fetch operation is redirected.
packages/loot-core/src/client/actions/app.ts	Introduced a reloadApp function that invokes the global.Actual.reload() method to facilitate application reloading.
packages/loot-core/src/platform/server/fetch/index.web.ts	Replaced the default fetch export with a custom asynchronous function that handles API request redirection and error notifications when the request is intercepted by an authentication proxy.
packages/loot-core/src/types/server-events.d.ts	Added a new event type 'api-fetch-redirected' to the ServerEvents interface to accommodate new API fetch scenarios.
packages/loot-core/typings/window.d.ts	Introduced a new optional reload method in the global interface, defined as an asynchronous function returning a Promise<void>, enhancing the control flow for reload operations.

Suggested reviewers

• TimQuelch
• StefanFabian
• Alexyao2015
• AWare
• youngcw

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  -- I pushed a fix in commit <commit_id>, please review it.
  -- Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  -- @coderabbitai generate unit testing code for this file.
  -- @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  -- @coderabbitai generate interesting stats about this repository and render them as a table.
  -- @coderabbitai read src/utils.ts and generate unit testing code.
  -- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  -- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 0

Outside diff range and nitpick comments (2)

packages/loot-core/src/types/server-events.d.ts (1)

21-21: LGTM! Consider using a more specific type for the event payload.

The addition of the 'api-fetch-redirected' event type enhances the server's ability to handle and respond to various states of API interactions, potentially improving the robustness of the event handling system.

However, consider using a more specific type for the event payload if possible. This can help catch type-related issues at compile time and provide better type safety in the event handlers.

packages/loot-core/src/platform/server/fetch/index.web.ts (1)

3-19: LGTM! The custom fetch function is a great addition to enhance error handling and monitoring.

The custom fetch function is well-structured and follows best practices for error handling and monitoring. It detects and handles API request redirection effectively, allowing for better debugging and security awareness in the application.

A few additional suggestions for improvement:

1. Consider adding a configuration option to enable or disable the redirection handling logic. This would provide flexibility for different setups and allow users to opt-out if needed.
2. Consider logging the redirection details (original URL, redirected URL, etc.) for easier debugging and tracking.
3. Consider adding tests to cover the redirection handling logic and ensure it works as expected.

Overall, great work on this enhancement!

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between cfa9ac0 and 2f0edf7.

Files ignored due to path filters (1)

• upcoming-release-notes/3286.md is excluded by !**/*.md

Files selected for processing (6)

• packages/desktop-client/src/browser-preload.browser.js (1 hunks)
• packages/desktop-client/src/global-events.ts (1 hunks)
• packages/loot-core/src/client/actions/app.ts (1 hunks)
• packages/loot-core/src/platform/server/fetch/index.web.ts (1 hunks)
• packages/loot-core/src/types/server-events.d.ts (1 hunks)
• packages/loot-core/typings/window.d.ts (1 hunks)

Additional comments not posted (4)

packages/loot-core/typings/window.d.ts (1)

18-18: LGTM! The optional reload method is a valuable addition.

The introduction of the optional reload method to the global Actual object aligns well with the PR objectives of enhancing the application's ability to manage state and handle API interactions more effectively. By returning a Promise<void>, the method allows for better control flow when handling reloads that may involve asynchronous tasks, such as refreshing data or reinitializing components.

The optional nature of the reload method provides flexibility for different environments or configurations, while its asynchronous behavior contributes to a more dynamic and responsive application. This change enables developers to handle reloads in a structured manner, improving the overall user experience.

packages/loot-core/src/client/actions/app.ts (1)

50-54: LGTM! The new reloadApp function looks good and serves its purpose.

The reloadApp function provides a way to reload the application by invoking the reload method on the global Actual object. This change is part of a larger effort to handle authentication issues when using forward authentication with proxies like Caddy or Traefik.

Based on the PR objectives and comments, this function was initially introduced along with a button in the advanced settings to unregister the service worker and reload the page. However, the implementation was later adjusted to automatically reload the application upon receiving a 302 response from any API call, with the aim of improving user experience without breaking existing setups.

It's important to consider the broader context and potential impact of this change on the application, especially in terms of handling redirects and ensuring that users are not inadvertently redirected back to an API call after authentication.

Overall, the reloadApp function itself looks good and serves its purpose within the larger scope of the PR.

packages/desktop-client/src/global-events.ts (1)

157-159: Verify if the automatic reloading behavior is configurable or has safeguards.

The implementation of the event listener for api-fetch-redirected looks good and aligns with the PR objective of automatically reloading the app when an API request is redirected.

However, as discussed in the PR comments, automatic reloading on a 302 response may not be suitable for all setups. Additionally, there is a risk of inadvertently redirecting users back to an API call after authentication if not handled carefully.

Please verify the following:

1. Is there a configuration option to manage the automatic reloading behavior?
2. Are there safeguards in place to prevent unintended redirects, such as a whitelist for trusted redirect URLs or modifications to the service worker logic to use a "network first" strategy?

You can use the following script to search for relevant code:

packages/desktop-client/src/browser-preload.browser.js (1)

54-68: LGTM!

The reload method is a well-designed addition to the global.Actual object. It effectively handles the unregistration of the service worker and reloads the application to ensure fresh data is fetched from the server.

The code segment demonstrates the following positive aspects:

• Graceful degradation by checking for service worker support before proceeding with the unregistration process.
• Proper use of promises and chaining to ensure the unregistration is complete before triggering the page reload.
• Clear and concise code structure that is easy to understand and maintain.

Overall, this change enhances the application's ability to refresh its state and content, particularly in scenarios where updates or changes on the server need to be reflected immediately in the client without interference from the service worker.

[jfdoming]

One concern I thought of with this approach is that the user could get stuck in a reload loop if the server sends a 302 to only API resources (if someone forgets to put the JS bundle behind a proxy). Maybe that is not a realistic scenario but thought it was worth mentioning the possibility