From 116f763bd8ab62f53e83a8213ecd2fb4a8780490 Mon Sep 17 00:00:00 2001 From: Andrew Nesbitt Date: Thu, 13 Aug 2026 10:36:34 +0100 Subject: [PATCH 01/18] Add Helm chart --- .github/workflows/ci.yml | 28 ++++ .github/workflows/publish.yml | 63 +++++++++ .gitignore | 4 +- README.md | 15 ++ deploy/charts/proxy/.helmignore | 6 + deploy/charts/proxy/Chart.yaml | 11 ++ deploy/charts/proxy/templates/NOTES.txt | 15 ++ deploy/charts/proxy/templates/_helpers.tpl | 46 ++++++ deploy/charts/proxy/templates/configmap.yaml | 12 ++ deploy/charts/proxy/templates/deployment.yaml | 102 ++++++++++++++ deploy/charts/proxy/templates/ingress.yaml | 36 +++++ deploy/charts/proxy/templates/pvc.yaml | 22 +++ deploy/charts/proxy/templates/service.yaml | 16 +++ deploy/charts/proxy/values.yaml | 133 ++++++++++++++++++ 14 files changed, 507 insertions(+), 2 deletions(-) create mode 100644 deploy/charts/proxy/.helmignore create mode 100644 deploy/charts/proxy/Chart.yaml create mode 100644 deploy/charts/proxy/templates/NOTES.txt create mode 100644 deploy/charts/proxy/templates/_helpers.tpl create mode 100644 deploy/charts/proxy/templates/configmap.yaml create mode 100644 deploy/charts/proxy/templates/deployment.yaml create mode 100644 deploy/charts/proxy/templates/ingress.yaml create mode 100644 deploy/charts/proxy/templates/pvc.yaml create mode 100644 deploy/charts/proxy/templates/service.yaml create mode 100644 deploy/charts/proxy/values.yaml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bb3d87d..03c0686 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -46,3 +46,31 @@ jobs: - name: golangci-lint run: go tool golangci-lint run ./... + + helm: + name: Helm chart + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - uses: azure/setup-helm@1a275c3b69536ee54be43f2070a358922e12c8d4 # v4.3.1 + with: + version: v3.18.6 + + - name: Lint chart + run: helm lint deploy/charts/proxy + + - name: Render chart variants + run: | + set -euo pipefail + helm template proxy deploy/charts/proxy >/dev/null + helm template proxy deploy/charts/proxy \ + --set persistence.enabled=false \ + --set config.existingConfigMap=proxy-config \ + --set ingress.enabled=true \ + --set 'ingress.hosts[0].host=proxy.example.com' \ + --set 'ingress.hosts[0].paths[0].path=/' \ + --set 'ingress.hosts[0].paths[0].pathType=Prefix' \ + >/dev/null diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 1c4eb1e..33601b3 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -96,3 +96,66 @@ jobs: for predicate in sbom-linux-amd64.spdx.json sbom-linux-arm64.spdx.json; do cosign attest --yes --type spdxjson --predicate "$predicate" "$reference" done + + publish_chart: + name: Push Helm chart to GHCR + if: github.ref_type == 'tag' + needs: push_to_registry + runs-on: ubuntu-latest + permissions: + contents: read + packages: write + steps: + - name: Check out the repo + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 + with: + persist-credentials: false + ref: ${{ github.sha }} + + - uses: azure/setup-helm@1a275c3b69536ee54be43f2070a358922e12c8d4 # v4.3.1 + with: + version: v3.18.6 + + - name: Validate and normalize release version + id: version + env: + TAG: ${{ github.ref_name }} + run: | + set -euo pipefail + semver='^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)(-((0|[1-9][0-9]*|[0-9]*[A-Za-z-][0-9A-Za-z-]*)(\.(0|[1-9][0-9]*|[0-9]*[A-Za-z-][0-9A-Za-z-]*))*))?$' + [[ "$TAG" =~ $semver ]] || { + echo "Tag must be strict SemVer of the form vMAJOR.MINOR.PATCH[-PRERELEASE]: $TAG" >&2 + exit 1 + } + version="${TAG#v}" + [[ "$version" != "0.0.0" ]] || { + echo "0.0.0 is a development placeholder and must not be published" >&2 + exit 1 + } + echo "version=$version" >> "$GITHUB_OUTPUT" + + - name: Log in to GHCR + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: printf '%s' "$GH_TOKEN" | helm registry login ghcr.io --username "$GITHUB_ACTOR" --password-stdin + + - name: Lint and package chart + env: + VERSION: ${{ steps.version.outputs.version }} + run: | + set -euo pipefail + helm lint deploy/charts/proxy + mkdir -p build + helm package \ + --destination build \ + --version "$VERSION" \ + --app-version "$VERSION" \ + deploy/charts/proxy + metadata="$(helm show chart "build/proxy-${VERSION}.tgz")" + [[ "$(awk '$1 == "version:" {print $2}' <<<"$metadata")" == "$VERSION" ]] + [[ "$(awk '$1 == "appVersion:" {gsub(/\"/, "", $2); print $2}' <<<"$metadata")" == "$VERSION" ]] + + - name: Push chart + env: + VERSION: ${{ steps.version.outputs.version }} + run: helm push "build/proxy-${VERSION}.tgz" oci://ghcr.io/git-pkgs/charts diff --git a/.gitignore b/.gitignore index 9016252..86f77d1 100644 --- a/.gitignore +++ b/.gitignore @@ -4,7 +4,7 @@ *.dll *.so *.dylib -proxy +/proxy # Test binary, built with `go test -c` *.test @@ -43,4 +43,4 @@ cache/* # Debug files __debug_bin -debug \ No newline at end of file +debug diff --git a/README.md b/README.md index 01012e2..232a116 100644 --- a/README.md +++ b/README.md @@ -62,6 +62,21 @@ brew install git-pkgs/git-pkgs/proxy Or download a binary from the [releases page](https://github.com/git-pkgs/proxy/releases). +### Helm + +Install the chart from GHCR, setting the public URL that package-manager clients +will use to reach the proxy: + +```bash +helm install proxy oci://ghcr.io/git-pkgs/charts/proxy \ + --set config.data.base_url=https://proxy.example.com +``` + +The default chart deploys one replica backed by a 10 GiB persistent volume, +using SQLite and filesystem artifact storage under `/data`. See +[`deploy/charts/proxy/values.yaml`](deploy/charts/proxy/values.yaml) for ingress, +external database and object-storage configuration options. + ## Quick Start ```bash diff --git a/deploy/charts/proxy/.helmignore b/deploy/charts/proxy/.helmignore new file mode 100644 index 0000000..4f7d63c --- /dev/null +++ b/deploy/charts/proxy/.helmignore @@ -0,0 +1,6 @@ +.DS_Store +.git/ +.github/ +*.swp +*.tmp +*.tgz diff --git a/deploy/charts/proxy/Chart.yaml b/deploy/charts/proxy/Chart.yaml new file mode 100644 index 0000000..4e6bff7 --- /dev/null +++ b/deploy/charts/proxy/Chart.yaml @@ -0,0 +1,11 @@ +apiVersion: v2 +name: proxy +description: A caching proxy for package registries +type: application +version: 0.0.0 +appVersion: "0.0.0" +home: https://github.com/git-pkgs/proxy +sources: + - https://github.com/git-pkgs/proxy +annotations: + artifacthub.io/license: MIT diff --git a/deploy/charts/proxy/templates/NOTES.txt b/deploy/charts/proxy/templates/NOTES.txt new file mode 100644 index 0000000..3b29f51 --- /dev/null +++ b/deploy/charts/proxy/templates/NOTES.txt @@ -0,0 +1,15 @@ +git-pkgs proxy has been installed. + +The default base URL is intended for local port forwarding. Before exposing the +proxy, set config.data.base_url to the URL used by package-manager clients. + +To access the proxy locally: + + kubectl -n {{ .Release.Namespace }} port-forward service/{{ include "proxy.fullname" . }} {{ .Values.service.port }}:{{ .Values.service.port }} + +Then visit http://localhost:{{ .Values.service.port }}/. + +{{- if not .Values.persistence.enabled }} +WARNING: persistence is disabled. Cached artifacts and the default SQLite +database will be lost when the pod is replaced. +{{- end }} diff --git a/deploy/charts/proxy/templates/_helpers.tpl b/deploy/charts/proxy/templates/_helpers.tpl new file mode 100644 index 0000000..9408a9f --- /dev/null +++ b/deploy/charts/proxy/templates/_helpers.tpl @@ -0,0 +1,46 @@ +{{/* Expand the chart name. */}} +{{- define "proxy.name" -}} +{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }} +{{- end }} + +{{/* Create a release-specific, DNS-safe resource name. */}} +{{- define "proxy.fullname" -}} +{{- if .Values.fullnameOverride }} +{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" }} +{{- else }} +{{- $name := include "proxy.name" . }} +{{- if contains $name .Release.Name }} +{{- .Release.Name | trunc 63 | trimSuffix "-" }} +{{- else }} +{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" }} +{{- end }} +{{- end }} +{{- end }} + +{{- define "proxy.labels" -}} +helm.sh/chart: {{ printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" }} +{{ include "proxy.selectorLabels" . }} +app.kubernetes.io/version: {{ .Chart.AppVersion | quote }} +app.kubernetes.io/managed-by: {{ .Release.Service }} +{{- end }} + +{{- define "proxy.selectorLabels" -}} +app.kubernetes.io/name: {{ include "proxy.name" . }} +app.kubernetes.io/instance: {{ .Release.Name }} +{{- end }} + +{{- define "proxy.configMapName" -}} +{{- default (include "proxy.fullname" .) .Values.config.existingConfigMap }} +{{- end }} + +{{- define "proxy.claimName" -}} +{{- default (include "proxy.fullname" .) .Values.persistence.existingClaim }} +{{- end }} + +{{- define "proxy.image" -}} +{{- if .Values.image.digest -}} +{{- printf "%s@%s" .Values.image.repository .Values.image.digest -}} +{{- else -}} +{{- printf "%s:%s" .Values.image.repository (default .Chart.AppVersion .Values.image.tag) -}} +{{- end -}} +{{- end }} diff --git a/deploy/charts/proxy/templates/configmap.yaml b/deploy/charts/proxy/templates/configmap.yaml new file mode 100644 index 0000000..40aad7b --- /dev/null +++ b/deploy/charts/proxy/templates/configmap.yaml @@ -0,0 +1,12 @@ +{{- if not .Values.config.existingConfigMap }} +apiVersion: v1 +kind: ConfigMap +metadata: + name: {{ include "proxy.fullname" . }} + namespace: {{ .Release.Namespace }} + labels: + {{- include "proxy.labels" . | nindent 4 }} +data: + {{ required "config.existingConfigMapKey is required" .Values.config.existingConfigMapKey }}: | + {{- toYaml .Values.config.data | nindent 4 }} +{{- end }} diff --git a/deploy/charts/proxy/templates/deployment.yaml b/deploy/charts/proxy/templates/deployment.yaml new file mode 100644 index 0000000..0c8efb3 --- /dev/null +++ b/deploy/charts/proxy/templates/deployment.yaml @@ -0,0 +1,102 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: {{ include "proxy.fullname" . }} + namespace: {{ .Release.Namespace }} + labels: + {{- include "proxy.labels" . | nindent 4 }} +spec: + replicas: {{ .Values.replicaCount }} + strategy: + {{- toYaml .Values.deploymentStrategy | nindent 4 }} + selector: + matchLabels: + {{- include "proxy.selectorLabels" . | nindent 6 }} + template: + metadata: + labels: + {{- include "proxy.selectorLabels" . | nindent 8 }} + {{- with .Values.podLabels }} + {{- toYaml . | nindent 8 }} + {{- end }} + annotations: + checksum/config: {{ include (print $.Template.BasePath "/configmap.yaml") . | sha256sum }} + {{- with .Values.podAnnotations }} + {{- toYaml . | nindent 8 }} + {{- end }} + spec: + automountServiceAccountToken: false + terminationGracePeriodSeconds: {{ .Values.terminationGracePeriodSeconds }} + securityContext: + {{- toYaml .Values.podSecurityContext | nindent 8 }} + {{- with .Values.imagePullSecrets }} + imagePullSecrets: + {{- toYaml . | nindent 8 }} + {{- end }} + containers: + - name: {{ .Chart.Name }} + image: {{ include "proxy.image" . | quote }} + imagePullPolicy: {{ .Values.image.pullPolicy }} + securityContext: + {{- toYaml .Values.containerSecurityContext | nindent 12 }} + args: + - serve + - -config + - /etc/proxy/{{ .Values.config.existingConfigMapKey }} + {{- with .Values.extraEnv }} + env: + {{- toYaml . | nindent 12 }} + {{- end }} + {{- with .Values.extraEnvFrom }} + envFrom: + {{- toYaml . | nindent 12 }} + {{- end }} + ports: + - name: http + containerPort: {{ .Values.service.containerPort }} + protocol: TCP + startupProbe: + {{- toYaml .Values.startupProbe | nindent 12 }} + readinessProbe: + {{- toYaml .Values.readinessProbe | nindent 12 }} + livenessProbe: + {{- toYaml .Values.livenessProbe | nindent 12 }} + resources: + {{- toYaml .Values.resources | nindent 12 }} + volumeMounts: + - name: config + mountPath: /etc/proxy/{{ .Values.config.existingConfigMapKey }} + subPath: {{ .Values.config.existingConfigMapKey }} + readOnly: true + - name: data + mountPath: {{ .Values.persistence.mountPath }} + volumes: + - name: config + configMap: + name: {{ include "proxy.configMapName" . }} + items: + - key: {{ required "config.existingConfigMapKey is required" .Values.config.existingConfigMapKey }} + path: {{ .Values.config.existingConfigMapKey }} + - name: data + {{- if .Values.persistence.enabled }} + persistentVolumeClaim: + claimName: {{ include "proxy.claimName" . }} + {{- else }} + emptyDir: {} + {{- end }} + {{- with .Values.nodeSelector }} + nodeSelector: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.affinity }} + affinity: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.tolerations }} + tolerations: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.topologySpreadConstraints }} + topologySpreadConstraints: + {{- toYaml . | nindent 8 }} + {{- end }} diff --git a/deploy/charts/proxy/templates/ingress.yaml b/deploy/charts/proxy/templates/ingress.yaml new file mode 100644 index 0000000..9484bb5 --- /dev/null +++ b/deploy/charts/proxy/templates/ingress.yaml @@ -0,0 +1,36 @@ +{{- if .Values.ingress.enabled }} +apiVersion: networking.k8s.io/v1 +kind: Ingress +metadata: + name: {{ include "proxy.fullname" . }} + namespace: {{ .Release.Namespace }} + labels: + {{- include "proxy.labels" . | nindent 4 }} + {{- with .Values.ingress.annotations }} + annotations: + {{- toYaml . | nindent 4 }} + {{- end }} +spec: + {{- with .Values.ingress.className }} + ingressClassName: {{ . | quote }} + {{- end }} + {{- with .Values.ingress.tls }} + tls: + {{- toYaml . | nindent 4 }} + {{- end }} + rules: + {{- range .Values.ingress.hosts }} + - host: {{ .host | quote }} + http: + paths: + {{- range .paths }} + - path: {{ .path | quote }} + pathType: {{ .pathType }} + backend: + service: + name: {{ include "proxy.fullname" $ }} + port: + number: {{ $.Values.service.port }} + {{- end }} + {{- end }} +{{- end }} diff --git a/deploy/charts/proxy/templates/pvc.yaml b/deploy/charts/proxy/templates/pvc.yaml new file mode 100644 index 0000000..80687b2 --- /dev/null +++ b/deploy/charts/proxy/templates/pvc.yaml @@ -0,0 +1,22 @@ +{{- if and .Values.persistence.enabled (not .Values.persistence.existingClaim) }} +apiVersion: v1 +kind: PersistentVolumeClaim +metadata: + name: {{ include "proxy.fullname" . }} + namespace: {{ .Release.Namespace }} + labels: + {{- include "proxy.labels" . | nindent 4 }} + {{- with .Values.persistence.annotations }} + annotations: + {{- toYaml . | nindent 4 }} + {{- end }} +spec: + accessModes: + {{- toYaml .Values.persistence.accessModes | nindent 4 }} + {{- with .Values.persistence.storageClass }} + storageClassName: {{ . | quote }} + {{- end }} + resources: + requests: + storage: {{ .Values.persistence.size }} +{{- end }} diff --git a/deploy/charts/proxy/templates/service.yaml b/deploy/charts/proxy/templates/service.yaml new file mode 100644 index 0000000..ee5931e --- /dev/null +++ b/deploy/charts/proxy/templates/service.yaml @@ -0,0 +1,16 @@ +apiVersion: v1 +kind: Service +metadata: + name: {{ include "proxy.fullname" . }} + namespace: {{ .Release.Namespace }} + labels: + {{- include "proxy.labels" . | nindent 4 }} +spec: + type: {{ .Values.service.type }} + ports: + - name: http + port: {{ .Values.service.port }} + targetPort: http + protocol: TCP + selector: + {{- include "proxy.selectorLabels" . | nindent 4 }} diff --git a/deploy/charts/proxy/values.yaml b/deploy/charts/proxy/values.yaml new file mode 100644 index 0000000..d7a7c26 --- /dev/null +++ b/deploy/charts/proxy/values.yaml @@ -0,0 +1,133 @@ +replicaCount: 1 + +nameOverride: "" +fullnameOverride: "" + +image: + repository: ghcr.io/git-pkgs/proxy + # An empty tag uses the chart appVersion. + tag: "" + # When set, digest takes precedence over tag. + digest: "" + pullPolicy: IfNotPresent + +imagePullSecrets: [] + +service: + type: ClusterIP + port: 8080 + # Keep this aligned with config.data.listen (or the listen address in an + # existing ConfigMap). + containerPort: 8080 + +# The generated configuration is ignored when existingConfigMap is set. +config: + existingConfigMap: "" + existingConfigMapKey: config.yaml + data: + listen: ":8080" + # Set this to the URL package-manager clients use to reach the proxy. + base_url: "http://localhost:8080" + storage: + url: "file:///data/artifacts" + database: + driver: sqlite + path: "/data/proxy.db" + log: + level: info + format: json + +# Environment variables override values from the configuration file. This is +# also the recommended way to supply secret values such as database passwords +# and object-storage credentials. +extraEnv: [] +# - name: PROXY_DATABASE_URL +# valueFrom: +# secretKeyRef: +# name: proxy-database +# key: url + +extraEnvFrom: [] +# - secretRef: +# name: proxy-object-storage + +persistence: + enabled: true + mountPath: /data + existingClaim: "" + annotations: {} + accessModes: + - ReadWriteOnce + size: 10Gi + storageClass: "" + +deploymentStrategy: + type: Recreate + +ingress: + enabled: false + className: "" + annotations: {} + hosts: [] + # - host: proxy.example.com + # paths: + # - path: / + # pathType: Prefix + tls: [] + # - secretName: proxy-tls + # hosts: + # - proxy.example.com + +podAnnotations: {} +podLabels: {} + +podSecurityContext: + runAsNonRoot: true + runAsUser: 1000 + runAsGroup: 1000 + fsGroup: 1000 + seccompProfile: + type: RuntimeDefault + +containerSecurityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: + - ALL + +resources: {} +# requests: +# cpu: 100m +# memory: 128Mi +# limits: +# memory: 512Mi + +startupProbe: + tcpSocket: + port: http + failureThreshold: 30 + periodSeconds: 2 + +# /health checks both the database and storage backends and can take up to ten +# seconds. It is intentionally a readiness check rather than a liveness check. +readinessProbe: + httpGet: + path: /health + port: http + timeoutSeconds: 11 + periodSeconds: 15 + failureThreshold: 2 + +livenessProbe: + tcpSocket: + port: http + periodSeconds: 20 + failureThreshold: 3 + +terminationGracePeriodSeconds: 30 + +nodeSelector: {} +tolerations: [] +affinity: {} +topologySpreadConstraints: [] From ed540053fadc13f1f6f520b5bca333b64d40ac9a Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 14 Aug 2026 09:15:33 +0100 Subject: [PATCH 02/18] Bump zizmorcore/zizmor-action from 0.6.1 to 0.6.2 (#251) Bumps [zizmorcore/zizmor-action](https://github.com/zizmorcore/zizmor-action) from 0.6.1 to 0.6.2. - [Release notes](https://github.com/zizmorcore/zizmor-action/releases) - [Commits](https://github.com/zizmorcore/zizmor-action/compare/6fc4b006235f201fdab3722e17240ab420d580e5...3dc1ecc9bcb9e94e9b2c709687979e1298497054) --- updated-dependencies: - dependency-name: zizmorcore/zizmor-action dependency-version: 0.6.2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/zizmor.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/zizmor.yml b/.github/workflows/zizmor.yml index 4df0e18..ac587dd 100644 --- a/.github/workflows/zizmor.yml +++ b/.github/workflows/zizmor.yml @@ -26,4 +26,4 @@ jobs: persist-credentials: false - name: Run zizmor - uses: zizmorcore/zizmor-action@6fc4b006235f201fdab3722e17240ab420d580e5 # v0.6.1 + uses: zizmorcore/zizmor-action@3dc1ecc9bcb9e94e9b2c709687979e1298497054 # v0.6.2 From 849500de1ec35442ef675361e4fe3769c86c9d9b Mon Sep 17 00:00:00 2001 From: wickedOne Date: Fri, 14 Aug 2026 11:38:08 +0200 Subject: [PATCH 03/18] fix: decode PURL percent-encoding in versions and package paths (#244) * fix: decode PURL percent-encoding in versions and package paths * review fix --- internal/database/queries.go | 37 ++- internal/database/types.go | 76 +++++- internal/database/version_purl_test.go | 159 ++++++++++++ internal/handler/debian_test.go | 5 + internal/server/api.go | 10 +- internal/server/browse.go | 34 ++- internal/server/browse_test.go | 17 +- internal/server/resolve.go | 95 ++++++- internal/server/resolve_test.go | 115 +++++++-- internal/server/server.go | 27 +- internal/server/server_test.go | 235 ++++++++++++++++++ .../server/templates/pages/browse_source.html | 11 +- .../templates/pages/compare_versions.html | 8 +- .../server/templates/pages/package_show.html | 12 +- .../server/templates/pages/version_show.html | 6 +- 15 files changed, 745 insertions(+), 102 deletions(-) create mode 100644 internal/database/version_purl_test.go diff --git a/internal/database/queries.go b/internal/database/queries.go index b01abe4..9fa5381 100644 --- a/internal/database/queries.go +++ b/internal/database/queries.go @@ -465,11 +465,14 @@ func (db *DB) GetMostPopularPackages(limit int) ([]PopularPackage, error) { } type RecentPackage struct { - Ecosystem string `db:"ecosystem"` - Name string `db:"name"` - Version string `db:"version"` - CachedAt time.Time `db:"fetched_at"` - Size int64 `db:"size"` + Ecosystem string `db:"ecosystem"` + Name string `db:"name"` + VersionPURL string `db:"version_purl"` + CachedAt time.Time `db:"fetched_at"` + Size int64 `db:"size"` + // Version is derived from VersionPURL rather than selected, so that the + // PURL percent-encoding is decoded (e.g. "%2B" back to "+"). + Version string `db:"-"` } func (db *DB) GetRecentlyCachedPackages(limit int) ([]RecentPackage, error) { @@ -483,10 +486,10 @@ func (db *DB) GetRecentlyCachedPackages(limit int) ([]RecentPackage, error) { } var packages []RecentPackage - // We need to extract version from the purl since there's no separate version column + // There is no separate version column, so the full version PURL is selected + // and the version is decoded from it in Go. query := db.Rebind(` - SELECT p.ecosystem, p.name, - SUBSTR(v.purl, INSTR(v.purl, '@') + 1) as version, + SELECT p.ecosystem, p.name, v.purl as version_purl, a.fetched_at, COALESCE(a.size, 0) as size FROM artifacts a JOIN versions v ON v.purl = a.version_purl @@ -496,25 +499,13 @@ func (db *DB) GetRecentlyCachedPackages(limit int) ([]RecentPackage, error) { LIMIT ? `) - // For postgres, use different string function - if db.dialect == DialectPostgres { - query = db.Rebind(` - SELECT p.ecosystem, p.name, - SUBSTRING(v.purl FROM POSITION('@' IN v.purl) + 1) as version, - a.fetched_at, COALESCE(a.size, 0) as size - FROM artifacts a - JOIN versions v ON v.purl = a.version_purl - JOIN packages p ON p.purl = v.package_purl - WHERE a.storage_path IS NOT NULL AND a.fetched_at IS NOT NULL - ORDER BY a.fetched_at DESC - LIMIT ? - `) - } - err = db.Select(&packages, query, limit) if err != nil { return nil, err } + for i := range packages { + packages[i].Version = VersionFromPURL(packages[i].VersionPURL) + } return packages, nil } diff --git a/internal/database/types.go b/internal/database/types.go index 3826c7a..5ddb9f3 100644 --- a/internal/database/types.go +++ b/internal/database/types.go @@ -2,6 +2,7 @@ package database import ( "database/sql" + "net/url" "strings" "time" ) @@ -47,10 +48,79 @@ type Version struct { // Version extracts the version string from the PURL. // e.g., "pkg:npm/lodash@4.17.21" -> "4.17.21" func (v *Version) Version() string { - if idx := strings.LastIndex(v.PURL, "@"); idx >= 0 { - return v.PURL[idx+1:] + return VersionFromPURL(v.PURL) +} + +// EscapedVersion returns the version escaped for use as a single URL path +// segment. +// +// Version returns decoded text, which is what should be shown to a user but is +// not safe to drop into a link: html/template preserves reserved characters and +// existing escapes in a URL, so "release/1" would split into two path segments, +// "v1?build" would start a query string, and a literal "%2B" would be read back +// as "+". Escaping here and decoding in splitWildcardPath round-trips the value, +// so the link resolves to the version that was stored. +func (v *Version) EscapedVersion() string { + return url.PathEscape(v.Version()) +} + +// DisplayPURL returns the PURL with its path components percent-decoded, for +// showing in the UI. The stored PURL keeps the canonical encoding (which is +// what the API and all lookups use); this is only a readable rendering, so that +// a version like "7.91+dfsg1-2ubuntu0.1" is not shown as "7.91%2Bdfsg1-2ubuntu0.1" +// and an npm scope is shown as "@babel" rather than "%40babel". Qualifiers and +// subpath keep their encoding, since decoding those would be ambiguous. +func (v *Version) DisplayPURL() string { + base, suffix := v.PURL, "" + if i := strings.IndexAny(base, "?#"); i >= 0 { + base, suffix = base[:i], base[i:] } - return "" + + name, version := base, "" + if idx := strings.LastIndex(base, "@"); idx >= 0 { + name, version = base[:idx], "@"+decodePURLComponent(base[idx+1:]) + } + + parts := strings.Split(name, "/") + for i, part := range parts { + parts[i] = decodePURLComponent(part) + } + return strings.Join(parts, "/") + version + suffix +} + +// VersionFromPURL extracts the decoded version string from a PURL. +// +// PURL percent-encodes characters that are not safe in a path component, so a +// Debian version like "7.91+dfsg1-2ubuntu0.1" is stored as +// "pkg:deb/nmap@7.91%2Bdfsg1-2ubuntu0.1". The raw substring after "@" is +// therefore not the version: it must be percent-decoded before being displayed +// or used to build a URL, otherwise "%2B" leaks into the UI and round-tripping +// the value back into a PURL double-encodes it. +// +// e.g., "pkg:npm/lodash@4.17.21" -> "4.17.21" +func VersionFromPURL(p string) string { + // Qualifiers ("?key=value") and subpath ("#path") follow the version. + if i := strings.IndexAny(p, "?#"); i >= 0 { + p = p[:i] + } + idx := strings.LastIndex(p, "@") + if idx < 0 { + return "" + } + return decodePURLComponent(p[idx+1:]) +} + +// decodePURLComponent percent-decodes a single PURL path component, returning +// the input unchanged if it is not valid percent-encoding. +func decodePURLComponent(s string) string { + if !strings.Contains(s, "%") { + return s + } + decoded, err := url.PathUnescape(s) + if err != nil { + return s + } + return decoded } // Artifact represents a cached artifact in the database. diff --git a/internal/database/version_purl_test.go b/internal/database/version_purl_test.go new file mode 100644 index 0000000..517022b --- /dev/null +++ b/internal/database/version_purl_test.go @@ -0,0 +1,159 @@ +package database + +import ( + "database/sql" + "net/url" + "testing" + "time" +) + +func TestVersionFromPURL(t *testing.T) { + tests := []struct { + name string + purl string + want string + }{ + {"simple", "pkg:npm/lodash@4.17.21", "4.17.21"}, + {"namespaced", "pkg:composer/symfony/console@6.0.0", "6.0.0"}, + // Debian/Ubuntu versions routinely contain "+", which PURL encodes. + {"encoded plus", "pkg:deb/nmap@7.91%2Bdfsg1%2Breally7.80%2Bdfsg1-2ubuntu0.1", "7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1"}, + {"encoded epoch", "pkg:deb/curl@1%3A7.81.0-1", "1:7.81.0-1"}, + {"encoded plus with qualifier", "pkg:deb/nmap@7.91%2Bdfsg1?repository_url=http%3A%2F%2Fexample.com", "7.91+dfsg1"}, + {"tilde is not encoded", "pkg:deb/foo@1.0~rc1", "1.0~rc1"}, + {"no version", "pkg:npm/lodash", ""}, + {"invalid escape passed through", "pkg:npm/lodash@1.0%zz", "1.0%zz"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := VersionFromPURL(tt.purl); got != tt.want { + t.Errorf("VersionFromPURL(%q) = %q, want %q", tt.purl, got, tt.want) + } + v := &Version{PURL: tt.purl} + if got := v.Version(); got != tt.want { + t.Errorf("Version.Version() for %q = %q, want %q", tt.purl, got, tt.want) + } + }) + } +} + +// TestVersionEscapedVersion checks the value the templates put in a URL. It +// must survive the round trip back through the router: escaping here and +// decoding per path segment on the way in has to yield the original version. +func TestVersionEscapedVersion(t *testing.T) { + tests := []struct { + name string + purl string + want string + }{ + {"simple", "pkg:npm/lodash@4.17.21", "4.17.21"}, + // "+" is legal in a path segment, so it stays literal and the UI keeps + // showing the version the way Debian writes it. + {"plus stays literal", "pkg:deb/nmap@7.91%2Bdfsg1-2ubuntu0.1", "7.91+dfsg1-2ubuntu0.1"}, + // A slash would otherwise split the version into two path segments. + {"slash", "pkg:golang/example@release%2F1", "release%2F1"}, + // A question mark would otherwise start the query string. + {"question mark", "pkg:npm/example@v1%3Fbuild", "v1%3Fbuild"}, + // A version containing a literal "%2B" is stored double-encoded; the + // link must re-encode it or it decodes back to "+" instead. + {"literal percent escape", "pkg:npm/example@1.0%252B", "1.0%252B"}, + {"space", "pkg:npm/example@1.0%20beta", "1.0%20beta"}, + {"no version", "pkg:npm/lodash", ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + v := &Version{PURL: tt.purl} + got := v.EscapedVersion() + if got != tt.want { + t.Errorf("EscapedVersion() for %q = %q, want %q", tt.purl, got, tt.want) + } + // The router decodes each path segment, which must give back the + // version the page displays. + decoded, err := url.PathUnescape(got) + if err != nil { + t.Fatalf("PathUnescape(%q) failed: %v", got, err) + } + if decoded != v.Version() { + t.Errorf("round trip for %q = %q, want %q", tt.purl, decoded, v.Version()) + } + }) + } +} + +func TestVersionDisplayPURL(t *testing.T) { + tests := []struct { + name string + purl string + want string + }{ + {"simple", "pkg:npm/lodash@4.17.21", "pkg:npm/lodash@4.17.21"}, + { + "encoded plus", + "pkg:deb/nmap@7.91%2Bdfsg1%2Breally7.80%2Bdfsg1-2ubuntu0.1", + "pkg:deb/nmap@7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1", + }, + { + "qualifier preserved", + "pkg:deb/nmap@7.91%2Bdfsg1?repository_url=http%3A%2F%2Fexample.com", + "pkg:deb/nmap@7.91+dfsg1?repository_url=http%3A%2F%2Fexample.com", + }, + // The namespace is encoded too: MakePURLString("npm", "@babel/core", …) + // produces "pkg:npm/%40babel/core@…". + {"encoded npm scope", "pkg:npm/%40babel/core@7.0.0", "pkg:npm/@babel/core@7.0.0"}, + {"encoded scope without version", "pkg:npm/%40babel/core", "pkg:npm/@babel/core"}, + {"no version", "pkg:npm/lodash", "pkg:npm/lodash"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + v := &Version{PURL: tt.purl} + if got := v.DisplayPURL(); got != tt.want { + t.Errorf("DisplayPURL() for %q = %q, want %q", tt.purl, got, tt.want) + } + }) + } +} + +// TestGetRecentlyCachedPackagesDecodesVersion guards the dashboard's "recently +// cached" list, which derives the version from the version PURL. +func TestGetRecentlyCachedPackagesDecodesVersion(t *testing.T) { + runWithBothDatabases(t, func(t *testing.T, db *DB) { + const versionPURL = "pkg:deb/nmap@7.91%2Bdfsg1%2Breally7.80%2Bdfsg1-2ubuntu0.1" + + if err := db.UpsertPackage(&Package{ + PURL: "pkg:deb/nmap", Ecosystem: "deb", Name: "nmap", + }); err != nil { + t.Fatalf("UpsertPackage failed: %v", err) + } + if err := db.UpsertVersion(&Version{ + PURL: versionPURL, PackagePURL: "pkg:deb/nmap", + }); err != nil { + t.Fatalf("UpsertVersion failed: %v", err) + } + if err := db.UpsertArtifact(&Artifact{ + VersionPURL: versionPURL, + Filename: "nmap_7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1_amd64.deb", + UpstreamURL: "http://archive.ubuntu.com/ubuntu/pool/universe/n/nmap/nmap.deb", + StoragePath: sql.NullString{String: "/cache/nmap.deb", Valid: true}, + FetchedAt: sql.NullTime{Time: time.Now(), Valid: true}, + }); err != nil { + t.Fatalf("UpsertArtifact failed: %v", err) + } + + recent, err := db.GetRecentlyCachedPackages(10) + if err != nil { + t.Fatalf("GetRecentlyCachedPackages failed: %v", err) + } + if len(recent) != 1 { + t.Fatalf("expected 1 recent package, got %d", len(recent)) + } + const want = "7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1" + if recent[0].Version != want { + t.Errorf("Version = %q, want %q", recent[0].Version, want) + } + if recent[0].VersionPURL != versionPURL { + t.Errorf("VersionPURL = %q, want %q", recent[0].VersionPURL, versionPURL) + } + }) +} diff --git a/internal/handler/debian_test.go b/internal/handler/debian_test.go index 60fa23a..b086fdf 100644 --- a/internal/handler/debian_test.go +++ b/internal/handler/debian_test.go @@ -12,6 +12,11 @@ func TestDebianHandler_parsePoolPath(t *testing.T) { {"pool/main/libn/libncurses/libncurses6_6.2-1_amd64.deb", "libncurses6", "6.2-1", "amd64"}, {"pool/contrib/v/virtualbox/virtualbox_6.1.38-1_amd64.deb", "virtualbox", "6.1.38-1", "amd64"}, {"pool/main/g/git/git_2.39.2-1_arm64.deb", "git", "2.39.2-1", "arm64"}, + { + "pool/universe/n/nmap/nmap_7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1_amd64.deb", + "nmap", "7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1", "amd64", + }, + {"pool/main/o/openssl/openssl_3.0.2-0ubuntu1.15~build1_amd64.deb", "openssl", "3.0.2-0ubuntu1.15~build1", "amd64"}, {"invalid/path", "", "", ""}, {"pool/main/n/nginx/nginx.deb", "", "", ""}, }) diff --git a/internal/server/api.go b/internal/server/api.go index ddb9ca7..992d736 100644 --- a/internal/server/api.go +++ b/internal/server/api.go @@ -139,12 +139,11 @@ type BulkResponse struct { // Resolves namespaced package names (Composer vendor/name, npm @scope/name) from the path. func (h *APIHandler) HandlePackagePath(w http.ResponseWriter, r *http.Request) { ecosystem := chi.URLParam(r, "ecosystem") - wildcard := chi.URLParam(r, "*") - if err := validatePackagePath(wildcard); err != nil { + segments, err := packagePathSegments(r) + if err != nil { badRequest(w, err.Error()) return } - segments := splitWildcardPath(wildcard) if ecosystem == "" || len(segments) == 0 { badRequest(w, "ecosystem and name are required") @@ -277,12 +276,11 @@ func (h *APIHandler) getVersion(w http.ResponseWriter, r *http.Request, ecosyste // Supports both {name} and {name}/{version} paths with namespaced package names. func (h *APIHandler) HandleVulnsPath(w http.ResponseWriter, r *http.Request) { ecosystem := chi.URLParam(r, "ecosystem") - wildcard := chi.URLParam(r, "*") - if err := validatePackagePath(wildcard); err != nil { + segments, err := packagePathSegments(r) + if err != nil { badRequest(w, err.Error()) return } - segments := splitWildcardPath(wildcard) if ecosystem == "" || len(segments) == 0 { badRequest(w, "ecosystem and name are required") diff --git a/internal/server/browse.go b/internal/server/browse.go index c60cba3..43ad9ae 100644 --- a/internal/server/browse.go +++ b/internal/server/browse.go @@ -147,12 +147,11 @@ type BrowseFileInfo struct { // {name}/{version}/file/{path} -> browse file func (s *Server) handleBrowsePath(w http.ResponseWriter, r *http.Request) { ecosystem := chi.URLParam(r, "ecosystem") - wildcard := chi.URLParam(r, "*") - if err := validatePackagePath(wildcard); err != nil { + segments, err := packagePathSegments(r) + if err != nil { badRequest(w, err.Error()) return } - segments := splitWildcardPath(wildcard) if ecosystem == "" || len(segments) < 2 { badRequest(w, "ecosystem, name, and version required") @@ -203,12 +202,11 @@ func (s *Server) handleBrowsePath(w http.ResponseWriter, r *http.Request) { // Supported paths: {name}/{fromVersion}/{toVersion} func (s *Server) handleComparePath(w http.ResponseWriter, r *http.Request) { ecosystem := chi.URLParam(r, "ecosystem") - wildcard := chi.URLParam(r, "*") - if err := validatePackagePath(wildcard); err != nil { + segments, err := packagePathSegments(r) + if err != nil { badRequest(w, err.Error()) return } - segments := splitWildcardPath(wildcard) if ecosystem == "" || len(segments) < 3 { badRequest(w, "ecosystem, name, fromVersion, and toVersion required") @@ -506,11 +504,16 @@ func isLikelyText(filename string) bool { } // BrowseSourceData contains data for the browse source page. +// +// Version is the decoded version, for display. EscapedVersion is the same value +// escaped as a single URL path segment and is what the links and the browse API +// calls must use; see database.Version.EscapedVersion. type BrowseSourceData struct { Layout - Ecosystem string - PackageName string - Version string + Ecosystem string + PackageName string + Version string + EscapedVersion string } // handleBrowseSource is now showBrowseSource in server.go, dispatched via handlePackagePath. @@ -601,12 +604,17 @@ func (s *Server) compareDiff(w http.ResponseWriter, r *http.Request, ecosystem, } // ComparePageData contains data for the version comparison page. +// +// FromVersion and ToVersion are decoded, for display; the Escaped variants are +// the path-segment form used to build the compare API URL. type ComparePageData struct { Layout - Ecosystem string - PackageName string - FromVersion string - ToVersion string + Ecosystem string + PackageName string + FromVersion string + ToVersion string + EscapedFromVersion string + EscapedToVersion string } // handleComparePage is now showComparePage in server.go, dispatched via handlePackagePath. diff --git a/internal/server/browse_test.go b/internal/server/browse_test.go index 6ea0f7e..3cc37c8 100644 --- a/internal/server/browse_test.go +++ b/internal/server/browse_test.go @@ -450,8 +450,10 @@ func TestHandleBrowseSourcePage(t *testing.T) { if !strings.Contains(body, "const packageName = 'test-browse'") { t.Error("browse source page missing packageName variable") } - if !strings.Contains(body, "const version = '1.0.0'") { - t.Error("browse source page missing version variable") + // The version reaches the browse API as one path segment, so the page holds + // its escaped form. + if !strings.Contains(body, "const versionPath = '1.0.0'") { + t.Error("browse source page missing versionPath variable") } // Verify content type @@ -617,12 +619,13 @@ func TestHandleComparePage(t *testing.T) { body := w.Body.String() - // Check that versions are set correctly in JavaScript - if !strings.Contains(body, "const fromVersion = '1.0.0'") { - t.Error("page should set fromVersion") + // Check that versions are set correctly in JavaScript. The compare API takes + // each version as a path segment, so the page holds their escaped forms. + if !strings.Contains(body, "const fromVersionPath = '1.0.0'") { + t.Error("page should set fromVersionPath") } - if !strings.Contains(body, "const toVersion = '2.0.0'") { - t.Error("page should set toVersion") + if !strings.Contains(body, "const toVersionPath = '2.0.0'") { + t.Error("page should set toVersionPath") } // Test invalid format (missing separator) diff --git a/internal/server/resolve.go b/internal/server/resolve.go index 51f203d..f7a1b23 100644 --- a/internal/server/resolve.go +++ b/internal/server/resolve.go @@ -2,10 +2,13 @@ package server import ( "fmt" + "net/http" + "net/url" "strings" "unicode" "github.com/git-pkgs/proxy/internal/database" + "github.com/go-chi/chi/v5" ) // maxPackagePathLen bounds the wildcard portion of package routes (name plus @@ -13,22 +16,69 @@ import ( // longer, so 512 leaves room without admitting pathological inputs. const maxPackagePathLen = 512 +// packagePathSegments validates the wildcard portion of a package route and +// splits it into decoded path segments. +func packagePathSegments(r *http.Request) ([]string, error) { + wildcard := chi.URLParam(r, "*") + encoded := wildcardIsEncoded(r) + if err := validatePackagePath(wildcard, encoded); err != nil { + return nil, err + } + + return splitWildcardPath(wildcard, encoded), nil +} + +// wildcardIsEncoded reports whether the chi wildcard for this request is still +// percent-encoded. +// +// chi routes on r.URL.RawPath when it is set and on r.URL.Path otherwise, and +// net/url only sets RawPath when the request's escaping differs from the +// canonical encoding of the decoded path. A version such as "release%2F1" is +// therefore routed raw, while "1.0%252B" (a version whose text contains a +// literal "%2B") encodes canonically and arrives already decoded once. The +// distinction decides whether the segments still need decoding: decoding the +// second case again would turn it into "1.0+" and resolve a different version. +func wildcardIsEncoded(r *http.Request) bool { + return r.URL.RawPath != "" +} + // validatePackagePath rejects wildcard package paths that cannot be valid in // any supported ecosystem. It is a coarse filter applied before database or // enrichment lookups; ecosystem-specific name rules are layered on top. -func validatePackagePath(path string) error { +// +// encoded has the meaning described on wildcardIsEncoded. +func validatePackagePath(path string, encoded bool) error { if path == "" { return fmt.Errorf("package name required") } if len(path) > maxPackagePathLen { return fmt.Errorf("package path exceeds %d bytes", maxPackagePathLen) } - for _, r := range path { - if r == 0 { - return fmt.Errorf("package path contains null byte") - } - if unicode.IsControl(r) { - return fmt.Errorf("package path contains control character %#U", r) + // Validate the decoded segments: the handlers work with decoded values, so + // an escape such as "%00" or "%2E%2E" must not slip past these checks. + for _, seg := range splitWildcardPath(path, encoded) { + // Each segment is checked both as the handlers see it and decoded once + // more: a segment can reach a handler with escapes intact, and the + // upstream registry is then the one that decodes them. + for _, value := range []string{seg, decodePathSegment(seg)} { + // A decoded segment can itself contain slashes (from "%2F"), and + // the segments are later rejoined into a package name that + // registries interpolate straight into an upstream URL. Check every + // path element, not just the segment as a whole, or + // "a%2F..%2F..%2Fb" traverses. + for _, elem := range strings.Split(value, "/") { + if elem == ".." { + return fmt.Errorf("package path contains parent directory segment") + } + } + for _, r := range value { + if r == 0 { + return fmt.Errorf("package path contains null byte") + } + if unicode.IsControl(r) { + return fmt.Errorf("package path contains control character %#U", r) + } + } } } return nil @@ -60,10 +110,37 @@ func resolvePackageName(db *database.DB, ecosystem string, segments []string) (n // splitWildcardPath splits a chi wildcard path value into segments, // trimming any leading/trailing slashes. -func splitWildcardPath(path string) []string { +// +// When encoded is set the value is still percent-encoded (see +// wildcardIsEncoded), so each segment is decoded after splitting. Splitting +// first keeps an encoded "%2F" inside a name from being mistaken for a +// separator. Decoding matters for versions such as "1.0%2Bbuild1", which must +// reach the handlers as "1.0+build1" so that rebuilding the PURL yields the +// value that was stored rather than a double-encoded one. +func splitWildcardPath(path string, encoded bool) []string { path = strings.Trim(path, "/") if path == "" { return nil } - return strings.Split(path, "/") + segments := strings.Split(path, "/") + if !encoded { + return segments + } + for i, seg := range segments { + segments[i] = decodePathSegment(seg) + } + return segments +} + +// decodePathSegment percent-decodes a single URL path segment, returning it +// unchanged if it is not valid percent-encoding. +func decodePathSegment(seg string) string { + if !strings.Contains(seg, "%") { + return seg + } + decoded, err := url.PathUnescape(seg) + if err != nil { + return seg + } + return decoded } diff --git a/internal/server/resolve_test.go b/internal/server/resolve_test.go index dd7d2dc..1867f46 100644 --- a/internal/server/resolve_test.go +++ b/internal/server/resolve_test.go @@ -1,12 +1,15 @@ package server import ( + "net/http" + "net/http/httptest" "os" "path/filepath" "strings" "testing" "github.com/git-pkgs/proxy/internal/database" + "github.com/go-chi/chi/v5" ) func newTestDB(t *testing.T) (*database.DB, func()) { @@ -95,26 +98,44 @@ func TestResolvePackageName(t *testing.T) { func TestSplitWildcardPath(t *testing.T) { tests := []struct { - input string - want []string + input string + encoded bool + want []string }{ - {"lodash", []string{"lodash"}}, - {"lodash/4.17.21", []string{"lodash", "4.17.21"}}, - {"monolog/monolog", []string{"monolog", "monolog"}}, - {"symfony/console/6.0.0/browse", []string{"symfony", "console", "6.0.0", "browse"}}, - {"", nil}, - {"/", nil}, + {"lodash", false, []string{"lodash"}}, + {"lodash/4.17.21", false, []string{"lodash", "4.17.21"}}, + {"monolog/monolog", false, []string{"monolog", "monolog"}}, + {"symfony/console/6.0.0/browse", false, []string{"symfony", "console", "6.0.0", "browse"}}, + {"", false, nil}, + {"/", false, nil}, + // chi routes on the raw path when it differs from the canonical + // encoding of the decoded path, so segments arrive percent-encoded and + // must be decoded. + { + "nmap/7.91%2Bdfsg1%2Breally7.80%2Bdfsg1-2ubuntu0.1", true, + []string{"nmap", "7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1"}, + }, + {"%40babel/core/7.0.0", true, []string{"@babel", "core", "7.0.0"}}, + // An encoded separator stays inside its segment rather than splitting. + {"vendor%2Fname/1.0.0", true, []string{"vendor/name", "1.0.0"}}, + // Invalid escapes are passed through untouched. + {"lodash/1.0%zz", true, []string{"lodash", "1.0%zz"}}, + // When chi routed on the already-decoded path, an escape that survived + // is part of the value: a version whose text is "1.0%2B" reaches here + // as "1.0%2B" and decoding it again would yield "1.0+". + {"nmap/1.0%2B", false, []string{"nmap", "1.0%2B"}}, } for _, tt := range tests { - got := splitWildcardPath(tt.input) + got := splitWildcardPath(tt.input, tt.encoded) if len(got) != len(tt.want) { - t.Errorf("splitWildcardPath(%q) = %v, want %v", tt.input, got, tt.want) + t.Errorf("splitWildcardPath(%q, %v) = %v, want %v", tt.input, tt.encoded, got, tt.want) continue } for i := range got { if got[i] != tt.want[i] { - t.Errorf("splitWildcardPath(%q)[%d] = %q, want %q", tt.input, i, got[i], tt.want[i]) + t.Errorf("splitWildcardPath(%q, %v)[%d] = %q, want %q", + tt.input, tt.encoded, i, got[i], tt.want[i]) } } } @@ -132,8 +153,19 @@ func TestValidatePackagePath(t *testing.T) { {"composer namespaced", "symfony/console/6.0.0", false}, {"maven coordinates", "org.apache.commons/commons-lang3/3.12.0", false}, {"unicode", "café/1.0.0", false}, + {"encoded plus in version", "nmap/7.91%2Bdfsg1-2ubuntu0.1", false}, {"empty", "", true}, {"null byte", "lodash\x00/4.17.21", true}, + {"encoded null byte", "lodash/%00", true}, + {"encoded newline", "lodash/1.0%0A", true}, + {"parent segment", "lodash/../4.17.21", true}, + {"encoded parent segment", "lodash/%2E%2E/4.17.21", true}, + // A decoded segment can contain slashes, so traversal can hide inside + // one segment. Registries interpolate the resolved name straight into + // an upstream URL, and Go sends dot-segments verbatim. + {"traversal inside one segment", "pkg%2F..%2F..%2Fadmin", true}, + {"traversal via encoded dots and slash", "pkg%2f%2e%2e%2fadmin", true}, + {"encoded slash alone is allowed", "vendor%2Fname/1.0.0", false}, {"null byte suffix", "lodash\x00", true}, {"newline", "lodash\n4.17.21", true}, {"carriage return", "lodash\r", true}, @@ -145,9 +177,64 @@ func TestValidatePackagePath(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := validatePackagePath(tt.path) - if (err != nil) != tt.wantErr { - t.Errorf("validatePackagePath(%q) error = %v, wantErr %v", tt.path, err, tt.wantErr) + // The verdict must not depend on whether chi routed on the raw or + // on the already-decoded path: an escape that reaches a handler + // undecoded is decoded by the upstream registry instead, so it is + // rejected either way. + for _, encoded := range []bool{false, true} { + err := validatePackagePath(tt.path, encoded) + if (err != nil) != tt.wantErr { + t.Errorf("validatePackagePath(%q, %v) error = %v, wantErr %v", + tt.path, encoded, err, tt.wantErr) + } + } + }) + } +} + +// TestPackagePathSegments drives the real router, which is what decides whether +// the wildcard still carries percent-encoding. Go decodes the request path +// itself unless the escaping is non-canonical, so the same version can arrive +// either way and only one of the two forms may be decoded again. +func TestPackagePathSegments(t *testing.T) { + tests := []struct { + name string + target string + want []string + }{ + {"plain", "/pkg/npm/lodash/4.17.21", []string{"lodash", "4.17.21"}}, + {"encoded plus", "/pkg/deb/nmap/7.91%2Bdfsg1-2ubuntu0.1", []string{"nmap", "7.91+dfsg1-2ubuntu0.1"}}, + {"decoded plus", "/pkg/deb/nmap/7.91+dfsg1-2ubuntu0.1", []string{"nmap", "7.91+dfsg1-2ubuntu0.1"}}, + // An encoded slash is one segment, not a separator. + {"encoded slash", "/pkg/composer/vendor%2Fname/1.0.0", []string{"vendor/name", "1.0.0"}}, + {"question mark", "/pkg/npm/example/v1%3Fbuild", []string{"example", "v1?build"}}, + // "1.0%252B" is the escaped form of the version "1.0%2B"; net/url + // already decoded it once, so it must not be decoded again. + {"literal percent escape", "/pkg/npm/example/1.0%252B", []string{"example", "1.0%2B"}}, + {"browse suffix", "/pkg/deb/nmap/7.91%2Bdfsg1/browse", []string{"nmap", "7.91+dfsg1", "browse"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var got []string + var gotErr error + + router := chi.NewRouter() + router.Get("/pkg/{ecosystem}/*", func(_ http.ResponseWriter, r *http.Request) { + got, gotErr = packagePathSegments(r) + }) + router.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", tt.target, nil)) + + if gotErr != nil { + t.Fatalf("packagePathSegments(%q) failed: %v", tt.target, gotErr) + } + if len(got) != len(tt.want) { + t.Fatalf("segments for %q = %v, want %v", tt.target, got, tt.want) + } + for i := range got { + if got[i] != tt.want[i] { + t.Errorf("segments for %q [%d] = %q, want %q", tt.target, i, got[i], tt.want[i]) + } } }) } diff --git a/internal/server/server.go b/internal/server/server.go index c98d1cb..e677bc9 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -54,6 +54,7 @@ import ( "fmt" "log/slog" "net/http" + "net/url" "strconv" "strings" "time" @@ -680,12 +681,11 @@ func (s *Server) handlePackagesList(w http.ResponseWriter, r *http.Request) { // {name}/compare/{v1}...{v2} -> compare versions func (s *Server) handlePackagePath(w http.ResponseWriter, r *http.Request) { ecosystem := chi.URLParam(r, "ecosystem") - wildcard := chi.URLParam(r, "*") - if err := validatePackagePath(wildcard); err != nil { + segments, err := packagePathSegments(r) + if err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } - segments := splitWildcardPath(wildcard) if ecosystem == "" || len(segments) == 0 { http.Error(w, "ecosystem and package name required", http.StatusBadRequest) @@ -825,10 +825,11 @@ func (s *Server) showVersion(w http.ResponseWriter, r *http.Request, ecosystem, func (s *Server) showBrowseSource(w http.ResponseWriter, r *http.Request, ecosystem, name, version string) { data := BrowseSourceData{ - Layout: s.layoutFor(r), - Ecosystem: ecosystem, - PackageName: name, - Version: version, + Layout: s.layoutFor(r), + Ecosystem: ecosystem, + PackageName: name, + Version: version, + EscapedVersion: url.PathEscape(version), } if err := s.templates.Render(w, "browse_source", data); err != nil { @@ -846,11 +847,13 @@ func (s *Server) showComparePage(w http.ResponseWriter, r *http.Request, ecosyst } data := ComparePageData{ - Layout: s.layoutFor(r), - Ecosystem: ecosystem, - PackageName: name, - FromVersion: parts[0], - ToVersion: parts[1], + Layout: s.layoutFor(r), + Ecosystem: ecosystem, + PackageName: name, + FromVersion: parts[0], + ToVersion: parts[1], + EscapedFromVersion: url.PathEscape(parts[0]), + EscapedToVersion: url.PathEscape(parts[1]), } if err := s.templates.Render(w, "compare_versions", data); err != nil { diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 2d27147..98b58cc 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -4,12 +4,16 @@ import ( "database/sql" "encoding/json" "fmt" + "html" "io" "log/slog" "net/http" "net/http/httptest" + "net/url" "os" "path/filepath" + "regexp" + "strconv" "strings" "testing" "time" @@ -18,6 +22,7 @@ import ( "github.com/git-pkgs/proxy/internal/database" "github.com/git-pkgs/proxy/internal/handler" "github.com/git-pkgs/proxy/internal/storage" + "github.com/git-pkgs/purl" "github.com/git-pkgs/registries/fetch" "github.com/go-chi/chi/v5" ) @@ -764,6 +769,236 @@ func TestVersionShowPage_NotFoundServer(t *testing.T) { } } +// TestVersionShowPage_PlusInVersion covers Debian/Ubuntu style versions such as +// nmap's "7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1". PURL percent-encodes "+" as +// "%2B", so the UI must show the decoded version and resolve both the decoded +// and the still-encoded form of the URL back to the same version. +func TestVersionShowPage_PlusInVersion(t *testing.T) { + ts := newTestServer(t) + defer ts.close() + + const version = "7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1" + const versionPURL = "pkg:deb/nmap@7.91%2Bdfsg1%2Breally7.80%2Bdfsg1-2ubuntu0.1" + + pkg := &database.Package{PURL: "pkg:deb/nmap", Ecosystem: "deb", Name: "nmap"} + if err := ts.db.UpsertPackage(pkg); err != nil { + t.Fatalf("failed to upsert package: %v", err) + } + if err := ts.db.UpsertVersion(&database.Version{ + PURL: versionPURL, PackagePURL: pkg.PURL, + }); err != nil { + t.Fatalf("failed to upsert version: %v", err) + } + + // The package page must link to and display the decoded version. + req := httptest.NewRequest("GET", "/ui/package/deb/nmap", nil) + w := httptest.NewRecorder() + ts.handler.ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Fatalf("package page: expected status 200, got %d", w.Code) + } + body := w.Body.String() + if strings.Contains(body, "%2B") { + t.Error("package page leaks PURL percent-encoding into the UI") + } + // html/template renders "+" as the "+" entity inside attributes and text. + if !strings.Contains(body, "7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1") { + t.Error("expected package page to show the decoded version") + } + + // Both the decoded and the encoded URL must reach the version page. + for _, path := range []string{ + "/ui/package/deb/nmap/" + version, + "/ui/package/deb/nmap/7.91%2Bdfsg1%2Breally7.80%2Bdfsg1-2ubuntu0.1", + } { + req := httptest.NewRequest("GET", path, nil) + w := httptest.NewRecorder() + ts.handler.ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Errorf("GET %s: expected status 200, got %d", path, w.Code) + } + } +} + +// TestVersionURLEscaping covers versions whose characters are significant in a +// URL path: "/" splits off another path segment, "?" starts a query string, and +// a literal "%xx" is read back as the character it encodes. The pages show the +// decoded version but must build every link from a separately escaped value, +// and those links have to resolve back to the same version. +func TestVersionURLEscaping(t *testing.T) { + // A second version is needed for the compare controls to be rendered. + const otherVersion = "1.0.0" + + tests := []struct { + name string + version string + }{ + {"slash", "release/1"}, + {"question mark", "v1?build"}, + {"literal percent escape", "1.0%2B"}, + {"plus", "7.91+dfsg1-2ubuntu0.1"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ts := newTestServer(t) + defer ts.close() + seedEscapingVersions(t, ts.db, tt.version, otherVersion) + + // The package page links to the escaped version. + escaped := url.PathEscape(tt.version) + versionPath := "/ui/package/deb/nmap/" + escaped + packagePage := ts.getOK(t, "/ui/package/deb/nmap") + if !containsValue(attrValues(packagePage, "href"), versionPath) { + t.Fatalf("package page has no link to %q; hrefs: %v", + versionPath, attrValues(packagePage, "href")) + } + + ts.checkVersionAndBrowsePages(t, versionPath, tt.version, escaped) + ts.checkComparePage(t, packagePage, tt.version, escaped, otherVersion) + }) + } +} + +// seedEscapingVersions stores a Debian package with the given versions, each +// with a cached artifact so that the version page offers its browse link. +func seedEscapingVersions(t *testing.T, db *database.DB, versions ...string) { + t.Helper() + + pkg := &database.Package{PURL: "pkg:deb/nmap", Ecosystem: "deb", Name: "nmap"} + if err := db.UpsertPackage(pkg); err != nil { + t.Fatalf("failed to upsert package: %v", err) + } + for _, v := range versions { + versionPURL := purl.MakePURLString("deb", "nmap", v) + if err := db.UpsertVersion(&database.Version{ + PURL: versionPURL, PackagePURL: pkg.PURL, + }); err != nil { + t.Fatalf("failed to upsert version %q: %v", v, err) + } + if err := db.UpsertArtifact(&database.Artifact{ + VersionPURL: versionPURL, + Filename: "nmap.deb", + UpstreamURL: "http://archive.ubuntu.com/ubuntu/pool/universe/n/nmap/nmap.deb", + StoragePath: sql.NullString{String: "/cache/nmap.deb", Valid: true}, + FetchedAt: sql.NullTime{Time: time.Now(), Valid: true}, + }); err != nil { + t.Fatalf("failed to upsert artifact for %q: %v", v, err) + } + } +} + +// checkVersionAndBrowsePages follows a version link from the package page and +// then the browse link from the version page, checking that both resolve to the +// stored version and display it decoded. +func (ts *testServer) checkVersionAndBrowsePages(t *testing.T, versionPath, version, escaped string) { + t.Helper() + + versionPage := ts.getOK(t, versionPath) + wantPURL := "pkg:deb/nmap@" + version + if !strings.Contains(html.UnescapeString(versionPage), wantPURL) { + t.Errorf("version page does not show %q", wantPURL) + } + + browsePath := versionPath + "/browse" + if !containsValue(attrValues(versionPage, "href"), browsePath) { + t.Fatalf("version page has no browse link to %q; hrefs: %v", + browsePath, attrValues(versionPage, "href")) + } + + browsePage := ts.getOK(t, browsePath) + if !strings.Contains(html.UnescapeString(browsePage), "nmap@"+version) { + t.Errorf("browse page does not show the decoded version %q", version) + } + // The browse API is called with the escaped version, not with the text shown + // in the heading. + if got := jsConstant(t, browsePage, "versionPath"); got != escaped { + t.Errorf("browse page passes %q to the browse API, want %q", got, escaped) + } +} + +// checkComparePage builds the compare URL the way the package page's script +// does, from the values its checkboxes carry, and checks the page it reaches. +func (ts *testServer) checkComparePage(t *testing.T, packagePage, version, escaped, otherVersion string) { + t.Helper() + + selectable := attrValues(packagePage, "data-version-path") + if !containsValue(selectable, escaped) { + t.Fatalf("package page compare data holds %v, want %q", selectable, escaped) + } + + comparePage := ts.getOK(t, "/ui/package/deb/nmap/compare/"+escaped+"..."+otherVersion) + decoded := html.UnescapeString(comparePage) + for _, want := range []string{version, otherVersion} { + if !strings.Contains(decoded, want) { + t.Errorf("compare page does not show version %q", want) + } + } + if got := jsConstant(t, comparePage, "fromVersionPath"); got != escaped { + t.Errorf("compare page passes %q to the compare API, want %q", got, escaped) + } +} + +// getOK performs a GET against the server and fails the test unless it returns +// 200, returning the response body. +func (ts *testServer) getOK(t *testing.T, path string) string { + t.Helper() + + req := httptest.NewRequest("GET", path, nil) + w := httptest.NewRecorder() + ts.handler.ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Fatalf("GET %s: expected status 200, got %d", path, w.Code) + } + + return w.Body.String() +} + +// attrValues returns the value of every occurrence of an HTML attribute in a +// rendered page, with HTML entities resolved so that values can be compared +// against the raw strings they were built from. +func attrValues(body, attr string) []string { + re := regexp.MustCompile(regexp.QuoteMeta(attr) + `="([^"]*)"`) + + var values []string + for _, match := range re.FindAllStringSubmatch(body, -1) { + values = append(values, html.UnescapeString(match[1])) + } + + return values +} + +// jsConstant returns the value of a single-quoted JavaScript string constant in +// a rendered page. html/template escapes characters that are significant in +// JavaScript, rendering "+" as "\\u002b" for instance, so the escapes are +// resolved to recover the value the page actually uses. +func jsConstant(t *testing.T, body, name string) string { + t.Helper() + + re := regexp.MustCompile(`const ` + regexp.QuoteMeta(name) + ` = '([^']*)'`) + match := re.FindStringSubmatch(body) + if match == nil { + t.Fatalf("page does not declare the constant %q", name) + } + + unescaped, err := strconv.Unquote(`"` + match[1] + `"`) + if err != nil { + t.Fatalf("cannot unescape %q: %v", match[1], err) + } + + return unescaped +} + +func containsValue(values []string, want string) bool { + for _, v := range values { + if v == want { + return true + } + } + + return false +} + func TestPackageShowPage_WithLicense(t *testing.T) { ts := newTestServer(t) defer ts.close() diff --git a/internal/server/templates/pages/browse_source.html b/internal/server/templates/pages/browse_source.html index ca06652..a949111 100644 --- a/internal/server/templates/pages/browse_source.html +++ b/internal/server/templates/pages/browse_source.html @@ -7,7 +7,7 @@ / {{.PackageName}} / - {{.Version}} + {{.Version}} / Browse Source @@ -51,7 +51,10 @@