Skip to content

Commit 03ffd0c

Browse files
ericscipleericsciple
authored
Add validation for literal text in if conditions (#216)
* Validate literal text in if-condition format expressions * test escaped left brace
1 parent 03d68e8 commit 03ffd0c

File tree

2 files changed

+303
-6
lines changed

2 files changed

+303
-6
lines changed

‎languageservice/src/validate.expressions-literal-text.test.ts‎

Lines changed: 214 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,214 @@
1+
import {DiagnosticSeverity} from "vscode-languageserver-types";
2+
import {registerLogger} from "./log";
3+
import {createDocument} from "./test-utils/document";
4+
import {TestLogger} from "./test-utils/logger";
5+
import {clearCache} from "./utils/workflow-cache";
6+
import {validate} from "./validate";
7+
8+
registerLogger(new TestLogger());
9+
10+
beforeEach(() => {
11+
clearCache();
12+
});
13+
14+
describe("expression literal text in conditions", () => {
15+
describe("job-if", () => {
16+
it("errors when literal text mixed with embedded expression", async () => {
17+
const input = `
18+
on: push
19+
jobs:
20+
build:
21+
if: push == \${{ github.event_name }}
22+
runs-on: ubuntu-latest
23+
steps:
24+
- run: echo hi
25+
`;
26+
const result = await validate(createDocument("wf.yaml", input));
27+
28+
expect(result).toContainEqual(
29+
expect.objectContaining({
30+
message:
31+
"Conditional expression contains literal text outside replacement tokens. This will cause the expression to always evaluate to truthy. Did you mean to put the entire expression inside ${{ }}?",
32+
code: "expression-literal-text-in-condition",
33+
severity: DiagnosticSeverity.Error
34+
})
35+
);
36+
});
37+
38+
it("allows format with only replacement tokens", async () => {
39+
const input = `
40+
on: push
41+
jobs:
42+
build:
43+
if: \${{ format('{0}', github.event_name) }}
44+
runs-on: ubuntu-latest
45+
steps:
46+
- run: echo hi
47+
`;
48+
const result = await validate(createDocument("wf.yaml", input));
49+
50+
expect(result).not.toContainEqual(
51+
expect.objectContaining({
52+
code: "expression-literal-text-in-condition"
53+
})
54+
);
55+
});
56+
57+
it("allows format with only replacement tokens and whitespace", async () => {
58+
const input = `
59+
on: push
60+
jobs:
61+
build:
62+
if: \${{ format('{0}{1}', github.event_name, 'test') }}
63+
runs-on: ubuntu-latest
64+
steps:
65+
- run: echo hi
66+
`;
67+
const result = await validate(createDocument("wf.yaml", input));
68+
69+
// Only replacement tokens, no literal text
70+
expect(result).not.toContainEqual(
71+
expect.objectContaining({
72+
code: "expression-literal-text-in-condition"
73+
})
74+
);
75+
});
76+
77+
it("errors with literal text and replacement tokens mixed", async () => {
78+
const input = `
79+
on: push
80+
jobs:
81+
build:
82+
if: \${{ format('event is {0}', github.event_name) }}
83+
runs-on: ubuntu-latest
84+
steps:
85+
- run: echo hi
86+
`;
87+
const result = await validate(createDocument("wf.yaml", input));
88+
89+
expect(result).toContainEqual(
90+
expect.objectContaining({
91+
message:
92+
"Conditional expression contains literal text outside replacement tokens. This will cause the expression to always evaluate to truthy. Did you mean to put the entire expression inside ${{ }}?",
93+
code: "expression-literal-text-in-condition",
94+
severity: DiagnosticSeverity.Error
95+
})
96+
);
97+
});
98+
99+
it("errors with escaped left brace followed by replacement token", async () => {
100+
const input = `
101+
on: push
102+
jobs:
103+
build:
104+
if: \${{ format('{{{0}', github.event_name) }}
105+
runs-on: ubuntu-latest
106+
steps:
107+
- run: echo hi
108+
`;
109+
const result = await validate(createDocument("wf.yaml", input));
110+
111+
expect(result).toContainEqual(
112+
expect.objectContaining({
113+
message:
114+
"Conditional expression contains literal text outside replacement tokens. This will cause the expression to always evaluate to truthy. Did you mean to put the entire expression inside ${{ }}?",
115+
code: "expression-literal-text-in-condition",
116+
severity: DiagnosticSeverity.Error
117+
})
118+
);
119+
});
120+
});
121+
122+
describe("step-if", () => {
123+
it("errors when literal text mixed with embedded expression", async () => {
124+
const input = `
125+
on: push
126+
jobs:
127+
build:
128+
runs-on: ubuntu-latest
129+
steps:
130+
- if: success == \${{ job.status }}
131+
run: echo hi
132+
`;
133+
const result = await validate(createDocument("wf.yaml", input));
134+
135+
expect(result).toContainEqual(
136+
expect.objectContaining({
137+
message:
138+
"Conditional expression contains literal text outside replacement tokens. This will cause the expression to always evaluate to truthy. Did you mean to put the entire expression inside ${{ }}?",
139+
code: "expression-literal-text-in-condition",
140+
severity: DiagnosticSeverity.Error
141+
})
142+
);
143+
});
144+
145+
it("allows valid expressions", async () => {
146+
const input = `
147+
on: push
148+
jobs:
149+
build:
150+
runs-on: ubuntu-latest
151+
steps:
152+
- if: \${{ success() }}
153+
run: echo hi
154+
`;
155+
const result = await validate(createDocument("wf.yaml", input));
156+
157+
expect(result).not.toContainEqual(
158+
expect.objectContaining({
159+
code: "expression-literal-text-in-condition"
160+
})
161+
);
162+
});
163+
});
164+
165+
describe("snapshot-if", () => {
166+
it("errors when literal text mixed with embedded expression", async () => {
167+
const input = `
168+
on: push
169+
jobs:
170+
build:
171+
runs-on: ubuntu-latest
172+
strategy:
173+
matrix:
174+
os: [ubuntu-latest]
175+
steps:
176+
- run: echo hi
177+
snapshot:
178+
image-name: my-image
179+
if: ubuntu == \${{ matrix.os }}
180+
`;
181+
const result = await validate(createDocument("wf.yaml", input));
182+
183+
expect(result).toContainEqual(
184+
expect.objectContaining({
185+
message:
186+
"Conditional expression contains literal text outside replacement tokens. This will cause the expression to always evaluate to truthy. Did you mean to put the entire expression inside ${{ }}?",
187+
code: "expression-literal-text-in-condition",
188+
severity: DiagnosticSeverity.Error
189+
})
190+
);
191+
});
192+
});
193+
194+
describe("non-if fields", () => {
195+
it("does not error for format in run", async () => {
196+
const input = `
197+
on: push
198+
jobs:
199+
build:
200+
runs-on: ubuntu-latest
201+
steps:
202+
- run: echo \${{ format('Event is {0}', github.event_name) }}
203+
`;
204+
const result = await validate(createDocument("wf.yaml", input));
205+
206+
// Format with literal text is OK outside of if conditions
207+
expect(result).not.toContainEqual(
208+
expect.objectContaining({
209+
code: "expression-literal-text-in-condition"
210+
})
211+
);
212+
});
213+
});
214+
});

‎languageservice/src/validate.ts‎

Lines changed: 89 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
import {Lexer, Parser} from "@actions/expressions";
2-
import {Expr} from "@actions/expressions/ast";
1+
import {Lexer, Parser, data} from "@actions/expressions";
2+
import {Expr, FunctionCall, Literal, Logical} from "@actions/expressions/ast";
33
import {ParseWorkflowResult, WorkflowTemplate, isBasicExpression, isString} from "@actions/workflow-parser";
44
import {ErrorPolicy} from "@actions/workflow-parser/model/convert";
55
import {ensureStatusFunction} from "@actions/workflow-parser/model/converter/if-condition";
@@ -105,7 +105,8 @@ async function additionalValidations(
105105
token,
106106
validationToken.definitionInfo?.allowedContext || [],
107107
config?.contextProviderConfig,
108-
getProviderContext(documentUri, template, root, token.range)
108+
getProviderContext(documentUri, template, root, token.range),
109+
key?.definition?.key
109110
);
110111
}
111112

@@ -213,17 +214,99 @@ function getProviderContext(
213214
return getWorkflowContext(documentUri, template, path);
214215
}
215216

217+
/**
218+
* Checks if a format function contains literal text in its format string.
219+
* This indicates user confusion about how expressions work.
220+
*
221+
* Example: format('push == {0}', github.event_name)
222+
* The literal text "push == " will always evaluate to truthy.
223+
*
224+
* @param expr The expression to check
225+
* @returns true if the expression is a format() call with literal text
226+
*/
227+
function hasFormatWithLiteralText(expr: Expr): boolean {
228+
// If this is a logical AND expression (from ensureStatusFunction wrapping)
229+
// check the right side for the format call
230+
if (expr instanceof Logical && expr.operator.lexeme === "&&" && expr.args.length === 2) {
231+
return hasFormatWithLiteralText(expr.args[1]);
232+
}
233+
234+
if (!(expr instanceof FunctionCall)) {
235+
return false;
236+
}
237+
238+
// Check if this is a format function
239+
if (expr.functionName.lexeme.toLowerCase() !== "format") {
240+
return false;
241+
}
242+
243+
// Check if the first argument is a string literal
244+
if (expr.args.length < 1) {
245+
return false;
246+
}
247+
248+
const firstArg = expr.args[0];
249+
if (!(firstArg instanceof Literal) || firstArg.literal.kind !== data.Kind.String) {
250+
return false;
251+
}
252+
253+
// Get the format string and trim whitespace
254+
const formatString = firstArg.literal.coerceString();
255+
const trimmed = formatString.trim();
256+
257+
// Check if there's literal text (non-replacement tokens) after trimming
258+
let inToken = false;
259+
for (let i = 0; i < trimmed.length; i++) {
260+
if (!inToken && trimmed[i] === "{") {
261+
inToken = true;
262+
} else if (inToken && trimmed[i] === "}") {
263+
inToken = false;
264+
} else if (inToken && trimmed[i] >= "0" && trimmed[i] <= "9") {
265+
// OK - this is a replacement token like {0}, {1}, etc.
266+
} else {
267+
// Found literal text
268+
return true;
269+
}
270+
}
271+
272+
return false;
273+
}
274+
216275
async function validateExpression(
217276
diagnostics: Diagnostic[],
218277
token: BasicExpressionToken,
219278
allowedContext: string[],
220279
contextProviderConfig: ContextProviderConfig | undefined,
221-
workflowContext: WorkflowContext
280+
workflowContext: WorkflowContext,
281+
keyDefinitionKey?: string
222282
) {
283+
const {namedContexts, functions} = splitAllowedContext(allowedContext);
284+
285+
// Check for literal text in if condition
286+
const definitionKey = keyDefinitionKey || token.definitionInfo?.definition?.key;
287+
if (definitionKey === "job-if" || definitionKey === "step-if" || definitionKey === "snapshot-if") {
288+
try {
289+
const l = new Lexer(token.expression);
290+
const lr = l.lex();
291+
const p = new Parser(lr.tokens, namedContexts, functions);
292+
const expr = p.parse();
293+
294+
if (hasFormatWithLiteralText(expr)) {
295+
diagnostics.push({
296+
message:
297+
"Conditional expression contains literal text outside replacement tokens. This will cause the expression to always evaluate to truthy. Did you mean to put the entire expression inside ${{ }}?",
298+
range: mapRange(token.range),
299+
severity: DiagnosticSeverity.Error,
300+
code: "expression-literal-text-in-condition"
301+
});
302+
}
303+
} catch {
304+
// Ignore parse errors here
305+
}
306+
}
307+
223308
// Validate the expression
224309
for (const expression of token.originalExpressions || [token]) {
225-
const {namedContexts, functions} = splitAllowedContext(allowedContext);
226-
227310
let expr: Expr | undefined;
228311

229312
try {

0 commit comments

Comments
 (0)