Renames #149
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -268,21 +268,36 @@ - (RACSignal *)installRequest:(SQRLShipItRequest *)request { | |
| return [[[[self | ||
| prepareAndValidateUpdateBundleURLForRequest:request] | ||
| flattenMap:^(NSURL *updateBundleURL) { | ||
| return [[[[self | ||
| acquireTargetBundleURLForRequest:request] | ||
| then:^{ | ||
| if (!request.allowRename) return [RACSignal return:request.targetBundleURL]; | ||
|
|
||
| return [self renamedTargetIfNeededWithTargetURL:request.targetBundleURL sourceURL:updateBundleURL]; | ||
| }] | ||
| flattenMap:^(NSURL *newTargetURL) { | ||
| SQRLShipItRequest *updatedRequest = [[SQRLShipItRequest alloc] initWithUpdateBundleURL:request.updateBundleURL targetBundleURL:newTargetURL bundleIdentifier:request.bundleIdentifier launchAfterInstallation:request.launchAfterInstallation allowRename:request.allowRename]; | ||
| return [[[[[[[self | ||
| installItemToURL:request.targetBundleURL fromURL:updateBundleURL] | ||
| then:^{ | ||
| if ([request.targetBundleURL isEqual:newTargetURL]) return [RACSignal empty]; | ||
|
|
||
| return [self installItemToURL:newTargetURL fromURL:request.targetBundleURL]; | ||
| }] | ||
| concat:[RACSignal return:request.updateBundleURL]] | ||
| concat:[RACSignal return:updateBundleURL]] | ||
| concat:[RACSignal defer:^{ | ||
| return [RACSignal return:self.ownedBundle.temporaryURL]; | ||
| }]] | ||
| flattenMap:^(NSURL *location) { | ||
| return [[[self | ||
| deleteOwnedBundleAtURL:location] | ||
| doError:^(NSError *error) { | ||
| NSLog(@"Couldn't remove owned bundle at location %@, error %@", location, error.sqrl_verboseDescription); | ||
| }] | ||
| catchTo:[RACSignal empty]]; | ||
| }] | ||
| concat:[RACSignal return:updatedRequest]]; | ||
| }] | ||
| doCompleted:^{ | ||
| self.ownedBundle = nil; | ||
|
|
@@ -413,6 +428,36 @@ - (RACSignal *)verifyBundleAtURL:(NSURL *)bundleURL usingSignature:(SQRLCodeSign | |
|
|
||
| #pragma mark Installation | ||
|
|
||
| /// Check if the target should be renamed and provide the renamed URL. | ||
| /// | ||
| /// targetURL - The URL for the target. Cannot be nil. | ||
| /// sourceURL - The URL for the source. Cannot be nil. | ||
| /// | ||
| /// Returns a signal which will send the URL for the renamed target. If a rename | ||
| /// isn't needed then it will send `targetURL`. | ||
| - (RACSignal *)renamedTargetIfNeededWithTargetURL:(NSURL *)targetURL sourceURL:(NSURL *)sourceURL { | ||
| return [RACSignal defer:^{ | ||
| NSBundle *sourceBundle = [NSBundle bundleWithURL:sourceURL]; | ||
| NSString *targetExecutableName = targetURL.lastPathComponent.stringByDeletingPathExtension; | ||
| NSString *sourceExecutableName = sourceBundle.sqrl_executableName; | ||
|
|
||
| // If they're already the same then we're good. | ||
| if ([targetExecutableName isEqual:sourceExecutableName]) { | ||
| return [RACSignal return:targetURL]; | ||
| } | ||
|
|
||
| NSString *newAppName = [sourceExecutableName stringByAppendingPathExtension:@"app"]; | ||
|
There was a problem hiding this comment. Can we use There was a problem hiding this comment. I think the bundle's executable name is more canonical than what the zipped thing is. There was a problem hiding this comment. @mdiep could you tie break on this one There was a problem hiding this comment. I don't know why these would ever be different at this point. It seems a little weird to me that this is even a key in the |
||
| NSURL *newTargetURL = [targetURL.URLByDeletingLastPathComponent URLByAppendingPathComponent:newAppName]; | ||
|
|
||
| // If there's already something there then don't rename to it. | ||
| if ([NSFileManager.defaultManager fileExistsAtPath:newTargetURL.path]) { | ||
|
There was a problem hiding this comment. This is a minor ToC-ToU race, to prevent it I’d like to flip the order of installation and rename. In my head, I’ve been modelling the file system as a datastore whose entries are protected by code sign requirements; entries that don’t exist have no protecting requirement (only BSD and ACL permissions). If we can safely (i.e. don't rename from the current entry to the new one if the new one already exists) the existing bundle to the requested name, the install is then from the update bundle (whose name is irrelevant at that point) to the file system entry of the target bundle, as before. There was a problem hiding this comment. Yeah so I started down that route originally. The problem we run into is that the So I think we could end up in a situation where:
Now, we have the same problem if we rename after installation, except that we're less likely to run into a problem at that point (though still possible). But for that matter, I'm not sure how swapping the order solves the potential ToC-ToU race. We still have to check for existence before renaming, regardless of when the rename happens. There was a problem hiding this comment.
That’s a great point, I hadn’t considered that 😢
It would be handled by instead of doing a My concern is renaming could be exploited to replace a different app, which the update is named for but doesn’t meet the codesign requirements of. |
||
| return [RACSignal return:targetURL]; | ||
| } | ||
|
|
||
| return [RACSignal return:newTargetURL]; | ||
| }]; | ||
| } | ||
|
|
||
| - (RACSignal *)installItemToURL:(NSURL *)targetURL fromURL:(NSURL *)sourceURL { | ||
| NSParameterAssert(targetURL != nil); | ||
| NSParameterAssert(sourceURL != nil); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly should this compare last components rather than executable names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. The plist is more canonical than whatever the name of the zipped thing is, I think.