Repository navigation
olevba: don't describe _Click handlers as running on document open - #896
Open
qianeric-backup wants to merge 1 commit into
Open
qianeric-backup wants to merge 1 commit into
qianeric-backup wants to merge 1 commit into
Conversation
AUTOEXEC_KEYWORDS_REGEX puts every ActiveX/control event in one group described as "Runs when the file is opened and ActiveX objects trigger events", including \w+_Click. That description is wrong for _Click: CommandButton1_Click fires when the user clicks the control and at no other time, so a document whose only macro entry point is a button was reported as having code that runs on open. Some of the other events in the group can plausibly fire while a control is initialised or rendered during load (_GotFocus, _LostFocus, _Resize, _Layout), so the wording is not wrong for them. Only _Click is split out. Detection is unchanged - \w+_Click still matches and is still reported as AutoExec, as introduced in decalage2#499. Only the description differs. The exact wording comes from the issue's suggestion; happy to reword. Test: tests/olevba/test_autoexec.py calls detect_autoexec directly and checks that CommandButton1_Click gets the interaction description (and that it contains no "file is opened" marker), that TextBox1_GotFocus and Workbook_Open keep their on-open descriptions, and that a document containing both a click and a GotFocus handler reports the two separately. Two of the five cases fail on master, all five pass with the change; tests/olevba/ as a whole gives 11 passed, 1 skipped. Note the maintainer may want to look at _Change and _FollowHyperlink in the same group, which are arguably interaction-only too - I left them alone to keep this change to what the issue asked for.
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
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.
What is wrong
AUTOEXEC_KEYWORDS_REGEXputs every ActiveX/control event into one group described as "Runs when the file is opened and ActiveX objects trigger events", and\w+_Clickis one of them:CommandButton1_Clickfires when the user clicks the control and at no other time. A document whose only macro entry point is a button therefore gets reported as having code that runs when the file is opened — and the wording gets carried downstream, into reports and summaries, as evidence of auto-execution when noWorkbook_Open/Auto_Open/Document_Openpath exists at all.Fixes #891.
What I changed
Split
\w+_Clickinto its own entry:Wording taken verbatim from your suggestion in the issue. Detection is unchanged —
\w+_Clickstill matches and is still reported as AutoExec, exactly as #499 introduced it. Only the description differs.Why only
_ClickThe rest of the group is not so clear-cut.
_GotFocus,_LostFocus,_Resizeand_Layoutcan plausibly fire while a control is initialised or rendered as a document loads, so "runs when the file is opened" is defensible for them._Clickis the one that unambiguously cannot.Two others in the group look interaction-only to me —
_Change(fires when the user edits a control's value) and_FollowHyperlink(fires on a link click) — but I left them alone rather than widening the diff beyond what the issue asked for. Say the word and I will split those too.Tests
tests/olevba/test_autoexec.py, five cases callingdetect_autoexecdirectly:CommandButton1_ClickCommandButton1_ClickTextBox1_GotFocusWorkbook_Open'Runs when the Excel Workbook is opened'The two failures on master are the description assertion and the combined-file assertion, both showing
'Runs when the file is opened and ActiveX objects trigger events'where the interaction text is expected.Aside
You noted in the issue that the autoexec patterns are hand-synced between
olevba's keyword table andmraptor'sre_autoexecregex. I checked:mraptoremits onlyA/W/Xflags and has no_Clickinre_autoexec, so it is untouched here. The drift risk is real but independent, as you said.