Skip to content

Commit

Permalink
fix: Security improvements, typing fixes
Browse files Browse the repository at this point in the history
Fixes potential security hole in no_cache option.
  • Loading branch information
maticzav committed Aug 26, 2019
1 parent 17911d5 commit bc18622
Show file tree
Hide file tree
Showing 16 changed files with 267 additions and 161 deletions.
53 changes: 35 additions & 18 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -103,17 +103,25 @@ function getUser(req) {

// Rules

const isAuthenticated = rule()(async (parent, args, ctx, info) => {
return ctx.user !== null
})
/* Read more about cache options down in the `rules/cache` section. */

const isAdmin = rule()(async (parent, args, ctx, info) => {
return ctx.user.role === 'admin'
})
const isAuthenticated = rule({ cache: 'contextual' })(
async (parent, args, ctx, info) => {
return ctx.user !== null
},
)

const isEditor = rule()(async (parent, args, ctx, info) => {
return ctx.user.role === 'editor'
})
const isAdmin = rule({ cache: 'contextual' })(
async (parent, args, ctx, info) => {
return ctx.user.role === 'admin'
},
)

const isEditor = rule({ cache: 'contextual' })(
async (parent, args, ctx, info) => {
return ctx.user.role === 'editor'
},
)

// Permissions

Expand Down Expand Up @@ -274,25 +282,34 @@ const admin = bool =>
)
```

- Cache is enabled by default across all rules. To prevent `cache` generation, set `{ cache: 'no_cache' }` or `{ cache: false }` when generating a rule.
- By default, no rule is executed more than once in complete query execution. This accounts for significantly better load times and quick responses.
- Cache is disabled by default. To enable `cache` generation, set cache option when generating a rule.

##### Cache

You can choose from three different cache options.

1. `no_cache` - prevents rules from being cached.
1. `contextual` - use when rule only relies on `ctx` parameter.
1. `strict` - use when rule relies on `parent` or `args` parameter as well.
1. `contextual` - use when rule only relies on `context` parameter (useful for authentication).
1. `strict` - use when rule relies on `parent` or `args` parameter as well (field specific modifications).

```ts
// Contextual
const admin = rule({ cache: 'contextual' })(async (parent, args, ctx, info) => {
return ctx.user.isAdmin
})
const isAdmin = rule({ cache: 'contextual' })(
async (parent, args, ctx, info) => {
return ctx.user.isAdmin
},
)
// Strict
const admin = rule({ cache: 'strict' })(async (parent, args, ctx, info) => {
const canSeeUserSensitiveData = rule({ cache: 'strict' })(
async (parent, args, ctx, info) => {
/* The id of observed User matches the id of authenticated viewer. */
return ctx.viewer.id === parent.id
},
)
// No-cache (defuault)
const admin = rule({ cache: 'no_cache' })(async (parent, args, ctx, info) => {
return ctx.user.isAdmin || args.code === 'secret' || parent.id === 'theone'
})
```
Expand Down Expand Up @@ -629,7 +646,7 @@ See [#126](https://github.com/maticzav/graphql-shield/issues/126#issuecomment-41

#### A rule is executed only once even though the dataset contains multiple values (and thus should execute the rule multiple times)

This occurs because of caching. When the cache is set to "contextual" only the contextual variable of the rule is expected to be evaluated. Setting the cache to "strict" allows the rule to rely on parent and args parameters as well.
This occurs because of caching. When the cache is set to `contextual` only the contextual variable of the rule is expected to be evaluated. Setting the cache to `strict` allows the rule to rely on parent and args parameters as well, while setting the cache to `no_cache` won't cache result at all.

## Contributors

Expand Down
14 changes: 8 additions & 6 deletions examples/advanced/src/permissions/rules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,16 @@ import { Context, getUserEmail } from '../utils'

// To see the effect with no cache, set { cache: false } in isCustomer rule.

export const isGrocer = rule()(async (parent, args, ctx: Context, info) => {
// console.log('SHIELD: IsGrocer?')
export const isGrocer = rule({ cache: 'contextual' })(
async (parent, args, ctx: Context, info) => {
// console.log('SHIELD: IsGrocer?')

const email = getUserEmail(ctx)
return ctx.db.exists.Grocer({ email })
})
const email = getUserEmail(ctx)
return ctx.db.exists.Grocer({ email })
},
)

export const isCustomer = rule({ cache: true })(
export const isCustomer = rule({ cache: 'contextual' })(
async (parent, args, ctx: Context, info) => {
// console.log('SHIELD: IsCustomer?')

Expand Down
24 changes: 15 additions & 9 deletions examples/basic/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,17 +75,23 @@ function getUser(req) {

// Rules

const isAuthenticated = rule()(async (parent, args, ctx, info) => {
return ctx.user !== null
})
const isAuthenticated = rule({ cache: 'contextual' })(
async (parent, args, ctx, info) => {
return ctx.user !== null
},
)

const isAdmin = rule()(async (parent, args, ctx, info) => {
return ctx.user.role === 'admin'
})
const isAdmin = rule({ cache: 'contextual' })(
async (parent, args, ctx, info) => {
return ctx.user.role === 'admin'
},
)

const isEditor = rule()(async (parent, args, ctx, info) => {
return ctx.user.role === 'editor'
})
const isEditor = rule({ cache: 'contextual' })(
async (parent, args, ctx, info) => {
return ctx.user.role === 'editor'
},
)

// Permissions

Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
const {rule} = require('graphql-shield');
const { rule } = require('graphql-shield')

const isAuthenticated = rule()((parent, args, ctx) => {
return Boolean(ctx.request.userId);
});
const isAuthenticated = rule({ cache: 'contextual' })((parent, args, ctx) => {
return Boolean(ctx.request.userId)
})

module.exports = {
isAuthenticated
};
isAuthenticated,
}
5 changes: 5 additions & 0 deletions jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,9 @@ module.exports = {
verbose: true,
coverageDirectory: './coverage',
coverageReporters: ['json', 'lcov', 'text', 'clover', 'html'],
globals: {
'ts-jest': {
tsConfig: 'tsconfig.test.json',
},
},
}
16 changes: 8 additions & 8 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,18 @@
"author": "Matic Zavadlal <matic.zavadlal@gmail.com>",
"scripts": {
"compile": "tsc -d",
"coverage": "yarn codecov",
"coverage": "codecov",
"clean": "rimraf dist",
"docz:dev": "docz dev",
"docz:build": "docz build",
"lint": "tslint --project tsconfig.json {src}/**/*.ts && prettier-check --ignore-path .gitignore src/**/*.ts",
"postinstall": "lightercollective postinstall",
"posttest": "npm-run-all lint",
"prepublishOnly": "npm-run-all compile",
"prerelease": "npm-run-all test",
"pretest": "npm-run-all clean compile",
"release": "semantic-release",
"test": "NODE_ENV=test jest"
"test": "npm-run-all --parallel test:*",
"test:jest": "NODE_ENV=test jest",
"test:types": "tsc --noEmit"
},
"dependencies": {
"@types/yup": "0.26.23",
Expand All @@ -38,17 +38,17 @@
"docz": "0.13.7",
"docz-theme-default": "0.13.7",
"graphql": "14.3.1",
"graphql-middleware": "3.0.2",
"graphql-middleware": "4.0.1",
"graphql-tools": "4.0.5",
"graphql-yoga": "1.18.1",
"graphql-yoga": "1.18.3",
"husky": "3.0.4",
"jest": "24.9.0",
"npm-run-all": "4.1.5",
"prettier": "1.18.2",
"prettier-check": "2.0.0",
"pretty-quick": "1.11.1",
"remark-emoji": "2.0.2",
"rimraf": "2.7.1",
"rimraf": "3.0.0",
"semantic-release": "15.13.24",
"ts-jest": "24.0.2",
"ts-node": "8.3.0",
Expand All @@ -59,7 +59,7 @@
},
"peerDependencies": {
"graphql": "^0.11.0 || ^0.12.0 || ^0.13.0 || ^14.0.0",
"graphql-middleware": "^2.0.0 || ^3.0.0"
"graphql-middleware": "^2.0.0 || ^3.0.0 || ^4.0.0"
},
"resolutions": {
"graphql-yoga/graphql": "14.3.1"
Expand Down
2 changes: 1 addition & 1 deletion src/constructors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ export const inputRule = <T>(
schema?: (yup: typeof Yup) => Yup.Schema<T>,
) => {
if (typeof name === 'string') {
return new InputRule(name, schema(Yup))
return new InputRule(name, schema!(Yup))
} else {
return new InputRule(Math.random().toString(), name(Yup))
}
Expand Down
17 changes: 11 additions & 6 deletions src/generator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
withDefault,
} from './utils'
import { ValidationError } from './validation'
import { IMiddlewareWithOptions } from 'graphql-middleware/dist/types'

/**
*
Expand All @@ -37,9 +38,14 @@ import { ValidationError } from './validation'
function generateFieldMiddlewareFromRule(
rule: ShieldRule,
options: IOptions,
): IMiddlewareFunction {
): IMiddlewareFunction<object, object, IShieldContext> {
async function middleware(
resolve: (parent, args, ctx, info) => any,
resolve: (
parent: object,
args: object,
ctx: IShieldContext,
info: GraphQLResolveInfo,
) => Promise<any>,
parent: { [key: string]: any },
args: { [key: string]: any },
ctx: IShieldContext,
Expand All @@ -53,7 +59,6 @@ function generateFieldMiddlewareFromRule(
if (!ctx._shield) {
ctx._shield = {
cache: {},
hashFunction: options.hashFunction,
}
}

Expand Down Expand Up @@ -83,17 +88,17 @@ function generateFieldMiddlewareFromRule(
return {
fragment: rule.extractFragment(),
resolve: middleware,
}
} as IMiddlewareWithOptions<object, object, IShieldContext>
}

if (isLogicRule(rule)) {
return {
fragments: rule.extractFragments(),
resolve: middleware,
}
} as IMiddlewareWithOptions<object, object, IShieldContext>
}

return middleware
return middleware as IMiddlewareFunction<object, object, IShieldContext>
}

/**
Expand Down
Loading

0 comments on commit bc18622

Please sign in to comment.