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

doc: add example for cipher.update() #28373

Open
wants to merge 1 commit into
base: master
from

Conversation

@sergiovillagran
Copy link

sergiovillagran commented Jun 21, 2019

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Copy link
Contributor

mscdex left a comment

I don't think it makes sense to duplicate examples like this.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Jun 23, 2019

@mscdex ... I disagree entirely. Our docs are excruciatingly light on examples. There is absolutely no harm in adding this example and this came in as a code and learn exercise for a new contributor. Please reconsider your -1 on it.

@jasnell jasnell requested review from addaleax and BridgeAR Jun 23, 2019
@mscdex

This comment has been minimized.

Copy link
Contributor

mscdex commented Jun 23, 2019

@jasnell Can't there be a way to link to an existing example instead then?

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jun 24, 2019

this came in as a code and learn exercise for a new contributor.

Not disagreeing with any of the facts stated but for the record: Documentation changes are a poor choice for code + learn tasks and we should avoid them in the future. They are easy to do poorly, often difficult to do in a way that materially improves the docs, attract way more comments and (often contradictory) suggestions than simple code changes, and require skills that many code + learn attendees may not have.

That's an aside, though. Now back to reviewing this specific change...

@Trott Trott force-pushed the nodejs:master branch from 1ecc406 to 49cf67e Sep 17, 2019
@gireeshpunathil

This comment has been minimized.

Copy link
Member

gireeshpunathil commented Nov 26, 2019

where do we stand on this PR?

@devnexen devnexen force-pushed the nodejs:master branch from e8a4568 to 5289f80 Dec 26, 2019
@BridgeAR BridgeAR added the tsc-agenda label Jan 12, 2020
@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Jan 12, 2020

@nodejs/tsc PTAL. There is a controversial opinion about landing this PR and it would be good to resolve that (what ever outcome that might be).

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Jan 15, 2020

@sergiovillagran - your interest in collaborating on Node.js development is really appreciated, this isn't at all about the quality of your work! Its unfortunate that you were asked to make this change, only to find out later that there is disagreement about whether its an improvement.

With respect to the change itself, I agree with @mscdex that the docs become harder to use when we copy the cipher examples from the cipher class section header into each individual cipher method. IMHO, this decreases the quality of our docs.

Copy link
Member

tniessen left a comment

Thank you for your contribution, and sorry for chiming in late. So this is mostly a copy of a previous example, the only addition seems to be this:

// If data is a Buffer, then inputEncoding is ignored.
let buff = Buffer.from('some clear text data', 'utf8')
let encryptedBuffer = cipher.update(buff, null, 'hex')
console.log(encryptedBuffer) // Prints: 2bb302c9618b22e9f843afc1befd7997

// If no outputEncoding is provided, a Buffer is returned.
let buffer = cipher.update('some clear text data', 'utf8')
console.log(buffer) // Prints <Buffer 42 6a 2e 23 bd 8d 51 c7 16 cf 47 f6 9f 16 b6 b6>

First, the snippet should probably use const instead of let for variables that are never reassigned.

Second, encrypting buff, obtaining a string, and calling the result encryptedBuffer seems a little counterintuitive, especially since there is an unrelated variable buffer, which (unlike encryptedBuffer), is actually an encrypted buffer.

It might also be confusing to beginners that the example is calling update twice with the same plaintext to be encrypted, but obtaining different ciphertexts (due to the statefulness of the Cipher object), and then stores these different results in separate and unrelated variables.

Since 'some clear text data' overlaps with the second AES block (encoded as 20 bytes, AES block size is 16 bytes), the result of the second call to update also depends on the first plaintext (independent of the block cipher mode, which could as well be CTR). As another side effect, this code snippet doesn't even encrypt both messages fully, but only the first two blocks (32 of 40 bytes).

The code snippet appears as if it was meant to be a complete example, but it's missing the important call to final. Without that, the chosen cipher mode aes-192-cbc (block cipher in CBC mode) only outputs partial ciphertexts. (And even for other modes that do not strictly require a call to final, e.g., CTR mode, it is advisable to call final.)

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Feb 10, 2020

It doesn’t seem like there’s anything new for the TSC to discuss at this point, and it doesn’t look like this PR is currently moving forward either, so I’ll remove the tsc-agenda label.

Anybody can feel free to re-add it, though. In particular, the @nodejs/tsc has not really discussed the underlying question here of whether we’re generally okay with mostly duplicated examples.

@addaleax addaleax removed the tsc-agenda label Feb 10, 2020
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

10 participants
You can’t perform that action at this time.