sourcegraph/src-cli

Replace `_` with `-` in custom headers

nihaals opened this issue · 4 comments

Headers usually use - as opposed to _, and some auth proxies may only support -. As setting environment variables with dashes is more complex than underscores, maybe it makes sense to replace them. This would technically be a breaking change if an auth proxy only supported underscores.

An example of a case where the proxy does not support underscores but does support dashes is Cloudflare Access (with service tokens, more info).

Hey @nihaals! What custom headers are you referring to? I don't see where we set headers with an underscore in them and if you pass in custom headers to be sent along you decide what they look like.

What custom headers are you referring to?

Ah, I realise I had an indirect link in the Cloudflare forum post but wasn't actually explicit here. I'm referring to the SRC_HEADER_ environment variables as documented in the auth proxy docs.

if you pass in custom headers to be sent along you decide what they look like

I think my issue is around how you actually set environment variables containing dashes. I think the way it works now makes the most sense from a code point of view and initially makes sense from a UX view, but environment variables with dashes can be a pain when using a shell as opposed to setting them from code (like in the test you linked) and wasn't something I considered until I first actually tried it.

Pretty much the only way to do it from a shell that I can find is using env to set the variables, which feels like a lot more friction than being able to use e.g. export. I can't really think of a perfect solution as replacing all underscores with dashes could mean this same problem happens but the other way around and it becomes impossible to work around, and having another environment variable that toggles replacing all underscores with dashes (so the OP but behind a flag set with an environment variable) might cause issues if you need a mix.

I can only think of 2 approaches that don't break those:

  1. A per-header toggle, e.g. SRC_HEADER_HEADER_NAME_DASHES=1 which matches with SRC_HEADER_HEADER_NAME=header_value, which feels annoying to work with
  2. Using a character that is easy to set with e.g. export and is invalid in header names, which can safely be universally replaced with dashes (so OP but instead of replacing _ with -, it would be this invalid-for-header-names character)

Since it is possible, maybe this feels like scope creep and should be the responsibility of something external to src, but I think from a UX perspective, these could be useful, and there could potentially be cases where using env isn't an easy option(?).

Thanks for the reply. I see the problem now. But like you I'm unsure of what the best solution is. I agree with the additional toggle being a bit much and I also want to be careful of blowing up the complexity of this feature.

Maybe we could introduce something like SRC_HEADERS which contains a newline-separated list of headers? SRC_HEADERS="My-cool-header: one\nMy-other-cool-header: two". Newlines aren't the most ergnomic, but would allow us to skip comma/semicolon-related issues with header-values using them.

Maybe we could introduce something like SRC_HEADERS which contains a newline-separated list of headers? SRC_HEADERS="My-cool-header: one\nMy-other-cool-header: two".

I think this would be an interesting approach, although it would need to be tested in bash, zsh and fish to make sure it's ergonomic and not just as awkward as using env.