Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getPatch: use github ssh url #211

Merged
merged 1 commit into from
Nov 21, 2024
Merged

getPatch: use github ssh url #211

merged 1 commit into from
Nov 21, 2024

Conversation

thypon
Copy link
Member

@thypon thypon commented Nov 21, 2024

No description provided.

@brave brave deleted a comment from github-actions bot Nov 21, 2024
@brave brave deleted a comment from github-actions bot Nov 21, 2024
@brave brave deleted a comment from github-actions bot Nov 21, 2024
Copy link

openai debug - [puLL-Merge] - brave/pull-merge@211

Description

This PR alters the fallback mechanism for obtaining the PR body when the diff retrieval fails. Previously, it made an additional API request to fetch repository details, but now it constructs the cloneUrl directly.

Possible Issues

  • The removal of the repository API request could potentially lead to issues if the owner or repo are malformed or incorrect within the cloneUrl. This was previously verified implicitly via the API call.

Security Hotspots

  1. Hardcoded cloneUrl Path: The construction of the cloneUrl directly might expose the system to command injection if the owner or repo parameters are not properly sanitized.
    • Risk: High
    • Mitigation: Validate and sanitize owner and repo parameters strictly.
Changes

Changes

  1. File: src/getPatch.js
    • Removed API request to fetch the repository details to obtain the clone_url. The URL is now directly constructed using a template string.
    • Modified cloneUrl construction to use an internal GitHub SSH URL directly.
sequenceDiagram
    participant Browser
    participant getPatchFunction
    participant GitHubAPI

    Browser ->> getPatchFunction: Request PR Data
    getPatchFunction ->> GitHubAPI: Get PR Diff
    GitHubAPI -->> getPatchFunction: PR Diff Fetched (Error Case)
    getPatchFunction ->> GitHubAPI: Get PR Details
    GitHubAPI -->> getPatchFunction: PR Details Fetched
    getPatchFunction ->> getPatchFunction: Construct Clone URL
    getPatchFunction ->> getPatchFunction: Clone Repository
    getPatchFunction -->> Browser: Return Processed Data
Loading
graph TD
    A[Browser Client] --> |Send PR Request| B[GitHub API]
    B --> |PR Diff Retrieved Successfully?| C{Success?}
    C --> |Yes| D[Return Response]
    C --> |No| E[Retrieve PR Details]
    E --> F[Construct Clone URL]
    F --> G[Clone Repository]
    G --> H[Process Repository]
    H --> I[Return Processed Data]
Loading

Copy link

anthropic debug - [puLL-Merge] - brave/pull-merge@211

Description

This PR modifies the getPatch function in the src/getPatch.js file. The main changes involve simplifying the error handling process when fetching a pull request's diff fails. Instead of making an additional API request to get repository details, the PR now directly constructs the Git clone URL using the owner and repo information.

Changes

Changes

  1. src/getPatch.js:
    • Removed the GitHub API request to fetch repository details (GET /repos/{owner}/{repo}).
    • Changed the construction of the cloneUrl from using the API response to directly formatting it with the owner and repo information.
    • Simplified the error handling flow by removing unnecessary steps.
sequenceDiagram
    participant getPatch
    participant GitHub API
    participant Local System

    getPatch->>GitHub API: Request PR diff
    alt Diff fetch successful
        GitHub API->>getPatch: Return diff
    else Diff fetch fails
        GitHub API->>getPatch: Error
        getPatch->>GitHub API: Request PR details
        GitHub API->>getPatch: PR details
        getPatch->>Local System: Construct clone URL
        getPatch->>Local System: Clone repository
        Local System->>getPatch: Repository cloned
        getPatch->>Local System: Generate diff manually
    end
Loading
graph TD
    A[getPatch function] --> B{Fetch PR diff}
    B -->|Success| C[Return diff]
    B -->|Failure| D[Fetch PR details]
    D --> E[Construct clone URL]
    E --> F[Clone repository]
    F --> G[Generate diff manually]
Loading

Copy link

bedrock debug - [puLL-Merge] - brave/pull-merge@211

Description

This PR modifies the getPatch function in the src/getPatch.js file. The changes aim to simplify the process of retrieving a pull request's diff when the initial attempt fails. Instead of making an additional API request to get repository details, the PR now directly constructs the clone URL using the owner and repo information.

Changes

Changes

  1. In src/getPatch.js:
    • Removed the GitHub API request to get repository details (GET /repos/{owner}/{repo}).
    • Changed the cloneUrl construction to use a template string with SSH format instead of using the clone_url from the removed API response.
    • Simplified the error handling flow by removing unnecessary steps.
sequenceDiagram
    participant Client
    participant getPatch
    participant GitHub API
    participant Git

    Client->>getPatch: Call with PR details
    getPatch->>GitHub API: Request diff
    alt Diff request succeeds
        GitHub API->>getPatch: Return diff
        getPatch->>Client: Return diff
    else Diff request fails
        GitHub API->>getPatch: Error
        getPatch->>GitHub API: Request PR details
        GitHub API->>getPatch: Return PR details
        getPatch->>Git: Clone repository
        Git->>getPatch: Repository cloned
        getPatch->>Client: Return manually obtained diff
    end
Loading
graph TD
    A[Start] --> B{Try to get diff}
    B -->|Success| C[Return diff]
    B -->|Fail| D[Get PR details]
    D --> E[Construct clone URL]
    E --> F[Clone repo]
    F --> G[End]
Loading

@thypon thypon merged commit b350f9f into main Nov 21, 2024
7 checks passed
@thypon thypon deleted the fix/github-url branch November 21, 2024 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant