Closed alisharify7 closed 7 months ago
Been busy, but I'll try to review this over the weekend :)
Code needs a fair bit of adjustment. See comments. In it's current form it would introduce new vulnerabilities.
Besides that:
- There are no tests and the existing test suite is broken.
- Pipenv is not up-to-date
I made the changes and only the tests remain to be updated
One question, why is unittest used when Flask itself suggested using pytest for testing ()?
Code needs a fair bit of adjustment. See comments. In it's current form it would introduce new vulnerabilities. Besides that:
- There are no tests and the existing test suite is broken.
- Pipenv is not up-to-date
I made the changes and only the tests remain to be updated
One question, why is unittest used when Flask itself suggested using pytest for testing ()?
No real reason, this is a pretty old project.
Code needs a fair bit of adjustment. See comments. In it's current form it would introduce new vulnerabilities. Besides that:
- There are no tests and the existing test suite is broken.
- Pipenv is not up-to-date
I made the changes and only the tests remain to be updated One question, why is unittest used when Flask itself suggested using pytest for testing ()? https://flask.palletsprojects.com/en/3.0.x/testing/
No real reason, this is a pretty old project.
Code needs a fair bit of adjustment. See comments. In it's current form it would introduce new vulnerabilities. Besides that:
- There are no tests and the existing test suite is broken.
- Pipenv is not up-to-date
I made the changes and only the tests remain to be updated One question, why is unittest used when Flask itself suggested using pytest for testing ()? https://flask.palletsprojects.com/en/3.0.x/testing/
No real reason, this is a pretty old project.
can I migrate the tests to pytest ?
Code needs a fair bit of adjustment. See comments. In it's current form it would introduce new vulnerabilities. Besides that:
- There are no tests and the existing test suite is broken.
- Pipenv is not up-to-date
I made the changes and only the tests remain to be updated One question, why is unittest used when Flask itself suggested using pytest for testing ()? https://flask.palletsprojects.com/en/3.0.x/testing/
No real reason, this is a pretty old project.
Code needs a fair bit of adjustment. See comments. In it's current form it would introduce new vulnerabilities. Besides that:
- There are no tests and the existing test suite is broken.
- Pipenv is not up-to-date
I made the changes and only the tests remain to be updated One question, why is unittest used when Flask itself suggested using pytest for testing ()? https://flask.palletsprojects.com/en/3.0.x/testing/
No real reason, this is a pretty old project.
can I migrate the tests to pytest ?
Sure, go for it.
Code needs a fair bit of adjustment. See comments. In it's current form it would introduce new vulnerabilities. Besides that:
- There are no tests and the existing test suite is broken.
- Pipenv is not up-to-date
I made the changes and only the tests remain to be updated One question, why is unittest used when Flask itself suggested using pytest for testing ()? https://flask.palletsprojects.com/en/3.0.x/testing/
No real reason, this is a pretty old project.
Code needs a fair bit of adjustment. See comments. In it's current form it would introduce new vulnerabilities. Besides that:
- There are no tests and the existing test suite is broken.
- Pipenv is not up-to-date
I made the changes and only the tests remain to be updated One question, why is unittest used when Flask itself suggested using pytest for testing ()? https://flask.palletsprojects.com/en/3.0.x/testing/
No real reason, this is a pretty old project.
can I migrate the tests to pytest ?
Sure, go for it.
hi, it is done all tests moved to pytest and also all changes applied
changes:
moving tests from unittest to pytest + adding new tests
Testing with non-numeric characters I notice a lot of cut-off characters which make the captcha unsolveable. For example:
(Solution: cmju
)
Not a blocker, likely relates to this old issue, so I'm not sure it's solvable without an upstream update. https://github.com/lepture/captcha/issues/22
We can create our own issue for this later once this is merged.
However an issue here is with the new functionality of setting different config via template:
<form method="post">
{{ captcha(numeric=True) }}
<input type="text" name="captcha">
<input type="submit" name="submit">
</form>
and
app.config['CAPTCHA_INCLUDE_ALPHABET'] = True # include alphabet<letters>
app.config['CAPTCHA_INCLUDE_NUMERIC'] = False # include numeric
app.config['CAPTCHA_INCLUDE_PUNCTUATION'] = True # include symbols
causes:
UnboundLocalError: local variable 'numeric' referenced before assignment
File "/home/tethik/code/python/tmp/flask-session-captcha/sample/application/templates/form.html", line 2, in top-level template code
{{ captcha(numeric=True) }}
File "/home/tethik/code/python/tmp/flask-session-captcha/flask_session_captcha/__init__.py", line 86, in generate
base64_captcha = self.__generate(*args, **kwargs)
File "/home/tethik/code/python/tmp/flask-session-captcha/flask_session_captcha/__init__.py", line 112, in __generate
if numeric and not alphabet and not punctuation:
UnboundLocalError: local variable 'numeric' referenced before assignment
Testing with non-numeric characters I notice a lot of cut-off characters which make the captcha unsolveable. For example: (Solution:
cmju
)Not a blocker, likely relates to this old issue, so I'm not sure it's solvable without an upstream update. lepture/captcha#22
We can create our own issue for this later once this is merged.
Blocker
However an issue here is with the new functionality of setting different config via template:
<form method="post"> {{ captcha(numeric=True) }} <input type="text" name="captcha"> <input type="submit" name="submit"> </form>
and
app.config['CAPTCHA_INCLUDE_ALPHABET'] = True # include alphabet<letters> app.config['CAPTCHA_INCLUDE_NUMERIC'] = False # include numeric app.config['CAPTCHA_INCLUDE_PUNCTUATION'] = True # include symbols
causes:
UnboundLocalError: local variable 'numeric' referenced before assignment File "/home/tethik/code/python/tmp/flask-session-captcha/sample/application/templates/form.html", line 2, in top-level template code {{ captcha(numeric=True) }} File "/home/tethik/code/python/tmp/flask-session-captcha/flask_session_captcha/__init__.py", line 86, in generate base64_captcha = self.__generate(*args, **kwargs) File "/home/tethik/code/python/tmp/flask-session-captcha/flask_session_captcha/__init__.py", line 112, in __generate if numeric and not alphabet and not punctuation: UnboundLocalError: local variable 'numeric' referenced before assignment
this is for old version pull new version and pass arguments like this:
{{ captcha(include_numeric=True) }}
or
{{ captcha(include_numeric=True, include_alphabet=False, include_punctuation=False) }}
Now trivially crashes:
File "/home/tethik/code/python/tmp/flask-session-captcha/sample/application/templates/form.html", line 2, in top-level template code
{{ captcha() }}
File "/home/tethik/code/python/tmp/flask-session-captcha/flask_session_captcha/__init__.py", line 87, in generate
base64_captcha = self.__generate(include_alphabet=kwargs.get(__key="include_alphabet", __default=False),
TypeError: dict.get() takes no keyword arguments
Please test your code.
Now trivially crashes:
File "/home/tethik/code/python/tmp/flask-session-captcha/sample/application/templates/form.html", line 2, in top-level template code {{ captcha() }} File "/home/tethik/code/python/tmp/flask-session-captcha/flask_session_captcha/__init__.py", line 87, in generate base64_captcha = self.__generate(include_alphabet=kwargs.get(__key="include_alphabet", __default=False), TypeError: dict.get() takes no keyword arguments
Please test your code.
Ops that was my mistake, i passed key and default to kwargs.get I remove it and now it's working OK
LGTM now. I'll do some cleanup and start preparing a release.
flask-sessionstore is an old flask extension > replace it with flask-session change FlaskSessionCaptcha.generate method > This method now can get three arg as an input
adding a logger for printing logs in stdout > flask_session_captcha/logger.py adding custom exception class flask_session_captcha/exception.py adding a base config class for handling all default values for captcha > flask_session_captcha/init.py:16:BaseConfig: and adding type annotation to methods and upgrade version from 1.3.0 to 1.4.0 update readme with the new version update sample app with new sample app delete old sample apps