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

Instant loading should default to window.location on 4xx status code #3886

Closed
4 tasks done
dmccaffery opened this issue May 5, 2022 · 5 comments
Closed
4 tasks done
Labels
bug Issue reports a bug resolved Issue is resolved, yet unreleased if open

Comments

@dmccaffery
Copy link
Sponsor

Contribution guidelines

I want to suggest an idea and checked that ...

  • ... to my best knowledge, my idea wouldn't break something for other users
  • ... the documentation does not mention anything about my idea
  • ... there are no open or closed issues that are related to my idea

Description

Our organisation protects our static sites behind authentication based on a token stored as a cookie. This is implemented via lambda@edge on cloudfront, but equally applies to any other proxy that supports authentication. When the cookie / token expires, the XHR requests used by the instant integration silently fails and navigation breaks. I would expect this to be a fairly common use case, such as those used by backstage.io and private GitHub pages deployments in GHE.

The recommendation is to have the instant integration fallback to setting the window location, which would allow any proxies to correctly handle the redirects required to refresh the tokens, or to at least fail with an unauthorised error so the user knows what is going on, instead of silently failing.

Use Cases

  1. Enable authenticated (private) sites to correctly handle oidc flows or other auth flows that cannot be handled via XHR (such as browser redirects)
  2. Enable the user to react to the error condition if the server renders error pages. As of right now, there is no notification / toast, etc, that would alert the user as to why navigation is not functioning.

Screenshots / Mockups

No response

@squidfunk
Copy link
Owner

Thanks for suggesting, that definitely sounds like a valid use case and a good idea.

@squidfunk squidfunk added the change request Issue requests a new feature or improvement label May 5, 2022
@squidfunk
Copy link
Owner

This behavior is actually implemented, but it does not trigger because failing requests are currently just filtered:

switchMap(url => request(url.href)
.pipe(
catchError(() => {
setLocation(url)
return NEVER
})
)
),

export function request(
url: URL | string, options: RequestInit = { credentials: "same-origin" }
): Observable<Response> {
return from(fetch(`${url}`, options))
.pipe(
filter(res => res.status === 200),
catchError(() => EMPTY)
)
}

Instant loading is definitely in need of refactoring as already noted in #3797 since it kind of outgrew its initial architecture/design. I'm going to tackle this problem as well as part of the refactoring when I can allocate enough time.

@squidfunk squidfunk changed the title feat(integration/instant): fallback to setting window location on non-recoverable errors such as unauthorised Instant loading should default to window.location on 4xx status code May 6, 2022
@squidfunk squidfunk added bug Issue reports a bug and removed change request Issue requests a new feature or improvement labels May 6, 2022
@squidfunk
Copy link
Owner

Fixed in 8beda2b. This is more of a mitigation than a proper fix, because as I noted – instant loading needs refactoring. However, it should fix the issue at hand, hopefully without introducing new problems, as I just moved the suboptimal error handling behavior downstream.

@squidfunk squidfunk added the resolved Issue is resolved, yet unreleased if open label May 7, 2022
@dmccaffery
Copy link
Sponsor Author

This is amazing, thank you. If I can help in any way, let me know. Happy to put hands behind keyboards. :)

@squidfunk
Copy link
Owner

Released as part of 8.2.14.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue reports a bug resolved Issue is resolved, yet unreleased if open
Projects
None yet
Development

No branches or pull requests

2 participants