Skip to content
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.

Sign up for GitHub

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

fix(graphqlsp): Fix unchecked index accesses #335

Merged
merged 3 commits into from
Jun 26, 2024
Merged

fix(graphqlsp): Fix unchecked index accesses #335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a0e9b9
Update tsconfig.json
kitten Jun 26, 2024
d584b97
Fix up index accesses or annotate as guaranteed non-undefined
kitten Jun 26, 2024
f8bf406
Add changeset
kitten Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/dry-moose-camp.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@0no-co/graphqlsp': patch
---

Address potential crashes on malformed TypeScript AST input (such as missing function arguments where they were previously assumed to be passed)
9 changes: 7 additions & 2 deletions packages/graphqlsp/src/ast/checks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export const isTadaGraphQLCall = (
return false;
} else if (node.arguments.length < 1 || node.arguments.length > 2) {
return false;
} else if (!ts.isStringLiteralLike(node.arguments[0])) {
} else if (!ts.isStringLiteralLike(node.arguments[0]!)) {
return false;
}
return checker ? isTadaGraphQLFunction(node.expression, checker) : false;
Expand All @@ -70,11 +70,16 @@ export const isTadaPersistedCall = (
}
};

// As per check in `isGraphQLCall()` below, enforces arguments length
export type GraphQLCallNode = ts.CallExpression & {
arguments: [ts.Expression] | [ts.Expression, ts.Expression];
};

/** Checks if node is a gql.tada or regular graphql() call */
export const isGraphQLCall = (
node: ts.Node,
checker: ts.TypeChecker | undefined
): node is ts.CallExpression => {
): node is GraphQLCallNode => {
return (
ts.isCallExpression(node) &&
node.arguments.length >= 1 &&
Expand Down
6 changes: 3 additions & 3 deletions packages/graphqlsp/src/ast/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,8 @@ function unrollFragment(
element.getStart()
);

if (!definitions || !definitions.length) return fragments;

const [fragment] = definitions;
const fragment = definitions && definitions[0];
if (!fragment) return fragments;

const externalSource = getSource(info, fragment.fileName);
if (!externalSource) return fragments;
Expand Down Expand Up @@ -263,6 +262,7 @@ export function getAllFragments(
if (
ts.isVariableStatement(node) &&
node.declarationList &&
node.declarationList.declarations[0] &&
node.declarationList.declarations[0].name.getText() === 'documents'
) {
const [declaration] = node.declarationList.declarations;
Expand Down
5 changes: 3 additions & 2 deletions packages/graphqlsp/src/ast/resolve.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,10 @@ export function resolveTemplate(
filename,
span.expression.getStart()
);
if (!definitions || !definitions.length) return;

const def = definitions[0];
const def = definitions && definitions[0];
if (!def) return;

const src = getSource(info, def.fileName);
if (!src) return;

Expand Down
2 changes: 1 addition & 1 deletion packages/graphqlsp/src/ast/token.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export const getToken = (
}
}

cPos += input[line].length + 1;
cPos += input[line]!.length + 1;
}

return foundToken;
Expand Down
2 changes: 1 addition & 1 deletion packages/graphqlsp/src/autoComplete.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ function runOnlineParser(
let stream: CharacterStream = new CharacterStream('');

for (let i = 0; i < lines.length; i++) {
stream = new CharacterStream(lines[i]);
stream = new CharacterStream(lines[i]!);
while (!stream.eol()) {
style = parser.token(stream, state);
const code = callback(stream, state, style, i);
Expand Down
66 changes: 24 additions & 42 deletions packages/graphqlsp/src/checkImports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ export const getColocatedFragmentNames = (
source.fileName,
imp.importClause.name.getStart()
);
if (definitions && definitions.length) {
const [def] = definitions;
const def = definitions && definitions[0];
if (def) {
if (def.fileName.includes('node_modules')) return;

const externalSource = getSource(info, def.fileName);
Expand All @@ -51,22 +51,16 @@ export const getColocatedFragmentNames = (
);

const names = fragmentsForImport.map(fragment => fragment.name.value);
if (
names.length &&
!importSpecifierToFragments[imp.moduleSpecifier.getText()]
) {
importSpecifierToFragments[imp.moduleSpecifier.getText()] = {
const key = imp.moduleSpecifier.getText();
let fragmentsEntry = importSpecifierToFragments[key];
if (names.length && fragmentsEntry) {
fragmentsEntry.fragments = fragmentsEntry.fragments.concat(names);
} else if (names.length && !fragmentsEntry) {
importSpecifierToFragments[key] = fragmentsEntry = {
start: imp.moduleSpecifier.getStart(),
length: imp.moduleSpecifier.getText().length,
fragments: names,
};
} else if (names.length) {
importSpecifierToFragments[
imp.moduleSpecifier.getText()
].fragments =
importSpecifierToFragments[
imp.moduleSpecifier.getText()
].fragments.concat(names);
}
}
}
Expand All @@ -79,8 +73,8 @@ export const getColocatedFragmentNames = (
source.fileName,
imp.importClause.namedBindings.getStart()
);
if (definitions && definitions.length) {
const [def] = definitions;
const def = definitions && definitions[0];
if (def) {
if (def.fileName.includes('node_modules')) return;

const externalSource = getSource(info, def.fileName);
Expand All @@ -92,22 +86,16 @@ export const getColocatedFragmentNames = (
info
);
const names = fragmentsForImport.map(fragment => fragment.name.value);
if (
names.length &&
!importSpecifierToFragments[imp.moduleSpecifier.getText()]
) {
importSpecifierToFragments[imp.moduleSpecifier.getText()] = {
const key = imp.moduleSpecifier.getText();
let fragmentsEntry = importSpecifierToFragments[key];
if (names.length && fragmentsEntry) {
fragmentsEntry.fragments = fragmentsEntry.fragments.concat(names);
} else if (names.length && !fragmentsEntry) {
importSpecifierToFragments[key] = fragmentsEntry = {
start: imp.moduleSpecifier.getStart(),
length: imp.moduleSpecifier.getText().length,
fragments: names,
};
} else if (names.length) {
importSpecifierToFragments[
imp.moduleSpecifier.getText()
].fragments =
importSpecifierToFragments[
imp.moduleSpecifier.getText()
].fragments.concat(names);
}
}
} else if (
Expand All @@ -119,8 +107,8 @@ export const getColocatedFragmentNames = (
source.fileName,
el.getStart()
);
if (definitions && definitions.length) {
const [def] = definitions;
const def = definitions && definitions[0];
if (def) {
if (def.fileName.includes('node_modules')) return;

const externalSource = getSource(info, def.fileName);
Expand All @@ -134,22 +122,16 @@ export const getColocatedFragmentNames = (
const names = fragmentsForImport.map(
fragment => fragment.name.value
);
if (
names.length &&
!importSpecifierToFragments[imp.moduleSpecifier.getText()]
) {
importSpecifierToFragments[imp.moduleSpecifier.getText()] = {
const key = imp.moduleSpecifier.getText();
let fragmentsEntry = importSpecifierToFragments[key];
if (names.length && fragmentsEntry) {
fragmentsEntry.fragments = fragmentsEntry.fragments.concat(names);
} else if (names.length && !fragmentsEntry) {
importSpecifierToFragments[key] = fragmentsEntry = {
start: imp.moduleSpecifier.getStart(),
length: imp.moduleSpecifier.getText().length,
fragments: names,
};
} else if (names.length) {
importSpecifierToFragments[
imp.moduleSpecifier.getText()
].fragments =
importSpecifierToFragments[
imp.moduleSpecifier.getText()
].fragments.concat(names);
}
}
});
Expand Down
65 changes: 36 additions & 29 deletions packages/graphqlsp/src/diagnostics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ export function getGraphQLDiagnostics(
: texts.join('-') + schema.version
);

let tsDiagnostics;
let tsDiagnostics: ts.Diagnostic[];
if (cache.has(cacheKey)) {
tsDiagnostics = cache.get(cacheKey)!;
} else {
Expand Down Expand Up @@ -161,8 +161,9 @@ export function getGraphQLDiagnostics(
ref,
start,
length;
if (callExpression.typeArguments) {
const [typeQuery] = callExpression.typeArguments;
const typeQuery =
callExpression.typeArguments && callExpression.typeArguments[0];
if (typeQuery) {
start = typeQuery.getStart();
length = typeQuery.getEnd() - typeQuery.getStart();

Expand Down Expand Up @@ -228,6 +229,7 @@ export function getGraphQLDiagnostics(
if (
!initializer ||
!ts.isCallExpression(initializer) ||
!initializer.arguments[0] ||
!ts.isStringLiteralLike(initializer.arguments[0])
) {
// TODO: we can make this check more stringent where we also parse and resolve
Expand Down Expand Up @@ -261,7 +263,8 @@ export function getGraphQLDiagnostics(
info,
initializer.arguments[0],
foundFilename,
ts.isArrayLiteralExpression(initializer.arguments[1])
initializer.arguments[1] &&
ts.isArrayLiteralExpression(initializer.arguments[1])
? initializer.arguments[1]
: undefined
);
Expand Down Expand Up @@ -310,7 +313,7 @@ export function getGraphQLDiagnostics(
fragments: fragmentNames,
start,
length,
} = moduleSpecifierToFragments[moduleSpecifier];
} = moduleSpecifierToFragments[moduleSpecifier]!;
const missingFragments = Array.from(
new Set(fragmentNames.filter(x => !usedFragments.has(x)))
);
Expand Down Expand Up @@ -348,7 +351,7 @@ const runDiagnostics = (
},
schema: SchemaRef,
info: ts.server.PluginCreateInfo
) => {
): ts.Diagnostic[] => {
const filename = source.fileName;
const isCallExpression = info.config.templateIsCallExpression ?? true;

Expand Down Expand Up @@ -429,26 +432,27 @@ const runDiagnostics = (
if (!diag.message.includes('Unknown directive')) return true;

const [message] = diag.message.split('(');
const matches = /Unknown directive "@([^)]+)"/g.exec(message);
const matches =
message && /Unknown directive "@([^)]+)"/g.exec(message);
if (!matches) return true;
const directiveNmae = matches[1];
return !clientDirectives.has(directiveNmae);
const directiveName = matches[1];
return directiveName && !clientDirectives.has(directiveName);
})
.map(x => {
const { start, end } = x.range;

// We add the start.line to account for newline characters which are
// split out
let startChar = startingPosition + start.line;
for (let i = 0; i <= start.line; i++) {
for (let i = 0; i <= start.line && i < lines.length; i++) {
if (i === start.line) startChar += start.character;
else if (lines[i]) startChar += lines[i].length;
else if (lines[i]) startChar += lines[i]!.length;
}

let endChar = startingPosition + end.line;
for (let i = 0; i <= end.line; i++) {
for (let i = 0; i <= end.line && i < lines.length; i++) {
if (i === end.line) endChar += end.character;
else if (lines[i]) endChar += lines[i].length;
else if (lines[i]) endChar += lines[i]!.length;
}

const locatedInFragment = resolvedSpans.find(x => {
Expand Down Expand Up @@ -516,22 +520,25 @@ const runDiagnostics = (
.flat()
.filter(Boolean) as Array<Diagnostic & { length: number; start: number }>;

const tsDiagnostics = diagnostics.map(diag => ({
file: source,
length: diag.length,
start: diag.start,
category:
diag.severity === 2
? ts.DiagnosticCategory.Warning
: ts.DiagnosticCategory.Error,
code:
typeof diag.code === 'number'
? diag.code
: diag.severity === 2
? USING_DEPRECATED_FIELD_CODE
: SEMANTIC_DIAGNOSTIC_CODE,
messageText: diag.message.split('\n')[0],
}));
const tsDiagnostics = diagnostics.map(
diag =>
({
file: source,
length: diag.length,
start: diag.start,
category:
diag.severity === 2
? ts.DiagnosticCategory.Warning
: ts.DiagnosticCategory.Error,
code:
typeof diag.code === 'number'
? diag.code
: diag.severity === 2
? USING_DEPRECATED_FIELD_CODE
: SEMANTIC_DIAGNOSTIC_CODE,
messageText: diag.message.split('\n')[0],
} as ts.Diagnostic)
);

if (isCallExpression) {
const usageDiagnostics =
Expand Down
Loading