fix: correctly bump justhtml; add config update to handle breaking changes#17279
fix: correctly bump justhtml; add config update to handle breaking changes#17279stevejalim wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a silent break caused by a justhtml upgrade in production by (1) aligning the pinned justhtml version across environments and (2) updating Bedrock’s URL sanitization policy to match justhtml >= 1.19’s per-rule default-handling behavior, restoring expected href/src preservation on allowlisted tags.
Changes:
- Bump
justhtmlto1.22.0in production inputs and compiled requirement locks, and align dev to run tests against the same semantics. - Update
bedrock/base/sanitization.pyto explicitly setUrlRule(..., handling="allow")so allowlisteda[href]/img[src|srcset]keep scheme-validated URLs under the stricterUrlPolicydefaults. - Refresh compiled requirement locks (minor patch-level updates to other dependencies as part of the recompile).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| requirements/prod.txt | Refresh compiled prod lockfile (includes justhtml==1.22.0 and related patch bumps). |
| requirements/prod.in | Pin justhtml==1.22.0 in the prod input set. |
| requirements/docs.txt | Refresh compiled docs lockfile (notably click patch bump). |
| requirements/dev.txt | Refresh compiled dev lockfile to align with updated dependency set (includes justhtml==1.22.0). |
| bedrock/base/sanitization.py | Explicitly set UrlRule handling to preserve allowlisted URL attributes under newer justhtml behavior. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #17279 +/- ##
=======================================
Coverage 82.80% 82.80%
=======================================
Files 178 178
Lines 9408 9408
=======================================
Hits 7790 7790
Misses 1618 1618 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Looks like the integration tests failed :( |
- some HTML render "errors" were "fixed" and buttons were displaying wrong The HTML was wrong, but not having those classes rendered was what the tests and CSS were expecting.
|
Added a fix for the VPN pages (broken HTML was fixed and that... broke something AFAICT). |
| "version": "2.3.2", | ||
| "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.3.2.tgz", | ||
| "integrity": "sha512-xiqMQR4xAeHTuB9uWm+fFRcIOgKBMiOBP+eXiyT7jsgVCq1bkVygt00oASowB7EdtpOHaaPgKt812P9ab+DDKA==", | ||
| "dev": true, |
There was a problem hiding this comment.
@stephaniehobson is this addition intentional? I'll leave this PR for you to tweak and/or merge as you need
There was a problem hiding this comment.
I was doing my usual "push so I can see the diff in GitHub" workflow. Which works great when I'm not collaborating 🙈
But, no, it's not intentional and I see it now. 👍
There was a problem hiding this comment.
I don't see how it got there though, shouldn't that reflect the config in the requirements files? I didn't edit them.
| assert template == "products/monitor/waitlist/scan.html" | ||
|
|
||
|
|
||
| @override_settings(DEV=False, VPN_ENDPOINT="https://vpn.mozilla.org/") |
There was a problem hiding this comment.
@stevejalim What do you think of this added test? Good addition or too specific?
|
Tests are still failing 😫 |
|
I think the right thing to do is revert. |
Summary
Fixes integration test fails on VPN /thanks/ page auto-download, which broke silently when dependabot bumped justhtml==1.18.0 to 1.22.0 in prod.txt only (f054c29) - eg https://github.com/mozilla/bedrock/actions/runs/28828525342
Root cause
justhtml 1.19.0 hardened
UrlPolicy: aUrlRulewith no explicit handling now inheritsUrlPolicy.default_handlingper-rule. Bedrock's policy usesdefault_handling="strip"with allow-rules fora[href],img[src],img[srcset], sohref/src` attributes on allowlisted tags started getting silently stripped.On the VPN /thanks/ pages the download anchor lost its
href,thanks.jsread an empty value, and Playwright'swaitForEvent('download')timed out.XSS protection is unchanged .
Unit tests didn't catch it because
requirements/dev.txtstill pinnedjusthtml==1.18.0, so pytest ran against the old (pre-hardening) semantics.Test plan
./manage.py update_legal_docs --force) and eyeball for regressions (per the prod.in comment about manual QA on justhtml bumps).