Add folder summary for source report#278
Draft
JustinWonjaePark wants to merge 5 commits into
Draft
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a ChangesMerge-by-folder feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
JustinWonjaePark
force-pushed
the
dev_folder-summary
branch
2 times, most recently
from
a6f1d0d to
3d4abc6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/fosslight_source/cli.py (3)
94-94: ⚡ Quick winConsider using
defusedxmlfor safer XML parsing.While the XML being parsed comes from XLSX files generated by this tool, users could potentially process third-party XLSX files.
xml.etree.ElementTree.fromstringis vulnerable to XML External Entity (XXE) attacks. Consider usingdefusedxml.ElementTreeas a safer alternative.-import xml.etree.ElementTree as ET +import defusedxml.ElementTree as ETThis would require adding
defusedxmlto dependencies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fosslight_source/cli.py` at line 94, The code at the ET.fromstring call is vulnerable to XXE attacks when processing third-party XLSX files. Replace the import statement to use defusedxml.ElementTree instead of xml.etree.ElementTree, then update the ET.fromstring call to use the safer defusedxml variant. Additionally, add defusedxml to the project's dependencies in the appropriate configuration file (such as requirements.txt, setup.py, or pyproject.toml) to ensure the required package is available.Source: Linters/SAST tools
224-224: ⚡ Quick winFix implicit
Optionaltype hint.
kb_reference_result: list = Noneviolates PEP 484. When a parameter defaults toNone, the type hint should explicitly includeNone.- run_kb_msg: str = "", merge_by_folder: bool = False, kb_reference_result: list = None + run_kb_msg: str = "", merge_by_folder: bool = False, kb_reference_result: list | None = None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fosslight_source/cli.py` at line 224, The parameter kb_reference_result has a default value of None but the type hint only specifies list, which violates PEP 484. Update the type hint for kb_reference_result to explicitly indicate that None is a valid type by using either Optional[list] from the typing module or the union syntax list | None (for Python 3.10+), ensuring the type annotation matches the actual default value.Source: Linters/SAST tools
503-503: ⚡ Quick winRename
licenseto avoid shadowing Python builtin.The variable name
licenseshadows Python's built-inlicensefunction.- return tuple(sorted([license.strip() for license in scan_item.licenses if license and license.strip()])) + return tuple(sorted([lic.strip() for lic in scan_item.licenses if lic and lic.strip()]))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fosslight_source/cli.py` at line 503, The variable name `license` in the list comprehension at line 503 shadows Python's built-in `license` function. Rename this loop variable to a non-shadowing name such as `lic`, `license_item`, or `item` throughout the list comprehension that iterates over `scan_item.licenses`, ensuring all three occurrences of the variable name in the comprehension (the loop variable, and the two conditional checks) are updated consistently.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/fosslight_source/cli.py`:
- Line 94: The code at the ET.fromstring call is vulnerable to XXE attacks when
processing third-party XLSX files. Replace the import statement to use
defusedxml.ElementTree instead of xml.etree.ElementTree, then update the
ET.fromstring call to use the safer defusedxml variant. Additionally, add
defusedxml to the project's dependencies in the appropriate configuration file
(such as requirements.txt, setup.py, or pyproject.toml) to ensure the required
package is available.
- Line 224: The parameter kb_reference_result has a default value of None but
the type hint only specifies list, which violates PEP 484. Update the type hint
for kb_reference_result to explicitly indicate that None is a valid type by
using either Optional[list] from the typing module or the union syntax list |
None (for Python 3.10+), ensuring the type annotation matches the actual default
value.
- Line 503: The variable name `license` in the list comprehension at line 503
shadows Python's built-in `license` function. Rename this loop variable to a
non-shadowing name such as `lic`, `license_item`, or `item` throughout the list
comprehension that iterates over `scan_item.licenses`, ensuring all three
occurrences of the variable name in the comprehension (the loop variable, and
the two conditional checks) are updated consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f898ad6-59ec-46d3-a38d-bb384ed195f3
📒 Files selected for processing (2)
src/fosslight_source/_help.pysrc/fosslight_source/cli.py
Signed-off-by: Wonjae Park <j.wonjae.park@gmail.com>
Signed-off-by: Wonjae Park <j.wonjae.park@gmail.com>
JustinWonjaePark
force-pushed
the
dev_folder-summary
branch
from
6465d6d to
89e5adf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/fosslight_source/cli.py (1)
94-94: 💤 Low valueConsider using
defusedxmlfor XML parsing as defense-in-depth.While the workbook.xml is parsed from an xlsx file generated by this tool, using
defusedxml.ElementTree.fromstringinstead ofxml.etree.ElementTree.fromstringprovides protection against XML entity expansion attacks as a defense-in-depth measure.Suggested change
-import xml.etree.ElementTree as ET +import defusedxml.ElementTree as ET🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fosslight_source/cli.py` at line 94, Replace the standard xml.etree.ElementTree with defusedxml for secure XML parsing. Change the ET.fromstring call that parses workbook_xml to use defusedxml.ElementTree.fromstring instead. This requires importing defusedxml.ElementTree (or importing just ElementTree from defusedxml) and using that safe alternative for the XML parsing operation to protect against XML entity expansion attacks.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fosslight_source/cli.py`:
- Around line 502-503: The loop variable `license` in the list comprehension
within the _get_merge_licenses function shadows Python's built-in license()
function. Rename the loop variable from `license` to a different name (such as
`lic` or `item`) throughout the list comprehension to avoid shadowing the
built-in, ensuring all occurrences in the `[license.strip() for license in
scan_item.licenses if license and license.strip()]` expression are updated
consistently.
- Line 224: The type annotation for the kb_reference_result parameter uses an
implicit Optional by setting a default value of None without explicitly
declaring the type as Optional, which violates PEP 484. Import Optional from the
typing module (if not already imported) and change the kb_reference_result
parameter annotation from list to Optional[list] to make the parameter's
nullable nature explicit in the type hint.
---
Nitpick comments:
In `@src/fosslight_source/cli.py`:
- Line 94: Replace the standard xml.etree.ElementTree with defusedxml for secure
XML parsing. Change the ET.fromstring call that parses workbook_xml to use
defusedxml.ElementTree.fromstring instead. This requires importing
defusedxml.ElementTree (or importing just ElementTree from defusedxml) and using
that safe alternative for the XML parsing operation to protect against XML
entity expansion attacks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 64e72122-61e1-49fc-b7c3-7b7afbe7d676
📒 Files selected for processing (2)
src/fosslight_source/_help.pysrc/fosslight_source/cli.py
✅ Files skipped from review due to trivial changes (1)
- src/fosslight_source/_help.py
- Use defusedxml for safer XML parsing - Fix implicit Optional type hint in kb_reference_result - Rename 'license' to 'lic' to avoid shadowing built-in function Signed-off-by: Wonjae Park <j.wonjae.park@gmail.com>
Signed-off-by: Wonjae Park <j.wonjae.park@gmail.com>
JustinWonjaePark
force-pushed
the
dev_folder-summary
branch
from
fcf6d92 to
fb01374
Compare
…arate excluded files Signed-off-by: Wonjae Park <j.wonjae.park@gmail.com>
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
1 participant
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.
--no_mergeoption to keep source report output file-based when folder summary is not needed.SRC_FL_Source_before_mergesheet in XLSX reports to preserve the original pre-merge source rows.Summary by CodeRabbit
--no_mergeCLI option to disable automatic folder-based merging of source file paths in generated reports..xlsx.--no_mergebehavior.defusedxmlto project dependencies.