Skip to content

Conversation

@bheylin
Copy link
Contributor

@bheylin bheylin commented Nov 7, 2025

I experienced a panic in production code at authorization:l159 for headers v 0.4.1.
I don't have logs of the actual header yet, but I know that the header value was length 3.

I propose returning a None if the prefix strip doesn't succeed.

@bheylin bheylin changed the title Use &[u8]::get instead of &[u8] to avoid panic in basic auth Use &[u8]::get() instead of &[u8]::index() to avoid panic in basic auth Nov 7, 2025
@seanmonstar
Copy link
Member

This should probably be fine, but I went to look, and the decode implementation for Authorization<C> should already make sure the bytes were long enough for the scheme... Were you using Credentials separate from the Authorization type?

@bheylin
Copy link
Contributor Author

bheylin commented Nov 7, 2025

The code that led to the panic is calling the Credentials::decode impl on headers::authorization::Basic directly, without any Authorization wrapper. So that explains why the panic occurred in this case.

Based on the code I read in Authorization::decode, I presume that you don't expect Basic::decode to be called directly from user code?

fn extract_auth_values(req: &mut axum::Request) -> Result<(uuid::Uuid, ApiPlainSecret), StatusCode> {
    let auth_header = req
        .headers()
        .get(http::header::AUTHORIZATION)
        .ok_or_else(|| {
            debug!("Authorization header not found");
            StatusCode::UNAUTHORIZED
        })?;

    let Some(auth) = headers::authorization::Basic::decode(auth_header) else {
        debug!("Failed to decode auth headers");
        return Err(StatusCode::UNAUTHORIZED);
    };
    
    let plain_secret = ApiPlainSecret::new(auth.password().to_owned());
    Ok((public_id, plain_secret))
}

@seanmonstar
Copy link
Member

Ah very interesting. Yea, I never originally meant for it to be used separately, but I didn't really prepare that or document or anything. Again, seems fine to fix that up. :)

@seanmonstar seanmonstar merged commit de0b1a1 into hyperium:master Nov 7, 2025
6 checks passed
@bheylin bheylin deleted the basic-auth-none-instead-of-panic branch November 9, 2025 16:14
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.

2 participants