Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demos/opensearch-cedarling/build.gradle`:
- Line 15: The build.gradle currently sets an invalid placeholder Maven group
("group = \"RenameGroup\"") which breaks publication coordinates for pluginZip;
replace that placeholder with a proper reverse-DNS Maven group (e.g.,
"com.yourcompany.project" or your organization’s groupId) by updating the group
property in build.gradle so pluginZip and any publications use valid
coordinates.
- Around line 1-8: Move the existing buildscript block that adds
org.opensearch.gradle:build-tools onto the script classpath so it appears before
the import org.opensearch.gradle.test.RestIntegTestTask and before the apply
plugin declarations (opensearch.opensearchplugin, opensearch.yaml-rest-test,
opensearch.pluginzip); this ensures the OpenSearch build-tools classes are on
the classpath during script compilation and prevents ClassNotFoundException for
RestIntegTestTask and the custom plugin ids.
- Line 102: The build uses a non-reproducible nightly dependency implementation
"io.jans:cedarling-java:0.0.0-nightly"; replace that artifact reference with a
pinned released version (for example "io.jans:cedarling-java:2.0.0") or add
dependency locking/constraints to force an approved version. Update the
implementation declaration in build.gradle (the line containing implementation
"io.jans:cedarling-java:0.0.0-nightly") to the chosen released version or
configure Gradle dependency constraints/lockfile to ensure a stable,
reproducible cedarling-java version.
In `@demos/opensearch-cedarling/gradlew.bat`:
- Around line 1-94: The gradlew.bat file uses LF-only endings which breaks
Windows label/goto parsing (see labels like :findJavaFromJavaHome, :execute,
:fail and the use of %~dp0); convert demos/opensearch-cedarling/gradlew.bat to
CRLF line endings and commit the updated file, and add a .gitattributes rule to
enforce CRLF for all batch files (e.g. add an entry for *.bat to force text
eol=crlf) so future commits preserve CRLF.
In `@demos/opensearch-cedarling/query_ext.json`:
- Around line 8-10: Remove the hardcoded JWT found in the JSON "tokens" object
under the "Jans::Userinfo_token" key and replace it with a non-secret
placeholder (e.g., "<REPLACE_WITH_JWT>") or load it at runtime from a secret
source; update any demo/test logic that reads query_ext.json to read the token
from an environment variable or secret manager instead of embedding it (check
code that references "tokens" or "Jans::Userinfo_token" to change lookup), and
ensure any real token used for testing is revoked/rotated and added to CI/dev
secrets rather than committed.
In `@demos/opensearch-cedarling/README.md`:
- Around line 190-192: Remove the user-facing TODO from the README by deleting
the "## TODO:" section that contains "Check linting and javadoc warnings", and
instead create a tracked issue (or add to the PR) to address linting and Javadoc
warnings; update the README.md to remove the unresolved instruction and, if
desired, add a short note linking to the issue number that will track the
remaining work.
- Around line 107-163: The README still links to external repo URLs for
settings.json, records.txt, query.json, pipeline.json and query_ext.json; update
the references in demos/opensearch-cedarling/README.md to point to the local
repo files (use relative paths like ./settings.json, ./records.txt,
./query.json, ./pipeline.json, ./query_ext.json) and ensure those files are
present in the demo directory so links resolve after migrating from
github.com/jgomer2001/pipelines-plugin; look for the sections that currently
reference each external URL (the occurrences around the example curl commands
and the "Here" links) and replace them with the corresponding relative paths.
In `@demos/opensearch-cedarling/settings.gradle`:
- Line 10: The rootProject.name is still set to the template value
"plugin-template"; change the project name to "opensearch-cedarling" by updating
the rootProject.name assignment (symbol: rootProject.name) so build artifacts
and documentation match the actual plugin name
("opensearch-cedarling"/cedarling.zip). Ensure the updated name is the only
change in settings.gradle and verify packaging produces the expected artifact
name.
In
`@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchExtBuilder.java`:
- Around line 61-68: Replace the unsafe direct cast in
CedarlingSearchResponseProcessor: instead of using
CedarlingSearchExtBuilder.class.cast(exts.get(0)) which assumes the Cedarling
extension is first and can throw ClassCastException, call the helper
CedarlingSearchExtBuilder.fromExtBuilderList(exts) to locate and return the
CedarlingSearchExtBuilder instance (or null) from the exts List; update the
variable assignment to use that method and handle the null case accordingly.
In
`@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchResponseProcessor.java`:
- Around line 91-95: The code mutates the ES hit's `_source` by calling
appendExtraAttributes(map, ...) on the map returned from hit.getSourceAsMap();
instead create a new copy (e.g., new HashMap<>(hit.getSourceAsMap())) and pass
that copy to appendExtraAttributes and any policy-evaluation logic so
authorization metadata is not written back into the original hit or able to
overwrite real document fields. Update both call sites of appendExtraAttributes
in CedarlingSearchResponseProcessor (the local variable `map` usage after
hit.getSourceAsMap() and the later call around the 145-155 region) to operate on
a copied Map for evaluation and keep the original hit source unchanged when
constructing the response.
- Around line 93-105: The catch block for cedarlingService.authorize currently
adds the hit to authorized on exception (in CedarlingSearchResponseProcessor
around appendExtraAttributes / cedarlingService.authorize), which is unsafe;
change the error handling to fail-closed by removing the authorized.add(hit)
from the catch, log the error with context (use logger.error with a descriptive
message and exception), and optionally record a metric or counter for
authorization failures (e.g., increment a failure metric or note in
decisionsTook) so the hit is omitted when authorize throws instead of being
included.
- Around line 108-124: The code uses searchHits.getTotalHits() when constructing
the new SearchHits, leaking the original (pre-filter) total; replace that with a
TotalHits representing the filtered count by creating a new
org.apache.lucene.search.TotalHits using authorized.size() and
TotalHits.Relation.EQUAL_TO and pass that into the SearchHits constructor
(update the import to org.apache.lucene.search.TotalHits); keep the rest of the
SearchHits construction and subsequent SearchResponseSections (sections -> new
SearchResponseSections(...)) unchanged so pagination and displayed totals
reflect authorized.size() instead of searchHits.getTotalHits().
- Around line 62-74: request.source() can be null and casting exts.get(0)
directly is unsafe; replace the current logic that reads request.source().ext()
and casts exts.get(0) with a null check for request.source() and use the helper
CedarlingSearchExtBuilder.fromExtBuilderList(exts) to defensively locate the
Cedarling extension (handle the empty/absent result and return the original
response), and remove the direct CedarlingSearchExtBuilder.class.cast(...) usage
so unrelated ext lists won't cause ClassCastException.
In
`@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingService.java`:
- Around line 14-17: The init() method currently flips the shared state unsafely
causing races; make CedarlingAdapter and started visible and updatable
atomically by declaring the adapter field volatile (or using
AtomicReference<CedarlingAdapter>) and avoid setting started=false before
reload; instead construct and validate a new CedarlingAdapter instance locally,
and only after successful validation atomically replace the published adapter
and set started=true. Also ensure authorize() and
CedarlingSearchResponseProcessor use the volatile adapter (or
AtomicReference.get()) and add a null/started check to reject requests when no
valid adapter is published (throw an appropriate exception or return an error),
so failed init does not leave the service returning unfiltered hits.
In
`@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/PluginSettings.java`:
- Around line 27-45: PluginSettings.from() currently treats missing string
fields incorrectly (optString() returns "" not null) and returns null for
missing bootstrapProperties which leads to NPEs downstream in
SettingsService.reloadPluginSettings(); update PluginSettings.from() to validate
required fields explicitly: check ps.bootstrapProperties != null, and use
job.optString("searchActionName").isEmpty() and
job.optString("schemaPrefix").isEmpty() to detect missing strings, and throw a
descriptive IllegalArgumentException (or a custom ValidationException) listing
which required field is missing instead of returning null; alternatively change
the factory signature to return Optional<PluginSettings> and make
SettingsService.reloadPluginSettings() handle the empty Optional with a clear
error message rather than allowing an NPE when dereferencing the result.
In
`@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/rest/SettingsRestHandler.java`:
- Around line 54-99: handlePut currently persists the raw request body into
CedarlingPlugin.SETTINGS_KEY without validation or size checks; update handlePut
to (1) enforce a reasonable max payload size (e.g., reject > N bytes with
BytesRestResponse(RestStatus.PAYLOAD_TOO_LARGE)) before parsing, (2) parse the
payload into a JSONObject and validate required fields bootstrapProperties,
searchActionName, and schemaPrefix exist and have expected types, returning a
4xx (BAD_REQUEST) with a clear message if validation fails, and only then create
the ClusterUpdateSettingsRequest (cusr) and call
CedarlingPlugin.getClusterAdminClient().updateSettings(...); this prevents
storing malformed/oversized payloads that later fail in
SettingsService.reloadPluginSettings and gives immediate client feedback.
- Around line 34-36: The routes() method in SettingsRestHandler currently
returns a raw List; change its signature to return a parameterized List to be
type-safe (e.g., change public List routes() to public List<RestHandler.Route>
routes()) and keep the body constructing List.of(new RestHandler.Route(GET,
PATH), new RestHandler.Route(PUT, PATH)) unchanged so callers and the compiler
see the proper generic type for RestHandler.Route.
- Around line 122-125: In SettingsRestHandler remove the commented-out
alternative error response inside the catch(Exception e) block—delete the line
"//channel.sendResponse(new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR,
"Error processing audit request: " + e.getMessage()));" so only the active error
handling remains (the channel.sendResponse(new BytesRestResponse(channel, e))
call) and no dead/commented code is left in that catch block.
- Around line 59-65: The Content-Type check in SettingsRestHandler uses
request.getAllHeaderValues("Content-Type") and checks
List.contains("application/json"), which fails for values like
"application/json; charset=utf-8"; change the hasJsonHeader logic to iterate the
header values and normalize each value (trim, toLowerCase) then split on ';' or
use startsWith("application/json") on the normalized string to detect the base
media type; update the Optional.ofNullable(...).map(...) lambda that computes
hasJsonHeader to return true when any header value matches the normalized base
media type so requests with parameters (e.g., charset) are accepted.
In
`@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/SettingsService.java`:
- Around line 23-40: The pluginSettings field is accessed concurrently in
getSettings() without safe publication; mark the pluginSettings field as
volatile and prevent concurrent reloads by guarding the
reloadPluginSettings(AbstractScopedSettings) call with a lock (e.g., a private
final ReentrantLock or synchronized block) so readers either see a
fully-initialized PluginSettings or wait while reload completes; update
getSettings(), reloadPluginSettings(...) and any writers that assign
pluginSettings to use this lock (and ensure CedarlingService.started/useLogging
mutations are similarly synchronized if they share concurrency concerns).
- Around line 66-75: PluginSettings.from(job, lastUpdated) can return null and
is immediately dereferenced; change the code to first assign the result to a
temporary variable, check for null, and if null log an error and return (or skip
reloading) instead of assigning to the field, otherwise assign to the
pluginSettings field and call
CedarlingService.getInstance().init(pluginSettings.getBootstrapProperties(),
pluginSettings.isLogCedarlingLogs()); ensure getSettings() logic isn't left with
a null pluginSettings reference.
In
`@demos/opensearch-cedarling/src/test/java/io/jans/cedarling/opensearch/AlterSuiteListener.java`:
- Around line 20-41: The Properties reader created by
Files.newBufferedReader(propertiesFilePath, UTF_8) is never closed; wrap the
reader passed to prop.load(...) in a try-with-resources to ensure it is closed
(e.g., open a Reader in a try(...) block before calling prop.load), keep the
rest of the logic that populates parameters and reads the queryFile the same,
and change the catch block to log the thrown Throwable (pass the exception
object to logger.error) so the stack trace is preserved; update references
around XmlSuite suite, propertiesFilePath, prop.load, and
Files.newBufferedReader accordingly.
In
`@demos/opensearch-cedarling/src/test/java/io/jans/cedarling/opensearch/BenchmarkTest.java`:
- Around line 167-170: getADecimal's implementation uses ranma.nextInt(max - min
+ 1) + min which yields [min, max] while its comment says [min, max) and
benchmark expects 2024..2027; change the implementation to ranma.nextInt(max -
min) + min to produce a uniformly distributed value in [min, max) (or
alternatively update the comment and benchmark to include the inclusive max),
and ensure any call sites (e.g., the call using (2024, 2028)) are adjusted to
match the chosen convention so doc, tests, and code agree.
- Around line 36-37: The Authorization header is using URL-safe Base64
(Base64.getUrlEncoder()) which is incorrect for HTTP Basic auth; replace
Base64.getUrlEncoder() with Base64.getEncoder() when encoding (the bytes
variable creation) so the "Basic " + new String(bytes, UTF_8) produces standard
Base64 per RFC 7617; locate this in BenchmarkTest.java around the bytes
assignment/NetworkUtil construction and update the encoder call accordingly.
- Around line 64-71: The payload is built with repeated String concatenation in
BenchmarkTest (variable payload inside the loop using
bulkEntryTemplate/getAString/getADecimal/ranma/MAX_GPA), which is O(n²) and
causes excessive allocation; replace it with a StringBuilder (append each
formatted entry) and after the loop call toString() to get the final payload,
and compute byte length using an explicit charset (e.g., StandardCharsets.UTF_8)
instead of platform default when calling getBytes for the logger.info call.
In `@demos/opensearch-cedarling/src/test/resources/testng.properties`:
- Around line 4-6: Remove the hardcoded credentials in testng.properties (the
"user" and "password" entries) and replace them with placeholders that instruct
tests to read from environment variables or CI secret store (e.g.,
user=${TEST_USER} and password=${TEST_PASSWORD}). Update the test harness/setup
to obtain credentials from process environment or system properties (e.g.,
System.getenv("TEST_USER") / System.getenv("TEST_PASSWORD") or corresponding
test framework config) and fail fast with a clear error if those vars are
missing; ensure CI pipeline injects the secrets into TEST_USER and TEST_PASSWORD
instead of committing real credentials.