Adversarial review: close a credential leak, a path-traversal bypass, and three more - #11
Merged
Merged
Conversation
…defects A five-lens panel reviewed the shipped code. Its stale findings (already fixed in #9) are excluded; these are the ones that reproduced. CRITICAL - the scrubber leaked the credential it existed to hide. The named-secret pattern captured only the first whitespace-delimited token after a key, so "Authorization: Bearer <token>" redacted the word "Bearer" and passed the token through in full. Reachable from two live paths: Databricks CLI stderr scrubbing and every GenieException message. Proven by twelve evasion tests, seven of which failed before the fix. The earlier tests all used single-token values with no embedded space, which is exactly the shape the pattern handled - they confirmed it worked on convenient input rather than trying to defeat it. HIGH - a symlink or Windows junction beside a pack file defeated the output-path guard. The check was purely lexical, so "link/report.md" passed every string comparison while the write followed the reparse point outside the pack directory: traversal with no ".." at all. Each component between root and target is now resolved. Reproduced with a real junction before and after. HIGH - YamlDotNet assigns its own dictionary over the field initializer, discarding the OrdinalIgnoreCase comparer, so an alias written "Finance:" did not match "--agent finance". Resolution then fell through to title matching, which can select a different Agent entirely - the answer-against-the-wrong-data failure the resolver exists to prevent. The comparer is rebuilt after load. HIGH - timestamps were parsed as milliseconds unconditionally, though the project's own API notes say the unit is undocumented and the one published example is seconds. On a seconds value that yields 1970, a silent 55-year error on a public property. Now detected by magnitude. MEDIUM - EXTERNAL_LINKS results omit data_array entirely, which would have returned zero rows as a complete success. Refused loudly instead; an empty-looking success is worse than a failure. Also: the CSV formula guard missed a leading tab or carriage return, both of which OWASP documents as accepted prefixes. Two false claims corrected. --verbose was registered but never read while its help text promised "credentials are always redacted"; it now emits diagnostics through a stderr logger that scrubs every record, verified live against a real workspace with a grep for the token. compatibility.md asserted a live verification in one section and "no live workspace has been contacted" seventeen lines later; SECURITY.md referenced a --debug flag that never existed. Also dropped a dead ProjectReference from QuestionPacks to Application, which used nothing from it. 127 tests, up from 109.
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.
A five-lens panel reviewed the shipped code. Stale findings (already fixed in #9) excluded; these reproduced.
Critical — the scrubber leaked the credential it exists to hide
DiagnosticRedactioncaptured only the first whitespace-delimited token after a key name, soAuthorization: Bearer <token>redacted the word "Bearer" and passed the token through in full. Reachable from two live paths: Databricks CLI stderr scrubbing, and everyGenieExceptionmessage.Twelve evasion tests added; seven failed before the fix. The pre-existing tests all used single-token values with no embedded space — exactly the shape the pattern handled. They confirmed it worked on convenient input rather than trying to defeat it.
High — a symlink/junction defeated the pack path guard
The containment check was purely lexical, so
link/report.mdpassed every string comparison while the write followed a reparse point outside the pack directory — traversal with no..at all. Each component between root and target is now resolved. Reproduced with a real Windows junction before and after.High — YAML discarded the case-insensitive comparer
YamlDotNet assigns its own dictionary over the field initializer, so an alias written
Finance:did not match--agent finance. Resolution then fell through to title matching, which can select a different Agent — the answer-against-the-wrong-data failure the resolver exists to prevent.High — timestamps parsed as milliseconds unconditionally
The project's own API notes say the unit is undocumented and the one published example is seconds. On a seconds value that yields 1970 — a silent 55-year error on a public property. Now detected by magnitude.
Medium
EXTERNAL_LINKSresults omitdata_arrayentirely, which would have returned zero rows as a complete success; now refused loudly, because an empty-looking success is worse than a failure. The CSV formula guard missed a leading tab or carriage return, both OWASP-documented prefixes.Two false claims corrected
--verbosewas registered but never read, while its help text promised "credentials are always redacted". It now emits diagnostics through a stderr logger that scrubs every record — verified live against a real workspace by grepping the output for the token. Anddocs/compatibility.mdasserted a live verification in one section while stating "no live workspace has been contacted" seventeen lines later.Also dropped a dead
ProjectReferencefrom QuestionPacks to Application, which used nothing from it.127 tests, up from 109.