Skip to content

[DNM] fix(security): backport v0.48.0 fixes to v0.42.x#2761

Draft
chmouel wants to merge 6 commits into
release-v0.42.xfrom
security-header-hijacking-v0-42-x
Draft

[DNM] fix(security): backport v0.48.0 fixes to v0.42.x#2761
chmouel wants to merge 6 commits into
release-v0.42.xfrom
security-header-hijacking-v0-42-x

Conversation

@chmouel
Copy link
Copy Markdown
Member

@chmouel chmouel commented Jun 4, 2026

📝 Description of the Change

[DNM] Backport of the security fixes released in v0.48.0 to release-v0.42.x.

This PR backports the public security hardening from v0.48.0:

  • Prevent GitHub Enterprise Host header hijacking before GitHub App token requests.
  • Scope GitHub App installation tokens to the triggering repository for remote task resolution.
  • Deep-copy cached remote Pipeline/Task resources before inlining so one PipelineRun cannot mutate cached resources used by another.
  • Redact incoming webhook query strings from logs so URL-based ?secret= values are not written to stdout.

How this backport was done:

  • Started from release-v0.42.x in a dedicated worktree.
  • Backported the security code paths from the v0.48.0 commits, resolving release-branch API differences manually.
  • Kept the query-string redaction as a separate commit with the original upstream author.
  • Added branch-local golangci/toolchain pinning so PAC CI and local make lint-go use the same known-good linter binary from tmp/.
  • Added dependency security bumps needed for this release stream: go-jose/v3, go-jose/v4, and tektoncd/pipeline.

👨🏻‍ Linked Jira

N/A

🔗 Linked GitHub Issue

N/A

🧪 Testing Strategy

  • Unit tests
  • Integration tests
  • End-to-end tests
  • Manual testing
  • Not Applicable

Validation run locally:

  • make test
  • make lint-go
  • go test ./pkg/adapter after the incoming webhook log redaction backport
  • git diff --check

The normal pre-push hooks completed successfully for this branch when pushed.

🤖 AI Assistance

  • I have not used any AI assistance for this PR.
  • I have used AI assistance for this PR.

If you have used AI assistance, please provide the following details:

Which LLM was used?

  • GitHub Copilot
  • ChatGPT (OpenAI)
  • Claude (Anthropic)
  • Cursor
  • Gemini (Google)
  • Other: ____________

Extent of AI Assistance:

  • Documentation and research only
  • Unit tests or E2E tests only
  • Code generation (parts of the code)
  • Full code generation (most of the PR)
  • PR description and comments
  • Commit message(s)

✅ Submitter Checklist

  • 📝 My commit messages are clear, informative, and follow the project's How to write a git commit message guide. The Gitlint linter ensures in CI it's properly validated
  • ✨ I have ensured my commit message prefix (e.g., fix:, feat:) matches the "Type of Change" I selected above.
  • ♽ I have run make test and make lint locally to check for and fix any
    issues. For an efficient workflow, I have considered installing
    pre-commit and running pre-commit install to
    automate these checks.
  • 📖 I have added or updated documentation for any user-facing changes.
  • 🧪 I have added sufficient unit tests for my code changes.
  • 🎁 I have added end-to-end tests where feasible. See README for more details.
  • 🔎 I have addressed any CI test flakiness or provided a clear reason to bypass it.
  • If adding a provider feature, I have filled in the following and updated the provider documentation:
    • GitHub App
    • GitHub Webhook
    • Gitea/Forgejo
    • GitLab
    • Bitbucket Cloud
    • Bitbucket Data Center

chmouel and others added 6 commits June 3, 2026 13:37
Validate GitHub App webhook payloads before minting tokens and avoid
trusting a forged enterprise host header. Also include the upstream App
token scoping and remote-resource deep-copy fixes needed for the same
supported release stream.

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
Download the branch-compatible golangci-lint binary into tmp and use it
from make lint-go. Pin GOTOOLCHAIN for local make targets and PAC Go
steps so linting does not drift with the host Go installation.

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
Signed-off-by: Shubham Bhardwaj <shubbhar@redhat.com>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several security and robustness improvements, including validating GitHub App webhook signatures and enterprise hosts before minting tokens, and deep-copying cached remote pipelines and tasks to prevent cross-run mutation leaks. It also updates various dependencies, pins the golangci-lint version in the Makefile, and migrates test requests to use context-aware constructors. The review feedback highlights three critical issues: a potential nil pointer dereference in pkg/resolve/remote.go when calling .DeepCopy() on an unchecked pipeline, an invalid golangci-lint version (v2.12.2) in the Makefile that will cause download failures, and a potential nil pointer dereference in pkg/provider/github/parse_payload.go if the retrieved Kubernetes secret's data is nil.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread pkg/resolve/remote.go
// if PipelineRef is used then, first resolve pipeline and replace all taskRef{Finally/Task} of Pipeline, then put inlinePipeline in PipelineRun
if pipelinerun.Spec.PipelineRef != nil && pipelinerun.Spec.PipelineRef.Resolver == "" {
pipelineResolved := fetchedResourcesForPipelineRun.Pipeline
pipelineResolved := fetchedResourcesForPipelineRun.Pipeline.DeepCopy()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

Calling .DeepCopy() on fetchedResourcesForPipelineRun.Pipeline without a nil check can lead to a nil pointer dereference panic if the pipeline fails to resolve or is not found. Ensure that fetchedResourcesForPipelineRun.Pipeline is not nil before copying.

			if fetchedResourcesForPipelineRun.Pipeline == nil {
				return nil, fmt.Errorf("pipeline %s not found", pipelinerun.Spec.PipelineRef.Name)
			}
			pipelineResolved := fetchedResourcesForPipelineRun.Pipeline.DeepCopy()

Comment thread Makefile
TARGET_NAMESPACE=pipelines-as-code
HUGO_VERSION=0.96.0
GOLANGCI_LINT=golangci-lint
GOLANGCI_LINT_VERSION ?= v2.12.2
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The version v2.12.2 for golangci-lint does not exist. The official golangci-lint releases use v1.y.z versioning (e.g., v1.62.2). Using an invalid version will cause the curl download command to fail with a 404 error, breaking the linting process.

GOLANGCI_LINT_VERSION ?= v1.62.2

if err != nil {
return "", err
}
return strings.TrimSpace(string(secret.Data[controllerWebhookSecretKey])), nil
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

If secret.Data is nil (which can happen if the Kubernetes Secret has no data fields), indexing it with secret.Data[controllerWebhookSecretKey] will cause a nil pointer dereference panic. A defensive check should be added to ensure secret.Data is not nil before accessing its keys. Additionally, use bytes.TrimSpace to trim whitespace from the secret data, following the project's convention.

Suggested change
return strings.TrimSpace(string(secret.Data[controllerWebhookSecretKey])), nil
if secret.Data == nil {
return "", fmt.Errorf("secret %s has no data", run.Info.Controller.Secret)
}
return string(bytes.TrimSpace(secret.Data[controllerWebhookSecretKey])), nil
References
  1. Webhook payloads should be trimmed of whitespace using bytes.TrimSpace before being processed, as this is the established convention in the project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants