test(core): Add chain-pattern order-sensitive match test#150
Open
misonijnik wants to merge 6 commits into
Open
test(core): Add chain-pattern order-sensitive match test#150misonijnik wants to merge 6 commits into
misonijnik wants to merge 6 commits into
Conversation
A multi-statement sink pattern that uses
`HttpRequest.newBuilder().uri($URL)` as a literal sub-expression in
the let-binding `$BUILDER = ...uri(...)` does NOT match a chained
source like
HttpRequest req = HttpRequest.newBuilder()
.uri(URI.create(t))
.GET()
.build();
client.send(req);
even though `HttpRequest.newBuilder()` is literally the receiver of
`.uri(...)` in the source. Replacing the literal static-method
receiver with `$_` (`$BUILDER = $_.uri($URL)`) matches the same
shape correctly — that's the workaround currently used in the
production `ssrf-sinks.yaml` `java-ssrf-sink` block (see the
`Builder-chain inline forms` comment). The literal form ought to
match the same source but doesn't.
Stubs (`issues/iChain/`) mirror the JDK `java.net.http.HttpRequest`
+ `java.net.URI` + `java.net.http.HttpClient` API surface needed
for the chain; the rule and test reference only the stub classes so
the reproducer doesn't depend on JDK classpath presence.
The test fails with:
Expected [PositiveCase(className=issues.issueChain$PositiveTaint)]
to be positive, but no vulnerability was found.
(constructor call `new okhttp3.Request.Builder().url(x)` works as a
literal in the same SSRF rule, so the mismatch is specific to a
static-method call in the receiver slot of the next chained call.)
…erpart Reframe the issueChain reproducer: the literal nested-receiver sink (`$BUILDER = newBuilder().uri($URL)` then `$BUILDER.build()`) not matching the fluent chain `newBuilder().uri(...).GET().build()` is INTENDED behaviour, not an analyzer false-negative. The calls are matched one at a time, so the order/structure of calls in the rule has to line up with the source; the intervening `.GET()` means `newBuilder().uri(...)` is never the direct receiver of `.build()`, so the nested literal receiver never binds. Drop the inaccurate docs that claimed a `$_.uri($URL)` form is "used in production ssrf-sinks.yaml (Builder-chain inline forms)" — that block does not exist in ssrf-sinks.yaml. Add issueChainSplitBuilder: same source, but bind `newBuilder()` in its own pattern-inside (`$NEW_BUILDER = ...newBuilder();`) and then match `$BUILDER = $NEW_BUILDER.uri($URL)`, matching the chain call-by-call. This fires (test passes), showing the correct way to express the intent. The issueChain test stays enabled/red as a documented reproducer.
The issueChain sample's tainted fluent-chain case was named
PositiveTaint,
so the sample harness (SampleData/SampleBasedTest) expected a finding.
The
chain-pattern rule legitimately cannot match that shape — the nested
static
receiver never binds (see the class Javadoc) — so the `issue
chain-pattern
literal static receiver` test was deliberately failing ("red").
Reclassify it as an expected non-detection by renaming the class to
NegativeTaintIntendedNonMatch (distinct from the existing literal-only
NegativeTaint case), which the harness treats as no-finding-expected.
The
test now passes. Update the now-inaccurate "expected to be red" comments
in
issueChain.java and IssuesTest.kt to match.
Remove the long block comments that documented the chain-pattern non-match across the issueChain / issueChainSplitBuilder samples, their rule yamls, and the IssuesTest entries. The self-documenting class names (NegativeTaintIntendedNonMatch, issueChainSplitBuilder) plus a short inline note now carry the intent; also drop the now-orphaned "see class Javadoc" reference left over after the Javadoc removal. No functional change — comments only.
The `issue chain-pattern literal static receiver` test now records an expected non-detection (the NegativeTaintIntendedNonMatch sample), so rename it to `issue chain-pattern literal static receiver (intended non-match)` to make that intent clear. Test body unchanged.
The reproducer's real point is that sub-pattern order affects
reachability:
the code creates the builder first and the URL second
(newBuilder().uri(URI.create(t))), while the rule binds $URL =
URI.create(...)
first (pattern-inside) and then newBuilder().uri($URL). Because the
rule's
sub-pattern order doesn't line up with the code's call order, the sink
is
unreachable and the rule does not fire — so it's recorded as a Negative
sample. (issueChainSplitBuilder reorders the rule so the order lines
up.)
Rename accordingly to describe the cause rather than the symptom:
- test: `issue chain-pattern literal static receiver (intended
non-match)`
-> `issue chain-pattern order-sensitive match`
- class: NegativeTaintIntendedNonMatch -> NegativeTaintOrderSensitive
- rule message updated to mention the order mismatch / unreachable sink
misonijnik
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.