Skip to content

Conversation

@fanatid
Copy link
Contributor

@fanatidfanatid commented Nov 1, 2016

Checklist
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

Description of change

Throw error after calling CipherBase#final as in CipherBase::SetAAD, CipherBase::SetAuthTag, CipherBase::GetAuthTag.

@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 1, 2016
@addaleaxaddaleax added crypto Issues and PRs related to the crypto subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Nov 1, 2016
@addaleax
Copy link
Member

Tagging this as semver-major because it turns a silent failure (?) into a thrown error.

@sam-github
Copy link
Contributor

Needs docs and tests

@fanatid
Copy link
ContributorAuthor

updated

Copy link
Contributor

Choose a reason for hiding this comment

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

If you assert on the error message instead of just the Error constructor, it gives us better detection of things like error message changes (which are semver major). It also verifies that we got the error we planned for.

@fanatidfanatidforce-pushed the crypto/SetAutoPadding branch from abc29e8 to 4c04d3eCompareNovember 3, 2016 07:02
@fanatidfanatidforce-pushed the crypto/SetAutoPadding branch from 4c04d3e to e06b6c2CompareFebruary 7, 2017 10:09
Throw error after calling CipherBase#final
@fanatidfanatidforce-pushed the crypto/SetAutoPadding branch from e06b6c2 to 2f83f0eCompareFebruary 7, 2017 10:13
@fanatid
Copy link
ContributorAuthor

Rebased and fix lint errors.

@addaleax
Copy link
Member

@addaleax
Copy link
Member

Landed in e90f382, thanks for the PR!

@addaleaxaddaleax closed this Feb 7, 2017
addaleax pushed a commit that referenced this pull request Feb 7, 2017
Throw error after calling CipherBase#final PR-URL: #9405 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
Throw error after calling CipherBase#final PR-URL: nodejs#9405 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@jasnelljasnell mentioned this pull request Apr 4, 2017
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Oct 29, 2017
The previous commit is a back-port of pull request nodejs#13821 to v6.x. Its regression test does not apply to the v6.x branch (depends on semver-major pull request nodejs#9405) so this commit adds a new test. Refs: nodejs#13821 Refs: nodejs#9405
@fanatidfanatid deleted the crypto/SetAutoPadding branch September 7, 2019 14:11
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.cryptoIssues and PRs related to the crypto subsystem.semver-majorPRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@fanatid@addaleax@sam-github@jasnell@cjihrig@nodejs-github-bot