- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 9
 
Fix/broken authorization #1297
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
          
     Merged
      
      
    
  
     Merged
                    Fix/broken authorization #1297
Changes from all commits
      Commits
    
    
            Show all changes
          
          
            13 commits
          
        
        Select commit
          Hold shift + click to select a range
      
      6a50939
              
                Basic change auth error throw method
              
              
                7dev7urandom 4e9c2b8
              
                Create eslint rule to prevent accidental missing authz checks
              
              
                7dev7urandom 6eb8aa7
              
                Add simple suggestions to eslint rule
              
              
                7dev7urandom f31d308
              
                Halt responses when security is not handled
              
              
                7dev7urandom 305ee35
              
                Add auth to every route, fix linting, remove old code
              
              
                7dev7urandom 6e2cb46
              
                Change /projects checks in line with #1275
              
              
                FyreByrd 017416d
              
                Fix coderabbit errors
              
              
                7dev7urandom 1d99692
              
                Add login redirect flow
              
              
                7dev7urandom 42fc2a8
              
                Fix remaining review comments
              
              
                7dev7urandom b4488d2
              
                Fix more coderabbit errors
              
              
                FyreByrd 1ea0177
              
                Fix review issues
              
              
                7dev7urandom 684563d
              
                Fix mismatched auth checks
              
              
                FyreByrd ae79ca5
              
                Add 404 if user not found
              
              
                FyreByrd File filter
Filter by extension
Conversations
          Failed to load comments.   
        
        
          
      Loading
        
  Jump to
        
          Jump to file
        
      
      
          Failed to load files.   
        
        
          
      Loading
        
  Diff view
Diff view
          Some comments aren't visible on the classic Files Changed page.
        
There are no files selected for viewing
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
              | Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,208 @@ | ||
| import { ESLintUtils } from '@typescript-eslint/utils'; | ||
| // Walk the AST to find a node that matches the callback, up to maxDepth levels deep | ||
| // This is a depth-first search and could be improved | ||
| function walkFind(node, callback, maxDepth = 10) { | ||
| if (maxDepth <= 0) | ||
| return null; | ||
| if (node && typeof node !== 'object') | ||
| return null; | ||
| if (callback(node)) | ||
| return node; | ||
| for (const key in node) { | ||
| if (key === 'parent') | ||
| continue; | ||
| const val = node[key]; | ||
| if (Array.isArray(val)) { | ||
| for (const child of val) { | ||
| const result = walkFind(child, callback, maxDepth - 1); | ||
| if (result) | ||
| return result; | ||
| } | ||
| } | ||
| else { | ||
| const result = walkFind(val, callback, maxDepth - 1); | ||
| if (result) | ||
| return result; | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
| function blockStatementIncludesSecurityCall(block) { | ||
| const nodeIsSecurityCall = (node) => { | ||
| // Valid check if the node is a CallExpression of the form: | ||
| // *.locals.security.require*() | ||
| // or locals.security.require*() | ||
| if (node?.type === 'CallExpression' && node.callee.type === 'MemberExpression') { | ||
| const obj = node.callee.object; | ||
| const prop = node.callee.property; | ||
| if (obj.type === 'MemberExpression' && | ||
| // object is either *.locals or just locals | ||
| ((obj.object.type === 'MemberExpression' && | ||
| obj.object.property.type === 'Identifier' && | ||
| obj.object.property.name === 'locals') || | ||
| (obj.object.type === 'Identifier' && obj.object.name === 'locals')) && | ||
| // locals.security | ||
| obj.property.type === 'Identifier' && | ||
| obj.property.name === 'security' && | ||
| prop.type === 'Identifier' && | ||
| prop.name.startsWith('require') // ex. requireAuthenticated, requireRole, etc. | ||
| ) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| }; | ||
| return !!block.body.find((n) => !!walkFind(n, nodeIsSecurityCall, 6)); | ||
| } | ||
| export default ESLintUtils.RuleCreator(() => '')({ | ||
| name: 'require-security-check', | ||
| meta: { | ||
| type: 'problem', | ||
| hasSuggestions: true, | ||
| docs: { | ||
| description: 'Require calling event.locals.security.* in every server load/action' | ||
| }, | ||
| schema: [], | ||
| messages: { | ||
| missingSecurityCheck: 'Missing call to requireAuthenticated() or similar security check. This must be of the form locals.security.require*()', | ||
| unexpectedFunction: 'Unexpected export function type {{type}}. Expected ArrowFunctionExpression, FunctionExpression, TSSatisfiesExpression or ObjectExpression.', | ||
| suggestAuthCall: 'Require authentication by adding "event.locals.security.requireAuthenticated();" to the start of the function body.', | ||
| suggestNoAuthCall: 'Mark this function as not requiring any auth by adding "event.locals.security.requireNothing();" to the start of the function body.' | ||
| } | ||
| }, | ||
| defaultOptions: [], | ||
| create(context) { | ||
| return { | ||
| ExportNamedDeclaration(node) { | ||
| // Application of this rule is limited to: | ||
| // export const load = async (event) => { ... } | ||
| // export const actions = { default: async (event) => { ... } } | ||
| // export function load(event) { ... } | ||
| // export POST/GET/PUT/etc. function for endpoints in +server.ts files | ||
| // Only check .server.ts and +server.ts files | ||
| if (!context.filename.endsWith('.server.ts') && !context.filename.endsWith('+server.ts')) | ||
| return; | ||
| // Find the function we are interested in | ||
| let functionExport; | ||
| // load or actions in .server.ts files | ||
| if (context.filename.endsWith('.server.ts') && node.declaration) { | ||
| if (node.declaration.type === 'FunctionDeclaration') { | ||
| if (node.declaration.id?.name === 'load') { | ||
| functionExport = node.declaration; | ||
| } | ||
| } | ||
| else { | ||
| functionExport = node.declaration.declarations?.find((decl) => { | ||
| return (decl.id.type === 'Identifier' && | ||
| (decl.id.name === 'load' || decl.id.name === 'actions')); | ||
| }); | ||
| } | ||
| // endpoint functions in +server.ts files | ||
| } | ||
| else if (context.filename.endsWith('+server.ts') && node.declaration) { | ||
| if (node.declaration.type === 'FunctionDeclaration') { | ||
| // export function POST/GET/PUT/etc. ... | ||
| if (node.declaration.id && | ||
| ['POST', 'GET', 'PUT', 'DELETE', 'PATCH'].includes(node.declaration.id.name)) { | ||
| functionExport = node.declaration; | ||
| } | ||
| } | ||
| else { | ||
| functionExport = node.declaration.declarations?.find((decl) => { | ||
| return (decl.id.type === 'Identifier' && | ||
| ['POST', 'GET', 'PUT', 'DELETE', 'PATCH'].includes(decl.id.name)); | ||
| }); | ||
| } | ||
| } | ||
| // This export is not a load, actions, or endpoint function | ||
| if (!functionExport) | ||
| return; | ||
| const blockStatements = []; | ||
| if (functionExport.id.name === 'actions') { | ||
| // This is the actions object, so find all the functions in its properties | ||
| const objectExpression = functionExport.type === 'VariableDeclarator' | ||
| ? walkFind(functionExport.init, (n) => n.type === 'ObjectExpression', 4) | ||
| : null; | ||
| if (functionExport.type !== 'VariableDeclarator' || !objectExpression) { | ||
| context.report({ | ||
| node: node.declaration, | ||
| data: { type: functionExport.type }, | ||
| messageId: 'unexpectedFunction' | ||
| }); | ||
| return; | ||
| } | ||
| const properties = objectExpression.properties.filter((p) => p.type === 'Property'); | ||
| for (const prop of properties) { | ||
| if (prop.value.type === 'ArrowFunctionExpression' || | ||
| prop.value.type === 'FunctionExpression') { | ||
| // There should be a block statement (function body) within a couple levels | ||
| const block = walkFind(prop.value, (n) => n.type === 'BlockStatement', 4); | ||
| if (block) | ||
| blockStatements.push(block); | ||
| } | ||
| else { | ||
| context.report({ | ||
| node: prop, | ||
| data: { type: prop.value.type }, | ||
| messageId: 'unexpectedFunction' | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| else if ((!functionExport?.init || | ||
| !['ArrowFunctionExpression', 'FunctionExpression', 'TSSatisfiesExpression'].includes(functionExport.init.type)) && | ||
| node.declaration.type !== 'FunctionDeclaration') { | ||
| // Unexpected type of export | ||
| context.report({ | ||
| node: node.declaration, | ||
| data: { type: functionExport.init?.type }, | ||
| messageId: 'unexpectedFunction' | ||
| }); | ||
| return; | ||
| } | ||
| else { | ||
| // This is a single function (load or endpoint), so find its body | ||
| // There should be a block statement (function body) within a couple levels | ||
| const blockStatement = walkFind(functionExport.init || | ||
| functionExport, (n) => n.type === 'BlockStatement', 4); | ||
| if (blockStatement) | ||
| blockStatements.push(blockStatement); | ||
| else { | ||
| context.report({ | ||
| node: node.declaration, | ||
| data: { type: functionExport.type }, | ||
| messageId: 'unexpectedFunction' | ||
| }); | ||
| return; | ||
| } | ||
| } | ||
| blockStatements.forEach((bs) => { | ||
| if (!blockStatementIncludesSecurityCall(bs)) { | ||
| // No security check found in this function's block statement | ||
| // Flag an error on the parent node of the block statement (the whole function) | ||
| context.report({ | ||
| node: bs.parent, | ||
| messageId: 'missingSecurityCheck', | ||
| suggest: [ | ||
| { | ||
| messageId: 'suggestAuthCall', | ||
| fix: (fixer) => fixer.insertTextBefore(bs.body[0] || bs, 'event.locals.security.requireAuthenticated();\n' + | ||
| (functionExport.id.name === 'actions' | ||
| ? ' ' | ||
| : ' ')) | ||
| }, | ||
| { | ||
| messageId: 'suggestNoAuthCall', | ||
| fix: (fixer) => fixer.insertTextBefore(bs.body[0] || bs, 'event.locals.security.requireNothing();\n' + | ||
| (functionExport.id.name === 'actions' | ||
| ? ' ' | ||
| : ' ')) | ||
| } | ||
| ] | ||
| }); | ||
| } | ||
| }); | ||
| } | ||
| }; | ||
| } | ||
| }); | ||
      
      Oops, something went wrong.
        
    
  
      
      Oops, something went wrong.
        
    
  
  Add this suggestion to a batch that can be applied as a single commit.
  This suggestion is invalid because no changes were made to the code.
  Suggestions cannot be applied while the pull request is closed.
  Suggestions cannot be applied while viewing a subset of changes.
  Only one suggestion per line can be applied in a batch.
  Add this suggestion to a batch that can be applied as a single commit.
  Applying suggestions on deleted lines is not supported.
  You must change the existing code in this line in order to create a valid suggestion.
  Outdated suggestions cannot be applied.
  This suggestion has been applied or marked resolved.
  Suggestions cannot be applied from pending reviews.
  Suggestions cannot be applied on multi-line comments.
  Suggestions cannot be applied while the pull request is queued to merge.
  Suggestion cannot be applied right now. Please check back later.
  
    
  
    
Uh oh!
There was an error while loading. Please reload this page.