Flask set-cookie codemod

[clavedeluna]

Overview

New codemod to check if flask's set_cookie is called with the most secure params

Description

• This codemod is merge after cursory review because it is possible to call set_cookie with default (seemingly unsafe) parameters but the safety is configured at the Flask app level configurations. However, whatever is passed to set_cookie takes precedence so calling it with unsafe params should be changed.
• Checking flask cookie config settings will be a separate codemod.

[codecov]

Codecov Report

Merging #101 (24e653b) into main (6ee8779) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #101      +/-   ##
==========================================
+ Coverage   95.68%   95.72%   +0.04%     
==========================================
  Files          61       62       +1     
  Lines        2504     2528      +24     
==========================================
+ Hits         2396     2420      +24     
  Misses        108      108              

Files	Coverage Δ
src/core_codemods/__init__.py	100.00% <100.00%> (ø)
src/core_codemods/secure_flask_cookie.py	100.00% <100.00%> (ø)

[clavedeluna]

We could consider allowing users to pass samesite=Strict and not changing it to Lax

[drdavella]

Yes I think we should not modify samesite if it is already less permissive than our codemod.

[andrecsilva]

Lacking tests with open(...) as a source, otherwise ok.

[clavedeluna]

Lacking tests with open(...) as a source, otherwise ok.

Can you clarify with a code example?

[andrecsilva]

Lacking tests with open(...) as a source, otherwise ok.

Can you clarify with a code example?

Your semgrep rule has 3 possibilities for source (flask.make_response(...), flask.Response(...) and open(...)). See:

            pattern-sources:
              - pattern-either:
                  - patterns:
                    - pattern: flask.make_response(...)
                    - pattern-inside: |
                        import flask
                        ...
                  - patterns:
                    - pattern: flask.Response(...)
                    - pattern-inside: |
                        import flask
                        ...
                  - pattern: open(...)

All the unit tests are parameterized with this:

each_func = pytest.mark.parametrize("func", ["make_response", "Response"])

Thus, no test covers an example like:

import flask
response = open(something)
var = "hello"
response.set_cookie("name", "value")

I'm not sure if this is actually valid code. Just following the semgrep rule.

[clavedeluna]

Lacking tests with open(...) as a source, otherwise ok.

Can you clarify with a code example?

Your semgrep rule has 3 possibilities for source (flask.make_response(...), flask.Response(...) and open(...)). See:

            pattern-sources:
              - pattern-either:
                  - patterns:
                    - pattern: flask.make_response(...)
                    - pattern-inside: |
                        import flask
                        ...
                  - patterns:
                    - pattern: flask.Response(...)
                    - pattern-inside: |
                        import flask
                        ...
                  - pattern: open(...)

All the unit tests are parameterized with this:

each_func = pytest.mark.parametrize("func", ["make_response", "Response"])

Thus, no test covers an example like:

import flask
response = open(something)
var = "hello"
response.set_cookie("name", "value")

I'm not sure if this is actually valid code. Just following the semgrep rule.

OHHHH this is a total typo, from copy paste. open is not a valid source. I will remove it. Good catch!

[drdavella]

This looks really nice to me overall, but I do think we should preserve samesite="Strict" if it's already there. We don't want to make the app less permissive, only moreso.

[drdavella]

Yes I think we should not modify samesite if it is already less permissive than our codemod.