velestest: add optional position adjustment for group-based detectors#1777
velestest: add optional position adjustment for group-based detectors#17770xXA wants to merge 1 commit intogoogle:mainfrom
Conversation
AcceptDetector previously assumed that a Detector returns match positions
corresponding to the beginning of the full example string passed to the
acceptance test.
This assumption does not hold for detectors that use capture-group based
finders (e.g. ntuple.FindAllMatchesGroup). In those cases, the returned
Match.Start corresponds to the start of the captured secret value rather
than the beginning of the contextual match (e.g. client_id:).
As a result, acceptance tests would fail even though the detector was
behaving correctly.
This change introduces an optional AcceptDetector configuration:
WithPositionAdjust(func(example string) int)
When provided, the returned offset is added to the expected positions
before comparison. The default behavior remains unchanged, ensuring full
backward compatibility for existing detectors that rely on full-match
spans.
This makes AcceptDetector compatible with both:
- full-match based detectors
- capture-group based detectors
without modifying detector implementations or altering existing test
semantics.
Signed-off-by: Yuvraj Saxena <ysaxenax@gmail.com>
|
'cc @alessandro-Doyensec since he's been investigating failing acceptance tests for other detectors. |
Are you having issues with ntuples? Why didn't you inform me? |
The engineer who write the detector engine is out of office for the next few weeks so I can't ask him directly, but from the code it looks like the engine is using the returned positions and MaxSecretLen to figure out how which portions of the buffer should be given to detector plugins' Detect() calls to allow them to find the secret. That means that MaxSecretLen() and the returned positions should be about the whole match, not just the secret part of them. So I think we'd need to update the n-tuples plugin to behave that way rather than change the acceptance tests. The names MaxSecretLen and "returned secret positions" are a bit misleading in this case but I think these were initially developed without considering contextual matches use cases, but we can update the documentation to make it more clear that these fields are the length/position of the entire match, not just the secret parts of it. |
AFAIUI the issue was present in the original pair library that ntuples was derived from so it's likely not a new issue. |
Oh, thanks for informing. |
Are you talking about FindAllMatchesGroup or the core ntuple detector code ? |
|
I think this might mostly be a problem FindAllMatchesGroup function - I'm somewhat unsure though since I don't have as much context on how the pair/ntuples library works exactly. @alessandro-Doyensec pointed out the following about my #issuecomment-3914334889: @0xXA do you have an example where acceptance tests would today be failing with the current n-tuples library? |
2 participants
AcceptDetector previously assumed that a Detector returns match positions corresponding to the beginning of the full example string passed to the acceptance test.
This assumption does not hold for detectors that use capture-group based finders (e.g. ntuple.FindAllMatchesGroup). In those cases, the returned Match.Start corresponds to the start of the captured secret value rather than the beginning of the contextual match (e.g. client_id:).
As a result, acceptance tests would fail even though the detector was behaving correctly.
This change introduces an optional AcceptDetector configuration:
When provided, the returned offset is added to the expected positions before comparison. The default behavior remains unchanged, ensuring full backward compatibility for existing detectors that rely on full-match spans.
This makes AcceptDetector compatible with both:
without modifying detector implementations or altering existing test semantics.