From 9fc8d23dceea1b8fce2e1907f2b6f3a3aba56599 Mon Sep 17 00:00:00 2001 From: blaine-arcjet <146491715+blaine-arcjet@users.noreply.github.com> Date: Wed, 15 Jan 2025 10:14:20 -0700 Subject: [PATCH] WIP cleanup reason deprecation (#2857) --- decorate/index.ts | 31 +++++-- decorate/test/decorate.test.ts | 31 ++++--- protocol/client.ts | 1 - protocol/convert.ts | 47 ++++++++-- protocol/index.ts | 19 ++-- protocol/test/client.test.ts | 22 +---- protocol/test/convert.test.ts | 165 +++++++++++++++++++++++++++++---- 7 files changed, 237 insertions(+), 79 deletions(-) diff --git a/decorate/index.ts b/decorate/index.ts index 2efd1d31c..c6f402158 100644 --- a/decorate/index.ts +++ b/decorate/index.ts @@ -122,9 +122,19 @@ function extractReason(result: ArcjetRuleResult): ArcjetReason { } function isRateLimitReason( - reason?: ArcjetReason, + reason: ArcjetReason, ): reason is ArcjetRateLimitReason { - return typeof reason !== "undefined" && reason.isRateLimit(); + return reason.isRateLimit(); +} + +function getRateLimitReason( + decision: ArcjetDecision, +): ArcjetRateLimitReason | undefined { + if (decision.isDenied() || decision.isChallenged()) { + if (decision.reason.isRateLimit()) { + return decision.reason; + } + } } function nearestLimit( @@ -211,21 +221,22 @@ export function setRateLimitHeaders( } else { // For cached decisions, we may not have rule results, but we'd still have // the top-level reason. - if (isRateLimitReason(decision.reason)) { + const rateLimitReason = getRateLimitReason(decision); + if (rateLimitReason) { if ( - typeof decision.reason.max !== "number" || - typeof decision.reason.window !== "number" || - typeof decision.reason.remaining !== "number" || - typeof decision.reason.reset !== "number" + typeof rateLimitReason.max !== "number" || + typeof rateLimitReason.window !== "number" || + typeof rateLimitReason.remaining !== "number" || + typeof rateLimitReason.reset !== "number" ) { console.error( - format("Invalid rate limit encountered: %o", decision.reason), + format("Invalid rate limit encountered: %o", rateLimitReason), ); return; } - limit = toLimitString(decision.reason); - policy = toPolicyString([decision.reason.max, decision.reason.window]); + limit = toLimitString(rateLimitReason); + policy = toPolicyString([rateLimitReason.max, rateLimitReason.window]); } else { return; } diff --git a/decorate/test/decorate.test.ts b/decorate/test/decorate.test.ts index 4087fba26..898a09c27 100644 --- a/decorate/test/decorate.test.ts +++ b/decorate/test/decorate.test.ts @@ -3,6 +3,7 @@ import { expect } from "expect"; import { setRateLimitHeaders } from "../index"; import { ArcjetAllowDecision, + ArcjetDenyDecision, ArcjetRateLimitReason, ArcjetReason, ArcjetRuleResult, @@ -307,7 +308,7 @@ describe("setRateLimitHeaders", () => { const headers = new Headers(); setRateLimitHeaders( headers, - new ArcjetAllowDecision({ + new ArcjetDenyDecision({ results: [], ttl: 0, reason: new ArcjetRateLimitReason({ @@ -332,7 +333,7 @@ describe("setRateLimitHeaders", () => { const headers = new Headers(); setRateLimitHeaders( headers, - new ArcjetAllowDecision({ + new ArcjetDenyDecision({ results: [], ttl: 0, reason: new ArcjetRateLimitReason({ @@ -356,7 +357,7 @@ describe("setRateLimitHeaders", () => { const headers = new Headers(); setRateLimitHeaders( headers, - new ArcjetAllowDecision({ + new ArcjetDenyDecision({ results: [], ttl: 0, reason: new ArcjetRateLimitReason({ @@ -380,7 +381,7 @@ describe("setRateLimitHeaders", () => { const headers = new Headers(); setRateLimitHeaders( headers, - new ArcjetAllowDecision({ + new ArcjetDenyDecision({ results: [], ttl: 0, reason: new ArcjetRateLimitReason({ @@ -404,7 +405,7 @@ describe("setRateLimitHeaders", () => { const headers = new Headers(); setRateLimitHeaders( headers, - new ArcjetAllowDecision({ + new ArcjetDenyDecision({ results: [], ttl: 0, reason: new ArcjetRateLimitReason({ @@ -1023,7 +1024,7 @@ describe("setRateLimitHeaders", () => { const resp = new Response(); setRateLimitHeaders( resp, - new ArcjetAllowDecision({ + new ArcjetDenyDecision({ results: [], ttl: 0, reason: new ArcjetRateLimitReason({ @@ -1048,7 +1049,7 @@ describe("setRateLimitHeaders", () => { const resp = new Response(); setRateLimitHeaders( resp, - new ArcjetAllowDecision({ + new ArcjetDenyDecision({ results: [], ttl: 0, reason: new ArcjetRateLimitReason({ @@ -1072,7 +1073,7 @@ describe("setRateLimitHeaders", () => { const resp = new Response(); setRateLimitHeaders( resp, - new ArcjetAllowDecision({ + new ArcjetDenyDecision({ results: [], ttl: 0, reason: new ArcjetRateLimitReason({ @@ -1096,7 +1097,7 @@ describe("setRateLimitHeaders", () => { const resp = new Response(); setRateLimitHeaders( resp, - new ArcjetAllowDecision({ + new ArcjetDenyDecision({ results: [], ttl: 0, reason: new ArcjetRateLimitReason({ @@ -1120,7 +1121,7 @@ describe("setRateLimitHeaders", () => { const resp = new Response(); setRateLimitHeaders( resp, - new ArcjetAllowDecision({ + new ArcjetDenyDecision({ results: [], ttl: 0, reason: new ArcjetRateLimitReason({ @@ -1834,7 +1835,7 @@ describe("setRateLimitHeaders", () => { const resp = new OutgoingMessage(); setRateLimitHeaders( resp, - new ArcjetAllowDecision({ + new ArcjetDenyDecision({ results: [], ttl: 0, reason: new ArcjetRateLimitReason({ @@ -1859,7 +1860,7 @@ describe("setRateLimitHeaders", () => { const resp = new OutgoingMessage(); setRateLimitHeaders( resp, - new ArcjetAllowDecision({ + new ArcjetDenyDecision({ results: [], ttl: 0, reason: new ArcjetRateLimitReason({ @@ -1883,7 +1884,7 @@ describe("setRateLimitHeaders", () => { const resp = new OutgoingMessage(); setRateLimitHeaders( resp, - new ArcjetAllowDecision({ + new ArcjetDenyDecision({ results: [], ttl: 0, reason: new ArcjetRateLimitReason({ @@ -1907,7 +1908,7 @@ describe("setRateLimitHeaders", () => { const resp = new OutgoingMessage(); setRateLimitHeaders( resp, - new ArcjetAllowDecision({ + new ArcjetDenyDecision({ results: [], ttl: 0, reason: new ArcjetRateLimitReason({ @@ -1931,7 +1932,7 @@ describe("setRateLimitHeaders", () => { const resp = new OutgoingMessage(); setRateLimitHeaders( resp, - new ArcjetAllowDecision({ + new ArcjetDenyDecision({ results: [], ttl: 0, reason: new ArcjetRateLimitReason({ diff --git a/protocol/client.ts b/protocol/client.ts index e1876f823..6e4bf1f92 100644 --- a/protocol/client.ts +++ b/protocol/client.ts @@ -115,7 +115,6 @@ export function createClient(options: ClientOptions): Client { runtime: context.runtime, ttl: decision.ttl, conclusion: decision.conclusion, - reason: decision.reason, ruleResults: decision.results, }, "Decide response", diff --git a/protocol/convert.ts b/protocol/convert.ts index 011d8a65d..f2199c249 100644 --- a/protocol/convert.ts +++ b/protocol/convert.ts @@ -392,13 +392,45 @@ export function ArcjetRuleResultFromProtocol( } export function ArcjetDecisionToProtocol(decision: ArcjetDecision): Decision { - return new Decision({ - id: decision.id, - ttl: decision.ttl, - conclusion: ArcjetConclusionToProtocol(decision.conclusion), - reason: decision.reason && ArcjetReasonToProtocol(decision.reason), - ruleResults: decision.results.map(ArcjetRuleResultToProtocol), - }); + if (decision.isDenied()) { + return new Decision({ + id: decision.id, + ttl: decision.ttl, + conclusion: ArcjetConclusionToProtocol(decision.conclusion), + reason: ArcjetReasonToProtocol(decision.reason), + ruleResults: decision.results.map(ArcjetRuleResultToProtocol), + }); + } + + if (decision.isChallenged()) { + return new Decision({ + id: decision.id, + ttl: decision.ttl, + conclusion: ArcjetConclusionToProtocol(decision.conclusion), + reason: ArcjetReasonToProtocol(decision.reason), + ruleResults: decision.results.map(ArcjetRuleResultToProtocol), + }); + } + + if (decision.isErrored()) { + return new Decision({ + id: decision.id, + ttl: decision.ttl, + conclusion: ArcjetConclusionToProtocol(decision.conclusion), + ruleResults: decision.results.map(ArcjetRuleResultToProtocol), + }); + } + + if (decision.isAllowed()) { + return new Decision({ + id: decision.id, + ttl: decision.ttl, + conclusion: ArcjetConclusionToProtocol(decision.conclusion), + ruleResults: decision.results.map(ArcjetRuleResultToProtocol), + }); + } + + return new Decision(); } export function ArcjetIpDetailsFromProtocol( @@ -667,6 +699,7 @@ export function ArcjetRuleToProtocol( rule: { case: "sensitiveInfo", value: { + mode: ArcjetModeToProtocol(rule.mode), allow: rule.allow, deny: rule.deny, }, diff --git a/protocol/index.ts b/protocol/index.ts index 321138cf9..4a1c62df2 100644 --- a/protocol/index.ts +++ b/protocol/index.ts @@ -575,10 +575,6 @@ export class ArcjetIpDetails { * the decision in the Arcjet dashboard. * @property `conclusion` - Arcjet's conclusion about the request. This will be * one of `"ALLOW"`, `"DENY"`, `"CHALLENGE"`, or `"ERROR"`. - * @property `reason` - A structured data type about the reason for the - * decision. One of: {@link ArcjetRateLimitReason}, {@link ArcjetEdgeRuleReason}, - * {@link ArcjetBotReason}, {@link ArcjetShieldReason}, - * {@link ArcjetEmailReason}, or {@link ArcjetErrorReason}. * @property `ttl` - The duration in milliseconds this decision should be * considered valid, also known as time-to-live. * @property `results` - Each separate {@link ArcjetRuleResult} can be found here @@ -599,7 +595,6 @@ export abstract class ArcjetDecision { ip: ArcjetIpDetails; abstract conclusion: ArcjetConclusion; - abstract reason: ArcjetReason | undefined; constructor(init: { id?: string; @@ -637,7 +632,9 @@ export abstract class ArcjetDecision { export class ArcjetAllowDecision extends ArcjetDecision { conclusion = "ALLOW" as const; - /** @deprecated: use results instead */ + /** + * @deprecated Iterate rule results via `decision.results` instead. + **/ reason: ArcjetReason | undefined; constructor(init: { @@ -651,10 +648,6 @@ export class ArcjetAllowDecision extends ArcjetDecision { this.reason = init.reason; } - - hasReason(): this is ArcjetAllowDecision & { reason: ArcjetReason } { - return this.reason !== undefined; - } } export class ArcjetDenyDecision extends ArcjetDecision { @@ -692,14 +685,16 @@ export class ArcjetChallengeDecision extends ArcjetDecision { export class ArcjetErrorDecision extends ArcjetDecision { conclusion = "ERROR" as const; - /** @deprecated: use results instead */ + /** + * @deprecated Iterate rule results via `decision.results` instead. + * */ reason: ArcjetErrorReason | undefined; constructor(init: { id?: string; results: ArcjetRuleResult[]; ttl: number; - reason: ArcjetErrorReason; + reason?: ArcjetErrorReason; ip?: ArcjetIpDetails; }) { super(init); diff --git a/protocol/test/client.test.ts b/protocol/test/client.test.ts index 603b59b23..90102cdc1 100644 --- a/protocol/test/client.test.ts +++ b/protocol/test/client.test.ts @@ -565,7 +565,8 @@ describe("createClient", () => { const decision = await client.decide(context, details, []); expect(decision.isErrored()).toBe(true); - expect(decision.reason).toMatchObject({ + // Duplicated `isErrored()` narrowing because the assertion library isn't assertion aware + expect(decision.isErrored() && decision.reason).toMatchObject({ message: "Unknown error occurred", }); }); @@ -619,7 +620,8 @@ describe("createClient", () => { const decision = await client.decide(context, details, []); expect(decision.isErrored()).toBe(true); - expect(decision.reason).toMatchObject({ + // Duplicated `isErrored()` narrowing because the assertion library isn't assertion aware + expect(decision.isErrored() && decision.reason).toMatchObject({ message: "Boom!", }); }); @@ -785,7 +787,6 @@ describe("createClient", () => { decision: { id: decision.id, conclusion: Conclusion.ALLOW, - reason: new Reason(), ruleResults: [], }, }), @@ -921,14 +922,6 @@ describe("createClient", () => { decision: { id: decision.id, conclusion: Conclusion.ERROR, - reason: new Reason({ - reason: { - case: "error", - value: { - message: "Failure", - }, - }, - }), ruleResults: [], }, }), @@ -1057,12 +1050,7 @@ describe("createClient", () => { ...details, headers: { "user-agent": "curl/8.1.2" }, }, - decision: { - id: decision.id, - conclusion: Conclusion.UNSPECIFIED, - reason: new Reason(), - ruleResults: [], - }, + decision: {}, }), expect.anything(), ]); diff --git a/protocol/test/convert.test.ts b/protocol/test/convert.test.ts index 95a5fcf9a..1941c9fc4 100644 --- a/protocol/test/convert.test.ts +++ b/protocol/test/convert.test.ts @@ -23,6 +23,7 @@ import { EmailType, Mode, RateLimitAlgorithm, + RateLimitReason, Reason, Rule, RuleResult, @@ -35,6 +36,8 @@ import type { ArcjetFixedWindowRateLimitRule, ArcjetSlidingWindowRateLimitRule, ArcjetShieldRule, + ArcjetSensitiveInfoRule, + ArcjetConclusion, } from "../index.js"; import { ArcjetAllowDecision, @@ -51,6 +54,7 @@ import { ArcjetShieldReason, ArcjetIpDetails, ArcjetSensitiveInfoReason, + ArcjetDecision, } from "../index.js"; import { Timestamp } from "@bufbuild/protobuf"; @@ -113,6 +117,12 @@ describe("convert", () => { expect(ArcjetStackToProtocol("NODEJS")).toEqual(SDKStack.SDK_STACK_NODEJS); expect(ArcjetStackToProtocol("NEXTJS")).toEqual(SDKStack.SDK_STACK_NEXTJS); expect(ArcjetStackToProtocol("BUN")).toEqual(SDKStack.SDK_STACK_BUN); + expect(ArcjetStackToProtocol("SVELTEKIT")).toEqual( + SDKStack.SDK_STACK_SVELTEKIT, + ); + expect(ArcjetStackToProtocol("DENO")).toEqual(SDKStack.SDK_STACK_DENO); + expect(ArcjetStackToProtocol("NESTJS")).toEqual(SDKStack.SDK_STACK_NESTJS); + expect(ArcjetStackToProtocol("REMIX")).toEqual(SDKStack.SDK_STACK_REMIX); expect( ArcjetStackToProtocol( // @ts-expect-error @@ -319,6 +329,17 @@ describe("convert", () => { reason: "NOT_VALID", }); }).toThrow("Invalid Reason"); + // Bot rule V1 is deprecated and produces an Error Reason + expect( + ArcjetReasonFromProtocol( + new Reason({ + reason: { + case: "bot", + value: {}, + }, + }), + ), + ).toBeInstanceOf(ArcjetErrorReason); }); test("ArcjetReasonToProtocol", () => { @@ -510,25 +531,119 @@ describe("convert", () => { }); test("ArcjetDecisionToProtocol", () => { - const decision = new ArcjetAllowDecision({ - id: "abc123", - ttl: 0, - results: [], - reason: new ArcjetReason(), - ip: new ArcjetIpDetails(), - }); - if (decision.hasReason()) { - expect(ArcjetDecisionToProtocol(decision)).toEqual( - new Decision({ + expect( + ArcjetDecisionToProtocol( + new ArcjetAllowDecision({ id: "abc123", - conclusion: Conclusion.ALLOW, - ruleResults: [], - reason: new Reason(), + ttl: 0, + results: [], + ip: new ArcjetIpDetails(), }), - ); - } else { - throw new Error("decision created with reason have a reason"); - } + ), + ).toEqual( + new Decision({ + id: "abc123", + conclusion: Conclusion.ALLOW, + ruleResults: [], + }), + ); + expect( + ArcjetDecisionToProtocol( + new ArcjetErrorDecision({ + id: "abc123", + ttl: 0, + results: [], + ip: new ArcjetIpDetails(), + }), + ), + ).toEqual( + new Decision({ + id: "abc123", + conclusion: Conclusion.ERROR, + ruleResults: [], + }), + ); + expect( + ArcjetDecisionToProtocol( + new ArcjetChallengeDecision({ + id: "abc123", + ttl: 0, + results: [], + reason: new ArcjetRateLimitReason({ + max: 1, + remaining: 0, + window: 1, + reset: 1, + }), + ip: new ArcjetIpDetails(), + }), + ), + ).toEqual( + new Decision({ + id: "abc123", + conclusion: Conclusion.CHALLENGE, + ruleResults: [], + reason: new Reason({ + reason: { + case: "rateLimit", + value: new RateLimitReason({ + max: 1, + remaining: 0, + windowInSeconds: 1, + resetInSeconds: 1, + }), + }, + }), + }), + ); + expect( + ArcjetDecisionToProtocol( + new ArcjetDenyDecision({ + id: "abc123", + ttl: 0, + results: [], + reason: new ArcjetRateLimitReason({ + max: 1, + remaining: 0, + window: 1, + reset: 1, + }), + ip: new ArcjetIpDetails(), + }), + ), + ).toEqual( + new Decision({ + id: "abc123", + conclusion: Conclusion.DENY, + ruleResults: [], + reason: new Reason({ + reason: { + case: "rateLimit", + value: new RateLimitReason({ + max: 1, + remaining: 0, + windowInSeconds: 1, + resetInSeconds: 1, + }), + }, + }), + }), + ); + + const ArcjetInvalidDecision = class extends ArcjetDecision { + // @ts-expect-error + conclusion: ArcjetConclusion = "INVALID"; + }; + expect( + ArcjetDecisionToProtocol( + new ArcjetInvalidDecision({ + id: "abc123", + ttl: 0, + results: [], + ip: new ArcjetIpDetails(), + }), + ), + ).toEqual(new Decision()); }); test("ArcjetDecisionFromProtocol", () => { @@ -709,5 +824,21 @@ describe("convert", () => { }, }), ); + expect( + ArcjetRuleToProtocol(>{ + type: "SENSITIVE_INFO", + mode: "DRY_RUN", + priority: 1, + }), + ).toEqual( + new Rule({ + rule: { + case: "sensitiveInfo", + value: { + mode: Mode.DRY_RUN, + }, + }, + }), + ); }); });