opencollective / opencollective

We're tracking all our Issues, RFCs and a few other documents in this repository.
https://opencollective.com
MIT License
2k stars 365 forks source link

Set up redirect URL for host terms #2757

Closed alanna closed 2 years ago

alanna commented 4 years ago

User story

As a fiscal host admin I am constantly having to link to our terms and conditions to reference them in user inquiries, etc, and it would be great if they were easier to access.

As a Collective admin, I want to easily access the terms for any fiscal host.

Best solution for this problem

Set up a URL like opencollective.com/host/terms that redirects to whatever hosts puts in the "terms" link box in their settings. They are already linked from the top of host pages, but having a URL would help a lot.

Screen Shot 2019-12-20 at 2 54 02 PM

brymut commented 4 years ago

Hi, I'm looking to get my feet wet again, may I have a go at this? @alanna / @Betree

Betree commented 4 years ago

Hi @brymut, feel free to! Host terms are stored in host.settings.tos.

I'd add to the specs above that:

brymut commented 4 years ago

Hi @Betree / @alanna , on the design side of things. This is what I've been able to come up with.

Clicking through would open a new tab so that redirecting doesn't have the Collective admin leave opencollective completely.

I'm using the legacy <CollectiveCover> at the top since its the only component I can find so far that fits, the newer version has tabs for "Contributions"/"Transactions" and doesn't use have a title property so that we can show that the page is the Fiscal Host's terms using a <FormattedMessage>.

  1. If the terms are an external link
image with tos link
  1. If the terms are not available/ not set by the Fiscal Host admin.
image with no tos
  1. If the terms are set up as plain text by the Fiscal Host admin.
image with plain text tos

May I please get some feedback on this?

Betree commented 4 years ago

Nice work!

We should not use the legacy cover, we're looking to get rid of it soon so we can't use it in new devs.

The rest is looking great. I wonder if there's a way with NextJs to redirect by sending back a special HTTP status, so that the transitional page will not appear. Not sure if that's possible, I'm going to check.

brymut commented 4 years ago

Any suggestion/alternative to what might work in place of the legacy cover?

Betree commented 4 years ago

I just checked and NextJS in fact recommends to redirect after mount - and to avoid it during rendering (see https://github.com/zeit/next.js/issues/4931#issuecomment-512787861). So we have to go above NextJS if we want to handle this properly, in https://github.com/opencollective/opencollective-frontend/blob/05228242f3327232c2b051ed8360d53944aa1f36/server/routes.js. Check app.renderToHTML / app.render to generate the fallback page from there.

Any suggestion/alternative to what might work in place of the legacy cover?

@brymut Try this one:

<Container pt={[4, 5]} mb={4}>
  <Flex alignItems="center" flexDirection="column" mx="auto" width="100%" maxWidth={320}>
    <LinkCollective collective={collective}>
      <Logo collective={collective} className="logo" height="10rem" style={{ margin: '0 auto' }} />
      <H2 as="h1" color="black.900" textAlign="center">
        {collective.name}
      </H2>
    </LinkCollective>
  </Flex>
</Container>
brymut commented 4 years ago

Hi again @Betree, I've had some progress on this and have opened a pull request. Could you please have a look when you have a chance.

I'm having an issue with understanding what you meant on how we could go above Nextjs when handling the redirect in routes.js, could you please explain a little more on that?

Betree commented 4 years ago

The routes in /server/routes.js have priority over NextJS's routes. They are pure node endpoints.

From there we can still generate NextJS's pages (ie. by calling renderToHTML) but we can also redirect and do other kind of magic that NextJS would not allow us to do while rendering.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. We haven't had the time to address it yet, but we want to keep it open. This message is just a reminder for us to help triage issues.

alanna commented 3 years ago

Bumping this again. This causes me pain consistently, and it seems like such a small thing to ask for. Can it just be done?

SudharakaP commented 3 years ago

@Betree : I can help with this, but my thinking is it might be better to show the terms and conditions in a modal, that way de don't take the user out of the context of the page; maybe they came to check something and navigating them to a special page in my mind might be counter intuitive. So how about a simple modal with the terms and conditions and the user can just dismiss it. 😄 WDYT?

Betree commented 3 years ago

@SudharakaP The problem is that we don't really know where the link is pointing to. It can be a web page, a Google Doc, a PDF, etc. Using an iframe for this could cause many problems, I think we should stick with a redirect.

I'm a bit worried about security though. Today, when people are redirected from unverified user links, we show a warning like this one: https://opencollective.com/redirect?url=http%3A%2F%2Fwebsite.com. The goal is to prevent phishing attacks where someone would make you click on a link that redirects to a website that looks like opencollective (eg. opnecollective.com) that the attacker could use to steal users info. With this redirect link in place, I could just bypass the redirect verification by linking to /host/terms (in my longDescription, private messages, etc.) and set my host terms URL to https://opnecollective.com/sign-in.

We must make sure that such an attack is not possible when implementing this issue.

image

alanna commented 2 years ago

Looks like this might never happen.... can someone just set me up a redirect from opencollective.foundation/terms to https://docs.google.com/document/u/2/d/e/2PACX-1vQ_fs7IOojAHaMBKYtaJetlTXJZLnJ7flIWkwxUSQtTkWUMtwFYC2ssb-ooBnT-Ldl6wbVhNQiCkSms/pub as a workaround?

SudharakaP commented 2 years ago

@SudharakaP The problem is that we don't really know where the link is pointing to. It can be a web page, a Google Doc, a PDF, etc. Using an iframe for this could cause many problems, I think we should stick with a redirect.

I'm a bit worried about security though. Today, when people are redirected from unverified user links, we show a warning like this one: https://opencollective.com/redirect?url=http%3A%2F%2Fwebsite.com. The goal is to prevent phishing attacks where someone would make you click on a link that redirects to a website that looks like opencollective (eg. opnecollective.com) that the attacker could use to steal users info. With this redirect link in place, I could just bypass the redirect verification by linking to /host/terms (in my longDescription, private messages, etc.) and set my host terms URL to https://opnecollective.com/sign-in.

We must make sure that such an attack is not possible when implementing this issue.

@Betree : I am a bit confused by this. Today we have the code,

https://github.com/opencollective/opencollective-frontend/blob/39cace1de0f2f2d7bad2119df006fb0bdff80f70/components/collective-page/hero/Hero.js#L280-L290

for the link to the terms for fiscal sponsorship which appears on the host page. This doesn't go through the external-redirect.js and thus the host can put any URL they want. My thinking is that we trust the hosts. So adding this kinda redirect doesn't quite make sense to me as only the host can set the terms url in the host settings. 🤔

In any case, I've opened a PR with the external-redirect.js so that whatever the host puts in the terms we shall treat it as an external redirect and show the user the warning (see the screen-cast there). However as I've explained above I think this might be unnecessary as we are already allowing the user to go to this url from the host page without the warning. Does that make sense? 🤔 😸

Betree commented 2 years ago

My thinking is that we trust the hosts

To create a host, all it takes is to create an organization then click a button. So no, we can't automatically trust a host just because they're a host.

This doesn't go through the external-redirect.js and thus the host can put any URL they want

This is exactly the problem here. What I tried to explain in https://github.com/opencollective/opencollective/issues/2757#issuecomment-912308225 is that someone could exploit such feature to bypass our redirect validations, since we're not going through external-redirect:

  1. Create a new host
  2. Set its host terms to redirect URL to https://malicious-phishing-website.com
  3. Now paste your https://opencollective.com/my-host/terms link in long descriptions, updates, and comments. Since it's an Open Collective link, it won't be sanitized behind a redirect.
  4. When clicking on the https://opencollective.com/my-host/terms link, users will be redirected to https://malicious-phishing-website.com without any warning, facilitating phishing attacks (eg. with a domain like https://opnecollective.com) which could result in stealing user's personal information.
Betree commented 2 years ago

@alanna I've marked foundation as a trusted host. With what @SudharakaP implemented, you can now use https://opencollective.com/foundation/terms to access the TOS.

Edit: also added the flag for for europe, ocnz and opensource

alanna commented 2 years ago

Amazing! Thank you!