Flip insecure defined flask session config

[clavedeluna]

Overview

A codemod that will check if SESSION_COOKIE_HTTPONLY, SESSION_COOKIE_SECURE or SESSION_COOKIE_SAMESITE are set to an insecure value on a flask app config object.

Description

• This codemod checks any instances of a Flask app for any calls to its configuration. If any of those calls leads to setting any of the 3 flags to their unsafe value, this codemod will change the value.
• Note that after quite a bit of back and forth, we've decided this codemod will not at this time add set the secure values for any of these configs. We will only handle the case when the user specifically set to an unsafe value. We're aware that some default values are unsafe.
• I've left some commented out code which did handle ^ above by adding a line at the very end of the module any of the config values which were not set. We decided there is some risk to adding this line at the end of the module so we won't do so right now.

[codecov]

Codecov Report

Merging #119 (0b21ebd) into main (6f113f4) will increase coverage by 0.12%.
The diff coverage is 99.04%.

❗ Current head 0b21ebd differs from pull request most recent head 3bbbfee. Consider uploading reports for the commit 3bbbfee to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #119      +/-   ##
==========================================
+ Coverage   96.33%   96.46%   +0.12%     
==========================================
  Files          66       67       +1     
  Lines        2729     2828      +99     
==========================================
+ Hits         2629     2728      +99     
  Misses        100      100              

Files	Coverage Δ
...ject_analysis/file_parsers/setup_py_file_parser.py	100.00% <100.00%> (ø)
src/core_codemods/__init__.py	100.00% <100.00%> (ø)
src/core_codemods/secure_flask_session_config.py	100.00% <100.00%> (ø)
src/codemodder/utils/utils.py	94.11% <94.11%> (ø)

... and 2 files with indirect coverage changes

[andrecsilva]

Some minor issues, otherwise it looks good.

[andrecsilva]

This won't work with AnnAssign, as the target value is stored in the target attribute. You should separate the cases.

This may help:

    def _extract_targets_of_assignment(
        self, assignment: cst.AnnAssign | cst.Assign | cst.WithItem | cst.NamedExpr
    ) -> list[cst.BaseExpression]:
        match assignment:
            case cst.AnnAssign():
                if assignment.target:
                    return [assignment.target]
            case cst.Assign():
                return [t.target for t in assignment.targets]
            case cst.NamedExpr():
                return [assignment.target]
            case cst.WithItem():
                if assignment.asname:
                    return [assignment.asname.name]
        return []

[clavedeluna]

I assume you wrote that function and it isn't from somewhere I should cite, so I'm taking it as is.

[andrecsilva]

I wrote it for the file resource leak codemod. This PR will probably be merged before, so go ahead.

[andrecsilva]

This assumes that the target is a Name, but the target may be any slew of different types of expressions like Subscript:(l[0] = Flask(...)) , Attribute:(a.b = Flask(...)), etc. So you should make it fail in the other cases.

My suggestion is to break this detection part into a new method is_assigned_to_variable with return Optional[Name].

[andrecsilva]

When using matchers.matches, you should use the matchers version of the nodes instead (i.e. matchers.Name instead of cst.Name).

See: https://libcst.readthedocs.io/en/latest/matchers.html#libcst.matchers.matches

[andrecsilva]

See comment in _is_config_subscript. Same issue.

[drdavella]

Thanks for all of the back and forth on this, I know it ended up being fairly tricky.

Based on the expected behavior in the tests this looks good to me. My only substantive comment is to remove the file that doesn't belong in this PR. Once @andrecsilva's other comments are addressed this should be good to go.

[drdavella]

I think this file doesn't belong in this PR, it looks like it's from another ticket.

[clavedeluna]

This change does belong here, I didn't add the file, I just moved a function to utils so I had to change the import

[drdavella]

Ah sorry my bad.