Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix black security vulnerability #5306

Merged
merged 4 commits into from
Jun 14, 2024
Merged

Conversation

snejus
Copy link
Member

@snejus snejus commented Jun 14, 2024

@snejus snejus self-assigned this Jun 14, 2024
Copy link
Member

@wisp3rwind wisp3rwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with me (even though I'm not sure this really has any security impact). Was there a reason that we capped black at a specific minor version (24.3)?

Since the format check was skipped in CI, it seems possible that changed black defaults in version 25 might break our style checks which we can't see here.

@snejus
Copy link
Member Author

snejus commented Jun 14, 2024

Fine with me (even though I'm not sure this really has any security impact). Was there a reason that we capped black at a specific minor version (24.3)?

Since the format check was skipped in CI, it seems possible that changed black defaults in version 25 might break our style checks which we can't see here.

Good point, let's make the workflow to get triggered by a change in poetry.lock too!

@snejus
Copy link
Member Author

snejus commented Jun 14, 2024

Now let me just figure out how to make flake8 ignore poetry.lock here 😅

@snejus snejus force-pushed the fix-black-security-vulnerability branch from e925675 to 099568a Compare June 14, 2024 18:11
@snejus
Copy link
Member Author

snejus commented Jun 14, 2024

I made the linting workflow to check the entire repository whenever poetry.lock is updated

Run poe check-format
Poe => black --check --diff --color .
All done! ✨ 🍰 ✨
191 files would be left unchanged.
Poe => isort --check --diff --color .
Skipped 1 files

@snejus
Copy link
Member Author

snejus commented Jun 14, 2024

Ignoring flake8-docstrings in aura.py before #5234 is merged

@snejus snejus force-pushed the fix-black-security-vulnerability branch from 7827b62 to 09b67c1 Compare June 14, 2024 19:12
@snejus
Copy link
Member Author

snejus commented Jun 14, 2024

Since the format check was skipped in CI, it seems possible that changed black defaults in version 25 might break our style checks which we can't see here.

@wisp3rwind black version is constrained between >=24.3 and <25 so it can't be updated to 25, see pyproject.toml:

black = ">=24.3,<25"

Relatedly,

Was there a reason that we capped black at a specific minor version (24.3)?

It's constrained at least or above this version, as indicated by the security vulnerability details
image

If you have a glance at poetry.lock you will find that the resolved version is 24.4.2.

@snejus
Copy link
Member Author

snejus commented Jun 14, 2024

Merging this in as I think I addressed your comments @wisp3rwind

@snejus snejus merged commit db1b72f into master Jun 14, 2024
9 checks passed
@snejus snejus deleted the fix-black-security-vulnerability branch June 14, 2024 19:53
@wisp3rwind
Copy link
Member

I made the linting workflow to check the entire repository whenever poetry.lock is updated

Nice!

@wisp3rwind black version is constrained between >=24.3 and <25 so it can't be updated to 25, see pyproject.toml:

Obviously, I wasn't sufficiently awake when I commented here yesterday 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants