sandstorm-io / sandstorm

Sandstorm is a self-hostable web productivity suite. It's implemented as a security-hardened web app package manager.
https://sandstorm.io
Other
6.73k stars 707 forks source link

Powerbox UI: add a "don't allow this page to make more requests" button #3417

Open zenhack opened 4 years ago

zenhack commented 4 years ago

@abliss hit this when experimenting with matrix: right now it's possible for an app to basically DoS a user's UI session by repeatedly issuing powerbox requests.

The same problem exists with browsers and the alert() function, and some browsers solve this by adding a checkbox to the alert dialog to prevent the page from opening further popups (after the 2nd or 3rd popup).

We could do something similar for powerbox requests.

ocdtrekkie commented 4 years ago

I'd like to avoid the bad solution browsers have chosen: Can we avoid the Powerbox request locking up the UI session? Like, people should be able to exit the grain, switch tabs, restart the grain, whatever, even if a Powerbox dialog is open.

zenhack commented 4 years ago

hm, yeah I suppose with browsers blocking this can make pages "degrade gracefully" but for things that are fundamentally applications, it's maybe less helpful and we should just make sure they can't interfere with other grains and/or the rest of Sandstorm.

jdougan commented 4 years ago

While I think this is a good idea, it really isn't more than a bandaid. As an additional bandaid, Deny should be an explicit action button on the dialog and the close button should be more of a "delay until later" action.

For a real solution, the powerbox UI needs to be cumulative, asynchronous and never block. For example, put an icon in the sandstorm bar that opens something listing the outstanding requests (as expandable tiles?) for the current app. The requests should be sortable, searchable and selectable and there are at least 3 actions you can do to them: show details, approve, deny, Ignore for now/delay is implicit in this model.

ocdtrekkie commented 4 years ago

@jdougan I like the idea of having some sort of UI up in the top bar. Ultimately we'll need one anyways: Right now after you approve a Powerbox request, there is no way to see that it's allowed, or to revoke it.

jdougan commented 4 years ago

There is no general capabilities/requests management UI? I see. Yes, that is important. And there will probably be some overlap in the code support needed.

zenhack commented 4 years ago

I don't think asynchronous is actually what you want -- generally, apps should be asking for resources at the time of use. This makes it easier to understand why a permission is being requested.

Bear in mind the way TTRSS is set up right now it's not really using the powerbox "correctly;" the current behavior is a concession to easily porting a legacy app. A feed reader built "sandstorm first" would, when you clicked subscribe to feed, immediately make a powerbox request, and you would supply the feed URL via the powerbox UI, which it would then use to fetch the feed, immediately. Powerbox requests would generally only be triggered in response to direct actions taken by the user. It would be very strange if clicking subscribe queued up a request in the background...

zenhack commented 4 years ago

(And yes, some sort of management UI is in the roadmap)

jdougan commented 4 years ago

It would be very strange if clicking subscribe queued up a request in the background...

Except it already is doing something similar when it is getting content referenced by a feed item. Or if the UI is closed and the feed fetch returns a redirect that it hasn't seen before. I can think of a bunch of cases where you will end up with powerbox requests coming up while unattended and those clearly need a UI

zenhack commented 4 years ago

Quoting jdougan (2020-09-23 01:53:18)

 It would be very strange if clicking subscribe queued up a request
 in the background...

Except it already is doing something similar when it is getting content referenced by a feed item. Or if the UI is closed and the feed fetch returns a redirect that it hasn't seen before. I can think of a bunch of cases where you will end up with powerbox requests coming up while unattended and those clearly need a UI

AFAIK it doesn't actually ever fetch referenced resources; images and the like are fetched by the client.

Usually the extra pb requests you see are because it's trying to find a favicon for the domain, and it tries both http + https, adding or removing a www. if necessary.

I've yet to see a pb request from TTRSS after the feeds are "initialized;" I'd prefer to just improve the way TTRSS uses the powerbox so all of that happens immediately when you subscribe, rather than possibly lagging by a few minutes.

We could possibly cut down on the number of requests by having Sandstorm just follow redirects automatically (maybe this could go in the powerbox tag so apps could specify whether or not they want this when making the pb request). That would at least generally cover the http -> https redirects that we see, which are the most common.

I also wonder whether the favicon being on a different domain than the feed happens often enough to be worth it; I suspect this is a rare occurrence and a better approach would be to just patch TTRSS to not bother with the favicon if it isn't found at the same domain, so as not to bug the user with an extra pb request on the off chance the icon isn't in the obvious place, as opposed to just not existing.

jdougan commented 4 years ago

AFAIK it doesn't actually ever fetch referenced resources; images and the like are fetched by the client.

I don't think that is entirely correct as I've seen PB requests that I know are not from feed URLSs. Plugins will fetch somethings sometimes such as cache_starred_images (which I'm not using at present) especially if the filter terms automatically star an entry.

As an example, I recently returned to the tt-rss UI to find a powerbox request for battlechat.co. As I am certain that none of my feeds have the domain in the feed URL, I did a search, found the entry that had this, then dumped the rss feed it was coming from so I could check it. That rss data is attached along with a screenshot of what it looks like in the UI.

ttrss-battlechat-powerbox-rss.txt image

     <entry>
        <author>
            <name>/u/Qwixzo</name>
            <uri>https://www.reddit.com/user/Qwixzo</uri>
        </author>
        <category term="classicwow" label="r/classicwow" />
        <content type="html">&lt;table&gt; &lt;tr&gt;&lt;td&gt; &lt;a href="https://www.reddit.com/r/classicwow/comments/iy89o1/former_blizzard_execs_including_mike_morhaime/"&gt; &lt;img src="https://b.thumbs.redditmedia.com/UemZoXUA43GlnIxudFQtp2nfpfOJkehWOqwuCHkQNeM.jpg" alt="Former Blizzard Exec’s including Mike Morhaime launch new game company Dreamhaven, free of Activision" title="Former Blizzard Exec’s including Mike Morhaime launch new game company Dreamhaven, free of Activision" /&gt; &lt;/a&gt; &lt;/td&gt;&lt;td&gt; &amp;#32; submitted by &amp;#32; &lt;a href="https://www.reddit.com/user/Qwixzo"&gt; /u/Qwixzo &lt;/a&gt; &lt;br/&gt; &lt;span&gt;&lt;a href="https://www.battlechat.co/2020/09/former-blizzard-execs-launch-new-game-company-dreamhaven/"&gt;[link]&lt;/a&gt;&lt;/span&gt; &amp;#32; &lt;span&gt;&lt;a href="https://www.reddit.com/r/classicwow/comments/iy89o1/former_blizzard_execs_including_mike_morhaime/"&gt;[comments]&lt;/a&gt;&lt;/span&gt; &lt;/td&gt;&lt;/tr&gt;&lt;/table&gt;</content>
        <id>t3_iy89o1</id>
        <media:thumbnail url="https://b.thumbs.redditmedia.com/UemZoXUA43GlnIxudFQtp2nfpfOJkehWOqwuCHkQNeM.jpg" />
        <link href="https://www.reddit.com/r/classicwow/comments/iy89o1/former_blizzard_execs_including_mike_morhaime/" />
        <updated>2020-09-23T11:24:49+00:00</updated>
        <title>Former Blizzard Exec’s including Mike Morhaime launch new game company Dreamhaven, free of Activision</title>
    </entry>

That attachment in the image refers to https://b.thumbs.redditmedia.com/UemZoXUA43GlnIxudFQtp2nfpfOJkehWOqwuCHkQNeM.jpg . So it isn't immediately apparent to me why it asked me to approve battlechat.co, but it did.

jdougan commented 4 years ago

And while I was writing the above I got a few more. I'm seeing some requests twice as noted before the image..

image Twice image Twice image image

Going down the wowhead rabbit hole because that is the same feed that the battlechat.co PB request came from so I can ID it pretty quickly: ttrss-wowhead-powerbox-rss.txt

image

    <entry>
        <author>
            <name>/u/diegorybak</name>
            <uri>https://www.reddit.com/user/diegorybak</uri>
        </author>
        <category term="classicwow" label="r/classicwow" />
        <content type="html">&lt;!-- SC_OFF --&gt;&lt;div class="md"&gt;&lt;p&gt;Im now level 45 paladin and I dont know if I am using most efficent talent build or rotation. Lot of sources are telling different things. For now I am using &lt;a href="https://classic.wowhead.com/guides/wow-classic-retribution-paladin-leveling-talent-build-1-60"&gt;https://classic.wowhead.com/guides/wow-classic-retribution-paladin-leveling-talent-build-1-60&lt;/a&gt; talents (don&amp;#39;t know why I should give points into seal of righteouesness because I am using seal of command instead. Also Kargoz build suggest me to spec prot and do some sitting reckoning stuff. Can u tell me which u think is the best and most efficent?&lt;/p&gt; &lt;/div&gt;&lt;!-- SC_ON --&gt; &amp;#32; submitted by &amp;#32; &lt;a href="https://www.reddit.com/user/diegorybak"&gt; /u/diegorybak &lt;/a&gt; &lt;br/&gt; &lt;span&gt;&lt;a href="https://www.reddit.com/r/classicwow/comments/iy8rx5/paladin_leveling_im_confused/"&gt;[link]&lt;/a&gt;&lt;/span&gt; &amp;#32; &lt;span&gt;&lt;a href="https://www.reddit.com/r/classicwow/comments/iy8rx5/paladin_leveling_im_confused/"&gt;[comments]&lt;/a&gt;&lt;/span&gt;</content>
        <id>t3_iy8rx5</id>
        <link href="https://www.reddit.com/r/classicwow/comments/iy8rx5/paladin_leveling_im_confused/" />
        <updated>2020-09-23T12:04:34+00:00</updated>
        <title>Paladin leveling, im confused.</title>
    </entry>

So I'd say that it is doing something to links embedded in content tags.

This also, of course, make it clear why descriptions are needed for the PB requests. I happen to know there are only a couple of feeds a classic.wowhead.com ref could have come from in my setup, but in a more complex situation (eg. the BBC requests) where there are a bunch of possibilities, the current undescriptive box is almost useless.

zenhack commented 4 years ago

Ok, yeah, it does look like it's trying to follow links in articles. I'm not sure if we can make that bit work more smoothly without granting much broader access.

I agree pb descriptions would help here, though note that actually taking advantage of them will require more invasive changes to TTRSS than the transparent proxy approach we use right now.

jdougan commented 4 years ago

My more general point was that there are situations (eg. software acting as an agent) where some software running under sandstorm while unattended will want to access previously unapproved resources, and you will want that to go as smoothly as possible. Which is why I put in asynchronous. It seems to me the right answer would be to (in effect, this is not an implementation description as I don't know the internals) to store a (request, continuation) in a list the user can look at when they have the time. If it is critical for something to be approved now to not break the application then you can use existing notification mechanisms to ensure it is seen sooner. If the user is verifiably at the UI then you can try a synchronous PB request, though it is extremely irritating to have one of these blocking dialogs pop up while I'm in the middle of something.

The other option would be to give the software broader capabilities than just approving single urls.