GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: add known issues test for #31733 by bnoordhuis · Pull Request #31734 · nodejs/node

Conversation

@bnoordhuis

@bnoordhuis bnoordhuis commented Feb 11, 2020

Copy link
Copy Markdown
Member

Authenticated decryption works for file streams up to 32768 bytes but
not beyond. Other streams and direct decryption are not affected.

Refs: #31733

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. label Feb 11, 2020
@bnoordhuis

bnoordhuis commented Feb 11, 2020

Copy link
Copy Markdown
Member Author

cc @nodejs/crypto
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnoordhuis bnoordhuis mentioned this pull request Feb 11, 2020
fs,crypto: AAD decryption of fs stream > 32768 bytes fails #31733
Closed
cjihrig
cjihrig approved these changes Feb 11, 2020
View reviewed changes
@jasnell

jasnell commented Feb 11, 2020

Copy link
Copy Markdown
Member

/cc @mcollina @ronag
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell added the known limitation Issues that are identified as known limitations. label Feb 11, 2020
jasnell
jasnell approved these changes Feb 11, 2020
View reviewed changes
mcollina
mcollina approved these changes Feb 11, 2020
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

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.

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jasnell

jasnell commented Feb 11, 2020

Copy link
Copy Markdown
Member

@mcollina ... it's possible that this issue is caused by a timing issue in streams.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax added author ready PRs with CI started, the required approvals, and no outstanding review comments. crypto Issues and PRs related to the crypto subsystem. fs Issues and PRs related to file-system APIs and the fs module. labels Feb 11, 2020
@nodejs-github-bot

nodejs-github-bot commented Feb 11, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29075/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Feb 11, 2020

Copy link
Copy Markdown
Member

I don't know what can cause this. What's not clear is if this is a regression or not.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag

ronag commented Feb 11, 2020

Copy link
Copy Markdown
Member

The error comes from cpp land https://github.com/nodejs/node/blob/master/src/node_crypto.cc#L4574.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax reviewed Feb 13, 2020
View reviewed changes
Comment thread test/known_issues/test-crypto-authenticated-stream.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Feb 13, 2020
bnoordhuis added 3 commits February 18, 2020 12:26
@bnoordhuis
test: add known issues test for nodejs#31733
c3b1dfc 
Authenticated decryption works for file streams up to 32768 bytes but
not beyond. Other streams and direct decryption are not affected.

Refs: nodejs#31733
@bnoordhuis
fixup! add extra sanity check
5d80d47
@bnoordhuis
fixup! remove hasCrypto check
416e214
@bnoordhuis
bnoordhuis force-pushed the bug31733 branch from cb6334a to 416e214 Compare February 18, 2020 11:27
@bnoordhuis bnoordhuis added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Feb 18, 2020
Trott
Trott reviewed Feb 21, 2020
View reviewed changes
Comment thread test/known_issues/test-crypto-authenticated-stream.js
@@ -0,0 +1,142 @@
/* eslint-disable node-core/crypto-check */

@Trott Trott Feb 21, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Might not be a bad idea to create a test/known_issues/.eslintrc.yaml and disable the lint rule there, but also probably outside the scope of this PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot

nodejs-github-bot commented Feb 21, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29259/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Mar 9, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29647/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Mar 11, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29736/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Mar 12, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29778/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Mar 13, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29810/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Mar 27, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30121/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Mar 30, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30259/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

addaleax commented Apr 2, 2020

Copy link
Copy Markdown
Member

Landed in 6f94221
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax closed this Apr 2, 2020
addaleax pushed a commit that referenced this pull request Apr 2, 2020
@bnoordhuis @addaleax
test: add known issues test for #31733
6f94221 
Authenticated decryption works for file streams up to 32768 bytes but
not beyond. Other streams and direct decryption are not affected.

Refs: #31733

PR-URL: #31734
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@mmarchini

mmarchini commented Apr 4, 2020

Copy link
Copy Markdown
Contributor

I'm seeing this failing on #32521, not sure if it's flaky or that PR broke something.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mmarchini mmarchini mentioned this pull request Apr 4, 2020
Flakly known_issues/test-crypto-authenticated-stream on arm #32650
Closed
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
@bnoordhuis @BethGriggs
test: add known issues test for #31733
71f3a8b 
Authenticated decryption works for file streams up to 32768 bytes but
not beyond. Other streams and direct decryption are not affected.

Refs: #31733

PR-URL: #31734
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this pull request Apr 12, 2020
@bnoordhuis @targos
test: add known issues test for #31733
0e4ce8f 
Authenticated decryption works for file streams up to 32768 bytes but
not beyond. Other streams and direct decryption are not affected.

Refs: #31733

PR-URL: #31734
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@targos targos mentioned this pull request Apr 13, 2020
v13.13.0 release proposal #32813
Merged
targos pushed a commit that referenced this pull request Apr 22, 2020
@bnoordhuis @targos
test: add known issues test for #31733
d53d152 
Authenticated decryption works for file streams up to 32768 bytes but
not beyond. Other streams and direct decryption are not affected.

Refs: #31733

PR-URL: #31734
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@targos targos mentioned this pull request Apr 22, 2020
v12.16.3 release proposal #33009
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott left review comments
@addaleax addaleax addaleax left review comments
@mcollina mcollina mcollina approved these changes
@jasnell jasnell jasnell approved these changes
@cjihrig cjihrig cjihrig approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. crypto Issues and PRs related to the crypto subsystem. fs Issues and PRs related to file-system APIs and the fs module. known limitation Issues that are identified as known limitations. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@bnoordhuis @jasnell @nodejs-github-bot @mcollina @ronag @addaleax @mmarchini @Trott @cjihrig
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.