Skip to content

test: automated PR review for massive changesets - #29

Open
jescalada wants to merge 36 commits into
mainfrom
ui-refactor-review-automation-tests
Open

test: automated PR review for massive changesets#29
jescalada wants to merge 36 commits into
mainfrom
ui-refactor-review-automation-tests

Conversation

@jescalada

Copy link
Copy Markdown
Owner

Testing whether automated PR reviewer can handle large PRs and actual token costs

andypols added 30 commits June 5, 2026 18:24
# Conflicts:
#	package-lock.json
# Conflicts:
#	package-lock.json
#	package.json
#	src/config/generated/config.ts
#	src/config/index.ts
#	src/db/index.ts
#	src/db/types.ts
#	src/proxy/index.ts
#	src/service/urls.ts
#	src/ui/components/CustomButtons/CodeActionButton.tsx
#	src/ui/views/User/UserProfile.tsx
#	test/testProxy.test.ts
Brings in commits from main (8c629c2) including ssh-agent support,
tag push, Dockerfile optimizations, and various bug fixes.
Brings in commits from main (d6264eb) including dependabot dependency
updates (uuid, globals, env-paths, github-actions, types/supertest).
# Conflicts:
#	package.json
#	src/db/mongo/pushes.ts
#	src/db/types.ts
#	src/ui/utils.tsx
#	src/ui/views/Login/Login.tsx
#	src/ui/views/PushDetails/PushDetails.tsx
#	src/ui/views/PushRequests/components/PushesTable.tsx
#	test/db/mongo/push.test.ts
Lockfile still reflected the pre-migration react@16/@material-ui tree
after the main merge, causing npm ci to fail against @headlessui/react.
npm 11 resolved @emnapi/core and @emnapi/runtime differently than npm 10,
which CI's Node 22.x/24.x runners ship, causing npm ci to fail with
'Missing: @emnapi/core@1.11.3 from lock file'.
Sink, User, and PublicUser in db/types.ts were never updated when the
file/mongo backends added mustChangePassword and findUserByGitAccount,
causing check-types:server to fail.
Flagged by the unused-dependencies CI job via depcheck. Its icon
classes were only referenced by leftover dead CSS from the old
Material Dashboard theme this PR replaces; the npm package itself
was never imported.
react-is is a real dependency, pinned to satisfy @primer/react's peer
requirement on React 19, but it's never directly imported in src. In
CI depcheck runs before node_modules exists, so it can't tell it
satisfies a peer dependency and always flags it as unused.
@jescalada

Copy link
Copy Markdown
Owner Author

/security-review

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

Automated Security Review

Summary

Reviewed the visible portions of this large refactor (Material-UI → Primer/Tailwind, TanStack Query, new SCM-metadata and user-activity backend endpoints). Most changes are presentational and low risk, but three items are worth attention: a server-side request to a repo-controlled host (SSRF surface) behind a new API route, dependency version downgrades, and a new user-activity endpoint that returns full push records for any username.

Note: several files were truncated or excluded from the diff (including package-lock.json and a number of deleted UI files), so this review does not cover them in full.

Findings

src/service/gitProviders/index.ts, src/service/gitProviders/GitLabProvider.ts, src/service/routes/repo.ts
Server-side request forgery (SSRF) / outbound request to a stored, user-influenced host
GET /api/v1/repo/:id/scm-metadata resolves a provider from the stored repo remote URL and issues an outbound HTTP request to whatever hostname that URL contains. Any host whose name merely contains gitlab (e.g. gitlab.internal.corp, evil-gitlab.example) is treated as a GitLab instance and contacted, with the response body mapped into JSON returned to the caller. If repository registration is reachable by non-admin users, this becomes an authenticated SSRF plus information-disclosure primitive against internal networks. There is also no try/catch around new URL(remoteUrl) in getGitProvider, so a malformed stored URL yields an unhandled rejection in the route handler.

export function getGitProvider(remoteUrl: string): GitProvider {
  const hostname = new URL(remoteUrl).hostname.toLowerCase();
  if (hostname === 'github.com') return new GitHubProvider();
  if (hostname.includes('gitlab')) return new GitLabProvider(hostname);
  return new UnsupportedProvider();
}

Recommended fix: match GitLab hosts against an explicit allow-list (or a configured self-hosted GitLab host) rather than a substring check; reject private/loopback/link-local resolved addresses before issuing the request; set an explicit request timeout and maxRedirects: 0; and wrap new URL(...) so an invalid stored URL returns null instead of throwing.

package.json
Dependency version regression
Several dependencies are moved backwards, including axios ^1.18.1^1.13.4 and connect-mongo ^6.0.0^5.1.0 (a major downgrade that pulls an older MongoDB driver). Because package-lock.json was excluded from this diff, the actually installed versions cannot be confirmed here; a caret range does not guarantee the newer patched release is retained if the lockfile pins lower.

-    "axios": "^1.18.1",
+    "axios": "^1.13.4",
-    "connect-mongo": "^6.0.0",
+    "connect-mongo": "^5.1.0",

Recommended fix: restore the previous floor versions unless the downgrade is required, and verify the resolved versions in package-lock.json are free of known advisories (npm audit).

src/service/routes/users.ts
Missing authorization on data-exposing endpoint
The new GET /:id/activity route returns the full push records for an arbitrary username (db.getPushesForUserProfile), which include commit metadata, author emails and diff-related fields. The handler only checks that the user exists — there is no check that the caller is the same user or an admin, and no admin guard is visible in the added code.

router.get('/:id/activity', async (req: Request<{ id: string }>, res: Response) => {
  const username = req.params.id.toLowerCase();
  if (!(await db.findUser(username))) { ...404... }
  const pushes = await db.getPushesForUserProfile(username);
  res.send(pushes);
});

Recommended fix: confirm the router is mounted behind authentication, and add an explicit authorization check (requester is the same user or isAdminUser) before returning another user's activity; alternatively trim the returned fields to what the profile UI needs.

src/ui/views/PushDetails/components/Diff.tsx
Low severity — HTML from user-controlled diff content rendered via a parser
The renderer swaps react-html-parser for html-react-parser on HTML generated from attacker-influenceable diff text. diff2html escapes content and html-react-parser does not use dangerouslySetInnerHTML by default, so this is low risk, but the trust boundary is worth keeping explicit.

const outputHtml = Diff2Html.html(diff, { ... });
return <>{parse(outputHtml)}</>;

Recommended fix: no change required if diff2html output escaping is relied upon; if any custom replace/transform option is added later, ensure raw HTML is not re-injected and consider sanitising (e.g. DOMPurify) before parsing.


Coverage: 128 of 150 changed files were reviewed.
Not reviewed: package-lock.json (generated, vendored or lockfile), src/ui/assets/img/git-proxy.png (binary or too large for a text patch), src/ui/components/Card/Card.tsx (file deleted), src/ui/components/Card/CardAvatar.tsx (file deleted), src/ui/components/Card/CardBody.tsx (file deleted), src/ui/components/Card/CardFooter.tsx (file deleted), src/ui/components/Card/CardHeader.tsx (file deleted), src/ui/components/Card/CardIcon.tsx (file deleted), src/ui/components/CustomButtons/Button.tsx (file deleted), src/ui/components/CustomTabs/CustomTabs.tsx (file deleted), src/ui/components/Filtering/Filtering.css (file deleted), src/ui/components/Filtering/Filtering.tsx (file deleted), src/ui/components/Grid/GridContainer.tsx (file deleted), src/ui/components/Navbars/DashboardNavbarLinks.tsx (file deleted), src/ui/components/Pagination/Pagination.css (file deleted), src/ui/components/Search/Search.css (file deleted), src/ui/components/Search/Search.tsx (file deleted), src/ui/components/Sidebar/Sidebar.tsx (file deleted), src/ui/components/Snackbar/Snackbar.tsx (file deleted), src/ui/components/Snackbar/SnackbarContent.tsx (file deleted), src/ui/layouts/Dashboard.tsx (file deleted), src/ui/views/PushDetails/components/AttestationView.tsx (file deleted).
Shown only partially, because the diff exceeded the size budget: cypress/e2e/repo.cy.js, package.json, src/db/file/pushes.ts, src/db/index.ts, src/db/mongo/pushes.ts, src/db/types.ts, src/routes.tsx, src/ui/components/ErrorBoundary/ErrorBoundary.tsx, src/ui/services/git-push.ts, src/ui/services/repo.ts, src/ui/services/runtime-config.ts, src/ui/services/user.ts, src/ui/types.ts, src/ui/utils.tsx, src/ui/views/Login/Login.tsx, src/ui/views/PushDetails/PushDetails.tsx, src/ui/views/RepoDetails/Components/DeleteRepoDialog.tsx, src/ui/views/RepoList/Components/NewRepo.tsx, test/db/mongo/push.test.ts, test/ui/repo.test.ts, test/ui/user.test.ts. Consider splitting this PR up so it can be reviewed in full.

Disclaimer: This review is AI-generated and covers only what is listed above. Please validate the findings before acting on them.

Reviewed by claude-opus-5. Re-run by commenting /security-review on this PR.

@jescalada
jescalada force-pushed the ui-refactor-review-automation-tests branch from 7b03d62 to 145c0a9 Compare August 10, 2026 06:42
@jescalada

Copy link
Copy Markdown
Owner Author

/security-review

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.

2 participants