Skip to content

fix: resolve extension gitUrl by parsing GitHub data URL instead of substring replace#157

Merged
jmaxdev merged 2 commits intoTrixtyAI:mainfrom
matiaspalmac:fix/extension-giturl-resolution
Apr 21, 2026
Merged

fix: resolve extension gitUrl by parsing GitHub data URL instead of substring replace#157
jmaxdev merged 2 commits intoTrixtyAI:mainfrom
matiaspalmac:fix/extension-giturl-resolution

Conversation

@matiaspalmac
Copy link
Copy Markdown
Contributor

@matiaspalmac matiaspalmac commented Apr 20, 2026

[Fix]: Resolve extension gitUrl from manifest instead of fragile string .replace

Describe the fix

installExtension and fetchFile derived a clone URL by literally calling

```ts
entry.data?.replace("/ide/blob/main/extensions/example-addon/package.json", ".git")
```

That hard-coded path was specific to the bundled trixty.example-addon. Any future entry whose data URL didn't end with that exact suffix would silently get its original URL fed into git clone, which then either failed or cloned the wrong target.

Change

Introduced resolveGitRepoUrl(entry) in ExtensionContext.tsx (exported so other consumers can adopt it):

  1. Returns entry.repository when present (current happy path).
  2. Otherwise parses entry.data with URL. Accepts both github.com/<owner>/<repo>/blob/<branch>/<path> and raw.githubusercontent.com/<owner>/<repo>/<branch>/<path> shapes.
  3. Reconstructs https://github.com/<owner>/<repo>.git from the first two path segments, which git clone handles directly and the existing Rust repo_to_raw_base helper normalizes via its .trim_end_matches(\".git\").
  4. Returns null for non-GitHub origins, so installExtension surfaces the existing "No repository URL defined" error instead of feeding git clone a doc URL.

Replaced both .replace(...) call sites with resolveGitRepoUrl(entry).

Trade-offs

  • Non-GitHub data URLs return null rather than guessing. That's intentional — the previous behavior of "hand the raw URL to git clone and hope" was the bug.
  • Backend changes were left out: the issue lists "move derivation to Rust" or "publish a gitUrl field" as alternatives, but both involve registry schema or Tauri command surface changes that are larger than the actual breakage described. The frontend helper closes the gap with a focused diff.

Verification

  • pnpm eslint on the touched file → 0 findings.
  • pnpm tsc --noEmit across the desktop app → clean.
  • Hand-traced both call sites; gitUrl is now derived through the same code path everywhere.

Trixty Version

v1.0.10

What operating system are you using?

Windows

Related Issue

fix #65

Copilot AI review requested due to automatic review settings April 20, 2026 23:23
@github-actions github-actions Bot added bug Something isn't working documentation Improvements or additions to documentation labels Apr 20, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 20, 2026

Thanks for the contribution! I'll review it as soon as possible. If you have still changes, please mark this PR as draft and all reviews will be cancelled. Tests reviews will be re-run only when the PR is marked as ready for review.

Copilot AI reviewed Apr 20, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes extension git clone URL derivation by replacing a fragile hard-coded string replacement with a GitHub URL parser (resolveGitRepoUrl) that reconstructs a proper https://github.com/<owner>/<repo>.git clone URL from MarketplaceEntry.data when repository is absent.

Changes:

  • Added resolveGitRepoUrl(entry) to derive a clone-friendly GitHub repo URL from repository or GitHub data URLs.
  • Updated installExtension to use resolveGitRepoUrl rather than a hard-coded .replace(...).
  • Updated fetchFile to use resolveGitRepoUrl for fetch_extension_file calls.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread apps/desktop/src/context/ExtensionContext.tsx
Comment thread apps/desktop/src/context/ExtensionContext.tsx
@jmaxdev jmaxdev force-pushed the fix/extension-giturl-resolution branch from 0c3d075 to 2ec3c5a Compare April 21, 2026 01:37
@jmaxdev jmaxdev merged commit bab566c into TrixtyAI:main Apr 21, 2026
1 check passed
@matiaspalmac matiaspalmac mentioned this pull request Apr 21, 2026
Merged
4 tasks
jmaxdev pushed a commit that referenced this pull request Apr 21, 2026
@matiaspalmac
fix: allow-list git_url before install_extension spawns git clone (#168)
26eeb2a 
# [Fix]: Allow-list `git_url` before `install_extension` spawns `git
clone`

## Description

`install_extension` forwarded the caller-supplied `git_url` straight
to `git clone`. Any scheme went through — `file://`, Windows UNC,
`ssh://`, `git://`, and the especially dangerous `ext::` transport
helpers. A compromised catalog entry could therefore trigger arbitrary
code at install time via git helpers, symlink tricks, or
credential-embedding URLs. Combined with the plain-HTTP registry fixed
in #162 and the fragile `.replace` derivation fixed in #157, this was
the final gate between a MITM and code execution on the user's
machine.

## Change

`apps/desktop/src-tauri/src/extensions.rs` — new
`validate_git_clone_url` is called **before** `install_extension`
builds a target directory, and the clone invocation now runs with
defensive `git -c` flags.

### URL validation

- **Strict `https://` scheme** (case-sensitive). Catches `HTTP://`,
  `http://`, `file://`, `ssh://`, `git://`, UNC `\\host\share`, etc.
- **Host allowlist**: `github.com`, `gitlab.com`, `bitbucket.org`.
  Every legitimate catalog entry today already satisfies this, and
  the frontend helper `resolveGitRepoUrl` produces exactly this
  shape.
- **Path**: exactly `<owner>/<repo>` or `<owner>/<repo>.git`.
- **Segment content**: ASCII alphanumeric + `-`, `_`, `.`; no leading
  `-` (prevents `--upload-pack` / `--config=` flag injection through
  the path).
- **No credentials or alternate-host tricks**: rejects any `@` in the
  authority section (`https://user@github.com/...`,
  `https://github.com@evil.example/...`).
- **No transport-helper syntax**: rejects any `::` anywhere in the
  URL.
- **No control characters or whitespace**.

### Clone hardening

Clone is now invoked as:

```
git -c protocol.ext.allow=never \
    -c protocol.file.allow=never \
    -c protocol.git.allow=never \
    clone --depth 1 <url> <target>
```

The allow-listed `https` host is unaffected. If anything ever gets
past the URL check, git itself refuses those transports at runtime.

### Tests

Added `git_url_validation_tests` with 12 cases covering the
accept/reject matrix. `cargo test git_url_validation` → 12/12 pass.

## Trade-offs

- **Allowlist is conservative.** Only three hosts are accepted today.
  Catalog entries on other git hosts (codeberg, sourcehut, self-hosted
  instances) would be rejected. That matches the marketplace's
  current scope; widening it later is a one-line diff.
- **No `.git` suffix required.** I considered requiring `.git` on the
  repo segment, but GitHub/GitLab both redirect `…/owner/repo` to the
  `.git` form for git clients, and `resolveGitRepoUrl` on the
  frontend already appends `.git` when it normalizes URLs. Both
  shapes are accepted.
- **`update_extension` / `uninstall_extension` not touched.** Those
  operate on the already-cloned directory and don't take a URL
  argument, so they're not part of the `git clone` attack surface
  the issue describes. Hardening them (e.g. adding the same `-c`
  flags to `git pull`) is a separate improvement.

## Verification

- `cargo check`, `cargo clippy --tests -- -D warnings` → clean.
- `cargo test git_url_validation` → 12 passed.
- Manual trace against the in-tree marketplace entry (the example
  addon on github.com/TrixtyAI) → clone still succeeds.
- Each rejection branch exercised by a unit test, listed in
  `git_url_validation_tests`.

## Related Issue

Fixes #55

## Checklist

- [x] I have tested this on the latest version.
- [x] I have followed the project's coding guidelines.
- [x] My changes generate no new warnings or errors.
- [x] I have verified the fix on:
  - OS: Windows
  - Version: v1.0.10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

bug Something isn't working documentation Improvements or additions to documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Fix]: Resolve extension gitUrl from manifest instead of fragile string .replace

3 participants

@matiaspalmac @jmaxdev