fix: refuse userinfo in an object-store endpoint (#995)

[OffgridwithJD]

Closes #995. #997 closed the caller's half — s3://u:p@bucket/key. This is the operator's half: userinfo in the endpoint, which the bucket guard cannot see because it is not in the URL at all.

Two shapes, and only one was ever caught

Measured on the authority parse before writing anything:

http://u:p@host:30829   -> host "u",          port 0       refused, wrong reason
http://user@host:30829  -> host "user@host",  port 30829   port VALID
http://host:30829       -> host "host",       port 30829   (clean control)

The first has a colon inside the userinfo, so the authority split lands there and the port becomes atoi("p@host:30829") = 0. The second has no such colon: the real port survives, the @ rides along in the host, and the invalid-port refusal never fires.

The issue measured the first shape and concluded "refused as invalid host or port". That is true of that shape rather than of the code.

The second shape is why this is a guard and not a message change

Its refusal came from the allow-list, naming the host it could not match, and the hint then told the operator:

ALTER SYSTEM SET pgcolumnar.objstore_allowed_endpoints = 'user@host'

A diagnostic that invites widening a security boundary to accommodate a parse bug is worse than a wrong error code. Following it moves the failure from the allow-list to a DNS miss — the operator has loosened their endpoint allow-list and still cannot read the object.

Placement: before the scheme and region demands

Not at the authority parse eighty lines later. Placed there the guard is unreachable whenever no region is configured, so an operator with a userinfo endpoint and no AWS_REGION is told about the region instead.

Measured — every endpoint arm returned requires a region option until it moved. That is the unreachable-guard trap the issue itself names, and the same reasoning the bucket guard carries for sitting before the endpoint is resolved at all.

The message names the endpoint rather than the URL, because that is the string carrying the userinfo; userinfo in "s3://bucket/key" would name a URL that has none.

Removal proof, on both harnesses

Each run asserts the .so was rebuilt and installed (mtime moved) before its verdict is read, and the source is restored to md5 0de468b1.

harness	control	guard removed	restored
objstore_userinfo.sh	26 passed	5 failed	26 passed
test_objstore_endpoint_userinfo.py	5 passed	3 failed	5 passed

Two traps walked into while measuring, both already in the record

PGC_SKIP_BUILD=0 SKIPS the build. lib.sh tests [ -z "${PGC_SKIP_BUILD:-}" ], so any non-empty value means skip — including 0, which reads as "do not skip". The first run measured a .so from the previous day and reported five failures against code that did not contain the guard. The suite printed PGC_SKIP_BUILD=1: not building AND NOT INSTALLING on line 5 of its own output and I did not read it; the timestamps did.

git checkout -- FILE reverted the uncommitted fix. The removal proof's restore took the file back to HEAD, which had no guard, so the final control failed identically to the mutant. Caught because the restore's md5 assertion did not fire. Fixed by committing before mutating.

Tests

Arms in test/objstore_userinfo.sh beside the existing ones, with a clean-endpoint control in the same run — without it these cannot tell a userinfo refusal from a foreign server that reaches nothing, which is exactly how the issue's first probe wasted a run.

And independently in test/pytest/test_objstore_endpoint_userinfo.py. That file is not a port and not a pair — objstore_endpoint_userinfo.sh does not exist. It asserts the same properties through the python harness, and nothing in it sources, invokes or reads anything under test/*.sh. It carries two arms the bash side does not: that the guard fires with no region configured (the placement claim, stated as a test rather than a comment), and that an @ in the object key is untouched — without which the arms would also pass on a guard that refused every @ anywhere.

objstore_userinfo.sh  26 checks, 0 failed   (pg16a assert build)
guard leg            340 passed, 867 checks, 0 fail
cluster leg          325 passed, 898 checks, 0 fail

cluster_tests 320 → 325 by collection.

One thing for @jdatcmd

The issue said "#706's author should say whether the endpoint message moves." I did not move the existing invalid host or port message — the guard now fires before that path is reached for any @-carrying endpoint, so that message is unchanged and still covers the genuine invalid-port cases. If you would rather the endpoint message read userinfo in "%s" to match the other three schemes exactly, rather than naming the endpoint, say so and I will change it.

🤖 Generated with Claude Code

https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs

[OffgridwithJD]

The open wording question is settled: the message stays as it is. Recording the reason here rather than leaving it in a side channel, because "why is this one phrased differently" is exactly the question a future reader will ask.

@jdatcmd's argument, and it is not stylistic:

the existing three   errmsg("columnar: userinfo in \"%s\" is not supported", url)
this one             errmsg("columnar: userinfo in object-store endpoint \"%s\" ...", ep)

The three name the URL the CALLER wrote. This one names the endpoint the OPERATOR configured. A caller who wrote a perfectly clean s3://bucket/key and got back userinfo in "s3://bucket/key" is not supported would be looking at a string with no userinfo in it, being told it has userinfo. Matching the wording exactly would make the message identical and the referent wrong, which is the worse of the two.

The version here also agrees with its own hint — the message names the endpoint, the hint says remove it from the endpoint — so message and hint point at the same object. That is the property worth preserving; uniform phrasing across a different subject is not.

If exact uniformity across all four is wanted, the right move is to make the subject explicit in all of them — userinfo in URL "%s" / userinfo in endpoint "%s" — rather than flattening this one into the caller-URL form. That is a separate change touching three existing messages and their arms, and I have not made it here.

CI is 14/14, 0 pending, 0 failures on 1850be81.

[jdatcmd]

Approving at 1850be81, 14/14. Being explicit about which parts I verified myself and which I am relying on you for, since I did not rebuild — the box is mid-measurement for #890.

Verified directly from the source

The placement is real and checkable from the line numbers:

1435  if (strchr(ep, '@') != NULL)        <- the guard
1479  "requires a region option"          <- the demand that made it unreachable

Guard above demand, which is what lets it fire with no region configured. That is the whole finding, and it is visible without running anything.

The arms cover the direction that matters. test_an_at_sign_in_the_object_key_is_not_userinfo is the one I would keep hardest — @ is legal in an S3 key, and without that arm every other arm here would pass equally well against a guard that refused any @ anywhere. That is "make it say the other thing" applied properly rather than described.

test_the_guard_fires_without_a_region_configured is the placement claim stated as a test instead of trusted to a comment. A comment saying "this must go above the region demand" rots; an arm that fails when it moves does not.

Relying on you for

The removal proof (26 -> 5 -> 26 and 5 -> 3 -> 5, each asserting the .so mtime moved first) and the two suites legs. I have not rebuilt, so those are your measurements, not mine. Flagging it rather than implying I re-ran them.

On the wording

Answered in the channel and you have already put it on the PR, which is the right place — "why is this one phrased differently" is what a future reader asks, and the answer belongs where they will look.

The traps

PGC_SKIP_BUILD=0 skipping the build is a genuinely nasty one: the value that reads as "do not skip" is the value that skips, and the suite announced it on line 5 of its own output. What caught it was the .so timestamps, not the announcement — a printed fact nobody reads is not a guard.

And git checkout -- FILE reverting an uncommitted fix, so the control fails identically to the mutant: that one produces a removal proof where both arms are red for the same reason, which reads as a working proof if you only check that the mutant reddened. Your restore md5 assertion is what made it visible.