fix: address code audit findings (C1-C4, I1, I4, I6)#981
Merged
Conversation
Security: - C1: Fix page.evaluate injection in browser type/select commands and 6 adapter files by using JSON.stringify for user input interpolation - C2: Close WebSocket on CDP connect timeout to prevent resource leak - C3: Reject CDP connect promise on Page.enable failure instead of silently swallowing the error Reliability: - C4: Guard against corrupted adapter-manifest.json hashes to prevent false-positive override deletion - I1: Throw on pre-navigation failure instead of warn-and-continue - I4: Use Map<string, Promise<void>> for lazy module loading to prevent concurrent double-imports of the same adapter Performance: - I6: Replace O(n) registry alias cleanup with O(k) direct deletion
- C1: add quotes around CSS selector attribute values in browser type/select to match other commands (get text/value/attributes) - C2: clear this._ws in timeout handler to prevent race with open event - C4: refine corruption guard — treat null/undefined hashes as empty, only skip sync for truly invalid types (string, number, array)
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
1 participant
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.
Summary
Fixes 7 issues identified in the systematic code audit:
Security (Critical)
page.evaluateinjection inbrowser type/browser selectcommands and 6 adapter files (barchart/flow,barchart/greeks,barchart/options,barchart/quote,reuters/search,yahoo-finance/quote). User input was interpolated directly into JS template literals — now usesJSON.stringify().src/browser/cdp.ts.Page.enablefailure instead of silently swallowing the error.Reliability (Critical + Important)
adapter-manifest.jsonhashes to prevent false-positive override deletion inscripts/fetch-adapters.js.CommandExecutionErroron pre-navigation failure instead of warn-and-continue insrc/execution.ts.Map<string, Promise<void>>for lazy module loading to prevent concurrent double-imports of the same adapter.Performance (Important)
src/registry.ts.Test plan
opencli browser typewith special characters in selector indexopencli barchart flow/yahoo-finance quote AAPLwith quotes in args