[GrandComicsDatabase/gcd-django] Handle Cloudflare challenges in editing forms (PR #734)
jhunterjActual
Summary
Cloudflare may return an interactive Challenge Page to an AJAX autocomplete request when a user's verification expires. Select2 expects JSON from these requests, so it previously displayed only its generic failure message.
This change:
- detects Cloudflare Challenge Page responses through the documented
cf-mitigated: challengeresponse header; - displays a visible verification notice instead of relying solely on the generic autocomplete error;
- opens the challenged request in a separate tab so the original form and its unsaved data remain intact;
- restricts the verification link to same-origin request URLs;
- keeps the notice styling in the external stylesheet;
- includes JavaScript files in Tailwind's content scan; and
- removes the obsolete
msDropDowninitialization and stylesheet references left behind after the old brand field was replaced by the brand-emblem autocomplete.
Testing
Tested locally using the Docker development environment.
- Simulated a
cf-mitigated: challengeAJAX completion. - Confirmed that the verification notice appears.
- Confirmed that the verification link opens in a new tab.
- Confirmed that unsaved form contents remain in the original tab.
- Confirmed that repeated challenge responses do not create duplicate notices.
- Confirmed that normal autocomplete controls continue to work.
- Confirmed that the obsolete
msDropDownconsole exception is gone. - JavaScript syntax checks and
git diff --checkpass.
You can view, comment on, or merge this pull request online at:
https://github.com/GrandComicsDatabase/gcd-django/pull/734
Commit Summary
- 4ecc1c1 Handle Cloudflare challenges in editing forms
File Changes
(9 files)
- M static/css/input.css (21)
- M static/css/output.css (22)
- A static/js/cloudflare_challenge.js (70)
- M static/js/oi/issuerevision_form_utils.js (3)
- M tailwind.config.js (2)
- M templates/oi/bits/jquery.html (3)
- M templates/oi/edit/add_frame.html (6)
- M templates/oi/edit/bulk_frame.html (6)
- M templates/oi/edit/revision.html (6)
Patch Links:
- https://github.com/GrandComicsDatabase/gcd-django/pull/734.patch
- https://github.com/GrandComicsDatabase/gcd-django/pull/734.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
gemini-code-assist[bot]
@gemini-code-assist[bot] commented on this pull request.
Code Review
This pull request introduces a mechanism to detect Cloudflare Challenge Pages during jQuery AJAX calls and display a verification notice banner, while also removing unused msDropDown dropdown styling and logic. The feedback recommends adding defensive checks in the JavaScript file to handle undefined or non-string request URLs and missing AJAX response headers. Additionally, it is suggested to refactor the dynamically generated HTML notice banner into a Django template for better maintainability and internationalization, which would also allow reverting the Tailwind configuration change that scans JavaScript files to avoid CSS bloat from minified libraries.
In static/js/cloudflare_challenge.js:
> + function challengeUrl(requestUrl) {
+ try {
+ const url = new URL(requestUrl, window.location.href);
+ // Do not open a third-party URL supplied by an AJAX request.
+ if (url.origin === window.location.origin) {
+ return url.href;
+ }
+ } catch (error) {
+ // Use the current page when the request URL cannot be parsed.
+ }
+ return window.location.href;
+ }
If requestUrl is undefined or not a string, new URL(undefined, window.location.href) does not throw an error in modern JS engines. Instead, it stringifies undefined to "undefined" and resolves it as a relative path (e.g., http://localhost/undefined). Since the origin matches, this function will return the invalid URL /undefined instead of falling back to window.location.href.
To prevent this, add a guard check to ensure requestUrl is a valid, non-empty string before parsing.
function challengeUrl(requestUrl) { if (!requestUrl || typeof requestUrl !== 'string') { return window.location.href; } try { const url = new URL(requestUrl, window.location.href); // Do not open a third-party URL supplied by an AJAX request. if (url.origin === window.location.origin) { return url.href; } } catch (error) { // Use the current page when the request URL cannot be parsed. } return window.location.href; }
In static/js/cloudflare_challenge.js:
> + function showChallengeNotice(requestUrl) {
+ let notice = document.getElementById(bannerId);
+ if (notice) {
+ notice.querySelector('a').href = challengeUrl(requestUrl);
+ return;
+ }
+
+ notice = document.createElement('div');
+ notice.id = bannerId;
+ notice.className = 'cloudflare_challenge_notice';
+ notice.setAttribute('role', 'alert');
+
+ const message = document.createElement('span');
+ message.textContent = 'Cloudflare verification is required. ';
+ notice.appendChild(message);
+
+ const verifyLink = document.createElement('a');
+ verifyLink.href = challengeUrl(requestUrl);
+ verifyLink.target = '_blank';
+ verifyLink.rel = 'noopener';
+ verifyLink.textContent = 'Verify in a new tab';
+ notice.appendChild(verifyLink);
+
+ const instructions = document.createElement('span');
+ instructions.textContent = ', close that tab, then retry this field. ' +
+ 'Your unsaved changes will remain here.';
+ notice.appendChild(instructions);
+
+ document.body.appendChild(notice);
+ }
Building complex HTML structures dynamically in JavaScript is verbose, harder to maintain, and makes internationalization (i18n) of user-facing strings difficult.
A cleaner approach is to render the notice markup directly in the Django template (e.g., templates/oi/bits/jquery.html) with Django's standard translation tags ({% translate %}), and keep it hidden by default using Tailwind's .hidden class. Then, the JavaScript only needs to update the link's href and remove the hidden class when a challenge is detected.
function showChallengeNotice(requestUrl) { const notice = document.getElementById(bannerId); if (notice) { const link = notice.querySelector('a'); if (link) { link.href = challengeUrl(requestUrl); } notice.classList.remove('hidden'); } }
In static/js/cloudflare_challenge.js:
> + $(document).ajaxComplete(function(event, xhr, settings) {
+ // Cloudflare sets this header on every Challenge Page response.
+ const mitigation = xhr.getResponseHeader('cf-mitigated');
+ if (mitigation && mitigation.toLowerCase() === 'challenge') {
+ showChallengeNotice(settings.url);
+ }
+ });
Add defensive checks to ensure xhr is defined and has the getResponseHeader method before calling it, and safely access settings.url to prevent potential runtime errors.
$(document).ajaxComplete(function(event, xhr, settings) { if (!xhr || typeof xhr.getResponseHeader !== 'function') { return; } // Cloudflare sets this header on every Challenge Page response. const mitigation = xhr.getResponseHeader('cf-mitigated'); if (mitigation && mitigation.toLowerCase() === 'challenge') { const url = settings && settings.url; showChallengeNotice(url); } });
> content: ['./templates/*.html', './templates/**/*.html',
'./templates/**/**/*.html',
+ // Include class names assigned by browser-side scripts.
+ './static/js/*.js', './static/js/**/*.js',
'./apps/indexer/templates/indexer/*.html',
'./apps/indexer/templates/indexer/bits/*.html',
'./apps/voting/templates/voting/*.html',
Scanning all JavaScript files (especially with a wildcard like **/*.js) can cause Tailwind to scan large minified third-party libraries (such as htmx.min.js), leading to significant CSS bloat from false-positive utility class matches in minified code.
If we adopt the suggested refactoring of rendering the notice markup directly in the HTML template (e.g., templates/oi/bits/jquery.html), Tailwind will automatically detect the cloudflare_challenge_notice class from the HTML files (which are already scanned). This completely eliminates the need to scan any JS files, allowing you to revert this change to tailwind.config.js entirely.
- content: ['./templates/*.html', './templates/**/*.html', - './templates/**/**/*.html', - // Include class names assigned by browser-side scripts. - './static/js/*.js', './static/js/**/*.js', - './apps/indexer/templates/indexer/*.html', - './apps/indexer/templates/indexer/bits/*.html', - './apps/voting/templates/voting/*.html', + content: ['./templates/*.html', './templates/**/*.html', + './templates/**/**/*.html', + './apps/indexer/templates/indexer/*.html', + './apps/indexer/templates/indexer/bits/*.html', + './apps/voting/templates/voting/*.html',
In templates/oi/bits/jquery.html:
> +{# Detect Cloudflare challenges returned to AJAX form controls. #}
+<script type="text/javascript"
+ src="{% static 'js/cloudflare_challenge.js' %}"></script>
To support the suggested refactoring (rendering the notice in HTML for better maintainability and i18n), define the notice markup directly in this template. It will be hidden by default using Tailwind's .hidden class, and the JavaScript will toggle its visibility when needed.
{# Detect Cloudflare challenges returned to AJAX form controls. #}
{% load i18n %}
<div id="cloudflare_challenge_notice" class="cloudflare_challenge_notice hidden" role="alert">
<span>{% translate "Cloudflare verification is required. " %}</span>
<a href="#" target="_blank" rel="noopener">{% translate "Verify in a new tab" %}</a>
<span>{% translate ", close that tab, then retry this field. Your unsaved changes will remain here." %}</span>
</div>
<script type="text/javascript"
src="{% static 'js/cloudflare_challenge.js' %}"></script>
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
jhunterjActual
@jhunterjActual pushed 1 commit.
- da8162a Address review feedback and use the base site for the Cloudflare challenge URL instead of the autocomplete
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
jhunterjActual
@jhunterjActual commented on this pull request.
In static/js/cloudflare_challenge.js:
> + function challengeUrl(requestUrl) {
+ try {
+ const url = new URL(requestUrl, window.location.href);
+ // Do not open a third-party URL supplied by an AJAX request.
+ if (url.origin === window.location.origin) {
+ return url.href;
+ }
+ } catch (error) {
+ // Use the current page when the request URL cannot be parsed.
+ }
+ return window.location.href;
+ }
The implementation no longer constructs the verification destination from the AJAX request URL. The failed request will usually be an autocomplete endpoint, and opening that endpoint directly can leave the user on an error page after Cloudflare verification.
The verification link now has href="/" in the Django template. Therefore, an undefined, empty, or malformed requestUrl cannot produce /undefined or another unintended destination. The user verifies through the site root and can then close that tab and retry the autocomplete control without losing the unsaved form in the original tab.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
jhunterjActual
> + function showChallengeNotice(requestUrl) {
+ let notice = document.getElementById(bannerId);
+ if (notice) {
+ notice.querySelector('a').href = challengeUrl(requestUrl);
+ return;
+ }
+
+ notice = document.createElement('div');
+ notice.id = bannerId;
+ notice.className = 'cloudflare_challenge_notice';
+ notice.setAttribute('role', 'alert');
+
+ const message = document.createElement('span');
+ message.textContent = 'Cloudflare verification is required. ';
+ notice.appendChild(message);
+
+ const verifyLink = document.createElement('a');
+ verifyLink.href = challengeUrl(requestUrl);
+ verifyLink.target = '_blank';
+ verifyLink.rel = 'noopener';
+ verifyLink.textContent = 'Verify in a new tab';
+ notice.appendChild(verifyLink);
+
+ const instructions = document.createElement('span');
+ instructions.textContent = ', close that tab, then retry this field. ' +
+ 'Your unsaved changes will remain here.';
+ notice.appendChild(instructions);
+
+ document.body.appendChild(notice);
+ }
I moved the complete notice markup into the Django template. The JavaScript no longer constructs the elements or contains user-facing strings. It only finds the existing notice and removes the hidden class when a Cloudflare Challenge Page is detected.
The message uses Django's {% blocktranslate trimmed %} support, so the complete sentence is available for translation as one unit rather than as several disconnected JavaScript strings.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
jhunterjActual
> + $(document).ajaxComplete(function(event, xhr, settings) {
+ // Cloudflare sets this header on every Challenge Page response.
+ const mitigation = xhr.getResponseHeader('cf-mitigated');
+ if (mitigation && mitigation.toLowerCase() === 'challenge') {
+ showChallengeNotice(settings.url);
+ }
+ });
The AJAX completion handler now verifies that xhr exists and that xhr.getResponseHeader is a function before reading the cf-mitigated header.
The handler no longer reads settings.url at all because the verification destination is always /. Consequently, a missing settings object or URL cannot cause a runtime error. The notice is displayed only when the cf-mitigated header is present and its value is challenge.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
jhunterjActual
@jhunterjActual commented on this pull request.
> content: ['./templates/*.html', './templates/**/*.html',
'./templates/**/**/*.html',
+ // Include class names assigned by browser-side scripts.
+ './static/js/*.js', './static/js/**/*.js',
'./apps/indexer/templates/indexer/*.html',
'./apps/indexer/templates/indexer/bits/*.html',
'./apps/voting/templates/voting/*.html',
I removed the ./static/js/*.js and ./static/js/**/*.js entries from tailwind.config.js. This prevents Tailwind from scanning minified third-party libraries and avoids the possible false matches and CSS bloat identified.
The notice classes now appear in a Django template, so the existing HTML template patterns in the Tailwind configuration discover them without scanning JavaScript.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
jhunterjActual
@jhunterjActual commented on this pull request.
In templates/oi/bits/jquery.html:
> +{# Detect Cloudflare challenges returned to AJAX form controls. #}
+<script type="text/javascript"
+ src="{% static 'js/cloudflare_challenge.js' %}"></script>
I placed the notice markup in templates/oi/base_view.html rather than templates/oi/bits/jquery.html. The jQuery partial can be included more than once on some revision pages, so placing the markup there could create multiple elements with the same cloudflare_challenge_notice ID.
base_view.html renders the notice once per OI page, initially applies the hidden class, includes role="alert", and supplies the root verification link. The JavaScript remains loaded through the jQuery partial and retains its global installation guard so repeated inclusion cannot register duplicate AJAX handlers.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
jhunterjActual
jhunterjActual left a comment (GrandComicsDatabase/gcd-django#734)I validated the changes with a JavaScript syntax check, Django template parsing, manage.py check, git diff --check, a Docker rebuild, and a browser-side simulated Cloudflare challenge. The simulation now reveals one notice, produces a link to /, and does not depend on the AJAX request URL.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JochenGCD
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.