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

chore: Block POST requests to all page routes + remove /user & /user/type dynamic routes from middleware #19281

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

hbjORbj
Copy link
Contributor

@hbjORbj hbjORbj commented Feb 13, 2025

What does this PR do?

  1. Block POST requests to every route that goes through middleware except those starting with /api (only /api/trpc/:path* and /api/auth/signup go through middleware btw)

405 as expected

Screenshot 2025-02-13 at 9 52 43 AM

No 405 as expected for allowed endpoints

Screenshot 2025-02-13 at 10 23 17 AM
  1. Remove /[user] and /[user]/[type] dynamic routes from middleware while ensuring that metadata is still being created correctly
Screenshot 2025-02-14 at 8 45 15 AM Screenshot 2025-02-14 at 8 45 29 AM

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • N/A - I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • Test metadata in /[user] and /[user]/[type] booking pages

@graphite-app graphite-app bot requested a review from a team February 13, 2025 15:22
@hbjORbj hbjORbj changed the title chore: block posts requests to get routes chore: Block POST requests to normal routes Feb 13, 2025
Copy link

vercel bot commented Feb 13, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
cal-com-ui-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 14, 2025 2:49pm
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Feb 14, 2025 2:49pm
calcom-web-canary ⬜️ Ignored (Inspect) Visit Preview Feb 14, 2025 2:49pm

@keithwillcode keithwillcode added consumer core area: core, team members only labels Feb 13, 2025
Copy link

graphite-app bot commented Feb 13, 2025

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (02/13/25)

1 reviewer was added to this PR based on Keith Williams's automation.

@hbjORbj hbjORbj changed the title chore: Block POST requests to normal routes chore: Block POST requests to normal page routes Feb 13, 2025
@hbjORbj hbjORbj changed the title chore: Block POST requests to normal page routes chore: Block POST requests to all page routes Feb 13, 2025
Copy link
Contributor

github-actions bot commented Feb 13, 2025

E2E results are ready!

Comment on lines -186 to -187
"/:user/:type/",
"/:user/",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these dynamic routes won't go through middleware anymore

Comment on lines +63 to +65
const _pathname = h.get("x-pathname") ?? pathname ?? "";
const canonical = buildCanonical({ path: _pathname, origin: origin ?? CAL_URL });
const locale = h.get("x-locale") ?? (await getLocale(buildLegacyRequest(h, cookies()))) ?? "en";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

routes needed to go through middleware for these 2 special headers: x-pathname and x-locale.
However, getLocale can replace x-locale and hard-coded pathname (passed as arg) can replace x-pathname

@hbjORbj hbjORbj changed the title chore: Block POST requests to all page routes chore: Block POST requests to all page routes + remove /user & /user/type dynamic routes from middleware Feb 14, 2025
@hbjORbj hbjORbj added the High priority Created by Linear-GitHub Sync label Feb 14, 2025
@@ -36,7 +36,8 @@ export const generateMetadata = async ({ params, searchParams }: PageProps) => {
(t) => `${rescheduleUid && !!booking ? t("reschedule") : ""} ${title} | ${profileName}`,
(t) => `${rescheduleUid ? t("reschedule") : ""} ${title}`,
isBrandingHidden,
getOrgFullOrigin(eventData?.entity.orgSlug ?? null)
getOrgFullOrigin(eventData?.entity.orgSlug ?? null),
`/${params.user}/${params.type}`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2025-02-14 at 8 45 15 AM

og:url is being created correctly

@@ -30,7 +30,8 @@ export const generateMetadata = async ({ params, searchParams }: PageProps) => {
() => profile.name,
() => markdownStrippedBio,
false,
getOrgFullOrigin(entity.orgSlug ?? null)
getOrgFullOrigin(entity.orgSlug ?? null),
`/${params.user}`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2025-02-14 at 8 45 29 AM

og:url is being created correctly

],
};

export default collectEvents({
middleware: abTestMiddlewareFactory(middleware),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

abTestMiddlewareFactory isn't used and needed (it was testing infra for app dir when we supported /future dir)

@@ -1,9 +0,0 @@
import { AB_TEST_BUCKET_PROBABILITY } from "@calcom/lib/constants";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

abTest dir itself is deprecated

@@ -1,62 +0,0 @@
import { getBucket } from "abTest/utils";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

abTest dir itself is deprecated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consumer core area: core, team members only High priority Created by Linear-GitHub Sync ready-for-e2e
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants