Skip to content

Conversation

@kixa
Copy link
Contributor

@kixa kixa commented Oct 24, 2025

This implements From conversions on TwirpError for http::header::InvalidHeaderName and http::header::InvalidHeaderValue under the http flag for convenience when building custom channels.

For example, right now we do something like:

self.example_channel
  .clone()
  .with_user_agent("my-example-service_v1.1.0")
  .map_err(|err| {
    TwirpError::invalid_argument(format!("invalid header value: {err}"))
  })?
  .with_request_id(&context.request_id())
  .map_err(|err| {
    TwirpError::invalid_argument(format!("invalid header value: {err}"))
  })?;

Where any custom header setter returns Result<Self, InvalidHeaderValue> for protecting against invalid header character (rather than panicking). This would allow us to ? each of those instead.

…dHeaderName and http::header::InvalidHeaderValue
@kixa kixa changed the title add: implement From conversion to TwirpError for http::header::Invali… add: implement From conversion to TwirpError for http header errors Oct 24, 2025
@kixa kixa changed the title add: implement From conversion to TwirpError for http header errors add: implement From conversions on TwirpError for http header errors Oct 24, 2025
@kixa kixa changed the title add: implement From conversions on TwirpError for http header errors add: implement From conversions on TwirpError for http header errors Oct 24, 2025
@vsiles
Copy link
Contributor

vsiles commented Oct 24, 2025

LGTM ! Leaving till Monday to make sur @Tpt has a look too

Copy link
Collaborator

@Tpt Tpt left a comment

Choose a reason for hiding this comment

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

Not sure about the error type. My guess is that it's likely that this error is not raised because the user request is wrong but because of bad data when building the response. So, an internal error might be better.

Also I am a bit scared this conversion code might be used for the authentication header, leading to tokens being logged somewhere.

Given these two unclear elements, I would tend to slightly prefer that we don't have these implementations and let user code deal with them (just .unwrap() is likely fine in a lot of cases)

Edit: the error do not contain the actual header name/value.

@vsiles
Copy link
Contributor

vsiles commented Oct 27, 2025

Maybe let's only keep the wrapper for invalid header name ?

@Tpt
Copy link
Collaborator

Tpt commented Oct 27, 2025

I just checked again http and the content is not in the error so there is no security risk. However I am still puzzled by the error type (a bad header on the input is a malformed header but because this is validated by hyper, the only case this error will be used is likely when building the output, so "internal" might make more sense).

BTW, we might improve error formatting a bit, the Display out from InvalidHeaderNameis always "invalid HTTP header name" and from InvalidHeaderValue "failed to parse header value"

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.

4 participants