toolness
commented
3 years ago Hi @simonw, any chance you could take a look at this soon? No worries if not, I know you're super busy, but this addresses a major use case/pain point for our use of the tool. If you can't get to it anytime soon that's okay, I think we might just maintain a fork with this patch applied.
Also apologies the PR description is so long! I thought it'd be helpful to document my thought process and everything I'd tried, but maybe it ended up being kind of intimidating as a result...
This fixes #133, but in a funky way.
Originally, I thought this would be fairly straightforward: I started by simply removing the conditions for hiding the export buttons if the user was on a saved dashboard page, and tried to move on from there, poking and prodding at the codebase until it did what I wanted.
However, I quickly noticed that saved dashboard pages use a GET form, while dashboard index pages (the ones with custom SQL and fully exportable queries) use a POST form. It seemed like it might be a security hazard to allow for a GET request to potentially hold up the server for really long queries--I was imagining a CSRF that initiated a denial of service attack on a dashboard site, and though I'm not 100% sure it's possible, I wanted to play it safe--so I decided not to go the GET route.
I then learned that submit buttons actually support a
formmethodattribute which would allow the export buttons to convert the request to a POST if they were clicked, but then I faced the problem of embedding the CSRF token in the form only on POST requests (I didn't want it to be included in GET requests because that would make the URLs look gross). All of this led me down a bit of a rabbit hole and I started worrying about how much new logic this whole endeavor would introduce to the back-end, some of which might potentially accidentally introduce new security holes.Then I thought of a weird solution: what if I didn't add any new logic to the backend, and instead made the export buttons trigger some JS on the client-side that simply simulated a user entering an SQL query in the dashboard index page and exporting it? It turns out that this can be simulated via a properly constructed form, so that's what this PR does right now. As a result, my (possibly flawed) belief is that it can't introduce any new security holes in the back-end, because it only makes changes to the front-end.
Anyways, it's also possibly too clever for its own good, and could be brittle. And one major limitation is that it doesn't currently support dashboards with named parameters, though I don't think adding support for that would be hard. But I figured I'd submit what I've got so far and see what you think before moving forward, @simonw.