Fix FAB session cookie TypeError with Werkzeug 3.0+#67141
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
|
@Sriniketh24 Converting to draft — this PR doesn't yet meet our Pull Request quality criteria.
See the linked criteria for how to fix each item, then mark the PR "Ready for review". This is not a rejection — just an invitation to bring the PR up to standard. No rush. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting |
a3d8050 to
7386c21
Compare
Root cause: Werkzeug 3.0 removed automatic bytes-to-str coercion in set_cookie(). When session_backend=securecookie, the itsdangerous signing serializer may return bytes, causing a TypeError in dump_cookie(). Fix: override save_session() in AirflowSecureCookieSessionInterface to wrap response.set_cookie() with bytes-to-str coercion. The database session serializer (_LazySafeSerializer) is left unchanged since msgpack encoding is correct for LargeBinary columns. closes: apache#64921 Authored with AI assistance (Claude Code).
7386c21 to
02104f9
Compare
|
Reworked the approach based on the CI failures: Previous approach (broken): Changed New approach: Leave the database serializer ( Drafted-by: Claude Code (Opus 4.6); reviewed by @Sriniketh24 before posting |
|
@Sriniketh24 A few things need addressing before review — see our Pull Request quality criteria.
No rush. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting |
The tests patched `__mro__[1]` (SessionExemptMixin) but
`AirflowSecureCookieSessionInterface.save_session()` calls `super()`
which chains through SessionExemptMixin before reaching
SecureCookieSessionInterface. This caused two failures:
1. SessionExemptMixin.save_session accesses `flask.request.path` —
added a `patch("flask.request")` context manager.
2. `patch.object` replaces the class method with a MagicMock whose
`side_effect` does NOT receive `self` — changed the lambda
signature to `*args, **kwargs`.
3. Changed patch target from `__mro__[1]` to `__mro__[2]` to patch
SecureCookieSessionInterface directly, letting SessionExemptMixin
run its real check so the bytes-to-str wrapper is exercised.
|
Thanks for the heads-up! I've pushed a fix for the CI failures — the test mocks had two issues:
The fix patches Drafted-by: Claude Code (Opus 4.7) (no human review before posting) |
|
@Sriniketh24 Converting to draft — this PR doesn't yet meet our Pull Request quality criteria.
See the linked criteria for how to fix each item, then mark the PR "Ready for review". This is not a rejection — just an invitation to bring the PR up to standard. No rush. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting |
session.py imports request via 'from flask import request', binding the name in the module's own namespace at import time. Patching flask.request has no effect on that already-bound reference — the LocalProxy kept in airflow.providers.fab.www.session.request is what SessionExemptMixin.save_session reads. Use that path instead so the mock is visible during the call.
|
Pushed another fix — identified the actual root cause from the CI logs. Root cause: Fix: Changed both mock targets from All other logic (MRO[2] target, Would you be able to move this back out of draft once CI passes? Happy to address any other feedback. Drafted-by: Claude Code (claude-sonnet-4-6) (no human review before posting) |
|
This is already fixed and released as part of Airflow providers fab 3.6.4 under pr#66565. Already tested and working fine. Thank you. Earlier with 3.6.1 i had used explicit type conversion in webserver_config.py to handle the error. Thanks a lot guys for working on this issue. |
|
Closing this in favour of #66565 which was merged 2026-05-08 and addresses the same issue at the root cause level. #66565 moved Thanks @potiuk for the review and @ashwani4588 for pointing out the existing fix. |
Root Cause
Werkzeug 3.0 removed automatic bytes-to-str coercion in
dump_cookie(). Whensession_backend=securecookie, the itsdangerous signing serializer may returnbytes, which Werkzeug 3.0+ rejects withTypeError: cannot use a string pattern on a bytes-like object.Fix
Override
save_session()inAirflowSecureCookieSessionInterfaceto wrapresponse.set_cookie()with bytes-to-str coercion. If the signed session value isbytes, it is decoded tostrbefore being passed to Werkzeug.The database session serializer (
_LazySafeSerializer) is not modified — msgpack encoding produces proper binarybytesforLargeBinarycolumns and works correctly.Tests
Added
providers/fab/tests/unit/fab/www/test_session.pycovering:_LazySafeSerializerroundtrip, LazyString handling, bytes output, encode/decode aliasesAirflowSecureCookieSessionInterface.save_sessionbytes-to-str coercioncloses: #64921
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 4.6) following the guidelines