Skip to content

Conversation

@whoisarpit
Copy link
Contributor

PR Checklist

  • The commit message follows our guidelines: Code of conduct
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Does this PR introduce a breaking change?
  • Include PR in release notes?

PR Type

  • Bugfix
  • Feature
  • Refactoring
  • Build /CI
  • Documentation
  • Others

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Introduces a new step BrowserUse that allows you to use the browser to interact with websites and optionally get a result from it.

Other information

@whoisarpit whoisarpit requested a review from CTY-git February 26, 2025 10:38
@whoisarpit whoisarpit changed the title Feature/browser use Introduce BrowserUse step Feb 26, 2025
@patched-admin
Copy link
Contributor

File Changed: .github/workflows/release.yml

Rule 2: Do not overlook possible security vulnerabilities

Details: The workflow uses the actions/setup-python@v5 action which is a good practice as it uses a specific major version. However, not pinning to a specific hash of the action could potentially lead to supply chain attacks if the v5 tag is compromised. Consider using the SHA hash of the action for enhanced security.

Affected Code Snippet:

      - name: Setup Python
        id: setup-python
        uses: actions/setup-python@v5

Start Line: 17

End Line: 19


Rule 3: Do not deviate from the original coding standards

Details: The changes introduce inconsistency in quote usage. The original file used single quotes ('v*.*.*', '3.9'), while the changes switch to double quotes ("v*.*.*", "3.11"). This deviates from the established coding standard in the original file.

Affected Code Snippet:

     tags:
-      - 'v*.*.*'
+      - "v*.*.*"

Start Line: 4

End Line: 6

Second instance:

-          python-version: '3.9'
+          python-version: "3.11"

Start Line: 17

End Line: 19

File Changed: .github/workflows/test.yml

Rule 2: Do not overlook possible security vulnerabilities
Details: Found potential security vulnerability - API keys are exposed in command line arguments which could be logged in CI/CD output or error messages. Consider passing sensitive information through environment variables instead.

Affected Code Snippet:

patchwork GenerateREADME --log debug \
--patched_api_key=$https://github.com/patched-codes/patchwork/pull/1338/files#diff-9a4e1b5185aca60ac378253dd90a488e3b1858536e09d72d694490c065248833 \
--github_api_key=$https://github.com/patched-codes/patchwork/pull/1338/files#diff-dcfdde616c57a27ef1c8dcb483375e99d5bc36af829f9834e372718bdf91a893 \

Start Line: 216
End Line: 218

Similar patterns found in lines:

  • 169-171 (GenerateDocstring command)
  • 178-180 (GenerateDiagram command)
  • 186-188 (GenerateCodeReview command)
  • 196-198 (GenerateCodeUsageExample command)

Rule 3: Do not deviate from original coding standards
Details: Found a minor coding standard inconsistency - inconsistent spacing around colon in YAML. The original file had no space before colon in task names, but line 171 was changed to add a space.

Affected Code Snippet:

- name : Generate Diagram

Start Line: 174
End Line: 174

The standard format used elsewhere in the file is:

- name: Task Name

File Changed: patchwork/common/client/llm/google.py

Rule 1: Do not ignore potential bugs in the code

Details: The code change from if file is NotGiven to if isinstance(file, NotGiven) is actually fixing a potential bug rather than introducing one. The original code used identity comparison (is) which might not always be the correct way to check for type instances. The new code properly uses isinstance() which is the Pythonic way to check types.

Affected Code Snippet: N/A
Start Line: N/A
End Line: N/A

File Changed: patchwork/steps/BrowserUse/BrowserUse.py

Rule 1: Do not ignore potential bugs in the code

Details: Several potential bugs were identified in the code:

  1. No browser cleanup/shutdown mechanism which could lead to resource leaks
  2. Lack of proper error handling for browser initialization
  3. Unclosed event loop in run() method

Affected Code Snippet:

def init_browser():
    global _browser, _controller
    if _browser is not None and _controller is not None:
        return _browser, _controller
    # ... initialization code ...
    _browser = Browser(config=config)
    _controller = controller
    return _browser, _controller

Start Line: 17
End Line: 52

Affected Code Snippet:

def run(self) -> dict:
    loop = asyncio.new_event_loop()
    self.history = loop.run_until_complete(agent.run())
    # Loop is never closed

Start Line: 164
End Line: 188

Rule 2: Do not overlook possible security vulnerabilities

Details: Multiple security vulnerabilities identified:

  1. disable_security=True in browser configuration is potentially dangerous
  2. File operations without path sanitization
  3. Direct file content reading without size limits or content validation

Affected Code Snippet:

config = BrowserConfig(
    headless=True, disable_security=True, new_context_config=context_config
)

Start Line: 42
End Line: 44

Affected Code Snippet:

@controller.action(description="Read the file content of a file given a path")
async def read_file(path: str):
    if not os.path.exists(path):
        return ActionResult(error=f"File {path} does not exist")
    with open(path, "r") as f:
        content = f.read()

Start Line: 90
End Line: 108

File Changed: patchwork/steps/BrowserUse/README.md

Rule 1: Do not ignore potential bugs in the code

Details: Several potential bugs are present in the code that should be addressed:

  1. No error handling for failed browser initialization or LLM API errors
  2. Unclosed browser and asyncio resources
  3. Unhandled case when no API keys are provided
  4. Potential KeyError when accessing self.inputs["debug"] as it's not defined in BrowserUseInputs

Affected Code Snippet:

def __init__(self, inputs):
    super().__init__(inputs)
    
    if not all(key in inputs.keys() for key in self.required_keys):
        raise ValueError(f'Missing required data: "{self.required_keys}"')

    self.browser = Browser(config=config)  # No error handling
    if "google_api_key" in self.inputs:
        self.llm = ChatGoogleGenerativeAI(...)
    elif "openai_api_key" in self.inputs:
        self.llm = ChatOpenAI(...)
    elif "anthropic_api_key" in self.inputs:
        self.llm = ChatAnthropic(...)
    # No else case for when no API keys are provided

Start Line: 73
End Line: 90


Rule 2: Do not overlook possible security vulnerabilities

Details: Several security concerns are present:

  1. API keys are passed directly as inputs without any validation or sanitization
  2. Debug mode creates files on disk without path validation or size limits
  3. No input validation for the task parameter which could lead to injection attacks
  4. No HTTPS/SSL verification settings mentioned for browser connections

Affected Code Snippet:

class BrowserUseInputs(TypedDict, total=False):
    task: str  # No input validation
    json_example_schema: str
    openai_api_key: Annotated[str, ...]  # No key validation
    anthropic_api_key: Annotated[str, ...]
    google_api_key: Annotated[str, ...]

Start Line: 28
End Line: 42

File Changed: patchwork/steps/BrowserUse/typed.py

Rule 1: Do not ignore potential bugs in the code

Details: Potential bug in API key configuration logic. The or_op configuration allows any combination of API keys, but there's no validation to ensure at least one API key is provided. This could lead to runtime errors if no API key is specified.

Affected Code Snippet:

    openai_api_key: Annotated[
        str,
        StepTypeConfig(is_config=True, or_op=["google_api_key", "anthropic_api_key"]),
    ]
    anthropic_api_key: Annotated[
        str, StepTypeConfig(is_config=True, or_op=["google_api_key", "openai_api_key"])
    ]
    google_api_key: Annotated[
        str,
        StepTypeConfig(is_config=True, or_op=["openai_api_key", "anthropic_api_key"]),
    ]

Start Line: 8
End Line: 19


Rule 2: Do not overlook possible security vulnerabilities

Details: Security vulnerability in API key handling. The TypedDict is marked with total=False, meaning all fields are optional. This could lead to unintended exposure or logging of API keys if not properly handled, and there's no indication of secret management or encryption for these sensitive credentials.

Affected Code Snippet:

class BrowserUseInputs(TypedDict, total=False):
    task: str
    example_json: str
    openai_api_key: Annotated[
        str,
        StepTypeConfig(is_config=True, or_op=["google_api_key", "anthropic_api_key"]),
    ]
    anthropic_api_key: Annotated[
        str, StepTypeConfig(is_config=True, or_op=["google_api_key", "openai_api_key"])
    ]
    google_api_key: Annotated[
        str,
        StepTypeConfig(is_config=True, or_op=["openai_api_key", "anthropic_api_key"]),
    ]

Start Line: 6
End Line: 19

File Changed: patchwork/steps/__init__.py

Rule 2: Do not overlook possible security vulnerabilities

Details: Potential security concern identified. The addition of BrowserUse module could introduce security vulnerabilities if not properly sandboxed or restricted. Browser automation can be a vector for security issues if not properly configured, especially regarding:

  • JavaScript execution
  • Cookie handling
  • Access to sensitive web content
  • Cross-origin requests

Affected Code Snippet:

from patchwork.steps.BrowserUse.BrowserUse import BrowserUse

Start Line: 51

End Line: 51

File Changed: pyproject.toml

Rule 1: Do not ignore potential bugs in the code

Details: Potential bug risk identified in dependency constraints. The browser-use package has a Python version constraint (>=3.11) while other packages don't specify Python version requirements. This could lead to version compatibility issues.

Affected Code Snippet:

browser-use = [{ version = "~0.1.40", python = ">=3.11", optional = true }]

Start Line: 14
End Line: 14


Rule 2: Do not overlook possible security vulnerabilities

Details: Security concern identified with adding multiple AI/ML related dependencies (langchain-google-genai, langchain-openai, langchain-anthropic) and browser automation tools (playwright) without explicit version upper bounds. Using the ~ operator only locks the minor version but allows patch updates, while ^ allows minor version updates. This could potentially lead to automatic updates bringing in security vulnerabilities.

Affected Code Snippet:

browser-use = [{ version = "~0.1.40", python = ">=3.11", optional = true }]
playwright = [{ version = "~1.50.0", optional = true }]
langchain-google-genai = [{ version = "^2.0.11", optional = true }]
langchain-openai = [{ version = "~0.3.1", optional = true }]
langchain-anthropic = [{ version = "~0.3.3", optional = true }]

Start Line: 14
End Line: 18

@whoisarpit whoisarpit merged commit cc113b9 into main Mar 2, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants