nodejs-common util.decorateRequest mechanism may edit user provided strings

Copied from GoogleCloudPlatform/google-cloud-node#1891
@ofrobots
December 19, 2016 20:15

Environment details

OS: all

Node.js version: all

npm version: all

google-cloud-node version: master

Steps to reproduce

The request mechanism provided used by Service and ServiceObject go through the request body and modify all occurrences of the string {{projectId}} and replace it with the actual project Id.

const translate = require('@google-cloud/translate'); translate.detect('{{projectId}}', (err, results) => { console.log(results); });

Output:

{ language: 'fr', confidence: 0.15950840711593628, input: '{{projectId}}' }

It is surprising that the above example discovers french in the input string {{projectId}}! It does so happen that the actual id for my project on Google Cloud is a Quebecois phrase.

The above example is a bit contrived, but it is possible for user input to happen to contain the string {{projectId}}. This is a real concern for us in the Cloud Debug agent as we capture program state upon user request and send it to the debugger API. It is quite possible for the user application to have the above string, or any other possible string, that will be silently replaced in transit. Other services like Storage, compute or resource might also have plausible failure cases.

I do like the convenience of the projectId placeholder string auto-replaced to the projectId during transit, but this leaves open the possibility that valid user input may get replaced accidentally. It might be a bit less elegant/convenient, but I think we should not use a mechanism that can accidentally edit user provided strings, however unlikely.
@ofrobots
February 10, 2017 22:34
Bump. Any traction on this?
@stephenplusplus
February 16, 2017 19:32
The only thing I can think of is a more randomized string, e.g. {{projectId + uuid.v1()}}. Do you have any ideas?
@bjwatson
March 2, 2017 00:10
@stephenplusplus Could we add an optional boolean that says to interpret the string literally, rather than doing auto-replace? Kind of like the difference between grep and fgrep?

Copied from GoogleCloudPlatform/google-cloud-node#1891
	@ofrobots December 19, 2016 20:15
Environment details OS: all Node.js version: all npm version: all google-cloud-node version: `master` Steps to reproduce The request mechanism provided used by `Service` and `ServiceObject` go through the request body and modify all occurrences of the string `{{projectId}}` and replace it with the actual project Id. `const translate = require('@google-cloud/translate'); translate.detect('{{projectId}}', (err, results) => { console.log(results); });` Output: `{ language: 'fr', confidence: 0.15950840711593628, input: '{{projectId}}' }` It is surprising that the above example discovers french in the input string `{{projectId}}`! It does so happen that the actual id for my project on Google Cloud is a Quebecois phrase. The above example is a bit contrived, but it is possible for user input to happen to contain the string `{{projectId}}`. This is a real concern for us in the Cloud Debug agent as we capture program state upon user request and send it to the debugger API. It is quite possible for the user application to have the above string, or any other possible string, that will be silently replaced in transit. Other services like Storage, compute or resource might also have plausible failure cases. I do like the convenience of the projectId placeholder string auto-replaced to the projectId during transit, but this leaves open the possibility that valid user input may get replaced accidentally. It might be a bit less elegant/convenient, but I think we should not use a mechanism that can accidentally edit user provided strings, however unlikely.
	@ofrobots February 10, 2017 22:34
Bump. Any traction on this?
	@stephenplusplus February 16, 2017 19:32
The only thing I can think of is a more randomized string, e.g. `{{projectId + uuid.v1()}}`. Do you have any ideas?
	@bjwatson March 2, 2017 00:10
@stephenplusplus Could we add an optional boolean that says to interpret the string literally, rather than doing auto-replace? Kind of like the difference between `grep` and `fgrep`?

Dec 13 '17 20:12 stephenplusplus

Adding the approach here from an email from @ofrobots:

implement Service.prototype.projectIdPromise. Ideally we should remove Service.prototype.projectId as it is asynchronously initialized. (GAX based libraries will need their analogue for this. It would have been nice if there was a common-gax, but that not existing, perhaps the generator needs to be modified. I don't fully understand how the generator works)
anything that needs a projectId to be available should do const promise = await this.projectIdPromise or use a promise.then. This should be easy in libraries that are already using TypeScript, but that is not a strict requirement.
Get rid of all instances of the string {{projectId}} from across of all repos.

Jul 17 '18 16:07 JustinBeckwith

Closing based on age, and I think it was addressed. Feel free to reopen if not.

Feb 02 '23 20:02 sofisl