mirror of
https://github.com/iflytek/skillhub.git
synced 2026-09-10 22:41:02 +00:00
Merge pull request #830 from iflytek/codex/fix/issue-827-secret-expression-regression-20260908
fix(validation): ignore credential expressions
This commit is contained in:
XiaoSeS
GitHub
commit
c9dca27cc2
2 changed files with 296 additions and 5 deletions
|
|
@ -17,6 +17,12 @@ import java.util.regex.Pattern;
|
|||
@Component
|
||||
public class BasicPrePublishValidator implements PrePublishValidator {
|
||||
|
||||
private static final int MIN_GENERIC_SECRET_LENGTH = 12;
|
||||
private static final Pattern ASSIGNMENT_WITH_SENSITIVE_KEY = Pattern.compile(
|
||||
"(?i)(api[_-]?key|access[_-]?key|secret|password|token)\\s*[:=]\\s*"
|
||||
);
|
||||
private static final Pattern IDENTIFIER = Pattern.compile("[A-Za-z_][A-Za-z0-9_]*");
|
||||
private static final Pattern BARE_LITERAL = Pattern.compile("[A-Za-z0-9_\\-]{12,}");
|
||||
private static final Set<String> PLACEHOLDER_MARKERS = Set.of(
|
||||
"your", "example", "sample", "placeholder", "changeme", "replace", "dummy",
|
||||
"mock", "test", "fake", "todo", "xxx", "redacted");
|
||||
|
|
@ -24,10 +30,7 @@ public class BasicPrePublishValidator implements PrePublishValidator {
|
|||
new SecretRule(Pattern.compile("(AKIA[0-9A-Z]{16})"), 1, "cloud access key"),
|
||||
new SecretRule(Pattern.compile("(ghp_[A-Za-z0-9]{20,})"), 1, "GitHub token"),
|
||||
new SecretRule(Pattern.compile("(sk-[A-Za-z0-9]{20,})"), 1, "API key"),
|
||||
new SecretRule(
|
||||
Pattern.compile("(?i)(api[_-]?key|access[_-]?key|secret|password|token)\\s*[:=]\\s*['\\\"]?([A-Za-z0-9_\\-]{12,})"),
|
||||
2,
|
||||
"secret or token")
|
||||
new SecretRule(ASSIGNMENT_WITH_SENSITIVE_KEY, 0, "secret or token")
|
||||
);
|
||||
|
||||
@Override
|
||||
|
|
@ -47,7 +50,11 @@ public class BasicPrePublishValidator implements PrePublishValidator {
|
|||
if (!matcher.find()) {
|
||||
continue;
|
||||
}
|
||||
String matchedValue = matcher.group(rule.valueGroup());
|
||||
String matchedValue = extractMatchedValue(
|
||||
line, matcher, rule, isBareSecretConfiguration(entry.path()));
|
||||
if (matchedValue == null) {
|
||||
continue;
|
||||
}
|
||||
if (isPlaceholderValue(matchedValue)) {
|
||||
continue;
|
||||
}
|
||||
|
|
@ -80,6 +87,13 @@ public class BasicPrePublishValidator implements PrePublishValidator {
|
|||
|| lowerPath.endsWith(".zsh") || lowerPath.endsWith(".bash");
|
||||
}
|
||||
|
||||
private boolean isBareSecretConfiguration(String path) {
|
||||
String lowerPath = path.toLowerCase(Locale.ROOT);
|
||||
return lowerPath.endsWith(".yaml") || lowerPath.endsWith(".yml")
|
||||
|| lowerPath.endsWith(".toml") || lowerPath.endsWith(".ini")
|
||||
|| lowerPath.endsWith(".cfg") || lowerPath.endsWith(".env");
|
||||
}
|
||||
|
||||
private boolean isPlaceholderValue(String value) {
|
||||
if (value == null || value.isBlank()) {
|
||||
return false;
|
||||
|
|
@ -89,5 +103,151 @@ public class BasicPrePublishValidator implements PrePublishValidator {
|
|||
|| value.chars().allMatch(ch -> ch == 'x' || ch == 'X' || ch == '*' || ch == '-');
|
||||
}
|
||||
|
||||
private String extractMatchedValue(
|
||||
String line, Matcher matcher, SecretRule rule, boolean allowBareLiteral) {
|
||||
if (rule.valueGroup() > 0) {
|
||||
return matcher.group(rule.valueGroup());
|
||||
}
|
||||
|
||||
do {
|
||||
GenericValueScan scan = scanGenericValue(line, matcher.end(), allowBareLiteral);
|
||||
if (scan.literal() != null) {
|
||||
return scan.literal();
|
||||
}
|
||||
if (scan.nextSearchIndex() >= line.length()) {
|
||||
return null;
|
||||
}
|
||||
matcher.region(scan.nextSearchIndex(), line.length());
|
||||
} while (matcher.find());
|
||||
return null;
|
||||
}
|
||||
|
||||
private GenericValueScan scanGenericValue(String line, int valueStart, boolean allowBareLiteral) {
|
||||
int start = valueStart;
|
||||
while (start < line.length() && Character.isWhitespace(line.charAt(start))) {
|
||||
start++;
|
||||
}
|
||||
if (start == line.length()) {
|
||||
return new GenericValueScan(null, line.length());
|
||||
}
|
||||
|
||||
QuotedLiteralStart quotedStart = findQuotedLiteralStart(line, start);
|
||||
char first = line.charAt(quotedStart.index());
|
||||
if (first == '\'' || first == '"') {
|
||||
return scanQuotedLiteral(line, quotedStart.index(), first, quotedStart.wrapperDepth());
|
||||
}
|
||||
|
||||
int end = start;
|
||||
while (end < line.length() && !isBareValueTerminator(line, end)) {
|
||||
end++;
|
||||
}
|
||||
String bareValue = line.substring(start, end);
|
||||
if (!allowBareLiteral && IDENTIFIER.matcher(bareValue).matches()) {
|
||||
return new GenericValueScan(null, end);
|
||||
}
|
||||
String literal = BARE_LITERAL.matcher(bareValue).matches() ? bareValue : null;
|
||||
return new GenericValueScan(literal, end);
|
||||
}
|
||||
|
||||
private QuotedLiteralStart findQuotedLiteralStart(String line, int start) {
|
||||
int index = start;
|
||||
int wrapperDepth = 0;
|
||||
while (index < line.length() && line.charAt(index) == '(') {
|
||||
wrapperDepth++;
|
||||
index++;
|
||||
while (index < line.length() && Character.isWhitespace(line.charAt(index))) {
|
||||
index++;
|
||||
}
|
||||
}
|
||||
return index < line.length() && (line.charAt(index) == '\'' || line.charAt(index) == '"')
|
||||
? new QuotedLiteralStart(index, wrapperDepth)
|
||||
: new QuotedLiteralStart(start, 0);
|
||||
}
|
||||
|
||||
private GenericValueScan scanQuotedLiteral(
|
||||
String line, int start, char quote, int wrapperDepth) {
|
||||
boolean escaped = false;
|
||||
for (int i = start + 1; i < line.length(); i++) {
|
||||
char current = line.charAt(i);
|
||||
if (escaped) {
|
||||
escaped = false;
|
||||
continue;
|
||||
}
|
||||
if (current == '\\') {
|
||||
escaped = true;
|
||||
continue;
|
||||
}
|
||||
if (current == quote) {
|
||||
String value = line.substring(start + 1, i);
|
||||
String literal = hasLiteralTerminator(line, i + 1, wrapperDepth)
|
||||
&& value.length() >= MIN_GENERIC_SECRET_LENGTH
|
||||
? value
|
||||
: null;
|
||||
return new GenericValueScan(literal, i + 1);
|
||||
}
|
||||
}
|
||||
return new GenericValueScan(null, line.length());
|
||||
}
|
||||
|
||||
private boolean hasLiteralTerminator(String line, int startIndex, int wrapperDepth) {
|
||||
int index = skipWhitespace(line, startIndex);
|
||||
for (int i = 0; i < wrapperDepth; i++) {
|
||||
if (index == line.length() || line.charAt(index) != ')') {
|
||||
return false;
|
||||
}
|
||||
index = skipWhitespace(line, index + 1);
|
||||
}
|
||||
if (isLiteralTerminatorAt(line, index)) {
|
||||
return true;
|
||||
}
|
||||
|
||||
if (!line.startsWith("as", index)
|
||||
|| index + 2 >= line.length()
|
||||
|| !Character.isWhitespace(line.charAt(index + 2))) {
|
||||
return false;
|
||||
}
|
||||
index = skipWhitespace(line, index + 2);
|
||||
if (!line.startsWith("const", index)
|
||||
|| (index + 5 < line.length()
|
||||
&& Character.isJavaIdentifierPart(line.charAt(index + 5)))) {
|
||||
return false;
|
||||
}
|
||||
return isLiteralTerminatorAt(line, skipWhitespace(line, index + 5));
|
||||
}
|
||||
|
||||
private int skipWhitespace(String line, int startIndex) {
|
||||
int index = startIndex;
|
||||
while (index < line.length() && Character.isWhitespace(line.charAt(index))) {
|
||||
index++;
|
||||
}
|
||||
return index;
|
||||
}
|
||||
|
||||
private boolean isLiteralTerminatorAt(String line, int index) {
|
||||
if (index == line.length()) {
|
||||
return true;
|
||||
}
|
||||
char current = line.charAt(index);
|
||||
return isTrailingDelimiter(current)
|
||||
|| current == '#'
|
||||
|| (current == '/' && index + 1 < line.length() && line.charAt(index + 1) == '/');
|
||||
}
|
||||
|
||||
private boolean isTrailingDelimiter(char value) {
|
||||
return value == ',' || value == ';' || value == ')' || value == '}' || value == ']';
|
||||
}
|
||||
|
||||
private boolean isBareValueTerminator(String line, int index) {
|
||||
char current = line.charAt(index);
|
||||
return Character.isWhitespace(current)
|
||||
|| isTrailingDelimiter(current)
|
||||
|| current == '#'
|
||||
|| (current == '/' && index + 1 < line.length() && line.charAt(index + 1) == '/');
|
||||
}
|
||||
|
||||
private record GenericValueScan(String literal, int nextSearchIndex) {}
|
||||
|
||||
private record QuotedLiteralStart(int index, int wrapperDepth) {}
|
||||
|
||||
private record SecretRule(Pattern pattern, int valueGroup, String label) {}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@ import java.nio.charset.StandardCharsets;
|
|||
import java.util.List;
|
||||
import java.util.Map;
|
||||
|
||||
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
|
||||
import static org.junit.jupiter.api.Assertions.assertFalse;
|
||||
import static org.junit.jupiter.api.Assertions.assertTrue;
|
||||
|
||||
|
|
@ -98,4 +99,134 @@ class BasicPrePublishValidatorTest {
|
|||
|
||||
assertTrue(result.passed());
|
||||
}
|
||||
|
||||
@Test
|
||||
void shouldNotWarnOnRuntimeExpressionsAssignedToSensitiveVariables() {
|
||||
PackageEntry script = new PackageEntry(
|
||||
"scripts/oauth.py",
|
||||
"""
|
||||
refresh_token = token_response.get("refresh_token")
|
||||
client_secret = configured_secret
|
||||
self._client_secret = credentials.client_secret
|
||||
access_token = ensure_valid_access_token(session)
|
||||
headers = build_headers(access_token=access_token)
|
||||
client_secret = "prefix-" + configured_secret
|
||||
access_token = token_v2
|
||||
access_token = configuredToken123
|
||||
refresh_token = foo123bar456
|
||||
token = ("static-prefix-") + configuredToken
|
||||
access_token = ("static_prefix_") + configured_token
|
||||
""".getBytes(StandardCharsets.UTF_8),
|
||||
476,
|
||||
"text/x-python"
|
||||
);
|
||||
|
||||
ValidationResult result = validator.validate(new PrePublishValidator.SkillPackageContext(
|
||||
List.of(script),
|
||||
new SkillMetadata("OAuth Skill", "desc", "1.0.0", "body", Map.of()),
|
||||
"user-1",
|
||||
1L
|
||||
));
|
||||
|
||||
assertTrue(result.passed());
|
||||
assertTrue(result.warnings().isEmpty());
|
||||
}
|
||||
|
||||
@Test
|
||||
void shouldKeepWarningOnHardcodedAndProviderSpecificCredentials() {
|
||||
PackageEntry script = new PackageEntry(
|
||||
"scripts/leaked.js",
|
||||
"""
|
||||
client_secret = "literalcredential123"
|
||||
github_token = "ghp_abcdefghijklmnopqrstuvwxyz1234"
|
||||
const token = "javascriptcredential123";
|
||||
const config = { token: "objectcredential123", };
|
||||
const options = { token: "multipropertycredential123", endpoint: "/api" };
|
||||
const escaped = { token: "credential\\\"value123" };
|
||||
const emptyFirst = { token: "", password: "passwordafterempty123" };
|
||||
const dynamicFirst = { token: configuredToken, password: "passwordafterdynamic123" };
|
||||
token=("wrappedcredential123");
|
||||
token="assertedcredential123" as const;
|
||||
""".getBytes(StandardCharsets.UTF_8),
|
||||
568,
|
||||
"text/javascript"
|
||||
);
|
||||
PackageEntry configuration = new PackageEntry(
|
||||
"config/settings.env",
|
||||
"token=barecredential123 // leaked\n".getBytes(StandardCharsets.UTF_8),
|
||||
35,
|
||||
"text/plain"
|
||||
);
|
||||
|
||||
ValidationResult result = validator.validate(new PrePublishValidator.SkillPackageContext(
|
||||
List.of(script, configuration),
|
||||
new SkillMetadata("Unsafe Skill", "desc", "1.0.0", "body", Map.of()),
|
||||
"user-1",
|
||||
1L
|
||||
));
|
||||
|
||||
assertTrue(result.passed());
|
||||
assertTrue(result.warnings().stream().anyMatch(warning -> warning.contains("line 1")));
|
||||
assertTrue(result.warnings().stream().anyMatch(warning -> warning.contains("line 2")));
|
||||
assertTrue(result.warnings().stream().anyMatch(warning -> warning.contains("line 3")));
|
||||
assertTrue(result.warnings().stream().anyMatch(warning -> warning.contains("line 4")));
|
||||
assertTrue(result.warnings().stream().anyMatch(warning -> warning.contains("line 5")));
|
||||
assertTrue(result.warnings().stream().anyMatch(warning -> warning.contains("line 6")));
|
||||
assertTrue(result.warnings().stream().anyMatch(warning -> warning.contains("line 7")));
|
||||
assertTrue(result.warnings().stream().anyMatch(warning -> warning.contains("line 8")));
|
||||
assertTrue(result.warnings().stream().anyMatch(warning -> warning.contains("line 9")));
|
||||
assertTrue(result.warnings().stream().anyMatch(warning -> warning.contains("line 10")));
|
||||
assertTrue(result.warnings().stream().anyMatch(warning ->
|
||||
warning.contains("config/settings.env line 1")));
|
||||
}
|
||||
|
||||
@Test
|
||||
void shouldNotWarnOnEmptyOrShortSensitiveLiterals() {
|
||||
PackageEntry script = new PackageEntry(
|
||||
"scripts/defaults.py",
|
||||
"""
|
||||
token = ""
|
||||
client_secret = "short"
|
||||
password = 'unset'
|
||||
""".getBytes(StandardCharsets.UTF_8),
|
||||
58,
|
||||
"text/x-python"
|
||||
);
|
||||
|
||||
ValidationResult result = validator.validate(new PrePublishValidator.SkillPackageContext(
|
||||
List.of(script),
|
||||
new SkillMetadata("Defaults Skill", "desc", "1.0.0", "body", Map.of()),
|
||||
"user-1",
|
||||
1L
|
||||
));
|
||||
|
||||
assertTrue(result.passed());
|
||||
assertTrue(result.warnings().isEmpty());
|
||||
}
|
||||
|
||||
@Test
|
||||
void shouldScanDeeplyNestedSingleLineObjectWithoutOverflowingRegexStack() {
|
||||
String content = "const config = "
|
||||
+ "{ nested: ".repeat(5_000)
|
||||
+ "{ token: \"literalcredential123\""
|
||||
+ " }".repeat(5_001)
|
||||
+ ";";
|
||||
PackageEntry script = new PackageEntry(
|
||||
"scripts/deeply-nested.js",
|
||||
content.getBytes(StandardCharsets.UTF_8),
|
||||
content.length(),
|
||||
"text/javascript"
|
||||
);
|
||||
PrePublishValidator.SkillPackageContext context = new PrePublishValidator.SkillPackageContext(
|
||||
List.of(script),
|
||||
new SkillMetadata("Deeply Nested Skill", "desc", "1.0.0", "body", Map.of()),
|
||||
"user-1",
|
||||
1L
|
||||
);
|
||||
|
||||
ValidationResult result = assertDoesNotThrow(() -> validator.validate(context));
|
||||
|
||||
assertTrue(result.passed());
|
||||
assertTrue(result.warnings().stream().anyMatch(warning -> warning.contains("line 1")));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue