Bound the chunk walk, and stop putting a secret in argv - #43
Merged
Conversation
Three findings from an independent adversarial review, each verified before being acted on. A response that supplies a fresh link every time while carrying no rows defeats every guard the walk had: nothing grows toward MaxResultRows, no link ever repeats, and a link is always present. The review drove 680,307 authenticated requests through that gap before a timeout cut it off. There is now a hard bound of 1,000 chunk requests per result. Removing it makes the new test hang rather than fail, which is how the bound was confirmed. The service-principal recipe passed the OAuth secret as `curl --user`, which is the form Databricks' own documentation shows. An argument is readable from the process table -- ps -ef, /proc/<pid>/cmdline -- by anything else on the machine while the request is in flight. On a shared or self-hosted runner, which is exactly what the recipe is written for, that loses a credential no amount of log masking protects. The credentials are piped in on stdin instead. Verified against the live endpoint that Databricks accepts them in the request body: wrong credentials return 401 invalid_client, the same as the --user form. The documented-command test stripped shell terminators off the raw string before tokenizing, so a '>' or '#' inside a quoted question cut the command in half. The dangerous direction is not the crash: `ask --agent sales "priced > 50?"` truncated to something that still PARSED, so the test went green having validated a string the documentation does not contain. It now tokenizes with System.CommandLine's own SplitCommandLine and drops terminator tokens, which deletes the hand-rolled tokenizer rather than patching it, and a regression test covers both characters. Also corrected: the comment on shortOfTotal claimed it catches a chunk being unreachable, which the loop already reports. It actually catches a walk that finished cleanly and still came up short of the manifest -- Databricks disagreeing with itself -- and now says so.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three findings from an independent adversarial review panel run over the merged cycle. Each was
verified before being acted on; none was taken on the reviewer's word.
1. The chunk walk was unbounded (security, medium)
Every guard the walk had could be defeated at once. A response that returns
next_chunk_index: N+1with a fresh
next_chunk_internal_linkanddata_array: []every time means:MaxResultRows— no rows are ever addedseenLinksguard never firesThe reviewer drove 680,307 authenticated requests through that gap before a five-second token
cut it off, with the
seenLinksset growing the whole time. Against a real workspace that is arequest flood.
My own commit message on #36 claimed "every way of failing to follow it still reports truncated:
no link, a link that is not a workspace path, a chunk the caller may not read, or the
MaxResultRowscap." This was a fifth way, and the claim was wrong.Fixed with a hard bound of 1,000 chunk requests per result — generous, since a chunk observed live
held ~41,000 rows. Removing the bound makes the new test hang rather than fail, which is how it
was confirmed.
2. The service-principal recipe put the OAuth secret in
argv(security, low)curl --user "$ID:$SECRET"— the form Databricks' own documentation shows — makes the secretreadable from the process table (
ps -ef,/proc/<pid>/cmdline) by anything else on the machinewhile the request runs. On a shared or self-hosted CI runner, exactly what this recipe is written
for, that loses a credential which never appears in any log. The workflow's
::add-mask::addresseslogging, not this.
Credentials are now piped in on stdin. Verified against the live endpoint that Databricks
accepts them in the request body: deliberately wrong credentials return
401 invalid_client, thesame as the
--userform.3. The documented-command test could pass on a string the docs do not contain
It stripped shell terminators off the raw string before tokenizing, so a
>or#inside aquoted question cut the command in half. The dangerous direction is not the crash:
truncated to
lakespeak ask --agent sales "What sold at a price— which still parses, becausethe remainder satisfies the required
questionargument. The test went green having validated astring the documentation does not contain, defeating its own stated purpose.
Fixed by tokenizing with
System.CommandLine's ownCommandLineParser.SplitCommandLineanddropping terminator tokens — which deletes the 44-line hand-rolled tokenizer rather than
patching it. A regression test covers both
>and#.Also
The comment on
shortOfTotalclaimed it catches an unreachable chunk — which the loop alreadyreports, making the check read as redundant. It actually catches a walk that finished cleanly and
still came up short of the manifest: Databricks disagreeing with itself. Corrected.
Verification
229 tests green (up from 226). Build clean under warnings-as-errors; format clean. The workflow
sample parses as YAML and its
runblocks passbash -n.Reviewers also confirmed several things as non-issues, worth recording:
ResolvesToWorkspacesurvived protocol-relative links, userinfo-in-authority in both directions, subdomain-suffix
spoofing, case and port variation, scheme downgrade, IPv6 literals and a Cyrillic homograph; and
.NET's
SocketsHttpHandlerstripsAuthorizationon any automatic redirect, so a validatedsame-host link that redirects cannot carry the bearer token off-host.