Feature/add ip restriction middleware#3035
Conversation
src/middlewares/ip_restriction.ts
Outdated
| export type AddressType = "IPv4" | "IPv6"; | ||
|
|
||
| export type NetAddrInfo = { | ||
| /** | ||
| * Transport protocol type | ||
| */ | ||
| transport?: "tcp" | "udp"; | ||
| /** | ||
| * Transport port number | ||
| */ | ||
| port: number; | ||
| address: string; | ||
| addressType?: AddressType; | ||
| }; | ||
|
|
||
| export interface ConnInfo { | ||
| /** | ||
| * Remote information | ||
| */ | ||
| remote: NetAddrInfo; | ||
| } |
There was a problem hiding this comment.
These types already exist within the Deno runtime API. See https://docs.deno.com/api/deno/network.
There was a problem hiding this comment.
Each test should be wrapped in a Deno.test() call.
There was a problem hiding this comment.
@iuioiua san.
Thanks for your comment!
Sorry if I’m mistaken, but I thought when writing in a BDD style, Deno.test would be replaced by describe. Was I incorrect about that?
There was a problem hiding this comment.
I looked at the other code as well.
I understand now that using Deno.test instead of a BDD style is the standard.
I’ll prepare a fix.
src/middlewares/ip_restriction.ts
Outdated
There was a problem hiding this comment.
There's a lot of IP address logic here that might be valuable in other domains. I recommend creating an issue for the Standard Library for any useful utilities to @std/net.
There was a problem hiding this comment.
The part you’re pointing out is indeed a generic utility — in Hono, it’s placed in utils as well.
I’ll go ahead and create an issue for now.
There was a problem hiding this comment.
I have created an issue.Thanks.
denoland/std#6722
|
@iuioiua |
| import type { Middleware } from "./mod.ts"; | ||
| import { isIPv4, isIPv6, matchSubnets } from "@std/net/unstable-ip"; | ||
|
|
||
| export type AddressType = "IPv6" | "IPv4" | "none"; |
There was a problem hiding this comment.
I suggest using Deno.NetworkInterfaceInfo.family instead of this new type.
| function findType(addr: string): AddressType { | ||
| if (isIPv4(addr)) { | ||
| return "IPv4"; | ||
| } else if (isIPv6(addr)) { | ||
| return "IPv6"; | ||
| } | ||
| return "none"; | ||
| } |
There was a problem hiding this comment.
See first comment.
| function findType(addr: string): AddressType { | |
| if (isIPv4(addr)) { | |
| return "IPv4"; | |
| } else if (isIPv6(addr)) { | |
| return "IPv6"; | |
| } | |
| return "none"; | |
| } | |
| function findType(addr: string): Deno.NetworkInterfaceInfo.family | undefined { | |
| if (isIPv4(addr)) { | |
| return "IPv4"; | |
| } else if (isIPv6(addr)) { | |
| return "IPv6"; | |
| } | |
| return undefined; | |
| } |
| headers: { | ||
| "Content-Type": "text/plain", | ||
| }, |
There was a problem hiding this comment.
Not needed. The content-type header by default is "text/plain;charset=UTF-8" anyway.
| headers: { | |
| "Content-Type": "text/plain", | |
| }, |
| export interface IPRestrictionRules { | ||
| denyList?: string[]; | ||
| allowList?: string[]; | ||
| } |
There was a problem hiding this comment.
This interface and its properties should have JSDocs.
| if (!ctx.info.remoteAddr.hostname) { | ||
| return blockError(); | ||
| } |
There was a problem hiding this comment.
IIUC, this conditional will never be true because ctx.info.remoteAddr.hostname is always defined.
| if (!ctx.info.remoteAddr.hostname) { | |
| return blockError(); | |
| } |
| * app.use(ipRestriction({denyList: ["192.168.1.10", "2001:db8::1"]})) | ||
| * ``` | ||
| * | ||
| * @example custom error handling |
There was a problem hiding this comment.
Nit
| * @example custom error handling | |
| * @example Custom error handling |
| * @example custom error handling | ||
| * ```ts | ||
| * const customOnError: IpRestrictionOptions = { | ||
| * onError: () => { |
There was a problem hiding this comment.
I think we should show how the onErrors parameters can be used here.
There was a problem hiding this comment.
I've gone ahead and added the usage examples.
| function buildMatcher(ipList: string[]) { | ||
| return (addr: string) => { | ||
| return matchSubnets(addr, ipList); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Not needed if we go ahead with using matchSubnets inline, as stated below.
| function buildMatcher(ipList: string[]) { | |
| return (addr: string) => { | |
| return matchSubnets(addr, ipList); | |
| }; | |
| } |
|
|
||
| const denyMatcher = buildMatcher(denyList); | ||
| const allowMatcher = buildMatcher(allowList); | ||
|
|
There was a problem hiding this comment.
Something like this will avoid the need for if (options?.onError) conditionals and being inline makes the behaviour of onError more immediately understood.
| const onError = options?.onError ?? (() => | |
| new Response("Forbidden", { | |
| status: 403, | |
| headers: { | |
| "Content-Type": "text/plain", | |
| }, | |
| })); |
There was a problem hiding this comment.
I've made the adjustment to make it inline. Thanks!
| * app.use(ipRestriction({denyList: ["192.168.1.10", "2001:db8::1"]}, customOnError)) | ||
| * ``` | ||
| */ | ||
| export function ipRestriction<State>( |
There was a problem hiding this comment.
I'm not strongly for or against this name... Are we sure about this name? Any other alternatives?
There was a problem hiding this comment.
@iuioiua
How about 'IpRule' or 'IpFilter' instead of 'IP Restriction'? They sound pretty good.
There was a problem hiding this comment.
ipFilter sounds okay to me.
iuioiua
left a comment
iuioiua
left a comment
There was a problem hiding this comment.
Getting closer to being ready, IMO. Few nits. HMU on Discord (same username) if you want to chat about any of this.
| * app.use(ipRestriction({denyList: ["192.168.1.10", "2001:db8::1"]}, customOnError)) | ||
| * ``` | ||
| */ | ||
| export function ipRestriction<State>( |
There was a problem hiding this comment.
ipFilter sounds okay to me.
| } | ||
|
|
||
| if (matchSubnets(addr, rules.allowList || [])) { | ||
| const res = await ctx.next(); |
There was a problem hiding this comment.
| const res = await ctx.next(); | |
| return ctx.next(); |
Again, awaiting ctx.next() adds no value.
| } | ||
|
|
||
| if ((rules.allowList || []).length === 0) { | ||
| return await ctx.next(); |
There was a problem hiding this comment.
| return await ctx.next(); | |
| return ctx.next(); |
Ditto
| return await ctx.next(); | ||
| } else { | ||
| if (options?.onError) { | ||
| return await options.onError({ addr, type }, ctx); |
There was a problem hiding this comment.
| return await options.onError({ addr, type }, ctx); | |
| return options.onError({ addr, type }, ctx); |
Ditto
| ): Middleware<State> { | ||
| return async function ipRestriction<State>(ctx: Context<State>) { |
There was a problem hiding this comment.
| ): Middleware<State> { | |
| return async function ipRestriction<State>(ctx: Context<State>) { | |
| ): Middleware<State> { | |
| const onBlock = options.onError ?? () => new Response("Forbidden", { status: 403 }); | |
| return async function ipRestriction<State>(ctx: Context<State>) { |
Reminder: avoids having to write if (options?.onError) blocks and makes things clear more immediately.
| if (options?.onError) { | ||
| return options.onError({ addr, type }, ctx); | ||
| } | ||
| return blockError(); |
There was a problem hiding this comment.
Ditto. See comment about onError being defined within the exported function.
| if (options?.onError) { | ||
| return await options.onError({ addr, type }, ctx); | ||
| } |
There was a problem hiding this comment.
Ditto. See comment about onError being defined within the exported function.
| /** | ||
| * IP restriction Middleware for Fresh. | ||
| * | ||
| * @param rules rules `{ denyList: string[], allowList: string[] }`. |
There was a problem hiding this comment.
| * @param rules rules `{ denyList: string[], allowList: string[] }`. | |
| * @param rules Deny and allow rules object. |
How you had it isn't how @param JSDoc tags should be written. They need to be written in natural human language describing what the variable is.
| * ```ts | ||
| * const app = new App<State>() | ||
| * | ||
| * app.use(ipRestriction({denyList: ["192.168.1.10", "2001:db8::1"]})) |
There was a problem hiding this comment.
| * app.use(ipRestriction({denyList: ["192.168.1.10", "2001:db8::1"]})) | |
| * app.use(ipRestriction({ | |
| * denyList: ["192.168.1.10", "2001:db8::1"], | |
| * })); |
Nit
| * return new Response("custom onError", { status: 401 }); | ||
| * }, | ||
| * }; | ||
| * app.use(ipRestriction({denyList: ["192.168.1.10", "2001:db8::1"]}, customOnError)) |
There was a problem hiding this comment.
| * app.use(ipRestriction({denyList: ["192.168.1.10", "2001:db8::1"]}, customOnError)) | |
| * app.use(ipRestriction({ | |
| * denyList: ["192.168.1.10", "2001:db8::1"] | |
| * }, customOnError)); |
Nit
There was a problem hiding this comment.
Should these changes be reverted?
There was a problem hiding this comment.
I think ip_filter is necessary because it relies on @std/net.
| export interface ipFilterOptions { | ||
| onError?: <State>( | ||
| remote: { | ||
| addr: string; | ||
| type: Deno.NetworkInterfaceInfo["family"] | undefined; | ||
| }, | ||
| ctx: Context<State>, | ||
| ) => Response | Promise<Response>; | ||
| } |
There was a problem hiding this comment.
This interface and its properties need documentation.
There was a problem hiding this comment.
We have addressed this matter.
| } | ||
|
|
||
| const addr = ctx.info.remoteAddr.hostname; | ||
| const type = findType(addr); |
There was a problem hiding this comment.
I think whether you're using addr and type is unrelated to my point 😛
See my suggestion below.
| } | ||
|
|
||
| const addr = ctx.info.remoteAddr.hostname; | ||
| const type = findType(addr); |
There was a problem hiding this comment.
| const type = findType(addr); | |
| const type = IPv4(addr) ? "IPv4" : "IPv6"; |
There was a problem hiding this comment.
I seem to have been overthinking it unnecessarily. I would like to adopt your proposal.
| if (type == undefined) { | ||
| return blockError(); | ||
| } |
There was a problem hiding this comment.
Yes, I do think it's unnecessary because type can only ever be IPv4 or IPv6.
| throw new TypeError( | ||
| "Unsupported transport protocol. TCP & UDP is supported.", | ||
| ); |
There was a problem hiding this comment.
| throw new TypeError( | |
| "Unsupported transport protocol. TCP & UDP is supported.", | |
| ); | |
| return ctx.next(); |
| if ( | ||
| ctx.info.remoteAddr.transport !== "udp" && | ||
| ctx.info.remoteAddr.transport !== "tcp" | ||
| ) { | ||
| throw new TypeError( | ||
| "Unsupported transport protocol. TCP & UDP is supported.", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Sorry, to correct my stance, yes, we should return early if not UDP or TCP traffic, because an IP address isn't being used. But I don't think throwing is the correct behaviour. Instead, we should just pass through. See my suggestion above.
| rules: IpFilterRules, | ||
| options?: ipFilterOptions, | ||
| ): Middleware<State> { | ||
| const onBlock = options?.onError ?? (() => blockError()); |
There was a problem hiding this comment.
| const onBlock = options?.onError ?? (() => blockError()); | |
| const onBlock = options?.onError ?? (() => new Response("Forbidden", { status: 403 })); |
So the reader doesn't have to scroll back up to see what onBlock does by default (since it's simple anyway).
| if ((rules.allowList || []).length === 0) { | ||
| return ctx.next(); | ||
| } else { | ||
| return onBlock({ addr, type }, ctx); |
There was a problem hiding this comment.
| if ((rules.allowList || []).length === 0) { | |
| return ctx.next(); | |
| } else { | |
| return onBlock({ addr, type }, ctx); | |
| return onBlock({ addr, type }, ctx); |
I believe this first if statement is already covered by the preceding allow block.
There was a problem hiding this comment.
this section is needs.
This is because there were cases where the permission list was not explicitly set.
However, as pointed out, some parts seemed unnecessary, so I have reorganized the content.
Thank you
| } | ||
|
|
||
| /** | ||
| * IP restriction Middleware for Fresh. |
There was a problem hiding this comment.
You should explain this filter. I.e. the precedence of rules. So deny rules take precedence over allow rules and traffic not matching any rule is denied by default (implicit deny).
There was a problem hiding this comment.
I mentioned this matter.
Thankyou
|
Thank you for the review on this PR a while back. It’s been about two months since iuioiua-san kindly provided feedback, and I truly appreciate the support. Please let me know if there is anything additional I can do to help get this merged—I'm ready to jump back in and make any necessary updates immediately. |
3 participants
Why was this PR initiated?
I opened an issue and held discussions about officially providing middleware in Fresh by porting middleware from Hono.
#3011
What does this PR do?
I introduced the IP Restriction middleware bundled with Fresh by porting the IP Restriction middleware from Hono.
I appreciate your review.