Skip to content

Commit

Permalink
Merge pull request sparkle-project#794 from zorgiepoo/fix-symlinks
Browse files Browse the repository at this point in the history 
Don't follow symbolic links for file operations
  • Loading branch information
@kornelski
kornelski committed Apr 12, 2016
2 parents bdb3dd8 + d263bf4 commit 7fdedad
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 21 deletions.
31 changes: 28 additions & 3 deletions Sparkle/SUFileManager.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,19 @@ - (BOOL)_changeOwnerAndGroupOfItemAtURL:(NSURL *)targetURL ownerID:(NSNumber *)o
return NO;
}

if (chown(path, ownerID.unsignedIntValue, groupID.unsignedIntValue) != 0) {
int fileDescriptor = open(path, O_RDONLY | O_SYMLINK);
if (fileDescriptor == -1) {
if (error != NULL) {
*error = [NSError errorWithDomain:NSPOSIXErrorDomain code:errno userInfo:@{ NSLocalizedDescriptionKey: [NSString stringWithFormat:@"Failed to open file descriptor to %@", targetURL.path.lastPathComponent] }];
}
return NO;
}

// We use fchown instead of chown because the latter can follow symbolic links
BOOL success = fchown(fileDescriptor, ownerID.unsignedIntValue, groupID.unsignedIntValue) == 0;
close(fileDescriptor);

if (!success) {
if (errno == EPERM) {
if (needsAuth != NULL) {
*needsAuth = YES;
Expand Down Expand Up @@ -701,7 +713,20 @@ - (BOOL)updateModificationAndAccessTimeOfItemAtURL:(NSURL *)targetURL error:(NSE
return NO;
}

if (utimes(path, NULL) == 0) {
int fileDescriptor = open(path, O_RDONLY | O_SYMLINK);
if (fileDescriptor == -1) {
if (error != NULL) {
*error = [NSError errorWithDomain:NSPOSIXErrorDomain code:errno userInfo:@{ NSLocalizedDescriptionKey: [NSString stringWithFormat:@"Failed to open file descriptor to %@", targetURL.path.lastPathComponent] }];
}
return NO;
}

// Using futimes() because utimes() follows symbolic links
BOOL updatedTime = (futimes(fileDescriptor, NULL) == 0);

close(fileDescriptor);

if (updatedTime) {
return YES;
}

Expand All @@ -716,7 +741,7 @@ - (BOOL)updateModificationAndAccessTimeOfItemAtURL:(NSURL *)targetURL error:(NSE
return NO;
}

BOOL success = AuthorizationExecuteWithPrivilegesAndWait(_auth, "/usr/bin/touch", kAuthorizationFlagDefaults, (char *[]){ "--", path, NULL });
BOOL success = AuthorizationExecuteWithPrivilegesAndWait(_auth, "/usr/bin/touch", kAuthorizationFlagDefaults, (char *[]){ "-h", "--", path, NULL });
if (!success && error != NULL) {
NSString *errorMessage = [NSString stringWithFormat:@"Failed to update modification & access time on %@ with authentication.", targetURL.path.lastPathComponent];
*error = [NSError errorWithDomain:SUSparkleErrorDomain code:SUAuthenticationFailure userInfo:@{ NSLocalizedDescriptionKey: errorMessage }];
Expand Down
81 changes: 63 additions & 18 deletions Tests/SUFileManagerTest.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import XCTest

class SUFileManagerTest: XCTestCase
{
func makeTempFiles(testBlock: (SUFileManager, NSURL, NSURL, NSURL, NSURL) -> Void)
func makeTempFiles(testBlock: (SUFileManager, NSURL, NSURL, NSURL, NSURL, NSURL, NSURL) -> Void)
{
let fileManager = SUFileManager(allowingAuthorization: false)

Expand All @@ -29,12 +29,18 @@ class SUFileManagerTest: XCTestCase
let fileInDirectoryURL = directoryURL.URLByAppendingPathComponent("a file inside a directory written by sparkles unit tests")
try! "bar baz".dataUsingEncoding(NSUTF8StringEncoding)!.writeToURL(fileInDirectoryURL, options: .DataWritingAtomic)

testBlock(fileManager, tempDirectoryURL, ordinaryFileURL, directoryURL, fileInDirectoryURL)
let validSymlinkURL = tempDirectoryURL.URLByAppendingPathComponent("symlink test")
try! NSFileManager.defaultManager().createSymbolicLinkAtURL(validSymlinkURL, withDestinationURL: directoryURL)

let invalidSymlinkURL = tempDirectoryURL.URLByAppendingPathComponent("symlink test 2")
try! NSFileManager.defaultManager().createSymbolicLinkAtURL(invalidSymlinkURL, withDestinationURL: tempDirectoryURL.URLByAppendingPathComponent("does not exist"))

testBlock(fileManager, tempDirectoryURL, ordinaryFileURL, directoryURL, fileInDirectoryURL, validSymlinkURL, invalidSymlinkURL)
}

func testMoveFiles()
{
makeTempFiles() { fileManager, rootURL, ordinaryFileURL, directoryURL, fileInDirectoryURL in
makeTempFiles() { fileManager, rootURL, ordinaryFileURL, directoryURL, fileInDirectoryURL, validSymlinkURL, invalidSymlinkURL in
XCTAssertNil(try? fileManager.moveItemAtURL(ordinaryFileURL, toURL: directoryURL))
XCTAssertNil(try? fileManager.moveItemAtURL(ordinaryFileURL, toURL: directoryURL.URLByAppendingPathComponent("foo").URLByAppendingPathComponent("bar")))
XCTAssertNil(try? fileManager.moveItemAtURL(rootURL.URLByAppendingPathComponent("does not exist"), toURL: directoryURL))
Expand All @@ -44,6 +50,17 @@ class SUFileManagerTest: XCTestCase
XCTAssertFalse(fileManager._itemExistsAtURL(ordinaryFileURL))
XCTAssertTrue(fileManager._itemExistsAtURL(newFileURL))

let newValidSymlinkURL = (ordinaryFileURL.URLByDeletingLastPathComponent?.URLByAppendingPathComponent("new symlink"))!
try! fileManager.moveItemAtURL(validSymlinkURL, toURL: newValidSymlinkURL)
XCTAssertFalse(fileManager._itemExistsAtURL(validSymlinkURL))
XCTAssertTrue(fileManager._itemExistsAtURL(newValidSymlinkURL))
XCTAssertTrue(fileManager._itemExistsAtURL(directoryURL))

let newInvalidSymlinkURL = (ordinaryFileURL.URLByDeletingLastPathComponent?.URLByAppendingPathComponent("new invalid symlink"))!
try! fileManager.moveItemAtURL(invalidSymlinkURL, toURL: newInvalidSymlinkURL)
XCTAssertFalse(fileManager._itemExistsAtURL(invalidSymlinkURL))
XCTAssertTrue(fileManager._itemExistsAtURL(newValidSymlinkURL))

let newDirectoryURL = (ordinaryFileURL.URLByDeletingLastPathComponent?.URLByAppendingPathComponent("new directory"))!
try! fileManager.moveItemAtURL(directoryURL, toURL: newDirectoryURL)
XCTAssertFalse(fileManager._itemExistsAtURL(directoryURL))
Expand All @@ -55,7 +72,7 @@ class SUFileManagerTest: XCTestCase

func testMoveFilesToTrash()
{
makeTempFiles() { fileManager, rootURL, ordinaryFileURL, directoryURL, fileInDirectoryURL in
makeTempFiles() { fileManager, rootURL, ordinaryFileURL, directoryURL, fileInDirectoryURL, validSymlinkURL, invalidSymlinkURL in
XCTAssertNil(try? fileManager.moveItemAtURLToTrash(rootURL.URLByAppendingPathComponent("does not exist")))

let trashURL = try! NSFileManager.defaultManager().URLForDirectory(.TrashDirectory, inDomain: .UserDomainMask, appropriateForURL: nil, create: false)
Expand All @@ -67,6 +84,12 @@ class SUFileManagerTest: XCTestCase
XCTAssertTrue(fileManager._itemExistsAtURL(ordinaryFileTrashURL))
try! fileManager.removeItemAtURL(ordinaryFileTrashURL)

let validSymlinkTrashURL = trashURL.URLByAppendingPathComponent(validSymlinkURL.lastPathComponent!)
try! fileManager.moveItemAtURLToTrash(validSymlinkURL)
XCTAssertTrue(fileManager._itemExistsAtURL(validSymlinkTrashURL))
XCTAssertTrue(fileManager._itemExistsAtURL(directoryURL))
try! fileManager.removeItemAtURL(validSymlinkTrashURL)

try! fileManager.moveItemAtURLToTrash(directoryURL)
XCTAssertFalse(fileManager._itemExistsAtURL(directoryURL))
XCTAssertFalse(fileManager._itemExistsAtURL(fileInDirectoryURL))
Expand All @@ -81,7 +104,7 @@ class SUFileManagerTest: XCTestCase

func testCopyFiles()
{
makeTempFiles() { fileManager, rootURL, ordinaryFileURL, directoryURL, fileInDirectoryURL in
makeTempFiles() { fileManager, rootURL, ordinaryFileURL, directoryURL, fileInDirectoryURL, validSymlinkURL, invalidSymlinkURL in
XCTAssertNil(try? fileManager.copyItemAtURL(ordinaryFileURL, toURL: directoryURL))
XCTAssertNil(try? fileManager.copyItemAtURL(ordinaryFileURL, toURL: directoryURL.URLByAppendingPathComponent("foo").URLByAppendingPathComponent("bar")))
XCTAssertNil(try? fileManager.copyItemAtURL(rootURL.URLByAppendingPathComponent("does not exist"), toURL: directoryURL))
Expand All @@ -91,6 +114,10 @@ class SUFileManagerTest: XCTestCase
XCTAssertTrue(fileManager._itemExistsAtURL(ordinaryFileURL))
XCTAssertTrue(fileManager._itemExistsAtURL(newFileURL))

let newSymlinkURL = (ordinaryFileURL.URLByDeletingLastPathComponent?.URLByAppendingPathComponent("new symlink file"))!
try! fileManager.copyItemAtURL(invalidSymlinkURL, toURL: newSymlinkURL)
XCTAssertTrue(fileManager._itemExistsAtURL(newSymlinkURL))

let newDirectoryURL = (ordinaryFileURL.URLByDeletingLastPathComponent?.URLByAppendingPathComponent("new directory"))!
try! fileManager.copyItemAtURL(directoryURL, toURL: newDirectoryURL)
XCTAssertTrue(fileManager._itemExistsAtURL(directoryURL))
Expand All @@ -102,12 +129,16 @@ class SUFileManagerTest: XCTestCase

func testRemoveFiles()
{
makeTempFiles() { fileManager, rootURL, ordinaryFileURL, directoryURL, fileInDirectoryURL in
makeTempFiles() { fileManager, rootURL, ordinaryFileURL, directoryURL, fileInDirectoryURL, validSymlinkURL, invalidSymlinkURL in
XCTAssertNil(try? fileManager.removeItemAtURL(rootURL.URLByAppendingPathComponent("does not exist")))

try! fileManager.removeItemAtURL(ordinaryFileURL)
XCTAssertFalse(fileManager._itemExistsAtURL(ordinaryFileURL))

try! fileManager.removeItemAtURL(validSymlinkURL)
XCTAssertFalse(fileManager._itemExistsAtURL(validSymlinkURL))
XCTAssertTrue(fileManager._itemExistsAtURL(directoryURL))

try! fileManager.removeItemAtURL(directoryURL)
XCTAssertFalse(fileManager._itemExistsAtURL(directoryURL))
XCTAssertFalse(fileManager._itemExistsAtURL(fileInDirectoryURL))
Expand All @@ -116,9 +147,10 @@ class SUFileManagerTest: XCTestCase

func testReleaseFilesFromQuarantine()
{
makeTempFiles() { fileManager, rootURL, ordinaryFileURL, directoryURL, fileInDirectoryURL in
makeTempFiles() { fileManager, rootURL, ordinaryFileURL, directoryURL, fileInDirectoryURL, validSymlinkURL, invalidSymlinkURL in
try! fileManager.releaseItemFromQuarantineAtRootURL(ordinaryFileURL)
try! fileManager.releaseItemFromQuarantineAtRootURL(directoryURL)
try! fileManager.releaseItemFromQuarantineAtRootURL(validSymlinkURL)

let quarantineData = "does not really matter what is here".cStringUsingEncoding(NSUTF8StringEncoding)!
let quarantineDataLength = Int(strlen(quarantineData))
Expand All @@ -135,6 +167,11 @@ class SUFileManagerTest: XCTestCase
XCTAssertEqual(0, setxattr(fileInDirectoryURL.path!, SUAppleQuarantineIdentifier, quarantineData, quarantineDataLength, 0, XATTR_CREATE))
XCTAssertGreaterThan(getxattr(fileInDirectoryURL.path!, SUAppleQuarantineIdentifier, nil, 0, 0, XATTR_NOFOLLOW), 0)

// Extended attributes can't be set on symbolic links in OS X, currently
try! fileManager.releaseItemFromQuarantineAtRootURL(validSymlinkURL)
XCTAssertGreaterThan(getxattr(directoryURL.path!, SUAppleQuarantineIdentifier, nil, 0, 0, XATTR_NOFOLLOW), 0)
XCTAssertEqual(-1, getxattr(validSymlinkURL.path!, SUAppleQuarantineIdentifier, nil, 0, 0, XATTR_NOFOLLOW))

try! fileManager.releaseItemFromQuarantineAtRootURL(directoryURL)

XCTAssertEqual(-1, getxattr(directoryURL.path!, SUAppleQuarantineIdentifier, nil, 0, 0, XATTR_NOFOLLOW))
Expand All @@ -153,7 +190,7 @@ class SUFileManagerTest: XCTestCase
// Instead we try to change the group ID - we just have to be a member of that group
func testAlterFilesGroupID()
{
makeTempFiles() { fileManager, rootURL, ordinaryFileURL, directoryURL, fileInDirectoryURL in
makeTempFiles() { fileManager, rootURL, ordinaryFileURL, directoryURL, fileInDirectoryURL, validSymlinkURL, invalidSymlinkURL in
XCTAssertNil(try? fileManager.changeOwnerAndGroupOfItemAtRootURL(ordinaryFileURL, toMatchURL: rootURL.URLByAppendingPathComponent("does not exist")))

XCTAssertNil(try? fileManager.changeOwnerAndGroupOfItemAtRootURL(rootURL.URLByAppendingPathComponent("does not exist"), toMatchURL: ordinaryFileURL))
Expand All @@ -169,12 +206,15 @@ class SUFileManagerTest: XCTestCase
XCTAssertEqual(staffGroupID, self.groupIDAtPath(ordinaryFileURL.path!))
XCTAssertEqual(staffGroupID, self.groupIDAtPath(directoryURL.path!))
XCTAssertEqual(staffGroupID, self.groupIDAtPath(fileInDirectoryURL.path!))

XCTAssertEqual(staffGroupID, self.groupIDAtPath(validSymlinkURL.path!))

try! fileManager.changeOwnerAndGroupOfItemAtRootURL(fileInDirectoryURL, toMatchURL: ordinaryFileURL)
try! fileManager.changeOwnerAndGroupOfItemAtRootURL(ordinaryFileURL, toMatchURL: ordinaryFileURL)
try! fileManager.changeOwnerAndGroupOfItemAtRootURL(validSymlinkURL, toMatchURL: ordinaryFileURL)

XCTAssertEqual(staffGroupID, self.groupIDAtPath(ordinaryFileURL.path!))
XCTAssertEqual(staffGroupID, self.groupIDAtPath(directoryURL.path!))
XCTAssertEqual(staffGroupID, self.groupIDAtPath(validSymlinkURL.path!))

XCTAssertEqual(0, chown(ordinaryFileURL.path!, getuid(), everyoneGroupID))
XCTAssertEqual(everyoneGroupID, self.groupIDAtPath(ordinaryFileURL.path!))
Expand All @@ -185,6 +225,9 @@ class SUFileManagerTest: XCTestCase
try! fileManager.changeOwnerAndGroupOfItemAtRootURL(fileInDirectoryURL, toMatchURL: directoryURL)
XCTAssertEqual(staffGroupID, self.groupIDAtPath(fileInDirectoryURL.path!))

try! fileManager.changeOwnerAndGroupOfItemAtRootURL(validSymlinkURL, toMatchURL: ordinaryFileURL)
XCTAssertEqual(everyoneGroupID, self.groupIDAtPath(validSymlinkURL.path!))

try! fileManager.changeOwnerAndGroupOfItemAtRootURL(directoryURL, toMatchURL: ordinaryFileURL)
XCTAssertEqual(everyoneGroupID, self.groupIDAtPath(directoryURL.path!))
XCTAssertEqual(everyoneGroupID, self.groupIDAtPath(fileInDirectoryURL.path!))
Expand All @@ -193,28 +236,33 @@ class SUFileManagerTest: XCTestCase

func testUpdateFileModificationTime()
{
makeTempFiles() { fileManager, rootURL, ordinaryFileURL, directoryURL, fileInDirectoryURL in
makeTempFiles() { fileManager, rootURL, ordinaryFileURL, directoryURL, fileInDirectoryURL, validSymlinkURL, invalidSymlinkURL in
XCTAssertNil(try? fileManager.updateModificationAndAccessTimeOfItemAtURL(rootURL.URLByAppendingPathComponent("does not exist")))

let oldOrdinaryFileAttributes = try! NSFileManager.defaultManager().attributesOfItemAtPath(ordinaryFileURL.path!)
let oldDirectoryAttributes = try! NSFileManager.defaultManager().attributesOfItemAtPath(directoryURL.path!)
let oldValidSymlinkAttributes = try! NSFileManager.defaultManager().attributesOfItemAtPath(validSymlinkURL.path!)

sleep(1); // wait for clock to advance

try! fileManager.updateModificationAndAccessTimeOfItemAtURL(ordinaryFileURL)
try! fileManager.updateModificationAndAccessTimeOfItemAtURL(directoryURL)
try! fileManager.updateModificationAndAccessTimeOfItemAtURL(validSymlinkURL)

let newOrdinaryFileAttributes = try! NSFileManager.defaultManager().attributesOfItemAtPath(ordinaryFileURL.path!)
XCTAssertGreaterThan((newOrdinaryFileAttributes[NSFileModificationDate] as! NSDate).timeIntervalSinceDate(oldOrdinaryFileAttributes[NSFileModificationDate] as! NSDate), 0)

let newDirectoryAttributes = try! NSFileManager.defaultManager().attributesOfItemAtPath(directoryURL.path!)
XCTAssertGreaterThan((newDirectoryAttributes[NSFileModificationDate] as! NSDate).timeIntervalSinceDate(oldDirectoryAttributes[NSFileModificationDate] as! NSDate), 0)

let newSymlinkAttributes = try! NSFileManager.defaultManager().attributesOfItemAtPath(validSymlinkURL.path!)
XCTAssertGreaterThan((newSymlinkAttributes[NSFileModificationDate] as! NSDate).timeIntervalSinceDate(oldValidSymlinkAttributes[NSFileModificationDate] as! NSDate), 0)
}
}

func testFileExists()
{
makeTempFiles() { fileManager, rootURL, ordinaryFileURL, directoryURL, fileInDirectoryURL in
makeTempFiles() { fileManager, rootURL, ordinaryFileURL, directoryURL, fileInDirectoryURL, validSymlinkURL, invalidSymlinkURL in
XCTAssertTrue(fileManager._itemExistsAtURL(ordinaryFileURL))
XCTAssertTrue(fileManager._itemExistsAtURL(directoryURL))
XCTAssertFalse(fileManager._itemExistsAtURL(rootURL.URLByAppendingPathComponent("does not exist")))
Expand All @@ -227,17 +275,11 @@ class SUFileManagerTest: XCTestCase

XCTAssertFalse(fileManager._itemExistsAtURL(rootURL.URLByAppendingPathComponent("does not exist"), isDirectory: nil))

let validSymlinkURL = rootURL.URLByAppendingPathComponent("symlink test")
try! NSFileManager.defaultManager().createSymbolicLinkAtURL(validSymlinkURL, withDestinationURL: directoryURL)

XCTAssertTrue(fileManager._itemExistsAtURL(validSymlinkURL))

var validSymlinkIsADirectory: ObjCBool = false
XCTAssertTrue(fileManager._itemExistsAtURL(validSymlinkURL, isDirectory: &validSymlinkIsADirectory) && !validSymlinkIsADirectory)

let invalidSymlinkURL = rootURL.URLByAppendingPathComponent("symlink test 2")
try! NSFileManager.defaultManager().createSymbolicLinkAtURL(invalidSymlinkURL, withDestinationURL: rootURL.URLByAppendingPathComponent("does not exist"))

// Symlink should still exist even if it doesn't point to a file that exists
XCTAssertTrue(fileManager._itemExistsAtURL(invalidSymlinkURL))

Expand All @@ -248,7 +290,7 @@ class SUFileManagerTest: XCTestCase

func testMakeDirectory()
{
makeTempFiles() { fileManager, rootURL, ordinaryFileURL, directoryURL, fileInDirectoryURL in
makeTempFiles() { fileManager, rootURL, ordinaryFileURL, directoryURL, fileInDirectoryURL, validSymlinkURL, invalidSymlinkURL in
XCTAssertNil(try? fileManager._makeDirectoryAtURL(ordinaryFileURL))
XCTAssertNil(try? fileManager._makeDirectoryAtURL(directoryURL))

Expand All @@ -260,6 +302,9 @@ class SUFileManagerTest: XCTestCase

var isDirectory: ObjCBool = false
XCTAssertTrue(fileManager._itemExistsAtURL(newDirectoryURL, isDirectory: &isDirectory))

try! fileManager.removeItemAtURL(directoryURL)
XCTAssertNil(try? fileManager._makeDirectoryAtURL(validSymlinkURL))
}
}

Expand Down

0 comments on commit 7fdedad

Please sign in to comment.