Skip to content

Remove ignoreUrl file setting property #123718

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

Merged
merged 4 commits into from
Mar 2, 2025
Merged

Remove ignoreUrl file setting property #123718

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0344b08
Remove ignoreUrl file setting property
rjernst Feb 28, 2025
1e00352
[CI] Auto commit changes from spotless
Feb 28, 2025
06bf4db
Merge branch 'main' into entitlements/always_ignore_setting_urls
rjernst Mar 2, 2025
00bbcca
[CI] Auto commit changes from spotless
Mar 2, 2025
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
21 changes: 7 additions & 14 deletions ...main/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FilesEntitlement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@ static FileData ofRelativePath(Path relativePath, BaseDir baseDir, Mode mode) {
return new RelativePathFileData(relativePath, baseDir, mode, null, false);
}

static FileData ofPathSetting(String setting, BaseDir baseDir, Mode mode, boolean ignoreUrl) {
return new PathSettingFileData(setting, baseDir, mode, ignoreUrl, null, false);
static FileData ofPathSetting(String setting, BaseDir baseDir, Mode mode) {
return new PathSettingFileData(setting, baseDir, mode, null, false);
}

/**
Expand Down Expand Up @@ -221,13 +221,13 @@ public FileData withPlatform(Platform platform) {
}
}

private record PathSettingFileData(String setting, BaseDir baseDir, Mode mode, boolean ignoreUrl, Platform platform, boolean exclusive)
private record PathSettingFileData(String setting, BaseDir baseDir, Mode mode, Platform platform, boolean exclusive)
implements
RelativeFileData {

@Override
public PathSettingFileData withExclusive(boolean exclusive) {
return new PathSettingFileData(setting, baseDir, mode, ignoreUrl, platform, exclusive);
return new PathSettingFileData(setting, baseDir, mode, platform, exclusive);
}

@Override
Expand All @@ -239,9 +239,7 @@ public Stream<Path> resolveRelativePaths(PathLookup pathLookup) {
String path = pathLookup.settingResolver().apply(setting);
result = path == null ? Stream.of() : Stream.of(path);
}
if (ignoreUrl) {
result = result.filter(s -> s.toLowerCase(Locale.ROOT).startsWith("https://") == false);
}
result = result.filter(s -> s.toLowerCase(Locale.ROOT).startsWith("https://") == false);
Copy link
Contributor

@ldematte ldematte Feb 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we ignore all URLs? http://, ftp:// (if we even allow that thing).
Even file:// if passed down to FileAccessTree as is would make a mess of the path sting.

Copy link
Contributor

@mosche mosche Feb 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't rely on all setting rejecting http, right? Shouldn't we make sure to exclude such urls as well, so we keep the filtetree clean?

return result.map(pathLookup.configDir()::resolve);
}

Expand All @@ -250,7 +248,7 @@ public FileData withPlatform(Platform platform) {
if (platform == platform()) {
return this;
}
return new PathSettingFileData(setting, baseDir, mode, ignoreUrl, platform, exclusive);
return new PathSettingFileData(setting, baseDir, mode, platform, exclusive);
}
}

Expand Down Expand Up @@ -338,8 +336,6 @@ public static FilesEntitlement build(List<Object> paths) {
String settingBaseDirAsString = checkString.apply(file, "basedir_if_relative");
String modeAsString = checkString.apply(file, "mode");
String platformAsString = checkString.apply(file, "platform");
Boolean ignoreUrlAsStringBoolean = checkBoolean.apply(file, "ignore_url");
boolean ignoreUrlAsString = ignoreUrlAsStringBoolean != null && ignoreUrlAsStringBoolean;
Boolean exclusiveBoolean = checkBoolean.apply(file, "exclusive");
boolean exclusive = exclusiveBoolean != null && exclusiveBoolean;

Expand All @@ -366,9 +362,6 @@ public static FilesEntitlement build(List<Object> paths) {
throw new PolicyValidationException("'relative_to' may only be used with 'relative_path'");
}

if (ignoreUrlAsStringBoolean != null && pathSetting == null) {
throw new PolicyValidationException("'ignore_url' may only be used with 'path_setting'");
}
if (settingBaseDirAsString != null && pathSetting == null) {
throw new PolicyValidationException("'basedir_if_relative' may only be used with 'path_setting'");
}
Expand All @@ -395,7 +388,7 @@ public static FilesEntitlement build(List<Object> paths) {
throw new PolicyValidationException("files entitlement with a 'path_setting' must specify 'basedir_if_relative'");
}
BaseDir baseDir = parseBaseDir(settingBaseDirAsString);
fileData = FileData.ofPathSetting(pathSetting, baseDir, mode, ignoreUrlAsString);
fileData = FileData.ofPathSetting(pathSetting, baseDir, mode);
} else {
throw new AssertionError("File entry validation error");
}
Expand Down
34 changes: 6 additions & 28 deletions ...java/org/elasticsearch/entitlement/runtime/policy/entitlements/FilesEntitlementTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,25 +102,25 @@ public void testPathSettingResolve() {
List.of(Map.of("path_setting", "foo.bar", "basedir_if_relative", "config", "mode", "read"))
);
var filesData = entitlement.filesData();
assertThat(filesData, contains(FileData.ofPathSetting("foo.bar", CONFIG, READ, false)));
assertThat(filesData, contains(FileData.ofPathSetting("foo.bar", CONFIG, READ)));

var fileData = FileData.ofPathSetting("foo.bar", CONFIG, READ, false);
var fileData = FileData.ofPathSetting("foo.bar", CONFIG, READ);
// empty settings
assertThat(fileData.resolvePaths(TEST_PATH_LOOKUP).toList(), empty());

fileData = FileData.ofPathSetting("foo.bar", CONFIG, READ, false);
fileData = FileData.ofPathSetting("foo.bar", CONFIG, READ);
settings = Settings.builder().put("foo.bar", "/setting/path").build();
assertThat(fileData.resolvePaths(TEST_PATH_LOOKUP).toList(), contains(Path.of("/setting/path")));

fileData = FileData.ofPathSetting("foo.*.bar", CONFIG, READ, false);
fileData = FileData.ofPathSetting("foo.*.bar", CONFIG, READ);
settings = Settings.builder().put("foo.baz.bar", "/setting/path").build();
assertThat(fileData.resolvePaths(TEST_PATH_LOOKUP).toList(), contains(Path.of("/setting/path")));

fileData = FileData.ofPathSetting("foo.*.bar", CONFIG, READ, false);
fileData = FileData.ofPathSetting("foo.*.bar", CONFIG, READ);
settings = Settings.builder().put("foo.baz.bar", "/setting/path").put("foo.baz2.bar", "/other/path").build();
assertThat(fileData.resolvePaths(TEST_PATH_LOOKUP).toList(), containsInAnyOrder(Path.of("/setting/path"), Path.of("/other/path")));

fileData = FileData.ofPathSetting("foo.bar", CONFIG, READ, false);
fileData = FileData.ofPathSetting("foo.bar", CONFIG, READ);
settings = Settings.builder().put("foo.bar", "relative_path").build();
assertThat(fileData.resolvePaths(TEST_PATH_LOOKUP).toList(), contains(Path.of("/config/relative_path")));
}
Expand All @@ -141,28 +141,6 @@ public void testPathSettingBasedirValidation() {
assertThat(e.getMessage(), is("'basedir_if_relative' may only be used with 'path_setting'"));
}

public void testPathSettingIgnoreUrl() {
var fileData = FileData.ofPathSetting("foo.*.bar", CONFIG, READ, true);
settings = Settings.builder().put("foo.nonurl.bar", "/setting/path").put("foo.url.bar", "https://mysite").build();
assertThat(fileData.resolvePaths(TEST_PATH_LOOKUP).toList(), contains(Path.of("/setting/path")));
}

public void testIgnoreUrlValidation() {
var e = expectThrows(
PolicyValidationException.class,
() -> FilesEntitlement.build(List.of(Map.of("path", "/foo", "mode", "read", "ignore_url", true)))
);
assertThat(e.getMessage(), is("'ignore_url' may only be used with 'path_setting'"));

e = expectThrows(
PolicyValidationException.class,
() -> FilesEntitlement.build(
List.of(Map.of("relative_path", "foo", "relative_to", "config", "mode", "read", "ignore_url", true))
)
);
assertThat(e.getMessage(), is("'ignore_url' may only be used with 'path_setting'"));
}

public void testExclusiveParsing() throws Exception {
Policy parsedPolicy = new PolicyParser(new ByteArrayInputStream("""
entitlement-module-name:
Expand Down