Upgrade the umask fix to match upstream PR #9/#12 - #3
Merged
Conversation
…desktop#12 Our own fix (merged earlier in this fork) worked but was weaker than what independently showed up in telegramdesktop#9 and telegramdesktop#12 for the same bug (telegramdesktop#10): - The MTProxy permission fix only ran inside the "rebuild from source" branch of install-mtproxy.sh. A host where a previous run left /opt/MTProxy built with bad permissions (interrupted mid-way, or installed before this fix existed) would keep skipping the build on every later run - the pinned commit is already present - and so never reach the chmod. Move it after the build-or-skip block so it runs on every invocation. - It also granted a+rX to every user on the host. Narrowed to chown root:mtproxy + chmod 0750 on exactly objs, objs/bin, and the binary, matching the root:mtproxy 0750 scheme this installer already uses for /etc/mtproxy - confirmed the mtproxy service user's primary group actually is mtproxy before relying on that. - The go test fix wrapped the call site in install.sh ((umask 022; ...)), which only fixes it there; go test ./... run directly, outside install.sh, under a restrictive shell umask would still fail the same way. Fixed at the source instead: TestLoadAcceptsSystemdCredentialReadPermissions now chmods its own fixture explicitly after writing it, so the assertion means what it says regardless of the caller's umask. The now-unnecessary install.sh wrapper is reverted. Re-verified on a real host: go test ./... passes under umask 077 with no wrapper at the call site. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Our earlier umask fix (merged in #2) worked but is weaker than the fixes independently proposed upstream in telegramdesktop/tproxy-server#9 and #12 for telegramdesktop/tproxy-server#10. Adopting their approach here:
/opt/MTProxybuilt with bad permissions would keep skipping the (already-present, pinned-commit) build on every later run and never reach the fix. Moved after the build-or-skip block so it runs on every invocation. Also narrowed fromchmod -R a+rX(every user on the host) tochown root:mtproxy+chmod 0750on exactlyobjs,objs/bin, and the binary — matching theroot:mtproxy 0750scheme this installer already uses for/etc/mtproxy. Confirmed on the deployed host that themtproxyservice user's primary group actually ismtproxybefore relying on that.install.sh((umask 022; ...)), which only helps there —go test ./...run directly, outsideinstall.sh, under a restrictive shell umask would still fail the same way. Fixed at the source instead:TestLoadAcceptsSystemdCredentialReadPermissionsnowchmods its own fixture explicitly after writing it. The now-unnecessaryinstall.shwrapper is reverted.Test plan
go test ./...passes underumask 077with no wrapper at the call site (re-verified on a real host, this exact tree)gofmt -l .andgo vet ./...cleanid mtproxy→ primary groupmtproxyon the deployed host, so the narrowerchown root:mtproxygrants exactly what the service needsprofiles.json, which isn't something to do incidentally while verifying a permissions tweak🤖 Generated with Claude Code