Admin foreign key widgets don't quote keys.

[Joshua Goodwin]

In django.contrib.admin.ModelAdmin, _changeform_view, _delete_view and history_view and do unquote(object_id):

​https://github.com/django/django/blob/917fd9d03fdd21538864af4b412ac30b36d99268/django/contrib/admin/options.py#L1537

However, ForeignKeyRawIdWidget and RelatedFieldWidgetWrapper create links to this view without calling quote():

​https://github.com/django/django/blob/89a2216486fa8a0513cbb1d49d2d587d4116c60b/django/contrib/admin/widgets.py#L191

Steps to reproduce:

1. Create two models: Topping with a CharField primary key, and Pizza with a ForeignKey to Topping. Register both models with the admin site.
2. Create a Topping with the primary key '_40', and a Pizza with that topping
3. In the admin site, go the the /change/ page for the Pizza, and click on the 'change' icon for the Topping foreign key, or (if using ForeignKeyRawIdWidget) the link to the Topping '_40'.
4. See message 'Topping with ID "@" doesn't exist. Perhaps it was deleted?'.

[Mariusz Felisiak]

Reproduced at 25b5eea8cdc69a353bb2d22ea2012b09df6c62e4.

[zeynel]

I believe there are two main parts to handle to fix this bug.

• Quoting obj.pk value on ​https://github.com/django/django/blob/89a2216486fa8a0513cbb1d49d2d587d4116c60b/django/contrib/admin/widgets.py#L191
• Quoting object pk(or sending quoted values in context) on client side when user changes the selection on RelatedFieldWidget ​https://github.com/django/django/blob/89a2216486fa8a0513cbb1d49d2d587d4116c60b/django/contrib/admin/static/admin/js/admin/RelatedObjectLookups.js#L69

[Mariusz Felisiak]

I still have some issue when saving models, please check attached project and try to add a new pizza with e.g. _40 topping on admin/test_one/pizza/add/.

[Alexander Pervakov]

I'm dive deeper into this and found that quote(obj.id) maybe not the best option and should be debated but the presence of a warning message when not isinstance(obj.pk, int) would be great and will lead people to this thread. Because stackoverflow is full of questions about quoting the link in admin but no one provide refs to this ticket that I found after couple hours of code investigating and testing.

Something like this in django.db.models.fields.RelatedField:

class RelatedField(FieldCacheMixin, Field):

    def check(self, **kwargs):
        return [
            # ...
            *self._check_pk_int(),
        ]

    def _check_pk_int(self):
        remote_pk = getattr(self.remote_field, 'pk', None)
        if isinstance(remote_pk, int):
            return []
        else:
            return [
                checks.Warning(
                    'Not integer primary key may lead you to unpredictable url in admin when ForeignKeyRawIdWidget is used',
                    hint='Use extra quote() in this relation',
                    obj=self,
                    id='fields.W346',
                )
            ]

[Yhemisi]

Replying to Mariusz Felisiak:

I still have some issue when saving models, please check attached project and try to add a new pizza with e.g. _40 topping on admin/test_one/pizza/add/.

@Mariusz Felisiak I have checked the attached project and also tried to add a new pizza which I was able to save models. Could you please confirm if you are unable to save.

[Yhemisi]

As regards to this ticket, I worked on it following this steps

class House(models.Model):
    name = models.CharField(max_length=255, primary_key=True)
    def __str__(self):
        return self.name

class Room(models.Model):
    house = models.ForeignKey(House, on_delete=models.CASCADE)
    def __str__(self):
        return self.house.name

using the quote function from django.contrib.admin.utils should fix this

 def test_foreign_key_raw_id_widget_renders_quoted_pk_in_change_url(self):
        house = House.objects.create(name='?a=b')
        rel = Room._meta.get_field('house').remote_field
        w = widgets.ForeignKeyRawIdWidget(rel, widget_admin_site)

        # apply quote function to primary key value
        pk_quoted = quote(str(house.pk))

        # render the widget
        rendered = w.render('test', house.pk, attrs={})

        # check that the primary key is properly quoted in the rendered HTML
        self.assertIn(f'href="/admin_widgets/house/{pk_quoted}/change/"', rendered)

[Mariusz Felisiak]

Replying to Yhemisi:

Replying to Mariusz Felisiak:

I still have some issue when saving models, please check attached project and try to add a new pizza with e.g. _40 topping on admin/test_one/pizza/add/.

@Mariusz Felisiak I have checked the attached project and also tried to add a new pizza which I was able to save models. Could you please confirm if you are unable to save.

The issue is that you will not be able to add a new pizza with the proposed patch.

[Yhemisi]

Does it mean the quote function isn't a better way of solving this?

[Carlton Gibson]

The issue is that you will not be able to add a new pizza with the proposed patch.

OK, I'm struggling to reproduce this now.

Even at 25b5eea8cdc69a353bb2d22ea2012b09df6c62e4 — which was the reproduce commit above, with the test project, in Firefox, Edge and Safari, I'm able to create Pizzas with the Topping _40 without error. I can't work out why I'm not seeing this. (Like, did browsers change? 🤔) (

The particular tests from the PRs checking the quoting fail — but pausing at those points — there's no error saving — i.e. the key isn't quoted but its working (AFAICS 🤔)

@Yhemisi: given that you're working on this, can you adapt the Selenium test from the original PR to visit the add a Pizza page and create a new instance with a topping with e.g. _40 PK? Does that pass on main with no other changes?

[Carlton Gibson]

This added on top of the model changes from the ​original PR already passes:

# In class RelatedFieldWidgetSeleniumTests...

    def test_pizzas_with_topping_can_be_created(self):
        from selenium.webdriver.common.by import By

        self.admin_login(username="super", password="secret", login_url="/")
        self.selenium.get(
            self.live_server_url + reverse("admin:admin_widgets_pizza_add")
        )
        self.select_option("#id_topping", self.toppings[0].pk)
        name_field = self.selenium.find_element(By.ID, "id_name")
        name_field.send_keys("testing pizza")
        save_button_css_selector = ".submit-row > input[type=submit]"
        self.selenium.find_element(By.CSS_SELECTOR, save_button_css_selector).click()
        self.wait_for_text(
            "li.success", "The pizza “testing pizza” was added successfully."
        )

[Mariusz Felisiak]

I can still reproduce the issue with save when PR 11280 is applied, see issue_30386_with_patch_and_save.mp4.

[Mariusz Felisiak]

With the new ​PR I can reproduce the original issue, see issue_30386.mp4

[Carlton Gibson]

Yes, good — thanks for the clarification — got it now. 👍 Thanks!

It looks like the quoting needs to occur in RelatedObjectLookups.js updateRelatedObjectLinks() as per the second part of comment:2

@Yhemisi: does that give you enough pointers to work on?

[Yhemisi]

Replying to Carlton Gibson:

Yes, good — thanks for the clarification — got it now. 👍 Thanks!

It looks like the quoting needs to occur in RelatedObjectLookups.js updateRelatedObjectLinks() as per the second part of comment:2

@Yhemisi: does that give you enough pointers to work on?

Yes It does. Thank you. I will look it up

[Yhemisi]

Replying to Mariusz Felisiak:

With the new ​PR I can reproduce the original issue, see issue_30386.mp4

@Mariusz Felisiak please i want to ask these questions below to be sure that i understand the problem.

1. Are you trying to say that the issue is not "Admin foreign key widgets don't quote keys" but the issue is that one will not be able to add a new pizza ?

2. Is that the inability to add a new pizza is caused by the issue of admin foreign key widgets not quoting keys with special characters
3. Are you trying to say that both issues exist differently and the solution doesn't affect each other?

[Mariusz Felisiak]

1. Are you trying to say that the issue is not "Admin foreign key widgets don't quote keys" but the issue is that one will not be able to add a new pizza ?

No. There is an original issue from the ticket description, that you can see on the attached record issue_30386.mp4. Your patch doesn't fix it.

2. Is that the inability to add a new pizza is caused by the issue of admin foreign key widgets not quoting keys with special characters

No. It's a side-effect of ​patch proposed by Zeynel, so Zeynel's patch fixes the original issue (see issue_30386.mp4) but introduces a regression (see issue_30386_with_patch_and_save.mp4​). That's why we couldn't accept it.

[Yhemisi]

Replying to Mariusz Felisiak:

1. Are you trying to say that the issue is not "Admin foreign key widgets don't quote keys" but the issue is that one will not be able to add a new pizza ?

No. There is an original issue from the ticket description, that you can see on the attached record issue_30386.mp4. Your patch doesn't fix it.

2. Is that the inability to add a new pizza is caused by the issue of admin foreign key widgets not quoting keys with special characters

No. It's a side-effect of ​patch proposed by Zeynel, so Zeynel's patch fixes the original issue (see issue_30386.mp4) but introduces a regression (see issue_30386_with_patch_and_save.mp4​). That's why we couldn't accept it.

I got it now. Thank you.

[Natalia Bidart]

For those reading from the top that can't find the "original PR", this is the one: ​PR

[Sarah Boyce]

(marking for review as we think the bug is fixed 🤞)