-
Notifications
You must be signed in to change notification settings - Fork 119
Validate and sanitize string literals in runtime expression evaluation #14851
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
+84
−3
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
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
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 | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -185,12 +185,36 @@ function isSafeExpression(expr) { | |||||||||
|
|
||||||||||
| // Check if right side is a literal string (single, double, or backtick quotes) | ||||||||||
| const isStringLiteral = /^(['"`]).*\1$/.test(rightExpr); | ||||||||||
| if (isStringLiteral) { | ||||||||||
| // Validate string literal content for security | ||||||||||
| const contentMatch = rightExpr.match(/^(['"`])(.+)\1$/); | ||||||||||
| if (contentMatch) { | ||||||||||
| const content = contentMatch[2]; | ||||||||||
|
|
||||||||||
| // Reject nested expressions | ||||||||||
| if (content.includes("${{") || content.includes("}}")) { | ||||||||||
| return false; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Reject escape sequences that could hide keywords | ||||||||||
| if (/\\[xu][\da-fA-F]/.test(content) || /\\[0-7]{1,3}/.test(content)) { | ||||||||||
| return false; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Reject zero-width characters | ||||||||||
| if (/[\u200B-\u200D\uFEFF]/.test(content)) { | ||||||||||
| return false; | ||||||||||
| } | ||||||||||
| } | ||||||||||
| return true; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Check if right side is a number literal | ||||||||||
| const isNumberLiteral = /^-?\d+(\.\d+)?$/.test(rightExpr); | ||||||||||
| // Check if right side is a boolean literal | ||||||||||
| const isBooleanLiteral = rightExpr === "true" || rightExpr === "false"; | ||||||||||
|
|
||||||||||
| if (isStringLiteral || isNumberLiteral || isBooleanLiteral) { | ||||||||||
| if (isNumberLiteral || isBooleanLiteral) { | ||||||||||
| return true; | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
@@ -230,7 +254,9 @@ function evaluateExpression(expr) { | |||||||||
| // If right side is a literal, extract and return it | ||||||||||
| const stringLiteralMatch = rightExpr.match(/^(['"`])(.+)\1$/); | ||||||||||
|
Comment on lines
254
to
255
|
||||||||||
| // If right side is a literal, extract and return it | |
| const stringLiteralMatch = rightExpr.match(/^(['"`])(.+)\1$/); | |
| // If right side is a literal, extract and return it (including empty string literals) | |
| const stringLiteralMatch = rightExpr.match(/^(['"`])(.*)\1$/); |
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 |
|---|---|---|
|
|
@@ -207,6 +207,44 @@ describe("runtime_import", () => { | |
| // 5 levels (max) | ||
| expect(isSafeExpression("needs.job.outputs.foo.bar")).toBe(!0); | ||
| }); | ||
| it("should reject string literals with nested expressions", () => { | ||
| // Reject ${{ markers in string literals | ||
| expect(isSafeExpression("inputs.value || '${{ secrets.TOKEN }}'")).toBe(!1); | ||
| expect(isSafeExpression("inputs.value || 'text ${{ expr }}'")).toBe(!1); | ||
| // Reject }} markers in string literals | ||
| expect(isSafeExpression("inputs.value || 'text }} more'")).toBe(!1); | ||
| // Reject both markers | ||
| expect(isSafeExpression("inputs.value || '${{ }} combined'")).toBe(!1); | ||
| }); | ||
| it("should reject string literals with escape sequences", () => { | ||
| // Reject hex escape sequences (\x) | ||
| expect(isSafeExpression("inputs.value || '\\x41'")).toBe(!1); | ||
| expect(isSafeExpression("inputs.value || 'test\\x20value'")).toBe(!1); | ||
| // Reject unicode escape sequences (\u) | ||
| expect(isSafeExpression("inputs.value || '\\u0041'")).toBe(!1); | ||
| expect(isSafeExpression("inputs.value || 'test\\u0020value'")).toBe(!1); | ||
| // Reject octal escape sequences | ||
| expect(isSafeExpression("inputs.value || '\\101'")).toBe(!1); | ||
| expect(isSafeExpression("inputs.value || 'test\\040value'")).toBe(!1); | ||
|
Comment on lines
+219
to
+228
|
||
| }); | ||
| it("should reject string literals with zero-width characters", () => { | ||
| // Reject zero-width space (U+200B) | ||
| expect(isSafeExpression("inputs.value || 'test\u200Bvalue'")).toBe(!1); | ||
| // Reject zero-width non-joiner (U+200C) | ||
| expect(isSafeExpression("inputs.value || 'test\u200Cvalue'")).toBe(!1); | ||
| // Reject zero-width joiner (U+200D) | ||
| expect(isSafeExpression("inputs.value || 'test\u200Dvalue'")).toBe(!1); | ||
| // Reject zero-width no-break space (U+FEFF) | ||
| expect(isSafeExpression("inputs.value || 'test\uFEFFvalue'")).toBe(!1); | ||
| }); | ||
| it("should allow safe string literals", () => { | ||
| // Normal strings should still work | ||
| expect(isSafeExpression("inputs.value || 'normal string'")).toBe(!0); | ||
| expect(isSafeExpression("inputs.value || 'with-dashes_and_underscores'")).toBe(!0); | ||
| // Safe escape sequences like \n, \t should work | ||
| expect(isSafeExpression("inputs.value || 'line\\nbreak'")).toBe(!0); | ||
| expect(isSafeExpression("inputs.value || 'tab\\there'")).toBe(!0); | ||
| }); | ||
|
Comment on lines
+240
to
+247
|
||
| }), | ||
| describe("evaluateExpression", () => { | ||
| beforeEach(() => { | ||
|
|
@@ -289,6 +327,24 @@ describe("runtime_import", () => { | |
| const outOfBounds = evaluateExpression("github.event.release.assets[999].id"); | ||
| expect(outOfBounds).toContain("${{"); | ||
| }); | ||
| it("should escape $ and { in extracted string literals", () => { | ||
| // Test escaping of $ character | ||
| expect(evaluateExpression("inputs.missing || 'test$value'")).toBe("test\\$value"); | ||
| expect(evaluateExpression("inputs.missing || 'multiple$$$signs'")).toBe("multiple\\$\\$\\$signs"); | ||
| // Test escaping of { character | ||
| expect(evaluateExpression("inputs.missing || 'test{value'")).toBe("test\\{value"); | ||
| expect(evaluateExpression("inputs.missing || 'multiple{{{braces'")).toBe("multiple\\{\\{\\{braces"); | ||
| // Test escaping of both $ and { | ||
| expect(evaluateExpression("inputs.missing || 'test${value}'")).toBe("test\\$\\{value}"); | ||
| expect(evaluateExpression("inputs.missing || '${combined}'")).toBe("\\$\\{combined}"); | ||
| }); | ||
| it("should not escape characters in non-literal expressions", () => { | ||
| // When left side is defined, should not escape | ||
| expect(evaluateExpression("inputs.repository || 'default'")).toBe("testorg/testrepo"); | ||
| // Number and boolean literals don't need escaping | ||
| expect(evaluateExpression("inputs.missing || 42")).toBe("42"); | ||
| expect(evaluateExpression("inputs.missing || true")).toBe("true"); | ||
| }); | ||
| }), | ||
| describe("processRuntimeImport", () => { | ||
| (it("should read and return file content", async () => { | ||
|
|
||
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex pattern
/\\[xu][\da-fA-F]/only matches one hex digit after\xor\u, but JavaScript escape sequences require exactly 2 hex digits for\x(e.g.,\x41) and exactly 4 hex digits for\u(e.g.,\u0041). The pattern should be/\\x[\da-fA-F]{2}|\\u[\da-fA-F]{4}/to match complete hex and unicode escape sequences. As currently written, it may miss incomplete sequences or flag partial matches incorrectly.