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

Safe signature #445

Merged
merged 3 commits into from
Mar 3, 2015
Merged

Safe signature #445

merged 3 commits into from
Mar 3, 2015

Conversation

joshaber
Copy link
Member

@joshaber joshaber commented Mar 3, 2015

Create a signature using default values if libgit2’s validation fails.

@jspahrsummers jspahrsummers self-assigned this Mar 3, 2015
NSString *domain = NSProcessInfo.processInfo.hostName ?: @"nowhere.local";
email = [NSString stringWithFormat:@"%@@%@", username, domain];
}
if (email == nil) email = self.class.defaultEmail;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why shouldn't this be a length check too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Libgit2 used to be fine with empty emails: libgit2/libgit2@76e3c43

Apparently not anymore 😢

@jspahrsummers
Copy link
Contributor

Can we unart test this too?

@joshaber
Copy link
Member Author

joshaber commented Mar 3, 2015

I went back and forth on whether we should test for < and > in the email explicitly. That'd let us be more useful, but it'd be duplicating libgit2's test. Thoughts?

@jspahrsummers
Copy link
Contributor

That'd let us be more useful

What do you mean? I was thinking we could just test that invalid strings result in the default.

@joshaber
Copy link
Member Author

joshaber commented Mar 3, 2015

Ah, sorry, I meant that if we detect that the email address has < or > in it (which means libgit2 will reject it), we could still try using the configured user name. As is, libgit2 would reject the whole signature and we'd fallback to the default username and email.

@jspahrsummers
Copy link
Contributor

If there's a libgit2 API to check validity without actually instantiating a signature, we could test that with the given username and email separately, then both together.

If that sort of thing doesn't exist, I think this implementation is fine.

@joshaber
Copy link
Member Author

joshaber commented Mar 3, 2015

If there's a libgit2 API to check validity without actually instantiating a signature, we could test that with the given username and email separately, then both together.

Sadly no.

🍂

@jspahrsummers
Copy link
Contributor

🍃

jspahrsummers added a commit that referenced this pull request Mar 3, 2015
@jspahrsummers jspahrsummers merged commit 3f617c5 into master Mar 3, 2015
@jspahrsummers jspahrsummers deleted the safer-signature branch March 3, 2015 19:09
@jspahrsummers jspahrsummers removed their assignment May 22, 2015
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.

2 participants