Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log when form is using file fields without proper enctype attribute on the form. #1682

Closed
tim-schilling opened this issue Oct 8, 2022 · 5 comments · Fixed by #1933
Closed

Comments

@tim-schilling
Copy link
Contributor

One of the issues that can trip up new developers is the fact that forms require enctype="multipart/form-data" when there's a file being uploaded. If a developer doesn't know this, it can be hard to track down. I think it'd be reasonable to have the toolbar inspect the loaded HTML content and inspect it for invalid configurations. The first of which being a <input type="file" ..> /> in a form that isn't defined as <form enctype="multipart/form-data">.

bkdekoning added a commit to bkdekoning/django-debug-toolbar that referenced this issue Jun 3, 2024
bkdekoning added a commit to bkdekoning/django-debug-toolbar that referenced this issue Jun 3, 2024
@bkdekoning
Copy link
Contributor

Hi all,

I've taken a shot at the issue and am curious to hear what you think about my approach.

Since there is no warnings/alerts panel yet (as discussed in issue #1845), I figured the Templates panel would be the most sensible place to implement is.

What I've done is subclass Python's HTML parser and added a function called check_invalid_file_form_configuration in panels/templates/panel.py. The function checks all forms for a file input type and, if it exists, checks if the form has encoding type multipart/form-data. If it does not, it warns the user in the Templates panel by passing the error message to record_stats. I've added two tests and checked that the test suite passes for all compatible versions of Django and Python through Tox.

In lieu of having a dedicated warnings/alerts panel, I am wondering whether it would be helpful to add a little notification flag (perhaps in nav_title, or nav_subtitle) to indicate that something is up.

Happy to hear any and all thoughts!

@matthiask
Copy link
Member

matthiask commented Jun 6, 2024

In lieu of having a dedicated warnings/alerts panel, I am wondering whether it would be helpful to add a little notification flag (perhaps in nav_title, or nav_subtitle) to indicate that something is up.

Yeah, I think we definitely would need some sort of notification for things like these. I would certainly miss the alert if it was buried somewhere in a panel.

I have briefly looked at the code and think that it makes sense, however I'd certainly want Tim to chime in as well. Feel free to open a pull request without prior discussion, then we can let GitHub run the testsuite and maybe also add some review comments if necessary, directly on the PR!

bkdekoning added a commit to bkdekoning/django-debug-toolbar that referenced this issue Jun 6, 2024
bkdekoning added a commit to bkdekoning/django-debug-toolbar that referenced this issue Jun 6, 2024
@bkdekoning bkdekoning mentioned this issue Jun 6, 2024
2 tasks
@tim-schilling
Copy link
Contributor Author

I'd be more in favor of putting this on a separate panel rather than trying to make room for it on the template or request panel. The reason being is that we can't associate it with a particular template in every case. And it's not really a request panel issue. I think that means it should be a new panel.

I like the idea of a notification on the toolbar itself somehow.

I did a little more searching and TIL that you can set the form's encoding type on the submit input: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#formenctype That's probably something we should cover to avoid false positives if we're actively alerting people.

@bkdekoning
Copy link
Contributor

That makes sense. I can create an alerts panel as part of this issue, keeping in mind that it needs to be able to accommodate issues such as #1435 and #1845.

Thanks for the heads-up about setting the form's encoding type on the submit input. I'll make sure to include that in the checks and see if there are any other gotchas.

bkdekoning added a commit to bkdekoning/django-debug-toolbar that referenced this issue Jun 7, 2024
@tim-schilling
Copy link
Contributor Author

Eesh, another way to associate a form and input: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Input#form

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants