Skip to content

Commit 3ec4764

Browse files
authored
fix(auth): require explicit GraphQL authorization (#2074)
## Summary Enforce explicit authorization on GraphQL handlers so an authenticated read-only key cannot perform notification writes, change UPS shutdown configuration, or retrieve persisted OIDC client secrets. Work intent: address the reported GraphQL authorization gaps and prevent missing permission decorators from reopening them. Related notification report: #2065. ## Resolution Extend the existing `nest-authz` guard to deny GraphQL handlers with missing or empty permission metadata. Keep Casbin as the permission authority and preserve existing REST behavior. Public login queries retain explicit public access; empty mutation namespaces require authentication and leave operation permissions to their child handlers. Add permissions to notification operations, UPS queries/subscriptions/configuration, settings fields, and flash-backup initiation. Complete the corresponding nested-field permissions in the API and bundled Connect plugin so default-deny preserves supported reads. Add `unraid-auth/require-graphql-authorization` to ESLint and run its focused audit over API and bundled-plugin source from both `lint` and `lint:fix`. It follows decorator imports, including aliases and namespaces, requires method-level access metadata, and rejects empty permission declarations. Test fixtures and generator templates are excluded. The rule has no automatic permission fix. ## Reviewer considerations - VIEWER already has `CONFIG / READ_ANY`. Secret-bearing settings values and OIDC provider/configuration queries therefore require `CONFIG / UPDATE_ANY`, including existing root OIDC queries. Ordinary configuration reads retain read permission. - Review the explicit authentication exception for empty mutation namespaces. Their child handlers still enforce resource permissions, including for narrowly scoped keys. - Third-party plugin handlers without permission metadata will now be denied. The runtime resolver audit covers the API and bundled plugins; generator templates are not deployed schema handlers. - Flash-backup initiation remains an unimplemented stub, now permission-protected. ## Verification - `pnpm --filter ./api coverage` — full API run passed: 2,159 tests across 186 passing suites. The lint configuration integration tests allow 30 seconds for cold ESLint startup under concurrent CI coverage load; unit-test timeouts remain unchanged. - `pnpm --filter ./api test eslint/require-graphql-authorization.spec.ts src/unraid-api/auth/resolver-authorization.spec.ts src/unraid-api/auth/authorization.guard.graphql.spec.ts` — 69 tests passed, including 34 lint-rule/config tests. - `pnpm --filter ./api lint` — passed with the new API/plugin authorization pass. A CLI negative check with temporary unguarded API and plugin endpoints failed with both expected lint errors; fixtures were removed. - `pnpm --filter ./api test src/unraid-api/auth src/unraid-api/graph/resolvers/notifications src/unraid-api/graph/resolvers/ups src/unraid-api/graph/resolvers/settings src/unraid-api/graph/resolvers/sso src/unraid-api/app/__test__/app.module.integration.spec.ts` — 610 tests passed across 31 suites. - Final resolver metadata audit rerun after expanding module-provider discovery — passed. - `pnpm --filter ./api lint` and `pnpm --filter ./api type-check` — passed; the final audit file also passed targeted lint. - `pnpm --filter ./api build` and `pnpm --filter unraid-api-plugin-connect build` — passed. Coverage includes real HTTP GraphQL requests with aliases and fragments, denial before service invocation, ADMIN and scoped-key access, public login, read-only monitoring, missing/empty metadata, and nested mutation authorization. Device services use test doubles; no live UPS configuration was changed. ## QA VM verification (2026-09-05) Tested the pinned CI plugin on DGTest01 running Unraid 7.3.2; live API reported `4.37.3+0b90b741` (merge build containing `b2976cefe`). All 38 GUEST/VIEWER denial checks passed, covering all notification mutations, UPS writes, flash backup, sensitive settings/OIDC queries, API keys, and nested parity operations. ADMIN and notification-scoped keys completed notification writes; VIEWER monitoring/configuration reads remained accessible. A persisted synthetic OIDC secret was readable by ADMIN and blocked from VIEWER, including aliases/fragments; public login queries remained accessible without exposing the secret. The VM has no physical UPS or configured UPS service: denied writes left configuration unchanged, but successful ADMIN UPS writes and shutdown behavior were not tested. An existing notification ID mismatch required using the persisted ID from the notification list for archive/unread/delete; the service code producing that mismatch is unchanged by this PR. Temporary keys and notifications were removed, SSO settings restored, and the provider restored and released the VM. Final status: available, powered off, drive-free. ## Release note Fix GraphQL authorization gaps affecting notifications, UPS configuration, and OIDC settings. Require explicit authorization metadata on GraphQL plugin handlers.
1 parent 97f639f commit 3ec4764

31 files changed

Lines changed: 1059 additions & 19 deletions

‎api/.eslintrc.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,10 @@ import noRelativeImportPaths from 'eslint-plugin-no-relative-import-paths';
44
import prettier from 'eslint-plugin-prettier';
55
import tseslint from 'typescript-eslint';
66

7+
import { graphqlAuthorizationConfig } from './eslint/graphql-authorization.config.mjs';
8+
79
export default tseslint.config(
10+
...graphqlAuthorizationConfig,
811
eslint.configs.recommended,
912
...tseslint.configs.recommended,
1013
{
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
import tseslint from 'typescript-eslint';
2+
3+
import requireGraphqlAuthorization from './require-graphql-authorization.mjs';
4+
5+
export const graphqlAuthorizationConfig = [
6+
{
7+
files: ['**/*.ts'],
8+
// Tests include deliberately unguarded fixtures; templates are not deployed handlers.
9+
ignores: [
10+
'**/*.spec.ts',
11+
'**/*.test.ts',
12+
'**/__test__/**',
13+
'**/__tests__/**',
14+
'**/templates/**',
15+
],
16+
languageOptions: { parser: tseslint.parser },
17+
plugins: {
18+
'unraid-auth': { rules: { 'require-graphql-authorization': requireGraphqlAuthorization } },
19+
},
20+
rules: { 'unraid-auth/require-graphql-authorization': 'error' },
21+
},
22+
];
23+
24+
// This focused pass does not evaluate the existing style-rule suppressions.
25+
export default [
26+
...graphqlAuthorizationConfig,
27+
{
28+
plugins: { '@typescript-eslint': tseslint.plugin },
29+
linterOptions: { reportUnusedDisableDirectives: 'off' },
30+
},
31+
];
Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
const graphqlHandlers = new Set(['Query', 'Mutation', 'Subscription', 'ResolveField']);
2+
const permissionSources = new Set(['@unraid/shared/use-permissions.directive.js', 'nest-authz']);
3+
const accessMarkers = new Map([
4+
['@app/unraid-api/auth/public.decorator.js', 'Public'],
5+
['@app/unraid-api/auth/authenticated.decorator.js', 'Authenticated'],
6+
]);
7+
8+
function importedDecorator(context, decorator) {
9+
const call = decorator.expression;
10+
if (call.type !== 'CallExpression') return;
11+
const callee = call.callee;
12+
const identifier = callee.type === 'Identifier' ? callee : callee.object;
13+
if (identifier?.type !== 'Identifier') return;
14+
let scope = context.sourceCode.getScope(decorator);
15+
while (scope) {
16+
const variable = scope.set.get(identifier.name);
17+
if (variable) {
18+
const definition = variable.defs.find((entry) => entry.type === 'ImportBinding');
19+
if (!definition) return;
20+
const specifier = definition.node;
21+
const source = definition.parent.source.value;
22+
if (callee.type === 'Identifier' && specifier.type === 'ImportSpecifier') {
23+
return { source, name: specifier.imported.name ?? specifier.imported.value, call };
24+
}
25+
if (callee.type === 'MemberExpression' && specifier.type === 'ImportNamespaceSpecifier') {
26+
const name = callee.computed ? callee.property.value : callee.property.name;
27+
return { source, name, call };
28+
}
29+
return;
30+
}
31+
scope = scope.upper;
32+
}
33+
}
34+
35+
function hasPermissionArgument(call) {
36+
return (
37+
call.arguments.length > 0 &&
38+
call.arguments.every((argument) => {
39+
if (argument.type === 'ObjectExpression') {
40+
const keys = new Set(
41+
argument.properties
42+
.filter((property) => property.type === 'Property')
43+
.map((property) => property.key.name ?? property.key.value)
44+
);
45+
return keys.has('action') && keys.has('resource');
46+
}
47+
return !['Literal', 'ArrayExpression'].includes(argument.type);
48+
})
49+
);
50+
}
51+
52+
/** @type {import('eslint').Rule.RuleModule} */
53+
export default {
54+
meta: {
55+
type: 'problem',
56+
docs: { description: 'Require an explicit access policy on each GraphQL handler' },
57+
schema: [],
58+
messages: {
59+
missing:
60+
'GraphQL handlers require @UsePermissions(...), @Public(), or @Authenticated() on the method. @UseGuards alone does not declare permissions.',
61+
empty: '@UsePermissions must declare an action and resource; empty permission metadata is denied at runtime.',
62+
},
63+
},
64+
create(context) {
65+
return {
66+
MethodDefinition(node) {
67+
const decorators = (node.decorators ?? [])
68+
.map((decorator) => importedDecorator(context, decorator))
69+
.filter(Boolean);
70+
if (
71+
!decorators.some(
72+
({ source, name }) => source === '@nestjs/graphql' && graphqlHandlers.has(name)
73+
)
74+
)
75+
return;
76+
const permissions = decorators.filter(
77+
({ source, name }) => permissionSources.has(source) && name === 'UsePermissions'
78+
);
79+
if (permissions.length) {
80+
if (permissions.some(({ call }) => !hasPermissionArgument(call))) {
81+
context.report({ node, messageId: 'empty' });
82+
}
83+
return;
84+
}
85+
if (
86+
!decorators.some(
87+
({ source, name }) =>
88+
accessMarkers.has(source) && accessMarkers.get(source) === name
89+
)
90+
) {
91+
context.report({ node, messageId: 'missing' });
92+
}
93+
},
94+
};
95+
},
96+
};
Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
import { fileURLToPath } from 'node:url';
2+
3+
import { ESLint, RuleTester } from 'eslint';
4+
import tseslint from 'typescript-eslint';
5+
import { describe, expect, it } from 'vitest';
6+
7+
import rule from './require-graphql-authorization.mjs';
8+
9+
RuleTester.describe = describe;
10+
RuleTester.it = it;
11+
const tester = new RuleTester({ languageOptions: { parser: tseslint.parser } });
12+
const handlerImports = "import { Query, Mutation, Subscription, ResolveField } from '@nestjs/graphql';";
13+
const permissionsImport =
14+
"import { UsePermissions } from '@unraid/shared/use-permissions.directive.js';";
15+
const publicImport = "import { Public } from '@app/unraid-api/auth/public.decorator.js';";
16+
const authenticatedImport =
17+
"import { Authenticated } from '@app/unraid-api/auth/authenticated.decorator.js';";
18+
const permission = "@UsePermissions({ action: 'READ_ANY', resource: 'CONFIG' })";
19+
20+
const code = (decorators: string, imports = permissionsImport) => `${handlerImports} ${imports}
21+
class Resolver { ${decorators} arbitraryName() { return true; } }`;
22+
23+
tester.run('require-graphql-authorization', rule, {
24+
valid: [
25+
...['Query', 'Mutation', 'Subscription', 'ResolveField'].map((name) =>
26+
code(`@${name}(() => Boolean) ${permission}`)
27+
),
28+
code(`${permission} @Query(() => Boolean)`),
29+
code('@Query(() => Boolean) @Public()', publicImport),
30+
code('@Mutation(() => Boolean) @Authenticated()', authenticatedImport),
31+
code('@Query(() => Boolean) @UsePermissions(permission)', permissionsImport),
32+
code(`@Query(() => Boolean) ${permission}`, "import { UsePermissions } from 'nest-authz';"),
33+
"import { Query as Read } from '@nestjs/graphql'; import { UsePermissions as Policy } from 'nest-authz'; class R { @Read(() => Boolean) @Policy({ action: 'READ_ANY', resource: 'CONFIG' }) value() {} }",
34+
"import * as gql from '@nestjs/graphql'; import * as auth from 'nest-authz'; class R { @gql.Query(() => Boolean) @auth.UsePermissions({ action: 'READ_ANY', resource: 'CONFIG' }) value() {} }",
35+
"import * as gql from '@nestjs/graphql'; import * as auth from 'nest-authz'; class R { @(gql['Query'])(() => Boolean) @(auth['UsePermissions'])({ action: 'READ_ANY', resource: 'CONFIG' }) value() {} }",
36+
code(''),
37+
"import { Query } from 'unrelated'; class R { @Query() value() {} }",
38+
],
39+
invalid: [
40+
...['Query', 'Mutation', 'Subscription', 'ResolveField'].map((name) => ({
41+
code: code(`@${name}(() => Boolean)`),
42+
errors: [{ messageId: 'missing' }],
43+
})),
44+
{
45+
code: "import { Query as Read } from '@nestjs/graphql'; class R { @Read(() => Boolean) renamed() {} }",
46+
errors: [{ messageId: 'missing' }],
47+
},
48+
{
49+
code: "import * as gql from '@nestjs/graphql'; class R { @gql.Mutation(() => Boolean) renamed() {} }",
50+
errors: [{ messageId: 'missing' }],
51+
},
52+
{
53+
code: code('@Query(() => Boolean) @UseGuards(AuthZGuard)'),
54+
errors: [{ messageId: 'missing' }],
55+
},
56+
{
57+
code: code('@Query(() => Boolean) @Public()', "import { Public } from 'unrelated';"),
58+
errors: [{ messageId: 'missing' }],
59+
},
60+
{
61+
code: `${handlerImports} ${publicImport} @Public() class R { @Query(() => Boolean) value() {} }`,
62+
errors: [{ messageId: 'missing' }],
63+
},
64+
{
65+
code: `${handlerImports} ${permissionsImport} function factory(UsePermissions) { return class { @Query(() => Boolean) ${permission} value() {} }; }`,
66+
errors: [{ messageId: 'missing' }],
67+
},
68+
...['', '{}', '[]', 'null', "{ action: 'READ_ANY' }"].map((args) => ({
69+
code: code(`@Query(() => Boolean) @UsePermissions(${args})`),
70+
errors: [{ messageId: 'empty' }],
71+
})),
72+
{
73+
code: code(
74+
'@Query(() => Boolean) @Public() @UsePermissions()',
75+
`${publicImport} ${permissionsImport}`
76+
),
77+
errors: [{ messageId: 'empty' }],
78+
},
79+
],
80+
});
81+
82+
// Loading the full ESLint config includes TypeScript transpilation and plugin startup.
83+
// Allow for that cold start while CI runs coverage across packages concurrently.
84+
describe('authorization lint configuration', { timeout: 30_000 }, () => {
85+
const cwd = fileURLToPath(new URL('..', import.meta.url));
86+
it.each([
87+
['.eslintrc.ts', 'src/new-handler.ts'],
88+
['eslint/graphql-authorization.config.mjs', 'api/src/new-handler.ts'],
89+
[
90+
'eslint/graphql-authorization.config.mjs',
91+
'packages/unraid-api-plugin-connect/src/new-handler.ts',
92+
],
93+
])('rejects an unguarded endpoint using %s at %s', async (config, filePath) => {
94+
const eslint = new ESLint({
95+
cwd: config === '.eslintrc.ts' ? cwd : fileURLToPath(new URL('../..', import.meta.url)),
96+
overrideConfigFile: fileURLToPath(new URL(`../${config}`, import.meta.url)),
97+
});
98+
const results = await eslint.lintText(code('@Mutation(() => Boolean)'), { filePath });
99+
expect(
100+
results
101+
.flatMap((result) => result.messages)
102+
.filter((message) => message.ruleId === 'unraid-auth/require-graphql-authorization')
103+
).toHaveLength(1);
104+
});
105+
it('keeps deliberate test fixtures outside the production policy', async () => {
106+
const eslint = new ESLint({ cwd, overrideConfigFile: '.eslintrc.ts' });
107+
const results = await eslint.lintText(code('@Query(() => Boolean)'), {
108+
filePath: 'src/negative-fixture.spec.ts',
109+
});
110+
expect(
111+
results
112+
.flatMap((result) => result.messages)
113+
.filter((message) => message.ruleId === 'unraid-auth/require-graphql-authorization')
114+
).toHaveLength(0);
115+
});
116+
});

‎api/package.json‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,9 @@
3333
"// Internationalization": "",
3434
"i18n:extract": "node ./scripts/extract-translations.mjs",
3535
"// Code Quality": "",
36-
"lint": "eslint --config .eslintrc.ts src/",
37-
"lint:fix": "eslint --fix --config .eslintrc.ts src/",
36+
"lint": "eslint --config .eslintrc.ts src/ && pnpm lint:authorization",
37+
"lint:authorization": "cd .. && eslint --config api/eslint/graphql-authorization.config.mjs api/src/ packages/unraid-api-plugin-*/src/",
38+
"lint:fix": "eslint --fix --config .eslintrc.ts src/ && pnpm lint:authorization",
3839
"pretype-check": "pnpm --filter @unraid/shared build",
3940
"type-check": "tsc --noEmit",
4041
"// Testing": "",

‎api/src/unraid-api/app/__test__/app.module.integration.spec.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,13 @@ import { INestApplication } from '@nestjs/common';
22
import { FastifyAdapter, NestFastifyApplication } from '@nestjs/platform-fastify';
33
import { Test, TestingModule } from '@nestjs/testing';
44

5-
import { AuthZGuard } from 'nest-authz';
65
import request from 'supertest';
76
import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest';
87

98
import { AppModule } from '@app/unraid-api/app/app.module.js';
109
import { AuthService } from '@app/unraid-api/auth/auth.service.js';
1110
import { AuthenticationGuard } from '@app/unraid-api/auth/authentication.guard.js';
11+
import { AuthorizationGuard } from '@app/unraid-api/auth/authorization.guard.js';
1212

1313
// Mock the store before importing it
1414
vi.mock('@app/store/index.js', () => ({
@@ -76,7 +76,7 @@ describe('AppModule Integration Tests', () => {
7676
canActivate: () => true,
7777
})
7878
// Override authorization guard
79-
.overrideGuard(AuthZGuard)
79+
.overrideGuard(AuthorizationGuard)
8080
.useValue({
8181
canActivate: () => true,
8282
})

‎api/src/unraid-api/app/app.module.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,14 @@ import { APP_GUARD } from '@nestjs/core';
44
import { ScheduleModule } from '@nestjs/schedule';
55
import { ThrottlerModule } from '@nestjs/throttler';
66

7-
import { AuthZGuard } from 'nest-authz';
87
import { LoggerModule } from 'nestjs-pino';
98

109
import { apiLogger } from '@app/core/log.js';
1110
import { LOG_LEVEL } from '@app/environment.js';
1211
import { PubSubModule } from '@app/unraid-api/app/pubsub.module.js';
1312
import { AuthModule } from '@app/unraid-api/auth/auth.module.js';
1413
import { AuthenticationGuard } from '@app/unraid-api/auth/authentication.guard.js';
14+
import { AuthorizationGuard } from '@app/unraid-api/auth/authorization.guard.js';
1515
import { LegacyConfigModule } from '@app/unraid-api/config/legacy-config.module.js';
1616
import { CronModule } from '@app/unraid-api/cron/cron.module.js';
1717
import { JobModule } from '@app/unraid-api/cron/job.module.js';
@@ -67,7 +67,7 @@ import { UnraidFileModifierModule } from '@app/unraid-api/unraid-file-modifier/u
6767
},
6868
{
6969
provide: APP_GUARD,
70-
useClass: AuthZGuard,
70+
useClass: AuthorizationGuard,
7171
},
7272
],
7373
})
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
import { SetMetadata } from '@nestjs/common';
2+
3+
export const IS_AUTHENTICATED_ENDPOINT_KEY = 'isAuthenticatedEndpoint';
4+
5+
// For empty mutation namespaces; their child handlers enforce resource permissions.
6+
export const Authenticated = (): MethodDecorator => SetMetadata(IS_AUTHENTICATED_ENDPOINT_KEY, true);

0 commit comments

Comments
 (0)