Fix dangerous destructuration in typescript-nestjs services #20157

GregoryMerlet · 2024-11-21T16:06:13Z

Closes #20156

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package || exit
./bin/generate-samples.sh ./bin/configs/*.yaml || exit
./bin/utils/export_docs_generators.sh || exit
```
(For Windows users, please run the script in Git BASH)
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

TypeScript committee members: @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02) @davidgamero (2022/03) @mkusaka (2022/04) @joscha (2024/10)

joscha

Can we add an example resource that shows this behavior and ideally also fails the type check if this is ever broken again? I think this is quite fragile and from time to time people might destructure without thinking about reserved names, so it would be good to cover it with a resource that has some reserved words as parameters?

GregoryMerlet · 2024-11-21T16:57:22Z

@joscha I've updated with a new sample that should break in case of destructuration. Let me know if there's more to add/change 😃

joscha · 2024-11-21T17:10:36Z

samples/client/petstore/typescript-nestjs/builds/reservedParamNames/api/default.service.ts

+     * @type {string}
+     * @memberof DefaultServiceTestReservedParamNames
+     */
+    readonly from: string


Are these not expected to be quoted? I think some other models also use a sanitized name prefixed with a _ - @macjohnny ?

I'm not sure to understand what you mean by quoted here

Sorry, like this: https://stackoverflow.com/questions/43124403/is-there-a-way-to-escape-and-use-reserved-words-in-typescript-class-definitions

And an example of the (automatically added) prefix: https://github.com/planet-a-ventures/affinity-node/blob/main/src%2Fv2%2Fgenerated%2Fmodels%2FEmail.ts#L33

You can see that the properties in the typescript generator are also all quoted by default.

I've added the quotes.
I saw how sanitized names works with the reservedWords Set, I'm not sure from should be considered as a reserved word here as it's not a problem anymore after the modifications. But if you confirm you want it added to the reservedWords I can add it

in general its better to add to the reserved keywords instead of the modifications you suggest, since there are many places where variables are used, so the code change is rather extensive and complicates the templates.

the reserved keyword would be instead of the quotes, which will prefix the variable with an underscore, e.g. _from. however, for the model this makes it differ from what the server actually sends, so probably for the model the quotes are fine. i would suggest to keep this PR as small as possible and only fix one thing at a time, to make it easier to review

From what I understand if we add it to the reserved keywords it will be changed in the model too. Or there is a way to get a non-sanitized name in the model ?

you are right, adding to the reserved keywords changes it for the model as well. you could apply the same as in

openapi-generator/modules/openapi-generator/src/main/resources/typescript-angular/modelGeneric.mustache

Line 20 in ed21105

{{#isReadOnly}}readonly {{/isReadOnly}}{{#hasSanitizedName}}'{{{baseName}}}'{{/hasSanitizedName}}{{^hasSanitizedName}}{{{name}}}{{/hasSanitizedName}}{{^required}}?{{/required}}: {{#isEnum}}{{{datatypeWithEnum}}}{{/isEnum}}{{^isEnum}}{{{dataType}}}{{/isEnum}}{{#isNullable}} | null{{/isNullable}};

Ok thank you ! I will adapt my changes as soon as possible

It should be better now 😃

macjohnny · 2024-11-27T16:31:52Z

modules/openapi-generator/src/main/resources/typescript-nestjs/api.service.mustache

@@ -106,7 +106,7 @@ export class {{classname}} {
 {{#useSingleRequestParameter}}
        const {
        {{#allParams}}
-            {{paramName}},
+            {{#hasSanitizedName}}'{{{baseName}}}': {{/hasSanitizedName}}{{paramName}},


this is not an object, but destructuring the request parameters, please revert this change

I know, it's to keep the request parameter with the original names while renaming the params in this method to avoid conflicts with existing properties.
In the end it looks like this :

const { notReserved, 'from': _from, 'headers': _headers, } = requestParameters;

ah i see, i didnt know this is possible

macjohnny · 2024-11-27T16:34:59Z

...enerator/src/main/java/org/openapitools/codegen/languages/TypeScriptNestjsClientCodegen.java

@@ -265,6 +269,34 @@ private boolean isLanguageGenericType(String type) {
        return false;
    }

+    @Override
+    public List<CodegenParameter> fromRequestBodyToFormParameters(RequestBody body, Set<String> imports) {


please separate this change of parameters into a separate PR. also, is there an easier way than to extends CodegenParameter?

I was inpired by the typescript-fetch generator. If you know a better way to do it I can make the changes

ah i see. the risk is just that whenever new properties are added to the CodegenParameter, we will probably forget to add them here. can this be automated/avoided somehow?

I see uses of param.vendorExtensions in others generators:

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/PhpNextgenClientCodegen.java

Line 204 in ed21105

param.vendorExtensions.putIfAbsent("x-php-param-type", "array");

Do you think it would be a better approach as it doesn't require to extend CodegenParameter ?

oh that sounds compelling, yes please try if that works!

Done ! It works as well 😃

thanks!
are you motivated to also do that for the typescript-fetch generator in a separate PR?

You mean using vendorExtensions instead of extending CodegenParameter ? Or keeping original names in requestParameters interfaces ? Or both maybe ? 😃

refactor: remove requestParameters destructuration

3a8800e

joscha reviewed Nov 21, 2024

View reviewed changes

feat: add reserved param names sample

9d9259e

joscha reviewed Nov 21, 2024

View reviewed changes

GregoryMerlet added 2 commits November 25, 2024 17:31

feat: quote params

eabc989

feat: improve with reservedWords

9c7fcfa

macjohnny reviewed Nov 27, 2024

View reviewed changes

feat: use vendorExtensions instead of extending CodegenParameter

d87f086

GregoryMerlet force-pushed the fix_issue_20156 branch from bf1102b to d87f086 Compare December 2, 2024 16:08

macjohnny approved these changes Dec 3, 2024

View reviewed changes

macjohnny merged commit cf78f10 into OpenAPITools:master Dec 3, 2024
27 checks passed

GregoryMerlet deleted the fix_issue_20156 branch December 9, 2024 12:58

wing328 added this to the 7.11.0 milestone Dec 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dangerous destructuration in typescript-nestjs services #20157

Fix dangerous destructuration in typescript-nestjs services #20157

GregoryMerlet commented Nov 21, 2024

joscha left a comment

GregoryMerlet commented Nov 21, 2024 •

edited

Loading

joscha Nov 21, 2024

GregoryMerlet Nov 22, 2024

joscha Nov 22, 2024

GregoryMerlet Nov 25, 2024

macjohnny Nov 25, 2024

macjohnny Nov 26, 2024

GregoryMerlet Nov 26, 2024

macjohnny Nov 26, 2024

GregoryMerlet Nov 26, 2024

GregoryMerlet Nov 27, 2024

macjohnny Nov 27, 2024

GregoryMerlet Nov 27, 2024

macjohnny Nov 27, 2024

macjohnny Nov 27, 2024

GregoryMerlet Nov 27, 2024

macjohnny Nov 27, 2024

GregoryMerlet Nov 27, 2024

macjohnny Nov 29, 2024

GregoryMerlet Nov 29, 2024

macjohnny Dec 3, 2024

GregoryMerlet Dec 9, 2024

Fix dangerous destructuration in typescript-nestjs services #20157

Fix dangerous destructuration in typescript-nestjs services #20157

Conversation

GregoryMerlet commented Nov 21, 2024

PR checklist

joscha left a comment

Choose a reason for hiding this comment

GregoryMerlet commented Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GregoryMerlet commented Nov 21, 2024 •

edited

Loading