Skip to content

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

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

[DNM] fix(security): backport v0.48.0 fixes to v0.39.x#2762
chmouel wants to merge 4 commits into
release-v0.39.xfrom
security-header-hijacking-v0-39-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.39.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.39.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 the remaining dependency security bump needed for this release stream: go-jose/v4 replacement to v4.1.4. go-jose/v3 and tektoncd/pipeline were already at fixed versions on this branch.

👨🏻‍ 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

Full pre-push hooks were not used for the final push because an earlier concurrent push hit golangci-lint's single-process lock. The same make lint-go check passed locally when run sequentially.

🤖 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 4 commits June 3, 2026 13:38
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: 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 enhancements, including validating GitHub App webhook signatures and enterprise hosts before minting tokens, scoping GitHub App tokens, and deep-copying cached remote pipelines and tasks to prevent shared cache mutation. It also updates dependencies, configures the Go toolchain, and refactors logging and string building. Feedback on these changes highlights a critical issue in the Makefile where a non-existent golangci-lint version (v2.12.2) is specified, which will break the build, and a potential nil pointer dereference panic in pkg/resolve/remote.go if the pipeline is not successfully resolved before calling .DeepCopy().

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 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 (golangci-lint is currently on v1.x). Attempting to download this non-existent version will cause the curl command to fail with a 404 Not Found error, breaking both local development and CI linting. Since the Go toolchain is pinned to go1.24.13, please use a version of golangci-lint that supports Go 1.24, such as v1.64.5.

GOLANGCI_LINT_VERSION ?= v1.64.5

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.

high

If fetchedResourcesForPipelineRun.Pipeline is nil (for example, if the pipeline could not be resolved or found), calling .DeepCopy() on it will trigger a nil pointer dereference panic. To ensure robustness and prevent panics, add a nil check before copying the pipeline.

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

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