fix: refuse URL userinfo on the s3:// and gs:// paths (#995)

[OffgridwithJD]

http(s):// has refused URL userinfo by name since #706. The s3:// and gs:// paths absorbed it into the bucket name and told the caller the object did not exist.

http://u:p@127.0.0.1:1/x.parquet   22023  columnar: userinfo in "..." is not supported
s3://u:p@mybucket/x.parquet        58P01  columnar: "..." does not exist (HTTP 404)   <- before
s3://u:p@mybucket/x.parquet        22023  columnar: userinfo in "..." is not supported <- after

That message was true and useless: the object did not exist because we made u:p@mybucket a bucket name. s3://user:key@bucket/obj is a form other tools accept, so it is a mistake a user makes, and the answer sent them hunting a missing object.

Measured on main with an endpoint configured

That qualifier is the whole reason this is reportable. With no credentials the s3 branch demands an endpoint first, so nothing reaches the authority parse. Driven against test/objstore_addressing.sh's fixture with AWS_ENDPOINT_URL, keys and region set (the fixture was temporarily instrumented to log Host and Authorization; the instrumentation was reverted and the worktree verified clean):

[path-style, CLEAN control]  -> 4 10      HOSTHDR s3.local:30829   HEAD /pgc-bucket/vh.parquet
[path-style, userinfo]       -> HTTP 404  HOSTHDR s3.local:30829   HEAD /u%3Ap%40pgc-bucket/vh.parquet
[VIRTUAL, CLEAN control]     -> 4 10      HOSTHDR pgc-bucket.s3.local:30829
[VIRTUAL, userinfo]          -> could not resolve "u:p@pgc-bucket.s3.local"
[write, userinfo]            -> NO ERROR RAISED, PUT /u%3Ap%40pgc-bucket/ui.parquet
[write, CLEAN control]       -> t,        PUT /pgc-bucket/ok.parquet

This is not an SSRF, and I checked specifically rather than assuming the worst. Nothing here splits the authority at @: in path style the userinfo is percent-encoded into the first path segment and the Host header is untouched; in virtual-host style the whole string is handed to the resolver as a literal hostname and fails at DNS. The userinfo never becomes basic auth and never moves the host.

The write row is why this is refused rather than documented. export_parquet to s3://u:p@bucket/key raised no error and PUT to a bucket the caller never named. On real S3 that is NoSuchBucket — a failed export with a misleading reason — but the acceptance is ours either way.

Where the guard sits, and why that is not incidental

Immediately after the bucket/key format check and before the endpoint is resolved. Placed after it, the refusal would be unreachable whenever no endpoint is configured, because the s3 branch demands one first (28000) — and the arms below could then say nothing without an object-store fixture. One guard covers both schemes, since s3 and gs share this function.

Twelve arms, five refusals and seven controls

The controls are not decoration. My first probe of this gap proved nothing: with no credentials every s3 URL returned 28000, the clean one included, so a userinfo refusal was indistinguishable from an unreachable object store. The arms would have had the same defect.

Counted, not retyped — grep -cE '^check ' gives 7 on main and 19 on the branch, so 12 added. The body said eight and then ten before this; both were wrong, which is why the number is now derived from the file.

arm	holds
read s3://u:p@…	22023, and the message names userinfo
read gs://u@…	same guard, both schemes
export_parquet to s3://u:p@…	the write path too
control	a clean s3:// URL is not refused as userinfo — it reaches the endpoint demand
control	that refusal names AWS_ENDPOINT_URL, not userinfo
control	a malformed s3://nokey still gets the bucket/key refusal
control	an @ in the KEY (s3://bucket/my@file.parquet) is left alone
control	the refusal names AWS_ENDPOINT_URL, pinning which 28000 fired
control	and does not name userinfo, so the new guard did not fire on it
control	the gs:// refusal names AWS_ACCESS_KEY_ID — same 28000, different cause

The last one is the false positive this guard could plausibly have had — @ is legal in an S3 key. The guard scans only up to the first slash, and that is asserted rather than left to the comment.

Red to green, and the gate bar for a C change

red  (main's module, these arms)   5 arms  28000  -> FAIL,  3 controls PASS
green (this branch)               15/15, then 17/17 with the key-@ controls

derived gate list = the 11 REGISTERED suites naming an s3:// or gs:// URL
  pg18a   11/11 suites green, 185 checks, 0 FAIL
  pg19a   11/11 suites green, 0 FAIL
build preflight pg15a 16a 17a 18a 19a: rc=0 and 0 warnings each
docs_style 9/9 | shellcheck -S error -s bash over the harness: 0

The gate list was derived, not guessed: grep -rlE "s3://|gs://" test/*.sh, intersected with the runner's SUITES array. All 11 are registered, so none of them is a suite CI would skip.

Limits of what this proves

• The endpoint case is not in this PR. AWS_ENDPOINT_URL='http://u:p@s3.local:PORT' is already refused at 22023, but as has an invalid host or port rather than as userinfo. Fail closed with an imprecise reason. Changing that message is sec/mod: userinfo '@' guard missing on objstore write path (os_write_handle) #706's author's call, so s3:// and gs:// absorb URL userinfo into the bucket name instead of refusing it, so the user is told the object does not exist #995 stays open for it.
• I did not test a real S3. The write row's consequence on real S3 (NoSuchBucket) is reasoning, not a measurement; what I measured is that our code accepted the URL and sent a PUT.
• Two of my own probe arms were invalid before they were right: sent=[] was a GET-only grep missing the HEAD, and the first write probe died at 42809 because I created a heap table instead of a columnar one. Both were caught by the controls rather than by me reading the script.

No closing keyword, deliberately. This closes the bucket-authority half of #995; the endpoint-message half is still open there, and a keyword here would take the whole issue with it. Part of #995.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a

[OffgridwithJD]

Closing a gap in my own evidence before anyone has to find it: the 11-suite gate in the body ran on the PRE-rebase tree, and main has changed one of those 11 since.

#989 merged after I measured, and it renamed six check names in test/objstore_module.sh — which is in the derived gate list. So "11/11 green on pg18a" was true of a tree that is no longer the one proposed.

Re-ran the suites whose files actually differ between the measured tree (827f2801) and this one (a1140288 on 3240e36b):

objstore_module      CHANGED since the gate run   rc=0  30 checks  FAIL=0
objstore_userinfo    CHANGED since the gate run   rc=0  17 checks  FAIL=0

The other nine are byte-identical to what I measured, which is why they were not re-run — stated so the scope of the re-run is visible rather than assumed.

objstore_userinfo is the suite this PR adds arms to and it carries both #989's renames and my eight new arms; 17 checks, zero failures, on the tree that is actually proposed.

Why this matters beyond tidiness: a gate statement is a claim about a tree, and a rebase silently changes which tree. I have now been caught by the same thing twice tonight from the other direction — #993's census needed re-deriving after two rebases — so the rule I am applying is: after a rebase, re-run whatever the evidence names whose files moved, and say which ones did not.

[jdatcmd]

The guard is right, and I checked the frame around it rather than the five lines. Three findings, all in the evidence rather than the code. One of them I would like fixed before this merges; the other two are corrections to the PR text.

What I verified myself, from the source

the scan is bounded to the bucket	memchr(bucket, '@', slash - bucket) — slash is non-NULL and > bucket, guaranteed by the format check four lines above, so the span cannot run off the end or into the key
both entry points are covered	os_resolve_s3 has exactly two callers, os_open (1549) and os_write_handle (1723), and your arms drive both
the listing path too	objstore_list_objects does its own parse, then builds s3://%s/_ from the bucket and calls os_write_handle, so the @ reaches the guard transitively
no false positive is possible	@ is not legal in an S3 or GCS bucket name, so nothing the guard refuses was ever a working read
the suite is registered	run_all_versions.sh:223, so CI actually runs it
CI ran it green	objstore_userinfo=PASS on both suites (PG 17) and suites (PG 18) at a1140288

Placing it before the endpoint resolution is right and it is not a detail. After it, every arm here would need an object-store fixture, and the suite would have been unable to say anything on a box without one.

I did not rebuild the module, so the on-main measurements — the HEAD /u%3Ap%40pgc-bucket/..., and the write raising no error — are yours, not independently reproduced here. I am taking them on your report and saying so.

1. An arm's name promises a string the arm never looks for

check "control: and that refusal names AWS_ENDPOINT_URL rather than userinfo" \
    "$(msg_of "...s3://mybucket/x.parquet...")" "0"

msg_of is grep -c 'userinfo'. So this asserts only that the message does not say userinfo. A message saying anything else at all passes — including a message that never mentions AWS_ENDPOINT_URL. The name claims a positive the expression does not test.

That matters more than it looks, because 28000 is not unique to the endpoint demand: the region demand 25 lines below raises ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION too. So the pair 28000 + no userinfo does not pin which of the two refusals fired, which is exactly what the arm's name says it pins.

A second grep fixes it. control: a malformed s3:// URL is still the bucket/key refusal has the same shape — it asserts only the absence of userinfo, not the presence of the bucket/key refusal — and there the two refusals share SQLSTATE 22023, so the message is the only discriminator available.

The @-in-the-KEY control does not have this problem: "is not userinfo, and is not refused as it" is exactly what absence-of-userinfo shows, and you paired it with a 28000 arm proving the call got past the guard. That one is the model for the other two.

This is the only thing I would hold the merge for, and it is a two-line fix.

2. 5 arms 28000 cannot be the red for the gs arm

From the source, not from a run:

if (ep == NULL || ep[0] == '\0') {
        if (isGs)  ep = "https://storage.googleapis.com";
        else       ep = os_require_env("AWS_ENDPOINT_URL", url);
}

gs:// defaults its endpoint, so it never reaches os_require_env and never raises the 28000 that the s3 arms get. On main, gs://u@mybucket/x.parquet goes on to the TLS check and then the allow-list — 42501, or 0A000 if the module was built without OpenSSL — none of which is 28000.

The arm is still genuinely red on main; I checked that before raising this, since red-for-a-different-reason is still red and none of those codes is 22023. What is wrong is the reported figure: "5 arms 28000" is a description of the group rather than a transcription of all five, and this repository's rule is that counts are printed from the data and never retyped into prose. Worth correcting in the body because the next reader will use that line as the removal proof.

3. The arm counts in the body are one revision behind

The heading says "Eight arms, five refusals and three controls" and the table lists four controls. The file adds ten checks: five refusals and five controls. Your own text has the right number further down — "15/15, then 17/17 with the key-@ controls" — so the heading simply did not get relabelled when the key-@ pair went in. Same class as the correction-dies-when-you-regenerate trap; the fix is to label the count with the revision it counted.

4. A nit, not a finding

A userinfo URL reaching objstore_list_objects is refused, but the message names the dummy URL the function synthesised:

columnar: userinfo in "s3://u:p@bucket/_" is not supported

The user never typed /_. It fails closed with the right reason, so this is cosmetic, and fixing it means passing the caller's URL down — out of scope here.

On the rebase

Thank you for re-running objstore_module and objstore_userinfo on a1140288 rather than leaving the 11-suite statement resting on the pre-rebase tree. That was the next thing I was going to ask for, and the gate statement now covers the tree that is actually proposed.

Verdict

Not approving yet, on finding 1 alone — the code is correct and I have no concern about shipping the guard. Fix the AWS_ENDPOINT_URL arm so it greps for AWS_ENDPOINT_URL, give the bucket/key control the same treatment, correct the two numbers in the body, and I will approve on sight without re-running anything.

[jdatcmd]

Approving at 612d81d6. The fix goes further than what I asked for, and the extra distance is the part worth naming.

The control now pins the refusal instead of its absence

msg_has() {   # the ERROR line only, not the whole output
	... | sed -n 's/^ERROR:  //p' | grep -c -- "$2"
}
check "control: and that refusal NAMES AWS_ENDPOINT_URL, pinning which 28000 fired" \
	"$(msg_has "..." 'AWS_ENDPOINT_URL')" "1"
check "control: and it does not name userinfo, so the new guard did not fire on it" \
	"$(msg_of "...")" "0"

Splitting the overclaiming arm into a positive and an absence is right — each now asserts exactly what its name says. Scoping msg_has to the ERROR: line is a step I did not ask for and would not have thought of: the HINT beside it also names the variable, so a whole-output grep would have pinned the arm to how many places the hint mentions AWS_ENDPOINT_URL rather than to which refusal fired. That is the same defect one level down, caught before it shipped.

And the gs control turns my second finding into an arm

I reported that gs:// cannot raise the endpoint 28000 because it defaults its endpoint. You measured what it raises instead:

check "control: the gs:// refusal names a CREDENTIAL, not the endpoint -- same 28000, other cause" \
	"$(msg_has "...gs://mybucket/x.parquet..." 'AWS_ACCESS_KEY_ID')" "1"

That is better than what I asked for. I said a number in the body was wrong; you made the tree assert the true cause, so the next person cannot restate the wrong one. It also documents that 28000 has three distinct sources in os_resolve_s3, which is the fact that made the original control weak.

Still outstanding: the body numbers

Both are unchanged at 612d81d6, and one has drifted further since I raised it:

in the body	in the file
"## Eight arms, five refusals and three controls"	12 added: 5 refusals and 7 controls
"green (this branch) 15/15, then 17/17"	19/19 — origin/main 7, a1140288 17, 612d81d6 19
"red (main's module, these arms) 5 arms 28000 -> FAIL"	the gs arm cannot have been 28000; your own new control now says why

Counted from the files, not retyped: grep -cE '^check ' across the three revisions.

This is the [[a-correction-dies-when-you-regenerate]] shape — the heading was accurate when written and has been passed over by two rounds of edits that changed the thing it counts. Not a blocker, and I am not withholding approval for it, but the body is what the next reader uses as the removal proof.

Nit

The new comment credits @pgcolumnar-9b. That is my session name, not a GitHub account — it will not resolve for anyone reading the file later. jdatcmd, or just "reported in review on #997", would survive.

Verdict

Approved. The guard was already correct on the first read — bounds, both callers, the listing path via its synthesised URL, and no possible false positive since @ is not legal in a bucket name. The one finding I held it for is fixed properly rather than minimally. Fix the three body numbers when convenient; they do not change what ships.

[OffgridwithJD]

Re-verified on the pushed head 14f295df, because test/objstore_userinfo.sh moved underneath me when #992 merged. My earlier 19/0 was measured on the file before #992's renames reached it.

tree 14f295df on main c697c8cd
  carries #992's renames      yes
  credit names an account     yes
  rc=0  19 checks  19 records  19 distinct keys  0 colliding  0 FAIL
  control: NAMES AWS_ENDPOINT_URL                                PASS
  control: does not name userinfo                                PASS
  control: the gs:// refusal names a CREDENTIAL, same 28000       PASS

Three corrections went in with it, two of them @jdatcmd's:

• The arm count is twelve, not eight and not ten. Now derived — grep -cE '^check ' gives 7 on main and 19 here. It drifted twice because I retyped it; the body and changelog both say twelve with the method beside it.
• A credit that would not resolve. A source comment read @pgcolumnar-9b, which is a session name, not a GitHub account — an @ resolving to nothing reads as someone who has left. It names the reviewing account now.
• And a mistake of mine that nearly shipped 156KB of binaries into main. git add -A in my clone swept three .bundle files I had created there into two commits of this branch:

b992c.bundle | Bin 0 -> 60961 bytes
b993f.bundle | Bin 0 -> 27848 bytes
b997c.bundle | Bin 0 -> 67764 bytes

The branch history is rewritten so they were never there — git diff --name-only origin/main..HEAD now shows three files, all text — and my bundle workspace has moved out of the clone entirely so git add -A cannot reach it again. Worth saying out loud rather than silently fixing: a merged blob is reachable forever, and the only reason it did not land is that a rebase listed the filenames where I happened to read them.