Repository navigation
Conversation
|
🚨 SLOP COP 🚨 · I am SlopCop. I started the review for PR #1312. I will check security, code quality, performance, architecture, and practical test coverage. |
| const repoViewArgs = [ | ||
| "repo", | ||
| "view", | ||
| remoteUrl, |
There was a problem hiding this comment.
🚨 slopcop/review — High: Validate the remote host before you give this URL to gh repo view.
A workspace controls remoteUrl. The daemon also gives its GH_ENTERPRISE_TOKEN to gh.
I reproduced this issue with a dummy token and https://httpbin.org/owner/repo.git.
gh sent a GraphQL request to httpbin.org with an authorization header.
An agent can change .git/config and set a renamed upstream on a hostile remote. The automatic PR check then contacts that remote.
This path can expose an enterprise token. It also permits a server-side request to an arbitrary host.
Accept only the trusted GitHub host for the base repository. Add a test with a hostile remote and a dummy token.
There was a problem hiding this comment.
Addressed in 930ade3. Configured remote URLs are now parsed locally and are never passed to gh. The upstream host must match the origin host before BB constructs the owner:branch selector. The regression test sets a dummy enterprise token and a hostile httpbin remote, then verifies gh is never invoked.
| ]; | ||
| let repoViewStdout: string; | ||
| try { | ||
| ({ stdout: repoViewStdout } = await execFileAsync("gh", repoViewArgs, { |
There was a problem hiding this comment.
🚨 slopcop/review — Medium: Cache the canonical repository for each remote URL.
The app checks an open PR with pending checks every five seconds.
This path now starts two Git processes and one serial gh repo view request before each gh pr view request.
One active environment can add 720 network requests and 1,440 Git processes each hour.
Use a bounded cache by remote URL. Remove the entry when the URL changes.
There was a problem hiding this comment.
Addressed in 930ade3 by removing canonical repository lookup over the network entirely. Upstream branch config plus origin/upstream URLs are now read in one local git config process, followed only by the existing gh pr view request. A regression test changes the remote URL between lookups and verifies the next selector changes immediately.
| export async function getPullRequestForCurrentBranch( | ||
| args: GetPullRequestForCurrentBranchArgs, | ||
| ): Promise<GitHostPullRequestLookup> { | ||
| const target = await getPullRequestTarget(args); |
There was a problem hiding this comment.
🚨 slopcop/review — Medium: Use this resolved target for PR actions too.
The lookup now finds the renamed upstream. However, ready, draft, and merge still run gh without this target.
For local-name tracking fork-owner:upstream-name, those actions search for fork-owner:local-name and fail.
Pass localBranch into the action path. Use the same target selector for lookup and actions.
Add action tests for a differently named upstream branch.
There was a problem hiding this comment.
Addressed in 930ade3. Lookup and actions now share the same target resolver, Workspace passes the local branch into the action path, and ready, draft, and merge receive the qualified upstream selector. The real-Git integration test covers all three action argument lists.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5
Think of two labels on one toy box.
This PR helps BB use the second label to find the box. That part works.
Some buttons still use the first label. A bad address can also receive a secret key.
BB also repeats extra work every five seconds.
Review result
I found one high issue and two medium issues.
-
High security: A workspace remote can make
ghsendGH_ENTERPRISE_TOKENto an arbitrary host. This issue blocks a safe merge. -
Medium correctness: The ready, draft, and merge actions do not use the new upstream target. They can fail after the lookup succeeds.
-
Medium performance: Each renamed-upstream refresh adds a serial network request and two Git processes. Pending checks can cause a refresh every five seconds.
The new target resolver should serve both lookup and actions. A bounded cache should own the canonical repository result for each remote URL.
I found no other security, quality, duplication, naming, or test issue.
Validation
-
The live dev app found cross-repository PR #1236 from a local branch with a different name.
-
I did not select the ready, draft, or merge actions.
-
A dummy-token probe showed that
gh repo viewsent an authorization header tohttpbin.org. -
Turbo passed 189 host-workspace tests and 49 contract tests.
-
Turbo passed the host-workspace, host-daemon-contract, and host-daemon type checks.
-
git diff --checkpassed. -
All GitHub checks passed for the reviewed SHA.
I submitted a comment-only review. I did not approve the PR or request changes.
996eab5 to
a5ea63c
Compare
Summary
Validation