--custom-header option doesn't work with only one header

[Timost]

Hi ! Thank you for jupyterhub 🙏 ❤️ !
I've been stumbling on a bug with this proxy. Probably introduced with #223

Bug description

The --custom-header flag doesn't work in version v4.5.0.
Running

Proxy cmd: ['configurable-http-proxy', '--custom-header', 'testcustomheader:mytestcustomheader', '--log-level', 'debug', '--ip', '0.0.0.0', '--port', '8000', '--api-ip', '127.0.0.1', '--api-port', '8001', '--error-target', 'http://127.0.0.1:8081/hub/error']

Doesn't add the testcustomheader:mytestcustomheader header to proxified requests.

Expected behaviour

The headers contain the custom header set with the command-line option.

Actual behaviour

The headers don't contain the additional header. More over the parsed args are wrong:

Args: {
  apiIp: '127.0.0.1',
  xForward: true,
  prependPath: true,
  includePrefix: true,
  customHeader: undefined,
  metricsIp: '',
  logLevel: 'debug',
  ip: '0.0.0.0',
  port: 8000,
  apiPort: 8001,
  errorTarget: 'http://127.0.0.1:8081/hub/error'
}

notice the undefined for the customHeader option.

How to reproduce

I've stumbled on this bug while using jupyterhub, so the setup is quite intricate and I don't have an easily reproducible setup at hand.
I can work on it though if needed.

Your personal set up

I'm using configurable-http-proxy with jupyterhub with a non configurable reverse-proxy in front. So I can't setup the X-Forwarded as recommended by the docs. So I was trying to enforce them directly in configurable-http-proxy using the --custom-header flag.

• OS: docker debian bullseye

• Version(s): jupyterhub 2.0.2

• Configuration

# jupyterhub_config.py
import sys
c.Application.log_level = 'DEBUG'
c.JupyterHub.authenticate_prometheus = False
c.JupyterHub.bind_url = 'http://0.0.0.0:8000'
c.JupyterHub.load_roles = [
    {
        "name": "jupyterhub-idle-culler-role",
        "scopes": [
            "list:users",
            "read:users:activity",
            "read:servers",
            "delete:servers",
            # "admin:users", # if using --cull-users
        ],
        # assignment of role's permissions to:
        "services": ["jupyterhub-idle-culler-service"],
    }
]
c.ConfigurableHTTPProxy.command = [
    'configurable-http-proxy',
    '--custom-header',
    'testcustomheader:mytestcustomheader',
    '--log-level', 'debug',
]
c.JupyterHub.services = [
    {
        "name": "jupyterhub-idle-culler-service",
        "command": [
            sys.executable,
            "-m", "jupyterhub_idle_culler",
            "--timeout=3600",
        ],
        # "admin": True,
    }
]
c.Spawner.debug = True

Proposed fix

After some fidling, it appears the collectHeadersIntoObject is missing a return statement (and also calls an undefined log function in case of error).
Adding it fixes the issue.

function collectHeadersIntoObject(value, previous) {
  var headerParts = value.split(":").map((p) => p.trim());
  if (headerParts.length != 2) {
    console.error("A single colon was expected in custom header: " + value);
    process.exit(1);
  }
  previous[headerParts[0]] = headerParts[1];
  return previous;
}

I'm willing to submit a PR if you are accepting them.

[welcome]

Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please try to follow the issue template as it helps other other community members to contribute more effectively.

You can meet the other Jovyans by joining our Discourse forum. There is also an intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[consideRatio]

Thanks for this investigation @Timost. Can you also clarify what version of node is used btw?

Adding return previous statement

Your proposal sounds great, looking at https://github.com/tj/commander.js/#custom-option-processing I see that they do indeed also provide an example to clarify one should return a value.

I'm willing to submit a PR if you are accepting them.

Yes! 💯 🚀 🎉 ❤️

Confusion why this test didn't fail

I'm confused, as we seem to have a test to verify there are headers in some place at least...

configurable-http-proxy/test/cli_spec.js

Lines 217 to 247 in ac921c4

	it("custom-header", function (done) {
	var args = [
	"--ip",
	"127.0.0.1",
	"--ssl-cert",
	"test/server.crt",
	"--ssl-key",
	"test/server.key",
	"--port",
	port,
	"--default-target",
	testUrl,
	"--custom-header",
	"k1: v1",
	"--custom-header",
	" k2 : v2 v2 ",
	];
	executeCLI(execCmd, args).then((cliProcess) => {
	childProcess = cliProcess;
	r(SSLproxyUrl).then((body) => {
	body = JSON.parse(body);
	expect(body.headers).toEqual(
	jasmine.objectContaining({
	k1: "v1",
	k2: "v2 v2",
	})
	);
	done();
	});
	});
	});

Hmm... --custom-header should be headers are to be added to the proxied HTTP request, not the proxied response - is it perhaps so that we add them to the proxied response rather than the proxied request? I think the test confirms that headers are added at one point, but to what?

Related

• Command line option for custom headers #206
• fix: --custom-header flag implementation #242

[consideRatio]

@minrk do you have an idea why our test succeeded?

[Timost]

No problem ! Thank you for the quick answer 💪

Can you also clarify what version of node is used btw?

Sure it's node 16 from this docker image lts-bullseye

I'm confused, as we seem to have a test to verify there are headers in some place at least...

Yeah I read #242, and I was also surprised about that. I didn't spend time to dig into the issue though. 😅

Regarding the PR, is there some specific things about the dev setup/environement I should be aware of ?

[consideRatio]

Regarding the PR, is there some specific things about the dev setup/environement I should be aware of ?

Hmmm, I'm not sure, I've not developed code for this repo much. The CI/CD system can make a test for you without needing to set things up yourself, but if you do, I imagine you should use npm install and npm run test or similar.

[minrk]

Fixed by #369

The test passing is super weird, but reveals a workaround while you wait for a release: add a second --custom-header x-ignore: whatever and it should work! A bug that should have prevented it from ever setting any custom headers apparently only prevented it from setting exactly one custom header.

[Timost]

Wow thank you for looking into this and for #369.
Super weird bug indeed, I tried looking into commander's code but I couldn't pinpoint the problem.