Improve ListSerializer

[felipediel]

Description

https://github.com/encode/django-rest-framework/pull/8979 prevents list serializers from having data and instances with different lengths:

https://github.com/encode/django-rest-framework/pull/8979/files#diff-c33f1f011c9f5cf3ed1357b809ebf2b4ed0b808b227c6c007291bf9b259422ecR612-R617

This design choice breaks some use cases. Let's say we want the list serializer to delete from queryset all elements that are not provided in the data during an update. The queryset will be bigger than the data in this case:

# Example: serializer that will break after #8979

class BankBulkUpdateSerializer(serializers.ListSerializer):
    """Bank bulk update serializer."""

    def update(
        self,
        instance: models.QuerySet[Bank],
        validated_data: list[OrderedDict],
    ) -> list[Bank]:
        """Bulk update banks."""
        bank_dict = {bank.pk: bank for bank in instance}
        banks_to_create: list[Bank] = []
        banks_to_update: list[Bank] = []
        banks_created: list[Bank] = []
        banks_updated: list[Bank] = []
        bank_pks_to_keep: list[int] = []

        for bank_data in validated_data:
            bank_id = bank_data.get("id")
            bank = bank_dict.get(bank_id) if bank_id is not None else None

            if bank is not None:
                for attr, value in bank_data.items():
                    setattr(bank, attr, value)
                banks_to_update.append(bank)
                bank_pks_to_keep.append(bank.pk)

            else:
                bank = Bank(**bank_data)
                banks_to_create.append(bank)

        with db_transaction.atomic():
            instance.exclude(pk__in=bank_pks_to_keep).delete()

            if banks_to_update:
                update_fields = ["name", "compe", "ispb", "website"]
                Bank.objects.bulk_update(banks_to_update, update_fields)
                banks_updated = banks_to_update

            if banks_to_create:
                banks_created: list[Bank] = Bank.objects.bulk_create(
                    banks_to_create
                )

        return sorted(banks_created + banks_updated, key=lambda a: a.name)

The intention of the original PR is good, it solves the problem of not passing initial data and instances to child serializers correctly. But instead of indexing, it's better to relate the elements by id, so we can have different lengths. This PR addresses the issue.

refs https://github.com/encode/django-rest-framework/issues/8926 https://github.com/encode/django-rest-framework/pull/8979

[JoshBarr]

I agree we need to look at the way those scripts are attached – it makes the lines between frontend and backend a little blurry :) I'm hoping to take a look at it after the Torchbox team have published the 1.0 release.

[moggers87]

I started working on a patch for Wagtail to use nonces (as they're now supported in in django-csp) for inline scripts. However, I had to give up when I realised that Wagtail would still require 'unsafe-eval'.

[Pomax]

What is the cause of the unsafe-eval CSP violation, though? jQuery, or something else? (still seeing it today)

[moggers87]

https://github.com/wagtail/wagtail/search?l=JavaScript&q=eval&type=&utf8=%E2%9C%93

[rspeed]

I only see one relevant result in that search, and if I’m not mistaken it could be replaced with JSON.parse.

[Pomax]

That's a little dry of a response, @moggers87: CSP's unsafe-eval is not just about the Javascript eval() function, but about any JS that can result in the execution of arbitrary code. Obviously, eval() is the biggest offender here, but there are more cases that need to be checked. The full list of unsafe evaluating constructions, as far as the CSP spec for this keyword is concerned, comprises:

• eval(),
• new Function() (regardless of whether there is a string argument or not. CSP doesn't like it),
• any use of a blob: URL,
• any use of a filesystem: URL,
• setTimeout() with a "not-callable" execution argument (e.g. a string),
• setInterval()with a "not-callable" execution argument (e.g. a string).

The last two in particular are hilarious, almost no one remembers that they can even be used to effect eval behaviour in a way that doesn't even have a tracelog to follow.

[gasman]

With #4583 merged, we've hopefully eliminated all uses of eval - would be keen to hear from people implementing CSP about whether there are any other significant blockers remaining.

[robvdl]

Unfortunately I have found one more case that triggers unsafe eval and that appears to be modals.

I can trigger a CSP error by clicking the insert image or insert embed icon for example in the rich text editor on Chrome.

[gasman]

@robvdl Are you testing against current master? The issue with modals should have been fixed by #4583 (which will be in Wagtail 2.2).

[robvdl]

Ah I see thanks, yeah I was on 2.1.1

[konstin]

With wagtail 2.4 and a relatively strict csp (img-src 'self' data:; script-src 'self'; frame-ancestors 'self'; frame-src 'self'; form-action 'self'; default-src 'self'), you can't open modals. This means e.g. that you can't create a new site because "choose a root page" fails to open.

[SaptakS]

@gasman I still get a lot of unsafe-inline issues with CSP for the various form widgets in wagtail 2.7.4 . Adding hashes for each is difficult, since that would mean adding hashes everytime we add a new field/change field name/etc. Is there a better way to do it (now or in plans) instead of adding unsafe-inline in CSP rules?

[gasman]

@SaptakS The upcoming redevelopment of StreamField (https://wagtail.io/blog/telepath/, https://github.com/gasman/telepath-poc) is going to adopt an approach that avoids inline <script> tags, but we're a long way off eliminating them from Wagtail entirely - whenever form fields need Javascript behaviour, it's a fairly standard pattern in Wagtail code to implement the behaviour in an external JS file and then trigger it with a one-liner like <script> initSomeFormField('field-id'); </script>. There's an outline of how we could migrate away from those in https://github.com/gasman/weps/blob/inline-script-deprecation/text/033-inline-script-deprecation.md , but we currently have no resources allocated to doing that.

[ph00lt0]

This issue is still valid, I am quite shocked that his is opened in 2015 already, and yet not resolved. Using nonces would already be a good step towards fixing this. I would recommend to make use of the https://github.com/mozilla/django-csp. It would be great if this could be included in Wagtail fully. Wide use of Content Security Policy could prevent a lot of security problems in current web.

[thibaudcolas]

@ph00lt0 if you or anyone else wants to see this happen, we’ve been very welcoming of contributions in this area, all the way since 6 years ago. The reason it’s taken a while is that a lot of the Wagtail editing experience is built on those dynamic scripts that have proven hard to refactor, but we’re getting there bit by bit as outlined in the previous comments.

For starters it would really help if people needing this gave us precise reports of which widgets raise errors with which CSP rule, so we can identify which of these will be fixed by upcoming refactorings (https://github.com/wagtail/wagtail/issues/1288#issuecomment-733955699), and which won’t, and need further contributions.

[ph00lt0]

@thibaudcolas currently the following settings work fine with Wagtail back-end:

CSP_DEFAULT_SRC = 'none' CSP_SCRIPT_SRC = ["'self'", "'unsafe-inline'"] CSP_STYLE_SRC = ["'self'", "'unsafe-inline'"] CSP_FONT_SRC = ["'self'"] CSP_IMG_SRC = ["'self'"] CSP_FRAME_SRC = None CSP_OBJECT_SRC = ["'unsafe-eval'"] CSP_CONNECT_SRC = ["'self'"] CSP_PREFETCH_SRC = ["'self'"] CSP_MEDIA_SRC = ["'self'"]

I think these could be integrated already. What holds you back from using nonces for the inline scripts? Alternatively hashes could be added to the settings. CSP supports sha256, sha384 and sha512. (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src)

Some things that are broken when only allowing self script settings:

• content.js, utils.js, admin.js, vendor.js and modernizr all won't load.
• Adding streamfields is broken by not loading the required scripts
• The image panel
• SVG icons in navigation.

Inline css is used for the wagtail-userbar's stylesheet.

Also Gravatar won't load. But honestly I would prefer being able to turn that off completely by default.

[thibaudcolas]

👍 thanks for sharing those settings. I don’t think we’re held back from using nonces. Based on the list above we’ve preferred refactoring away from inline scripts (and eval, historically) so far, but I could see us preferring nonces for some areas of the admin. I think it’s a decision that would have to be made point by point.

content.js, utils.js, admin.js, vendor.js and modernizr all won't load.

All of these are same-origin scripts so should load fine with script-src: self? Or am I missing something?

Inline css is used for the wagtail-userbar's stylesheet.

Do you have more info about this? I can’t see any inline styles inspecting a site running Wagtail v2.10, nor in the current source on main.

Also Gravatar won't load. But honestly I would prefer being able to turn that off completely by default.

You should be able to turn it off with:

WAGTAIL_GRAVATAR_PROVIDER_URL = None

[hazho]

@moggers87

I started working on a patch for Wagtail to use nonces (as they're now supported in in django-csp) for inline scripts. However, I had to give up when I realised that Wagtail would still require 'unsafe-eval'.

The code is can be found on this branch if anyone is interested.

your link is broken, kindly update it

[moggers87]

@hazho I deleted my fork, that code no longer exists. It didn't work anyway (hence me giving up on it) and would now be 4 years out of date. I would suggest reading @gasman 's last comment for an update on the current status of this issue.

[zerolab]

Related: A list of widgets breaking strict Content-Security-Policy (CSP) directives #7053

[thibaudcolas]

I’d like us to proceed with CSP compatibility as part of the page editor 2022 project. I think an important step would be for us to have a simple way to test CSP compatibility during local development, for example by setting it up for bakerydemo. Ideally enforcing a baseline compatible CSP at all times, with optional settings to try out compatibility with stricter CSPs.

How does that sound? Is anyone here interested in taking this on?

[thibaudcolas]

For people interested in this – we are reviewing our approach to JS dependencies, and looking for a new framework. Feedback very welcome – CSP support is an explicit goal.

[ChesterFourDigits]

Has there been an update regarding this issue? I couldn't find a reference in the page editor project.

[lb-]

Has there been an update regarding this issue? I couldn't find a reference in the page editor project.

I am not aware of a specific update, beyond what is already linked above.

At the moment, the 3.0 release does not contain too many JS changes, but there have been smaller tweaks made to how we load JS, including in some templates.

See the latest 3.0 changes - https://github.com/wagtail/wagtail/discussions/7739#discussioncomment-2615089 (pertaining to page editor).

If you have some time, it would be greatly appreciated if you could review the lightweight framework research here https://github.com/wagtail/wagtail/discussions/7689#discussioncomment-2037913 and give any feedback.

Additionally, if you have any input you can add it to the related RFC here https://github.com/wagtail/rfcs/pull/33

[lb-]

RFC 78 created https://github.com/wagtail/rfcs/pull/78 - with the goal of giving us a practical solution to widget scripts (amongst other things).

[dgabrahams]

In the process of adding CSP to a site I originally used CSP_EXCLUDE_URL_PREFIXES to exclude /admin/ but this was not an accepted solution, django-csp documentation stated the following about exclusions:

Excluding any path on your site will eliminate the benefits of CSP everywhere on your site. The typical browser security model for JavaScript considers all paths alike. A Cross-Site Scripting flaw on, e.g., excluded-page/ can therefore be leveraged to access everything on the same origin.

After discussion on the Wagtail support channel it was advised that "a weak CSP implemented everywhere is better than no CSP (similarly, a strong CSP over everything but the admin is also better than nothing)." Taking this into account I decided to implement a weak CSP policy for admin using middleware. Please see an example below for reference:

class CSPOverrideMiddleware(MiddlewareMixin):
    """
    Custom middleware to override Content Security Policy directives for the admin interface.
    Add to MIDDLEWARE list after official django-csp CSPMiddleware.
    """

    def process_request(self, request):
        if request.path_info.startswith("/admin"):
            # Remove nonce value otherwise 'unsafe-inline' will not work.
            request.csp_nonce = None

    def process_response(self, request, response):
        if request.path_info.startswith("/admin"):
            # Add CSP sources to enable admin pages to work.
            response._csp_update = {
                "script-src": "'unsafe-inline'",
                "style-src": "'unsafe-inline'",
                "object-src": "'unsafe-eval'",
                "img-src": "http://www.gravatar.com",
                "connect-src": "https://releases.wagtail.org",
            }

        return response

I also had to override wagtailadmin/userbar/base.html to get the admin icon to show bottom right corner when logged in and viewing the front end. Please see example below:

{% extends "wagtailadmin/userbar/base.html" %}

{# Override of branding block to remove inline style for Content Security Policy #}
{# To be removed when Wagtail Admin is CSP compliant #}
{% block branding_logo %}
    <div class="icon--hide">
        <svg>
            <defs>
                {% include "wagtailadmin/icons/wagtail.svg" %}
                {% include "wagtailadmin/icons/folder-open-inverse.svg" %}
                {% include "wagtailadmin/icons/edit.svg" %}
                {% include "wagtailadmin/icons/plus.svg" %}
                {% include "wagtailadmin/icons/tick.svg" %}
                {% include "wagtailadmin/icons/cross.svg" %}
            </defs>
        </svg>
    </div>
    {% comment %} Intentionally not using the icon template tag to show as SVG only {% endcomment %}
    <svg class="wagtail-userbar-icon" aria-hidden="true">
        <use href="#icon-wagtail-icon"></use>
    </svg>
{% endblock %}

Still in the process of testing but adding here to share the general approach. Feedback/comments welcome. Thanks!

[lb-]

General update

• There is agreement that supporting CSP is a goal, it will take some time though. If anyone is keen to contribute to this process - add a comment in this thread.
• RFC 78 will be a key part of this
• I also think that maybe we should update all our documentation snippets that suggest inline scripts to maybe link to an external script / maybe even module syntax. Or adopt the nonce usage in all examples if that's easier.
• https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script#attr-nonce
• I don't think much work has started on inline styles though - this may also be difficult with some of the jQuery UI plugins. However, it's probably a lower priority compared to JS inline and any eval usage.

[jkevingutierrez]

I'm having another CSP issue with wagtailuserbar.

wagtailuserbar is blocked by CSP when we use strict-dynamic.

It would be great to have the ability to add a nonce to the scripts/styles.

For example, the one in https://github.com/wagtail/wagtail/blob/main/wagtail/admin/templates/wagtailadmin/userbar/base.html#L42

Could be something like

<script src="{% versioned_static 'wagtailadmin/js/userbar.js' %}" nonce="{{ CUSTOM_NONCE }}"></script>

[thibaudcolas]

@jkevingutierrez is what you’re describing actually specific to the userbar? From what I understand of your example, the userbar is blocked because you’ve not added http://localhost:8000/ to the list of sources, not because of the strict-dynamic usage.

Adding nonces would certainly be nice but for the time being I’d expect we’ll still focus on removing inline scripts as the first order of business. strict-dynamic seems to me like a nice to have if you don’t want to manage origin lists, and in the case of Wagtail there are only a very limited set anyway (the ones the site is hosted from, and https://releases.wagtail.org for upgrade notifications).

[jkevingutierrez]

Hey @thibaudcolas,

Thanks for your help.

Correct, it is specific to the userbar as that's the only script that we can't add a nonce right now.

We started to use strict-dynamic not as a nice to have, but for security reasons after checking our site with https://csp-evaluator.appspot.com/:

Host whitelists can frequently be bypassed. Consider using 'strict-dynamic' in combination with CSP nonces or hashes.

And thing is that when you add strict-dynamic, you can't add sources to the CSP whitelist (they won't have any effect), so, you need to use nonce:

Note that 'strict-dynamic' is present, so host-based allowlisting is disabled.

According to csplite.com server with CSP header script-src 'self' 'strict-dynamic' 'nonce-{randomString}' set...

'strict-dynamic' only works in conjunction with 'hash-value' or 'nonce-value', and if they are absent, all scripts on the page will be disabled, because 'strict-dynamic' overrides the 'unsafe-inline' action and disables whitelisting of host-based sources, including the 'self' token