feat: revamp plugin system

[jescalada]

Changelog

• Added Phases for pull and push chains to make plugin system more flexible
• Extended plugins to take config options including
  • phase: Which phase in the chain to execute
  • displayName: User-facing string for sideband streaming and audits
  • isCollectible: Same as regular processors, non-critical errors allow chain to continue running, errors are collected in the end
  • chains: Which push chains to run the plugin for (ATM: tags, branches or both)
• Added a sample diff scanning plugin to show plugin configuration
  • Also shows TypeScript plugin support
• Fixed plugin descriptions on sideband streaming messages
• Updated plugin documentation to reflect new capabilities

Description

Note: This PR was human-written, and I would appreciate your human thoughts on it 😃

It revamps the plugin system to allow inserting plugins into specific phases in each chain. It also adds PluginOptions to configure the plugin displayName, isCollectible and chains properties.

Each chain is divided into Phases, which are considered ChainElements. Both plugins and chain elements are resolved into executable actions (ProcessorExec).

This allows plugins to access certain Action fields that get populated later on, such as the push diff in the AFTER_DIFF phase.

For example:

const branchPushChainElements: ChainElement[] = [
  proc.push.resolveUserFromToken,
  proc.push.checkEmptyBranch,
  proc.push.checkRepoInAuthorisedList,
  PushPhase.AFTER_PERMISSIONS,
  proc.push.checkMessages,
  proc.push.checkAuthorEmails,
  proc.push.checkUserPushPermission,
  proc.push.pullRemote, // cleanup is handled after chain execution if successful
  proc.push.writePack,
  PushPhase.AFTER_CHECKOUT,
  proc.push.checkHiddenCommits,
  proc.push.checkIfWaitingAuth,
  proc.push.preReceive,
  proc.push.getDiff,
  PushPhase.AFTER_DIFF,
  proc.push.gitleaks,
  proc.push.scanDiff,
  PushPhase.BEFORE_APPROVAL,
  proc.push.blockForAuth,
];

Each phase along with the guaranteed properties is described in the updated plugin documentation.

CustomSecretScanner plugin execution

This simple plugin demonstrates how to access the diff and scan it - previously not possible:

Related Issue

Resolves #

The idea of phases was already introduced by @dcoric in #1639. I expanded on it and focused on plugin revamp specifically.

I think safe, accurate pull scanning (supply chain scans) cannot actually be done via plugins at the moment: if we naively pull the diff using simpleGit within a plugin, we end up getting only the default branch (thus a user pulling a different, compromised branch wouldn't be detected), and on top of that the scan would complete first and then trigger a second "authorized" pull, thus if a vulnerability was introduced right after the scan finished, the user would be able to pull it anyways.

As a follow-up to this PR. I'd like to rewrite the pull chain logic to actually obtain the data requested by the user git pull <specific-branch>, store it in the action so it's extensible via plugins, and finally forward the git pull result to the user.

Checklist

General

• I have read the CONTRIBUTING.md guidelines
• Commit messages follow Conventional Commits format
• I have a FINOS CLA on file

Documentation

• Documentation has been added/updated for any new features

Tests

• Tests have been added/updated for new functionality
• Unit tests pass (npm test)
• Linting and formatting pass (npm run lint and npm run format:check)
• Type checks pass (npm run check-types)

[netlify]

✅ Deploy Preview for endearing-brigadeiros-63f9d0 ready!

Name	Link
🔨 Latest commit	de2ea61
🔍 Latest deploy log	https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/6aa0ec32cdfe0b0008507452
😎 Deploy Preview	https://deploy-preview-1683.git-proxy.preview.finos.org
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[codecov]

Codecov Report

❌ Patch coverage is 80.48780% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.98%. Comparing base (c4e2107) to head (de2ea61).

Files with missing lines	Patch %	Lines
src/proxy/chain.ts	82.75%	5 Missing ⚠️
src/plugin.ts	75.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1683      +/-   ##
==========================================
- Coverage   86.05%   85.98%   -0.07%     
==========================================
  Files         101      101              
  Lines        5571     5581      +10     
  Branches      995     1006      +11     
==========================================
+ Hits         4794     4799       +5     
- Misses        526      531       +5     
  Partials      251      251              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[dcoric]

Went through this properly. Also had a go at the pull scanning problem you raised at the end of the description, since that seemed like the interesting part. Your chain handles it as-is, details at the bottom.

What's good

Some of this is easy to skim past in a diff, so worth spelling out.

getChain throwing instead of logging and carrying on. The old path set pluginsInserted = true and then ran the chain with zero plugins, which is fail-open on a policy gate. Yours turns it into a clean rejection through executeChain's catch. That's a security fix rather than a refactor.

Moving plugins after the auth checks. I measured this on main: pull plugins splice in at pullActionChain index 0, so today they run before checkRepoInAuthorisedList. Plugins are inspecting repos that haven't been authorised yet, and a plugin author has no way to fix it. AFTER_AUTHORISATION and AFTER_PERMISSIONS as defaults close that, and your reasoning in the docs (an unauthorised repo could carry content crafted against a downstream plugin) is right.

toPluginExec calling plugin.exec(req, action) rather than splicing the method unbound. Quietly fixes this for any plugin that uses it.

Immutable chains kill both the splice mutation and the pluginsInserted latch it needed.

Docs are good too. The sideband transcript with remote: Running CustomSecretScanner... shown in context is the sort of thing that gets an extension point actually used.

Things I'd change before merge

PushPhase / PullPhase need as const. Without it, (typeof PushPhase)[keyof typeof PushPhase] widens to string, so type PushPhase = string:

// as written:        const typo: PushPhase = 'AFTER_DIF'   compiles clean, tsc exit 0
// with `} as const`: error TS2820: Type '"AFTER_DIF"' is not assignable to type 'PushPhase'.
//                                  Did you mean '"AFTER_DIFF"'?

A typo'd phase compiles, loads, and then matches nothing, silently unhooking the plugin. One word fixes it, and it's what makes the phase API type-safe at all.

Plugins built against an older git-proxy get dropped silently. isCompatiblePlugin is duck-typed, so a plugin compiled against 2.2 or earlier has no phase, passes the check, logs found push plugin, and then plugins.filter(p => p.phase === element) never matches it. I ran it: chain length 15, plugin absent, nothing logged. Someone's policy plugin stops enforcing after an upgrade with no signal at all. Maybe assert that every loaded plugin lands in at least one chain and throw if it doesn't, same spirit as your getChain change.

Smaller things:

The sample doesn't run on tag pushes. customSecretScanner sets phase: AFTER_DIFF with chains: ['branch', 'tag'], but the tag chain has no getDiff and no AFTER_DIFF marker, so in TAG chain? false. Anyone copying it as a secret scanner would assume tags were covered. Either drop 'tag' with a comment, or document which phases exist per chain, since the docs currently read as though all four PushPhases apply to both push chains.

The three "should load plugins" tests don't test anything any more. They compare against chain.branchPushChain, which is builtChains?.branch ?? [] and therefore empty until getChain builds it:

branchPushChain.length BEFORE getChain = 0
getChain() with ZERO plugins           = 15     assertion passes

All three pass with no plugins loaded. Asserting on plugin identity rather than length would fix them, and would catch the three items above.

Docs mention action.diff, which doesn't exist. Both scanDiff.ts and your own sample use action.steps.find(s => s.stepName === 'diff')?.content.

Trivia: buildChain's chainName param is unused, the docs link to customPushSecretScanner.ts (404, the file is customSecretScanner.ts), and the JSON config example is missing a closing quote.

On pull scanning

I don't think the plugin system is the blocker. The problem is that nothing parses the upload-pack request.

A client never asks for a branch. It asks for exact object IDs, and it says so before any data moves:

v2 POST #1   command=ls-refs     ref-prefix refs/heads/feature-x
v2 POST #2   command=fetch       want a1d4ae77c3be81fb...

That body is already buffered. extractRawBody's route predicate matches git-upload-pack as well as git-receive-pack, and proxyFilter assigns it to req.body before executeChain runs. It just never gets decoded.

Add a parsePull pre-processor alongside parsePush, put the decoded result on action.fetchRequest, and a plugin can read the wants, fetch exactly those objects, and inspect them.

That also takes care of the TOCTOU issue you described. Scan-then-refetch is unsafe if you refetch by ref name, but an object ID is content addressed, so you inspect X, allow the fetch, and the client gets X regardless of what happens to the ref afterwards. git fetch <url> <oid> works against default configured servers including GitHub, because a want is always a tip the client read out of the advertisement.

I ran this against a live proxy and real GitHub rather than reasoning about it. Plugin loaded through the normal PluginLoader:

small repo      command=ls-refs, wants=0, refPrefixes=3
                command=fetch,   wants=1  -> a1d4ae77...  (matches git ls-remote)

1436-ref repo   Content-Encoding=gzip, body=3190 bytes, magic 1f8b0800
                command=fetch,   wants=122, incomplete=false

and rejecting on a want:

$ git clone http://localhost:8123/github.com/...       (v2)
fatal: remote error: Pull blocked by probe plugin.
Requested commit a1d4ae77... is not permitted.
exit 128, no working tree created

$ git clone --depth 1 .../finos/git-proxy.git          (control, not blocked)
exit 0, 47 files

Two things that will catch you out

git gzips upload-pack bodies once they pass 1024 bytes. Measured: 1024 plaintext, 1025 gzipped, which is roughly 18 short refs, so effectively every real repo. Nothing in the chain inflates, so a parser sees gzip bytes and reports no wants at all. This got past several rounds of my own review because all my fixtures came from a two-branch repo and were under 1 KB.

There's a trap in the obvious fix. Don't inflate in extractRawBody. That buffer becomes req.body, and forwardReceivePackUpstream sends req.body verbatim with the client's headers, only recomputing content-length. Inflating there would post inflated bytes under a Content-Encoding: gzip header and break pushes. It has to happen per consumer, in the parser.

want-ref gives you zero want lines. With uploadpack.allowRefInWant set on the server, the fetch body carries want-ref refs/heads/... and no wants at all (verified). So a plugin has to treat "parsed, zero wants" as unscannable rather than nothing to scan. Get that ordering backwards and it fails open.

Which suggests an invariant worth writing down wherever the parsing ends up living: report a superset of what the server will actually serve, or mark the result incomplete. Over-reporting costs one redundant read. Under-reporting means content ships uninspected.

Next steps

I can send parsePull as a separate PR against main so it doesn't tangle with this one. It's independent of the phase work and useful either way. There's also a small fix worth pulling out of #1639 on its own: blocked pulls currently get application/x-git-receive-pack-result, which fetch clients can't render, so a blocked clone just reports "the remote end hung up unexpectedly".

One question back at you. With pull data available, does the pull chain want more than AFTER_AUTHORISATION, something like AFTER_WANTS, or would you rather keep one phase and have plugins branch on command?

No attachment to my version of any of this. Happy to hand it over, split it differently, or fold it into this PR if you'd rather own it.