Skip to content

Feature: Support redirecting to an external URL on successful booking#2087

Merged
pumfleet merged 34 commits intocalcom:mainfrom
hariombalhara:feat/success-url
Apr 5, 2022
Merged

Feature: Support redirecting to an external URL on successful booking#2087
pumfleet merged 34 commits intocalcom:mainfrom
hariombalhara:feat/success-url

Conversation

@hariombalhara
Copy link
Copy Markdown
Member

@hariombalhara hariombalhara commented Mar 8, 2022

What does this PR do?

Fixes #1457 #2088

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How should this be tested?

  • Ensure only URL starting with http:// https:// are accepted even if a direct hit to mutation goes without browser validation.
  • Upgrade Modal Box
    • Shown on Input click and Pro Badge Click. Pro Badge shown in Free account only. Trial and Pro doesn’t have it.
  • Redirection Test
    • Automatic Redirect after 10 seconds
    • Cross Button to cancel automatic redirection. User stays on the success page and toast is dismissed
    • Continue button takes to external URL immediately.
    • Redirect URL can have query params or hash fragment and they are forwarded as is.
    • Mobile Device
      • Redirecting to {URL} is visible but timer isn’t there considering limited space.
  • Team Event - Redirect can be added and redirection works [Trial And PRO].
    • From Trial to Free downgrade Team Event keeps working(I would have expected it to stop working) and thus redirection keeps working with it.
    • From Pro to Free - Same thing
  • User Event - Redirect can be added and redirection works.[Trial And PRO]
    • From Trial to Free downgrade - Redirect stops working but event booking is still working. User can’t edit the redirect URL but he would be able to see the already added URL which won’t work. Pro Upgrade Modal would be shown.
    • From Pro to Free downgrade - Same thing.

Checklist

  • I haven't checked if my changes generate no new warnings
  • I haven't added tests that prove my fix is effective or that my feature works
  • I haven't checked if new and existing unit tests pass locally with my changes

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 8, 2022

@hariombalhara is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 8, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

docs – ./apps/docs

🔍 Inspect: https://vercel.com/cal/docs/GhvV7WnWD3ttjdm5WQo9VRamBNkQ
✅ Preview: https://docs-git-fork-hariombalhara-feat-success-url-cal.vercel.app

[Deployment for 36dcc82 canceled]

calendso – ./apps/web

🔍 Inspect: https://vercel.com/cal/calendso/6oLHphrGyXRPVWdwTGgVqicuUtuS
✅ Preview: https://calendso-git-fork-hariombalhara-feat-success-url-cal.vercel.app

@vercel vercel Bot temporarily deployed to Preview – docs March 8, 2022 10:16 Inactive
Comment thread packages/prisma/migrations/20220308100145_/migration.sql
Copy link
Copy Markdown
Contributor

@alannnc alannnc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It worked great! It only leaves successRedirectUrl with a lot of url query params. Should we clear all of this info from the final url?
image
@zomars @baileypumfleet

Also can we improve a hint or a label here that says https:// in the url its required? I would change "Redirect on successful booking" to "Redirect on successful booking(https)"
image

Comment thread packages/prisma/migrations/20220308100145_/migration.sql Outdated
@vercel vercel Bot temporarily deployed to Preview – docs March 9, 2022 19:31 Inactive
@vercel vercel Bot temporarily deployed to Preview – docs March 10, 2022 03:25 Inactive
@hariombalhara
Copy link
Copy Markdown
Member Author

hariombalhara commented Mar 10, 2022

It worked great! It only leaves successRedirectUrl with a lot of url query params. Should we clear all of this info from the final url? image @zomars @baileypumfleet

I believe all this info should be kept as is so that whatever info we need to show that success page, owner can also use that to build a very customized success message using that. It also means that we should very well document those query parms

Also can we improve a hint or a label here that says https:// in the url its required? I would change "Redirect on successful booking" to "Redirect on successful booking(https)" image

Updated placeholder for the hint
Screenshot 2022-03-10 at 9 12 29 AM

@agustif
Copy link
Copy Markdown
Contributor

agustif commented Mar 10, 2022

Wouldn't this new feature allow any bad actor redirect a booking user to any badUrl after booking? Should we at least add some frontend notice that you're getting out of cal.com?

zomars
zomars previously requested changes Mar 10, 2022
Copy link
Copy Markdown
Contributor

@zomars zomars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm blocking this PR for a little bit more so we can address these Qs:

  • Can this be used maliciously?
  • Wouldn't this reopen a previously fixed vulnerability when you can use Cal to redirect to a malicious site?
  • If that's the case how could we handle that?
  • And lastly how does the redirect going to work for embedded calendars? (current frames)

Comment thread packages/prisma/migrations/20220308100145_/migration.sql Outdated
@hariombalhara
Copy link
Copy Markdown
Member Author

@agustif Yeah adding that would make sense. In the current flow there is no confirmation being shown by default any way(if redirect Url is set) . So, maybe we can add a small delay in redirection and there we can show the warning (in a way that is still good UX) as well as confirmation.

@zomars That should solve the malicious URL problem.

In iframe integration also we can redirect within the iframe(this should be the behavioue right now) Or do we have a usecase for top level redirection?

@PeerRich
Copy link
Copy Markdown
Member

I'm blocking this PR for a little bit more so we can address these Qs:

  • Can this be used maliciously?
  • Wouldn't this reopen a previously fixed vulnerability when you can use Cal to redirect to a malicious site?
  • If that's the case how could we handle that?
  • And lastly how does the redirect going to work for embedded calendars? (current frames)

I agree with your assessment. We also want to show the success page at least for a couple seconds. I recommend doing this:

image

<div class="fixed top-4 inset-x-0 pb-2 sm:pb-5 z-30" style="">
  <div class="max-w-7xl mx-auto px-2 sm:px-6 lg:px-8">
    <div class="p-2 rounded-sm bg-red-600 shadow-lg sm:p-3 bg-green-500">
      <div class="flex items-center justify-between flex-wrap">
        <div class="w-0 flex-1 flex items-center">
          <p class="ml-3 font-medium text-white truncate">
            <span class="md:hidden">Redirecting ...</span>
            <span class="hidden md:inline">You are being redirected to https://external.website.com in 9 seconds.</span>
          </p>
        </div>
        <div class="order-3 mt-2 flex-shrink-0 w-full sm:order-2 sm:mt-0 sm:w-auto">
          <a href="#" class="flex items-center justify-center px-4 py-2 border border-transparent rounded-sm shadow-sm text-sm font-medium text-indigo-600 bg-white hover:bg-indigo-50">Continue</a>
        </div>
        <div class="order-2 flex-shrink-0 sm:order-3 sm:ml-2">
          <button type="button" class="-mr-1 flex p-2 rounded-md hover:bg-indigo-500 focus:outline-none focus:ring-2 focus:ring-white">
            <span class="sr-only">Dismiss</span>
            <!-- Heroicon name: outline/x -->
            <svg class="h-6 w-6 text-white" xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24" stroke="currentColor" aria-hidden="true">
              <path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M6 18L18 6M6 6l12 12"></path>
            </svg>
          </button>
        </div>
      </div>
    </div>
  </div>
</div>

@PeerRich
Copy link
Copy Markdown
Member

PeerRich commented Mar 13, 2022

this was also planned as a PRO feature, which means it needs to be in /ee/ and disabled for FREE (show the same badge as we do for custom theme)

image

image

"In order to redirect to an external page after booking, you need to upgrade to a Pro account."

@vercel vercel Bot temporarily deployed to Preview – docs March 13, 2022 12:48 Inactive
Copy link
Copy Markdown
Member

@PeerRich PeerRich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PeerRich
Copy link
Copy Markdown
Member

another problem: when embedding in an iframe, it'll show the new redirect in an iframe.

we need to open in a new tab (until we have the script embed)

@hariombalhara hariombalhara requested a review from zomars April 4, 2022 13:01
@hariombalhara hariombalhara dismissed stale reviews from alannnc and zomars April 4, 2022 13:03

This is fixed

@vercel vercel Bot temporarily deployed to Preview – calendso April 4, 2022 13:04 Inactive
@vercel vercel Bot temporarily deployed to Preview – docs April 4, 2022 13:26 Inactive
@vercel vercel Bot temporarily deployed to Preview – calendso April 4, 2022 13:26 Inactive
Comment thread apps/web/server/routers/viewer/eventTypes.tsx Outdated
@zomars zomars added the ❗️ migrations contains migration files label Apr 4, 2022
@vercel vercel Bot temporarily deployed to Preview – docs April 4, 2022 15:58 Inactive
@vercel vercel Bot temporarily deployed to Preview – docs April 5, 2022 03:11 Inactive
@hariombalhara
Copy link
Copy Markdown
Member Author

This is ready to merge. Just migrations need to be run first @PeerRich @zomars

@vercel vercel Bot temporarily deployed to Preview – docs April 5, 2022 05:01 Inactive
@vercel vercel Bot temporarily deployed to Preview – docs April 5, 2022 05:02 Inactive
@vercel vercel Bot temporarily deployed to Preview – docs April 5, 2022 05:08 Inactive
@vercel vercel Bot temporarily deployed to Preview – docs April 5, 2022 07:07 Inactive
@vercel vercel Bot temporarily deployed to Preview – calendso April 5, 2022 07:58 Inactive
@pumfleet pumfleet enabled auto-merge (squash) April 5, 2022 07:58
@pumfleet pumfleet merged commit d76b9b0 into calcom:main Apr 5, 2022
joeauyeung added a commit that referenced this pull request Apr 6, 2022
@PeerRich PeerRich added the core area: core, team members only label Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core area: core, team members only ✨ feature New feature or request ❗️ migrations contains migration files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Redirect to URL after successful booking

7 participants