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

feat: implement Arcjet security bot detection & Shield WAF #330

Merged
merged 5 commits into from
Nov 26, 2024

Conversation

davidmytton
Copy link
Contributor

This adds Arcjet bot protection and attack detection, with custom rules depending on the route.

It uses the global error handler by default, but this can be customized. Instructions are in the README.

The tsconfig.json type checking was looking in the node_modules directory because of the include match patterns. Changing the match patterns to just consider files in src highlights a lot of errors in the existing files, so I opted to disable the checks for now because fixing them is out of scope of this PR.

Copy link

vercel bot commented Nov 21, 2024

@davidmytton is attempting to deploy a commit to the Ixartz's projects Team on Vercel.

A member of the Team first needs to authorize it.

@ixartz
Copy link
Owner

ixartz commented Nov 22, 2024

Very strange about the TypeScript error, it shouldn't look inside node_modules: https://github.com/ixartz/Next-js-Boilerplate/blob/main/tsconfig.json#L58, node_modules is excluded.

Anyway, it's out of the scope of the PR.

Copy link

codecov bot commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 0% with 48 lines in your changes missing coverage. Please review.

Project coverage is 10.23%. Comparing base (2f9646b) to head (ac670a5).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
src/app/[locale]/layout.tsx 0.00% 18 Missing ⚠️
src/libs/Arcjet.ts 0.00% 10 Missing and 1 partial ⚠️
src/components/Sponsors.tsx 0.00% 10 Missing ⚠️
src/app/[locale]/(marketing)/page.tsx 0.00% 7 Missing ⚠️
src/libs/Env.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #330      +/-   ##
==========================================
- Coverage   10.67%   10.23%   -0.45%     
==========================================
  Files          39       40       +1     
  Lines        1115     1163      +48     
  Branches       45       46       +1     
==========================================
  Hits          119      119              
- Misses        963     1010      +47     
- Partials       33       34       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

import { routing } from '@/libs/i18nNavigation';
import { enUS, frFR } from '@clerk/localizations';
import { ClerkProvider } from '@clerk/nextjs';
import { setRequestLocale } from 'next-intl/server';

const aj = arcjet.withRule(
Copy link
Owner

@ixartz ixartz Nov 22, 2024

Choose a reason for hiding this comment

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

Is it possible to combine with the one at src/app/[locale]/(marketing)/layout.tsx?

And move to the root layout: https://github.com/ixartz/Next-js-Boilerplate/blob/main/src/app/%5Blocale%5D/layout.tsx

So, we keep only one and everything is inside the root layout.

CATEGORY:SEARCH_ENGINE won't pass the middleware due to authentication.
Most pages under (auth) required authentication. This is why I'm thinking to add directly in the root layout, did I miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it that way because often you want more restrictive bot detection on the authenticated routes vs your website, but I've made the change. The Arcjet call is now in the root layout. It can easily be moved by the user later.

@davidmytton davidmytton requested a review from ixartz November 22, 2024 13:30
Copy link
Owner

@ixartz ixartz left a comment

Choose a reason for hiding this comment

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

It looks good to me, I'll merge this weekend.

README.md Outdated Show resolved Hide resolved
@ixartz
Copy link
Owner

ixartz commented Nov 25, 2024

@davidmytton, I wanted to merge the PR but I notice the changes in tsconfig.json is related to Arcjet dependency. I thought maybe the issue is related to your environment but actually, I got the same issue.

When I run the type check, npm run check-types and here is the error:

node_modules/@arcjet/protocol/client.ts:165:16 - error TS6133: 'response' is declared but its value is never read.

165         .then((response) => {
                   ~~~~~~~~

node_modules/@arcjet/protocol/convert.ts:63:13 - error TS6133: '_exhaustive' is declared but its value is never read.

63       const _exhaustive: never = mode;
               ~~~~~~~~~~~

node_modules/@arcjet/protocol/convert.ts:84:13 - error TS6133: '_exhaustive' is declared but its value is never read.

84       const _exhaustive: never = emailType;
               ~~~~~~~~~~~

node_modules/@arcjet/protocol/convert.ts:107:13 - error TS6133: '_exhaustive' is declared but its value is never read.

107       const _exhaustive: never = emailType;
                ~~~~~~~~~~~

node_modules/@arcjet/protocol/convert.ts:130:13 - error TS6133: '_exhaustive' is declared but its value is never read.

130       const _exhaustive: never = stack;
                ~~~~~~~~~~~

node_modules/@arcjet/protocol/convert.ts:147:13 - error TS6133: '_exhaustive' is declared but its value is never read.

147       const _exhaustive: never = stack;
                ~~~~~~~~~~~

node_modules/@arcjet/protocol/convert.ts:168:13 - error TS6133: '_exhaustive' is declared but its value is never read.

168       const _exhaustive: never = ruleState;
                ~~~~~~~~~~~

node_modules/@arcjet/protocol/convert.ts:187:13 - error TS6133: '_exhaustive' is declared but its value is never read.

187       const _exhaustive: never = conclusion;
                ~~~~~~~~~~~

node_modules/@arcjet/protocol/convert.ts:208:13 - error TS6133: '_exhaustive' is declared but its value is never read.

208       const _exhaustive: never = conclusion;
                ~~~~~~~~~~~

node_modules/@arcjet/protocol/convert.ts:274:13 - error TS6133: '_exhaustive' is declared but its value is never read.

274       const _exhaustive: never = proto.reason;
                ~~~~~~~~~~~

node_modules/@arcjet/protocol/convert.ts:511:13 - error TS6133: '_exhaustive' is declared but its value is never read.

511       const _exhaustive: never = decision.conclusion;
                ~~~~~~~~~~~

node_modules/@arcjet/protocol/index.ts:694:23 - error TS6133: 'Props' is declared but its value is never read.

694 export type ArcjetRule<Props extends {} = {}> = {

So, it's only related to Arcjet.

I would be great to know the root cause before merging it, in the tsconfig:

  • node_modules/ is defined in exclude

The type check shouldn't be run inside node_modules, it don't happen with other libraries.

On my side, I'll also try to investigate.

@mbtools
Copy link

mbtools commented Nov 25, 2024

Aj looks like a nice feature, but do you want to introduce a dependency on a solution in alpha release?
As a potential customer, this is a turn-off.

@davidmytton
Copy link
Contributor Author

The tsconfig.json is configured to compile all .ts files which include those in the node_modules directory. The line at

is causing this and it overrides the exclude. That's causing the node_modules directory to be compiled.

This is what I was referring to in the PR description. Changing it to scope only files in src causes a lot of TS errors because they weren't previously being checked, so I disabled the check for this PR.

To avoid this, we're going to stop publishing the TS files in a new SDK release shortly (arcjet/arcjet-js#2326). Ideally you'd be able to change the tsconfig.json to exclude node_modules, but we've seen this a few times with other configs. There are some benefits to including the TS files in our package, but avoiding these issues outweighs them.

Once the release is out I'll update this PR and it should resolve the problem.

That's why we still keep the SDK in alpha @mbtools - we're operating the service as if it were stable and fully supported, but there may still be some API changes we want to make like the fix needed here. arcjet/arcjet-js#928 is tracking pre-beta changes, but everything so far has been backwards compatible. Our approach is to avoid breaking anything even though the label is technically alpha.

@davidmytton
Copy link
Contributor Author

I'd also add there are other dependencies that are technically alpha (i.e pre v1.0) already in this boilerplate e.g. @electric-sql/pglite, @logtail/pino, drizzle-orm, etc. 😅

@davidmytton
Copy link
Contributor Author

@ixartz I just pushed the version update and reverted the tsconfig.json changes, which resolves the type check errors. This should be good to merge now.

@ixartz ixartz merged commit 909d0df into ixartz:main Nov 26, 2024
3 of 6 checks passed
Copy link

🎉 This PR is included in version 3.60.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@davidmytton davidmytton deleted the david/arcjet branch November 27, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants