Skip to content

Cross language compatibility: case insensitivity flag#552

Merged
mkienow-r7 merged 2 commits intorapid7:mainfrom
TomSellers:regex_flag_and_misc_cleanup
Mar 3, 2023
Merged

Cross language compatibility: case insensitivity flag#552
mkienow-r7 merged 2 commits intorapid7:mainfrom
TomSellers:regex_flag_and_misc_cleanup

Conversation

@TomSellers
Copy link
Copy Markdown
Contributor

@TomSellers TomSellers commented Mar 2, 2023

Description

This PR moves the inline flag (?i) (case insensitivity) to the beginning of the regex. In Python this flags has global meaning and use of it anywhere other than the start of the pattern is deprecated. This does not change the function of this flag in current patterns for other languages.

See previous work in #367

This was caught by internal testing in recog-go and the fixes are required in order for us to implement and test the changes from the following PRs into recog-go:

How Has This Been Tested?

bundle exec rake tests
ruby bin/recog_verify xml/*.xml | grep -v WARN | grep -v SUMMARY

Types of changes>

  • Content

Checklist:

  • I have updated the documentation accordingly (or changes are not required).
  • I have added tests to cover my changes (or new tests are not required).
  • All new and existing tests passed.

Comment thread xml/imap_banners.xml
<example>Dovecot (Debian) ready.</example>
<example>[CAPABILITY IMAP4rev1 SASL-IR LOGIN-REFERRALS ID ENABLE IDLE LITERAL+ AUTH=PLAIN AUTH=LOGIN] Dovecot (Debian) ready.</example>
<param pos="0" name="service.vendor" value="Dovecot"/>
<param pos="0" name="service.vendor" value="Dovecot"/>
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not flag related, but duplicate param use detected by tests in recog-go.

@mkienow-r7
Copy link
Copy Markdown
Contributor

This was caught by internal testing in recog-go ...

The hope behind the Go Verify workflow was to detect issues between the various recog language implementation. What sort of internal testing discovered this issue that was not discoverable when verifying the fingerprints against their examples via cmd/recog_verify/main.go?

Comment thread xml/dhcp_vendor_class.xml Outdated
-->

<fingerprint pattern="^[Mm]fg=(?:Fuji)?(?i:Xerox);[Tt]yp=(?:MFP|AIO|[Pp]rinter);[Mm]od=(?:Xerox )?(\S+) ([a-zA-Z0-9]+).*;[Ss]er=([A-Z0-9]{9,10})(?:;[Ll]oc=.*)?$">
<fingerprint pattern="(?i)^[Mm]fg=(?:Fuji)?Xerox;[Tt]yp=(?:MFP|AIO|[Pp]rinter);[Mm]od=(?:Xerox )?(\S+) ([a-zA-Z0-9]+).*;[Ss]er=([A-Z0-9]{9,10})(?:;[Ll]oc=.*)?$">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With case insensitivity applied to the whole pattern the explicit case matching is no longer necessary.

  • [Mm]
  • [Tt]
  • [Pp]
  • [Ss]
  • [Ll]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed.

@TomSellers
Copy link
Copy Markdown
Contributor Author

TomSellers commented Mar 2, 2023

@mkienow-r7

The hope behind the Go Verify workflow was to detect issues between the various recog language implementation. What sort of internal testing discovered this issue that was not discoverable when verifying the fingerprints against their examples via cmd/recog_verify/main.go?

Testing in the Go version of recog_verify should be the same as the ruby version so that we can test consistently.

The test for this condition is in fingerprints_test.go which would similar to the spec tests.

https://github.com/runZeroInc/recog-go/blob/1f868cde9c4b2e110f99d1114d1df4e530842f90/fingerprints_test.go#L51-L58

EDIT: I'll see about adding checks to the Ruby and Go versions recog-verify to avoid XML being Ok with one implementation and not another.

Copy link
Copy Markdown
Contributor

@mkienow-r7 mkienow-r7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution @TomSellers!

@mkienow-r7 mkienow-r7 merged commit 8356f74 into rapid7:main Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants