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

[ENG-2157] [Refix] Allow rx.download to resolve rx.get_upload_url #4470

Merged
merged 3 commits into from
Jan 4, 2025

Conversation

masenf
Copy link
Collaborator

@masenf masenf commented Dec 3, 2024

Update the special case in a way that works with the new Var system and its unstoppable determination to concatenate strings instead of using template string

This has been broken since 0.6.0, so a regression test was added to avoid future inadvertant breakage.

Fix #2812 (again)

Break it?

import reflex as rx


class State(rx.State):
    x: str

    def do_download(self):
        return rx.download(rx.get_upload_url(self.x))


def index() -> rx.Component:
    return rx.container(
        rx.color_mode.button(position="top-right"),
        rx.vstack(
            rx.heading("Welcome to XSSflex!", size="9"),
            rx.form(
                rx.input(
                    rx.input.slot(
                        rx.icon_button(
                            "x",
                            size="1",
                            type="button",
                            on_click=State.set_x(""),
                            variant="ghost",
                        ),
                    ),
                    placeholder="A filename to download?",
                    value=State.x,
                    on_change=State.set_x,
                ),
                rx.button("Download"),
                on_submit=State.do_download(),
            ),
        ),
        rx.logo(),
    )


app = rx.App()
app.add_page(index)

Update the special case in a way that works with the new Var system and its
unstoppable determination to concatenate strings instead of using template
string

This has been broken since 0.6.0, so a regression test was added to avoid
future inadvertant breakage.

Fix #2812 (again)
Lendemor
Lendemor previously approved these changes Dec 3, 2024
@masenf masenf marked this pull request as draft December 3, 2024 19:13
@masenf
Copy link
Collaborator Author

masenf commented Dec 3, 2024

actually i just realized we can't merge this as is; since it's maybe creating a security problem for users that take whatever filename is given in the upload and potentially introduce script injection

@masenf
Copy link
Collaborator Author

masenf commented Dec 3, 2024

actually, maybe it's okay... @adhami3310 any idea about the script injection with this?

i tried the following

return rx.download(rx.get_upload_url('`${window.alert("foo")}`'))
return rx.download(rx.get_upload_url('"; window.alert("foo"); "'))

and both of those had proper escapes around the danger characters

@masenf masenf marked this pull request as ready for review December 12, 2024 13:12
@masenf masenf merged commit dcdcbfd into main Jan 4, 2025
41 checks passed
@masenf masenf deleted the masenf/fix-download-upload branch January 4, 2025 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[REF-2157] rx.download doesn't work with rx.get_upload_url from a backend event handler
3 participants