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

Sanitize routes #11

Open
wants to merge 15 commits into
base: dev
Choose a base branch
from
Open

Sanitize routes #11

wants to merge 15 commits into from

Conversation

jdonszelmann
Copy link
Contributor

No description provided.

docs/running_server.md Outdated Show resolved Hide resolved
@@ -7,11 +7,17 @@ edition = "2021"

[dependencies]
dotenv = "0.15.0"
axum = "0.2.8"
axum = {git="https://github.com/tokio-rs/axum", branch="main"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Which features do we get from main, just curious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there are some pretty major bugs with middleware on not-main. Basically early-returning middleware do not work in any older version of axum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also fixes compile times of the generic stuff, even without boxed routes

Copy link
Contributor

Choose a reason for hiding this comment

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

axum 0.3 is now available, can we use that instead of pulling from their git

server/src/auth/mod.rs Show resolved Hide resolved
server/src/db_connection.rs Show resolved Hide resolved
server/src/error.rs Show resolved Hide resolved
server/src/handlers/users.rs Show resolved Hide resolved
server/src/handlers/users.rs Show resolved Hide resolved
server/src/handlers/users.rs Show resolved Hide resolved
server/src/repository.rs Outdated Show resolved Hide resolved
server/src/test_util.rs Show resolved Hide resolved
@NULLx76 NULLx76 mentioned this pull request Nov 2, 2021
Closed
@NULLx76 NULLx76 requested review from Druue and NULLx76 November 3, 2021 12:10
NotAdmin,

#[error("this route can only be accessed for your own user or if you are admin")]
NotSelf,
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels superfluous, as in, this shouldn't be a state that is accessible to a user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it can be. When you ask for a user's information but that user is not yourself. Then this error is returned.

Self::Mongo(e) => response_error!(e, internal server error),
Self::Oid(e) => response_error!(e, internal server error),
CheeseError::Login(l) => match l {
e @ LoginError::InvalidCredentials => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a specific reason for using this e @ ... syntax over just using a &a ?
This feels, at face value, like un-needed complexity wrt how easy the code base is to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to match the entire error to give the right error message, but also want to print the original error message.

}
LoginError::Database(e) => response_error!(e, internal server error),
LoginError::Bcrypt(e) => response_error!(e, internal server error),
e @ LoginError::UserWithoutId => response_error!(e, internal server error),
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the work flow that causes this to happen? This shouldn't be possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just don't want the server to crash in these cases. But it can't normally happen unless we have a bug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants