From 60c37a102eee10d267f8ae1777b0ace7623b1986 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Tue, 18 Aug 2026 15:27:22 -0700 Subject: [PATCH 1/4] Update TableSelectorTestCase to test more MySQL/MariaDB databases (#7946) ## Rationale Improve testing of MySQL and MariaDB databases ## Changes - Detect MySQL and MariaDB data sources based on product name, not data source name - If `sakila` is not present, try testing with `sys.sys_config` --- .../api/data/TableSelectorTestCase.java | 27 ++++++++++++++++--- .../api/data/dialect/StandardJdbcHelper.java | 2 +- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/api/src/org/labkey/api/data/TableSelectorTestCase.java b/api/src/org/labkey/api/data/TableSelectorTestCase.java index 281681ebc2e..68d5c2ce623 100644 --- a/api/src/org/labkey/api/data/TableSelectorTestCase.java +++ b/api/src/org/labkey/api/data/TableSelectorTestCase.java @@ -17,6 +17,7 @@ import org.apache.commons.lang3.mutable.MutableInt; import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.Logger; import org.junit.Test; import org.labkey.api.collections.CsvSet; import org.labkey.api.data.Selector.ForEachBlock; @@ -30,6 +31,7 @@ import org.labkey.api.util.ExceptionUtil; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.TestContext; +import org.labkey.api.util.logging.LogHelper; import org.springframework.jdbc.UncategorizedSQLException; import java.sql.ResultSet; @@ -50,6 +52,8 @@ public class TableSelectorTestCase extends AbstractSelectorTestCase { + private static final Logger LOG = LogHelper.getLogger(TableSelectorTestCase.class, "Test progress"); + @Test public void testTableSelector() throws SQLException { @@ -59,13 +63,25 @@ public void testTableSelector() throws SQLException // testTableSelector(DbSchema.get("oracle.granite", DbSchemaType.Bare).getTable("account"), Account.class); // Test MySQL or MariaDB database, if present - List mySqlScopes = Stream.of("mySql", "mariadb") - .map(DbScope::getDbScope).filter(Objects::nonNull).toList(); + List mySqlScopes = DbScope.getDbScopesToTest().stream() + .filter(scope -> Set.of("MySQL", "MariaDB").contains(scope.getSqlDialect().getProductName())) + .toList(); + for (DbScope mySqlScope: mySqlScopes) { DbSchema sakila = mySqlScope.getSchema("sakila", DbSchemaType.Bare); if (sakila.existsInDatabase()) - testTableSelector(sakila.getTable("country"), Country.class); + { + testTableSelector(sakila.getTable("Country"), Country.class); + } + else + { + DbSchema sys = mySqlScope.getSchema("sys", DbSchemaType.Bare); + if (sys.existsInDatabase()) + { + testTableSelector(sys.getTable("sys_config"), Config.class); + } + } } testTableSelector(CoreSchema.getInstance().getTableInfoActiveUsers(), User.class); testTableSelector(CoreSchema.getInstance().getTableInfoModules(), ModuleContext.class); @@ -124,6 +140,8 @@ public int hashCode() } } + record Config(String Variable, String Value, Date Set_Time, String Set_By){} + // public static class Account // { // private int _account_id; @@ -430,6 +448,9 @@ private void testColumnList(TableSelector selector, boolean stable) throws SQLEx private void testTableSelector(TableInfo table, Class clazz) throws SQLException { + DbSchema schema = table.getSchema(); + LOG.info("Testing {}.{}.{}", schema.getScope().getDisplayName(), schema.getName(), table.getName()); + TableSelector selector = new TableSelector(table); test(selector, clazz); diff --git a/api/src/org/labkey/api/data/dialect/StandardJdbcHelper.java b/api/src/org/labkey/api/data/dialect/StandardJdbcHelper.java index 986b0e0fa97..9362ddcde91 100644 --- a/api/src/org/labkey/api/data/dialect/StandardJdbcHelper.java +++ b/api/src/org/labkey/api/data/dialect/StandardJdbcHelper.java @@ -44,7 +44,7 @@ protected String parseDatabase(String url) throws ServletException if (-1 == dbEnd) dbEnd = url.length(); - // Last '/' is the database delimiter, except for "jdbc:postgresql:database" + // Last '/' is the database delimiter, except for "jdbc:postgresql:database" and old Oracle formats char dbDelimiter = url.contains("/") ? '/' : ':'; int dbDelimiterIndex = url.lastIndexOf(dbDelimiter, dbEnd); From a57718cfd015e99e492a6bb0d21f96880ee30434 Mon Sep 17 00:00:00 2001 From: labkey-matthewb Date: Thu, 20 Aug 2026 16:59:57 -0700 Subject: [PATCH 2/4] Merge from release25.7 (#7962) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Rationale merge from 25.7_fb_root_refactor ## Related Pull Requests - ## Changes - --- core/src/org/labkey/core/CoreModule.java | 1 + .../labkey/core/admin/AdminController.java | 129 ++++++++++++++++-- 2 files changed, 117 insertions(+), 13 deletions(-) diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index 550c5a80b2c..e391d350bbd 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -1465,6 +1465,7 @@ public TabDisplayMode getTabDisplayMode() public @NotNull Set> getUnitTests() { return Set.of( + AdminController.FileRootPermissionTestCase.class, ApiJsonWriter.TestCase.class, ClassLoaderTestCase.class, CopyFileRootPipelineJob.TestCase.class, diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 6498f2c4930..f07766d4d3f 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -6259,6 +6259,7 @@ else if (form.hasSiteDefaultRoot()) { if (service.isFileRootDisabled(ctx.getContainer()) || !service.isUseDefaultRoot(ctx.getContainer())) { + throwIfUnauthorizedFileRootChange(ctx, service, form); service.setIsUseDefaultRoot(ctx.getContainer(), true); changed = true; shouldCopyMove = true; @@ -6403,22 +6404,62 @@ private static void initiateCopyFilesPipelineJobs(ViewContext ctx, @NotNull List private static void throwIfUnauthorizedFileRootChange(ViewContext ctx, FileContentService service, FileManagementForm form) { - // test permissions. only site admins are able to turn on a custom file root for a folder - // this is only relevant if the folder is either being switched to a custom file root, - // or if the file root is changed. - if (!service.isUseDefaultRoot(ctx.getContainer())) - { - Path fileRootPath = service.getFileRootPath(ctx.getContainer()); - if (null != fileRootPath) + // Only site admins (AdminOperationsPermission) are able to switch a folder to a custom file root, change + // an existing custom root's path, or revert a custom root back to the site default -- any of these moves + // where the folder's files live. Resubmitting the folder's own current root unchanged is a no-op and does + // not require the elevated permission. + boolean hasAdminOpsPermission = ctx.getUser().hasRootPermission(AdminOperationsPermission.class); + boolean isUseDefaultRoot = service.isUseDefaultRoot(ctx.getContainer()); + String requestedRoot; + String currentRoot; + + if (form.hasSiteDefaultRoot()) + { + // Requesting the default root: no root is being submitted; the current custom root (if any) is + // whatever type the container has now. + requestedRoot = null; + if (isUseDefaultRoot) + currentRoot = null; + else if (service.isCloudRoot(ctx.getContainer())) + currentRoot = service.getCloudRootName(ctx.getContainer()); + else { - String absolutePath = FileUtil.getAbsolutePath(ctx.getContainer(), fileRootPath); - if (Strings.CI.equals(absolutePath, form.getFolderRootPath())) - { - if (!ctx.getUser().hasRootPermission(AdminOperationsPermission.class)) - throw new UnauthorizedException("Only site admins can change file roots"); - } + Path fileRootPath = service.getFileRootPath(ctx.getContainer()); + currentRoot = null != fileRootPath ? FileUtil.getAbsolutePath(ctx.getContainer(), fileRootPath) : null; } } + else if (form.isCloudFileRoot()) + { + requestedRoot = form.getCloudRootName(); + currentRoot = (!isUseDefaultRoot && service.isCloudRoot(ctx.getContainer())) ? service.getCloudRootName(ctx.getContainer()) : null; + } + else + { + requestedRoot = StringUtils.trimToNull(form.getFolderRootPath()); + Path fileRootPath = isUseDefaultRoot ? null : service.getFileRootPath(ctx.getContainer()); + currentRoot = null != fileRootPath ? FileUtil.getAbsolutePath(ctx.getContainer(), fileRootPath) : null; + } + + if (!isFileRootChangeAuthorizedOrNoChange(hasAdminOpsPermission, isUseDefaultRoot, currentRoot, requestedRoot)) + throw new UnauthorizedException("Only site admins can change file roots"); + } + + /** + * Pure decision logic behind {@link #throwIfUnauthorizedFileRootChange}, factored out for unit testing. + * @param hasAdminOpsPermission whether the requesting user holds root AdminOperationsPermission + * @param isUseDefaultRoot whether the target container currently uses the default (inherited) file root + * @param currentRoot the container's existing custom root path/cloud name, or null if there isn't one + * @param requestedRoot the root path/cloud name submitted in the request, or null if none was submitted + */ + private static boolean isFileRootChangeAuthorizedOrNoChange(boolean hasAdminOpsPermission, boolean isUseDefaultRoot, @Nullable String currentRoot, @Nullable String requestedRoot) + { + if (hasAdminOpsPermission) + return true; + if (null == requestedRoot) + return isUseDefaultRoot || null == currentRoot; // clearing an existing custom root is still a change to it + if (!isUseDefaultRoot && requestedRoot.equalsIgnoreCase(currentRoot)) + return true; // no-op resubmission of the folder's own existing custom root + return false; } public static void setEnabledCloudStores(ViewContext ctx, FileManagementForm form, BindException errors) @@ -9250,6 +9291,68 @@ public void modulesWithSchemaVersionButNoScripts() } } + // Regression coverage for the file root privilege-escalation fix: a folder/project admin without root + // AdminOperationsPermission must never be able to switch a container to a custom file root, or change an + // existing custom root's path. + public static class FileRootPermissionTestCase extends Assert + { + @Test + public void defaultRootRequiresAdminOpsPermissionForNewCustomPath() + { + // This is the case the original (inverted) condition silently skipped: a container on the default + // root, submitting any custom path, from a user without AdminOperationsPermission. + assertFalse(isFileRootChangeAuthorizedOrNoChange(false, true, null, "/some/path")); + } + + @Test + public void customRootRequiresAdminOpsPermissionForDifferentPath() + { + assertFalse(isFileRootChangeAuthorizedOrNoChange(false, false, "/existing/path", "/attacker/path")); + } + + @Test + public void customRootResubmissionOfSamePathIsAllowed() + { + assertTrue(isFileRootChangeAuthorizedOrNoChange(false, false, "/existing/path", "/existing/path")); + assertTrue(isFileRootChangeAuthorizedOrNoChange(false, false, "/Existing/Path", "/existing/path")); + } + + @Test + public void noRequestedRootOnDefaultRootIsAllowed() + { + // No custom root requested and none currently exists -- nothing to protect. + assertTrue(isFileRootChangeAuthorizedOrNoChange(false, true, null, null)); + } + + @Test + public void clearingAnExistingCustomRootRequiresAdminOpsPermission() + { + // A request that omits the root (e.g. a non-ops admin's disabled form fields not being submitted) + // must not be able to silently clear an existing custom root back to default. + assertFalse(isFileRootChangeAuthorizedOrNoChange(false, false, "/existing/path", null)); + assertTrue(isFileRootChangeAuthorizedOrNoChange(true, false, "/existing/path", null)); + } + + @Test + public void revertingCustomRootToSiteDefaultRequiresAdminOpsPermission() + { + // Selecting the site default root while a custom root (file path or cloud) is in effect relocates the + // folder's file storage, so it needs the same elevated permission as setting a custom root. + assertFalse(isFileRootChangeAuthorizedOrNoChange(false, false, "/existing/path", null)); + assertFalse(isFileRootChangeAuthorizedOrNoChange(false, false, "myCloudStore", null)); + assertTrue(isFileRootChangeAuthorizedOrNoChange(true, false, "myCloudStore", null)); + // Already on the default root (e.g. re-enabling file sharing from the disabled state) is a no-op. + assertTrue(isFileRootChangeAuthorizedOrNoChange(false, true, null, null)); + } + + @Test + public void adminOpsPermissionIsAlwaysAllowed() + { + assertTrue(isFileRootChangeAuthorizedOrNoChange(true, true, null, "/some/path")); + assertTrue(isFileRootChangeAuthorizedOrNoChange(true, false, "/existing/path", "/attacker/path")); + } + } + public static class ModuleForm { private String _name; From 6c7125f6e3d3b2e5e738e451b557c562e915aaba Mon Sep 17 00:00:00 2001 From: Nick Kerr Date: Fri, 21 Aug 2026 07:39:16 -0700 Subject: [PATCH 3/4] ExpressionAssistant: improve fence parsing (#7963) --- .../ExpressionAssistantAgentAction.java | 271 +++++++++++++++--- .../prompts/ExpressionAssistant.md | 3 +- 2 files changed, 236 insertions(+), 38 deletions(-) diff --git a/query/src/org/labkey/query/controllers/ExpressionAssistantAgentAction.java b/query/src/org/labkey/query/controllers/ExpressionAssistantAgentAction.java index 297fbb80825..070f73b973b 100644 --- a/query/src/org/labkey/query/controllers/ExpressionAssistantAgentAction.java +++ b/query/src/org/labkey/query/controllers/ExpressionAssistantAgentAction.java @@ -43,6 +43,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import static org.apache.commons.lang3.StringUtils.isBlank; import static org.apache.commons.lang3.StringUtils.isNotBlank; @@ -194,48 +195,74 @@ private static JSONArray buildSegments(List response MarkdownService md = MarkdownService.get(); StringBuilder htmlBuf = new StringBuilder(); - for (var response : responses) - { - String text = response.text(); - if (isBlank(text)) - continue; + // Scan the turns as one document: tool calling can split a single assistant turn so that a + // fence opens in one MessageResponse and closes in the next. + String text = responses.stream() + .map(McpService.MessageResponse::text) + .filter(StringUtils::isNotBlank) + .collect(Collectors.joining("\n")); + + LOG.debug("Expression assistant raw response:\n{}", text); - String[] lines = text.split("\n", -1); - int i = 0; - while (i < lines.length) + String[] lines = text.split("\n", -1); + int i = 0; + while (i < lines.length) + { + Fence f = readFence(lines, i); + if (f != null && f.terminated && ("sql".equals(f.tag) || "expression".equals(f.tag))) { - Fence f = readFence(lines, i); - if (f != null && f.terminated && ("sql".equals(f.tag) || "expression".equals(f.tag))) - { - flushHtmlSegment(segments, htmlBuf, md); - segments.put(buildSqlSegment(f.tag, f.body)); - i = f.nextIndex; - } - else if (f != null && !f.terminated) - { - // Unterminated fence — fold the body back into prose so we don't drop content, - // but skip the opening fence line itself so the user doesn't see a stray - // "```expression" rendered as a code marker. - if (!htmlBuf.isEmpty()) htmlBuf.append("\n"); - htmlBuf.append(f.body); - break; - } - else - { - // Either not a fence opener or an unknown tag (e.g., python). In the unknown-tag - // case we leave the fence intact in prose so the Markdown renderer turns it into - // a code block. - if (!htmlBuf.isEmpty()) htmlBuf.append("\n"); - htmlBuf.append(lines[i]); - i++; - } + flushHtmlSegment(segments, htmlBuf, md); + segments.put(buildSqlSegment(f.tag, f.body)); + i = f.nextIndex; + } + else if (f != null && !f.terminated) + { + // Unterminated fence — skip only the opening line, so the user doesn't see a stray + // "```expression" rendered as a code marker, and keep scanning. The body lands in prose + // line by line via the branch below, and a well-formed fence later in the turn still parses. + i++; + } + else + { + // Either not a fence opener or an unknown tag (e.g., python). In the unknown-tag + // case we leave the fence intact in prose so the Markdown renderer turns it into + // a code block. + if (!htmlBuf.isEmpty()) htmlBuf.append("\n"); + htmlBuf.append(lines[i]); + i++; } } flushHtmlSegment(segments, htmlBuf, md); + + // A payload the user can read means the model broke the fence protocol, so it renders as raw JSON + // instead of a working Apply Expression action. Log the response that caused it. + if (hasLeakedPayload(segments)) + LOG.warn("Expression assistant leaked a validator payload into its reply:\n{}", text); + return segments; } + /** + * True when a validateCalculatedColumnExpression payload reached the user as text rather than being unpacked + * into an `expression` segment. Prose is one surface; the others are an illustrative `sql` block and an + * `expression` block whose body didn't yield an expression, which shows the raw JSON behind an Apply button. + * A well-formed `expression` segment carries jdbcType as its own key, so only the displayed text is checked. + */ + private static boolean hasLeakedPayload(JSONArray segments) + { + for (int i = 0; i < segments.length(); i++) + { + JSONObject segment = segments.getJSONObject(i); + String displayed = "html".equals(segment.optString("type")) + ? segment.optString("html", "") + : segment.optString("sql", ""); + if (displayed.contains("jdbcType")) + return true; + } + return false; + } + /** * Build a segment JSON object for an `sql` or `expression` fenced block. For `expression` * blocks the body is expected to be the JSON returned by validateCalculatedColumnExpression; @@ -277,19 +304,38 @@ private static JSONObject buildSqlSegment(String tag, String body) private record Fence(String tag, String body, int nextIndex, boolean terminated) {} + /** Length of the leading backtick run if {@code trimmed} is long enough to delimit a fence, else 0. */ + private static int backtickRun(String trimmed) + { + int n = 0; + while (n < trimmed.length() && trimmed.charAt(n) == '`') + n++; + return n >= 3 ? n : 0; + } + + /** Per CommonMark a closing fence is backticks only, and at least as long as the opener. */ + private static boolean isFenceClose(String line, int openLength) + { + String trimmed = line.trim(); + int n = backtickRun(trimmed); + return n == trimmed.length() && n >= openLength; + } + private static Fence readFence(String[] lines, int i) { String trimmed = lines[i].trim(); - if (!trimmed.startsWith("```")) + int openLength = backtickRun(trimmed); + if (openLength == 0) return null; - String rest = trimmed.substring(3).trim(); + String rest = trimmed.substring(openLength).trim(); if (rest.isEmpty()) return null; - String tag = rest.toLowerCase(); + // Only the first word of the info string is the tag; models pad it ("expression json"). + String tag = rest.split("\\s+", 2)[0].toLowerCase(); int j = i + 1; StringBuilder body = new StringBuilder(); - while (j < lines.length && !"```".equals(lines[j].trim())) + while (j < lines.length && !isFenceClose(lines[j], openLength)) { if (!body.isEmpty()) body.append("\n"); body.append(lines[j]); @@ -348,6 +394,24 @@ private static String expressionPayload(String sql, String jdbcType) return json.toString(); } + private static void assertExpressionSegment(JSONArray segments, int i, String sql, String jdbcType) + { + assertEquals("expression", segment(segments, i).getString("type")); + assertEquals(sql, segment(segments, i).getString("sql")); + assertEquals(jdbcType, segment(segments, i).getString("jdbcType")); + } + + /** The validator payload is transport between the tool and this action; it must never reach the user as text. */ + private static void assertNoPayloadInProse(JSONArray segments) + { + for (int i = 0; i < segments.length(); i++) + { + JSONObject seg = segments.getJSONObject(i); + if ("html".equals(seg.optString("type"))) + assertFalse("validator payload leaked into prose: " + seg, seg.getString("html").contains("jdbcType")); + } + } + @Test public void emptyResponseList() { @@ -469,6 +533,19 @@ public void unterminatedFenceFallsBackToHtml() assertFalse("opening fence must be stripped: " + html, html.contains("```")); } + @Test + public void unterminatedFenceDoesNotDiscardLaterResponses() + { + // An unterminated fence must cost only its own block — the four-backtick opener below is never + // closed (its closer is shorter), and the well-formed block after it still has to parse. + var r1 = markdownResponse("````expression\n" + expressionPayload("SELECT 1", "INTEGER") + "\n```"); + var r2 = markdownResponse("Corrected:\n```expression\n" + expressionPayload("SELECT 2", "BIGINT") + "\n```"); + JSONArray segments = buildSegments(List.of(r1, r2)); + assertEquals(2, segments.length()); + assertEquals("html", segment(segments, 0).getString("type")); + assertExpressionSegment(segments, 1, "SELECT 2", "BIGINT"); + } + @Test public void unknownFenceLanguageIsTreatedAsProse() { @@ -490,6 +567,126 @@ public void fenceTagIsCaseInsensitive() assertEquals("INTEGER", segment(segments, 0).getString("jdbcType")); } + @Test + public void expressionFenceIsRecognizedAtAnyBacktickLength() + { + for (int n = 3; n <= 6; n++) + { + String delim = "`".repeat(n); + String md = delim + "expression\n" + expressionPayload("Int7 + Int6", "INTEGER") + "\n" + delim; + JSONArray segments = buildSegments(List.of(markdownResponse(md))); + assertEquals("fence length " + n, 1, segments.length()); + assertExpressionSegment(segments, 0, "Int7 + Int6", "INTEGER"); + assertNoPayloadInProse(segments); + } + } + + @Test + public void longerClosingFenceTerminatesShorterOpener() + { + String md = "```expression\n" + expressionPayload("Int7 + Int6", "INTEGER") + "\n````"; + JSONArray segments = buildSegments(List.of(markdownResponse(md))); + assertEquals(1, segments.length()); + assertExpressionSegment(segments, 0, "Int7 + Int6", "INTEGER"); + } + + @Test + public void shorterClosingFenceDoesNotTerminateLongerOpener() + { + // Per CommonMark the short line is fence content, not a closer, so the block never terminates. + String md = "````expression\n" + expressionPayload("Int7 + Int6", "INTEGER") + "\n```"; + JSONArray segments = buildSegments(List.of(markdownResponse(md))); + assertEquals(1, segments.length()); + assertEquals("html", segment(segments, 0).getString("type")); + } + + @Test + public void leakedPayloadDetectedWhenFenceIsMissing() + { + // No opening fence, so the payload lands in prose and the user gets no Apply affordance. + String md = "Adding Int7 and Int6.\n\n" + expressionPayload("Int7 + Int6", "INTEGER") + "\n```"; + JSONArray segments = buildSegments(List.of(markdownResponse(md))); + assertEquals(1, segments.length()); + assertEquals("html", segment(segments, 0).getString("type")); + assertTrue("prose payload should be reported as a leak", hasLeakedPayload(segments)); + } + + @Test + public void leakedPayloadDetectedInSqlFence() + { + // The model reached for the illustrative `sql` tag, so the payload renders as a read-only code block. + String md = "```sql\n" + expressionPayload("Int7 + Int6", "INTEGER") + "\n```"; + JSONArray segments = buildSegments(List.of(markdownResponse(md))); + assertEquals(1, segments.length()); + assertEquals("sql", segment(segments, 0).getString("type")); + assertTrue("payload in a sql block should be reported as a leak", hasLeakedPayload(segments)); + } + + @Test + public void leakedPayloadDetectedWhenExpressionBodyIsNotJson() + { + // The model narrated inside the fence, so the body doesn't parse and Apply would write JSON to the field. + String body = "Here is the payload: " + expressionPayload("Int7 + Int6", "INTEGER"); + JSONArray segments = buildSegments(List.of(markdownResponse("```expression\n" + body + "\n```"))); + assertEquals(1, segments.length()); + assertEquals("expression", segment(segments, 0).getString("type")); + assertEquals(body, segment(segments, 0).getString("sql")); + assertTrue("unparsable payload should be reported as a leak", hasLeakedPayload(segments)); + } + + @Test + public void leakedPayloadDetectedWhenExpressionKeyIsMissing() + { + // Parses, but nothing to unpack, so buildSqlSegment falls back to the raw body. + String body = "{\"jdbcType\":\"INTEGER\",\"sql\":\"Int7 + Int6\"}"; + JSONArray segments = buildSegments(List.of(markdownResponse("```expression\n" + body + "\n```"))); + assertEquals(body, segment(segments, 0).getString("sql")); + assertTrue("payload without an expression key should be reported as a leak", hasLeakedPayload(segments)); + } + + @Test + public void wellFormedExpressionSegmentIsNotALeak() + { + // "jdbcType" is a key on the expression segment itself, not part of the SQL the user sees. + String md = "```expression\n" + expressionPayload("Int7 + Int6", "INTEGER") + "\n```"; + JSONArray segments = buildSegments(List.of(markdownResponse(md))); + assertExpressionSegment(segments, 0, "Int7 + Int6", "INTEGER"); + assertFalse("a parsed expression segment is not a leak", hasLeakedPayload(segments)); + } + + @Test + public void proseWithoutPayloadIsNotALeak() + { + JSONArray segments = buildSegments(List.of(markdownResponse("Which date field should be used?"))); + assertEquals("html", segment(segments, 0).getString("type")); + assertFalse(hasLeakedPayload(segments)); + } + + @Test + public void expressionFenceWithExtraInfoStringTokenIsRecognized() + { + // Only the info string's first word is the tag, so "expression json" still resolves to "expression". + String md = "```expression json\n" + expressionPayload("Int7 + Int6", "INTEGER") + "\n```"; + JSONArray segments = buildSegments(List.of(markdownResponse(md))); + assertEquals(1, segments.length()); + assertExpressionSegment(segments, 0, "Int7 + Int6", "INTEGER"); + assertNoPayloadInProse(segments); + } + + @Test + public void expressionFenceSplitAcrossResponsesIsRecognized() + { + // Tool calling can split one assistant turn mid-fence; the scan joins the turns so the block still closes. + String payload = expressionPayload("Int7 + Int6", "INTEGER"); + var r1 = markdownResponse("Adding Int7 and Int6.\n\n```expression\n" + payload); + var r2 = markdownResponse("```"); + JSONArray segments = buildSegments(List.of(r1, r2)); + assertEquals(2, segments.length()); + assertEquals("html", segment(segments, 0).getString("type")); + assertExpressionSegment(segments, 1, "Int7 + Int6", "INTEGER"); + assertNoPayloadInProse(segments); + } + @Test public void preservesMultilineSqlBody() { diff --git a/query/src/org/labkey/query/controllers/prompts/ExpressionAssistant.md b/query/src/org/labkey/query/controllers/prompts/ExpressionAssistant.md index 1eafede8e9e..1f8be652624 100644 --- a/query/src/org/labkey/query/controllers/prompts/ExpressionAssistant.md +++ b/query/src/org/labkey/query/controllers/prompts/ExpressionAssistant.md @@ -96,7 +96,8 @@ ambiguity materially affects the result. 2. **Validate Silently:** When you produce a SQL expression, you must validate it using the `validateCalculatedColumnExpression` tool. You must not mention this tool to the user. 3. **Format Final Expressions:** When presenting a final SQL expression for the user to apply, you must place the tool's - JSON return value verbatim inside a fenced code block tagged `expression` (e.g., ````expression\n{...}\n````). + JSON return value verbatim inside a fenced code block tagged `expression`. Open the block with exactly three + backticks followed by `expression`, and close it with exactly three backticks — see the example below. * Emit this block **ONLY AFTER** a successful validation. * The body of the block must be exactly the JSON string the tool returned. Do not reformat, strip fields, add fields, summarize, or pretty-print it differently than the tool produced. From ae641a2c9317513d57bbb8ac39be3e9c3d85c96d Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Fri, 21 Aug 2026 15:54:50 -0700 Subject: [PATCH 4/4] Increase required permissions for wiki bulk print actions (#7957) ## Rationale https://github.com/LabKey/internal-issues/issues/1415 --- wiki/src/org/labkey/wiki/WikiController.java | 51 ++++++++++++++++--- wiki/src/org/labkey/wiki/WikiModule.java | 3 +- wiki/src/org/labkey/wiki/WikiTOC.java | 17 ++++--- .../org/labkey/wiki/model/BaseWikiView.java | 20 +++++--- wiki/src/org/labkey/wiki/model/WikiView.java | 10 +--- .../org/labkey/wiki/model/WikiWebPart.java | 2 +- 6 files changed, 72 insertions(+), 31 deletions(-) diff --git a/wiki/src/org/labkey/wiki/WikiController.java b/wiki/src/org/labkey/wiki/WikiController.java index a48f322dd34..d5f307af81a 100644 --- a/wiki/src/org/labkey/wiki/WikiController.java +++ b/wiki/src/org/labkey/wiki/WikiController.java @@ -22,7 +22,6 @@ import org.apache.commons.collections4.multimap.ArrayListValuedHashMap; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.Strings; -import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -60,19 +59,24 @@ import org.labkey.api.security.User; import org.labkey.api.security.UserManager; import org.labkey.api.security.WikiTermsOfUseProvider; +import org.labkey.api.security.permissions.AbstractActionPermissionTest; import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.permissions.UpdatePermission; import org.labkey.api.security.roles.EditorRole; import org.labkey.api.security.roles.FolderAdminRole; import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.settings.AdminConsole; import org.labkey.api.settings.AppProps; +import org.labkey.api.test.TestWhen; import org.labkey.api.util.GUID; import org.labkey.api.util.HtmlString; import org.labkey.api.util.HtmlStringBuilder; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Pair; +import org.labkey.api.util.TestContext; +import org.labkey.api.util.logging.LogHelper; import org.labkey.api.view.ActionURL; import org.labkey.api.view.GridView; import org.labkey.api.view.HtmlView; @@ -130,7 +134,7 @@ public class WikiController extends SpringActionController { - private static final Logger LOG = LogManager.getLogger(WikiController.class); + private static final Logger LOG = LogHelper.getLogger(WikiController.class, "Wiki action debugging"); private static final DefaultActionResolver _actionResolver = new DefaultActionResolver(WikiController.class); public WikiController() @@ -750,7 +754,8 @@ public String getCommandClassMethodName() public abstract Set getWikiTrees(FORM form, Container c); } - @RequiresPermission(ReadPermission.class) + // Require update to prevent bots from excessive crawling of expensive action, GitHub Issue #1415 + @RequiresPermission(UpdatePermission.class) public class PrintAllAction extends PrintMultipleAction { @Override @@ -766,7 +771,8 @@ public void addNavTrail(NavTree root) } } - @RequiresPermission(ReadPermission.class) + // Require update to prevent bots from excessive crawling of expensive action, GitHub Issue #1415 + @RequiresPermission(UpdatePermission.class) public class PrintBranchAction extends PrintMultipleAction { private Wiki _rootWiki; @@ -823,7 +829,8 @@ public void addNavTrail(NavTree root) } } - @RequiresPermission(ReadPermission.class) + // Require update to prevent bots from excessive crawling of expensive action, GitHub Issue #1415 + @RequiresPermission(UpdatePermission.class) public class PrintAllRawAction extends SimpleViewAction { @Override @@ -2747,7 +2754,8 @@ public ApiResponse execute(SetTocPreferenceForm form, BindException errors) } } - @RequiresPermission(ReadPermission.class) + // Require update to prevent bots from excessive crawling of expensive action, GitHub Issue #1415 + @RequiresPermission(UpdatePermission.class) public class BackLinksAction extends SimpleViewAction { @Override @@ -2972,7 +2980,36 @@ public void testAttachFilesRequiresUpdate() throws Exception // Positive control: an Editor passes the UpdatePermission guard. User editor = createUserInRole(folder, EditorRole.class); assertNotEquals("An editor must pass the attachment UpdatePermission guard, not be blocked at 403", - HttpServletResponse.SC_FORBIDDEN, post(url, editor).getStatus()); + HttpServletResponse.SC_FORBIDDEN, post(url, editor).getStatus()); + } + } + + @TestWhen(TestWhen.When.BVT) + public static class PermissionTestCase extends AbstractActionPermissionTest + { + @Override + @Test + public void testActionPermissions() + { + User user = TestContext.get().getUser(); + assertTrue(user.hasSiteAdminPermission()); + WikiController controller = new WikiController(); + + // TODO: Check more actions + + // @RequiresPermission(ReadPermission.class) + assertForReadPermission(user, false, + controller.new PageAction(), + controller.new PrintRawAction() + ); + + // @RequiresPermission(UpdatePermission.class) + assertForUpdateOrDeletePermission(user, + controller.new BackLinksAction(), + controller.new PrintAllAction(), + controller.new PrintAllRawAction(), + controller.new PrintBranchAction() + ); } } } diff --git a/wiki/src/org/labkey/wiki/WikiModule.java b/wiki/src/org/labkey/wiki/WikiModule.java index 7025564475b..0af37f0c893 100644 --- a/wiki/src/org/labkey/wiki/WikiModule.java +++ b/wiki/src/org/labkey/wiki/WikiModule.java @@ -199,7 +199,8 @@ private void loadWikiContent(@Nullable Container c, User user, String name, Stri { return Set.of( WikiManager.TestCase.class, - WikiController.CopyWikiContainerScopingTestCase.class + WikiController.CopyWikiContainerScopingTestCase.class, + WikiController.PermissionTestCase.class ); } diff --git a/wiki/src/org/labkey/wiki/WikiTOC.java b/wiki/src/org/labkey/wiki/WikiTOC.java index dca464d0487..b5a264c0e4f 100644 --- a/wiki/src/org/labkey/wiki/WikiTOC.java +++ b/wiki/src/org/labkey/wiki/WikiTOC.java @@ -23,6 +23,7 @@ import org.labkey.api.security.User; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.InsertPermission; +import org.labkey.api.security.permissions.UpdatePermission; import org.labkey.api.util.DOM; import org.labkey.api.util.HtmlString; import org.labkey.api.util.LinkBuilder; @@ -100,26 +101,28 @@ private NavTree createNavMenu() ViewContext context = getViewContext(); User user = context.getUser(); - //output only this one if wiki contains no pages - boolean bHasInsert = _cToc.hasPermission("WikiTOC.getNavMenu()", user, InsertPermission.class); - boolean bHasCopy = _cToc.hasPermission("WikiTOC.getNavMenu()", user, AdminPermission.class) && !getElements().isEmpty(); - boolean bHasPrint = (bHasInsert || !isInWebPart(context)) && !getElements().isEmpty(); + //output "New" if wiki contains no pages + boolean hasInsert = _cToc.hasPermission("WikiTOC.getNavMenu()", user, InsertPermission.class); + boolean hasCopy = _cToc.hasPermission("WikiTOC.getNavMenu()", user, AdminPermission.class) && !getElements().isEmpty(); + // Must have update in the container since this is a folder-wide, potentially expensive operation. GitHub Issue #1415. + boolean hasUpdate = _cToc.hasPermission("WikiTOC.getNavMenu()", user, UpdatePermission.class); + boolean hasPrintAll = hasUpdate && !isInWebPart(context) && !getElements().isEmpty(); NavTree menu = new NavTree(); - if (bHasInsert) + if (hasInsert) { ActionURL newPageUrl = new ActionURL(WikiController.EditWikiAction.class, _cToc); newPageUrl.addParameter("cancel", context.getActionURL().getLocalURIString()); menu.addChild("New", newPageUrl.getLocalURIString()); } - if (bHasCopy) + if (hasCopy) { URLHelper copyUrl = new ActionURL(WikiController.CopyWikiLocationAction.class, _cToc); //pass in source container as a param. copyUrl.addParameter("sourceContainer", _cToc.getPath()); menu.addChild("Copy", copyUrl.toString()); } - if (bHasPrint) + if (hasPrintAll) { menu.addChild("Print all", new ActionURL(WikiController.PrintAllAction.class, _cToc).toString()); } diff --git a/wiki/src/org/labkey/wiki/model/BaseWikiView.java b/wiki/src/org/labkey/wiki/model/BaseWikiView.java index f9549f301b2..8a6b827a215 100644 --- a/wiki/src/org/labkey/wiki/model/BaseWikiView.java +++ b/wiki/src/org/labkey/wiki/model/BaseWikiView.java @@ -20,6 +20,7 @@ import org.labkey.api.data.PropertyManager; import org.labkey.api.portal.ProjectUrls; import org.labkey.api.security.User; +import org.labkey.api.security.permissions.UpdatePermission; import org.labkey.api.util.HtmlString; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.view.ActionURL; @@ -31,6 +32,7 @@ import org.labkey.api.wiki.WikiRendererType; import org.labkey.wiki.BaseWikiPermissions; import org.labkey.wiki.WikiController; +import org.labkey.wiki.WikiController.PrintBranchAction; import org.labkey.wiki.WikiSelectManager; import java.util.Map; @@ -51,8 +53,7 @@ public abstract class BaseWikiView extends JspView public ActionURL manageURL; public ActionURL customizeURL; public ActionURL printURL; - - protected WikiVersion wikiVersion = null; // TODO: Used internally only? Pass to init()? + public ActionURL printBranchURL; protected int _webPartId = 0; @@ -62,7 +63,7 @@ protected BaseWikiView() } - protected void init(Container c, String name) + protected void init(Container c, String name, WikiVersion wikiVersion) { ViewContext context = getViewContext(); User user = context.getUser(); @@ -190,9 +191,15 @@ else if (folderHasWikis) } } - if (null == context.getRequest().getParameter(ActionURL.Param._print.name())) + if (null == context.getRequestOrThrow().getParameter(ActionURL.Param._print.name())) { printURL = wiki.getPageURL().addParameter(ActionURL.Param._print, 1); + // Must have update in the container, not just owner update on the wiki, since this is a folder-wide, + // potentially expensive operation. GitHub Issue #1415 + if (wiki.hasChildren() && c.hasPermission(user, UpdatePermission.class)) + { + printBranchURL = new ActionURL(PrintBranchAction.class, getContextContainer()).addParameter("name", wiki.getName()); + } } // Initialize Custom Menus @@ -273,9 +280,8 @@ private NavTree initNavMenu() NavTree print = new NavTree("Print", printURL); print.setNoFollow(true); menu.addChild(print); - if (wiki.hasChildren()) - menu.addChild("Print Branch", new ActionURL(WikiController.PrintBranchAction.class, - getContextContainer()).addParameter("name", wiki.getName())); + if (null != printBranchURL) + menu.addChild("Print Branch", printBranchURL); } } else if (!(isEmbedded() && getFrame() == WebPartView.FrameType.NONE)) diff --git a/wiki/src/org/labkey/wiki/model/WikiView.java b/wiki/src/org/labkey/wiki/model/WikiView.java index ce4d4d6caeb..9733fe3c915 100644 --- a/wiki/src/org/labkey/wiki/model/WikiView.java +++ b/wiki/src/org/labkey/wiki/model/WikiView.java @@ -16,21 +16,15 @@ package org.labkey.wiki.model; -/** - * User: adam - * Date: Aug 11, 2007 - * Time: 3:30:42 PM - */ public class WikiView extends BaseWikiView { - public WikiView(Wiki wiki, WikiVersion wikiversion, boolean hasContent) + public WikiView(Wiki wiki, WikiVersion wikiVersion, boolean hasContent) { super(); this.wiki = wiki; - this.wikiVersion = wikiversion; this.hasContent = hasContent; - init(getViewContext().getContainer(), wiki.getName()); + init(getViewContext().getContainer(), wiki.getName(), wikiVersion); // For the webpart version, see sibling class WikiWebPart setIsWebPart(false); diff --git a/wiki/src/org/labkey/wiki/model/WikiWebPart.java b/wiki/src/org/labkey/wiki/model/WikiWebPart.java index 6463f2fcff6..968153f8cc6 100644 --- a/wiki/src/org/labkey/wiki/model/WikiWebPart.java +++ b/wiki/src/org/labkey/wiki/model/WikiWebPart.java @@ -37,7 +37,7 @@ public WikiWebPart(int webPartId, Map props) String name = props.get("name"); name = (name != null) ? name : "default"; - init(c, name); + init(c, name, null); // display edit pencil in frameless webpart setShowFloatingCustomBtn(true);