Release 0.81.4 - #4021
Merged
Merged
Release 0.81.4#4021
Conversation
…paces (#4002) * fix(auth): disable ApisixUserMiddleware by default in local dev/codespaces ApisixUserMiddleware trusts the X-Userinfo header (base64-encoded JSON identity claims) unconditionally, with no signature or secret tying it to a real Keycloak login through APISIX -- it just assumes the header could only be there because APISIX put it there. The default local dev profile (COMPOSE_PROFILES=backend,frontend) never runs APISIX/Keycloak, and Codespaces doesn't either while also exposing forwardable, network-reachable ports -- so in both cases nothing verifies that header before Django trusts it. A request straight to Django with a forged X-Userinfo header logs in (and creates, if the identity doesn't exist) as whoever the header claims. Verified live, end to end: reproduced the exact repro from the linked issue against a clean checkout (forged header -> is_authenticated: true, brand-new account) with docker compose up (no APISIX/Keycloak profile), then confirmed the same request is fully rejected (is_authenticated: false, no account created) once DISABLE_APISIX_USER_MIDDLEWARE=True is set. DISABLE_APISIX_USER_MIDDLEWARE was already the purpose-built lever for this -- traced through Django's RemoteUserMiddleware base class and confirmed AUTHENTICATION_BACKENDS has no remote_user-capable backend, so this is a safe no-op for real session-based Django logins. QA/ Production are unaffected (they don't set this anywhere; APISIX is the only path in there, confirmed separately in the issue). Existing middleware/admin tests set this explicitly per-test rather than relying on the env default, and aren't affected either (42 passed). Set the default in both env/backend.env and env/codespaces.env, since codespaces.env doesn't inherit from backend.env (extends replaces its env_file list rather than merging). Updated README-keycloak.md's opt-in-to-Keycloak instructions to add the now- necessary DISABLE_APISIX_USER_MIDDLEWARE=False override, and corrected its now-inaccurate "Keycloak is enabled by default" claim. Fixes mitodl/hq#13471 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(tests): force middleware enabled for apisix_user_test.py, add disabled-path case Copilot review feedback on this PR: env/backend.env's new DISABLE_APISIX_USER_MIDDLEWARE=True default is injected into the web container, so the documented `docker compose run --rm web ... pytest main/middleware/apisix_user_test.py` path now runs those tests with the middleware disabled. Confirmed live: on a fresh setup (no personal backend.local.env override), that command now fails 12 tests and errors 6 more with unrelated-looking TypeErrors -- process_request() takes the disabled-path branch (Django's base RemoteUserMiddleware), which touches real request attributes the test mocks don't provide. The existing userinfo_flag_defaults autouse fixture already exists specifically to pin settings regardless of backend.local.env drift (its own docstring says so, for the userinfo create/update flags) -- extended it to also force DISABLE_APISIX_USER_MIDDLEWARE = False, so every test in this file keeps exercising the enabled/trusting code path regardless of the new env default. Added test_disabled_middleware_ignores_forged_header as the requested regression case for the actual disabled path: sets the flag to True and asserts a forged X-Userinfo header is fully ignored through a real request (client.get, not a bare Mock, since the disabled path needs real request attributes) -- the same property already verified live against a real docker compose setup in this PR's earlier commit, now covered at the test level too. Verified via the exact cited repro: `docker compose run --rm web pytest main/middleware/apisix_user_test.py` now passes 19/19 (was 12 failed, 6 errored). Also re-ran via the normal host pytest path (43 passed, unaffected either way). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…#3988) * fix(webhooks): stop leaking the real OCW_WEBHOOK_KEY into logs/Sentry WebhookOCWView.handle_exception embedded the raw request body verbatim into every exception it raises, unconditionally -- including the case where the shared secret check already passed and something else in the request goes wrong afterward, at which point the body genuinely contains the real webhook_key the caller just authenticated with. That message reaches the django.request logger (ERROR, full traceback) and Sentry, both broader audiences than "whoever legitimately calls this webhook." A concrete, no-malice-required way this fires: prefixes arrives as something other than a list/string (e.g. an int, from a buggy client) -- .split(',') raises AttributeError *after* the key check succeeded, routing straight into handle_exception with the real secret still in the body. Redact the webhook_key value out of the body before it's ever embedded in a log/exception message, regardless of whether the key was right or wrong -- simpler than trying to special-case "was this the real secret" and just as effective, since redacting an already-known-wrong attempt costs nothing. Fixes mitodl/hq#13470 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(webhooks): redact based on parsed JSON, not raw-text pattern match Copilot review feedback: JSON allows an object member name to be spelled with \uXXXX escapes -- "webhook_key" decodes to "webhook_key" -- so a request using that spelling still authenticates (rapidjson.loads decodes it before the dict lookup), but the previous raw-text regex only matched the literal "webhook_key" bytes and would leave that spelling's value completely unredacted. Verified this is a real bypass: with the old regex, the real secret survived "redaction" verbatim when spelled this way. _redact_webhook_key now parses the body with the same rapidjson parser the view itself uses, and redacts based on the decoded key -- so any encoding of the same member name is caught, not just the literal bytes. A body that fails to parse as a JSON object (malformed JSON, or valid JSON that isn't an object) is redacted in full rather than assumed safe to log, since we can't confirm it doesn't contain a secret either. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(sentry): scrub OCW_WEBHOOK_KEY from captured Sentry events Human review feedback on this PR (mbertrand): redacting the secret from WebhookOCWView's own exception message doesn't stop it reaching Sentry another way -- verified locally against a fake Sentry endpoint that with the correct webhook_key and prefixes sent as an int (the same post-auth error this PR already fixes at the message level), the key still shows up in the captured event via the "content" local variable in post()'s frame (captured because include_local_variables defaults to True) and via request.data (captured unconditionally by DjangoIntegration for JSON requests). This codebase already has a general mechanism for exactly this class of problem: main/sentry.py's before_send -> scrub_pg_details -> _scrub_node walks every string in the serialized event, originally to truncate a Postgres DETAIL line wherever the SDK could put it (exception value, breadcrumbs, logentry.params, frame locals -- same places this secret can end up). Added scrub_ocw_webhook_key, called from the same recursive walk, so it's applied everywhere the walk already reaches rather than needing to be threaded through every capture path individually. Scoped to OCW_WEBHOOK_KEY specifically, not WEBHOOK_SECRET (the other webhook's secret): WEBHOOK_SECRET only ever exists as an HMAC signing key derived from the request body (webhooks/utils.py), never transmitted itself, so it doesn't have this same "the secret is also valid caller-supplied input" leak path. Verified the fix is real, not vacuous, the same way as the rest of this PR: a throwaway test that monkeypatched the new scrub to a no-op confirmed the secret genuinely leaks into a real captured Sentry event without it. New tests: a direct unit test on scrub_ocw_webhook_key, a scrub_pg_details-level test with the key planted in request.data and frame locals (matching mbertrand's exact finding), and an end-to-end test through the real Django view + real Sentry SDK with DjangoIntegration active (needed for request.data to actually attach) -- reproducing the exact reported scenario and confirming no captured event contains the real secret. 156 tests pass; pre-commit clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Typing into a new article or news item and pausing would appear to reload the page. The first autosave is what creates the item, and the page answered that by pushing the new item's URL -- which unmounts the editor and mounts the edit route in its place. The caret goes, the new route shows a spinner while it fetches the item the editor is already holding, and all of it happens a couple of seconds after the author stops typing, mid-sentence. The URL is now corrected in place with `history.replaceState`, Next's supported way to change it without re-running the route, as `useCanonicalizeResourceParam` already does for the resource drawer. Nothing unmounts. That is safe because the URL is not what the editor saves against: it remembers the row it created and PATCHes that, so only a reload, a bookmark or a shared link needed the address bar to catch up. `replace` rather than `push` also keeps Back out of a trail of one entry per autosave. The edit page's draft branch gets the same treatment. It is unreachable today -- `onSave` only fires there on a publish, since the item always exists -- but it was carrying a `usePathname` guard that existed purely to suppress this same navigation, and the guard goes with it. Co-authored-by: Ahtesham Quraish <ahtesham.quraish@arbisoft.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* chore(api): use the mitxonline-api-axios branch build with contract consent Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * feat(ol-components): let Dialog hide its close button Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * feat(api): add useDataConsentMutation and learner contract factories Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * feat(dashboard): add a disabled state to course cards Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * feat(dashboard): add the B2B data consent dialog behind a feature flag Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * feat(dashboard): require data consent on the B2B contract dashboard Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test(dashboard): use learner contract and organization types in test setup Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(dashboard): show the consent spinner on the button that was clicked Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * chore(api): rebuild the mitxonline-api-axios branch client with openapi-generator v7.25.0 The previous branch build used v7.2.0, which names some types differently from published releases. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * switch back to published api package --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
OpenAPI ChangesNo changes detected Unexpected changes? Ensure your branch is up-to-date with |
odlbot
added
waiting for checkboxes
all checkboxes checked
and removed
deploying to rc
waiting for checkboxes
labels
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Danielle Frappier
Carey P Gumaer
Ahtesham Quraish
Sar