aws-cdk icon indicating copy to clipboard operation
aws-cdk copied to clipboard

wafv2: `CfnWebACL.JsonMatchPatternProperty.all` doesn't accept `any` value as documented

Open gravieure opened this issue 1 year ago • 6 comments

Describe the bug

This pertains to the Python bindings, but I believe the issue affects TypeScript as well, though to a lesser degree.

The documentation for JsonMatchPatternProperty.all states:

all (Any) – Match all of the elements. See also MatchScope in the JsonBody FieldToMatch specification. You must specify either this setting or the IncludedPaths setting, but not both.

However, if I specify all=True, cdk synth fails:

RuntimeError: Error: Resolution error: Supplied properties not correct for "CfnWebACLProps"
  rules: element 2: supplied properties not correct for "RuleProperty"
    statement: supplied properties not correct for "StatementProperty"
      sqliMatchStatement: supplied properties not correct for "SqliMatchStatementProperty"
        fieldToMatch: supplied properties not correct for "FieldToMatchProperty"
          jsonBody: supplied properties not correct for "JsonBodyProperty"
            matchPattern: supplied properties not correct for "JsonMatchPatternProperty"
              all: true should be an 'object'.

So the type hint is incorrect; a value of Any type is not legal for the all argument.

The example code in the documentation is:

import aws_wafv2 as wafv2

# all: Any

json_match_pattern_property = wafv2.CfnWebACL.JsonMatchPatternProperty(
    all=all,
    included_paths=["includedPaths"]
)

This violates the immediately preceding text, "You must specify either this setting or the IncludedPaths setting, but not both." Specifying only all=all does not work; all is a built-in function in Python, which causes a JSII error:

jsii.errors.JSIIError: Cannot pass function as argument here (did you mean to call this function?): <built-in function all>

It appears that I have to pass some JSII-serializable value here to indicate truthy state, such as:

match_pattern=waf.CfnWebACL.JsonMatchPatternProperty(all={}),

This is a very unusual way to express a boolean, particularly because an empty dict is considered False in Python:

>>> "true" if {} else "false"
'false'
>>> "true" if {"some": "value"} else "false"
'true'
>>> 

Expected Behavior

  • I expected that a value conforming to the type hint of the all attribute would work.
  • I expected that the example code would work.
  • I expect fields for boolean options to accept boolean values.
  • I expect fields taking truthy/falsey values to conform to Python's notions of truthy and falsey.

Current Behavior

  • Example code does not work.
  • Boolean values are not accepted.
  • Values treated as falsey by Python are treated as truthy by CDK.

Reproduction Steps

  1. Synth a stack containing a WAF with JsonMatchPatternProperty whose all value is all, as the example code does.
  2. Observe that the synth fails

or

  1. Synth a stack containing a WAF with JsonMatchPatternProperty whose all value is True.
  2. Observe that the synth fails.

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.147.1 (build d3695d4)

Framework Version

2.147.1

Node.js Version

v18.15.0

OS

macOS Sonoma 14.5 (23F79)

Language

Python

Language Version

Python 3.10.7

Other information

The TypeScript documentation also says that all? can accept any type, though this is not true.

gravieure avatar Jun 25 '24 21:06 gravieure

@gravieure , thanks for reaching out. I am not able to reproduce the issue. Could you please share a minnimal reproducible code snippet?

khushail avatar Jun 26 '24 01:06 khushail

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

github-actions[bot] avatar Jun 28 '24 04:06 github-actions[bot]

@gravieure , thanks for reaching out. I am not able to reproduce the issue. Could you please share a minnimal reproducible code snippet?

The sample code in the documentation is a reproducer.

You can reproduce the other described behavior by changing the all=all in that code to all=True.

gravieure avatar Jun 28 '24 15:06 gravieure

@gravieure , Thanks for clarification.

Since this is a CFN issue, please create an issue with the CFN team here on their Cloudformation coverage map and follow that one for updates/resolution.

Thanks.

khushail avatar Jun 28 '24 20:06 khushail

@gravieure , Thanks for clarification.

Since this is a CFN issue, please create an issue with the CFN team here on their Cloudformation coverage map and follow that one for updates/resolution.

Thanks.

I apologize for not understanding, but could you explain why this is CFN issue? The type specified in CDK's code is incorrect -- the declared type is any, but values of any type are not accepted. The example code with all=all (which doesn't work) is in the CDK documentation.

Are the types in the CDK code, and the examples in the CDK documentation generated from CFN?

gravieure avatar Jun 28 '24 21:06 gravieure

Ah My Bad @gravieure , I missed comprehending the whole context. I will dive deep and get back to you

khushail avatar Jun 28 '24 22:06 khushail

@gravieure , AFAIK, the doc generation is from jsii-docgen which generates the API.md and we tend to write the original implementation in TypeScript and have jsii-docgen to generate docs for different languages. I noticed in the Tyepscript as well as Python doc that all and includePaths should not have come together in the example code.

Also CfnWebACL is cloudformation resource (Doc) - all takes JSON object as mentioned in the doc . An example is also given which explains how values are passed. Types are also auto generated. This seems to be a JSII related issue and how docs are autogenerated. So marking it as appropriate.

khushail avatar Jul 08 '24 20:07 khushail

AFAIK , the doc generation is from jsii-docgen which generates the API.md and we tend to write the original implementation in TypeScript and have jsii-docgen to generate docs for different languages. I noticed in the Tyepscript as well as Python doc that all and includePaths should not have come together in the example code.

Also CfnWebACL is cloudformation resource (Doc) - all takes JSON object as mentioned in the doc . An example is also given which explains how values are passed. Types are also auto generated.

Thank you for the explanations.

I note that the same incorrect type and example exists in the TypeScript docs and example -- the field type is any, but I'm fairly sure that true wouldn't work in this instance, either.

It's extra confusing in Python, though, because an empty dict is treated as false -- so a false value in the host language is used as a true value by CDK.

Are there any plans to improve this API? I've found it much more difficult to work with than other CDK APIs, and since WAF is an important security control, I think the chances of getting something wrong in a way that negatively impacts security is high.

gravieure avatar Jul 08 '24 22:07 gravieure

@gravieure , thanks for keeping patience!

I don't have any ETA but team surely has plans of improving the docs experience for the community.

khushail avatar Jul 09 '24 00:07 khushail

@gravieure , thanks for keeping patience!

I don't have any ETA but team surely has plans of improving the docs experience for the community.

Thank you. Please note that there are two components to my report: code problems and documentation problems.

The code is buggy, because it declares that certain fields can accept values of Any type, when they cannot. This leads to the nonsensical situation where fields representing a boolean can't accept True as their value -- they need a value which is considered false by Python. This is very confusing and unintuitative.

The examples in the documentation are incomplete and incorrect, which I suspect is because it's some kind of auto-generated output based on the code, which is buggy.

gravieure avatar Aug 14 '24 15:08 gravieure