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

Code Review Before Version 1.4.0 is Tagged #159

Merged
merged 12 commits into from Nov 3, 2016
Merged

Conversation

@paragonie-scott
Copy link
Member

@paragonie-scott paragonie-scott commented Nov 2, 2016

As with all major/minor releases, we're auditing the code we own for any potential security flaws.

@paragonie-scott
Copy link
Member Author

@paragonie-scott paragonie-scott commented Nov 3, 2016

Other than these nits, the diffs look good. Now to see about the security of the whole system.

if (\PHP_VERSION_ID >= 70100) {
// Forward compatibility.
unset($session_config['entropy_length']);
}

This comment has been minimized.

@paragonie-scott

paragonie-scott Nov 3, 2016
Author Member

This configuration directive is removed in PHP 7.1.0, so we might as well not pass it.

This comment has been minimized.

@kelunik

kelunik Nov 3, 2016
Contributor

Better have that as a code comment than GitHub comment.

@paragonie-scott
Copy link
Member Author

@paragonie-scott paragonie-scott commented Nov 3, 2016

Status report: No vulnerabilities found, but a lot of boyscouting (whitespace, docblocks) performed to make future auditing easier.

@paragonie-scott paragonie-scott merged commit 0e92895 into master Nov 3, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@paragonie-scott paragonie-scott deleted the pre-1.4.0-audit branch Nov 3, 2016
@@ -288,7 +291,7 @@ public function verifySessionCanary(int $userID, bool $logOut = true): bool
);
if (empty($canary)) {
$this->log(
'What is this even.',
'No session canary was registered with this user in the database.',

This comment has been minimized.

@kelunik

kelunik Nov 3, 2016
Contributor

😄

@@ -137,6 +137,9 @@ public function getSupplier(string $name, bool $flush = false): Supplier
/**
* Get all URLs
*
* By default, this shuffles them randomly.
* If you're in tor-only mode, it prioritizes .onion domains first.
*
* @param bool $doNotShuffle

This comment has been minimized.

@kelunik

kelunik Nov 3, 2016
Contributor

This should probably be changed to $shuffle = true in 2.0.

@@ -80,11 +80,17 @@ public function autoUpdate()
\AIRSHIP_VERSION,
'airship_version'
);

/**

This comment has been minimized.

@kelunik

kelunik Nov 3, 2016
Contributor

This should be a normal multiline comment, so /* instead of /**.

* If we get to this point:
*
* 1. We know the signature is signed by
* Paragon Initiative Enterprises, LLC.

This comment has been minimized.

@kelunik

kelunik Nov 3, 2016
Contributor

Signed by the supplier, which might not be Paragon, it's just Paragon by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants