Skip to content

Commit b4bb293

Browse files
Krishcalinclaude
andcommitted
Take the ABAP call graph across the whole tree, bounded by ABAP's own visibility
The per-artefact graph answered nothing about class-based code. Measured on sample_data/abap_src, every method parameter came back `no_caller` — a class's callers live in other files by construction, and class-based ABAP is most modern ABAP. So the graph now spans the scanned tree. SEEING THE TREE IS NOT PERMISSION TO CLEAR A METHOD, and that is the part worth reading. Who may call a procedure is decided by the language: FORM file-local in every codebase anyone writes PRIVATE only from inside its class — one artefact holds every caller PROTECTED its class and its subclasses, which need the whole tree PUBLIC anything that imports the class, including code never exported FUNCTION another system entirely, if it carries the RFC flag `by_public_literal` and `priv_literal` in the new fixture are the pair that makes this a rule rather than a convenience: identical evidence — every call in the whole tree passes a literal — and only the private one is downgraded. RESOLUTION HAD TO BECOME CLASS-QUALIFIED FIRST. `run`, `execute` and `get_data` are each defined dozens of times in a real custom-code base; resolving a call by bare name across a tree would make almost every method ambiguous, and the tree graph would have answered LESS than the per-artefact one it replaced. Receivers are typed from TYPE REF TO, NEW, me-> and zcl_x=>. A receiver that cannot be typed leaves the call unqualified, and an unqualified call may add taint to every method of that name and may never clear any of them. Local variable types are per artefact and never carried between files: `lo_worker` in two files is two variables, and carrying a type across would resolve one file's call against another file's declaration. THE TRACE POINTED AT THE WRONG FILE. A cross-file call step carrying only "line 20" sends the reader to line 20 of the artefact they are already reading, which is an unrelated statement. The step now carries the caller's file, the caller's own statement, and the variable the caller actually passed — `iv_user_input`, not the callee's `iv_where`, which does not appear on that line. The console ignored the new key and would have reproduced exactly that misdirection on screen, so FindingDetail renders it. Across every ABAP fixture, over the two commits: 11 confirmed / 0 tentative became 17 / 4, with 7 traces crossing a call boundary and 1 reaching into another artefact. The tree pass costs about 16% on a 120-file synthetic tree. Still not interprocedural data-flow analysis, and docs/CVA_MERGE_PLAN.md's "not claim interprocedural analysis" still holds: no fixpoint, and a non-literal actual is treated as tainted rather than proven to be. It decides where the walk starts; it does not trace a value through a procedure. Eight mutations against the backend tests and two against the frontend one. All ten fail them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 5f1e5b9 commit b4bb293

11 files changed

Lines changed: 964 additions & 114 deletions

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -717,7 +717,7 @@ wrong in that way.
717717
| `grcac` | grc_access_control | **GRC Access Control**: EAM/Firefighter usage+ownership, ARM access-request workflow, GRC-native SoD violations, mitigating controls, SoD ruleset governance |
718718
| `rolegov` | role_governance | **role design**: SU24 proposal hygiene for custom tcodes, ungenerated profiles (AGR_1016), derived-role authorization-value drift vs parent |
719719
| `atc` | atc_import | SAP's own ATC/CVA results, ingested rather than re-derived |
720-
| `cva` | abap_sast | **our** ABAP/CDS/BDEF scanner — **135 rules dispatched by file type** (ABAP/CDS/RAP, JS/UI5 and BTP descriptors — the split is in `modules/abap_sast_rules.py`, which is the only place worth counting), statement lexer, taint refinement with intra-artefact call-graph awareness (`modules/abap_callgraph.py` decides whether a procedure parameter is caller-controlled; it is NOT interprocedural data-flow and must not be described as such). `ABAP-XSS-006` is retired and `ABAP-AUTH-003` is handled in the engine, so 116 of the 118 fire from the rule table |
720+
| `cva` | abap_sast | **our** ABAP/CDS/BDEF scanner — **135 rules dispatched by file type** (ABAP/CDS/RAP, JS/UI5 and BTP descriptors — the split is in `modules/abap_sast_rules.py`, which is the only place worth counting), statement lexer, taint refinement with call-graph awareness across the scanned tree (`modules/abap_callgraph.py` decides whether a procedure parameter is caller-controlled, and what ABAP's own visibility rules allow us to conclude; it is NOT interprocedural data-flow and must not be described as such). `ABAP-XSS-006` is retired and `ABAP-AUTH-003` is handled in the engine, so 116 of the 118 fire from the rule table |
721721
| `logreview` | log_review | retrospective SM20 review: what the audit log actually recorded |
722722
| `capxsuaa` | cap_xsuaa | **CAP project as written** (`--cap-src`): `xs-security.json` exactly + CDS model lexically. Traces scope ← role-template ← role-collection ← IdP group; `CAPX-TOK-001` closes the application-override blind spot `BTP-TOK-*` declares |
723723
| `codeinv` | code_inventory_report | custom-code estate: size by type, unreachable, dormant, unknown-kept-separate |
Lines changed: 191 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,191 @@
1+
/**
2+
* "How untrusted input reaches it" — the taint trace on a code finding.
3+
*
4+
* THE STEP THAT CAN LIE. Since the ABAP call graph spans the scanned tree, the
5+
* call that feeds a method's parameter is usually in ANOTHER artefact: a class's
6+
* callers live in other files by construction. The backend marks such a step
7+
* with the file it came from.
8+
*
9+
* Rendered as line/role/var/statement alone, that step reads "line 20" — and the
10+
* reader looks at line 20 of the file they are already looking at, which is an
11+
* unrelated statement about something else. The row would be pointing at the
12+
* wrong code while looking exactly as authoritative as the rows around it.
13+
*
14+
* A step with no `file` is in the finding's own artefact, and must stay clean:
15+
* most traces in the product are entirely local, and stamping a filename on
16+
* every row of those is noise.
17+
*/
18+
import { render, screen } from '@testing-library/react'
19+
import { MemoryRouter } from 'react-router'
20+
import { beforeEach, describe, expect, it, vi } from 'vitest'
21+
22+
const finding = vi.fn()
23+
const findingHistory = vi.fn()
24+
const serviceRequest = vi.fn()
25+
26+
vi.mock('../api/client', () => ({
27+
finding: (...a: unknown[]) => finding(...a),
28+
findingHistory: (...a: unknown[]) => findingHistory(...a),
29+
serviceRequest: (...a: unknown[]) => serviceRequest(...a),
30+
setFindingState: vi.fn(),
31+
assignFinding: vi.fn(),
32+
ApiError: class ApiError extends Error {
33+
status: number
34+
constructor(status: number, message: string) { super(message); this.status = status }
35+
},
36+
}))
37+
vi.mock('../lib/title', () => ({ useTitle: () => {} }))
38+
vi.mock('../lib/session', () => ({
39+
useSession: () => ({ user: { username: 't', role: 'admin', can_write: true } }),
40+
}))
41+
vi.mock('react-router', async () => {
42+
const actual = await vi.importActual<typeof import('react-router')>('react-router')
43+
return { ...actual, useParams: () => ({ id: '1' }) }
44+
})
45+
46+
import { FindingDetail } from './FindingDetail'
47+
import type { FindingDetail as Finding } from '../api/types'
48+
49+
/** modules/abap_sast.py on tests/fixtures/abap_tree, verbatim: the SQL injection
50+
* in zcl_tree_worker~by_public_tainted, whose caller is in the other file. */
51+
const FLOW = [
52+
{
53+
line: 20,
54+
role: 'call',
55+
var: 'iv_user_input',
56+
code: 'lo_worker->by_public_tainted( iv_where = iv_user_input )',
57+
file: 'zcl_tree_caller.clas.abap',
58+
},
59+
{ line: 35, role: 'source', var: 'iv_where', code: 'METHOD by_public_tainted' },
60+
{
61+
line: 36,
62+
role: 'sink',
63+
var: 'iv_where',
64+
code: 'SELECT * FROM sflight INTO TABLE @DATA(lt_a) WHERE (iv_where)',
65+
},
66+
]
67+
68+
const FINDING = {
69+
id: 1001,
70+
landscape_id: 5,
71+
system_id: null,
72+
fingerprint: 'a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6',
73+
check_id: 'ABAP-SQLI-001',
74+
client: '100',
75+
fingerprint_basis: 'objects',
76+
scope: 'object',
77+
subject: [],
78+
severity: 'CRITICAL',
79+
priority_tier: null,
80+
priority_score: null,
81+
priority_factors: [],
82+
priority_rationale: null,
83+
state: 'open',
84+
remediation_owner: 'customer_fixable',
85+
assignee: null,
86+
owning_team: null,
87+
due_date: null,
88+
provider_ticket_ref: null,
89+
first_seen_run: null,
90+
last_seen_run: null,
91+
first_seen_at: '2026-09-01T00:00:00Z',
92+
last_detected_at: '2026-09-01T00:00:00Z',
93+
resolved_at: null,
94+
regression_count: 0,
95+
accepted_by: null,
96+
acceptance_reason: null,
97+
acceptance_from: null,
98+
acceptance_due: null,
99+
false_positive_reason: null,
100+
transitioned_by: null,
101+
last_transition_at: null,
102+
sla_started_at: null,
103+
taint_confidence: 'confirmed',
104+
reachability: null,
105+
title: 'Dynamic WHERE clause built from caller input',
106+
category: null,
107+
default_team: null,
108+
baseline_req_id: null,
109+
sid: 'PRD',
110+
system_client: null,
111+
system_tier: null,
112+
platform: null,
113+
external_key: null,
114+
system_label: null,
115+
expired_acceptance: false,
116+
is_overdue: false,
117+
days_open: 0,
118+
latest_evidence: null,
119+
risk_narrative: null,
120+
remediation: null,
121+
references_json: [],
122+
responsibility: null,
123+
cwe: 'CWE-89',
124+
tier: 'prod',
125+
deployment_mode: 'rise_pce',
126+
latest_details: {},
127+
} as unknown as Finding
128+
129+
async function draw(flow: unknown) {
130+
finding.mockResolvedValue({
131+
...(FINDING as unknown as Record<string, unknown>),
132+
latest_details: { source: 'abap_scan', taint_flow: flow, confidence: 'confirmed' },
133+
})
134+
findingHistory.mockResolvedValue({ history: [], observations: [] })
135+
serviceRequest.mockResolvedValue({})
136+
render(<MemoryRouter><FindingDetail /></MemoryRouter>)
137+
await screen.findByText('Dynamic WHERE clause built from caller input')
138+
}
139+
140+
/** The row a step renders into, found by its statement text. */
141+
function rowFor(code: string): HTMLElement {
142+
const cell = screen.getByText(code)
143+
const row = cell.closest('tr')
144+
if (!row) throw new Error('no row for ' + code)
145+
return row as HTMLElement
146+
}
147+
148+
beforeEach(() => { vi.clearAllMocks() })
149+
150+
describe('the taint trace', () => {
151+
it('renders every step', async () => {
152+
await draw(FLOW)
153+
expect(screen.getByText(/How untrusted input reaches it/)).toBeTruthy()
154+
expect(screen.getByText('METHOD by_public_tainted')).toBeTruthy()
155+
expect(screen.getByText(/lo_worker->by_public_tainted/)).toBeTruthy()
156+
})
157+
158+
it('names the artefact a cross-file step came from', async () => {
159+
await draw(FLOW)
160+
const row = rowFor('lo_worker->by_public_tainted( iv_where = iv_user_input )')
161+
expect(row.textContent).toContain('zcl_tree_caller.clas.abap')
162+
expect(row.textContent).toContain('20')
163+
})
164+
165+
it('leaves a step in the finding’s own file unmarked', async () => {
166+
await draw(FLOW)
167+
const row = rowFor('METHOD by_public_tainted')
168+
expect(row.textContent).not.toContain('.clas.abap')
169+
expect(row.textContent).not.toContain('.prog.abap')
170+
})
171+
172+
it('shows the variable the caller actually passed', async () => {
173+
await draw(FLOW)
174+
const row = rowFor('lo_worker->by_public_tainted( iv_where = iv_user_input )')
175+
// `iv_user_input`, not `iv_where`: the callee's parameter name is not on
176+
// the caller's line, and naming it there sends the reader looking for a
177+
// variable that is not in the statement they are shown.
178+
expect(row.textContent).toContain('iv_user_input')
179+
})
180+
181+
it('renders a wholly local trace without any filename', async () => {
182+
await draw(FLOW.slice(1))
183+
const table = screen.getByText('METHOD by_public_tainted').closest('table')
184+
expect(table?.textContent ?? '').not.toContain('.abap')
185+
})
186+
187+
it('renders nothing when there is no trace', async () => {
188+
await draw([])
189+
expect(screen.queryByText(/How untrusted input reaches it/)).toBeNull()
190+
})
191+
})

frontend/src/routes/FindingDetail.tsx

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -392,9 +392,18 @@ export function FindingDetail() {
392392
{arr(details['taint_flow']).map((raw, i) => {
393393
const hop = obj(raw) ?? {}
394394
const role = str(hop['role'])
395+
// A `file` appears only on a step that is in a DIFFERENT
396+
// artefact from the finding — a call reaching in from
397+
// another class. Without it the row reads "line 20", and
398+
// the reader looks at line 20 of the file they are
399+
// already in, which is an unrelated statement.
400+
const file = str(hop['file'])
395401
return (
396402
<tr key={i} className="hover:bg-panel2">
397-
<td className={`${TD} font-mono text-[12px]`}>{str(hop['line'])}</td>
403+
<td className={`${TD} font-mono text-[12px]`}>
404+
{file && <div className="text-ink3 break-all">{file}</div>}
405+
{str(hop['line'])}
406+
</td>
398407
<td className={TD}>
399408
{role === 'source' ? <span className="pill sev-HIGH">source</span>
400409
: role === 'sink' ? <span className="pill sev-CRITICAL">sink</span>

0 commit comments

Comments
 (0)