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.

Sign up for GitHub

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

Improve error handling (fixes #897) #1013

Closed
wants to merge 1 commit into from
Closed

Improve error handling (fixes #897) #1013

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions src/server/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import express from "express";
import fs from "fs";
import { IncomingHttpHeaders } from "http";
import { Helmet } from "inferno-helmet";
import { matchPath, StaticRouter } from "inferno-router";
import { StaticRouter, matchPath } from "inferno-router";
import { renderToString } from "inferno-server";
import IsomorphicCookie from "isomorphic-cookie";
import { GetSite, GetSiteResponse, LemmyHttp } from "lemmy-js-client";
Expand Down Expand Up @@ -141,7 +141,7 @@ server.get("/*", async (req, res) => {

const routeData = await Promise.all(promises);

// Redirect to the 404 if there's an API error
// Handle API errors
if (routeData[0] && routeData[0].error) {
const error = routeData[0].error;
console.error(error);
Expand Down Expand Up @@ -209,7 +209,7 @@ server.get("/*", async (req, res) => {

<!-- Current theme and more -->
${helmet.link.toString()}

</head>

<body ${helmet.bodyAttributes.toString()}>
Expand All @@ -223,10 +223,17 @@ server.get("/*", async (req, res) => {
<script defer src='/static/js/client.js'></script>
</body>
</html>
`);
`);
} catch (err) {
console.error(err);
return res.send(`404: ${removeAuthParam(err)}`);
var formatted: string;
if (err.status && err.message) {
res.status(err.status);
formatted = `${err.status}: ${err.message}`;
} else {
res.status(500);
formatted = err;
}
res.send(formatted);
Copy link
Member Author

Choose a reason for hiding this comment

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

This could be improved further by displaying the error message as a full page with normal header and footer (if site was requested successfully). In case of error 401 it can also redirect to /login like some routes already do.

Copy link
Member

Choose a reason for hiding this comment

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

The first part was mentioned in #982. If you don't do it as part of this PR, I could whip something up. I'm wondering: if the site returns a server error but the site is still fetched, is it a good idea to list the matrix user IDs of instance admins for users to reach out to for support? That information is already on the site response, so no further requests would be required.

}
});

Expand Down
3 changes: 3 additions & 0 deletions src/shared/components/home/admin-settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { InitialFetchRequest } from "../../interfaces";
import { WebSocketService } from "../../services";
import {
capitalizeFirstLetter,
check_auth,
isBrowser,
myAuth,
randomStr,
Expand Down Expand Up @@ -60,6 +61,8 @@ export class AdminSettings extends Component<any, AdminSettingsState> {
this.parseMessage = this.parseMessage.bind(this);
this.subscription = wsSubscribe(this.parseMessage);

check_auth();
Copy link
Member

Choose a reason for hiding this comment

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

Instead of doing this on the client, I think it would better to handle this on the server and redirect the user before even sending the page that requires auth back.

Copy link
Member Author

Choose a reason for hiding this comment

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

To be honest Im not good at frontend dev. If you want you can take over this PR because it takes me forever just to implement the basics. So Id rather stay in Rust :)


// Only fetch the data if coming from another route
if (this.isoData.path == this.context.router.route.match.url) {
this.state = {
Expand Down
3 changes: 3 additions & 0 deletions src/shared/components/home/setup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ export class Setup extends Component<any, State> {
}

render() {
if (this.state.siteRes.site_view.local_site.site_setup) {
throw { status: 403, message: "Site is already setup" };
}
return (
<div className="container-lg">
<Helmet title={this.documentTitle} />
Expand Down
6 changes: 2 additions & 4 deletions src/shared/components/person/inbox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import { i18n } from "../../i18next";
import { CommentViewType, InitialFetchRequest } from "../../interfaces";
import { UserService, WebSocketService } from "../../services";
import {
check_auth,
commentsToFlatNodes,
createCommentLikeRes,
editCommentRes,
Expand Down Expand Up @@ -111,10 +112,7 @@ export class Inbox extends Component<any, InboxState> {
this.handleSortChange = this.handleSortChange.bind(this);
this.handlePageChange = this.handlePageChange.bind(this);

if (!UserService.Instance.myUserInfo && isBrowser()) {
toast(i18n.t("not_logged_in"), "danger");
this.context.router.history.push(`/login`);
}
check_auth();

this.parseMessage = this.parseMessage.bind(this);
this.subscription = wsSubscribe(this.parseMessage);
Expand Down
6 changes: 6 additions & 0 deletions src/shared/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1603,3 +1603,9 @@ export function getQueryString<T extends Record<string, string | undefined>>(
"?"
);
}

export function check_auth() {
if (!UserService.Instance.myUserInfo) {
throw { status: 401, message: "Login required" };
}
}