Auth error handling with status codes

[MP-Tool]

Problem:

The current error handling in authentication is not optimal, as it always returns HTTP status code 500, regardless of whether no credentials were found in the header, the credentials are incorrect, or the user exists but is disabled. This prevents the client from distinguishing why authentication failed.

I noticed this problem in the browser console when navigating to the login page. Resources are being queried in the background even though we are not logged in. This would certainly be worth its own issue, but I will fix the problem piece by piece in smaller PRs.

Solution:

Similar to PR #819, I have adjusted the error handling to use different HTTP status codes and provide clearer information to the user about why authentication failed.

401 UNAUTHORIZED

[mbecker20]

You can .status_code() directly after .context()

[MP-Tool]

Oh, I didn't think so, sure

[mbecker20]

same here

[mbecker20]

This one is not consistent with the rest

[MP-Tool]

I'll adjust the others too so that it's consistent. I just left them as they were at first because the adjustment caused errors in the websocket file.

[MP-Tool]

I hope this version is what you had in mind.

I kept the low-level functions with their original 'anyhow::Result' signatures and logic unchanged. Only the high-level functions now consistently use 'serror::Result' with proper HTTP status codes.

[mbecker20]

I don't think this PR is necessary. It already does return UNAUTHORIZED for all failures to auth from fn auth_request

[MP-Tool]

I don't think this PR is necessary. It already does return UNAUTHORIZED for all failures to auth from fn auth_request

The Real Problem (With Evidence):

While the main branch does have .status_code(StatusCode::UNAUTHORIZED) in the auth_request function, in practice this doesn't work because of how the error propagation works.

Actual Test Results:

BEFORE this PR (main branch):

• Browser Console: POST http://localhost:9120/auth/GetUser
• Status Code: 500 (Internal Server Error) ❌

AFTER this PR:

• Browser Console: POST http://localhost:9120/auth/GetUser
• Status Code: 401 (Unauthorized) ✅

Technical Explanation:

The issue is in the error conversion chain:

Main Branch (BROKEN):

// auth_request function:
let user = authenticate_check_enabled(&headers)    // <-- Returns anyhow::Result<User>
  .await
  .status_code(StatusCode::UNAUTHORIZED)?;         // <-- Should convert anyhow -> serror

// authenticate_check_enabled function:
pub async fn authenticate_check_enabled(headers: &HeaderMap) -> anyhow::Result<User> {
  let user_id = get_user_id_from_headers(headers).await?;  // <-- anyhow::Error with no status code
  // ...
}

The problem is that inner anyhow::Errors (from JWT parsing, DB queries, etc.) don't have HTTP status codes attached. When these errors bubble up, they default to HTTP 500. Even though .status_code(UNAUTHORIZED) exists in auth_request, it doesn't get applied correctly to all error paths.

This PR (FIXED):

// auth_request function:
let user = authenticate_check_enabled(&headers).await?;  // <-- serror::Result with correct status

// authenticate_check_enabled function:  
pub async fn authenticate_check_enabled(headers: &HeaderMap) -> serror::Result<User> {
  let user_id = get_user_id_from_headers(headers).await?;  // <-- serror::Error with explicit status codes
  // ...
}

// get_user_id_from_headers function:
pub async fn get_user_id_from_headers(headers: &HeaderMap) -> serror::Result<String> {
  match (...) {
    (Some(jwt), _, _) => {
      auth_jwt_get_user_id(jwt).await
        .context("failed to authenticate jwt")
        .status_code(StatusCode::UNAUTHORIZED)  // <-- Explicit 401 for bad JWT
    }
    (None, Some(key), Some(secret)) => {
      auth_api_key_get_user_id(key, secret).await
        .context("failed to authenticate api key")  
        .status_code(StatusCode::UNAUTHORIZED)  // <-- Explicit 401 for bad API key
    }
    _ => {
      Err(anyhow!("missing auth headers")
        .status_code(StatusCode::BAD_REQUEST))  // <-- Explicit 400 for missing headers
    }
  }
}

By using serror::Result throughout the auth chain and attaching status codes at the error source, the correct HTTP status codes are preserved and returned to the client.

Benefits of This Approach:

1. Semantic HTTP Status Codes:

   • 400 BAD_REQUEST: Missing authentication headers (client error)
   • 401 UNAUTHORIZED: Invalid credentials (authentication failure)
   • 403 FORBIDDEN: Valid user but disabled (authorization failure)

2. Better Client Experience: Frontend can distinguish between different auth failure types

3. Actual Bug Fix: Resolves the practical issue where 500 errors were returned instead of proper auth errors

Conclusion:

This isn't just a "nice to have" refactor - it's fixing an actual bug where the server returns misleading HTTP status codes. The screenshots prove that the current main branch behavior is incorrect in practice, regardless of what the code appears to do on paper.

Recommendation: Keep this PR as it solves a real issue with incorrect HTTP status codes.

[mbecker20]

I do see that GetUser returns 500. This should be one line change though, add status code unauthorized to call in GetUser, there is unnecessary changes here.

[MP-Tool]

Okay then I'll refractor my PR to the following to fix the bug. I thought that I can also improve the behavior when no authorization filed was send in the header. In my opinion the server then should respond with status code 400 BAD_REQUEST but I have no clue how to achieve this in way that you like. Because of this I'll leave it as it is and commit the simple fix to complete the PR.

impl Resolve<AuthArgs> for GetUser {
  #[instrument(name = "GetUser", level = "debug", skip(self))]
  async fn resolve(
    self,
    AuthArgs { headers }: &AuthArgs,
  ) -> serror::Result<User> {
    let user_id = get_user_id_from_headers(headers)
      .await
      .status_code(StatusCode::UNAUTHORIZED)?;
    Ok(get_user(&user_id).await?)
  }
}

[mbecker20]

Suggested change

	Ok(get_user(&user_id).await?)
	get_user(&user_id).await.status_code(StatusCode::UNAUTHORIZED)

[MP-Tool]

done

[mbecker20]

Re-check, this is on get_user. Currently it will 500 if it can't find user at id.

[mbecker20]

I think unauthorized is fine in all these cases, there is still the message with the reason. 400 in particular already being used specifically for invalid json or other invalid body on otherwise authenticated request and shouldn't be mixed into auth related errors.

[MP-Tool]

I didn't expect that. From a developer's perspective, I agree with you that 401 and/or the error message clearly indicate what the actual problem is. But when I look at it from a security perspective, the potential attack vector could be minimised if we don't describe the actual problem in too much detail and return more details about the status code. I had that in mind when I was working on this PR.
I'm not saying that the current implementation is very insecure or represents a major attack vector, but a switch that detects whether we are working in a production or development environment to disable the detailed output in the API would be a good solution to work around this problem.

PR is ready

[mbecker20]

Not sure what you are referring to re attack vectors or security, don't see how status code is relevant there.

[MP-Tool]

Not sure what you are referring to re attack vectors or security, don't see how status code is relevant there.

I don't think this is a major issue currently, but it does provide the opportunity for username enumeration attacks.

The principle I'm referring to is Information Disclosure - where different error messages reveal system internals to potential attackers.

Current examples in the codebase:
"no api key matching key" vs "invalid api secret" (lines 119, 133) → reveals valid API keys
"api key expired" vs other errors → reveals key existence

The security concern: An attacker can systematically test credentials and learn about valid users/keys based on different error responses.

My suggested solution: Environment-based error handling - detailed errors in development, generic "Authentication failed" in production. This maintains developer experience while preventing information leakage in production environments.

With this explanation, I would like to conclude the PR at this point.

[mbecker20]

"no api key matching key" vs "invalid api secret" (lines 119, 133) → reveals valid API keys
"api key expired" vs other errors → reveals key existence

This is reasonable, just has more to do with the message than status code. It could be handled in another focused PR with intention to remove some of it.

There's still the one issue with this PR re #841 (comment)

[mbecker20]

Anyways, that's just an addition, this PR looks fine and will add the other part in follow up commit, thanks