feat: Update protobufjs (and peer dependencies) to ^7 #874

threema-lenny · 2023-07-11T14:10:17Z

I scanned the dependencies we have to protobufjs files (mostly reader/writer) and there seems to have been no changes requiring any further action.

I've also scanned for peer dependencies to protobufjs 6 and updated those accordingly (best effort).

Note: We have a cyclic dependency between ts-proto and ts-proto-descriptors whose protobufjs dependencies affect one another, so now we have two different protobufjs versions in the node_modules folder. Not sure how you want to handle this, @stephenh.

Resolves #834

threema-lenny · 2023-07-11T14:10:45Z

There is one failing test where I'd appreciate getting some help for because I'm unfamiliar with grpc and the intended behaviour. For what it's worth, downgrading @grpc/proto-loader to the previous version makes no difference.

stephenh · 2023-07-11T15:24:06Z

This is great @threema-lenny !

This failing test:

FAIL integration/nestjs-simple/nestjs-simple-test.ts
  ● nestjs-simple-test nestjs › should findOneHero

    expect(received).toEqual(expected) // deep equality

    - Expected  - 14
    + Received  +  1

      Object {
        "birthDate": Object {
          "nanos": 2,
          "seconds": 1,
        },
    -   "externalData": Object {
    -     "fizz": 1,
    -     "foo": "bar",
    -     "nested": Object {
    -       "arr": Array [
    -         1,
    -         "foo",
    -         Array [
    -           "bar",
    -         ],
    -       ],
    -       "isFailing": false,
    -     },
    -   },
    +   "externalData": Object {},

Is probably due to our google.protobuf.Struct well known type handling--iirc we try to turn pb Struct into "just objects" / POJOs, but our conversion process for that is bespoke to ts-proto, and not the just "stock protobuf bytes to protobuf message" that protobufjs expects.

So far we've been able to intercept protobufjs with their wrappers API and tell it about our special struct handling with this line in hero.ts:

wrappers[".google.protobuf.Struct"] = { fromObject: Struct.wrap, toObject: Struct.unwrap } as any;

And externalData is typed as a .google.protobuf.Struct in the hero.proto file.

So, worst-case scenario is that they removed this wrappers API entirely in protobufjs 7, or best-case scenario is that the API and/or maybe even just the import/export changed (maybe due to a bundling/packaging change from 6 to 7?), but really not sure...

Maybe that's enough guidance for you to run with it from there?

Also fwiw/fyi that @boukeversteegh is our resident Struct/well-known-types effort, just as an FYI to him that our wrappers approach has some currently-unknown, maybe-easy/maybe-not issue in pbjs 7.

Thanks!

threema-lenny · 2023-07-12T12:05:28Z

I think I'll need to pass on that task because I simply don't know anything about gRPC, NestJS and the Struct feature in protobufjs. I looked at it for a few hours but it would be a total poke in the dark. Sorry.

stephenh · 2023-07-12T13:29:33Z

@threema-lenny Np! I poked around a little bit, thinking that maybe protobufjs broke the wrappers contract/feature during the upgrade, but looks like it was just caused by having two versions of protobufjs in node_modules, specifically the @grpc/proto-loader (which NestJS uses) had kept an older/shadowed version of protobuf, and so was using its global wrapper const instead of the wrapper const that hero.ts configured.

I pushed up the yarn.lock change and 🤞 we'll see if that's it.

stephenh

Awesome! Thanks @threema-lenny !

# [1.153.0](v1.152.1...v1.153.0) (2023-07-12) ### Features * Update protobufjs (and peer dependencies) to ^7 ([#874](#874)) ([7f979a7](7f979a7))

stephenh · 2023-07-12T14:04:02Z

🎉 This PR is included in version 1.153.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

threema-lenny · 2023-07-12T14:10:20Z

Oh dear, yeah, those side effects are evil. Thanks a lot, @stephenh!

threema-danilo · 2023-07-12T14:13:24Z

Thanks @stephenh and @threema-lenny!

dankeboy36 · 2023-07-12T17:50:21Z

Hello,

thank you for the work!

After installing ts-proto@1.153.0, I still have the dependency to protobufjs@6.8.8 via ts-proto-descriptors. The package.json pulled from npm is different than in the repo.

ts-proto/protos/package.json

Lines 2 to 3 in ce064d0

    
           "name": "ts-proto-descriptors", 
        
           "version": "1.9.0",

ts-proto/protos/package.json

Line 15 in ce064d0

"protobufjs": "^7.0.0"

Script to reproduce:

% rm -rf ts-proto-test \
&& mkdir ts-proto-test \
&& cd ts-proto-test \
&& npm init -y \
&& npm i -S ts-proto@1.153.0 \
&& cat ./node_modules/ts-proto-descriptors/package.json | jq .dependencies
Wrote to /Users/akos.kitta/Desktop/dev/__trash/ts-proto-test/package.json:

{
  "name": "ts-proto-test",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "keywords": [],
  "author": "",
  "license": "ISC"
}



added 25 packages, and audited 26 packages in 2s

1 package is looking for funding
  run `npm fund` for details

1 high severity vulnerability

To address all issues, run:
  npm audit fix

Run `npm audit` for details.
{
  "long": "^4.0.0",
  "protobufjs": "^6.8.8"
}

npm audit still shows the protobufjs vulnerability:

% npm audit
# npm audit report

protobufjs  6.10.0 - 7.2.3
Severity: high
protobufjs Prototype Pollution vulnerability - https://github.com/advisories/GHSA-h755-8qp9-cq85
fix available via `npm audit fix`
node_modules/ts-proto-descriptors/node_modules/protobufjs

1 high severity vulnerability

To address all issues, run:
  npm audit fix

What am I doing wrong? Thank you!

threema-lenny · 2023-07-12T20:10:17Z

You're probably doing nothing wrong, I suspect it's the cyclic dependency I hinted at. I'm assuming we need to bump ts-proto-descriptors to depend on ts-proto@1.153.0, then publish it, then bump ts-proto to depend on ts-proto-descriptors@<next> and then publish that again. Quite the chore. 😅

stephenh · 2023-07-12T20:45:01Z

Yep! I forgot about that; I think it should be fixed with this PR that I just merged:

#876

dankeboy36 · 2023-07-13T17:00:17Z

After installing ts-proto@1.153.2, ts-proto-descriptors@1.11.0 is correctly pulled in. Thank you for the great work.

However, one of my tests started to fail with Error: Cannot find module 'long'. The long dependency has been removed, and I am wondering if it's expected to install long from now on. Or maybe my setup has been incorrect since day one, and long must be a dependency. I am happy to open a new issue if required, but I thought it belonged here and wanted to ask.

Click to see error

     Command failed with exit code 1: /Users/akos.kitta/Desktop/dev/ardunno-cli-gen/node_modules/protoc/protoc/bin/protoc --plugin=/Users/akos.kitta/Desktop/dev/ardunno-cli-gen/node_modules/ts-proto/protoc-gen-ts_proto --proto_path=/Users/akos.kitta/Desktop/dev/ardunno-cli-gen/node_modules/protoc/protoc/include --ts_proto_opt=outputServices=nice-grpc,outputServices=generic-definitions,oneof=unions,useExactTypes=false,paths=source_relative,esModuleInterop=true,exportCommonSymbols=false,useOptionals=none --ts_proto_out=/var/folders/g0/n_2cw4ds44l2byzv39xrc4340000gp/T/tmp-62256-Am7mYyJYZ83Y/src-gen google/protobuf/any.proto google/protobuf/api.proto google/protobuf/descriptor.proto google/protobuf/duration.proto google/protobuf/empty.proto google/protobuf/field_mask.proto google/protobuf/source_context.proto google/protobuf/struct.proto google/protobuf/timestamp.proto google/protobuf/type.proto google/protobuf/wrappers.proto google/protobuf/compiler/plugin.proto
node:internal/modules/cjs/loader:1080
  throw err;
  ^

Error: Cannot find module 'long'
Require stack:
- /Users/akos.kitta/Desktop/dev/ardunno-cli-gen/node_modules/ts-proto-descriptors/dist/google/protobuf/descriptor.js
- /Users/akos.kitta/Desktop/dev/ardunno-cli-gen/node_modules/ts-proto-descriptors/dist/index.js
- /Users/akos.kitta/Desktop/dev/ardunno-cli-gen/node_modules/ts-proto/build/plugin.js
- /Users/akos.kitta/Desktop/dev/ardunno-cli-gen/node_modules/ts-proto/protoc-gen-ts_proto
    at Module._resolveFilename (node:internal/modules/cjs/loader:1077:15)
    at Module._load (node:internal/modules/cjs/loader:922:27)
    at Module.require (node:internal/modules/cjs/loader:1143:19)
    at require (node:internal/modules/cjs/helpers:110:18)
    at Object.<anonymous> (/Users/akos.kitta/Desktop/dev/ardunno-cli-gen/node_modules/ts-proto-descriptors/dist/google/protobuf/descriptor.js:5:14)
    at Module._compile (node:internal/modules/cjs/loader:1256:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1310:10)
    at Module.load (node:internal/modules/cjs/loader:1119:32)
    at Module._load (node:internal/modules/cjs/loader:960:12)
    at Module.require (node:internal/modules/cjs/loader:1143:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/Users/akos.kitta/Desktop/dev/ardunno-cli-gen/node_modules/ts-proto-descriptors/dist/google/protobuf/descriptor.js',
    '/Users/akos.kitta/Desktop/dev/ardunno-cli-gen/node_modules/ts-proto-descriptors/dist/index.js',
    '/Users/akos.kitta/Desktop/dev/ardunno-cli-gen/node_modules/ts-proto/build/plugin.js',
    '/Users/akos.kitta/Desktop/dev/ardunno-cli-gen/node_modules/ts-proto/protoc-gen-ts_proto'
  ]
}

Node.js v18.16.1
--ts_proto_out: protoc-gen-ts_proto: Plugin failed with status code 1.

stephenh · 2023-07-13T21:20:02Z

Thanks for the bug report @dankeboy36 ; I just pushed out a fix, can you see if that works? Thanks!

dankeboy36 · 2023-07-14T16:22:15Z

It's working great now with ts-proto@1.153.3. Thank you!

Update protobufjs (and peer dependencies) to ^7

2d507e4

threema-lenny changed the title ~~Update protobufjs (and peer dependencies) to ^7~~ chore: Update protobufjs (and peer dependencies) to ^7 Jul 11, 2023

haines mentioned this pull request Jul 11, 2023

protobufjs Prototype Pollution vulnerability (CVE-2023-36665) cerbos/cerbos-sdk-javascript#591

Closed

fixup! Update protobufjs (and peer dependencies) to ^7

741964b

Run yarn dedupe.

947ae01

stephenh changed the title ~~chore: Update protobufjs (and peer dependencies) to ^7~~ feat: Update protobufjs (and peer dependencies) to ^7 Jul 12, 2023

stephenh approved these changes Jul 12, 2023

View reviewed changes

stephenh merged commit 7f979a7 into stephenh:main Jul 12, 2023
7 checks passed

stephenh pushed a commit that referenced this pull request Jul 12, 2023

chore(release): 1.153.0 [skip ci]

ce064d0

# [1.153.0](v1.152.1...v1.153.0) (2023-07-12) ### Features * Update protobufjs (and peer dependencies) to ^7 ([#874](#874)) ([7f979a7](7f979a7))

stephenh added the released label Jul 12, 2023

threema-lenny deleted the protobufjs7 branch July 12, 2023 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Update protobufjs (and peer dependencies) to ^7 #874

feat: Update protobufjs (and peer dependencies) to ^7 #874

threema-lenny commented Jul 11, 2023 •

edited

threema-lenny commented Jul 11, 2023

stephenh commented Jul 11, 2023 •

edited

threema-lenny commented Jul 12, 2023 •

edited

stephenh commented Jul 12, 2023

stephenh left a comment

stephenh commented Jul 12, 2023

threema-lenny commented Jul 12, 2023

threema-danilo commented Jul 12, 2023

dankeboy36 commented Jul 12, 2023

threema-lenny commented Jul 12, 2023 •

edited

stephenh commented Jul 12, 2023

dankeboy36 commented Jul 13, 2023

stephenh commented Jul 13, 2023

dankeboy36 commented Jul 14, 2023

feat: Update protobufjs (and peer dependencies) to ^7 #874

feat: Update protobufjs (and peer dependencies) to ^7 #874

Conversation

threema-lenny commented Jul 11, 2023 • edited

threema-lenny commented Jul 11, 2023

stephenh commented Jul 11, 2023 • edited

threema-lenny commented Jul 12, 2023 • edited

stephenh commented Jul 12, 2023

stephenh left a comment

Choose a reason for hiding this comment

stephenh commented Jul 12, 2023

threema-lenny commented Jul 12, 2023

threema-danilo commented Jul 12, 2023

dankeboy36 commented Jul 12, 2023

threema-lenny commented Jul 12, 2023 • edited

stephenh commented Jul 12, 2023

dankeboy36 commented Jul 13, 2023

stephenh commented Jul 13, 2023

dankeboy36 commented Jul 14, 2023

threema-lenny commented Jul 11, 2023 •

edited

stephenh commented Jul 11, 2023 •

edited

threema-lenny commented Jul 12, 2023 •

edited

threema-lenny commented Jul 12, 2023 •

edited