fix(velocity): harden SecureIntrospector restricted-class coverage

[nollymar]

Fixes dotCMS/private-issues#704

What

Extends the Velocity template sandbox's SecureIntrospector so method execution is denied on the file, IO, and network-resource type families. The introspector previously matched restricted classes and packages by exact string, so types outside the enumerated java.lang.* / reflection set — including java.io.File, java.nio.file.Path, IO streams, and java.net.URL/URI — were not covered.

Changes

• SecureIntrospectorImpl: adds a hierarchy-aware restricted-type check alongside the existing ClassLoader/Thread block. Matching by type hierarchy means concrete platform implementations (for example the JDK's internal Path type) are covered as well, which an exact-string list cannot reach. Covered families: File, Path, RandomAccessFile, InputStream, OutputStream, Reader, Writer, Channel, FileSystem, FileSystemProvider, URL, URI, plus the Files and Paths utility holders by identity.
• velocity.properties: documents the same network-resource and file-system utility types in the restricted-class list, alongside the existing entries. The code-level check is the enforcement floor.
• The existing Number/Boolean/String and Class.getName() fast-paths and all previously restricted classes are preserved unchanged.
• Progressive enhancement: @Override added to getMethod and checkObjectExecutePermission.

Testing

• Unit (SecureIntrospectorImplTest): asserts the file/IO/network-resource families (including a concrete platform Path implementation) are denied, the previously restricted reflection/system families stay denied, and the common value/collection types stay allowed. Also verifies the config list enumerates the exact-matchable resource types.
• Integration (SecureIntrospectorRenderTest, registered in MainSuite1b): renders templates through the real Velocity engine and confirms the guard is applied at render time, while ordinary value methods still render.

Both suites pass locally (Tests run: 5 unit, Tests run: 3 integration).

🤖 Generated with Claude Code

[claude]

Claude finished @nollymar's task in 2m 20s —— View job

---

Code Review — SecureIntrospector hardening

• Read PR diff (both commits: hierarchy floor + operator-configurable layer)
• Reviewed SecureIntrospectorImpl, velocity.properties, unit + integration tests
• Checked Config.getStringArrayProperty semantics and suite registration

The second commit (feat(velocity): make SecureIntrospector restricted packages/classes operator-configurable) directly addresses @wezell's and @sfreudenthaler's review feedback: it adds env-overridable velocity.introspector.restrict.packages (prefix deny), .restrict.classes (exact deny), and .allow.classes (carve-out), with denials logged via getMethod(). The full default-deny/whitelist flip wasn't taken (wezell noted it "could break some implementations"); the prefix-deny + allow-list gives operators the stricter stance without that risk. This is a reasonable resolution of the threads.

New Issues

No blocking issues found. The implementation is correct:

• Ordering is right — value-type fast-paths (Number/Boolean/String, Class.getName()) and the ClassLoader/Thread deny run before both the new hierarchy check and the config layer, so nothing is shadowed or weakened.
• The allow-list (isConfigRestricted, SecureIntrospectorImpl.java:256) only carves out of the configurable deny lists and is reached after the code-level floor, so it provably cannot rescue a statically-denied or resource-family type — matches the documented "never overrides the code-level floor" intent.
• configList (:234) tolerating an uninitialized Config via catch (Throwable) is justified for this vendored-package class so it stays usable in plain unit tests. Empty defaults mean zero behavior change unless configured.
• Config.getStringArrayProperty(prop, EMPTY) resolves to the (String, String[]) overload and is env-overridable — correct dotCMS convention.

Medium (non-blocking)

• 🟡 Medium: SecureIntrospectorImpl.java:295 — URL and URI are added to RESTRICTED_SUPERTYPES, so all method calls on any URL/URI object are now denied at render time, not just the file/IO ones. Assumption: some shipped or customer VTL may legitimately read from a URI/URL returned into the context (e.g. $x.getPath(), $uri.getQuery()). What to verify: confirm no core .vtl templates or common customer patterns resolve methods on URL/URI instances, since those will now silently return null and emit a Logger.warn. This is the intended hardening and reviewers accepted the direction — flagging only so the blast radius on existing templates is a conscious call. If concern is real, the new velocity.introspector.allow.classes gives operators an escape hatch, which is a fair mitigation.

Notes

• Perf question from @wezell is well-answered in the PR thread: the check runs only on introspection cache miss (once per distinct (class, method) per render context), and value types short-circuit before the new block. The added isConfigRestricted does call Config.getStringArrayProperty up to 3× per miss, but that too is off the hot path.
• Test coverage is solid: unit test pins the hierarchy floor (incl. a concrete platform Path), the regression guard for reflection/system families, the value-type allows, the config-sync assertion, and the new package-prefix + allow-list carve-out; integration test (SecureIntrospectorRenderTest, registered in MainSuite1b:48) proves the guard is wired into the real engine.

Nice work — this is a clean, well-tested hardening PR that also folds in the reviewer-requested configurability.
· issue-37508-velocity-introspector-hardening

[wezell]

Are there any performance implications of this change? Reflection is already Velocity's weakpoint. It would be interesting to do a before and after perf test.

[github-actions]

dotbot code review:

• Reviewer: ~z-ai/glm-latest (medium)
• Overall: patch is correct
• New findings this run: 0
• Prior unresolved dotbot findings still relevant: 0
• Active findings total: 0

The hierarchy-aware restricted-type check is correctly ordered after the existing allow fast-paths and the ClassLoader/Thread deny, so it neither weakens existing allows nor is shadowed. Tests cover unit-level denial, value-type allowances, config sync, and render-time wiring, and the new integration test is registered in MainSuite1b. No bugs introduced by this patch were identified.

Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads.

_{reviewed by dotbot · ~z-ai/glm-latest · medium}

[dotCMS-Machine-User]

✅ dotbot review: all reviewer models (meta/muse-spark-1.3, ~z-ai/glm-latest) agree — patch is correct.

_{approved automatically by dotbot}

[nollymar]

Good question @wezell — net impact is negligible, and I measured it.

Where the check runs. Method resolution goes through Velocity's per-context introspection cache (ClassUtils.getMethod → context.icacheGet / icachePut). checkObjectExecutePermission only runs on a cache miss — roughly once per distinct (class, method) per render context, not per invocation. A loop calling $item.getX() over N rows resolves once and is served from cache after that, so this isn't on the hot reflection path.

Cost of the check itself. The added coverage is a bounded set of isAssignableFrom calls plus two identity checks. The common value types (Number/Boolean/String, and Class.getName()) short-circuit on the existing fast-paths before reaching the new block, so they pay nothing extra.

Before/after microbenchmark of checkObjectExecutePermission (JDK 25, warmed up, best-of-N), measuring per-resolution cost:

• Mixed class workload (value types, ordinary collections, denied file/IO types): 21.2 ns → 17.7 ns — slightly faster, because denied file/IO types now return early instead of scanning the full string deny-list.
• Worst case, an ordinary allowed type (e.g. ArrayList) that runs the whole hierarchy scan and then the existing string loops: 27.6 ns → 34.2 ns, i.e. ~+6.6 ns per resolution.

So the worst case adds single-digit nanoseconds per distinct method resolution, paid once per render and cached after. A render resolving ~100 distinct signatures pays well under 1 µs total, one time — below the noise of parsing / I/O / DB.

Happy to run a full render-level benchmark end-to-end if you'd like to see it, but at the introspection level the delta is in the nanoseconds and partly negative.

[wezell]

why not go deeper here and include other packages?

java.nio
java.net

Also, can we configure (env var) packages to include or exclude? that would be handy and would allow us to push the fix deeper to a more white listed stance. We update these deny lists all the time as new holes are discovered.

[sfreudenthaler]

My opinion. Switch it to an allow list and add the default deny on the fall through.

[wezell]

I think that could break some implementations but that is where I was suggesting to move, as long as it is configurable and we log the deny message

[nollymar]

Good call — made it configurable. Pushed a follow-up that adds an operator-configurable layer on top of the code-level (type-hierarchy) floor, read via dotCMS Config so it's env-overridable and updatable without a code change:

• velocity.introspector.restrict.packages (env DOT_VELOCITY_INTROSPECTOR_RESTRICT_PACKAGES) — comma-separated package prefixes to deny, so java.nio,java.net also covers java.nio.file, java.net.http, etc.
• velocity.introspector.restrict.classes (env DOT_VELOCITY_INTROSPECTOR_RESTRICT_CLASSES) — exact class names.
• velocity.introspector.allow.classes (env DOT_VELOCITY_INTROSPECTOR_ALLOW_CLASSES) — carve specific classes back out of the configurable denial.

This gives us the deeper, whitelist-leaning stance you're after: set the restrict packages to java.nio,java.net (and beyond) per environment, and exempt the few classes a template legitimately needs via the allow-list. Denials are already logged in getMethod() ("... due to security restrictions").

On your point that broadening could break implementations — agreed, which is exactly why I left the defaults empty rather than denying all of java.nio/java.net out of the box: a blanket default would break common template usage (InetAddress, URLEncoder, ByteBuffer, charsets, etc.). So the aggressive stance is opt-in per environment, and the hardcoded type-hierarchy floor (File/Path/streams/URL/URI + concrete impls) stays as the always-on baseline that config can extend but not weaken.

One deliberate boundary: the allow-list carves classes out of the configurable layer only — it can't re-open the hardcoded floor or the existing reflection/system denials, so a config change can't accidentally undo the core fix. Easy to revisit if you'd rather it be fully override-able.

Covered by a unit test (package-prefix denial + allow-list carve-out). Happy to seed a stricter default set (e.g. deny java.nio/java.net with a curated allow-list) if you'd prefer that shipped on by default.

[wezell]

nice.

[wezell]

nice change

[github-actions]

dotbot code review:

• Reviewer: meta/muse-spark-1.3 (medium)
• Overall: patch is correct
• New findings this run: 0
• Prior unresolved dotbot findings still relevant: 0
• Active findings total: 0

Hierarchy-aware deny is correctly ordered after existing allows and covers concrete subtypes, with Files/Paths handled by identity. Config layer is additive-only with empty defaults and cannot weaken the code-level floor.

Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads.

_{reviewed by dotbot · meta/muse-spark-1.3 · medium}

[github-actions]

⚪ [P3] SecureIntrospectorImpl.java:241 swallows all Throwables when reading Config

Current code:

        catch (final Throwable t)
        {
            return EMPTY;
        }

Problem: Catching Throwable without logging silently disables the operator deny layer if Config misbehaves (e.g. system-table lookup throws).

Fix:

        catch (final Exception e)
        {
            Logger.debug(SecureIntrospectorImpl.class,
                    "Unable to read Velocity introspector config " + prop + ": " + e.getMessage());
            return EMPTY;
        }

[github-actions]

⚪ [P3] SecureIntrospectorImpl.java:294 blanket IO/URL deny also hits context-provided writers and URLs

Current code:

            OutputStream.class, Reader.class, Writer.class, Channel.class,
            FileSystem.class, FileSystemProvider.class, URL.class, URI.class

Problem: Deny is by supertype, so every method on any Writer/Reader/URL/URI reachable from the context is now inert — e.g. $response.getWriter().println(...) or $url.getPath() — not just the file/IO methods.

Assumption: customer VTL may call such methods.
What to verify: grep customer templates for getWriter, getInputStream, getPath()/getQuery() on URL/URI values; the new velocity.introspector.allow.classes is the escape hatch if so.

[github-actions]

dotbot code review:

• Reviewer: deepseek/deepseek-v4.1-flash (medium)
• Overall: patch is incorrect
• New findings this run: 2
• Prior unresolved dotbot findings still relevant: 0
• Active findings total: 2

The hierarchy-aware deny is correctly ordered after the allow fast-paths and the ClassLoader/Thread deny, so it only broadens denials; the configurable layer defaults to empty and can never weaken the code-level floor. No provable P0/P1 defect was found; the two items raised are low-priority robustness/compatibility concerns.

Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads.

_{reviewed by dotbot · deepseek/deepseek-v4.1-flash · medium}