Codemod Lxml parser defaults #61
Conversation
src/codemodder/registry.py
Outdated
| @@ -152,7 +152,7 @@ def match_codemods( | |||
| def load_registered_codemods() -> CodemodRegistry: | |||
| registry = CodemodRegistry() | |||
| logger.info("Loading registered codemod collections") | |||
| for entry_point in entry_points()["codemods"]: | |||
| for entry_point in entry_points().select(group="codemods"): | |||
There was a problem hiding this comment.
Fixed a DeprecationWarning. See https://importlib-metadata.readthedocs.io/en/latest/using.html#entry-points
There was a problem hiding this comment.
Not sure whether you ran into this problem with the change but I think the right way to fix this is probably by adding some conditional logic based on Python version.
There was a problem hiding this comment.
I ended up removing this change (sorry forgot to remove my comment) bc yeah py versions. We can leave as is for now.
clavedeluna
force-pushed
the
lxml-parser-defaults
branch
2 times, most recently
from
7c189c9 to
20af62a
Compare
| NAME = "safe-lxml-parser-defaults" | ||
| REVIEW_GUIDANCE = ReviewGuidance.MERGE_AFTER_CURSORY_REVIEW | ||
| SUMMARY = "Enable all security checks in `lxml.etree.XMLParser` call." | ||
| DESCRIPTION = "...........TODO" |
There was a problem hiding this comment.
Right now this actually gets used as the CHANGE_DESCRIPTION for the simplified API (we should fix that). But we do need to make sure it's present.
There was a problem hiding this comment.
woops total oversight here that's why the TODO. How verbose can I be, should I be? Is this used as the PR description when pixeebot makes the PR?
There was a problem hiding this comment.
Right now (and again we need to fix this) DESCRIPTION -> CHANGE_DESCRIPTION for the simple API. CHANGE_DESCRIPTION should be a very simple description of the individual change itself. So in this case probably something like "Replace lxml parser parameters with safe defaults". In contrast the SUMMARY becomes the title of the PR that gets opened so it should be very brief and descriptive.
| class LxmlSafeParserDefaults(SemgrepCodemod): | ||
| NAME = "safe-lxml-parser-defaults" | ||
| REVIEW_GUIDANCE = ReviewGuidance.MERGE_AFTER_CURSORY_REVIEW | ||
| SUMMARY = "Enable all security checks in `lxml.etree.XMLParser` call." |
There was a problem hiding this comment.
Sorry should have mentioned this before, but this becomes the PR title. I would suggest this could be even briefer, like "Use safe defaults for lxml parsers" (and without a trailing period).
clavedeluna
force-pushed
the
lxml-parser-defaults
branch
from
c294b1c to
6ac3925
Compare
| new_args = self.replace_args( | ||
| original_node, | ||
| [ | ||
| NewArg(name="resolve_entities", value="False", add_if_missing=True), |
There was a problem hiding this comment.
I like this, I think it makes sense.
Overview
New codemod for lxml classes that have security-related parameters
Description
replace_argtoreplace_argsso now we can replace a list of args. The method api acceptsargs_info, which we expect to be a list of the namedtupleNewArg.