Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

irregular mode file checks for Windows symlinks #79

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

notchairmk
Copy link
Member

@notchairmk notchairmk commented Jan 27, 2025

Adds checks for os.ModeIrregular anywhere where we were checking symlinks. This is required as of Go 1.23, additional discussion in golang/go#63703

Fixes hashicorp/terraform#36163

@Maed223 Maed223 force-pushed the check-mode-irregular branch 4 times, most recently from 7674941 to 2fbe084 Compare January 28, 2025 21:27
Copy link
Collaborator

@dduzgun-security dduzgun-security left a comment

Choose a reason for hiding this comment

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

Would it be better if we handle ModeIrregular in it's own condition block instead of the symlink one since ModeIrregular is for non-regular file; nothing else is known about this file which isn't technically a symlink and it could make things confusing?

@notchairmk
Copy link
Member Author

hey @dduzgun-security what this basically boils down to is that Windows and now Go only treat files as symlinks, not directories. the go issue mentioned in the description has more details on this.

in the context of go-slug, there is no distinction between symlinked directories and files. we could make this more specific like m&os.ModeSymlink != 0 || (m&os.ModeIrregular != 0 && runtime.GOOS == 'windows') or by adding comments describing what's going on. but it doesn't feel like making this check separate from the os.ModeSymlink checks is preferable.

@notchairmk
Copy link
Member Author

also just want to note that tests for Windows will fail on main already, they're not necessarily due to this change.

@dduzgun-security
Copy link
Collaborator

@notchairmk Thanks for the context, I would suggest a condition for Windows first by using the check runtime.GOOS == 'windows' then following up with the symlink & irregular file check to reduce the impact of the change. This library is quite sensitive and used by many, targeting the change for the affected environment would reduce the risk of incidents.

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.

Error on a plan or apply when calling a module from a local filesystem location
3 participants