Skip to content

fix: scope MD5 PASSWORD_HASHERS override to test runs only#1096

Merged
danh91 merged 1 commit into
karrioapi:mainfrom
mgradalska:fix/password-hashers-production-override
Jun 23, 2026
Merged

fix: scope MD5 PASSWORD_HASHERS override to test runs only#1096
danh91 merged 1 commit into
karrioapi:mainfrom
mgradalska:fix/password-hashers-production-override

Conversation

@mgradalska

Copy link
Copy Markdown
Contributor

refs https://github.com/orgs/karrioapi/discussions/1094

Bug

After upgrading past 2026.1.22, existing users can't log in to the dashboard, and new users have their passwords stored as MD5.

Root Cause

The test-only PASSWORD_HASHERS override in [settings/base.py](https://github.com/karrioapi/karrio/blob/main/apps api/karrio/server/settings/base.py#L377-L383) (added in 2026.1.22, commit f19fe206e) fires in production because its second predicate matches the install path:

if "test" in _sys.argv or "karrio" in _sys.argv[0]:

In karrio/server, gunicorn's sys.argv[0] is /karrio/venv/bin/gunicorn - which contains "karrio". The override fires unconditionally.

Fix

Replace the argv predicate with a custom TEST_RUNNER that sets PASSWORD_HASHERS in setup_test_environment. Django loads TEST_RUNNER only during karrio test (the test management command), so production is never affected and the test-suite perf win from f19fe206e is preserved.

Tests

Couldn't run the full suite locally. Relying on this PR's karrio-tests workflow.

Notes

Already MD5-hashed users (created on any 2026.1.22+ deploy) will be locked out by this fix the same way PBKDF2 users were locked out by the original bug. Worth shipping a release that appends MD5PasswordHasher to PASSWORD_HASHERS.

The previous predicate (`"karrio" in _sys.argv[0]`) matches the install
path of the `karrio` virtualenv (`/karrio/venv/bin/gunicorn`), so the
test-only MD5 PASSWORD_HASHERS override was firing in production
gunicorn workers as well. Two consequences for any deployment upgraded
past 2026.1.22:

- Pre-upgrade users with PBKDF2-hashed passwords can no longer log in,
  because the loaded hasher list cannot verify their hash format.
- New users (createsuperuser, dashboard signup) have their passwords
  stored as MD5, which Django itself documents as test-only.

Replace the argv-based predicate with a custom TEST_RUNNER subclass that
sets PASSWORD_HASHERS in `setup_test_environment`. Django loads
TEST_RUNNER only for `manage.py test` / `karrio test`, so the override
fires exactly when intended and never touches production process state.

refs https://github.com/orgs/karrioapi/discussions/1094
@vercel

vercel Bot commented May 26, 2026

Copy link
Copy Markdown

@mgradalska is attempting to deploy a commit to the karrio Team on Vercel.

A member of the Team first needs to authorize it.

@ChrisNolan

Copy link
Copy Markdown
Contributor

Getting the dev environment setup is a bit daunting, but the docs here are the current ones afaik and using the ./bin/install-dev worked well for me recently when I got back into fixing things. If you want to run the tests fully too just in case.

Nice to see more PRs imo 🫂 #1094

If It was me... I'd wonder if anything would need to be done from a data standpoint for systems that might have gotten some messed up users in the interim... or if we just trust those users to be able to re-setup the users which have the wrong hashing...?

@ChrisNolan

Copy link
Copy Markdown
Contributor

FYI I used

sudo docker exec -it karrio.db psql -U postgres -d db -c "
select email, is_staff, is_superuser, is_active, left(password, 80) as password_prefix
from \"user_user\"
order by email;
"

To 'see' the hash used for the existing passwords to help me debug after upgrading without this PR and also not being able to login -- I saw pbkdf2_sha256 in the password prefix. After applying this PR, without any data changes I was able to login.

Thanks for the save mgradalska! Hopefully it'll get merged soon so others don't have the same struggle.

@mgradalska

Copy link
Copy Markdown
Contributor Author

@ChrisNolan Honestly, I don't think there's a clean automated remediation worth shipping here. Anything we'd do (force reset, append MD5 as a verifier, etc.) is an operator-policy call rather than a library decision.

For my own deployment I just created a fresh user once we hit this - that was an acceptable solution in my case. But I imagine this can be quite problematic for anyone with lots of users - for those MD5PasswordHasher could be appended at the end of PASSWORD_HASHERS so existing accounts continue to verify. Django then auto-rehashes to PBKDF2 on next successful login.

A note in the changelog calling out that user passwords created on 2026.1.22+ may be stored as MD5 (and the available migration paths) feels like the right level of disclosure to me.

@danh91

danh91 commented Jun 23, 2026

Copy link
Copy Markdown
Member

Reviewed — looks good, approving. Thanks @mgradalska, and for tracing it to the venv path matching the predicate.

Correct root-cause fix: the old "karrio" in sys.argv[0] predicate matched the virtualenv install path and fired in production gunicorn workers. Moving the override into a TEST_RUNNER.setup_test_environment subclass means it loads only under karrio test / manage.py testTEST_RUNNER is wired in settings/base.py, so every test path gets the fast MD5 hasher and production never does. Confirmed the suite still runs green with the runner.

One follow-up (not for this PR): deployments that ran ≥2026.1.22 already persisted MD5 hashes, so those accounts keep verifying as MD5 until the password is reset — we'll add an upgrade note and look at an optional rehash-on-login separately.

Heads up: this has been bundled (cherry-picked, authorship preserved) into the 2026.1.32 patch in #1128 so it ships alongside the other critical fixes.

@danh91 danh91 merged commit 4a426a7 into karrioapi:main Jun 23, 2026
1 of 3 checks passed
danh91 added a commit that referenced this pull request Jun 23, 2026
#1096, #1120, #1095, #1089, #1114, #1118 were merged directly to main by
mistake — they belong on the 2026.1.32 release branch (#1128), where the same
changes already live (cherry-picks + conventions cleanups). No work is lost;
this only removes their effect from main. They will reach main via #1128.
danh91 added a commit that referenced this pull request Jun 23, 2026
- providers: guard migration 0093 against cascade data-loss (#1116) — depend on
  manager/0079 so carrier FK columns are dropped before the legacy carrier delete.
- manager: make migration 0078 production-safe — chunked iterator + bulk_update,
  idempotent (#1123).
- events: batch periodic_data_archiving deletes to avoid first-run OOM (#1125).
- settings: import workers before apm so huey binds REDIS_HOST under OTEL (#1124).
- settings: scope MD5 PASSWORD_HASHERS to the test runner only (#1096, @mgradalska).
- core: clean up async DB connections to stop the tracing connection leak
  (#1119/#1120 phase 1, @ChrisNolan) + phase-2 PRD.
danh91 added a commit that referenced this pull request Jun 23, 2026
…es (#1128)

* fix(server): critical ops + security fixes for 2026.1.32

- providers: guard migration 0093 against cascade data-loss (#1116) — depend on
  manager/0079 so carrier FK columns are dropped before the legacy carrier delete.
- manager: make migration 0078 production-safe — chunked iterator + bulk_update,
  idempotent (#1123).
- events: batch periodic_data_archiving deletes to avoid first-run OOM (#1125).
- settings: import workers before apm so huey binds REDIS_HOST under OTEL (#1124).
- settings: scope MD5 PASSWORD_HASHERS to the test runner only (#1096, @mgradalska).
- core: clean up async DB connections to stop the tracing connection leak
  (#1119/#1120 phase 1, @ChrisNolan) + phase-2 PRD.

* fix(usps): correct v3 API hosts + vendor official OpenAPI specs

- usps + usps_international: update server URLs to apis.usps.com / apis-tem.usps.com
  after USPS retired the legacy Web Tools / api-cat hosts (#1118, @zebradots).
- vendor the official USPS Developer Portal v3 specs (captured 2026-06-23) for
  usps and usps_international, each with a provenance README.

* feat(fedex): pickupType option, full customer references, pickup improvements

- pickupType settable via the fedex_pickup_type shipping option, typed with the
  FedExPickupType enum (#1095, @ChrisNolan).
- full multi-type customerReferences (CUSTOMER_REFERENCE/INVOICE_NUMBER/
  DEPARTMENT_NUMBER/P_O_NUMBER/RMA_ASSOCIATION), built inline in the request
  tree; fixes the REF field on labels (#1089, @ChrisNolan; disc #1082).
- pickup: map instruction -> remarks, type fedex_pickup_address_type as an enum
  option, resolve package_location via the FedExPackageLocation enum (#1114,
  @ChrisNolan). Multi-recipient notification emails deferred (EBE-124).

* chore(rules): require explicit permission before writing to main

Add a hard guardrail to CLAUDE.md + .claude/rules/git-workflow.md: never
merge/push/force-push/revert main (incl. gh pr merge) without explicit
per-action permission; contributor PRs integrate into the release branch, not
main. Adds the rule that was missing when six PRs were merged to main in error.

* fix(dashboard): align tracker client usage with regenerated API types

The 2026.1.32 metadata regen renamed the Trackers retrieve/remove path param
idOrTrackingNumber -> identifier and made TrackingEvent.description nullable.
Update consumers: tracker delete/retrieve calls use { identifier }, and
getEventIcon accepts string | null.

* release: 2026.1.32

Version bump 2026.1.31 -> 2026.1.32 across packages, regenerated API metadata
(REST/GraphQL types, OpenAPI, schemas), CHANGELOG, and frozen requirements.
Removes SPRINT_MCP.md. Bumps community + ee/platform submodules to their
release commits; ee/insiders stays at v2026.1.29 (its main carries #32 /
task_backend, which ship with 2026.5).
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.

3 participants