Don't skip symlinks during local source upload by dollierp · Pull Request #355 · shipwright-io/cli

dollierp · 2025-11-06T09:30:15Z

Changes

Don't skip symlinks during local source upload

/kind bug

Submitter Checklist

Includes tests if functionality changed/was added
Includes docs if changes are user-facing
Set a kind label on this PR
Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

Content from symlinks is included during local source upload

IrvingMg

Thanks for the PR!

Overall, LGTM. I left a couple of comments. It would also be nice to have unit tests for at least basic cases, as the changes are related to the core functionality of the upload command.

dollierp · 2026-04-06T13:30:53Z

Hi @IrvingMg,

Thanks for your review.

I updated the PR with new changes trying to address your remarks.

Regards,

Copilot

Pull request overview

This PR fixes a bug where symlinks were being skipped during local source uploads. Previously, the tar helper would skip any file that wasn't a regular file (including symlinks, directories, and other special file types). The changes now allow symlinks to be included in the tar archive while still skipping unsupported file types.

Changes:

Modified skipPath function to exclude symlinks from being skipped (allowing them to be processed)
Added symlink handling in writeFileToTar to read symlink targets, detect if they point outside the source directory, and properly set the tar header linkname
Added isSymlinkTargetOutsideOfDir function to check if symlink targets escape the source directory and warn appropriately
Updated test to verify that symlinks are captured in the tar archive
Fixed documentation from "buildrun" to "build" command references

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/shp/streamer/tar.go	Modified skipPath condition to allow symlinks through filtering
pkg/shp/streamer/util.go	Added symlink handling logic with safety check for targets pointing outside source directory
pkg/shp/streamer/tar_test.go	Enhanced test to verify symlinks are included in tar output
pkg/shp/cmd/build/upload.go	Fixed documentation typo from "buildrun" to "build"
docs/shp_build_upload.md	Updated documentation examples to match corrected command

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

IrvingMg · 2026-04-08T13:54:39Z

Hi @dollierp, thanks for the changes.

There’s one more Copilot comment, could you take a look?

dollierp · 2026-04-08T14:07:01Z

Hi @IrvingMg,

I updated the PR to make Copilot happy.

Let me know if something else is missing.

Regards.

IrvingMg

Thanks again for the changes. I left a couple more comments.

dollierp · 2026-04-09T19:02:55Z

Hi @IrvingMg,

I tried to address your latest remarks and updated the PR.

Regards,

IrvingMg

Thanks for the update, looks good overall. Just a few final comments from my side.

dollierp · 2026-04-11T08:48:21Z

Hi @IrvingMg,

I've tried to incorporate your latest feedback and have updated the pull request.

Regards,

IrvingMg

Thanks for the changes! Overall /lgtm

@shipwright-io/approvers PTAL, this PR has been open for a while.

IrvingMg · 2026-04-12T11:21:32Z

/lgtm

adambkaplan

Code changes generally look good, however I'm going to ask that we turn the existing warning on using symlinks outside the "root" directory into a failure. This is a common source of vulnerabilities. I am actually pretty sure that in Shipwright's predecessor (OpenShift BuildConfig), we had to issue a CVE on this exact issue.

adambkaplan · 2026-04-20T12:51:07Z

+		if err := os.Symlink(symlink.target, path); err != nil {
+			t.Fatalf("Couldn't setup test symlink: %v", err)
+		}
+		t.Cleanup(func() { os.Remove(path) })


Golangci-lint failing here on an unhandled error from os.Remove. I'm fine capturing the error and logging it.

Suggested change

t.Cleanup(func() { os.Remove(path) })

t.Cleanup(func() {

cleanupErr := os.Remove(path)

if cleanupErr != nil {

t.Logf("failed to remove path %q: %v", path, err)

}

})

dollierp · 2026-04-21T23:30:05Z

Hi @adambkaplan,

Thanks for your review.

I pushed a new commit which should address your concerns.

Regards,

adambkaplan

/lgtm

Updates look good

adambkaplan

/approve

Adding "approve" here, now that we are safely handling symlinks that potentially escape the root directory. We'll need at least one more round of review before this is ready for merge.

adambkaplan · 2026-04-22T16:10:10Z

+
+		if outside, _ := isSymlinkTargetOutsideOfDir(absSrc, absTarget); outside {
+			relPath, _ := filepath.Rel(t.src, fpath)
+			return fmt.Errorf("symlink %q points outside the source directory %q (target: %q)\n", relPath, absSrc, target)


I held back on pointing this out as a nit, but our lint checker is failing because of the trailing newline.

Suggested change

return fmt.Errorf("symlink %q points outside the source directory %q (target: %q)\n", relPath, absSrc, target)

return fmt.Errorf("symlink %q points outside the source directory %q (target: %q)", relPath, absSrc, target)

openshift-ci · 2026-04-22T16:11:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [adambkaplan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

adambkaplan · 2026-04-22T16:13:06Z

/lgtm cancel

Due to failing lint check.

dollierp · 2026-04-22T16:38:07Z

Hi @adambkaplan,

I updated the PR to fix the \n lint issue.

Regards.

adambkaplan · 2026-04-22T17:09:15Z

@dollierp one last item - please squash your commits using an interactive rebase, and provide a well written commit message.

Side note for @shipwright-io/contributors - we should add commit squashing and commit message guidance to our Contributor Guide.

Until now, the `shp build upload <build-name> /path` command would silently discard symbolic links when walking through a local source directory. This is not backward compatible with the `oc start-build <build-name> --from-dir=/path` command which supports symbolic links. This change adds support for symbolic links to Shipwright cli to match the legacy OpenShift build commands. To prevent the type of vulnerability described by CWE-61 [1], an additional security measure is implemented: an error is raised upon detecting a symbolic link that points to a file outside the provided directory. [1]: https://cwe.mitre.org/data/definitions/61.html Signed-off-by: Denis Ollier <dollierp@redhat.com>

dollierp · 2026-04-22T19:56:50Z

Hi @adambkaplan,

I squashed the commits into a single one and improved its message to match the guidelines.

Regards,

adambkaplan · 2026-04-23T15:37:54Z

/lgtm

Failed e2e test appears to be a flake. Re-running (which will gate merge until it passes).

openshift-ci Bot added the release-note label Nov 6, 2025

pull-request-size Bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 6, 2025

openshift-ci Bot added the kind/bug Categorizes issue or PR as related to a bug. label Nov 6, 2025

openshift-ci Bot requested review from HeavyWombat and adambkaplan November 6, 2025 09:30

shipwright-ci-bot added this to Issues Nov 6, 2025

IrvingMg reviewed Mar 23, 2026

View reviewed changes

Comment thread pkg/shp/streamer/tar.go Outdated

Comment thread pkg/shp/streamer/util.go

Copilot AI review requested due to automatic review settings April 6, 2026 13:28

dollierp force-pushed the symlinks branch from ec7519c to 668d3e3 Compare April 6, 2026 13:28

Copilot started reviewing on behalf of dollierp April 6, 2026 13:29 View session

Copilot AI reviewed Apr 6, 2026

View reviewed changes

Comment thread pkg/shp/streamer/util.go Outdated

dollierp requested a review from IrvingMg April 6, 2026 13:37

dollierp force-pushed the symlinks branch from 668d3e3 to 7b317ce Compare April 8, 2026 14:04

IrvingMg reviewed Apr 8, 2026

View reviewed changes

Comment thread pkg/shp/streamer/util.go Outdated

Comment thread test/symlink/symlink_inside_tree Outdated

dollierp force-pushed the symlinks branch from 7b317ce to 1c914e6 Compare April 9, 2026 18:04

pull-request-size Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 9, 2026

dollierp requested a review from Copilot April 9, 2026 18:06

Copilot started reviewing on behalf of dollierp April 9, 2026 18:07 View session

dollierp requested a review from IrvingMg April 9, 2026 19:03

IrvingMg reviewed Apr 10, 2026

View reviewed changes

Comment thread pkg/shp/streamer/util.go

Comment thread pkg/shp/streamer/tar_test.go Outdated

Comment thread pkg/shp/streamer/tar_test.go Outdated

Comment thread pkg/shp/streamer/tar_test.go Outdated

dollierp force-pushed the symlinks branch from 1c914e6 to 70578f5 Compare April 11, 2026 08:44

dollierp requested a review from IrvingMg April 11, 2026 08:48

IrvingMg reviewed Apr 12, 2026

View reviewed changes

openshift-ci Bot assigned IrvingMg Apr 12, 2026

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 12, 2026

adambkaplan requested changes Apr 20, 2026

View reviewed changes

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 21, 2026

dollierp requested a review from adambkaplan April 21, 2026 23:30

adambkaplan approved these changes Apr 22, 2026

View reviewed changes

openshift-ci Bot assigned adambkaplan Apr 22, 2026

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2026

adambkaplan approved these changes Apr 22, 2026

View reviewed changes

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2026

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2026

dollierp force-pushed the symlinks branch from b0ec3b9 to f3ea201 Compare April 22, 2026 16:37

adambkaplan mentioned this pull request Apr 22, 2026

Add Commit Message Guidance to Contributor Guide shipwright-io/.github#17

Open

dollierp force-pushed the symlinks branch from f3ea201 to 9e0a340 Compare April 22, 2026 19:54

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2026

-		t.Cleanup(func() { os.Remove(path) })
+		t.Cleanup(func() {
+		    cleanupErr := os.Remove(path)
+		    if cleanupErr != nil {
+		        t.Logf("failed to remove path %q: %v", path, err)
+		    }
+		})

	return fmt.Errorf("symlink %q points outside the source directory %q (target: %q)\n", relPath, absSrc, target)
	return fmt.Errorf("symlink %q points outside the source directory %q (target: %q)", relPath, absSrc, target)

Conversation

dollierp commented Nov 6, 2025 • edited by adambkaplan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

IrvingMg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dollierp commented Apr 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

IrvingMg commented Apr 8, 2026

Uh oh!

dollierp commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IrvingMg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dollierp commented Apr 9, 2026

Uh oh!

IrvingMg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dollierp commented Apr 11, 2026

Uh oh!

IrvingMg left a comment

Choose a reason for hiding this comment

Uh oh!

IrvingMg commented Apr 12, 2026

Uh oh!

adambkaplan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adambkaplan Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dollierp commented Apr 21, 2026

Uh oh!

adambkaplan left a comment

Choose a reason for hiding this comment

Uh oh!

adambkaplan left a comment

Choose a reason for hiding this comment

Uh oh!

adambkaplan Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Apr 22, 2026

Uh oh!

adambkaplan commented Apr 22, 2026

Uh oh!

dollierp commented Apr 22, 2026

Uh oh!

adambkaplan commented Apr 22, 2026

Uh oh!

dollierp commented Apr 22, 2026

Uh oh!

adambkaplan commented Apr 23, 2026

Uh oh!

Reviewers

Assignees

Labels

dollierp commented Nov 6, 2025 •

edited by adambkaplan

Loading

dollierp commented Apr 8, 2026 •

edited

Loading