-
-
Notifications
You must be signed in to change notification settings - Fork 33.3k
Fixed #36549 -- Doc'd use of OpenLayersWidget and OSMWidget with CSP. #19765
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
Conversation
nessita
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the proactive work on updating OpenLayers! 🌟 🏆 🏅
I completely understand the goal and the idea behind vendoring, and I appreciate the effort to remove external dependencies as we do with Select2.
That said, I have some reservations about vendoring in this case. There is a subtle but important difference from Select2: OpenLayers is only used if the gis app is installed and specific geo widgets are in use, while Select2 is part of the basic admin setup (for example, for filtering a User's groups and permissions). Recent Django Developer Surveys show that around 70% of users do not use any GeoDjango backend.
Additionally, vendoring can introduce issues such as:
- Difficulty keeping the vendored version in sync with upstream releases.
- Increased size of Django releases for a domain-specific feature.
- Extra complexity for maintainers when updating or patching the files.
Laslty, and as you clearly pointed out, even with vendoring the CSP issue is not fully avoided, so we still need to warn about it in the docs.
Given these points, I think the better approach is not to vendor OpenLayers, but instead to rely on the external URLs while keeping the docs updated with the specific CSP requirements for these widgets. This keeps the setup simpler, reduces maintenance burden, and clearly informs developers what needs to be configured in SECURE_CSP when using geo widgets.
Let me know if you are available to make these adjustments, otherwise I can do those myself early next week.
|
Hi @nessita, thanks for the feedback. I'll try to update this over the weekend. |
|
Hi @nessita I'm afraid this is as far as I have managed to progress this. I was wondering if we could not pin the version numbers, but I couldn't seem to get that to work. 🤔 Hope even what is here is helpful. |
Thank you @smithdc1, this is definitely helpful! I'll progress it to the finish line. Side question, do you think is worth updating the OpenLayers libs to 10.6.0 as initially proposed? I can work on that. What would be really useful for me is if you could share the models/admin that you use to test/exercise these bits. I have some but I think they are too basic. |
The OpenLayersWidget and OSMWidget load map tile images from NASA and OpenStreetMap respectively. If CSP is enabled, apporpriate CSP directives need to be added to allow those resources to be loaded.
3fcfbf3 to
8763548
Compare
nessita
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @smithdc1! I'll merge this to have the release blocker fixed and we can potentially do a separated PR with the openlayers version update.
|
Thanks@nessita. I'll come back to respond to your other points about updating the version when I get some time. (I'd like to share the test project with data, but that's currently in postgres, so a bit of thinking to do. It is just the world borders tutorial, though) |
Trac ticket number
ticket-36549
Branch description
The discussion on the ticket and related forum thread discussed that addional CSP rules are required for the
OpenLayersWidgetresources to be loaded. Discussion also went on to say that the current version is somewhat outdated. This patch therefore, covers a few things:.cssand.jsfiles. These can then be served by the django admin in the same way as Select2 and so on and means we can avoid neededing to add"script-src"and"style-src"rules forhttps://cdn.jsdelivr.net/.... I sourced the vendored files from https://github.com/openlayers/openlayers/releases/tag/v10.6.0.jsfile to add a version to that file. Hopefully that will make it clearer in future which version is currently being used. This is similar to the note that's at the top of theselect2.full.min.jsfile.img-srcdirective is required.Checklist
mainbranch.